<feature>[zbs]: ZSV-12579 support whole-VM backup#4558
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
Walkthrough新增外部主存储镜像下载上下文和扩展点接口，并将扩展选择、前置处理、加密下载及回滚逻辑接入 Changes外部主存储镜像下载
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ExternalPrimaryStorage
participant DownloadExtension
participant VolumeController
ExternalPrimaryStorage->>DownloadExtension: beforeDownload(...)
ExternalPrimaryStorage->>VolumeController: createVolume(...)
ExternalPrimaryStorage->>DownloadExtension: supportsDownload(context)
ExternalPrimaryStorage->>DownloadExtension: download(context, completion)
ExternalPrimaryStorage->>DownloadExtension: rollbackDownload(context)
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
MatheMatrix
force-pushed
the
sync/zstackio/zbs-volume-encryption-backup@@2
branch
7 times, most recently
from
783e073 to
937c249
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java (1)
1378-1401: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win扩展异常会阻断 rollback 中的资源清理。
rollback()先无条件遍历所有ExternalPrimaryStorageImageDownloadExtensionPoint调用rollbackDownload(context),再判断volume == null执行directDeleteBits。若任一扩展的rollbackDownload抛出未捕获异常,循环会中断,导致后面的 volume 清理逻辑永远不会执行,造成主存储上卷资源泄漏。建议将扩展通知包裹在异常隔离中(逐个 try/catch 并记录日志),确保核心清理逻辑始终执行。
🛡️ 建议修复
public void rollback(FlowRollback trigger, Map data) { ExternalPrimaryStorageImageDownloadContext context = buildDownloadContext(); context.setVolume(volume); context.setRemoteTarget(remoteTarget); for (ExternalPrimaryStorageImageDownloadExtensionPoint ext : pluginRgty.getExtensionList(ExternalPrimaryStorageImageDownloadExtensionPoint.class)) { - ext.rollbackDownload(context); + try { + ext.rollbackDownload(context); + } catch (Throwable t) { + logger.warn(String.format("rollbackDownload failed for extension[%s]", ext.getClass().getName()), t); + } } if (volume == null) { trigger.rollback(); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java` around lines 1378 - 1401, Update the rollback method to isolate each rollbackDownload call in its own try/catch, log any extension exception, and continue notifying remaining extensions. Ensure the existing volume null check and directDeleteBits cleanup always execute regardless of extension failures.
🧹 Nitpick comments (1)
header/src/main/java/org/zstack/header/storage/addon/primary/ExternalPrimaryStorageImageDownloadExtensionPoint.java (1)
6-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win接口方法缺少 Javadoc 注释。
beforeDownload/supportsDownload/download/rollbackDownload均未添加 Javadoc,不利于扩展点实现者理解各生命周期方法的调用时机、参数含义及回调契约(例如rollbackDownload是否保证只在download被调用后才触发)。As per path instructions: "接口方法不应有多余的修饰符（例如
public），且必须配有有效的 Javadoc 注释。"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@header/src/main/java/org/zstack/header/storage/addon/primary/ExternalPrimaryStorageImageDownloadExtensionPoint.java` around lines 6 - 15, 为接口 ExternalPrimaryStorageImageDownloadExtensionPoint 的 beforeDownload、supportsDownload、download 和 rollbackDownload 添加有效 Javadoc，说明各生命周期方法的调用时机、参数含义及 Completion 回调契约，明确 rollbackDownload 是否仅在 download 调用后触发；同时保持接口方法无多余的 public 等修饰符。Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java`:
- Around line 1378-1401: Update the rollback method to isolate each
rollbackDownload call in its own try/catch, log any extension exception, and
continue notifying remaining extensions. Ensure the existing volume null check
and directDeleteBits cleanup always execute regardless of extension failures.
---
Nitpick comments:
In
`@header/src/main/java/org/zstack/header/storage/addon/primary/ExternalPrimaryStorageImageDownloadExtensionPoint.java`:
- Around line 6-15: 为接口 ExternalPrimaryStorageImageDownloadExtensionPoint 的
beforeDownload、supportsDownload、download 和 rollbackDownload 添加有效
Javadoc，说明各生命周期方法的调用时机、参数含义及 Completion 回调契约，明确 rollbackDownload 是否仅在 download
调用后触发；同时保持接口方法无多余的 public 等修饰符。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e8b81ac8-8ffb-400f-9188-096a051e6bed
📒 Files selected for processing (3)
header/src/main/java/org/zstack/header/storage/addon/primary/ExternalPrimaryStorageImageDownloadContext.javaheader/src/main/java/org/zstack/header/storage/addon/primary/ExternalPrimaryStorageImageDownloadExtensionPoint.javastorage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java
Add external primary storage download context and rollback hook. Use the encrypted download flow to own extension cleanup. Tests: git diff --cached --check. Resolves: ZSV-12666 Change-Id: I7a0f3c2d91e4b58a6c01f92d3e84a6b77c9d0e12
MatheMatrix
force-pushed
the
sync/zstackio/zbs-volume-encryption-backup@@2
branch
from
937c249 to
2777526
Compare
1 participant
Summary
Support ZBS whole-VM backup restore on the existing VolumeBackup/ImageStore flow. Plain backup disks can be restored as encrypted ZBS disks through temporary cache conversion; the backup encryption property is not changed.
Changes
Testing
Resolves: ZSV-12579
sync from gitlab !10522