<feature>[storage]: ZSV-12577 support backup NBD conversion - #4616
Closed
ZStack-Robot wants to merge 0 commit into
Closed
Conversation
Walkthrough新增备份加密转换扩展机制，主存储响应返回执行主机标识； Changes卷加密转换
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant VolumeBase
participant PrimaryStorage
participant ExtensionHelper
participant Database
VolumeBase->>PrimaryStorage: 转换卷加密
PrimaryStorage-->>VolumeBase: 返回 hostUuid 与 actualSizes
VolumeBase->>ExtensionHelper: prepare 备份加密转换
VolumeBase->>Database: 提交卷加密转换
Database-->>VolumeBase: 返回提交结果
VolumeBase->>ExtensionHelper: cleanupCommitted
VolumeBase->>PrimaryStorage: 批量删除转换后的加密位
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)plugin/ceph/src/main/java/org/zstack/storage/ceph/primary/CephPrimaryStorageBase.javaast-grep timed out on this file plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageBase.javaast-grep retry budget exhausted before isolating this batch plugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageKVMBackend.javaast-grep retry budget exhausted before isolating this batch
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There 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/volume/VolumeBase.java (1)
3803-3843: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift在 DB 事务内部同步提交备份加密转换扩展，中途失败会导致 DB 与扩展侧状态不一致，且可能对已提交的扩展重复触发回滚。
volumeBackupEncryptionConversionExtensionHelper.commit(backupConversionContexts)是在SQLBatch.scripts()（即同一个 DB 事务）内部同步调用的。由于 helper 的commit()没有逐项异常隔离：
- 若中间某个扩展的
commitVolumeBackupEncryptionConversion抛异常，SQLBatch 事务整体回滚（撤销卷/快照的 install path、encrypted 等 DB 变更），但在此之前已成功提交的扩展的外部副作用（通常发生在事务之外）不会被撤销，出现"DB 回退、扩展副作用已生效"的不一致。- 该异常还会使本 flow 失败，触发 ShareFlow 回滚级联，进而对
backupConversionContexts（此时仍是完整列表，包含刚提交成功的Context）调用helper.rollback(...)，等价于对已经 commit 成功的扩展再执行一次 rollback，存在破坏已落地备份转换结果的风险。- 若扩展的 commit 实现涉及跨主机/跨存储的外部 I/O（如 PR 目标中的 ImageStore NBD 链导出），在 DB 事务内同步执行还会延长事务持有时间。
建议：将
commit()移出 DB 事务边界（例如在 DB 更新成功后单独调用，并在其内部对每个扩展做 try/catch 后再判断整体成功与否），或至少让commit()内部具备与rollback/cleanupCommitted一致的逐项异常隔离，避免部分提交导致的状态不一致与重复回滚。🤖 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/volume/VolumeBase.java` around lines 3803 - 3843, Move volumeBackupEncryptionConversionExtensionHelper.commit(backupConversionContexts) out of SQLBatch.scripts() so it runs only after the database transaction successfully completes. Preserve the existing DB updates and key-provider handling inside the transaction, and ensure commit failures are handled through the helper’s per-extension isolation/cleanup behavior rather than causing rollback of already committed contexts.
🧹 Nitpick comments (1)
header/src/main/java/org/zstack/header/volume/VolumeBackupEncryptionConversionExtensionPoint.java (1)
5-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win为扩展点方法补充 Javadoc，并明确布尔参数含义。
当前四个生命周期回调均无有效 Javadoc；同时
encrypted无法区分源状态与目标状态，建议统一命名为targetEncrypted，并在文档中说明各阶段的调用时序、失败语义及回滚边界。As per path instructions，接口方法必须配有有效 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/volume/VolumeBackupEncryptionConversionExtensionPoint.java` around lines 5 - 13, 为 VolumeBackupEncryptionConversionExtensionPoint 的四个生命周期方法补充有效 Javadoc，说明 prepare、commit、rollback、after 的调用时序、失败语义及回滚边界。将 prepareVolumeBackupEncryptionConversion 的布尔参数 encrypted 统一重命名为 targetEncrypted，并在文档中明确其表示目标加密状态；同步更新参数说明及所有实现和调用方，保持接口语义一致。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.
Inline comments:
In
`@storage/src/main/java/org/zstack/storage/encrypt/VolumeBackupEncryptionConversionExtensionHelper.java`:
- Around line 60-64: Update commit in VolumeBackupEncryptionConversionHelper to
isolate failures for each Context, matching rollback and cleanupCommitted: catch
RuntimeException from context.extension.commitVolumeBackupEncryptionConversion,
prevent it from aborting iteration or propagating to the caller, and continue
committing all remaining contexts.
---
Outside diff comments:
In `@storage/src/main/java/org/zstack/storage/volume/VolumeBase.java`:
- Around line 3803-3843: Move
volumeBackupEncryptionConversionExtensionHelper.commit(backupConversionContexts)
out of SQLBatch.scripts() so it runs only after the database transaction
successfully completes. Preserve the existing DB updates and key-provider
handling inside the transaction, and ensure commit failures are handled through
the helper’s per-extension isolation/cleanup behavior rather than causing
rollback of already committed contexts.
---
Nitpick comments:
In
`@header/src/main/java/org/zstack/header/volume/VolumeBackupEncryptionConversionExtensionPoint.java`:
- Around line 5-13: 为 VolumeBackupEncryptionConversionExtensionPoint
的四个生命周期方法补充有效 Javadoc，说明 prepare、commit、rollback、after 的调用时序、失败语义及回滚边界。将
prepareVolumeBackupEncryptionConversion 的布尔参数 encrypted 统一重命名为
targetEncrypted，并在文档中明确其表示目标加密状态；同步更新参数说明及所有实现和调用方，保持接口语义一致。
🪄 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: 9d40209c-eba5-417a-94e8-d9676ea29da3
⛔ Files ignored due to path filters (1)
conf/springConfigXml/VolumeManager.xmlis excluded by!**/*.xml
📒 Files selected for processing (8)
header/src/main/java/org/zstack/header/storage/primary/ConvertVolumeEncryptionOnPrimaryStorageReply.javaheader/src/main/java/org/zstack/header/volume/VolumeBackupEncryptionConversionContext.javaheader/src/main/java/org/zstack/header/volume/VolumeBackupEncryptionConversionExtensionPoint.javaplugin/ceph/src/main/java/org/zstack/storage/ceph/primary/CephPrimaryStorageBase.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageBase.javaplugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageKVMBackend.javastorage/src/main/java/org/zstack/storage/encrypt/VolumeBackupEncryptionConversionExtensionHelper.javastorage/src/main/java/org/zstack/storage/volume/VolumeBase.java
Comment on lines
+60
to
+64
| public void commit(List<Context> contexts) { | ||
| for (Context context : safeContexts(contexts)) { | ||
| context.extension.commitVolumeBackupEncryptionConversion(context.context); | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
commit() 未对每个扩展做异常隔离，与 rollback/cleanupCommitted 的防御性风格不一致。
一旦中间某个 Context.extension.commitVolumeBackupEncryptionConversion 抛出 RuntimeException，后续 Context 均不会被 commit，且异常会直接向上抛出到调用方（VolumeBase.updateVolumeEncryptionConversionInDb 是在 SQLBatch.scripts() 内调用的，详见该文件对应评论，二者共享同一根因，已在 consolidated_comments 中合并说明）。
🤖 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/encrypt/VolumeBackupEncryptionConversionExtensionHelper.java`
around lines 60 - 64, Update commit in VolumeBackupEncryptionConversionHelper to
isolate failures for each Context, matching rollback and cleanupCommitted: catch
RuntimeException from context.extension.commitVolumeBackupEncryptionConversion,
prevent it from aborting iteration or propagating to the caller, and continue
committing all remaining contexts.
MatheMatrix
force-pushed
the
sync/zstackio/dev/encrypted-volume-backup-nbd-conversion@@4
branch
from
July 21, 2026 04:31
e30a30e to
3bcefad
Compare
Owner
|
Comment from yaohua.wu: Review: MR !10590 — ZSV-12577Background (preserved across rounds)
本 MR Findings当前 5 个变更文件中未发现可发布的仓内高置信 finding。扩展 prepare → SQLBatch commit → rollback/cleanup 的接入顺序与当前框架事务语义一致。 跨仓阻塞项
关联 MR
Coverage
Verdict: REVISION_REQUIRED本仓接入本身未发现阻塞问题，但 @@4 组依赖的 store P0 会破坏转换输入，四仓不能在该问题修复前共同合入。 🤖 Robot Reviewer |
Collaborator
Author
|
Comment on Comment from jin.ma: 这是 finish 还是 cleanup 为啥不看返回 |
Owner
|
Comment on Comment from jin.ma: 放到 extension point 里 |
Collaborator
Author
|
Comment on Comment from jin.ma: 加TODO 移走， |
Owner
|
Comment on Comment from jin.ma: 按实际业务语义拆，不要 true/false 控制逻辑 |
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 free
to 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
Use ImageStore NBD chain exports for linked volume backup encryption conversion without staging complete backup files on KVM local disks.
Changes
Testing
Resolves: ZSV-12577
sync from gitlab !10590