<feature>[zbs]: ZSV-12579 support zbs volume encryption#4366
<feature>[zbs]: ZSV-12579 support zbs volume encryption#4366ZStack-Robot wants to merge 1 commit into
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 41 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 (26)
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 |
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
| 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.
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
d9afe36 to
cb12c2a
Compare
21be543 to
3903193
Compare
|
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 |
6e6eb2e to
af93180
Compare
Add ZBS encrypted volume handling for create, clone, snapshot restore, template consumption, resize, lifecycle, trash restore, image change, reimage, and VM clone flows. Move ZBS-specific orchestration into storage encrypt extensions while keeping the ZBS controller as a dispatcher. Tests: git diff --check. Resolves: ZSV-12579 Change-Id: I2676f28a78f00a85c85847494703ba0eb4708547
af93180 to
001b74a
Compare
Summary
Implement ZBS encrypted volume support aligned with LocalStorage, NFS, SharedBlock, and RBD behavior.
Changes
Testing
Resolves: ZSV-12579
sync from gitlab !10327