<feature>[storage]: switch volume encryption conversion to new install path#4530
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 17 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 (34)
WalkthroughChanges卷加密转换流程
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant VolumeBase
participant CephPrimaryStorageBase
participant KVMHost
participant TargetVolume
VolumeBase->>CephPrimaryStorageBase: 提交加密转换项
CephPrimaryStorageBase->>KVMHost: 检查目标路径
CephPrimaryStorageBase->>KVMHost: 执行LUKS转换
KVMHost-->>CephPrimaryStorageBase: 返回actualSize或空结果
CephPrimaryStorageBase->>TargetVolume: 空结果时同步实际大小
CephPrimaryStorageBase-->>VolumeBase: 返回转换结果
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/dev/encrypted-volume-snapshot-new-install-path@@3
branch
4 times, most recently
from
3a1ea10 to
9b021ab
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
storage/src/main/java/org/zstack/storage/volume/VolumeBase.java (1)
3636-3660: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value布尔参数在调用点含义不明确，建议改进。
makeConvertedResourceFileName(resourceUuid, targetEncrypted, conversionUuid, qcow2File)中qcow2File（以及makeConvertedVolumeInstallPath/makeConvertedSnapshotInstallPath的targetEncrypted）在调用点以裸true/false传入，可读性较差，例如makeConvertedResourceFileName(resourceUuid, targetEncrypted, conversionUuid, false)。可考虑拆分为语义化方法（如makeConvertedQcow2FileName/makeConvertedRawFileName）或使用枚举表达文件类型。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/volume/VolumeBase.java` around lines 3636 - 3660, 重构 makeConvertedResourceFileName 及其调用点，避免通过裸布尔值表示文件类型；拆分为语义明确的 makeConvertedQcow2FileName 和 makeConvertedRawFileName，或使用枚举表达类型，并更新 makeConvertedVolumeInstallPath、makeConvertedSnapshotInstallPath 中的所有调用。Source: Path instructions
plugin/ceph/src/main/java/org/zstack/storage/ceph/primary/CephPrimaryStorageBase.java (1)
3620-3641: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win建议将 DEK 封装移到目标存在性检查通过之后。
当前
materializeAndSealVolumeDekForHost(...)（Line 3620）在CheckIsBitsExistingCmd检查之前执行；若目标路径已存在（Line 3628 直接返回错误），则已在宿主上封装/暂存的 DEK 未被使用，属于无谓开销，并可能在宿主上残留 secret 材料。建议把kcmd.encryptedDek = ...移入存在性检查成功且!isExisting()的分支（即convertVolumeEncryptionOnKvmHost调用前）再计算。请确认
materializeAndSealVolumeDekForHost是否在宿主侧产生需要清理的 secret 副作用，以评估影响范围。♻️ 建议的调整
KVMHostLuksConvertCmd kcmd = new KVMHostLuksConvertCmd(); kcmd.psUuid = self.getUuid(); kcmd.installPath = item.getSourceInstallPath(); kcmd.targetInstallPath = item.getTargetInstallPath(); kcmd.targetEncrypted = msg.isTargetEncrypted(); kcmd.virtualSize = msg.getVolume().getSize(); - kcmd.encryptedDek = volumeEncryptedSecretHelper.materializeAndSealVolumeDekForHost(hostUuid, msg.getVolume().getUuid()); CheckIsBitsExistingCmd checkTargetCmd = new CheckIsBitsExistingCmd(); checkTargetCmd.setInstallPath(item.getTargetInstallPath()); httpCall(CHECK_BITS_PATH, checkTargetCmd, CheckIsBitsExistingRsp.class, new ReturnValueCompletion<CheckIsBitsExistingRsp>(msg) { `@Override` public void success(CheckIsBitsExistingRsp returnValue) { if (returnValue.isExisting()) { reply.setError(operr("RBD LUKS conversion target already exists: %s", item.getTargetInstallPath())); bus.reply(msg, reply); return; } + kcmd.encryptedDek = volumeEncryptedSecretHelper.materializeAndSealVolumeDekForHost(hostUuid, msg.getVolume().getUuid()); convertVolumeEncryptionOnKvmHost(msg, reply, kcmd, hostUuid); }🤖 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/ceph/src/main/java/org/zstack/storage/ceph/primary/CephPrimaryStorageBase.java` around lines 3620 - 3641, 将 materializeAndSealVolumeDekForHost 的调用从 CheckIsBitsExistingCmd 检查之前移入 success 方法中确认 !returnValue.isExisting() 的分支，并在调用 convertVolumeEncryptionOnKvmHost 前为 kcmd.encryptedDek 赋值；保持目标已存在或检查失败时不生成 DEK，同时确认该方法产生的宿主侧 secret 副作用及清理行为。
🤖 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.
Nitpick comments:
In
`@plugin/ceph/src/main/java/org/zstack/storage/ceph/primary/CephPrimaryStorageBase.java`:
- Around line 3620-3641: 将 materializeAndSealVolumeDekForHost 的调用从
CheckIsBitsExistingCmd 检查之前移入 success 方法中确认 !returnValue.isExisting() 的分支，并在调用
convertVolumeEncryptionOnKvmHost 前为 kcmd.encryptedDek 赋值；保持目标已存在或检查失败时不生成
DEK，同时确认该方法产生的宿主侧 secret 副作用及清理行为。
In `@storage/src/main/java/org/zstack/storage/volume/VolumeBase.java`:
- Around line 3636-3660: 重构 makeConvertedResourceFileName
及其调用点，避免通过裸布尔值表示文件类型；拆分为语义明确的 makeConvertedQcow2FileName 和
makeConvertedRawFileName，或使用枚举表达类型，并更新
makeConvertedVolumeInstallPath、makeConvertedSnapshotInstallPath 中的所有调用。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b6531ed8-e10b-481f-b03e-6eb5a83dca86
📒 Files selected for processing (4)
header/src/main/java/org/zstack/header/storage/primary/ConvertVolumeEncryptionOnPrimaryStorageMsg.javaplugin/ceph/src/main/java/org/zstack/storage/ceph/primary/CephPrimaryStorageBase.javastorage/src/main/java/org/zstack/storage/volume/VolumeBase.javatestlib/src/main/java/org/zstack/testlib/CephPrimaryStorageSpec.groovy
💤 Files with no reviewable changes (2)
- header/src/main/java/org/zstack/header/storage/primary/ConvertVolumeEncryptionOnPrimaryStorageMsg.java
- testlib/src/main/java/org/zstack/testlib/CephPrimaryStorageSpec.groovy
MatheMatrix
force-pushed
the
sync/zstackio/dev/encrypted-volume-snapshot-new-install-path@@3
branch
8 times, most recently
from
23608ea to
21355ca
Compare
Resolves: ZSV-12469 Change-Id: I65c44aa24161e828aab27463512ff5046c402789
MatheMatrix
force-pushed
the
dev/encrypted-volume-snapshot-conversion-rollback@@3
branch
from
763f2db to
ab4f2b5
Compare
1 participant
Summary
Switch volume/snapshot encryption conversion to generate a new installPath on top of the rollback refactor branch. Source bits stay untouched before DB commit; rollback only cleans the new target path.
Changes
Testing
sync from gitlab !10492