<feature>[zbs]: ZSV-12579 support zbs volume encryption#4366
Open
ZStack-Robot wants to merge 3 commits into
Open
<feature>[zbs]: ZSV-12579 support zbs volume encryption#4366ZStack-Robot wants to merge 3 commits into
ZStack-Robot wants to merge 3 commits into
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 33 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 (2)
📒 Files selected for processing (33)
Walkthrough本次改动扩展了加密卷的公共契约和扩展点，并让 ZBS 控制器、外部主存储与 Expon 扩容路径按卷的加密状态分派和填充相关字段。 ChangesZBS 加密卷与镜像缓存支持
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ExternalPrimaryStorage
participant ZbsStorageController
participant ZbsVolumeEncryptionExtensionPoint
Caller->>ExternalPrimaryStorage: 发送卷/快照相关主存储消息
ExternalPrimaryStorage->>ExternalPrimaryStorage: prepareVolumeSpecEncryption / prepareSnapshotImageCacheEncryption
ExternalPrimaryStorage->>ZbsStorageController: 提交带加密字段的处理请求
ZbsStorageController->>ZbsVolumeEncryptionExtensionPoint: 按 encrypted 状态选择对应扩展实现
ZbsVolumeEncryptionExtensionPoint-->>ZbsStorageController: 返回 VolumeStats 或加密结果
ZbsStorageController-->>ExternalPrimaryStorage: 回传处理结果
ExternalPrimaryStorage-->>Caller: 返回主存储回复
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (1)
plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.java (1)
672-688: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
resolveLuksHostUuid建议校验宿主可用性。此处仅取任意一条
status=Connected的PrimaryStorageHostRefVO，未校验宿主的hypervisorType=KVM与state=Enabled/status=Connected（对比ExternalPrimaryStorage.findConnectedKvmHostForEncryption的更严格筛选）。可能选中一台已 Disabled 或主机侧失联的宿主，导致 LUKS 操作失败。建议与外部主存储侧保持一致的宿主筛选条件。🤖 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/ZbsStorageController.java` around lines 672 - 688, The resolveLuksHostUuid method currently picks any connected PrimaryStorageHostRefVO without verifying the host is a usable KVM host. Update the host lookup in ZbsStorageController.resolveLuksHostUuid to apply the same stricter filtering as ExternalPrimaryStorage.findConnectedKvmHostForEncryption, including hypervisorType=KVM and host state/status checks such as Enabled and Connected. Keep the preferredHostUuid shortcut intact, but make the fallback selection only return a host that is actually eligible for the LUKS operation.
🤖 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/ZbsStorageController.java`:
- Around line 1202-1233: The replace-original-volume flow currently deletes the
original volume before the COPY step, creating an unrecoverable window if the
copy fails. Update the logic in the replace-original-volume NoRollbackFlow
within ZbsStorageController so the data is copied from tempPath to the target
volume first, and only after a successful httpCall(COPY_PATH, ...) should
doDeleteVolume(msg.getInstallPath(), true) run; alternatively make this step
rollback-safe by restoring from tempPath on failure. Keep the trigger/completion
handling in place, but ensure the original install path is not removed until the
replacement volume is fully written and verified.
In
`@storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java`:
- Around line 2244-2248: The GetVolumeSnapshotEncryptedOnPrimaryStorageMsg
handler is returning the primary storage install path in the encrypt field,
which is the wrong value. Update
ExternalPrimaryStorage.handle(GetVolumeSnapshotEncryptedOnPrimaryStorageMsg) so
reply.setEncrypt uses the actual encryption flag/status from the message or
computed result instead of msg.getPrimaryStorageInstallPath(), and keep the
snapshotUuid reply unchanged.
- Around line 176-198: The host selection logic in
ExternalPrimaryStorage.getConnectedHostUuid currently relaxes the filter when
connectedPsHostUuids is empty, which can allow picking a KVM host that is not
actually connected to the external primary storage. Update this method to
distinguish between “no connected PrimaryStorageHostRefVO exists” and “some refs
exist but none are currently connected,” and fail fast in the empty case instead
of removing the hostUuid constraint. Keep the existing query constraints on
clusterUuids, VmInstanceConstant.KVM_HYPERVISOR_TYPE, HostStatus.Connected, and
HostState.Enabled, but ensure the fallback does not broaden selection beyond
hosts truly attached to the storage.
---
Nitpick comments:
In `@plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.java`:
- Around line 672-688: The resolveLuksHostUuid method currently picks any
connected PrimaryStorageHostRefVO without verifying the host is a usable KVM
host. Update the host lookup in ZbsStorageController.resolveLuksHostUuid to
apply the same stricter filtering as
ExternalPrimaryStorage.findConnectedKvmHostForEncryption, including
hypervisorType=KVM and host state/status checks such as Enabled and Connected.
Keep the preferredHostUuid shortcut intact, but make the fallback selection only
return a host that is actually eligible for the LUKS operation.
🪄 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: c7290d64-3289-461e-9f24-d18323f993c4
📒 Files selected for processing (8)
header/src/main/java/org/zstack/header/storage/addon/primary/BaseVolumeInfo.javaheader/src/main/java/org/zstack/header/storage/addon/primary/CreateVolumeSnapshotSpec.javaheader/src/main/java/org/zstack/header/storage/addon/primary/CreateVolumeSpec.javaheader/src/main/java/org/zstack/header/storage/addon/primary/PrimaryStorageControllerSvc.javaplugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.javastorage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.javastorage/src/main/java/org/zstack/storage/volume/VolumeBase.javatest/src/test/java/org/zstack/test/unittest/storage/addon/primary/ExternalPrimaryStorageEncryptionSpecTest.java
Comment on lines
+176
to
+198
| List<String> connectedPsHostUuids = Q.New(PrimaryStorageHostRefVO.class) | ||
| .select(PrimaryStorageHostRefVO_.hostUuid) | ||
| .eq(PrimaryStorageHostRefVO_.primaryStorageUuid, self.getUuid()) | ||
| .eq(PrimaryStorageHostRefVO_.status, PrimaryStorageHostStatus.Connected) | ||
| .listValues(); | ||
|
|
||
| org.zstack.core.db.SimpleQuery<HostVO> q = dbf.createQuery(HostVO.class); | ||
| q.add(HostVO_.clusterUuid, org.zstack.core.db.SimpleQuery.Op.IN, clusterUuids); | ||
| if (!connectedPsHostUuids.isEmpty()) { | ||
| q.add(HostVO_.uuid, org.zstack.core.db.SimpleQuery.Op.IN, connectedPsHostUuids); | ||
| } | ||
| q.add(HostVO_.hypervisorType, org.zstack.core.db.SimpleQuery.Op.EQ, VmInstanceConstant.KVM_HYPERVISOR_TYPE); | ||
| q.add(HostVO_.status, org.zstack.core.db.SimpleQuery.Op.EQ, HostStatus.Connected); | ||
| q.add(HostVO_.state, org.zstack.core.db.SimpleQuery.Op.EQ, HostState.Enabled); | ||
| q.orderBy(HostVO_.uuid, org.zstack.core.db.SimpleQuery.Od.ASC); | ||
| q.setLimit(1); | ||
| HostVO host = q.find(); | ||
| if (host == null) { | ||
| throw new OperationFailureException(operr( | ||
| "cannot find a connected KVM host attached to external primary storage[uuid:%s] to %s", | ||
| self.getUuid(), operation)); | ||
| } | ||
| return host.getUuid(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 确认 PrimaryStorageHostRefVO 连接状态对外部主存储如何维护
rg -nP '\bupdatePrimaryStorageHostStatus\b' --type=java -C3 -g '!**/test/**'
echo "===== PrimaryStorageHostRefVO 写入点 ====="
rg -nP 'PrimaryStorageHostRefVO_\.status|set\(PrimaryStorageHostRefVO_\.status' --type=java -C2 -g '!**/*_.java'Repository: MatheMatrix/zstack
Length of output: 156
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== File outline =="
ast-grep outline storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java --view expanded || true
echo
echo "== Relevant slice of ExternalPrimaryStorage.java =="
sed -n '150,230p' storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java | cat -n
echo
echo "== Search for ExternalPrimaryStorageHostRefVO usages =="
rg -n "ExternalPrimaryStorageHostRefVO|PrimaryStorageHostRefVO" storage/src/main/java -g '*.java' -C 2
echo
echo "== Search for KVM host selection in this addon =="
rg -n "connectedPsHostUuids|HostState.Enabled|VmInstanceConstant.KVM_HYPERVISOR_TYPE" storage/src/main/java/org/zstack/storage -g '*.java' -C 2Repository: MatheMatrix/zstack
Length of output: 31282
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== UpdatePrimaryStorageHostStatusMsg handling in primary storage code =="
rg -n "UpdatePrimaryStorageHostStatusMsg|handle\\(UpdatePrimaryStorageHostStatusMsg" storage/src/main/java/org/zstack/storage -g '*.java' -C 4
echo
echo "== PrimaryStorageBase status update block =="
sed -n '540,600p' storage/src/main/java/org/zstack/storage/primary/PrimaryStorageBase.java | cat -n
echo
echo "== ExternalPrimaryStorage status update block =="
sed -n '330,360p' storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java | cat -n
echo
echo "== Find any explicit update of ExternalPrimaryStorageHostRefVO status =="
rg -n "ExternalPrimaryStorageHostRefVO_\\.status|set\\(ExternalPrimaryStorageHostRefVO_\\.status|PrimaryStorageHostStatus\\.Connected|PrimaryStorageHostStatus\\.Disconnected" storage/src/main/java/org/zstack/storage/addon/primary -g '*.java' -C 2Repository: MatheMatrix/zstack
Length of output: 12133
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Encryption host selection call sites =="
rg -n "findConnectedKvmHostForEncryption|resolveEncryptionHostUuid|materializeAndSealVolumeDekForHost|prepareVolumeSpecEncryption|prepareImageCacheSpecEncryption" storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java -C 3
echo
echo "== External primary storage client / ZBS references =="
rg -n "ZBS|external primary storage client|client.*primary storage|encrypt.*host|sealVolumeDek|LUKS" storage/src/main/java/org/zstack/storage/addon/primary -g '*.java' -C 3
echo
echo "== PrimaryStorageHostRefVO queries in external storage code =="
rg -n "PrimaryStorageHostRefVO" storage/src/main/java/org/zstack/storage/addon/primary -g '*.java' -C 3
echo
echo "== ExternalPrimaryStorageHostRefVO purpose =="
sed -n '1,220p' storage/src/main/java/org/zstack/storage/addon/primary/ExternalHostIdGetter.java | cat -nRepository: MatheMatrix/zstack
Length of output: 34918
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Volume encrypted secret helper references =="
rg -n "materializeAndSealVolumeDekForHost|volumeEncryptedSecretHelper|snapshotEncryptionHelper|prepareTemporarySnapshotImageEncryptedDek" storage/src/main/java -g '*.java' -C 3
echo
echo "== Host access path / encryption flow around external primary storage =="
sed -n '200,270p' storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java | cat -n
echo
echo "== Search for any check requiring connectedPsHostUuids non-empty =="
rg -n "connectedPsHostUuids.isEmpty|cannot find a connected KVM host attached to external primary storage|prepare LUKS secret" storage/src/main/java -g '*.java' -C 3Repository: MatheMatrix/zstack
Length of output: 41880
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== VolumeManagerImpl host ref logic around empty connected refs =="
sed -n '740,770p' storage/src/main/java/org/zstack/storage/volume/VolumeManagerImpl.java | cat -n
echo
echo "== PrimaryStorageMainAllocatorFlow host ref logic =="
sed -n '55,85p' storage/src/main/java/org/zstack/storage/primary/PrimaryStorageMainAllocatorFlow.java | cat -n
echo
echo "== External primary storage docs/comments mentioning host refs or connected status =="
rg -n "connectedHostUuids|PrimaryStorageHostStatus|ExternalPrimaryStorageHostRefVO|primary storage host ref|external primary storage" storage/src/main/java/org/zstack/storage -g '*.java' -C 2Repository: MatheMatrix/zstack
Length of output: 34599
空的 connected host 列表不能直接放宽筛选。
这里会退化为仅按 cluster / KVM / Connected 选宿主；如果 connectedPsHostUuids 为空表示该外部主存储没有任何 Connected 的 host ref，就可能把 LUKS 相关操作派到未连通该主存储的宿主。建议这里直接返回失败，或区分“没有 ref”和“全离线”的情况。
🤖 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 176 - 198, The host selection logic in
ExternalPrimaryStorage.getConnectedHostUuid currently relaxes the filter when
connectedPsHostUuids is empty, which can allow picking a KVM host that is not
actually connected to the external primary storage. Update this method to
distinguish between “no connected PrimaryStorageHostRefVO exists” and “some refs
exist but none are currently connected,” and fail fast in the empty case instead
of removing the hostUuid constraint. Keep the existing query constraints on
clusterUuids, VmInstanceConstant.KVM_HYPERVISOR_TYPE, HostStatus.Connected, and
HostState.Enabled, but ensure the fallback does not broaden selection beyond
hosts truly attached to the storage.
MatheMatrix
force-pushed
the
sync/zstackio/zbs-volume-encryption@@3
branch
12 times, most recently
from
d0db2d4 to
3085c2e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.java (1)
662-694: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win清理失败被
NopeCompletion静默吞掉，缺少日志。失败分支中
doDeleteVolume(cmd.dstPath, true, new NopeCompletion())用于清理部分创建的加密卷，但若该清理本身失败，NopeCompletion不会记录任何日志，可能导致 ZBS 上残留孤立的加密卷且无排查线索。建议至少在清理失败时打印警告日志。♻️ 建议的修改（以第一处清理调用为例，其余两处同理）
if (!reply.isSuccess()) { - doDeleteVolume(cmd.dstPath, true, new NopeCompletion()); + doDeleteVolume(cmd.dstPath, true, new Completion(trigger) { + `@Override` + public void success() {} + + `@Override` + public void fail(ErrorCode errorCode) { + logger.warn(String.format("failed to clean up dst volume[%s] after luks clone failure: %s", + cmd.dstPath, errorCode)); + } + }); trigger.fail(reply.getError()); 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 `@plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.java` around lines 662 - 694, The cleanup path in ZbsStorageController’s KVM callback is swallowing failures because doDeleteVolume(..., new NopeCompletion()) does not emit any log if the delete itself fails. Update the failure branches in the anonymous CloudBusCallBack.run handling KVMHostLuksRsp to use a completion that logs a warning on cleanup failure (or wrap NopeCompletion with logging) before calling trigger.fail, and apply the same treatment to each doDeleteVolume(cmd.dstPath, true, ...) call in this block so partial encrypted volumes are not left behind silently.header/src/main/java/org/zstack/header/storage/addon/primary/CreateVolumeSpec.java (1)
10-12: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win建议为
encryptedDek增加日志脱敏保护。下游
ZbsStorageController.KVMHostLuksCloneCmd.encryptedDek已用@NoLogging标注防止敏感信息写入日志，但此处CreateVolumeSpec.encryptedDek作为同一份密封 DEK 的载体，在被转发给KVMHostLuksCloneCmd之前若被框架的消息总线调试日志或toString/JSON 序列化打印，仍存在泄露风险。建议保持两端一致性，为该字段加上等效的日志脱敏标注。🔒 建议的修改
+import org.zstack.header.rest.NoLogging; + public class CreateVolumeSpec { private String name; private String uuid; private long size; private VolumeQos qos; private boolean encrypted; private String hostUuid; + `@NoLogging` private String encryptedDek;Also applies to: 81-87
🤖 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/CreateVolumeSpec.java` around lines 10 - 12, `CreateVolumeSpec.encryptedDek` currently carries sensitive DEK data without any logging protection, unlike `ZbsStorageController.KVMHostLuksCloneCmd.encryptedDek` which already uses `@NoLogging`. Add the same log-redaction annotation to `CreateVolumeSpec.encryptedDek` so framework debug logs, `toString`, or JSON serialization do not expose the sealed key material before it is forwarded, and keep the handling consistent with the downstream command field.
🤖 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/ZbsStorageController.java`:
- Around line 643-654: The encrypted clone path in ZbsStorageController around
the KVMHostLuksCloneCmd setup only checks hostUuid and should also validate
dst.getEncryptedDek() before sending the request. Add a blank/null check for
encryptedDek in the same dst.isEncrypted() branch, fail early with a clear operr
message from the controller, and only populate cmd.encryptedDek after the value
is confirmed present.
---
Nitpick comments:
In
`@header/src/main/java/org/zstack/header/storage/addon/primary/CreateVolumeSpec.java`:
- Around line 10-12: `CreateVolumeSpec.encryptedDek` currently carries sensitive
DEK data without any logging protection, unlike
`ZbsStorageController.KVMHostLuksCloneCmd.encryptedDek` which already uses
`@NoLogging`. Add the same log-redaction annotation to
`CreateVolumeSpec.encryptedDek` so framework debug logs, `toString`, or JSON
serialization do not expose the sealed key material before it is forwarded, and
keep the handling consistent with the downstream command field.
In `@plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.java`:
- Around line 662-694: The cleanup path in ZbsStorageController’s KVM callback
is swallowing failures because doDeleteVolume(..., new NopeCompletion()) does
not emit any log if the delete itself fails. Update the failure branches in the
anonymous CloudBusCallBack.run handling KVMHostLuksRsp to use a completion that
logs a warning on cleanup failure (or wrap NopeCompletion with logging) before
calling trigger.fail, and apply the same treatment to each
doDeleteVolume(cmd.dstPath, true, ...) call in this block so partial encrypted
volumes are not left behind silently.
🪄 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: e11ca870-feb4-4fb6-afd0-52ed479f7a3d
📒 Files selected for processing (3)
header/src/main/java/org/zstack/header/storage/addon/primary/CreateVolumeSpec.javaplugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.javastorage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java
Comment on lines
+643
to
+654
| if (dst.isEncrypted()) { | ||
| if (StringUtils.isBlank(dst.getHostUuid())) { | ||
| trigger.fail(operr("cannot clone encrypted ZBS volume from image, hostUuid is missing")); | ||
| return; | ||
| } | ||
|
|
||
| KVMHostLuksCloneCmd cmd = new KVMHostLuksCloneCmd(); | ||
| cmd.psUuid = self.getUuid(); | ||
| cmd.srcPath = srcInstallPath; | ||
| cmd.dstPath = buildVolumePath("", config.getLogicalPoolName(), dst.getName()); | ||
| cmd.encryptedDek = dst.getEncryptedDek(); | ||
| cmd.virtualSizeForLuksClone = dst.getSize(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
缺少对 encryptedDek 的空值校验。
第 644-647 行校验了 hostUuid 为空的情况并给出清晰错误，但未对 dst.getEncryptedDek() 做同样校验。若因未来代码变更导致 isEncrypted()=true 但 encryptedDek 为空/null（未经 ExternalPrimaryStorage.prepareVolumeSpecEncryption 正确填充），该请求仍会被发往 KVM Host，产生的将是较难定位的 agent 端错误而非明确的管理节点错误提示。
🛡️ 建议的修改
if (dst.isEncrypted()) {
if (StringUtils.isBlank(dst.getHostUuid())) {
trigger.fail(operr("cannot clone encrypted ZBS volume from image, hostUuid is missing"));
return;
}
+ if (StringUtils.isBlank(dst.getEncryptedDek())) {
+ trigger.fail(operr("cannot clone encrypted ZBS volume from image, encryptedDek is missing"));
+ return;
+ }📝 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.
Suggested change
| if (dst.isEncrypted()) { | |
| if (StringUtils.isBlank(dst.getHostUuid())) { | |
| trigger.fail(operr("cannot clone encrypted ZBS volume from image, hostUuid is missing")); | |
| return; | |
| } | |
| KVMHostLuksCloneCmd cmd = new KVMHostLuksCloneCmd(); | |
| cmd.psUuid = self.getUuid(); | |
| cmd.srcPath = srcInstallPath; | |
| cmd.dstPath = buildVolumePath("", config.getLogicalPoolName(), dst.getName()); | |
| cmd.encryptedDek = dst.getEncryptedDek(); | |
| cmd.virtualSizeForLuksClone = dst.getSize(); | |
| if (dst.isEncrypted()) { | |
| if (StringUtils.isBlank(dst.getHostUuid())) { | |
| trigger.fail(operr("cannot clone encrypted ZBS volume from image, hostUuid is missing")); | |
| return; | |
| } | |
| if (StringUtils.isBlank(dst.getEncryptedDek())) { | |
| trigger.fail(operr("cannot clone encrypted ZBS volume from image, encryptedDek is missing")); | |
| return; | |
| } | |
| KVMHostLuksCloneCmd cmd = new KVMHostLuksCloneCmd(); | |
| cmd.psUuid = self.getUuid(); | |
| cmd.srcPath = srcInstallPath; | |
| cmd.dstPath = buildVolumePath("", config.getLogicalPoolName(), dst.getName()); | |
| cmd.encryptedDek = dst.getEncryptedDek(); | |
| cmd.virtualSizeForLuksClone = dst.getSize(); |
🤖 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/ZbsStorageController.java`
around lines 643 - 654, The encrypted clone path in ZbsStorageController around
the KVMHostLuksCloneCmd setup only checks hostUuid and should also validate
dst.getEncryptedDek() before sending the request. Add a blank/null check for
encryptedDek in the same dst.isEncrypted() branch, fail early with a clear operr
message from the controller, and only populate cmd.encryptedDek after the value
is confirmed present.
d9afe36 to
cb12c2a
Compare
MatheMatrix
force-pushed
the
sync/zstackio/zbs-volume-encryption@@3
branch
from
cb12c2a to
9ddbfda
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
header/src/main/java/org/zstack/header/storage/addon/primary/ZbsVolumeEncryptionExtensionPoint.java (1)
9-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win请为新增扩展点契约补齐 Javadoc。
这是面向实现方的扩展点，当前没有说明参数顺序、
cleanup调用时机/幂等要求、以及 completion 成功失败语义，后续实现容易产生不一致行为。建议补充
+/** + * Extension point for ZBS encrypted volume operations. + */ public interface ZbsVolumeEncryptionExtensionPoint { + /** + * Creates an encrypted empty volume at the target install path. + */ void createEncryptedEmptyVolume(String primaryStorageUuid, String installPath, CreateVolumeSpec spec, VolumeCleanup cleanup, ReturnValueCompletion<VolumeStats> completion); + /** + * Clones an encrypted volume from an image path to the target install path. + */ void cloneEncryptedVolumeFromImage(String primaryStorageUuid, String srcInstallPath, String dstInstallPath, CreateVolumeSpec dst, VolumeCleanup cleanup, ReturnValueCompletion<VolumeStats> completion); + /** + * Copies an encrypted volume from a snapshot path to the target install path. + */ void copyEncryptedVolumeFromSnapshot(String primaryStorageUuid, String srcInstallPath, String dstInstallPath, CreateVolumeSpec dst, VolumeCleanup cleanup, ReturnValueCompletion<VolumeStats> completion); + /** + * Encrypts existing volume bits in place. + */ void encryptVolumeBits(String primaryStorageUuid, EncryptVolumeBitsOnPrimaryStorageMsg msg, ReturnValueCompletion<EncryptVolumeBitsOnPrimaryStorageReply> completion); + /** + * Resizes an encrypted volume and returns refreshed volume stats. + */ void resizeEncryptedVolume(String primaryStorageUuid, VolumeInventory volume, long size, ReturnValueCompletion<VolumeStats> completion); interface VolumeCleanup { + /** + * Cleans up a partially-created volume path. Implementations should be safe to call during failure handling. + */ void cleanup(String installPath); } }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/ZbsVolumeEncryptionExtensionPoint.java` around lines 9 - 29, Add Javadoc for ZbsVolumeEncryptionExtensionPoint and each method to document the parameter order, expected lifecycle and idempotency of VolumeCleanup.cleanup, and the success/failure contract for each ReturnValueCompletion callback. Also remove any redundant interface method modifiers if present, and ensure the nested VolumeCleanup contract is documented clearly so implementers know when cleanup is safe to call and how completion should signal errors.Source: Path instructions
header/src/main/java/org/zstack/header/storage/addon/primary/PrimaryStorageControllerSvc.java (1)
38-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win请为新增/变更的接口方法补充 Javadoc。
encryptVolumeBits(...)和新的expandVolume(VolumeInventory, ...)都是公共控制器契约；建议明确 unsupported 默认行为、VolumeInventory期望携带的路径/加密上下文，以及 completion 返回语义。建议补充
+ /** + * Encrypts existing volume bits in place. + * + * `@param` msg request carrying target volume and encryption context + * `@param` comp completion notified with the controller reply, or an unsupported-operation error by default + */ default void encryptVolumeBits(EncryptVolumeBitsOnPrimaryStorageMsg msg, ReturnValueCompletion<EncryptVolumeBitsOnPrimaryStorageReply> comp) { String details = String.format("primary storage controller[%s] does not support encrypting volume bits in place", getIdentity()); comp.fail(new ErrorCode(SysErrors.UNIMPLEMENTED_OPERATION_ERROR.toString(), "unimplemented operation", details)); } + + /** + * Expands the target volume to the requested size. + * + * `@param` volume target volume inventory, including install path and volume metadata + * `@param` size requested virtual size in bytes + * `@param` comp completion notified with refreshed volume stats + */ void expandVolume(VolumeInventory volume, long size, ReturnValueCompletion<VolumeStats> comp);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/PrimaryStorageControllerSvc.java` around lines 38 - 49, `PrimaryStorageControllerSvc` 的公共契约方法缺少必要的 Javadoc，需要为 `encryptVolumeBits(...)`、`flattenVolume(...)`、`stats(...)`、`batchStats(...)` 和新增的 `expandVolume(...)` 补充注释，明确默认不支持时的行为、参数含义（尤其是 `VolumeInventory` 携带的路径/加密上下文）以及 `ReturnValueCompletion` 的返回语义；同时保持接口方法不使用多余修饰符，并确保注释能清晰说明这些 controller contract 的使用约束。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 `@plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.java`:
- Around line 660-688: Encrypted volume clone flow still falls through to the
normal resize path, so a LUKS-backed clone can be expanded with the generic ZBS
`ExpandVolumeCmd` instead of the encrypted resize logic. Update the
`dst.isEncrypted()` handling in `ZbsStorageController` so the
`cloneEncryptedVolumeFromImage` path either returns a `stats.getSize()` that
already satisfies `dst.getSize()`, or make the later resize flow dispatch to the
encrypted expansion extension point (`resizeEncryptedVolume`) when the
destination volume is encrypted. Use the existing `stats`, `dst`, and encrypted
clone/resize callbacks to keep the behavior consistent.
In
`@storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorageFactory.java`:
- Line 609: The `encrypted` field is currently just being forwarded in
`ExternalPrimaryStorageFactory` without being consumed in the
`ExternalPrimaryStorage.handle(CreateTemplateFromVolumeOnPrimaryStorageMsg)`
flow. Update the receiver side to either read this flag and pass the
encryption/decryption or key context into the subsequent
`DownloadImageFromRemoteTargetMsg`/export-import path, or add
validation/interceptor handling that explicitly consumes it so the value is not
a no-op.
---
Nitpick comments:
In
`@header/src/main/java/org/zstack/header/storage/addon/primary/PrimaryStorageControllerSvc.java`:
- Around line 38-49: `PrimaryStorageControllerSvc` 的公共契约方法缺少必要的 Javadoc，需要为
`encryptVolumeBits(...)`、`flattenVolume(...)`、`stats(...)`、`batchStats(...)`
和新增的 `expandVolume(...)` 补充注释，明确默认不支持时的行为、参数含义（尤其是 `VolumeInventory`
携带的路径/加密上下文）以及 `ReturnValueCompletion` 的返回语义；同时保持接口方法不使用多余修饰符，并确保注释能清晰说明这些
controller contract 的使用约束。
In
`@header/src/main/java/org/zstack/header/storage/addon/primary/ZbsVolumeEncryptionExtensionPoint.java`:
- Around line 9-29: Add Javadoc for ZbsVolumeEncryptionExtensionPoint and each
method to document the parameter order, expected lifecycle and idempotency of
VolumeCleanup.cleanup, and the success/failure contract for each
ReturnValueCompletion callback. Also remove any redundant interface method
modifiers if present, and ensure the nested VolumeCleanup contract is documented
clearly so implementers know when cleanup is safe to call and how completion
should signal errors.
🪄 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: 22f7b575-6a38-4ebb-bc18-dd05278e0268
📒 Files selected for processing (7)
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/ZbsVolumeEncryptionExtensionPoint.javaplugin/expon/src/main/java/org/zstack/expon/ExponStorageController.javaplugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.javastorage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.javastorage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorageFactory.java
🚧 Files skipped from review as they are similar to previous changes (1)
- header/src/main/java/org/zstack/header/storage/addon/primary/CreateVolumeSpec.java
| cmsg.setSnapshotUuid(paramIn.getSnapshot().getUuid()); | ||
| cmsg.setImageInventory(paramIn.getImage()); | ||
| cmsg.setBackupStorageUuid(paramIn.getBackupStorageUuid()); | ||
| cmsg.setEncrypted(paramIn.getSnapshot().getEncrypted()); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
encrypted 需要在接收端被消费，否则这里只是无效透传。
当前提供的 ExternalPrimaryStorage.handle(CreateTemplateFromVolumeOnPrimaryStorageMsg) 只用 snapshot path 执行 export/import，没有读取该 encrypted 标志，也没有把加密上下文传给后续 DownloadImageFromRemoteTargetMsg。加密快照创建模板仍会走未感知加密的路径；请在接收端根据该字段接入解密/转换/密钥上下文，或补充验证该字段确实由其他拦截器消费。
🤖 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/ExternalPrimaryStorageFactory.java`
at line 609, The `encrypted` field is currently just being forwarded in
`ExternalPrimaryStorageFactory` without being consumed in the
`ExternalPrimaryStorage.handle(CreateTemplateFromVolumeOnPrimaryStorageMsg)`
flow. Update the receiver side to either read this flag and pass the
encryption/decryption or key context into the subsequent
`DownloadImageFromRemoteTargetMsg`/export-import path, or add
validation/interceptor handling that explicitly consumes it so the value is not
a no-op.
21be543 to
3903193
Compare
Owner
|
Comment from yaohua.wu: Review: MR !10327 — ZSV-12579Background
关联 MR
Cross-Repo Findings
zstack-side Review
Coverage
Verdict: REVISION_REQUIREDzstack 侧控制面可以和 utility endpoint 对齐，但该组 MR 仍需先修复 utility 侧的 CBD XML 重复 encryption 注入后再合入。 🤖 Robot Reviewer |
MatheMatrix
force-pushed
the
sync/zstackio/zbs-volume-encryption@@3
branch
6 times, most recently
from
81e7b4e to
53d20b6
Compare
added 3 commits
July 4, 2026 17:42
Resolves: ZSV-12469 Change-Id: I5ed4f9f00e700e4818a5c440b6dc5b04edee9c79
Resolves: ZSV-12577 Change-Id: I224e974f90f555267e06174179023c449eae1cd3
Support ZBS encrypted volume creation without relying on ZBS CLI on compute hosts, and keep the ZBS controller as a thin dispatcher to encryption extensions. Tests: git diff --check. Resolves: ZSV-12579 Change-Id: I2676f28a78f00a85c85847494703ba0eb4708547
MatheMatrix
force-pushed
the
sync/zstackio/zbs-volume-encryption@@3
branch
from
53d20b6 to
7cb765d
Compare
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
Implement ZBS encrypted volume support aligned with LocalStorage, NFS, SharedBlock, and RBD behavior.
Changes
Testing
Resolves: ZSV-12579
sync from gitlab !10327