feat(storage): local primary LUKS qcow2 creation only#4079
Open
zstack-robot-1 wants to merge 1 commit into
Open
feat(storage): local primary LUKS qcow2 creation only#4079zstack-robot-1 wants to merge 1 commit into
zstack-robot-1 wants to merge 1 commit into
Conversation
Walkthrough该 PR 为卷级 LUKS 加密添加端到端支持：数据库与实体字段、消息/DTO 扩展、密钥物化与宿主 secret 准备、KVM agent 与主存 LUKS 操作、快照加密继承、就地加密流程与 VM 启动时 secret 注入。 Changes卷加密端到端
Estimated code review effort: 🎯 4 (Complex) | ⏱️ ~75 minutes
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugin/ceph/src/main/java/org/zstack/storage/ceph/primary/CephPrimaryStorageBase.java (1)
3076-3105:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
FAST_CREATE分支遗漏了加密 snapshot cache 的临时密钥登记。新增逻辑只在 Line 3104 的非
FAST_CREATE分支给DownloadToCache.encrypted赋值；当useSnapshotAsCache == true且msg.getEncrypted() == true时，这里直接持久化 image cache，不会执行prepareTemporarySnapshotImageSecretMaterial()。后面的createVolumeFromTemplate()又依赖hasTemporarySnapshotImageKey(cache.getImageUuid())判断是否走加密 snapshot-cache 分支，因此这条路径会把加密缓存误判成普通缓存。🤖 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 3076 - 3105, The FAST_CREATE branch saves a temporary ImageCacheVO via createTemporaryImageCacheFromVolumeSnapshot without registering the temporary encryption key, so encrypted snapshots skip prepareTemporarySnapshotImageSecretMaterial() and later hasTemporarySnapshotImageKey(imageUuid) returns false; fix by detecting msg.getEncrypted()==true inside the useSnapshotAsCache branch (where ProtectSnapshotCmd/ProtectSnapshotRsp and createTemporaryImageCacheFromVolumeSnapshot are used), call the same secret setup logic as the DownloadToCache path (invoke prepareTemporarySnapshotImageSecretMaterial for the new cache's image UUID and persist any secret metadata) before dbf.persist(cache) so createVolumeFromTemplate will see hasTemporarySnapshotImageKey(cache.getImageUuid()) as true.
🧹 Nitpick comments (6)
storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedAttachExtension.java (1)
47-63: ⚡ Quick win建议增加防御性检查与日志记录。
当前代码在以下场景缺少必要的错误处理：
- Line 55：
resolveOrDefineSecretForVolume可能返回 null 或空字符串，但未进行验证。如果返回值为空，后续 attach 操作会因缺少 secret UUID 而失败，但问题发生在 agent 侧，难以排查。- Lines 56-59：当
cmd.getVolume()为 null 时静默跳过设置逻辑，缺少警告日志。建议改进：
🛡️ 建议增加验证与日志
String hostUuid = host.getUuid(); String vmUuid = vm.getUuid(); String volUuid = volume.getUuid(); String secretUuid = secretHelper.resolveOrDefineSecretForVolume(hostUuid, vmUuid, volUuid); + + if (StringUtils.isBlank(secretUuid)) { + logger.warn(String.format( + "failed to resolve LUKS secret for volume[uuid:%s] on host[uuid:%s], attach may fail", + volUuid, hostUuid)); + return; + } + VolumeTO to = cmd.getVolume(); if (to != null) { to.setLuksSecretUuid(secretUuid); + } else { + logger.warn(String.format( + "cmd.getVolume() is null for encrypted volume[uuid:%s], cannot set luksSecretUuid", + volUuid)); } logger.debug(String.format( "LUKS-ATTACH-EXT stamped secret %s onto attach cmd for volume[uuid:%s] on host[uuid:%s]", secretUuid, volUuid, 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 `@storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedAttachExtension.java` around lines 47 - 63, In beforeAttachVolume, add defensive checks and logging around secret resolution and the attach cmd: after calling secretHelper.resolveOrDefineSecretForVolume(hostUuid, vmUuid, volUuid) validate that secretUuid is not null/empty and if it is, log an error (including hostUuid, vmUuid, volUuid) and early-return or raise an appropriate exception to prevent a broken attach; also if cmd.getVolume() returns null, log a warning (including hostUuid, vmUuid, volUuid) instead of silently skipping so callers can diagnose missing VolumeTO; keep the existing logger.debug when a valid secret is stamped.plugin/kvm/src/main/java/org/zstack/kvm/KVMAgentCommands.java (1)
488-488: ⚡ Quick win建议为敏感字段添加
@NoLogging注解。
encryptedDek字段包含加密后的 DEK（数据加密密钥）的 Base64 封装，虽然已经加密，但仍属于敏感数据。为了增强纵深防御和保持与项目中其他敏感字段的一致性（如 line 2021 的consolePassword、line 2516 的chapPassword），建议添加@NoLogging注解以防止该字段在日志中被意外输出。🔒 建议的安全加固
public static class SecretHostEnsureLuksSecretFileCmd extends AgentCommand { + `@NoLogging` private String 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 `@plugin/kvm/src/main/java/org/zstack/kvm/KVMAgentCommands.java` at line 488, 为防止加密后的 DEK 在日志中泄露，请在 KVMAgentCommands 类中为字段 encryptedDek 添加 `@NoLogging` 注解；定位到声明 "private String encryptedDek;" 并在其上方加注解，并确保引入或使用与项目中已有敏感字段（如 consolePassword、chapPassword）相同的 `@NoLogging` 注解类型与包名以保持一致性和编译通过。storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedResourceKeyBackend.java (1)
25-25: ⚡ Quick win补充接口方法的 Javadoc 注释。
以下接口方法缺少 Javadoc 文档说明：
detachKeyProviderFromTemporarySnapshotImage(line 25)checkSnapshotKeyProviderAttached(line 37)checkTemporarySnapshotImageKeyProviderAttached(line 39)copyVolumeKeyToSnapshot(line 41)copySnapshotKeyToVolume(line 43)copySnapshotKeyToTemporarySnapshotImage(line 45)copyTemporarySnapshotImageKeyToVolume(line 47)findKeyProviderUuidBySnapshot(line 54)findKeyProviderUuidByTemporarySnapshotImage(line 56)findKeyVersionBySnapshot(line 70)根据编码规范第 3 节要求："接口方法不应有多余的修饰符（例如
public），且必须配有有效的 Javadoc 注释。"建议为这些方法添加清晰的 Javadoc 说明，包括参数含义、返回值说明以及可能的异常情况。Also applies to: 37-39, 41-47, 54-54, 56-56, 70-70
🤖 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/VolumeEncryptedResourceKeyBackend.java` at line 25, 为接口 VolumeEncryptedResourceKeyBackend 中列出的方法补充完整的 Javadoc 注释：为 detachKeyProviderFromTemporarySnapshotImage、checkSnapshotKeyProviderAttached、checkTemporarySnapshotImageKeyProviderAttached、copyVolumeKeyToSnapshot、copySnapshotKeyToVolume、copySnapshotKeyToTemporarySnapshotImage、copyTemporarySnapshotImageKeyToVolume、findKeyProviderUuidBySnapshot、findKeyProviderUuidByTemporarySnapshotImage、findKeyVersionBySnapshot 各方法添加说明性注释，描述方法的用途、每个参数含义（例如 imageUuid、snapshotUuid、volumeUuid、keyVersion 等）、返回值含义（若返回 Optional/nullable/UUID/string/boolean 等需明确）、以及可能抛出的异常或错误情况；确保注释遵循项目编码规范（接口方法不加多余修饰符），并放在方法声明上方以便生成文档。storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedSecretHelper.java (1)
236-297: 💤 Low valueJavadoc 与方法实现顺序不一致。
Lines 236-253 的 javadoc 描述了
resolveOrDefineSecretForVolume方法（"One-shot resolver"），但该方法的实际实现在 lines 283-297。中间插入了deleteSecretOnHostBestEffort的 javadoc（lines 254-261）和实现（lines 262-281）。这导致
resolveOrDefineSecretForVolume方法没有直接关联的 javadoc，而 lines 236-253 的注释变成了孤立的文档。建议调整方法顺序或移动 javadoc，使其紧邻对应的方法实现：
建议的调整方案
- /** - * One-shot resolver: "give me the libvirt secret UUID for this encrypted - * volume on this host". Used by both the start-vm path - * ({`@link` VolumeEncryptedStartExtension}) and the attach-data-volume - * path ({`@link` VolumeEncryptedAttachExtension}). - * - * <p>Order: - * <ol> - * <li>{`@code` findKeyVersionByVolume} — fail loudly if the volume has - * no {`@code` EncryptedResourceKeyRefVO} binding (would otherwise - * produce an unreadable qcow2 once attached).</li> - * <li>{`@link` `#getSecretOnHost`} — fast path, hits when the secret has - * already been defined on the host.</li> - * <li>{`@link` `#defineSecretFromBinding`} — fall back when the secret - * value is missing on the host (first attach, libvirtd restart, - * host reboot, migrate to a fresh host).</li> - * </ol> - */ /** * Best-effort delete of a single per-volume libvirt secret on {`@code` hostUuid}. * ... */ public void deleteSecretOnHostBestEffort(...) { ... } + /** + * One-shot resolver: "give me the libvirt secret UUID for this encrypted + * volume on this host". ... + */ public String resolveOrDefineSecretForVolume(...) { ... }🤖 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/VolumeEncryptedSecretHelper.java` around lines 236 - 297, The Javadoc for the "One-shot resolver" (lines describing findKeyVersionByVolume/getSecretOnHost/defineSecretFromBinding) is misplaced above deleteSecretOnHostBestEffort; move that Javadoc so it directly precedes the resolveOrDefineSecretForVolume method (or swap the positions of resolveOrDefineSecretForVolume and deleteSecretOnHostBestEffort) so the comment documents resolveOrDefineSecretForVolume; ensure deleteSecretOnHostBestEffort keeps its own Javadoc immediately above its implementation and update any reference tags if necessary.storage/src/main/java/org/zstack/storage/encrypt/VolumeSnapshotEncryptionExtension.java (1)
158-163: 💤 Low value
snapshot空指针检查后仍在错误消息中引用可能导致 NPE。在 Line 145 已经检查了
snapshot == null并提前返回，但在 Line 161-162 的 catch 块中，snapshot仍可能为null（如果findVolume抛出异常）。由于 Line 145 的检查，实际执行到 catch 块时snapshot不会是null，但代码结构不够清晰。当前实现在
findVolume抛出异常时，catch 块中的snapshot == null ? null : snapshot.getUuid()三元表达式是安全的，但建议统一错误处理逻辑。🤖 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/VolumeSnapshotEncryptionExtension.java` around lines 158 - 163, The catch block in VolumeSnapshotEncryptionExtension that calls completion.fail(...) references snapshot (via snapshot == null ? null : snapshot.getUuid()) which is confusing given earlier null-return logic and risks NPE if findVolume throws; refactor the method to compute a stable snapshot identifier before the try/catch (e.g. capture snapshotUuid from snapshot when it is safely available) or else build the error message without directly accessing snapshot inside the catch; update the RuntimeException catch to use that precomputed snapshotUuid (or a fixed "unknown" placeholder) and call completion.fail(...) with that value so the error handling no longer reads snapshot inside the exception handler.plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.java (1)
4068-4074: 💤 Low value错误链处理不一致。
当前代码将
errorCode作为字符串格式参数传递，这会丢失错误码的结构信息（如 code、details 等）。文件中其他类似处理（如第 2424 行）使用.causedBy(errorCode)来保持错误链的完整性。建议修改
`@Override` public void fail(ErrorCode errorCode) { - completion.fail(operr("failed to encrypt volume[uuid:%s] bits at path[%s] on host[uuid:%s]: %s", - msg.getVolumeUuid(), msg.getInstallPath(), msg.getHostUuid(), errorCode)); + completion.fail(operr("failed to encrypt volume[uuid:%s] bits at path[%s] on host[uuid:%s]", + msg.getVolumeUuid(), msg.getInstallPath(), msg.getHostUuid()).causedBy(errorCode)); }🤖 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/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.java` around lines 4068 - 4074, In LocalStorageKvmBackend's anonymous fail(ErrorCode) handler, stop interpolating the ErrorCode into the formatted string and instead attach it to the created ErrorCode via causedBy to preserve the error chain; replace the current completion.fail(operr("failed to encrypt volume[uuid:%s] bits at path[%s] on host[uuid:%s]: %s", msg.getVolumeUuid(), msg.getInstallPath(), msg.getHostUuid(), errorCode)) call in the fail method with a completion.fail(operr("failed to encrypt volume[uuid:%s] bits at path[%s] on host[uuid:%s]", msg.getVolumeUuid(), msg.getInstallPath(), msg.getHostUuid()).causedBy(errorCode)) so the original errorCode (with code/details) is preserved — this change should be made in the anonymous class implementing fail(ErrorCode errorCode) in LocalStorageKvmBackend.
🤖 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 `@compute/src/main/java/org/zstack/compute/vm/VmInstanceUtils.java`:
- Around line 146-208: The current logic unconditionally writes encrypted=false
because FORCE_ENCRYPT_VOLUME_HARDCODED is set to false and
applyForceEncryptEnvOverride always sets encrypted to forceOn; change
applyForceEncryptEnvOverride/CreateVmInstanceMsg handling so it only forces
encryption when isForceEncryptVolume() returns true and otherwise leaves
DiskAO.encrypted untouched (do not overwrite true->false). Update
applyForceEncryptEnvOverride (references: applyForceEncryptEnvOverride,
isForceEncryptVolume, FORCE_ENCRYPT_VOLUME_HARDCODED,
FORCE_ENCRYPT_VOLUME_PROPERTY, FORCE_ENCRYPT_VOLUME_ENV,
CreateVmInstanceMsg.getRootDisk, getDataDisks, getDeprecatedDataVolumeSpecs,
DiskAO.setEncrypted) to only setEncrypted(true) when forceOn is true and avoid
setting setEncrypted(false).
In `@compute/src/main/java/org/zstack/compute/vm/VmInstantiateOtherDiskFlow.java`:
- Around line 89-90: The rollback currently deletes any non-null volumeInventory
which may be an external existing volume passed in; modify
VmInstantiateOtherDiskFlow to distinguish volumes created by this flow vs
externally provided ones (e.g., introduce a boolean flag like createdByThisFlow
or a separate createdVolumeInventory reference set when the flow creates a new
volume in methods that allocate volumes), ensure
setupEncryptExistingVolumeFlow() and setupAttachVolumeFlows() paths do not mark
externally passed-in volumeInventory as created, and change rollback() to only
send DeleteVolumeMsg for volumes marked as created by this flow (leave external
volumeInventory untouched).
In `@header/src/main/java/org/zstack/header/volume/VolumeLuksAgentSpec.java`:
- Around line 11-14: The isComplete() logic in VolumeLuksAgentSpec is wrong: it
currently returns true if either encryptSecretUuid or
encryptLuksSecretMaterialFilePath is non-empty; change it to require both fields
be present. Update the isComplete() method to use AND semantics so it only
returns true when encryptSecretUuid is non-null/non-empty AND
encryptLuksSecretMaterialFilePath is non-null/non-empty, referencing the
encryptSecretUuid and encryptLuksSecretMaterialFilePath fields in
VolumeLuksAgentSpec.
In
`@plugin/ceph/src/main/java/org/zstack/storage/ceph/primary/CephPrimaryStorageBase.java`:
- Around line 5442-5455: The method findConnectedHostForCephLuks() currently
returns any Connected PrimaryStorageHostRefVO host which may not be a KVM host;
update it to ensure the returned host is a KVM hypervisor by joining/adding a
filter on HostVO_.hypervisorType == KVMConstant.KVM_HYPERVISOR_TYPE (or
alternatively change the API so callers supply the target KVM host explicitly).
Locate findConnectedHostForCephLuks and modify the query to only select hostUuid
whose HostVO.hypervisorType equals KVMConstant.KVM_HYPERVISOR_TYPE (or add an
overload/parameter to accept a hostUuid from the caller) so subsequent
KVMHostAsyncHttpCallMsg sends always target a KVM host.
- Around line 5760-5777: Remove the merge conflict markers and drop the old
SimpleQuery branch; keep the new logic that looks up the VolumeVO via
dbf.findByUuid(sp.getVolumeUuid(), VolumeVO.class), calls
snapshotEncryptionHelper.inheritVolumeKeyToSnapshot(volume, sp) when volume !=
null && volume.isEncrypted(), and then obtains volumePath with
Q.New(VolumeVO.class).select(VolumeVO_.installPath).eq(VolumeVO_.uuid,
sp.getVolumeUuid()).findValue(); ensure there are no leftover <<<<<<< / =======
/ >>>>>>> tokens and that the variables (volume, volumePath, sp) are used
consistently.
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java`:
- Around line 5434-5441: The code generates an envelope using
getSelf().getUuid() but later sends the command to msg.getHostUuid(), which can
cause “encrypt-for-A, send-to-B” failures; in KVMHost, validate that
msg.getHostUuid() equals getSelf().getUuid() (or if blank handle as error), then
use that single hostUuid consistently when calling
HostKeyIdentityHelper.getHostKeyIdentity(dbf, hostUuid) and when sending the
message; apply the same equality check and unified hostUuid usage in the other
occurrence around the 5512-5514 block so envelope generation and message
delivery always target the same host.
- Around line 5509-5514: The async HTTP call is ignoring the per-message timeout
because executeAsyncHttpCall()/Http.call() use the default asyncJsonPost(...)
overload rather than honoring KVMHostAsyncHttpCallMsg.getTimeout(); update the
call site so that executeAsyncHttpCall()/Http.call() reads kmsg.getTimeout() and
invokes the asyncJsonPost overload (or equivalent) that accepts a timeout, or
propagate kmsg.getTimeout() into the HTTP invocation path (e.g.
Http.asyncJsonPost(..., timeoutMs)) so the LUKS secret file write uses
KVMConstant.ENVELOPE_KEY_HTTP_TIMEOUT_SEC as intended; adjust KVMHost.java
handling of KVMHostAsyncHttpCallMsg and the
Http.executeAsyncHttpCall()/Http.call() implementation to pass and use the
timeout parameter.
In
`@storage/src/main/java/org/zstack/storage/encrypt/VolumeSnapshotEncryptionHelper.java`:
- Around line 254-280: The code in VolumeSnapshotEncryptionHelper is using
encryptedResourceKeyManager.getOrCreateKey with an asynchronous
ReturnValueCompletion and then immediately reads resultRef/errorRef (same race
as in materializeVolumeKey); replace this pattern by invoking a synchronous API
or blocking until callback completes (e.g., use a CountDownLatch/Future or reuse
the synchronous helper used by materializeVolumeKey) so that
encryptedResourceKeyManager.getOrCreateKey finishes before checking
resultRef/errorRef; update the call site referencing
encryptedResourceKeyManager.getOrCreateKey, the local resultRef/errorRef arrays
and the EncryptedResourceKeyManager.ResourceKeyResult handling to use the
blocking/waiting approach or the existing synchronous method to eliminate the
race.
- Around line 179-183: The call to encryptedResourceKeyManager.getKey(ctx) can
return null causing a NPE when accessing keyResult.getDekBase64(); update
VolumeSnapshotEncryptionHelper to null-check the result of
EncryptedResourceKeyManager.getKey (the ResourceKeyResult returned) before using
it, and if null throw an OperationFailureException (using operr) that includes
identifying context such as the volume UUID and the key manager class/name;
ensure subsequent checks (e.g. StringUtils.isBlank(keyResult.getDekBase64()))
only run after the null check and include a clear log or error message.
- Around line 232-237: The call to encryptedResourceKeyManager.getKey(ctx) in
VolumeSnapshotEncryptionHelper may return null and currently keyResult is used
without a null check, risking an NPE; add a null-check immediately after
obtaining keyResult (the result of EncryptedResourceKeyManager.getKey) and if
null throw an OperationFailureException (similar to the existing check at Line
179) with a descriptive message indicating the key manager returned null for the
encrypted temporary snapshot image (include imageUuid), then proceed to validate
keyResult.getDekBase64() as before.
- Around line 129-150: materializeVolumeKey currently assumes
EncryptedResourceKeyManager#getOrCreateKey invokes the completion synchronously
and only checks errorRef[0], which can return a null ResourceKeyResult or unset
dek; change materializeVolumeKey to wait for the async callback (e.g., use a
CountDownLatch or CompletableFuture) or refactor to accept a completion callback
so you don't rely on synchronous behavior, then after the callback completes
validate the EncryptedResourceKeyManager.ResourceKeyResult (not null) and its
dekBase64 (not empty) and throw an OperationFailureException with the errorRef
or a clear message if validation fails; mirror the defensive checks used in
createTemporarySnapshotImageKey and ensure both success and fail branches set
the result/error before unblocking.
---
Outside diff comments:
In
`@plugin/ceph/src/main/java/org/zstack/storage/ceph/primary/CephPrimaryStorageBase.java`:
- Around line 3076-3105: The FAST_CREATE branch saves a temporary ImageCacheVO
via createTemporaryImageCacheFromVolumeSnapshot without registering the
temporary encryption key, so encrypted snapshots skip
prepareTemporarySnapshotImageSecretMaterial() and later
hasTemporarySnapshotImageKey(imageUuid) returns false; fix by detecting
msg.getEncrypted()==true inside the useSnapshotAsCache branch (where
ProtectSnapshotCmd/ProtectSnapshotRsp and
createTemporaryImageCacheFromVolumeSnapshot are used), call the same secret
setup logic as the DownloadToCache path (invoke
prepareTemporarySnapshotImageSecretMaterial for the new cache's image UUID and
persist any secret metadata) before dbf.persist(cache) so
createVolumeFromTemplate will see
hasTemporarySnapshotImageKey(cache.getImageUuid()) as true.
---
Nitpick comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMAgentCommands.java`:
- Line 488: 为防止加密后的 DEK 在日志中泄露，请在 KVMAgentCommands 类中为字段 encryptedDek 添加
`@NoLogging` 注解；定位到声明 "private String encryptedDek;" 并在其上方加注解，并确保引入或使用与项目中已有敏感字段（如
consolePassword、chapPassword）相同的 `@NoLogging` 注解类型与包名以保持一致性和编译通过。
In
`@plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.java`:
- Around line 4068-4074: In LocalStorageKvmBackend's anonymous fail(ErrorCode)
handler, stop interpolating the ErrorCode into the formatted string and instead
attach it to the created ErrorCode via causedBy to preserve the error chain;
replace the current completion.fail(operr("failed to encrypt volume[uuid:%s]
bits at path[%s] on host[uuid:%s]: %s", msg.getVolumeUuid(),
msg.getInstallPath(), msg.getHostUuid(), errorCode)) call in the fail method
with a completion.fail(operr("failed to encrypt volume[uuid:%s] bits at path[%s]
on host[uuid:%s]", msg.getVolumeUuid(), msg.getInstallPath(),
msg.getHostUuid()).causedBy(errorCode)) so the original errorCode (with
code/details) is preserved — this change should be made in the anonymous class
implementing fail(ErrorCode errorCode) in LocalStorageKvmBackend.
In
`@storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedAttachExtension.java`:
- Around line 47-63: In beforeAttachVolume, add defensive checks and logging
around secret resolution and the attach cmd: after calling
secretHelper.resolveOrDefineSecretForVolume(hostUuid, vmUuid, volUuid) validate
that secretUuid is not null/empty and if it is, log an error (including
hostUuid, vmUuid, volUuid) and early-return or raise an appropriate exception to
prevent a broken attach; also if cmd.getVolume() returns null, log a warning
(including hostUuid, vmUuid, volUuid) instead of silently skipping so callers
can diagnose missing VolumeTO; keep the existing logger.debug when a valid
secret is stamped.
In
`@storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedResourceKeyBackend.java`:
- Line 25: 为接口 VolumeEncryptedResourceKeyBackend 中列出的方法补充完整的 Javadoc 注释：为
detachKeyProviderFromTemporarySnapshotImage、checkSnapshotKeyProviderAttached、checkTemporarySnapshotImageKeyProviderAttached、copyVolumeKeyToSnapshot、copySnapshotKeyToVolume、copySnapshotKeyToTemporarySnapshotImage、copyTemporarySnapshotImageKeyToVolume、findKeyProviderUuidBySnapshot、findKeyProviderUuidByTemporarySnapshotImage、findKeyVersionBySnapshot
各方法添加说明性注释，描述方法的用途、每个参数含义（例如 imageUuid、snapshotUuid、volumeUuid、keyVersion
等）、返回值含义（若返回 Optional/nullable/UUID/string/boolean
等需明确）、以及可能抛出的异常或错误情况；确保注释遵循项目编码规范（接口方法不加多余修饰符），并放在方法声明上方以便生成文档。
In
`@storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedSecretHelper.java`:
- Around line 236-297: The Javadoc for the "One-shot resolver" (lines describing
findKeyVersionByVolume/getSecretOnHost/defineSecretFromBinding) is misplaced
above deleteSecretOnHostBestEffort; move that Javadoc so it directly precedes
the resolveOrDefineSecretForVolume method (or swap the positions of
resolveOrDefineSecretForVolume and deleteSecretOnHostBestEffort) so the comment
documents resolveOrDefineSecretForVolume; ensure deleteSecretOnHostBestEffort
keeps its own Javadoc immediately above its implementation and update any
reference tags if necessary.
In
`@storage/src/main/java/org/zstack/storage/encrypt/VolumeSnapshotEncryptionExtension.java`:
- Around line 158-163: The catch block in VolumeSnapshotEncryptionExtension that
calls completion.fail(...) references snapshot (via snapshot == null ? null :
snapshot.getUuid()) which is confusing given earlier null-return logic and risks
NPE if findVolume throws; refactor the method to compute a stable snapshot
identifier before the try/catch (e.g. capture snapshotUuid from snapshot when it
is safely available) or else build the error message without directly accessing
snapshot inside the catch; update the RuntimeException catch to use that
precomputed snapshotUuid (or a fixed "unknown" placeholder) and call
completion.fail(...) with that value so the error handling no longer reads
snapshot inside the exception handler.
🪄 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: c69a06cd-f98c-4838-820a-fe6c85466b1d
⛔ Files ignored due to path filters (2)
conf/springConfigXml/VolumeManager.xmlis excluded by!**/*.xmlstorage/pom.xmlis excluded by!**/*.xml
📒 Files selected for processing (66)
compute/src/main/java/org/zstack/compute/vm/VmAllocateVolumeFlow.javacompute/src/main/java/org/zstack/compute/vm/VmInstanceUtils.javacompute/src/main/java/org/zstack/compute/vm/VmInstantiateOtherDiskFlow.javaconf/db/zsv/V5.1.0__schema.sqlheader/src/main/java/org/zstack/header/image/CreateTemporaryRootVolumeTemplateFromVolumeSnapshotMsg.javaheader/src/main/java/org/zstack/header/secret/SecretHostEnsureLuksSecretFileMsg.javaheader/src/main/java/org/zstack/header/secret/SecretHostEnsureLuksSecretFileReply.javaheader/src/main/java/org/zstack/header/secret/SecretHostGetReply.javaheader/src/main/java/org/zstack/header/storage/primary/CreateImageCacheFromVolumeSnapshotOnPrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/storage/primary/CreateVolumeFromVolumeSnapshotOnPrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/storage/primary/EncryptVolumeBitsOnPrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/storage/primary/EncryptVolumeBitsOnPrimaryStorageReply.javaheader/src/main/java/org/zstack/header/storage/primary/InstantiateVolumeOnPrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/storage/snapshot/CreateImageCacheFromVolumeSnapshotMsg.javaheader/src/main/java/org/zstack/header/storage/snapshot/TakeSnapshotsOnKvmJobStruct.javaheader/src/main/java/org/zstack/header/storage/snapshot/VolumeSnapshotAO.javaheader/src/main/java/org/zstack/header/storage/snapshot/VolumeSnapshotAO_.javaheader/src/main/java/org/zstack/header/storage/snapshot/VolumeSnapshotInventory.javaheader/src/main/java/org/zstack/header/vm/APICreateVmInstanceFromVolumeSnapshotGroupMsg.javaheader/src/main/java/org/zstack/header/vm/DiskAO.javaheader/src/main/java/org/zstack/header/volume/CreateDataVolumeExtensionPoint.javaheader/src/main/java/org/zstack/header/volume/CreateDataVolumeFromVolumeSnapshotMsg.javaheader/src/main/java/org/zstack/header/volume/CreateDataVolumeFromVolumeTemplateMsg.javaheader/src/main/java/org/zstack/header/volume/CreateVolumeMsg.javaheader/src/main/java/org/zstack/header/volume/EncryptVolumeMsg.javaheader/src/main/java/org/zstack/header/volume/EncryptVolumeReply.javaheader/src/main/java/org/zstack/header/volume/InstantiateVolumeMsg.javaheader/src/main/java/org/zstack/header/volume/VolumeAO.javaheader/src/main/java/org/zstack/header/volume/VolumeAO_.javaheader/src/main/java/org/zstack/header/volume/VolumeCreateMessage.javaheader/src/main/java/org/zstack/header/volume/VolumeInventory.javaheader/src/main/java/org/zstack/header/volume/VolumeLuksAgentSpec.javaimage/src/main/java/org/zstack/image/ImageManagerImpl.javaplugin/ceph/src/main/java/org/zstack/storage/ceph/primary/CephPrimaryStorageBase.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMAgentCommands.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMConstant.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHost.javaplugin/kvm/src/main/java/org/zstack/kvm/VolumeTO.javaplugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmExtensions.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageBase.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageCreateEmptyVolumeMsg.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageFactory.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageHypervisorBackend.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmFactory.javaplugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsDownloadImageToCacheJob.javaplugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorage.javaplugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageBackend.javaplugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageKVMBackend.javaplugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageKVMBackendCommands.javastorage/src/main/java/org/zstack/storage/encrypt/DummyVolumeEncryptedResourceKeyBackend.javastorage/src/main/java/org/zstack/storage/encrypt/SnapshotGroupRevertVolumeEncryptionHelper.javastorage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedAttachExtension.javastorage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedExpungeExtension.javastorage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedInitialExtension.javastorage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedResourceKeyBackend.javastorage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedSecretHelper.javastorage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedStartExtension.javastorage/src/main/java/org/zstack/storage/encrypt/VolumeSnapshotEncryptionExtension.javastorage/src/main/java/org/zstack/storage/encrypt/VolumeSnapshotEncryptionHelper.javastorage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotManagerImpl.javastorage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotTreeBase.javastorage/src/main/java/org/zstack/storage/volume/VolumeBase.javastorage/src/main/java/org/zstack/storage/volume/VolumeInPlaceEncryptor.javastorage/src/main/java/org/zstack/storage/volume/VolumeManagerImpl.javastorage/src/main/java/org/zstack/storage/volume/VolumeSystemTags.java
Comment on lines
+89
to
90
| setupEncryptExistingVolumeFlow(); | ||
| setupAttachVolumeFlows(); |
There was a problem hiding this comment.
attach-existing 路径不能复用“创建失败就删卷”的回滚语义。
这里的 volumeInventory 指向的是调用方传入的现有数据卷，但当前类的 rollback() 只要看到 volumeInventory != null 就会发送 DeleteVolumeMsg。现在又在同一路径前面插入了原地加密流程，后续任一步失败都会把现有卷当成临时资源删掉，存在直接数据丢失风险。建议显式区分“本流程创建的卷”和“外部传入的卷”，只有前者才允许在回滚里删除。
🔧 修正方向
class VmInstantiateOtherDiskFlow implements Flow {
DiskAO diskAO;
VolumeInventory volumeInventory;
+ boolean deleteVolumeOnRollback;
...
CreateVolumeReply cr = reply.castReply();
volumeInventory = cr.getInventory();
+ deleteVolumeOnRollback = true;
innerTrigger.next();
...
volumeInventory = VolumeInventory.valueOf(vo);
+ deleteVolumeOnRollback = true;
innerTrigger.next();
...
public void rollback(FlowRollback chain, Map data) {
- if (volumeInventory == null) {
+ if (volumeInventory == null || !deleteVolumeOnRollback) {
chain.rollback();
return;
}Also applies to: 423-458
🤖 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 `@compute/src/main/java/org/zstack/compute/vm/VmInstantiateOtherDiskFlow.java`
around lines 89 - 90, The rollback currently deletes any non-null
volumeInventory which may be an external existing volume passed in; modify
VmInstantiateOtherDiskFlow to distinguish volumes created by this flow vs
externally provided ones (e.g., introduce a boolean flag like createdByThisFlow
or a separate createdVolumeInventory reference set when the flow creates a new
volume in methods that allocate volumes), ensure
setupEncryptExistingVolumeFlow() and setupAttachVolumeFlows() paths do not mark
externally passed-in volumeInventory as created, and change rollback() to only
send DeleteVolumeMsg for volumes marked as created by this flow (leave external
volumeInventory untouched).
Comment on lines
+11
to
+14
| public boolean isComplete() { | ||
| return (encryptSecretUuid != null && !encryptSecretUuid.isEmpty()) | ||
| || (encryptLuksSecretMaterialFilePath != null && !encryptLuksSecretMaterialFilePath.isEmpty()); | ||
| } |
There was a problem hiding this comment.
逻辑错误：isComplete() 应使用 AND 而非 OR
当前实现中，只要 encryptSecretUuid 或 encryptLuksSecretMaterialFilePath 任一字段非空即返回 true。对于一个"完整"的 LUKS 代理规格，通常需要两个字段都存在才能正常工作。建议改为 AND 逻辑。
🐛 建议修复
public boolean isComplete() {
- return (encryptSecretUuid != null && !encryptSecretUuid.isEmpty())
- || (encryptLuksSecretMaterialFilePath != null && !encryptLuksSecretMaterialFilePath.isEmpty());
+ return (encryptSecretUuid != null && !encryptSecretUuid.isEmpty())
+ && (encryptLuksSecretMaterialFilePath != null && !encryptLuksSecretMaterialFilePath.isEmpty());
}🤖 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/VolumeLuksAgentSpec.java`
around lines 11 - 14, The isComplete() logic in VolumeLuksAgentSpec is wrong: it
currently returns true if either encryptSecretUuid or
encryptLuksSecretMaterialFilePath is non-empty; change it to require both fields
be present. Update the isComplete() method to use AND semantics so it only
returns true when encryptSecretUuid is non-null/non-empty AND
encryptLuksSecretMaterialFilePath is non-null/non-empty, referencing the
encryptSecretUuid and encryptLuksSecretMaterialFilePath fields in
VolumeLuksAgentSpec.
Comment on lines
+5442
to
+5455
| private String findConnectedHostForCephLuks() { | ||
| String hostUuid = Q.New(PrimaryStorageHostRefVO.class) | ||
| .eq(PrimaryStorageHostRefVO_.primaryStorageUuid, self.getUuid()) | ||
| .eq(PrimaryStorageHostRefVO_.status, PrimaryStorageHostStatus.Connected) | ||
| .select(PrimaryStorageHostRefVO_.hostUuid) | ||
| .limit(1) | ||
| .findValue(); | ||
| if (StringUtils.isBlank(hostUuid)) { | ||
| throw new OperationFailureException(operr( | ||
| "cannot find a connected host attached to ceph primary storage[uuid:%s] to run LUKS RBD operation", | ||
| self.getUuid())); | ||
| } | ||
| return hostUuid; | ||
| } |
There was a problem hiding this comment.
findConnectedHostForCephLuks() 可能选到非 KVM host。
这里仅按 PrimaryStorageHostRefVO 取一个 Connected host，但后续固定发送的是 KVMHostAsyncHttpCallMsg。Ceph primary storage 可以挂到非 KVM cluster，而 attachHook() 也会给这些 host 建 ref，所以混挂场景下 LUKS clone/create/resize/encrypt 会直接打到不支持 kvmagent 的 host。建议至少按 HostVO_.hypervisorType == KVMConstant.KVM_HYPERVISOR_TYPE 过滤，或由调用方显式传入目标 KVM host。
🤖 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 5442 - 5455, The method findConnectedHostForCephLuks() currently
returns any Connected PrimaryStorageHostRefVO host which may not be a KVM host;
update it to ensure the returned host is a KVM hypervisor by joining/adding a
filter on HostVO_.hypervisorType == KVMConstant.KVM_HYPERVISOR_TYPE (or
alternatively change the API so callers supply the target KVM host explicitly).
Locate findConnectedHostForCephLuks and modify the query to only select hostUuid
whose HostVO.hypervisorType equals KVMConstant.KVM_HYPERVISOR_TYPE (or add an
overload/parameter to accept a hostUuid from the caller) so subsequent
KVMHostAsyncHttpCallMsg sends always target a KVM host.
Comment on lines
+129
to
+150
| final EncryptedResourceKeyManager.ResourceKeyResult[] resultRef = | ||
| new EncryptedResourceKeyManager.ResourceKeyResult[1]; | ||
| final org.zstack.header.errorcode.ErrorCode[] errorRef = | ||
| new org.zstack.header.errorcode.ErrorCode[1]; | ||
| encryptedResourceKeyManager.getOrCreateKey(ctx, | ||
| new org.zstack.header.core.ReturnValueCompletion<EncryptedResourceKeyManager.ResourceKeyResult>(null) { | ||
| @Override | ||
| public void success(EncryptedResourceKeyManager.ResourceKeyResult returnValue) { | ||
| resultRef[0] = returnValue; | ||
| } | ||
|
|
||
| @Override | ||
| public void fail(org.zstack.header.errorcode.ErrorCode errorCode) { | ||
| errorRef[0] = errorCode; | ||
| } | ||
| }); | ||
|
|
||
| if (errorRef[0] != null) { | ||
| throw new OperationFailureException(operr( | ||
| "failed to materialize encryption key for volume[uuid:%s]", volumeUuid).withCause(errorRef[0])); | ||
| } | ||
| return resultRef[0]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 搜索 EncryptedResourceKeyManager 的实现，确认 getOrCreateKey 是否同步执行
ast-grep --pattern 'class $_ implements EncryptedResourceKeyManager {
$$$
getOrCreateKey($_, $_) {
$$$
}
$$$
}'
rg -n "getOrCreateKey" --type java -A 10Repository: MatheMatrix/zstack
Length of output: 18602
🏁 Script executed:
#!/bin/bash
# 定位 EncryptedResourceKeyManager 的实现类，核对 getOrCreateKey 是否在返回前同步触发 completion
set -euo pipefail
echo "== Implementations =="
rg -n "implements\s+EncryptedResourceKeyManager" --type java -S .
echo "== Candidate impl names (EncryptedResourceKeyManagerImpl) =="
rg -n "class\s+EncryptedResourceKeyManagerImpl" --type java -S .
echo "== Direct getOrCreateKey implementations =="
rg -n "void\s+getOrCreateKey\s*\(" --type java -S header/src/main/java compute/src/main/java storage/src/main/java plugin/src/main/java .
echo "== Show EncryptedResourceKeyManagerImpl getOrCreateKey if found =="
# 若找到了 impl 类名，这里会输出附近代码；否则不会报错
fd -a -t f 'EncryptedResourceKeyManagerImpl*.java' . | head -n 20 | catRepository: MatheMatrix/zstack
Length of output: 327
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find getOrCreateKey method definitions =="
rg -n "getOrCreateKey\s*\(\s*GetOrCreateResourceKeyContext" --type java -S .
echo "== Find EncryptedResourceKeyManagerImpl string references =="
rg -n "EncryptedResourceKeyManagerImpl" --type java -S .
echo "== Inspect VolumeSnapshotEncryptionHelper around key materialize code (line ~110-210) =="
nl -ba storage/src/main/java/org/zstack/storage/encrypt/VolumeSnapshotEncryptionHelper.java | sed -n '100,220p'
echo "== Inspect VolumeSnapshotEncryptionHelper around createTemporarySnapshotImageKey (line ~230-330) =="
nl -ba storage/src/main/java/org/zstack/storage/encrypt/VolumeSnapshotEncryptionHelper.java | sed -n '220,330p'Repository: MatheMatrix/zstack
Length of output: 793
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== DummyEncryptedResourceKeyManager.getOrCreateKey snippet =="
sed -n '1,120p' compute/src/main/java/org/zstack/compute/vm/devices/DummyEncryptedResourceKeyManager.java
echo "== VolumeEncryptedSecretHelper sync comment context (lines ~40-120) =="
sed -n '35,120p' storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedSecretHelper.java
echo "== VolumeSnapshotEncryptionHelper: find materializeVolumeKey and getKey/getDekBase64 usages =="
rg -n "materializeVolumeKey|getKey\(|getDekBase64" storage/src/main/java/org/zstack/storage/encrypt/VolumeSnapshotEncryptionHelper.java
echo "== VolumeSnapshotEncryptionHelper code around getOrCreateKey usage (~110-200) =="
sed -n '90,190p' storage/src/main/java/org/zstack/storage/encrypt/VolumeSnapshotEncryptionHelper.java
echo "== VolumeSnapshotEncryptionHelper code around second getOrCreateKey usage (~230-310) =="
sed -n '220,310p' storage/src/main/java/org/zstack/storage/encrypt/VolumeSnapshotEncryptionHelper.javaRepository: MatheMatrix/zstack
Length of output: 15159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== EncryptedResourceKeyManager javadoc around getOrCreateKey/getKey =="
sed -n '1,120p' header/src/main/java/org/zstack/header/keyprovider/EncryptedResourceKeyManager.java
echo
echo "== VolumeSnapshotEncryptionHelper: inspect calls to materializeVolumeKey (~60-140) =="
sed -n '60,170p' storage/src/main/java/org/zstack/storage/encrypt/VolumeSnapshotEncryptionHelper.java
echo
echo "== VolumeSnapshotEncryptionHelper: inspect materializeVolumeKey (~110-210) =="
sed -n '110,230p' storage/src/main/java/org/zstack/storage/encrypt/VolumeSnapshotEncryptionHelper.javaRepository: MatheMatrix/zstack
Length of output: 16205
修复：getOrCreateKey 回调同步假设导致竞态风险，且 materializeVolumeKey 未校验返回结果
EncryptedResourceKeyManager#getOrCreateKey的接口契约未保证completion会在方法返回前触发；VolumeSnapshotEncryptionHelper#materializeVolumeKey调用后仅检查errorRef[0]就返回resultRef[0]，若回调未及时执行可能导致返回null并产生静默未物化加密密钥的问题。createTemporarySnapshotImageKey已校验resultRef[0] == null || dekBase64 为空，但materializeVolumeKey没有；需要补齐防御性校验，并在依赖“同步回调”前提时将契约/实现对齐，必要时加入同步等待机制。
🤖 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/VolumeSnapshotEncryptionHelper.java`
around lines 129 - 150, materializeVolumeKey currently assumes
EncryptedResourceKeyManager#getOrCreateKey invokes the completion synchronously
and only checks errorRef[0], which can return a null ResourceKeyResult or unset
dek; change materializeVolumeKey to wait for the async callback (e.g., use a
CountDownLatch or CompletableFuture) or refactor to accept a completion callback
so you don't rely on synchronous behavior, then after the callback completes
validate the EncryptedResourceKeyManager.ResourceKeyResult (not null) and its
dekBase64 (not empty) and throw an OperationFailureException with the errorRef
or a clear message if validation fails; mirror the defensive checks used in
createTemporarySnapshotImageKey and ensure both success and fail branches set
the result/error before unblocking.
Comment on lines
+179
to
+183
| EncryptedResourceKeyManager.ResourceKeyResult keyResult = encryptedResourceKeyManager.getKey(ctx); | ||
| if (StringUtils.isBlank(keyResult.getDekBase64())) { | ||
| throw new OperationFailureException(operr( | ||
| "key manager returned empty DEK for encrypted volume[uuid:%s]", volume.getUuid())); | ||
| } |
There was a problem hiding this comment.
getKey 返回值可能为 null，导致 NPE。
Line 179 调用 encryptedResourceKeyManager.getKey(ctx) 后，Line 180 直接调用 keyResult.getDekBase64()。如果 getKey 返回 null，将抛出 NullPointerException。
🛡️ 建议添加空值检查
EncryptedResourceKeyManager.ResourceKeyResult keyResult = encryptedResourceKeyManager.getKey(ctx);
+ if (keyResult == null) {
+ throw new OperationFailureException(operr(
+ "key manager returned null result for encrypted volume[uuid:%s]", volume.getUuid()));
+ }
if (StringUtils.isBlank(keyResult.getDekBase64())) {📝 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
| EncryptedResourceKeyManager.ResourceKeyResult keyResult = encryptedResourceKeyManager.getKey(ctx); | |
| if (StringUtils.isBlank(keyResult.getDekBase64())) { | |
| throw new OperationFailureException(operr( | |
| "key manager returned empty DEK for encrypted volume[uuid:%s]", volume.getUuid())); | |
| } | |
| EncryptedResourceKeyManager.ResourceKeyResult keyResult = encryptedResourceKeyManager.getKey(ctx); | |
| if (keyResult == null) { | |
| throw new OperationFailureException(operr( | |
| "key manager returned null result for encrypted volume[uuid:%s]", volume.getUuid())); | |
| } | |
| if (StringUtils.isBlank(keyResult.getDekBase64())) { | |
| throw new OperationFailureException(operr( | |
| "key manager returned empty DEK for encrypted volume[uuid:%s]", volume.getUuid())); | |
| } |
🤖 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/VolumeSnapshotEncryptionHelper.java`
around lines 179 - 183, The call to encryptedResourceKeyManager.getKey(ctx) can
return null causing a NPE when accessing keyResult.getDekBase64(); update
VolumeSnapshotEncryptionHelper to null-check the result of
EncryptedResourceKeyManager.getKey (the ResourceKeyResult returned) before using
it, and if null throw an OperationFailureException (using operr) that includes
identifying context such as the volume UUID and the key manager class/name;
ensure subsequent checks (e.g. StringUtils.isBlank(keyResult.getDekBase64()))
only run after the null check and include a clear log or error message.
Comment on lines
+232
to
+237
| EncryptedResourceKeyManager.ResourceKeyResult keyResult = encryptedResourceKeyManager.getKey(ctx); | ||
| if (StringUtils.isBlank(keyResult.getDekBase64())) { | ||
| throw new OperationFailureException(operr( | ||
| "key manager returned empty DEK for encrypted temporary snapshot image[uuid:%s]", imageUuid)); | ||
| } | ||
| return keyResult; |
There was a problem hiding this comment.
与 Line 179 相同的问题：getKey 返回值可能为 null。
Line 232 调用 encryptedResourceKeyManager.getKey(ctx) 后直接访问 keyResult.getDekBase64()，存在 NPE 风险。
🛡️ 建议添加空值检查
EncryptedResourceKeyManager.ResourceKeyResult keyResult = encryptedResourceKeyManager.getKey(ctx);
+ if (keyResult == null) {
+ throw new OperationFailureException(operr(
+ "key manager returned null result for encrypted temporary snapshot image[uuid:%s]", imageUuid));
+ }
if (StringUtils.isBlank(keyResult.getDekBase64())) {🤖 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/VolumeSnapshotEncryptionHelper.java`
around lines 232 - 237, The call to encryptedResourceKeyManager.getKey(ctx) in
VolumeSnapshotEncryptionHelper may return null and currently keyResult is used
without a null check, risking an NPE; add a null-check immediately after
obtaining keyResult (the result of EncryptedResourceKeyManager.getKey) and if
null throw an OperationFailureException (similar to the existing check at Line
179) with a descriptive message indicating the key manager returned null for the
encrypted temporary snapshot image (include imageUuid), then proceed to validate
keyResult.getDekBase64() as before.
Comment on lines
+254
to
+280
| final EncryptedResourceKeyManager.ResourceKeyResult[] resultRef = | ||
| new EncryptedResourceKeyManager.ResourceKeyResult[1]; | ||
| final org.zstack.header.errorcode.ErrorCode[] errorRef = | ||
| new org.zstack.header.errorcode.ErrorCode[1]; | ||
| encryptedResourceKeyManager.getOrCreateKey(ctx, | ||
| new org.zstack.header.core.ReturnValueCompletion<EncryptedResourceKeyManager.ResourceKeyResult>(null) { | ||
| @Override | ||
| public void success(EncryptedResourceKeyManager.ResourceKeyResult returnValue) { | ||
| resultRef[0] = returnValue; | ||
| } | ||
|
|
||
| @Override | ||
| public void fail(org.zstack.header.errorcode.ErrorCode errorCode) { | ||
| errorRef[0] = errorCode; | ||
| } | ||
| }); | ||
|
|
||
| if (errorRef[0] != null) { | ||
| throw new OperationFailureException(operr( | ||
| "failed to materialize encryption key for temporary snapshot image[uuid:%s]", | ||
| imageUuid).withCause(errorRef[0])); | ||
| } | ||
| if (resultRef[0] == null || StringUtils.isBlank(resultRef[0].getDekBase64())) { | ||
| throw new OperationFailureException(operr( | ||
| "key manager returned empty DEK for encrypted temporary snapshot image[uuid:%s]", imageUuid)); | ||
| } | ||
| return resultRef[0]; |
There was a problem hiding this comment.
与 materializeVolumeKey 相同的异步回调问题。
此方法也使用了相同的异步回调同步使用模式，存在相同的潜在竞态条件风险。
🤖 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/VolumeSnapshotEncryptionHelper.java`
around lines 254 - 280, The code in VolumeSnapshotEncryptionHelper is using
encryptedResourceKeyManager.getOrCreateKey with an asynchronous
ReturnValueCompletion and then immediately reads resultRef/errorRef (same race
as in materializeVolumeKey); replace this pattern by invoking a synchronous API
or blocking until callback completes (e.g., use a CountDownLatch/Future or reuse
the synchronous helper used by materializeVolumeKey) so that
encryptedResourceKeyManager.getOrCreateKey finishes before checking
resultRef/errorRef; update the call site referencing
encryptedResourceKeyManager.getOrCreateKey, the local resultRef/errorRef arrays
and the EncryptedResourceKeyManager.ResourceKeyResult handling to use the
blocking/waiting approach or the existing synchronous method to eliminate the
race.
MatheMatrix
force-pushed
the
sync/zstackio/encrypt-volume@@2
branch
3 times, most recently
from
911113a to
09d7474
Compare
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/VolumeManagerImpl.java (1)
584-590:⚠️ Potential issue | 🟠 Major | ⚡ Quick win模板建卷失败时也要走删除前清理扩展。
前面的新流程已经可能给 volume 建立加密关联；这里直接
removeByPrimaryKey()会跳过VolumeJustBeforeDeleteFromDbExtensionPoint，和本文件快照建卷失败分支的新处理不一致，失败后容易留下孤儿加密引用。🔧 可参考的修正
error(new FlowErrorHandler(msg) { `@Override` public void handle(ErrorCode errCode, Map data) { reply.setError(errCode); reply.setSuccess(false); - dbf.removeByPrimaryKey(vol.getUuid(), vol.getClass()); + VolumeVO vvo = dbf.reload(vol); + if (vvo != null) { + VolumeInventory inventory = VolumeInventory.valueOf(vvo); + CollectionUtils.safeForEach(pluginRgty.getExtensionList(VolumeJustBeforeDeleteFromDbExtensionPoint.class), + ext -> ext.volumeJustBeforeDeleteFromDb(inventory)); + dbf.remove(vvo); + } bus.reply(msg, reply); } });🤖 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/VolumeManagerImpl.java` around lines 584 - 590, The error handler in VolumeManagerImpl currently calls dbf.removeByPrimaryKey(vol.getUuid(), vol.getClass()) directly, which bypasses VolumeJustBeforeDeleteFromDbExtensionPoint and can leave orphaned encryption links; replace that direct DB removal with the same deletion/cleanup path used in the snapshot-build-failure branch so the VolumeJustBeforeDeleteFromDbExtensionPoint (and any other pre-delete extensions) run—i.e., invoke the existing volume deletion routine or helper method used elsewhere in this class that triggers pre-delete extension points (instead of calling dbf.removeByPrimaryKey directly in the FlowErrorHandler).
🧹 Nitpick comments (1)
storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedResourceKeyBackend.java (1)
25-58: ⚡ Quick win给剩余接口方法补齐 Javadoc。
这一组新增接口方法里还有多处无注释声明，接口契约现在不完整，后续实现和文档生成都容易产生歧义。
As per coding guidelines, “接口方法不应有多余的修饰符（例如 public），且必须配有有效的 Javadoc 注释。”
Also applies to: 72-72
🤖 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/VolumeEncryptedResourceKeyBackend.java` around lines 25 - 58, Add Javadoc comments for all undeclared methods in VolumeEncryptedResourceKeyBackend (e.g., detachKeyProviderFromTemporarySnapshotImage, findKeyProviderUuidByVolume, checkVolumeKeyProviderAttached, checkSnapshotKeyProviderAttached, checkTemporarySnapshotImageKeyProviderAttached, copyVolumeKeyToSnapshot, copySnapshotKeyToVolume, copyVolumeKeyToVolume, copySnapshotKeyToTemporarySnapshotImage, copyTemporarySnapshotImageKeyToVolume, defaultKeyProviderUuid, findKeyProviderUuidBySnapshot, findKeyProviderUuidByTemporarySnapshotImage) describing purpose, parameters (`@param`), return values (`@return`) and behavior for edge cases (e.g., when crypto is not installed or result is null); ensure you do not add visibility modifiers (keep them package-private interface methods) and follow existing style/format of surrounding Javadoc.
🤖 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/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.java`:
- Around line 1553-1555: 在 handle(CreateTemplateFromVolumeOnPrimaryStorageMsg)
分支里也需要把 LUKS secret material 的路径透传到 agent，当前只在 ImageCache.download() 中对
CreateTemplateFromVolumeCmd 调用了 cmd.setEncryptLuksSecretMaterialFilePath(...)，但
handle(CreateTemplateFromVolumeOnPrimaryStorageMsg) 发送同一命令时未设置该字段，导致加密 volume
直接创建模板时 agent 拿不到 secret。修复方法：在
handle(CreateTemplateFromVolumeOnPrimaryStorageMsg) 构造或发送
CreateTemplateFromVolumeCmd 时，将 encryptLuksSecretMaterialFilePath 值赋入（调用
cmd.setEncryptLuksSecretMaterialFilePath(encryptLuksSecretMaterialFilePath) 或从
msg/volume 对应字段取值并设置），确保字段与 ImageCache.download() 中的处理保持一致。
---
Outside diff comments:
In `@storage/src/main/java/org/zstack/storage/volume/VolumeManagerImpl.java`:
- Around line 584-590: The error handler in VolumeManagerImpl currently calls
dbf.removeByPrimaryKey(vol.getUuid(), vol.getClass()) directly, which bypasses
VolumeJustBeforeDeleteFromDbExtensionPoint and can leave orphaned encryption
links; replace that direct DB removal with the same deletion/cleanup path used
in the snapshot-build-failure branch so the
VolumeJustBeforeDeleteFromDbExtensionPoint (and any other pre-delete extensions)
run—i.e., invoke the existing volume deletion routine or helper method used
elsewhere in this class that triggers pre-delete extension points (instead of
calling dbf.removeByPrimaryKey directly in the FlowErrorHandler).
---
Nitpick comments:
In
`@storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedResourceKeyBackend.java`:
- Around line 25-58: Add Javadoc comments for all undeclared methods in
VolumeEncryptedResourceKeyBackend (e.g.,
detachKeyProviderFromTemporarySnapshotImage, findKeyProviderUuidByVolume,
checkVolumeKeyProviderAttached, checkSnapshotKeyProviderAttached,
checkTemporarySnapshotImageKeyProviderAttached, copyVolumeKeyToSnapshot,
copySnapshotKeyToVolume, copyVolumeKeyToVolume,
copySnapshotKeyToTemporarySnapshotImage, copyTemporarySnapshotImageKeyToVolume,
defaultKeyProviderUuid, findKeyProviderUuidBySnapshot,
findKeyProviderUuidByTemporarySnapshotImage) describing purpose, parameters
(`@param`), return values (`@return`) and behavior for edge cases (e.g., when crypto
is not installed or result is null); ensure you do not add visibility modifiers
(keep them package-private interface methods) and follow existing style/format
of surrounding Javadoc.
🪄 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: 4fc5dd20-9bbd-42f1-acb0-02098f4df61c
⛔ Files ignored due to path filters (2)
conf/springConfigXml/VolumeManager.xmlis excluded by!**/*.xmlstorage/pom.xmlis excluded by!**/*.xml
📒 Files selected for processing (66)
compute/src/main/java/org/zstack/compute/vm/VmAllocateVolumeFlow.javacompute/src/main/java/org/zstack/compute/vm/VmInstanceUtils.javacompute/src/main/java/org/zstack/compute/vm/VmInstantiateOtherDiskFlow.javaconf/db/zsv/V5.1.0__schema.sqlheader/src/main/java/org/zstack/header/image/CreateTemporaryRootVolumeTemplateFromVolumeSnapshotMsg.javaheader/src/main/java/org/zstack/header/secret/SecretHostEnsureLuksSecretFileMsg.javaheader/src/main/java/org/zstack/header/secret/SecretHostEnsureLuksSecretFileReply.javaheader/src/main/java/org/zstack/header/secret/SecretHostGetReply.javaheader/src/main/java/org/zstack/header/storage/primary/CreateImageCacheFromVolumeSnapshotOnPrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/storage/primary/CreateVolumeFromVolumeSnapshotOnPrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/storage/primary/EncryptVolumeBitsOnPrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/storage/primary/EncryptVolumeBitsOnPrimaryStorageReply.javaheader/src/main/java/org/zstack/header/storage/primary/InstantiateVolumeOnPrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/storage/snapshot/CreateImageCacheFromVolumeSnapshotMsg.javaheader/src/main/java/org/zstack/header/storage/snapshot/TakeSnapshotsOnKvmJobStruct.javaheader/src/main/java/org/zstack/header/storage/snapshot/VolumeSnapshotAO.javaheader/src/main/java/org/zstack/header/storage/snapshot/VolumeSnapshotAO_.javaheader/src/main/java/org/zstack/header/storage/snapshot/VolumeSnapshotInventory.javaheader/src/main/java/org/zstack/header/vm/APICreateVmInstanceFromVolumeSnapshotGroupMsg.javaheader/src/main/java/org/zstack/header/vm/DiskAO.javaheader/src/main/java/org/zstack/header/volume/CreateDataVolumeExtensionPoint.javaheader/src/main/java/org/zstack/header/volume/CreateDataVolumeFromVolumeSnapshotMsg.javaheader/src/main/java/org/zstack/header/volume/CreateDataVolumeFromVolumeTemplateMsg.javaheader/src/main/java/org/zstack/header/volume/CreateVolumeMsg.javaheader/src/main/java/org/zstack/header/volume/EncryptVolumeMsg.javaheader/src/main/java/org/zstack/header/volume/EncryptVolumeReply.javaheader/src/main/java/org/zstack/header/volume/InstantiateVolumeMsg.javaheader/src/main/java/org/zstack/header/volume/VolumeAO.javaheader/src/main/java/org/zstack/header/volume/VolumeAO_.javaheader/src/main/java/org/zstack/header/volume/VolumeCreateMessage.javaheader/src/main/java/org/zstack/header/volume/VolumeInventory.javaheader/src/main/java/org/zstack/header/volume/VolumeLuksAgentSpec.javaimage/src/main/java/org/zstack/image/ImageManagerImpl.javaplugin/ceph/src/main/java/org/zstack/storage/ceph/primary/CephPrimaryStorageBase.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMAgentCommands.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMConstant.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHost.javaplugin/kvm/src/main/java/org/zstack/kvm/VolumeTO.javaplugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmExtensions.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageBase.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageCreateEmptyVolumeMsg.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageFactory.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageHypervisorBackend.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmFactory.javaplugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsDownloadImageToCacheJob.javaplugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorage.javaplugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageBackend.javaplugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageKVMBackend.javaplugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageKVMBackendCommands.javastorage/src/main/java/org/zstack/storage/encrypt/DummyVolumeEncryptedResourceKeyBackend.javastorage/src/main/java/org/zstack/storage/encrypt/SnapshotGroupRevertVolumeEncryptionHelper.javastorage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedAttachExtension.javastorage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedExpungeExtension.javastorage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedInitialExtension.javastorage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedResourceKeyBackend.javastorage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedSecretHelper.javastorage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedStartExtension.javastorage/src/main/java/org/zstack/storage/encrypt/VolumeSnapshotEncryptionExtension.javastorage/src/main/java/org/zstack/storage/encrypt/VolumeSnapshotEncryptionHelper.javastorage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotManagerImpl.javastorage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotTreeBase.javastorage/src/main/java/org/zstack/storage/volume/VolumeBase.javastorage/src/main/java/org/zstack/storage/volume/VolumeInPlaceEncryptor.javastorage/src/main/java/org/zstack/storage/volume/VolumeManagerImpl.javastorage/src/main/java/org/zstack/storage/volume/VolumeSystemTags.java
✅ Files skipped from review due to trivial changes (2)
- header/src/main/java/org/zstack/header/storage/primary/CreateImageCacheFromVolumeSnapshotOnPrimaryStorageMsg.java
- header/src/main/java/org/zstack/header/volume/VolumeAO_.java
🚧 Files skipped from review as they are similar to previous changes (59)
- header/src/main/java/org/zstack/header/volume/CreateDataVolumeFromVolumeSnapshotMsg.java
- header/src/main/java/org/zstack/header/volume/EncryptVolumeReply.java
- image/src/main/java/org/zstack/image/ImageManagerImpl.java
- header/src/main/java/org/zstack/header/secret/SecretHostEnsureLuksSecretFileReply.java
- header/src/main/java/org/zstack/header/volume/InstantiateVolumeMsg.java
- header/src/main/java/org/zstack/header/volume/CreateVolumeMsg.java
- header/src/main/java/org/zstack/header/volume/CreateDataVolumeExtensionPoint.java
- header/src/main/java/org/zstack/header/volume/VolumeCreateMessage.java
- plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageHypervisorBackend.java
- compute/src/main/java/org/zstack/compute/vm/VmAllocateVolumeFlow.java
- header/src/main/java/org/zstack/header/image/CreateTemporaryRootVolumeTemplateFromVolumeSnapshotMsg.java
- storage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotTreeBase.java
- header/src/main/java/org/zstack/header/storage/snapshot/CreateImageCacheFromVolumeSnapshotMsg.java
- header/src/main/java/org/zstack/header/storage/snapshot/TakeSnapshotsOnKvmJobStruct.java
- plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageFactory.java
- header/src/main/java/org/zstack/header/vm/APICreateVmInstanceFromVolumeSnapshotGroupMsg.java
- header/src/main/java/org/zstack/header/volume/VolumeAO.java
- storage/src/main/java/org/zstack/storage/volume/VolumeSystemTags.java
- storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedAttachExtension.java
- plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmFactory.java
- header/src/main/java/org/zstack/header/secret/SecretHostEnsureLuksSecretFileMsg.java
- header/src/main/java/org/zstack/header/storage/primary/CreateVolumeFromVolumeSnapshotOnPrimaryStorageMsg.java
- header/src/main/java/org/zstack/header/volume/CreateDataVolumeFromVolumeTemplateMsg.java
- header/src/main/java/org/zstack/header/volume/VolumeLuksAgentSpec.java
- header/src/main/java/org/zstack/header/storage/primary/InstantiateVolumeOnPrimaryStorageMsg.java
- header/src/main/java/org/zstack/header/storage/snapshot/VolumeSnapshotAO.java
- header/src/main/java/org/zstack/header/storage/primary/EncryptVolumeBitsOnPrimaryStorageReply.java
- plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmExtensions.java
- plugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageBackend.java
- storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedStartExtension.java
- storage/src/main/java/org/zstack/storage/encrypt/DummyVolumeEncryptedResourceKeyBackend.java
- compute/src/main/java/org/zstack/compute/vm/VmInstantiateOtherDiskFlow.java
- header/src/main/java/org/zstack/header/secret/SecretHostGetReply.java
- storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedInitialExtension.java
- plugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorage.java
- plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageCreateEmptyVolumeMsg.java
- header/src/main/java/org/zstack/header/volume/VolumeInventory.java
- plugin/kvm/src/main/java/org/zstack/kvm/KVMAgentCommands.java
- header/src/main/java/org/zstack/header/volume/EncryptVolumeMsg.java
- plugin/kvm/src/main/java/org/zstack/kvm/VolumeTO.java
- header/src/main/java/org/zstack/header/storage/snapshot/VolumeSnapshotInventory.java
- header/src/main/java/org/zstack/header/storage/primary/EncryptVolumeBitsOnPrimaryStorageMsg.java
- header/src/main/java/org/zstack/header/vm/DiskAO.java
- plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageBase.java
- storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedExpungeExtension.java
- plugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsDownloadImageToCacheJob.java
- plugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageKVMBackendCommands.java
- plugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageKVMBackend.java
- storage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotManagerImpl.java
- storage/src/main/java/org/zstack/storage/encrypt/SnapshotGroupRevertVolumeEncryptionHelper.java
- plugin/kvm/src/main/java/org/zstack/kvm/KVMConstant.java
- storage/src/main/java/org/zstack/storage/volume/VolumeBase.java
- storage/src/main/java/org/zstack/storage/volume/VolumeInPlaceEncryptor.java
- storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedSecretHelper.java
- plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java
- plugin/ceph/src/main/java/org/zstack/storage/ceph/primary/CephPrimaryStorageBase.java
- storage/src/main/java/org/zstack/storage/encrypt/VolumeSnapshotEncryptionHelper.java
- storage/src/main/java/org/zstack/storage/encrypt/VolumeSnapshotEncryptionExtension.java
- conf/db/zsv/V5.1.0__schema.sql
MatheMatrix
force-pushed
the
sync/zstackio/encrypt-volume@@2
branch
2 times, most recently
from
5a133da to
bd8d235
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
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/VolumeManagerImpl.java (1)
930-957:⚠️ Potential issue | 🟠 Major | ⚡ Quick win快照加密继承只补了内部消息路径，API 路径还在漏。
这里给
CreateDataVolumeFromVolumeSnapshotMsg补了encrypted和afterCreateVolume(vvo, snapshotUuid)，但同文件上面的handle(APICreateDataVolumeFromVolumeSnapshotMsg)仍然直接持久化vo，既没有设置encrypted，也没有调用这个扩展点。这样从 API 创建加密快照卷时，DB 标记和密钥继承仍会丢失。🤖 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/VolumeManagerImpl.java` around lines 930 - 957, The API path handler handle(APICreateDataVolumeFromVolumeSnapshotMsg) misses inheriting snapshot encryption and invoking post-create hooks: update that handler in VolumeManagerImpl to compute snapshotEncrypted (query VolumeSnapshotVO.encrypted by msg.getVolumeSnapshotUuid()), set vo.setEncrypted(Boolean.TRUE.equals(msg.getEncrypted()) || Boolean.TRUE.equals(snapshotEncrypted)), set vo.setAccountUuid(msg.getSession().getAccountUuid()), persist/reload vo, apply system/user tags via tagMgr.createTags(...) like the internal CreateDataVolumeFromVolumeSnapshotMsg flow, fire the volume status event (FireVolumeCanonicalEvent.fireVolumeStatusChangedEvent) and call all CreateDataVolumeExtensionPoint.afterCreateVolume(vvo, msg.getVolumeSnapshotUuid()) so API-created volumes get the same encryption flag and extension processing as the internal path.
♻️ Duplicate comments (1)
plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.java (1)
3612-3616:⚠️ Potential issue | 🟠 Major | ⚡ Quick win直接建模板分支仍未透传 LUKS secret material。
这里发送
CreateTemplateFromVolumeCmd时只设置了 install/volume path。对于加密卷，agent 侧仍然拿不到 secret material，和前面ImageCache.download()的 volume 导出路径不一致。🔧 建议补丁
CreateTemplateFromVolumeCmd cmd = new CreateTemplateFromVolumeCmd(); cmd.setInstallPath(temporaryTemplatePath); cmd.setVolumePath(msg.getVolumeInventory().getInstallPath()); + if (Boolean.TRUE.equals(msg.getVolumeInventory().getEncrypted())) { + VolumeLuksAgentSpec luksSpec = snapshotEncryptionHelper.prepareVolumeSecretMaterial( + ref.getHostUuid(), msg.getVolumeInventory().getUuid()); + if (luksSpec != null && luksSpec.isComplete()) { + cmd.setEncryptLuksSecretMaterialFilePath( + luksSpec.getEncryptLuksSecretMaterialFilePath()); + } + } httpCall(CREATE_TEMPLATE_FROM_VOLUME, ref.getHostUuid(), cmd, false,🤖 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/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.java` around lines 3612 - 3616, The CreateTemplateFromVolumeCmd is missing the LUKS secret material so encrypted volumes fail on the agent; when building the cmd in LocalStorageKvmBackend (CreateTemplateFromVolumeCmd, setInstallPath, setVolumePath, httpCall with CREATE_TEMPLATE_FROM_VOLUME), copy the LUKS secret/secret material used when exporting the volume (same data passed in ImageCache.download()) into the cmd (e.g., via the command's secret/luks fields) so the agent receives the encryption material; ensure the secret is read from the message/VolumeInventory and set on the cmd before invoking httpCall.
🤖 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/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.java`:
- Around line 1489-1490: The ImageCache constructed from
msg.getVolumeInventory() is not getting the LUKS secret material injected, so
ImageCache.download() cannot access encryptLuksSecretMaterialFilePath; fix by
detecting when the source volume is LUKS-encrypted during the ImageCache
creation, prepare or copy the LUKS secret material file, set the
ImageCache.encryptLuksSecretMaterialFilePath property with the prepared file
path, and then call ImageCache.download(); update the same pattern at the other
construction sites referenced (around the uses tied to msg.getVolumeInventory()
at the other spots) so every code path that builds an ImageCache from a volume
supplies the encryptLuksSecretMaterialFilePath before download() is invoked.
In
`@plugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageKVMBackend.java`:
- Around line 1549-1552: Calls to prepareVolumeSecretMaterialPath(...) may
return null or a shorter-than-expected list for encrypted volumes, but the code
(e.g., the block that sets cmd.setEncryptLuksSecretMaterialFilePath and
cmd.setResetBackingLuksSecretMaterialFilePath) continues to send KVM commands
and later causes mismatched rebaseLuksSecretMaterialFilePaths in
commitSnapshot(); change these call sites to fail fast for encrypted volumes:
after calling prepareVolumeSecretMaterialPath(host.getUuid(), volume) check for
null or incorrect list length and throw/return an error (as
resetRootVolumeFromImage() does) instead of setting the cmd fields and
proceeding; apply the same validation/early-fail logic to the other occurrences
you flagged (around the ranges that include commitSnapshot(), the other set*
calls, and any rebaseLuksSecretMaterialFilePaths assembly) so no command is sent
when the secret material is incomplete.
In
`@storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedInitialExtension.java`:
- Around line 47-48: 在 VolumeEncryptedInitialExtension 中不要无条件 new
VolumeLuksAgentSpec 并调用 msg.setVolumeLuksAgentSpec(spec); 改为先检查
msg.getVolumeLuksAgentSpec() 是否为 null，并且只有在确认该卷已加密时才创建新的 VolumeLuksAgentSpec
并回写；保持通过 InstantiateVolumeMsg/InstantiateVolumeOnPrimaryStorageMsg 透传同一份
spec（即不要覆盖已有的 spec，也不要给未加密的普通卷写入空 spec）。
In
`@storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedSecretHelper.java`:
- Around line 157-166: The stored host-only tag
(VolumeSystemTags.VOLUME_LIBVIRT_SECRET_HOST) is insufficient because
deleteSecretOnHostBestEffort() requires vmInstanceUuid and will skip cleanup
when the VM is gone; update the tag to persist an identifier that survives VM
detach (either vmInstanceUuid and/or secretUuid) when creating the tag in
VolumeEncryptedSecretHelper (the block that builds SystemTagCreator for
VOLUME_LIBVIRT_SECRET_HOST), e.g. add tokens for
VolumeSystemTags.VOLUME_LIBVIRT_SECRET_HOST_TOKEN (hostUuid) plus a new token
like VOLUME_LIBVIRT_SECRET_VM_TOKEN or VOLUME_LIBVIRT_SECRET_UUID_TOKEN carrying
vmUuid or secretUuid; then change deleteSecretOnHostBestEffort() to read the
persisted token(s) from the system tag instead of relying on the transient
vmInstanceUuid, or make it attempt deletion by secretUuid when present so
cleanup still runs after VM removal.
In `@storage/src/main/java/org/zstack/storage/volume/VolumeBase.java`:
- Around line 3318-3347: The handler handle(EncryptVolumeMsg msg) currently
calls volumeInPlaceEncryptor.encryptInPlace(...) directly, causing concurrent
write races; instead submit the work to the volume's serial thread via
thdf.chainSubmit using the volume's sync key (e.g. the volume UUID) so
encryptInPlace runs serialized with other write operations. Move the
refreshVO()/self-null check and the encryptInPlace(...) invocation into the
ChainTask/ChainCommand submitted by thdf.chainSubmit(syncThreadId, ...), keep
the EncryptVolumeReply creation and bus.reply(msg, reply) behavior inside the
encryptInPlace success/fail callbacks, and ensure the ChainTask correctly uses
msg as the context so failures/replies are still sent from inside the serialized
task.
In `@storage/src/main/java/org/zstack/storage/volume/VolumeManagerImpl.java`:
- Around line 513-523: The skip() check wrongly requires msg.getEncrypted() when
isTemplateFromEncryptedSource(msg.getImageUuid()) is true, causing the flow to
be skipped and the DB not updated; change the skip logic in the anonymous Flow
(the skip(Map) method) so the flow runs if either msg.getEncrypted() is true OR
isTemplateFromEncryptedSource(...) is true (e.g. replace the current OR-negated
expression with a negation of the combined condition), leaving the run(...) body
(reload vol, call
snapshotEncryptionHelper.inheritFromTemporarySnapshotImageKeyIfPossible(latest),
and SQL.New(...).set(VolumeVO_.encrypted, true).update()) unchanged.
---
Outside diff comments:
In `@storage/src/main/java/org/zstack/storage/volume/VolumeManagerImpl.java`:
- Around line 930-957: The API path handler
handle(APICreateDataVolumeFromVolumeSnapshotMsg) misses inheriting snapshot
encryption and invoking post-create hooks: update that handler in
VolumeManagerImpl to compute snapshotEncrypted (query VolumeSnapshotVO.encrypted
by msg.getVolumeSnapshotUuid()), set
vo.setEncrypted(Boolean.TRUE.equals(msg.getEncrypted()) ||
Boolean.TRUE.equals(snapshotEncrypted)), set
vo.setAccountUuid(msg.getSession().getAccountUuid()), persist/reload vo, apply
system/user tags via tagMgr.createTags(...) like the internal
CreateDataVolumeFromVolumeSnapshotMsg flow, fire the volume status event
(FireVolumeCanonicalEvent.fireVolumeStatusChangedEvent) and call all
CreateDataVolumeExtensionPoint.afterCreateVolume(vvo,
msg.getVolumeSnapshotUuid()) so API-created volumes get the same encryption flag
and extension processing as the internal path.
---
Duplicate comments:
In
`@plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.java`:
- Around line 3612-3616: The CreateTemplateFromVolumeCmd is missing the LUKS
secret material so encrypted volumes fail on the agent; when building the cmd in
LocalStorageKvmBackend (CreateTemplateFromVolumeCmd, setInstallPath,
setVolumePath, httpCall with CREATE_TEMPLATE_FROM_VOLUME), copy the LUKS
secret/secret material used when exporting the volume (same data passed in
ImageCache.download()) into the cmd (e.g., via the command's secret/luks fields)
so the agent receives the encryption material; ensure the secret is read from
the message/VolumeInventory and set on the cmd before invoking httpCall.
🪄 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: b9afa2ad-c0c9-4669-b820-aabb426be791
⛔ Files ignored due to path filters (2)
conf/springConfigXml/VolumeManager.xmlis excluded by!**/*.xmlstorage/pom.xmlis excluded by!**/*.xml
📒 Files selected for processing (67)
compute/src/main/java/org/zstack/compute/vm/VmAllocateVolumeFlow.javacompute/src/main/java/org/zstack/compute/vm/VmInstanceUtils.javacompute/src/main/java/org/zstack/compute/vm/VmInstantiateOtherDiskFlow.javaconf/db/zsv/V5.1.0__schema.sqlheader/src/main/java/org/zstack/header/image/CreateTemporaryRootVolumeTemplateFromVolumeSnapshotMsg.javaheader/src/main/java/org/zstack/header/secret/SecretHostEnsureLuksSecretFileMsg.javaheader/src/main/java/org/zstack/header/secret/SecretHostEnsureLuksSecretFileReply.javaheader/src/main/java/org/zstack/header/secret/SecretHostGetReply.javaheader/src/main/java/org/zstack/header/storage/primary/CreateImageCacheFromVolumeSnapshotOnPrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/storage/primary/CreateVolumeFromVolumeSnapshotOnPrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/storage/primary/EncryptVolumeBitsOnPrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/storage/primary/EncryptVolumeBitsOnPrimaryStorageReply.javaheader/src/main/java/org/zstack/header/storage/primary/InstantiateVolumeOnPrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/storage/primary/ReInitRootVolumeFromTemplateOnPrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/storage/snapshot/CreateImageCacheFromVolumeSnapshotMsg.javaheader/src/main/java/org/zstack/header/storage/snapshot/TakeSnapshotsOnKvmJobStruct.javaheader/src/main/java/org/zstack/header/storage/snapshot/VolumeSnapshotAO.javaheader/src/main/java/org/zstack/header/storage/snapshot/VolumeSnapshotAO_.javaheader/src/main/java/org/zstack/header/storage/snapshot/VolumeSnapshotInventory.javaheader/src/main/java/org/zstack/header/vm/APICreateVmInstanceFromVolumeSnapshotGroupMsg.javaheader/src/main/java/org/zstack/header/vm/DiskAO.javaheader/src/main/java/org/zstack/header/volume/CreateDataVolumeExtensionPoint.javaheader/src/main/java/org/zstack/header/volume/CreateDataVolumeFromVolumeSnapshotMsg.javaheader/src/main/java/org/zstack/header/volume/CreateDataVolumeFromVolumeTemplateMsg.javaheader/src/main/java/org/zstack/header/volume/CreateVolumeMsg.javaheader/src/main/java/org/zstack/header/volume/EncryptVolumeMsg.javaheader/src/main/java/org/zstack/header/volume/EncryptVolumeReply.javaheader/src/main/java/org/zstack/header/volume/InstantiateVolumeMsg.javaheader/src/main/java/org/zstack/header/volume/VolumeAO.javaheader/src/main/java/org/zstack/header/volume/VolumeAO_.javaheader/src/main/java/org/zstack/header/volume/VolumeCreateMessage.javaheader/src/main/java/org/zstack/header/volume/VolumeInventory.javaheader/src/main/java/org/zstack/header/volume/VolumeLuksAgentSpec.javaimage/src/main/java/org/zstack/image/ImageManagerImpl.javaplugin/ceph/src/main/java/org/zstack/storage/ceph/primary/CephPrimaryStorageBase.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMAgentCommands.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMConstant.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHost.javaplugin/kvm/src/main/java/org/zstack/kvm/VolumeTO.javaplugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmExtensions.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageBase.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageCreateEmptyVolumeMsg.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageFactory.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageHypervisorBackend.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmFactory.javaplugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsDownloadImageToCacheJob.javaplugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorage.javaplugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageBackend.javaplugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageKVMBackend.javaplugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageKVMBackendCommands.javastorage/src/main/java/org/zstack/storage/encrypt/DummyVolumeEncryptedResourceKeyBackend.javastorage/src/main/java/org/zstack/storage/encrypt/SnapshotGroupRevertVolumeEncryptionHelper.javastorage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedAttachExtension.javastorage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedExpungeExtension.javastorage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedInitialExtension.javastorage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedResourceKeyBackend.javastorage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedSecretHelper.javastorage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedStartExtension.javastorage/src/main/java/org/zstack/storage/encrypt/VolumeSnapshotEncryptionExtension.javastorage/src/main/java/org/zstack/storage/encrypt/VolumeSnapshotEncryptionHelper.javastorage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotManagerImpl.javastorage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotTreeBase.javastorage/src/main/java/org/zstack/storage/volume/VolumeBase.javastorage/src/main/java/org/zstack/storage/volume/VolumeInPlaceEncryptor.javastorage/src/main/java/org/zstack/storage/volume/VolumeManagerImpl.javastorage/src/main/java/org/zstack/storage/volume/VolumeSystemTags.java
✅ Files skipped from review due to trivial changes (1)
- header/src/main/java/org/zstack/header/volume/VolumeAO_.java
🚧 Files skipped from review as they are similar to previous changes (55)
- header/src/main/java/org/zstack/header/volume/CreateDataVolumeExtensionPoint.java
- header/src/main/java/org/zstack/header/storage/primary/EncryptVolumeBitsOnPrimaryStorageReply.java
- header/src/main/java/org/zstack/header/storage/snapshot/VolumeSnapshotAO.java
- header/src/main/java/org/zstack/header/volume/CreateDataVolumeFromVolumeSnapshotMsg.java
- header/src/main/java/org/zstack/header/volume/CreateDataVolumeFromVolumeTemplateMsg.java
- header/src/main/java/org/zstack/header/storage/primary/InstantiateVolumeOnPrimaryStorageMsg.java
- header/src/main/java/org/zstack/header/storage/snapshot/VolumeSnapshotAO_.java
- header/src/main/java/org/zstack/header/volume/VolumeCreateMessage.java
- plugin/kvm/src/main/java/org/zstack/kvm/KVMConstant.java
- storage/src/main/java/org/zstack/storage/volume/VolumeSystemTags.java
- plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageFactory.java
- header/src/main/java/org/zstack/header/volume/VolumeInventory.java
- header/src/main/java/org/zstack/header/storage/primary/EncryptVolumeBitsOnPrimaryStorageMsg.java
- header/src/main/java/org/zstack/header/volume/InstantiateVolumeMsg.java
- header/src/main/java/org/zstack/header/secret/SecretHostEnsureLuksSecretFileReply.java
- header/src/main/java/org/zstack/header/image/CreateTemporaryRootVolumeTemplateFromVolumeSnapshotMsg.java
- header/src/main/java/org/zstack/header/storage/primary/CreateImageCacheFromVolumeSnapshotOnPrimaryStorageMsg.java
- header/src/main/java/org/zstack/header/volume/VolumeAO.java
- header/src/main/java/org/zstack/header/storage/snapshot/TakeSnapshotsOnKvmJobStruct.java
- plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageCreateEmptyVolumeMsg.java
- image/src/main/java/org/zstack/image/ImageManagerImpl.java
- storage/src/main/java/org/zstack/storage/encrypt/DummyVolumeEncryptedResourceKeyBackend.java
- header/src/main/java/org/zstack/header/storage/snapshot/VolumeSnapshotInventory.java
- header/src/main/java/org/zstack/header/storage/primary/CreateVolumeFromVolumeSnapshotOnPrimaryStorageMsg.java
- plugin/kvm/src/main/java/org/zstack/kvm/VolumeTO.java
- storage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotTreeBase.java
- header/src/main/java/org/zstack/header/volume/VolumeLuksAgentSpec.java
- conf/db/zsv/V5.1.0__schema.sql
- storage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotManagerImpl.java
- storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedExpungeExtension.java
- header/src/main/java/org/zstack/header/volume/EncryptVolumeMsg.java
- header/src/main/java/org/zstack/header/volume/CreateVolumeMsg.java
- plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmExtensions.java
- plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageHypervisorBackend.java
- header/src/main/java/org/zstack/header/vm/DiskAO.java
- plugin/kvm/src/main/java/org/zstack/kvm/KVMAgentCommands.java
- header/src/main/java/org/zstack/header/storage/snapshot/CreateImageCacheFromVolumeSnapshotMsg.java
- header/src/main/java/org/zstack/header/vm/APICreateVmInstanceFromVolumeSnapshotGroupMsg.java
- plugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageBackend.java
- storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedResourceKeyBackend.java
- compute/src/main/java/org/zstack/compute/vm/VmAllocateVolumeFlow.java
- plugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsDownloadImageToCacheJob.java
- storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedStartExtension.java
- storage/src/main/java/org/zstack/storage/encrypt/SnapshotGroupRevertVolumeEncryptionHelper.java
- plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmFactory.java
- plugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorage.java
- storage/src/main/java/org/zstack/storage/volume/VolumeInPlaceEncryptor.java
- compute/src/main/java/org/zstack/compute/vm/VmInstanceUtils.java
- header/src/main/java/org/zstack/header/secret/SecretHostGetReply.java
- plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageBase.java
- storage/src/main/java/org/zstack/storage/encrypt/VolumeSnapshotEncryptionHelper.java
- plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java
- storage/src/main/java/org/zstack/storage/encrypt/VolumeSnapshotEncryptionExtension.java
- storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedAttachExtension.java
- plugin/ceph/src/main/java/org/zstack/storage/ceph/primary/CephPrimaryStorageBase.java
Comment on lines
+1549
to
+1552
| cmd.setEncryptLuksSecretMaterialFilePath(prepareVolumeSecretMaterialPath(host.getUuid(), volume)); | ||
| if (!cmd.isFullRebase()) { | ||
| cmd.setResetBackingLuksSecretMaterialFilePath(prepareVolumeSecretMaterialPath(host.getUuid(), volume)); | ||
| } |
There was a problem hiding this comment.
加密卷的 secret material 不能静默降级成 null/短列表。
prepareVolumeSecretMaterialPath(s) 在 volume.encrypted=true 时只要拿不到完整 spec 就返回 null 或更短的 List，而这些调用点仍继续下发 KVM 命令。这样会把“加密卷前置条件不满足”变成更晚、更难定位的 agent 侧失败；commitSnapshot() 里还会让 rebaseLuksSecretMaterialFilePaths 与子链数量失配。这里应当像 resetRootVolumeFromImage() 一样对加密卷 fail fast，而不是继续发送命令。
Also applies to: 1976-1994, 2069-2094, 2105-2108, 2142-2145
🤖 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/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageKVMBackend.java`
around lines 1549 - 1552, Calls to prepareVolumeSecretMaterialPath(...) may
return null or a shorter-than-expected list for encrypted volumes, but the code
(e.g., the block that sets cmd.setEncryptLuksSecretMaterialFilePath and
cmd.setResetBackingLuksSecretMaterialFilePath) continues to send KVM commands
and later causes mismatched rebaseLuksSecretMaterialFilePaths in
commitSnapshot(); change these call sites to fail fast for encrypted volumes:
after calling prepareVolumeSecretMaterialPath(host.getUuid(), volume) check for
null or incorrect list length and throw/return an error (as
resetRootVolumeFromImage() does) instead of setting the cmd fields and
proceeding; apply the same validation/early-fail logic to the other occurrences
you flagged (around the ranges that include commitSnapshot(), the other set*
calls, and any rebaseLuksSecretMaterialFilePaths assembly) so no command is sent
when the secret material is incomplete.
Comment on lines
+47
to
+48
| VolumeLuksAgentSpec spec = new VolumeLuksAgentSpec(); | ||
| msg.setVolumeLuksAgentSpec(spec); |
There was a problem hiding this comment.
不要在这里重建 VolumeLuksAgentSpec。
这里无条件 new 并回写到 msg，会覆盖调用方或前置扩展已经放进去的 LUKS 参数；而且普通卷也会被带上一个空 spec。既然这个 PR 已经改成通过 InstantiateVolumeMsg/InstantiateVolumeOnPrimaryStorageMsg 透传同一份 spec，这里应当只在 msg.getVolumeLuksAgentSpec() == null 且确认卷已加密之后再创建。
💡 建议修改
- VolumeLuksAgentSpec spec = new VolumeLuksAgentSpec();
- msg.setVolumeLuksAgentSpec(spec);
+ VolumeLuksAgentSpec spec = msg.getVolumeLuksAgentSpec();
String volUuid = msg.getVolumeUuid();
VolumeVO volume = dbf.findByUuid(volUuid, VolumeVO.class);
if (volume != null && volume.isEncrypted()) {
+ if (spec == null) {
+ spec = new VolumeLuksAgentSpec();
+ msg.setVolumeLuksAgentSpec(spec);
+ }
// Temporary root images created from snapshots own the image cache before
// the final root volume exists. Persisting the cache key on ImageVO lets
// the root volume inherit the same key, so its top layer can match the🤖 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/VolumeEncryptedInitialExtension.java`
around lines 47 - 48, 在 VolumeEncryptedInitialExtension 中不要无条件 new
VolumeLuksAgentSpec 并调用 msg.setVolumeLuksAgentSpec(spec); 改为先检查
msg.getVolumeLuksAgentSpec() 是否为 null，并且只有在确认该卷已加密时才创建新的 VolumeLuksAgentSpec
并回写；保持通过 InstantiateVolumeMsg/InstantiateVolumeOnPrimaryStorageMsg 透传同一份
spec（即不要覆盖已有的 spec，也不要给未加密的普通卷写入空 spec）。
Comment on lines
+157
to
+166
| // Remember which host now owns this volume's libvirt secret so that | ||
| // expunge can clean it up later, even if the owning VM is gone by then. | ||
| // recreate=true overwrites any stale tag from a previous host. | ||
| try { | ||
| SystemTagCreator tc = VolumeSystemTags.VOLUME_LIBVIRT_SECRET_HOST.newSystemTagCreator(volUuid); | ||
| tc.setTagByTokens(Collections.singletonMap( | ||
| VolumeSystemTags.VOLUME_LIBVIRT_SECRET_HOST_TOKEN, hostUuid)); | ||
| tc.inherent = false; | ||
| tc.recreate = true; | ||
| tc.create(); |
There was a problem hiding this comment.
当前的 host tag 无法支撑“VM 已不存在时”的 secret 清理。
这里持久化的只有 VOLUME_LIBVIRT_SECRET_HOST=hostUuid，但 deleteSecretOnHostBestEffort() 又把 vmUuid 当成必填项；一旦卷已经 detach / expunge、vmInstanceUuid 被清空，删除路径就会直接 return，宿主机上的 libvirt secret 会残留。要兑现这里注释里的语义，至少还需要把 vmUuid 或 secretUuid 一并持久化，或者提供一个不依赖当前 vmInstanceUuid 的删除键。
Also applies to: 178-183, 262-266
🤖 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/VolumeEncryptedSecretHelper.java`
around lines 157 - 166, The stored host-only tag
(VolumeSystemTags.VOLUME_LIBVIRT_SECRET_HOST) is insufficient because
deleteSecretOnHostBestEffort() requires vmInstanceUuid and will skip cleanup
when the VM is gone; update the tag to persist an identifier that survives VM
detach (either vmInstanceUuid and/or secretUuid) when creating the tag in
VolumeEncryptedSecretHelper (the block that builds SystemTagCreator for
VOLUME_LIBVIRT_SECRET_HOST), e.g. add tokens for
VolumeSystemTags.VOLUME_LIBVIRT_SECRET_HOST_TOKEN (hostUuid) plus a new token
like VOLUME_LIBVIRT_SECRET_VM_TOKEN or VOLUME_LIBVIRT_SECRET_UUID_TOKEN carrying
vmUuid or secretUuid; then change deleteSecretOnHostBestEffort() to read the
persisted token(s) from the system tag instead of relying on the transient
vmInstanceUuid, or make it attempt deletion by secretUuid when present so
cleanup still runs after VM removal.
Comment on lines
+3318
to
+3347
| private void handle(EncryptVolumeMsg msg) { | ||
| EncryptVolumeReply reply = new EncryptVolumeReply(); | ||
| refreshVO(); | ||
|
|
||
| if (self == null) { | ||
| reply.setError(operr("volume[uuid:%s] has been deleted", msg.getVolumeUuid())); | ||
| bus.reply(msg, reply); | ||
| return; | ||
| } | ||
|
|
||
| VolumeInPlaceEncryptor.Context ctx = new VolumeInPlaceEncryptor.Context() | ||
| .setHostUuid(msg.getHostUuid()) | ||
| .setPrimaryStorageUuid(msg.getPrimaryStorageUuid()) | ||
| .setInstallPath(msg.getInstallPath()) | ||
| .setPurpose(msg.getPurpose()); | ||
|
|
||
| volumeInPlaceEncryptor.encryptInPlace(self, ctx, new ReturnValueCompletion<VolumeVO>(msg) { | ||
| @Override | ||
| public void success(VolumeVO latest) { | ||
| self = latest; | ||
| reply.setInventory(getSelfInventory()); | ||
| bus.reply(msg, reply); | ||
| } | ||
|
|
||
| @Override | ||
| public void fail(ErrorCode errorCode) { | ||
| reply.setError(errorCode); | ||
| bus.reply(msg, reply); | ||
| } | ||
| }); |
There was a problem hiding this comment.
把 EncryptVolumeMsg 放进 syncThreadId 串行队列。
这个 handler 直接执行 encryptInPlace(...)，没有像删除、实例化这类写路径那样走 thdf.chainSubmit(...)。同一个 volume 上并发的加密、删除、重置等请求会交错修改底层 bits 和 DB，结果不确定，最坏会把卷状态和实际文件内容打乱。
🤖 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
3318 - 3347, The handler handle(EncryptVolumeMsg msg) currently calls
volumeInPlaceEncryptor.encryptInPlace(...) directly, causing concurrent write
races; instead submit the work to the volume's serial thread via
thdf.chainSubmit using the volume's sync key (e.g. the volume UUID) so
encryptInPlace runs serialized with other write operations. Move the
refreshVO()/self-null check and the encryptInPlace(...) invocation into the
ChainTask/ChainCommand submitted by thdf.chainSubmit(syncThreadId, ...), keep
the EncryptVolumeReply creation and bus.reply(msg, reply) behavior inside the
encryptInPlace success/fail callbacks, and ensure the ChainTask correctly uses
msg as the context so failures/replies are still sent from inside the serialized
task.
Comment on lines
+513
to
+523
| @Override | ||
| public boolean skip(Map data) { | ||
| return !Boolean.TRUE.equals(msg.getEncrypted()) || !isTemplateFromEncryptedSource(msg.getImageUuid()); | ||
| } | ||
|
|
||
| @Override | ||
| public void run(FlowTrigger trigger, Map data) { | ||
| VolumeVO latest = dbf.reload(vol); | ||
| snapshotEncryptionHelper.inheritFromTemporarySnapshotImageKeyIfPossible(latest); | ||
| SQL.New(VolumeVO.class).eq(VolumeVO_.uuid, vol.getUuid()).set(VolumeVO_.encrypted, true).update(); | ||
| trigger.next(); |
There was a problem hiding this comment.
加密来源模板不能继续受 msg.getEncrypted() 限制。
isTemplateFromEncryptedSource(...) 为真时，下载下来的 bits 已经是 LUKS；但这里仍然用 msg.getEncrypted() 作为前置条件。调用方如果没显式带这个标记，这两个 flow 都会被跳过，最终得到“文件已加密，但 DB 仍是 encrypted=false 且没有继承密钥绑定”的卷，后续 attach/start 就不会准备 secret。
🤖 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/VolumeManagerImpl.java`
around lines 513 - 523, The skip() check wrongly requires msg.getEncrypted()
when isTemplateFromEncryptedSource(msg.getImageUuid()) is true, causing the flow
to be skipped and the DB not updated; change the skip logic in the anonymous
Flow (the skip(Map) method) so the flow runs if either msg.getEncrypted() is
true OR isTemplateFromEncryptedSource(...) is true (e.g. replace the current
OR-negated expression with a negation of the combined condition), leaving the
run(...) body (reload vol, call
snapshotEncryptionHelper.inheritFromTemporarySnapshotImageKeyIfPossible(latest),
and SQL.New(...).set(VolumeVO_.encrypted, true).update()) unchanged.
Keep KVM LUKS agent params on InstantiateVolumeMsg / InstantiateVolumeOnPrimaryStorageMsg (VolumeLuksAgentSpec), premium pre-instantiate DEK path, and LocalStorageKvmBackend wiring. Shared MergeSnapshotCmd encrypt fields removed; helper script commit-push-three-repos.sh at repo root. Co-authored-by: Cursor <cursoragent@cursor.com>
MatheMatrix
force-pushed
the
sync/zstackio/encrypt-volume@@2
branch
from
bd8d235 to
63cd62f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugin/ceph/src/main/java/org/zstack/storage/ceph/primary/CephPrimaryStorageBase.java (2)
2790-2805:⚠️ Potential issue | 🟠 Major | ⚡ Quick win删掉这里多余的
trigger.next()。这个分支在
bus.send()后立刻推进 flow，回调里又会再次next()/fail()。结果就是 resize 还没完成，后续流程已经继续执行；一旦回调再回来，会出现二次推进，失败也可能被提前的成功路径掩盖。🛠️ 建议修改
bus.send(rmsg, new CloudBusCallBack(trigger) { `@Override` public void run(MessageReply reply) { if (reply.isSuccess()) { trigger.next(); } else { trigger.fail(reply.getError()); } } }); - trigger.next(); }🤖 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 2790 - 2805, The extra trigger.next() after bus.send causes the flow to advance before the ResizeVolumeOnPrimaryStorageMsg (rmsg) callback runs and can cause double-advancing; remove the immediate trigger.next() that follows bus.send(...) and rely on the CloudBusCallBack's run() implementation to call trigger.next() or trigger.fail() based on MessageReply so the flow only progresses once the resize completes.
3130-3135:⚠️ Potential issue | 🟠 Major | ⚡ Quick win这里只补了一个调用点，按 volume 创建 image cache 的路径还会漏掉加密态。
DownloadToCache是否走 LUKS 分支完全依赖encrypted。现在只有CreateImageCacheFromVolumeSnapshotOnPrimaryStorageMsg会给它赋值；handle(CreateImageCacheFromVolumeOnPrimaryStorageMsg)创建完 snapshot 后走的是同一个DownloadToCache，但没有同步透传加密标记。这样“从加密 volume 创建模板/image cache”仍会落回CP_PATH，也不会准备 temporary snapshot-image key，后续isEncryptedSnapshotImageCache()识别不到这个 cache。🤖 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 3130 - 3135, When creating a DownloadToCache in handle(CreateImageCacheFromVolumeOnPrimaryStorageMsg) you must propagate the encryption state the same way CreateImageCacheFromVolumeSnapshotOnPrimaryStorageMsg does: set cache.encrypted before calling cache.download so LUKS branch is chosen and temporary snapshot-image keys are prepared; locate the DownloadToCache construction in CephPrimaryStorageBase (symbol: DownloadToCache) and assign cache.encrypted = msg.getEncrypted() (or derive from the created snapshot/volume metadata if msg lacks it) so isEncryptedSnapshotImageCache() and CP_PATH/LUKS selection behave correctly.
♻️ Duplicate comments (1)
storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedSecretHelper.java (1)
157-166:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift这里只记录
hostUuid，VM 清空后清理路径还是会失效。当前 tag 只持久化了
hostUuid，但deleteSecretOnHostBestEffort()仍然要求vmUuid。卷在 detach/expunge 后vmInstanceUuid被清空时，Line 263-265 会直接return，宿主机上的 libvirt secret 会残留。建议把删除所需且能跨 VM 生命周期存活的标识一并持久化，删除时优先读取这些持久化值，而不是依赖当前卷上的vmInstanceUuid。Also applies to: 262-265
🤖 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/VolumeEncryptedSecretHelper.java` around lines 157 - 166, 当前实现只在 VolumeSystemTags.VOLUME_LIBVIRT_SECRET_HOST 标签里存了 hostUuid，但 deleteSecretOnHostBestEffort() 依赖 vmInstanceUuid 导致 VM 清空后无法删除宿主机上的 libvirt secret；请修改 VolumeEncryptedSecretHelper 在创建系统标签 (VolumeSystemTags.VOLUME_LIBVIRT_SECRET_HOST via VolumeSystemTags.VOLUME_LIBVIRT_SECRET_HOST_TOKEN) 时同时持久化删除所需的跨 VM 生命周期标识（例如 vmUuid 或直接持久化 secret 的唯一 id）而非仅 hostUuid，并在 deleteSecretOnHostBestEffort() 中优先从该系统标签读取这些持久化值（使用 volUuid 查找标签）来执行删除，而不是依赖当前卷对象的 vmInstanceUuid；更新相关逻辑以兼容旧标签（回退到 hostUuid+vmUuid 优先策略）。
🤖 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/VolumeEncryptedSecretHelper.java`:
- Around line 236-253: 将错放的长 Javadoc 注释（描述 resolver 流程并引用
findKeyVersionByVolume、getSecretOnHost、defineSecretFromBinding）从其当前位置（位于
deleteSecretOnHostBestEffort() 之前）移除，并把该注释插入到 resolveOrDefineSecretForVolume()
方法声明之前，确保 deleteSecretOnHostBestEffort() 保留其自身的 Javadoc 注释不被覆盖；这样 IDE
和文档生成器就能正确将该流程说明关联到 resolveOrDefineSecretForVolume()。
- Around line 69-99: The materializeDek method must not assume
EncryptedResourceKeyManager.getOrCreateKey invokes its completion synchronously;
change materializeDek to block explicitly until the ReturnValueCompletion
callback runs (e.g., use a CountDownLatch or CompletableFuture), populate the
result or error from the callback, handle a timeout and rethrow the ErrorCode as
an OperationFailureException if fail() was called, and return the non-null
ResourceKeyResult so defineSecretFromBinding won't NPE on
keyResult.getDekBase64(); also move the misplaced Javadoc about the "One-shot
resolver … resolveOrDefineSecretForVolume()" so it sits immediately above the
resolveOrDefineSecretForVolume method instead of before
deleteSecretOnHostBestEffort.
---
Outside diff comments:
In
`@plugin/ceph/src/main/java/org/zstack/storage/ceph/primary/CephPrimaryStorageBase.java`:
- Around line 2790-2805: The extra trigger.next() after bus.send causes the flow
to advance before the ResizeVolumeOnPrimaryStorageMsg (rmsg) callback runs and
can cause double-advancing; remove the immediate trigger.next() that follows
bus.send(...) and rely on the CloudBusCallBack's run() implementation to call
trigger.next() or trigger.fail() based on MessageReply so the flow only
progresses once the resize completes.
- Around line 3130-3135: When creating a DownloadToCache in
handle(CreateImageCacheFromVolumeOnPrimaryStorageMsg) you must propagate the
encryption state the same way
CreateImageCacheFromVolumeSnapshotOnPrimaryStorageMsg does: set cache.encrypted
before calling cache.download so LUKS branch is chosen and temporary
snapshot-image keys are prepared; locate the DownloadToCache construction in
CephPrimaryStorageBase (symbol: DownloadToCache) and assign cache.encrypted =
msg.getEncrypted() (or derive from the created snapshot/volume metadata if msg
lacks it) so isEncryptedSnapshotImageCache() and CP_PATH/LUKS selection behave
correctly.
---
Duplicate comments:
In
`@storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedSecretHelper.java`:
- Around line 157-166: 当前实现只在 VolumeSystemTags.VOLUME_LIBVIRT_SECRET_HOST 标签里存了
hostUuid，但 deleteSecretOnHostBestEffort() 依赖 vmInstanceUuid 导致 VM 清空后无法删除宿主机上的
libvirt secret；请修改 VolumeEncryptedSecretHelper 在创建系统标签
(VolumeSystemTags.VOLUME_LIBVIRT_SECRET_HOST via
VolumeSystemTags.VOLUME_LIBVIRT_SECRET_HOST_TOKEN) 时同时持久化删除所需的跨 VM 生命周期标识（例如
vmUuid 或直接持久化 secret 的唯一 id）而非仅 hostUuid，并在 deleteSecretOnHostBestEffort()
中优先从该系统标签读取这些持久化值（使用 volUuid 查找标签）来执行删除，而不是依赖当前卷对象的
vmInstanceUuid；更新相关逻辑以兼容旧标签（回退到 hostUuid+vmUuid 优先策略）。
🪄 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: ff57f44e-7746-4c68-a819-da118f8e8289
⛔ Files ignored due to path filters (2)
conf/springConfigXml/VolumeManager.xmlis excluded by!**/*.xmlstorage/pom.xmlis excluded by!**/*.xml
📒 Files selected for processing (67)
compute/src/main/java/org/zstack/compute/vm/VmAllocateVolumeFlow.javacompute/src/main/java/org/zstack/compute/vm/VmInstanceUtils.javacompute/src/main/java/org/zstack/compute/vm/VmInstantiateOtherDiskFlow.javaconf/db/zsv/V5.1.0__schema.sqlheader/src/main/java/org/zstack/header/image/CreateTemporaryRootVolumeTemplateFromVolumeSnapshotMsg.javaheader/src/main/java/org/zstack/header/secret/SecretHostEnsureLuksSecretFileMsg.javaheader/src/main/java/org/zstack/header/secret/SecretHostEnsureLuksSecretFileReply.javaheader/src/main/java/org/zstack/header/secret/SecretHostGetReply.javaheader/src/main/java/org/zstack/header/storage/primary/CreateImageCacheFromVolumeSnapshotOnPrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/storage/primary/CreateVolumeFromVolumeSnapshotOnPrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/storage/primary/EncryptVolumeBitsOnPrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/storage/primary/EncryptVolumeBitsOnPrimaryStorageReply.javaheader/src/main/java/org/zstack/header/storage/primary/InstantiateVolumeOnPrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/storage/primary/ReInitRootVolumeFromTemplateOnPrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/storage/snapshot/CreateImageCacheFromVolumeSnapshotMsg.javaheader/src/main/java/org/zstack/header/storage/snapshot/TakeSnapshotsOnKvmJobStruct.javaheader/src/main/java/org/zstack/header/storage/snapshot/VolumeSnapshotAO.javaheader/src/main/java/org/zstack/header/storage/snapshot/VolumeSnapshotAO_.javaheader/src/main/java/org/zstack/header/storage/snapshot/VolumeSnapshotInventory.javaheader/src/main/java/org/zstack/header/vm/APICreateVmInstanceFromVolumeSnapshotGroupMsg.javaheader/src/main/java/org/zstack/header/vm/DiskAO.javaheader/src/main/java/org/zstack/header/volume/CreateDataVolumeExtensionPoint.javaheader/src/main/java/org/zstack/header/volume/CreateDataVolumeFromVolumeSnapshotMsg.javaheader/src/main/java/org/zstack/header/volume/CreateDataVolumeFromVolumeTemplateMsg.javaheader/src/main/java/org/zstack/header/volume/CreateVolumeMsg.javaheader/src/main/java/org/zstack/header/volume/EncryptVolumeMsg.javaheader/src/main/java/org/zstack/header/volume/EncryptVolumeReply.javaheader/src/main/java/org/zstack/header/volume/InstantiateVolumeMsg.javaheader/src/main/java/org/zstack/header/volume/VolumeAO.javaheader/src/main/java/org/zstack/header/volume/VolumeAO_.javaheader/src/main/java/org/zstack/header/volume/VolumeCreateMessage.javaheader/src/main/java/org/zstack/header/volume/VolumeInventory.javaheader/src/main/java/org/zstack/header/volume/VolumeLuksAgentSpec.javaimage/src/main/java/org/zstack/image/ImageManagerImpl.javaplugin/ceph/src/main/java/org/zstack/storage/ceph/primary/CephPrimaryStorageBase.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMAgentCommands.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMConstant.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHost.javaplugin/kvm/src/main/java/org/zstack/kvm/VolumeTO.javaplugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmExtensions.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageBase.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageCreateEmptyVolumeMsg.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageFactory.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageHypervisorBackend.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmFactory.javaplugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsDownloadImageToCacheJob.javaplugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorage.javaplugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageBackend.javaplugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageKVMBackend.javaplugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageKVMBackendCommands.javastorage/src/main/java/org/zstack/storage/encrypt/DummyVolumeEncryptedResourceKeyBackend.javastorage/src/main/java/org/zstack/storage/encrypt/SnapshotGroupRevertVolumeEncryptionHelper.javastorage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedAttachExtension.javastorage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedExpungeExtension.javastorage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedInitialExtension.javastorage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedResourceKeyBackend.javastorage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedSecretHelper.javastorage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedStartExtension.javastorage/src/main/java/org/zstack/storage/encrypt/VolumeSnapshotEncryptionExtension.javastorage/src/main/java/org/zstack/storage/encrypt/VolumeSnapshotEncryptionHelper.javastorage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotManagerImpl.javastorage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotTreeBase.javastorage/src/main/java/org/zstack/storage/volume/VolumeBase.javastorage/src/main/java/org/zstack/storage/volume/VolumeInPlaceEncryptor.javastorage/src/main/java/org/zstack/storage/volume/VolumeManagerImpl.javastorage/src/main/java/org/zstack/storage/volume/VolumeSystemTags.java
🚧 Files skipped from review as they are similar to previous changes (63)
- header/src/main/java/org/zstack/header/storage/snapshot/VolumeSnapshotAO_.java
- plugin/kvm/src/main/java/org/zstack/kvm/VolumeTO.java
- header/src/main/java/org/zstack/header/vm/DiskAO.java
- header/src/main/java/org/zstack/header/volume/CreateDataVolumeFromVolumeSnapshotMsg.java
- header/src/main/java/org/zstack/header/storage/primary/CreateImageCacheFromVolumeSnapshotOnPrimaryStorageMsg.java
- header/src/main/java/org/zstack/header/volume/VolumeInventory.java
- header/src/main/java/org/zstack/header/volume/EncryptVolumeReply.java
- header/src/main/java/org/zstack/header/volume/VolumeAO_.java
- header/src/main/java/org/zstack/header/volume/VolumeLuksAgentSpec.java
- header/src/main/java/org/zstack/header/storage/primary/EncryptVolumeBitsOnPrimaryStorageMsg.java
- header/src/main/java/org/zstack/header/secret/SecretHostEnsureLuksSecretFileReply.java
- image/src/main/java/org/zstack/image/ImageManagerImpl.java
- header/src/main/java/org/zstack/header/volume/VolumeCreateMessage.java
- header/src/main/java/org/zstack/header/storage/primary/ReInitRootVolumeFromTemplateOnPrimaryStorageMsg.java
- header/src/main/java/org/zstack/header/storage/primary/InstantiateVolumeOnPrimaryStorageMsg.java
- header/src/main/java/org/zstack/header/storage/snapshot/TakeSnapshotsOnKvmJobStruct.java
- header/src/main/java/org/zstack/header/storage/snapshot/CreateImageCacheFromVolumeSnapshotMsg.java
- header/src/main/java/org/zstack/header/volume/CreateVolumeMsg.java
- storage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotManagerImpl.java
- plugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsDownloadImageToCacheJob.java
- header/src/main/java/org/zstack/header/image/CreateTemporaryRootVolumeTemplateFromVolumeSnapshotMsg.java
- header/src/main/java/org/zstack/header/secret/SecretHostGetReply.java
- header/src/main/java/org/zstack/header/secret/SecretHostEnsureLuksSecretFileMsg.java
- header/src/main/java/org/zstack/header/volume/InstantiateVolumeMsg.java
- storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedAttachExtension.java
- plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageFactory.java
- storage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotTreeBase.java
- plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageHypervisorBackend.java
- plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageBase.java
- plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageCreateEmptyVolumeMsg.java
- header/src/main/java/org/zstack/header/volume/CreateDataVolumeExtensionPoint.java
- storage/src/main/java/org/zstack/storage/encrypt/DummyVolumeEncryptedResourceKeyBackend.java
- header/src/main/java/org/zstack/header/vm/APICreateVmInstanceFromVolumeSnapshotGroupMsg.java
- conf/db/zsv/V5.1.0__schema.sql
- storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedInitialExtension.java
- header/src/main/java/org/zstack/header/storage/snapshot/VolumeSnapshotAO.java
- header/src/main/java/org/zstack/header/volume/CreateDataVolumeFromVolumeTemplateMsg.java
- header/src/main/java/org/zstack/header/storage/snapshot/VolumeSnapshotInventory.java
- compute/src/main/java/org/zstack/compute/vm/VmInstantiateOtherDiskFlow.java
- storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedStartExtension.java
- compute/src/main/java/org/zstack/compute/vm/VmAllocateVolumeFlow.java
- plugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageBackend.java
- header/src/main/java/org/zstack/header/storage/primary/EncryptVolumeBitsOnPrimaryStorageReply.java
- storage/src/main/java/org/zstack/storage/volume/VolumeManagerImpl.java
- storage/src/main/java/org/zstack/storage/volume/VolumeSystemTags.java
- storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedResourceKeyBackend.java
- storage/src/main/java/org/zstack/storage/volume/VolumeBase.java
- header/src/main/java/org/zstack/header/storage/primary/CreateVolumeFromVolumeSnapshotOnPrimaryStorageMsg.java
- plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmExtensions.java
- storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedExpungeExtension.java
- storage/src/main/java/org/zstack/storage/encrypt/VolumeSnapshotEncryptionHelper.java
- compute/src/main/java/org/zstack/compute/vm/VmInstanceUtils.java
- plugin/kvm/src/main/java/org/zstack/kvm/KVMAgentCommands.java
- storage/src/main/java/org/zstack/storage/encrypt/SnapshotGroupRevertVolumeEncryptionHelper.java
- storage/src/main/java/org/zstack/storage/volume/VolumeInPlaceEncryptor.java
- storage/src/main/java/org/zstack/storage/encrypt/VolumeSnapshotEncryptionExtension.java
- plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java
- plugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorage.java
- plugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageKVMBackendCommands.java
- header/src/main/java/org/zstack/header/volume/EncryptVolumeMsg.java
- plugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageKVMBackend.java
- plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmFactory.java
- plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.java
Comment on lines
+69
to
+99
| public EncryptedResourceKeyManager.ResourceKeyResult materializeDek(String volUuid, String kpUuid) { | ||
| EncryptedResourceKeyManager.GetOrCreateResourceKeyContext ctx = | ||
| new EncryptedResourceKeyManager.GetOrCreateResourceKeyContext(); | ||
| ctx.setResourceUuid(volUuid); | ||
| ctx.setResourceType(VolumeVO.class.getSimpleName()); | ||
| ctx.setKeyProviderUuid(kpUuid); | ||
| ctx.setPurpose("instantiate-volume"); | ||
|
|
||
| // getOrCreateKey is synchronous in EncryptedResourceKeyManagerImpl — | ||
| // the completion fires on this thread before the call returns. | ||
| final EncryptedResourceKeyManager.ResourceKeyResult[] resultRef = | ||
| new EncryptedResourceKeyManager.ResourceKeyResult[1]; | ||
| final ErrorCode[] errorRef = new ErrorCode[1]; | ||
| encryptedResourceKeyManager.getOrCreateKey(ctx, | ||
| new ReturnValueCompletion<EncryptedResourceKeyManager.ResourceKeyResult>(null) { | ||
| @Override | ||
| public void success(EncryptedResourceKeyManager.ResourceKeyResult r) { | ||
| resultRef[0] = r; | ||
| } | ||
|
|
||
| @Override | ||
| public void fail(ErrorCode err) { | ||
| errorRef[0] = err; | ||
| } | ||
| }); | ||
| if (errorRef[0] != null) { | ||
| throw new OperationFailureException(operr( | ||
| "failed to materialize encryption key for volume[uuid:%s]", volUuid) | ||
| .withCause(errorRef[0])); | ||
| } | ||
| return resultRef[0]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 定位接口与实现 =="
fd -i 'EncryptedResourceKeyManager*.java'
echo
echo "== getOrCreateKey 定义 / 实现 / 调用上下文 =="
rg -n -C8 --glob '*.java' 'interface EncryptedResourceKeyManager|class .*EncryptedResourceKeyManager|\\bgetOrCreateKey\\s*\\('
echo
echo "== 是否已有同步等待封装 =="
rg -n -C3 --glob '*.java' 'FutureReturnValueCompletion|WhileDoneCompletion|CountDownLatch'Repository: MatheMatrix/zstack
Length of output: 553
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 定位接口与实现 =="
fd -i 'EncryptedResourceKeyManager*.java'
echo
echo "== getOrCreateKey 定义 / 实现 / 调用上下文 =="
rg -n --glob '*.java' '\bgetOrCreateKey\s*\(' header/src/main/java/org/zstack/header/keyprovider/EncryptedResourceKeyManager.java \
compute/src/main/java/org/zstack/compute/vm/devices/DummyEncryptedResourceKeyManager.java 2>/dev/null || true
# Try to find additional implementations/callers in repo (bounded by what exists)
rg -n -S --glob '*.java' '\bgetOrCreateKey\s*\(' -g '!**/target/**' -g '!**/build/**'
echo
echo "== 是否已有同步等待封装 =="
rg -n -S --glob '*.java' 'FutureReturnValueCompletion|WhileDoneCompletion|CountDownLatch|await\(|join\(|get\(' -g '!**/target/**' -g '!**/build/**'Repository: MatheMatrix/zstack
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect the actual helper file around the cited line ranges
FILE="storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedSecretHelper.java"
echo "== Line context: $FILE (140-320) =="
nl -ba "$FILE" | sed -n '140,320p'
echo
echo "== Line context: $FILE (240-310) =="
nl -ba "$FILE" | sed -n '220,310p'
echo
echo "== Line context: $FILE (250-290) =="
nl -ba "$FILE" | sed -n '250,290p'
# 2) Locate real EncryptedResourceKeyManager implementation(s) and inspect getOrCreateKey
echo
echo "== Find EncryptedResourceKeyManager implementations =="
rg -n --glob '*.java' 'class\s+\w*EncryptedResourceKeyManager\w*' -S .
echo
echo "== Find EncryptedResourceKeyManagerImpl specifically (if exists) =="
fd -i '*EncryptedResourceKeyManager*Impl*.java' . || true
rg -n --glob '*.java' 'EncryptedResourceKeyManagerImpl' -S .
echo
echo "== Inspect getOrCreateKey implementations =="
# Print only the method bodies header area for any matching file/class
rg -n --glob '*.java' 'getOrCreateKey\s*\(\s*GetOrCreateResourceKeyContext' -S .
echo
echo "== Print getOrCreateKey method in interface =="
nl -ba "header/src/main/java/org/zstack/header/keyprovider/EncryptedResourceKeyManager.java" | sed -n '1,120p'
echo
echo "== Print DummyEncryptedResourceKeyManager.getOrCreateKey =="
nl -ba "compute/src/main/java/org/zstack/compute/vm/devices/DummyEncryptedResourceKeyManager.java" | sed -n '1,120p'Repository: MatheMatrix/zstack
Length of output: 235
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedSecretHelper.java"
echo "== Show VolumeEncryptedSecretHelper.java (lines 150-230) =="
sed -n '150,230p' "$FILE"
echo
echo "== Show VolumeEncryptedSecretHelper.java (lines 230-310) =="
sed -n '230,310p' "$FILE"
echo
echo "== Show VolumeEncryptedSecretHelper.java (lines 60-115) =="
sed -n '60,115p' "$FILE"
echo
echo "== Show VolumeEncryptedSecretHelper.java (lines 180-215) =="
sed -n '180,215p' "$FILE"
echo
echo "== EncryptedResourceKeyManager interface =="
sed -n '1,120p' "header/src/main/java/org/zstack/header/keyprovider/EncryptedResourceKeyManager.java"
echo
echo "== DummyEncryptedResourceKeyManager.getOrCreateKey =="
sed -n '1,220p' "compute/src/main/java/org/zstack/compute/vm/devices/DummyEncryptedResourceKeyManager.java"Repository: MatheMatrix/zstack
Length of output: 18376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 实现类定位：找出所有 implements EncryptedResourceKeyManager 的类 =="
rg -n --glob '*.java' 'implements\s+EncryptedResourceKeyManager' . || true
echo
echo "== 方法定位：找出所有对 getOrCreateKey(GetOrCreateResourceKeyContext ctx, ...completion) 的实现 =="
rg -n --glob '*.java' 'void\s+getOrCreateKey\s*\(\s*GetOrCreateResourceKeyContext' . || true
echo
echo "== deleteSecretOnHostBestEffort 调用点 =="
rg -n --glob '*.java' 'deleteSecretOnHostBestEffort\s*\(' storage/src/main/java/org/zstack/storage/encrypt . || true
echo
echo "== 读取/使用 VOLUME_LIBVIRT_SECRET_HOST_TOKEN 的地方 =="
rg -n --glob '*.java' 'VOLUME_LIBVIRT_SECRET_HOST_TOKEN' . || trueRepository: MatheMatrix/zstack
Length of output: 2258
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedExpungeExtension.java"
echo "== Show VolumeEncryptedExpungeExtension.java (50-170) =="
sed -n '50,170p' "$FILE"Repository: MatheMatrix/zstack
Length of output: 4665
不要假设 getOrCreateKey() 的 completion 会在返回前同步触发
VolumeEncryptedSecretHelper.materializeDek() 依赖 completion 回调里写入的 resultRef/errorRef，随后立刻返回；若 EncryptedResourceKeyManager#getOrCreateKey() 在后续实现中改为异步回调，同一调用链会返回 null，并在 defineSecretFromBinding() 里解引用 keyResult.getDekBase64() 触发 NPE。应改为显式等待 completion 完成，或由 key manager 提供真正的阻塞式封装（同步返回结果/异常）。
VolumeEncryptedSecretHelper中 Javadoc 块顺序不匹配：关于 “One-shot resolver … resolveOrDefineSecretForVolume()” 的 Javadoc 紧跟在deleteSecretOnHostBestEffort()之前，需调整到对应方法上。
🤖 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/VolumeEncryptedSecretHelper.java`
around lines 69 - 99, The materializeDek method must not assume
EncryptedResourceKeyManager.getOrCreateKey invokes its completion synchronously;
change materializeDek to block explicitly until the ReturnValueCompletion
callback runs (e.g., use a CountDownLatch or CompletableFuture), populate the
result or error from the callback, handle a timeout and rethrow the ErrorCode as
an OperationFailureException if fail() was called, and return the non-null
ResourceKeyResult so defineSecretFromBinding won't NPE on
keyResult.getDekBase64(); also move the misplaced Javadoc about the "One-shot
resolver … resolveOrDefineSecretForVolume()" so it sits immediately above the
resolveOrDefineSecretForVolume method instead of before
deleteSecretOnHostBestEffort.
Comment on lines
+236
to
+253
| /** | ||
| * One-shot resolver: "give me the libvirt secret UUID for this encrypted | ||
| * volume on this host". Used by both the start-vm path | ||
| * ({@link VolumeEncryptedStartExtension}) and the attach-data-volume | ||
| * path ({@link VolumeEncryptedAttachExtension}). | ||
| * | ||
| * <p>Order: | ||
| * <ol> | ||
| * <li>{@code findKeyVersionByVolume} — fail loudly if the volume has | ||
| * no {@code EncryptedResourceKeyRefVO} binding (would otherwise | ||
| * produce an unreadable qcow2 once attached).</li> | ||
| * <li>{@link #getSecretOnHost} — fast path, hits when the secret has | ||
| * already been defined on the host.</li> | ||
| * <li>{@link #defineSecretFromBinding} — fall back when the secret | ||
| * value is missing on the host (first attach, libvirtd restart, | ||
| * host reboot, migrate to a fresh host).</li> | ||
| * </ol> | ||
| */ |
There was a problem hiding this comment.
resolveOrDefineSecretForVolume() 的说明注释放错位置了。
这段长注释描述的是 resolver 流程，但它现在落在 deleteSecretOnHostBestEffort() 前面，后者自己又有一段 Javadoc；结果真正的 resolveOrDefineSecretForVolume() 反而没有拿到这段说明，IDE 和文档生成都会误导。把这段注释放到 Line 283 前面即可。
🤖 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/VolumeEncryptedSecretHelper.java`
around lines 236 - 253, 将错放的长 Javadoc 注释（描述 resolver 流程并引用
findKeyVersionByVolume、getSecretOnHost、defineSecretFromBinding）从其当前位置（位于
deleteSecretOnHostBestEffort() 之前）移除，并把该注释插入到 resolveOrDefineSecretForVolume()
方法声明之前，确保 deleteSecretOnHostBestEffort() 保留其自身的 Javadoc 注释不被覆盖；这样 IDE
和文档生成器就能正确将该流程说明关联到 resolveOrDefineSecretForVolume()。
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
1 participant
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.
Keep KVM LUKS agent params on InstantiateVolumeMsg / InstantiateVolumeOnPrimaryStorageMsg
(VolumeLuksAgentSpec), premium pre-instantiate DEK path, and LocalStorageKvmBackend wiring.
Shared MergeSnapshotCmd encrypt fields removed; helper script commit-push-three-repos.sh at repo root.
Co-authored-by: Cursor cursoragent@cursor.com
sync from gitlab !9978