<feature>[zbs]: ZSV-12666 support whole-VM backup - #4587
Closed
ZStack-Robot wants to merge 1 commit into
Closed
Conversation
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in:32 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 (13)
Walkthrough新增加密卷备份恢复标记及其消息契约，并将该标记从虚拟机卷实例化流程传递至外部主存储下载流程。加密场景增加准备、加密卷创建和专用下载分支，普通场景保留原有下载路径。 Changes外部主存储加密镜像下载
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ExternalPrimaryStorage
participant CryptoService
participant PrimaryStorage
ExternalPrimaryStorage->>CryptoService: 发送加密下载准备消息
CryptoService-->>ExternalPrimaryStorage: 返回准备结果
ExternalPrimaryStorage->>PrimaryStorage: 创建加密卷
ExternalPrimaryStorage->>PrimaryStorage: 执行加密镜像下载
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
zstack-robot-2
commented
Jul 16, 2026
Collaborator
Comment from yaohua.wu: Review: MR !10557 — ZSV-12666Background (preserved across rounds)
Findings
关联 MR
Coverage
Verdict: REVISION_REQUIRED本仓实现未发现独立阻塞项；联合交付被 zstack-store !1158 的 FUSE 并发安全问题阻塞。 🤖 Robot Reviewer |
MatheMatrixforce-pushed
the
sync/zstackio/zbs-volume-encryption-backup@@4
branch
from
July 16, 2026 11:02
54d7d7b to
06d28e4CompareThere 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)
1305-1345: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift镜像缓存命中条件要区分加密状态 现在只按
primaryStorageUuid + imageUuid复用缓存，而downloadImageTo(..., targetEncrypted, ...)会生成明文/加密两种不同结果；ImageCacheVO也没有加密状态字段，后续加密请求可能直接命中明文缓存。建议把加密状态纳入缓存标识，或命中时重建缓存。🤖 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 1305 - 1345, Update doDownloadImageCache so cache reuse distinguishes targetEncrypted state instead of matching only primaryStorageUuid and imageUuid. Persist and query an encryption-state identifier for ImageCacheVO, or invalidate and rebuild when the existing cache’s state differs, ensuring encrypted requests never reuse plaintext caches and vice versa.
🤖 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 1305-1345: Update doDownloadImageCache so cache reuse
distinguishes targetEncrypted state instead of matching only primaryStorageUuid
and imageUuid. Persist and query an encryption-state identifier for
ImageCacheVO, or invalidate and rebuild when the existing cache’s state differs,
ensuring encrypted requests never reuse plaintext caches and vice versa.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5187dd2b-955c-4f15-9202-75d2d4a90323
📒 Files selected for processing (7)
header/src/main/java/org/zstack/header/core/encrypt/CryptoServiceConstant.javaheader/src/main/java/org/zstack/header/storage/addon/primary/DownloadExternalPrimaryStorageEncryptedImageMsg.javaheader/src/main/java/org/zstack/header/storage/addon/primary/PrepareExternalPrimaryStorageEncryptedImageDownloadMsg.javaheader/src/main/java/org/zstack/header/storage/addon/primary/PrepareExternalPrimaryStorageEncryptedImageDownloadReply.javaheader/src/main/java/org/zstack/header/storage/primary/DownloadDataVolumeToPrimaryStorageMsg.javastorage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.javastorage/src/main/java/org/zstack/storage/volume/VolumeManagerImpl.java
MatheMatrixforce-pushed
the
sync/zstackio/zbs-volume-encryption-backup@@4
branch
from
July 21, 2026 04:38
c3e96f7 to
9904223CompareThere was a problem hiding this comment.
Actionable comments posted: 1
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)
1270-1346: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftImageCacheVO 缓存命中逻辑未区分加密恢复意图，可能导致明文/加密缓存被跨场景误用。
doDownloadImageCache的缓存复用判断（1312-1320行）仅按(primaryStorageUuid, imageUuid)查找已存在的ImageCacheVO，命中后直接completion.success(...)返回，完全没有考虑本次调用的encryptedVolumeBackupRestore参数。而
downloadImageTo在encryptedVolumeBackupRestore=true时会显式设置spec.setEncrypted(true)并绑定加密密钥（1399-1404行），走全新的prepare-encrypted-image-download/download-image-for-encryption分支产生加密格式的缓存；encryptedVolumeBackupRestore=false时则走普通下载产生明文缓存。这两种缓存对应的磁盘格式是不同的。由于缓存查找 key 不包含加密意图，同一个
imageUuid在同一主存储上：
- 若先以普通方式恢复过（生成明文缓存），之后有 VM 以"加密卷备份恢复"方式恢复同一镜像时，会直接复用明文缓存并跳过加密准备/下载分支，最终
createRootVolume会用明文缓存 clone 到spec.setEncrypted(true)的目标卷——卷被数据库标记为加密，但实际数据仍是明文（正是本文件其他位置代码注释特意提醒要规避的场景）。- 反过来，若先加密恢复过，之后普通恢复同一镜像会误用加密缓存 clone 到未加密的目标卷，可能导致克隆失败或产生不可读的卷。
这与 PR 描述的核心场景（同一份 plain 备份按需恢复为加密 ZBS 盘）直接相关，并非边缘 case。建议让缓存查找/持久化 key 也携带加密语义（例如为加密恢复单独打标或使用独立的 install path/记录），避免跨加密语境复用缓存。
🔒 可能的修复方向
private void doDownloadImageCache(ImageInventory image, boolean encryptedVolumeBackupRestore, ReturnValueCompletion<ImageCacheInventory> completion) { CreateVolumeSpec spec = new CreateVolumeSpec(); spec.setUuid(image.getUuid()); spec.setName(buildImageName(image.getUuid())); ImageCacheVO cache = Q.New(ImageCacheVO.class) .eq(ImageCacheVO_.primaryStorageUuid, self.getUuid()) .eq(ImageCacheVO_.imageUuid, image.getUuid()) + .eq(ImageCacheVO_.encrypted, encryptedVolumeBackupRestore) // 需要为ImageCacheVO增加加密维度字段 .find(); if (cache != null) { completion.success(ImageCacheInventory.valueOf(cache)); 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 1270 - 1346, Update doDownloadImageCache so cache lookup and persistence distinguish encryptedVolumeBackupRestore from ordinary restores, preventing an ImageCacheVO created for one mode from being reused in the other. Add or reuse an ImageCacheVO field/key representing the encryption intent, include it in the existing primary-storage/image lookup, and set it when creating the cache while preserving the encrypted download behavior in downloadImageTo.
🤖 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.
Inline comments:
In `@storage/src/main/java/org/zstack/storage/volume/VolumeManagerImpl.java`:
- Line 456: 更新 VolumeManagerImpl 中 requiresEncryptInPlace() 的判断逻辑，将
msg.isEncryptedVolumeBackupRestore() 纳入跳过原地加密的条件。保留现有
download-image-for-encryption 分支及普通加密卷的处理行为，确保 encryptedVolumeBackupRestore 不再触发
VolumeInPlaceEncryptor。
---
Outside diff comments:
In
`@storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java`:
- Around line 1270-1346: Update doDownloadImageCache so cache lookup and
persistence distinguish encryptedVolumeBackupRestore from ordinary restores,
preventing an ImageCacheVO created for one mode from being reused in the other.
Add or reuse an ImageCacheVO field/key representing the encryption intent,
include it in the existing primary-storage/image lookup, and set it when
creating the cache while preserving the encrypted download behavior in
downloadImageTo.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e909f6b9-8623-4166-b897-19bbcbfaef48
📒 Files selected for processing (13)
compute/src/main/java/org/zstack/compute/vm/VmInstantiateOtherDiskFlow.javaheader/src/main/java/org/zstack/header/core/encrypt/CryptoServiceConstant.javaheader/src/main/java/org/zstack/header/storage/addon/primary/DownloadExternalPrimaryStorageEncryptedImageMsg.javaheader/src/main/java/org/zstack/header/storage/addon/primary/PrepareExternalPrimaryStorageEncryptedImageDownloadMsg.javaheader/src/main/java/org/zstack/header/storage/primary/DownloadDataVolumeToPrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/storage/primary/InstantiateRootVolumeFromTemplateOnPrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/volume/CreateDataVolumeFromVolumeTemplateMsg.javaheader/src/main/java/org/zstack/header/volume/InstantiateRootVolumeMsg.javastorage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.javastorage/src/main/java/org/zstack/storage/volume/InstantiateVolumeForNewCreatedVmExtension.javastorage/src/main/java/org/zstack/storage/volume/InstantiateVolumeForVmChangeImageExtension.javastorage/src/main/java/org/zstack/storage/volume/VolumeBase.javastorage/src/main/java/org/zstack/storage/volume/VolumeManagerImpl.java
🚧 Files skipped from review as they are similar to previous changes (2)
- header/src/main/java/org/zstack/header/core/encrypt/CryptoServiceConstant.java
- header/src/main/java/org/zstack/header/storage/addon/primary/DownloadExternalPrimaryStorageEncryptedImageMsg.java
Uh oh!
There was an error while loading. Please reload this page.
Route encrypted volume/VM backup restore through the keyed download path in ExternalPrimaryStorage instead of the keyless ImageStore convert. - Add backup download hooks and route encrypted backup download through the crypto service. - Create the encrypted image volume with the sealed DEK reference. - Compute encryptedVolumeBackupRestore in InstantiateVolumeForVmChangeImageExtension by mirroring the parent InstantiateVolumeForNewCreatedVmExtension pattern (rootDisk.isVolumeBackupTemplate() && encrypted), fixing revert which otherwise failed with "encrypt.key-secret is required for cipher". Resolves: ZSV-12666 Change-Id: I131c5ee9b700560baa130d298aaf523d1702808b
MatheMatrixforce-pushed
the
sync/zstackio/zbs-volume-encryption-backup@@4
branch
from
July 21, 2026 05:06
9904223 to
f27ae34Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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-12666
sync from gitlab !10557