<feature>[storage]: ZSV-12664 support ZBS volume encryption conversion#4571
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 5 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 ignored due to path filters (1)
📒 Files selected for processing (16)
Walkthrough新增 ZBS/CBD 卷加密双向转换流程,涵盖接口契约、目标卷生命周期、KVM 转换、密钥材料准备、CBD 路径处理、错误清理及端到端测试。 ChangesZBS 卷加密转换
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant API
participant ExternalPrimaryStorage
participant ZbsStorageController
participant ZbsVolumeEncryptionConverter
participant KVMHost
API->>ExternalPrimaryStorage: 提交卷加密转换
ExternalPrimaryStorage->>ZbsStorageController: 转发转换消息
ZbsStorageController->>ZbsVolumeEncryptionConverter: 执行转换
ZbsVolumeEncryptionConverter->>KVMHost: 发送LUKS转换命令
KVMHost-->>ZbsVolumeEncryptionConverter: 返回actualSize或转换结果
ZbsVolumeEncryptionConverter-->>API: 返回转换回复
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
header/src/main/java/org/zstack/header/storage/addon/primary/ZbsVolumeEncryptionBackend.java (1)
24-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win请补充接口 Javadoc,并用枚举替代
targetEncrypted。这些方法需要说明路径所有权、创建结果和清理回调语义;建议将布尔参数替换为
ENCRYPTED/PLAIN枚举,避免调用方传反。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/storage/addon/primary/ZbsVolumeEncryptionBackend.java` around lines 24 - 29, 为 ZbsVolumeEncryptionBackend 接口的 validateConversionPaths、createConversionTarget 和 deleteConversionTarget 补充有效 Javadoc,说明路径所有权、创建结果及清理回调语义;将 createConversionTarget 的 targetEncrypted 布尔参数替换为表达 ENCRYPTED/PLAIN 的枚举类型,并同步更新相关调用方以使用该枚举。Source: Path instructions
storage/src/main/java/org/zstack/storage/encrypt/ZbsVolumeEncryptionMaterialFactory.java (1)
46-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win缓存密钥资源字段并将
kpUuid改为完整名称。建议使用
keyResourceType、keyResourceUuid和keyProviderUuid局部变量,避免重复读取spec并提升这段密钥选择逻辑的可审计性。As per path instructions, “不允许使用不必要的缩写”,命名应使用完整单词表达意图。
🤖 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/ZbsVolumeEncryptionMaterialFactory.java` around lines 46 - 86, 在 prepareVolumeEncryption 中先将 spec 的加密资源类型、资源 UUID 和密钥提供者 UUID 缓存到 keyResourceType、keyResourceUuid、keyProviderUuid 局部变量,后续校验、上下文设置及错误信息统一使用这些变量;同时将 kpUuid 重命名为完整的 keyProviderUuid,避免不必要缩写并减少重复读取 spec。Source: Path instructions
test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsVolumeEncryptionCase.groovy (1)
216-233: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win不要依赖缺失的 KVM handler 让密钥复用测试失败。
该用例当前以
outcome.error != null为预期,实际只证明材料准备发生在后续失败之前。请显式模拟 KVM 成功并断言命令中的encryptedDek与成功结果;或者直接测试材料工厂并重命名用例。🤖 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 `@test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsVolumeEncryptionCase.groovy` around lines 216 - 233, 更新 ZbsVolumeEncryptionExtension.createEncryptedEmptyVolume 用例,显式 stub KVM handler 返回成功,避免通过后续缺失 handler 的失败来验证材料复用;在 success 回调中断言结果及命令中的 encryptedDek,并相应移除 outcome.error 非空断言,保留必要的材料与删除路径验证。
🤖 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
`@plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsVolumeEncryptionBackendImpl.java`:
- Around line 200-261: Update the path-mismatch cleanup in
createConversionTarget so cleanupInstallPath uses createdInstallPath, the actual
path returned by ZBS, instead of targetInstallPath. Preserve the existing error
reporting and completion behavior while ensuring deleteConversionTarget removes
the created orphan volume.
---
Nitpick comments:
In
`@header/src/main/java/org/zstack/header/storage/addon/primary/ZbsVolumeEncryptionBackend.java`:
- Around line 24-29: 为 ZbsVolumeEncryptionBackend 接口的
validateConversionPaths、createConversionTarget 和 deleteConversionTarget 补充有效
Javadoc,说明路径所有权、创建结果及清理回调语义;将 createConversionTarget 的 targetEncrypted 布尔参数替换为表达
ENCRYPTED/PLAIN 的枚举类型,并同步更新相关调用方以使用该枚举。
In
`@storage/src/main/java/org/zstack/storage/encrypt/ZbsVolumeEncryptionMaterialFactory.java`:
- Around line 46-86: 在 prepareVolumeEncryption 中先将 spec 的加密资源类型、资源 UUID 和密钥提供者
UUID 缓存到 keyResourceType、keyResourceUuid、keyProviderUuid
局部变量,后续校验、上下文设置及错误信息统一使用这些变量;同时将 kpUuid 重命名为完整的 keyProviderUuid,避免不必要缩写并减少重复读取
spec。
In
`@test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsVolumeEncryptionCase.groovy`:
- Around line 216-233: 更新
ZbsVolumeEncryptionExtension.createEncryptedEmptyVolume 用例,显式 stub KVM handler
返回成功,避免通过后续缺失 handler 的失败来验证材料复用;在 success 回调中断言结果及命令中的 encryptedDek,并相应移除
outcome.error 非空断言,保留必要的材料与删除路径验证。
🪄 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: 970dc29e-d67b-42cb-99b2-f982048cc1ae
⛔ Files ignored due to path filters (1)
conf/springConfigXml/VolumeManager.xmlis excluded by!**/*.xml
📒 Files selected for processing (16)
header/src/main/java/org/zstack/header/storage/addon/primary/CreateVolumeSpec.javaheader/src/main/java/org/zstack/header/storage/addon/primary/PrimaryStorageControllerSvc.javaheader/src/main/java/org/zstack/header/storage/addon/primary/ZbsVolumeEncryptionBackend.javaheader/src/main/java/org/zstack/header/storage/addon/primary/ZbsVolumeEncryptionExtensionPoint.javaplugin/expon/src/main/java/org/zstack/expon/ExponStorageController.javaplugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.javaplugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsVolumeEncryptionBackendImpl.javastorage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.javastorage/src/main/java/org/zstack/storage/encrypt/ZbsEncryptedEmptyVolumeCreator.javastorage/src/main/java/org/zstack/storage/encrypt/ZbsVolumeEncryptionCommands.javastorage/src/main/java/org/zstack/storage/encrypt/ZbsVolumeEncryptionConstants.javastorage/src/main/java/org/zstack/storage/encrypt/ZbsVolumeEncryptionConverter.javastorage/src/main/java/org/zstack/storage/encrypt/ZbsVolumeEncryptionExtension.javastorage/src/main/java/org/zstack/storage/encrypt/ZbsVolumeEncryptionMaterialFactory.javastorage/src/main/java/org/zstack/storage/volume/VolumeBase.javatest/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsVolumeEncryptionCase.groovy
| @Override | ||
| public void createConversionTarget(String targetInstallPath, long virtualSize, boolean targetEncrypted, | ||
| ReturnValueCompletion<String> completion) { | ||
| CbdPath target; | ||
| try { | ||
| target = parseConversionCbdPath(targetInstallPath); | ||
| } catch (OperationFailureException e) { | ||
| completion.fail(e.getErrorCode()); | ||
| return; | ||
| } | ||
|
|
||
| ZbsStorageController.CreateVolumeCmd cmd = new ZbsStorageController.CreateVolumeCmd(); | ||
| cmd.setLogicalPool(target.logicalPool); | ||
| cmd.setVolume(target.volume); | ||
| cmd.setUnit(getSizeUnit(addonInfo.getClusterInfo().getVersion())); | ||
| long allocatedSize = targetEncrypted ? luksBackingSize(virtualSize) : virtualSize; | ||
| cmd.setSize(alignSizeTo(allocatedSize, cmd.getUnit())); | ||
| cmd.setSkipIfExisting(false); | ||
|
|
||
| controller.httpCall(ZbsStorageController.CREATE_VOLUME_PATH, cmd, ZbsStorageController.CreateVolumeRsp.class, | ||
| new ReturnValueCompletion<ZbsStorageController.CreateVolumeRsp>(completion) { | ||
| @Override | ||
| public void success(ZbsStorageController.CreateVolumeRsp returnValue) { | ||
| String createdInstallPath = returnValue.getInstallPath(); | ||
| if (targetInstallPath.equals(createdInstallPath)) { | ||
| completion.success(createdInstallPath); | ||
| return; | ||
| } | ||
|
|
||
| ErrorCode error = operr( | ||
| "ZBS volume encryption conversion target[%s] was created at unexpected path[%s]", | ||
| targetInstallPath, createdInstallPath); | ||
| String cleanupInstallPath = targetInstallPath; | ||
| try { | ||
| deleteConversionTarget(cleanupInstallPath, new Completion(completion) { | ||
| @Override | ||
| public void success() { | ||
| completion.fail(error); | ||
| } | ||
|
|
||
| @Override | ||
| public void fail(ErrorCode cleanupError) { | ||
| logger.warn(String.format( | ||
| "failed to cleanup unexpected ZBS conversion target[installPath:%s] after create path mismatch: %s", | ||
| cleanupInstallPath, cleanupError)); | ||
| completion.fail(error); | ||
| } | ||
| }); | ||
| } catch (Exception e) { | ||
| logger.warn(String.format( | ||
| "failed to cleanup unexpected ZBS conversion target[installPath:%s] after create path mismatch: %s", | ||
| cleanupInstallPath, e.getMessage())); | ||
| completion.fail(error); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public void fail(ErrorCode errorCode) { | ||
| completion.fail(errorCode); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
路径不一致时清理了错误的路径,导致泄漏卷无法被回收。
当创建目标卷成功但返回的 createdInstallPath 与请求的 targetInstallPath 不一致时,cleanupInstallPath 被赋值为 targetInstallPath(该路径实际从未被创建),而真正被 ZBS 创建出来的卷位于 createdInstallPath。对 targetInstallPath 执行删除无法清理真正泄漏的卷,会在 ZBS 侧留下一个无主卷;且由于创建时 skipIfExisting=false,后续对同一转换目标的重试还会因路径已存在而持续失败。
🐛 建议修复:清理实际创建出来的路径
ErrorCode error = operr(
"ZBS volume encryption conversion target[%s] was created at unexpected path[%s]",
targetInstallPath, createdInstallPath);
- String cleanupInstallPath = targetInstallPath;
+ String cleanupInstallPath = createdInstallPath;
try {
deleteConversionTarget(cleanupInstallPath, new Completion(completion) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| public void createConversionTarget(String targetInstallPath, long virtualSize, boolean targetEncrypted, | |
| ReturnValueCompletion<String> completion) { | |
| CbdPath target; | |
| try { | |
| target = parseConversionCbdPath(targetInstallPath); | |
| } catch (OperationFailureException e) { | |
| completion.fail(e.getErrorCode()); | |
| return; | |
| } | |
| ZbsStorageController.CreateVolumeCmd cmd = new ZbsStorageController.CreateVolumeCmd(); | |
| cmd.setLogicalPool(target.logicalPool); | |
| cmd.setVolume(target.volume); | |
| cmd.setUnit(getSizeUnit(addonInfo.getClusterInfo().getVersion())); | |
| long allocatedSize = targetEncrypted ? luksBackingSize(virtualSize) : virtualSize; | |
| cmd.setSize(alignSizeTo(allocatedSize, cmd.getUnit())); | |
| cmd.setSkipIfExisting(false); | |
| controller.httpCall(ZbsStorageController.CREATE_VOLUME_PATH, cmd, ZbsStorageController.CreateVolumeRsp.class, | |
| new ReturnValueCompletion<ZbsStorageController.CreateVolumeRsp>(completion) { | |
| @Override | |
| public void success(ZbsStorageController.CreateVolumeRsp returnValue) { | |
| String createdInstallPath = returnValue.getInstallPath(); | |
| if (targetInstallPath.equals(createdInstallPath)) { | |
| completion.success(createdInstallPath); | |
| return; | |
| } | |
| ErrorCode error = operr( | |
| "ZBS volume encryption conversion target[%s] was created at unexpected path[%s]", | |
| targetInstallPath, createdInstallPath); | |
| String cleanupInstallPath = targetInstallPath; | |
| try { | |
| deleteConversionTarget(cleanupInstallPath, new Completion(completion) { | |
| @Override | |
| public void success() { | |
| completion.fail(error); | |
| } | |
| @Override | |
| public void fail(ErrorCode cleanupError) { | |
| logger.warn(String.format( | |
| "failed to cleanup unexpected ZBS conversion target[installPath:%s] after create path mismatch: %s", | |
| cleanupInstallPath, cleanupError)); | |
| completion.fail(error); | |
| } | |
| }); | |
| } catch (Exception e) { | |
| logger.warn(String.format( | |
| "failed to cleanup unexpected ZBS conversion target[installPath:%s] after create path mismatch: %s", | |
| cleanupInstallPath, e.getMessage())); | |
| completion.fail(error); | |
| } | |
| } | |
| @Override | |
| public void fail(ErrorCode errorCode) { | |
| completion.fail(errorCode); | |
| } | |
| }); | |
| } | |
| ErrorCode error = operr( | |
| "ZBS volume encryption conversion target[%s] was created at unexpected path[%s]", | |
| targetInstallPath, createdInstallPath); | |
| String cleanupInstallPath = createdInstallPath; | |
| try { | |
| deleteConversionTarget(cleanupInstallPath, new Completion(completion) { |
🤖 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
`@plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsVolumeEncryptionBackendImpl.java`
around lines 200 - 261, Update the path-mismatch cleanup in
createConversionTarget so cleanupInstallPath uses createdInstallPath, the actual
path returned by ZBS, instead of targetInstallPath. Preserve the existing error
reporting and completion behavior while ensuring deleteConversionTarget removes
the created orphan volume.
There was a problem hiding this comment.
Comment from 周众:
ZBS 创建卷的实际标识由请求中的 logicalPool/volume 决定,CreateVolumeRsp.installPath 只是基于相同请求字段拼装的响应值;删除接口同样按 logicalPool/volume 删除。路径不一致说明响应字段异常,不能信任它执行删除,否则返回源卷或其他卷路径时可能误删数据。当前逻辑及回归测试有意只清理请求的 targetInstallPath。
There was a problem hiding this comment.
@ZStack-Robot 感谢周众的解释,这个说明是合理的。既然 ZBS 创建卷时实际使用的标识是请求中的 logicalPool/volume(即 target 解析出的字段),而 CreateVolumeRsp.installPath 只是基于同样字段拼装出的回显值,一旦该字段异常,也不能保证其指向的就是真正创建出来的卷;而删除接口同样是按 logicalPool/volume 定位卷。若信任这个不可靠的回显字段去执行删除,反而有误删源卷或其他卷的风险。因此当前只清理 targetInstallPath(对应实际用于创建的请求参数)是更安全的选择,我撤回此前的建议。
|
Comment from 周众: 第二个Critical问题不在这个patch解,后面会单独提patch解。 |
1 similar comment
|
Comment from 周众: 第二个Critical问题不在这个patch解,后面会单独提patch解。 |
2e62674 to
b6690f9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsVolumeEncryptionCase.groovy`:
- Around line 193-249: Update testEncryptedEmptyVolumeCreatorReusesImageKey to
configure secretHelper with a non-empty sealedDek so the sealing step succeeds,
then assert successful stats creation and no deletedPaths. Remove the
failure/error assertions from this reuse-success test; cover cleanup behavior
separately if needed, while preserving the existing key reuse request
assertions.
🪄 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: 1da40e80-e791-4253-8776-1706f47738f7
⛔ Files ignored due to path filters (1)
conf/springConfigXml/VolumeManager.xmlis excluded by!**/*.xml
📒 Files selected for processing (16)
header/src/main/java/org/zstack/header/storage/addon/primary/CreateVolumeSpec.javaheader/src/main/java/org/zstack/header/storage/addon/primary/PrimaryStorageControllerSvc.javaheader/src/main/java/org/zstack/header/storage/addon/primary/ZbsVolumeEncryptionBackend.javaheader/src/main/java/org/zstack/header/storage/addon/primary/ZbsVolumeEncryptionExtensionPoint.javaplugin/expon/src/main/java/org/zstack/expon/ExponStorageController.javaplugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.javaplugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsVolumeEncryptionBackendImpl.javastorage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.javastorage/src/main/java/org/zstack/storage/encrypt/ZbsEncryptedEmptyVolumeCreator.javastorage/src/main/java/org/zstack/storage/encrypt/ZbsVolumeEncryptionCommands.javastorage/src/main/java/org/zstack/storage/encrypt/ZbsVolumeEncryptionConstants.javastorage/src/main/java/org/zstack/storage/encrypt/ZbsVolumeEncryptionConverter.javastorage/src/main/java/org/zstack/storage/encrypt/ZbsVolumeEncryptionExtension.javastorage/src/main/java/org/zstack/storage/encrypt/ZbsVolumeEncryptionMaterialFactory.javastorage/src/main/java/org/zstack/storage/volume/VolumeBase.javatest/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsVolumeEncryptionCase.groovy
🚧 Files skipped from review as they are similar to previous changes (14)
- storage/src/main/java/org/zstack/storage/encrypt/ZbsEncryptedEmptyVolumeCreator.java
- header/src/main/java/org/zstack/header/storage/addon/primary/PrimaryStorageControllerSvc.java
- plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.java
- storage/src/main/java/org/zstack/storage/encrypt/ZbsVolumeEncryptionExtension.java
- storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java
- header/src/main/java/org/zstack/header/storage/addon/primary/ZbsVolumeEncryptionExtensionPoint.java
- storage/src/main/java/org/zstack/storage/encrypt/ZbsVolumeEncryptionCommands.java
- header/src/main/java/org/zstack/header/storage/addon/primary/CreateVolumeSpec.java
- plugin/expon/src/main/java/org/zstack/expon/ExponStorageController.java
- plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsVolumeEncryptionBackendImpl.java
- storage/src/main/java/org/zstack/storage/encrypt/ZbsVolumeEncryptionMaterialFactory.java
- storage/src/main/java/org/zstack/storage/volume/VolumeBase.java
- header/src/main/java/org/zstack/header/storage/addon/primary/ZbsVolumeEncryptionBackend.java
- storage/src/main/java/org/zstack/storage/encrypt/ZbsVolumeEncryptionConverter.java
Route ZBS conversion through a dedicated backend and KVM handler. Tests: cbok zsv groovy_test ZbsVolumeEncryptionCase Resolves: ZSV-12664 Change-Id: Ibd30f74dd17cb4dc817e57c713b0f32915487e7c
b6690f9 to
73a5dfe
Compare
Summary
为 ZBS/CBD 云盘增加明文与加密格式双向转换,转换语义参考 RBD;云盘存在主存储快照时拒绝转换。
Changes
Testing
Resolves: ZSV-12664
sync from gitlab !10539