<fix>[kvm]: detach resource key from VmHostBackupFileVO when remove TpmVO#3885
<fix>[kvm]: detach resource key from VmHostBackupFileVO when remove TpmVO#3885ZStack-Robot wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough将 TPM 分离/重置的残留清理改为基于 CloudBus 消息的两阶段流程：在 reset 路径先发送非强制的 Changes
Sequence Diagram(s)sequenceDiagram
participant Manager as KvmTpmManager
participant SecureBoot as SecureBootService
participant KeyBackend as tpmKeyBackend
participant DB as BackupStore
Manager->>SecureBoot: VmHostFileDeletionMsg(lastHostFileUuid, forceDelete=false)
SecureBoot-->>Manager: reply (success / error)
alt host-file deletion 成功
Manager->>DB: list TPM-state backup UUIDs for VM
DB-->>Manager: [uuid1, uuid2, ...] or []
alt 有备份
loop 对每个 uuid
Manager->>KeyBackend: cleanEncryptedResourceKey(uuid)
KeyBackend-->>Manager: success / failure
Manager->>SecureBoot: VmHostFileDeletionMsg(uuid, forceDelete=true)
SecureBoot-->>Manager: success / error
Note over Manager: 聚合并记录失败，继续下一个 UUID
end
else 无备份
Note over Manager: 跳过备份清理阶段
end
else host-file deletion 失败
Note over Manager: 记录错误并失败退出该步骤
end
Manager->>Manager: 继续后续链式步骤
代码审查工作量评估🎯 4 (Complex) | ⏱️ ~45 minutes 诗歌
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 2/5 reviews remaining, refill in 29 minutes and 8 seconds. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java (1)
799-850:⚠️ Potential issue | 🔴 Critical严重问题：SQLBatch 会在异步 While 循环完成前执行，导致竞态条件。
第 837-849 行的
SQLBatch和trigger.next()位于While...run()调用之后但在WhileDoneCompletion回调之外。由于While是异步执行的，这些代码会在备份文件删除消息处理完成之前立即执行，导致：
- 数据库记录可能在
VmHostBackupFileDeletionMsg消息处理之前就被删除TpmStateVmHostBackupFileBase.clean()中的cleanEncryptedResourceKey可能无法正确执行正确的做法是将 SQLBatch 移到
WhileDoneCompletion回调内部。🐛 建议的修复
}).run(new WhileDoneCompletion(trigger) { `@Override` public void done(ErrorCodeList errorCodeList) { if (errorCodeList.hasError()) { String details = String.join("\n", transform(errorCodeList.getCauses(), ErrorCode::getReadableDetails)); logger.warn("failed to clean backup files but still continue:\n" + details); } - trigger.next(); - } - }); - - new SQLBatch() { - `@Override` - protected void scripts() { - sql(VmHostFileVO.class) - .eq(VmHostFileVO_.uuid, context.hostFileToDeleteLast.getUuid()) - .delete(); - sql(VmHostBackupFileVO.class) - .eq(VmHostBackupFileVO_.resourceUuid, vmUuid) - .eq(VmHostBackupFileVO_.type, VmHostFileType.TpmState) - .delete(); + new SQLBatch() { + `@Override` + protected void scripts() { + sql(VmHostFileVO.class) + .eq(VmHostFileVO_.uuid, context.hostFileToDeleteLast.getUuid()) + .delete(); + sql(VmHostBackupFileVO.class) + .eq(VmHostBackupFileVO_.resourceUuid, vmUuid) + .eq(VmHostBackupFileVO_.type, VmHostFileType.TpmState) + .delete(); + } + }.execute(); + trigger.next(); } - }.execute(); - trigger.next(); + }); }) .build())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java` around lines 799 - 850, The SQLBatch that deletes VmHostFileVO and VmHostBackupFileVO runs immediately after While.run(), causing a race because the asynchronous While (sending VmHostBackupFileDeletionMsg) may not have finished; move the SQLBatch execution and the trigger.next() call into the WhileDoneCompletion.done(...) callback so the DB deletes occur only after the WhileDoneCompletion finishes processing errors; specifically, relocate the SQLBatch block and the trigger.next() currently after While.run() into the done(ErrorCodeList) method (after handling errorCodeList) to ensure VmHostBackupFileDeletionMsg handling (and cleanEncryptedResourceKey in TpmStateVmHostBackupFileBase) completes before removing records tied to context.hostFileToDeleteLast, VmHostFileVO, and VmHostBackupFileVO.
🧹 Nitpick comments (1)
plugin/kvm/src/main/java/org/zstack/kvm/tpm/TpmStateVmHostBackupFileBase.java (1)
89-94: 日志级别可能过高。
logger.warn()通常用于警告或异常情况，但根据上下文，这只是 TPM 移除时的常规清理操作。建议改为logger.debug()或logger.info()，与该文件中其他日志保持一致（参见第 41、47、58、65、71 行）。此外，日志格式建议统一使用
uuid:%s而非uuid=%s，与同文件其他日志保持风格一致。♻️ 建议的修改
`@Override` public void clean() { - logger.warn(String.format("clean TpmState VmBackupFileVO[uuid=%s]", self.getUuid())); + logger.debug(String.format("clean TpmState VmBackupFileVO[uuid:%s]", self.getUuid())); resourceKeyBackend.cleanEncryptedResourceKey(self.getUuid()); super.clean(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/kvm/src/main/java/org/zstack/kvm/tpm/TpmStateVmHostBackupFileBase.java` around lines 89 - 94, In TpmStateVmHostBackupFileBase.clean(), lower the log level from logger.warn to logger.info (or logger.debug for verbose) and change the message format to match the file style by using "uuid:%s" instead of "uuid=%s"; keep the calls to resourceKeyBackend.cleanEncryptedResourceKey(self.getUuid()) and super.clean() unchanged so only the logger invocation (in method clean) is adjusted for consistency with other logs in this class.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java`:
- Around line 461-474: The loop in KvmTpmManager's flow uses a raw for-loop to
call tpmKeyBackend.cleanEncryptedResourceKey for each backupUuid, lacking the
fault-tolerant behavior used in KvmSecureBootManager; replace the for-loop with
CollectionUtils.safeForEach(backupUuidList,
tpmKeyBackend::cleanEncryptedResourceKey) so failures cleaning one
VmHostBackupFileVO backupUuid won't abort the flow, keeping the detach step
(tpmKeyBackend.detachKeyProviderFromTpm with context.tpmUuid) and the query that
builds backupUuidList (Q.New on VmHostBackupFileVO /
VmHostBackupFileVO_.resourceUuid and VmHostBackupFileVO_.type using
context.vmInstanceUuid and VmHostFileType.TpmState) intact.
---
Outside diff comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java`:
- Around line 799-850: The SQLBatch that deletes VmHostFileVO and
VmHostBackupFileVO runs immediately after While.run(), causing a race because
the asynchronous While (sending VmHostBackupFileDeletionMsg) may not have
finished; move the SQLBatch execution and the trigger.next() call into the
WhileDoneCompletion.done(...) callback so the DB deletes occur only after the
WhileDoneCompletion finishes processing errors; specifically, relocate the
SQLBatch block and the trigger.next() currently after While.run() into the
done(ErrorCodeList) method (after handling errorCodeList) to ensure
VmHostBackupFileDeletionMsg handling (and cleanEncryptedResourceKey in
TpmStateVmHostBackupFileBase) completes before removing records tied to
context.hostFileToDeleteLast, VmHostFileVO, and VmHostBackupFileVO.
---
Nitpick comments:
In
`@plugin/kvm/src/main/java/org/zstack/kvm/tpm/TpmStateVmHostBackupFileBase.java`:
- Around line 89-94: In TpmStateVmHostBackupFileBase.clean(), lower the log
level from logger.warn to logger.info (or logger.debug for verbose) and change
the message format to match the file style by using "uuid:%s" instead of
"uuid=%s"; keep the calls to
resourceKeyBackend.cleanEncryptedResourceKey(self.getUuid()) and super.clean()
unchanged so only the logger invocation (in method clean) is adjusted for
consistency with other logs in this class.
🪄 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: c88b2313-11e5-4f44-991d-677ef0f94669
📒 Files selected for processing (2)
plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.javaplugin/kvm/src/main/java/org/zstack/kvm/tpm/TpmStateVmHostBackupFileBase.java
| .then(Flow.of("detach-resource-key") | ||
| .handle(trigger -> { | ||
| tpmKeyBackend.detachKeyProviderFromTpm(context.tpmUuid); | ||
| List<String> backupUuidList = Q.New(VmHostBackupFileVO.class) | ||
| .eq(VmHostBackupFileVO_.resourceUuid, context.vmInstanceUuid) | ||
| .eq(VmHostBackupFileVO_.type, VmHostFileType.TpmState) | ||
| .select(VmHostBackupFileVO_.uuid) | ||
| .listValues(); | ||
| for (String backupUuid : backupUuidList) { | ||
| tpmKeyBackend.cleanEncryptedResourceKey(backupUuid); | ||
| } | ||
| trigger.next(); | ||
| }) | ||
| .build()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 检查 cleanEncryptedResourceKey 的实现，确认其异常处理行为
ast-grep --pattern $'void cleanEncryptedResourceKey($$$) {
$$$
}'Repository: MatheMatrix/zstack
Length of output: 44
🏁 Script executed:
rg -t java "cleanEncryptedResourceKey" --max-count 20Repository: MatheMatrix/zstack
Length of output: 999
🏁 Script executed:
fd -t f -name "*.java" | xargs rg "interface.*TpmEncryptedResourceKeyBackend|class.*TpmEncryptedResourceKeyBackend" -lRepository: MatheMatrix/zstack
Length of output: 535
🏁 Script executed:
fd -t f -name "*.java" -path "*tpm*" | head -20Repository: MatheMatrix/zstack
Length of output: 293
🏁 Script executed:
cat -n compute/src/main/java/org/zstack/compute/vm/devices/TpmEncryptedResourceKeyBackend.javaRepository: MatheMatrix/zstack
Length of output: 3594
🏁 Script executed:
cat -n compute/src/main/java/org/zstack/compute/vm/devices/DummyTpmEncryptedResourceKeyBackend.javaRepository: MatheMatrix/zstack
Length of output: 2982
🏁 Script executed:
rg -A 10 "safeForEach.*cleanEncryptedResourceKey" plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootManager.javaRepository: MatheMatrix/zstack
Length of output: 1165
🏁 Script executed:
cat -n plugin/kvm/src/main/java/org/zstack/kvm/tpm/TpmStateVmHostBackupFileBase.java | grep -A 5 -B 5 "cleanEncryptedResourceKey"Repository: MatheMatrix/zstack
Length of output: 367
🏁 Script executed:
rg -t java "class.*implements.*TpmEncryptedResourceKeyBackend" --max-count 10Repository: MatheMatrix/zstack
Length of output: 248
🏁 Script executed:
rg -A 20 "public void cleanEncryptedResourceKey" plugin/kvm/src/main/java/org/zstack/kvm/tpm/ -t javaRepository: MatheMatrix/zstack
Length of output: 44
🏁 Script executed:
rg "safeForEach" plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootManager.java -B 5 -A 5Repository: MatheMatrix/zstack
Length of output: 995
🏁 Script executed:
rg -t java "void safeForEach" --max-count 5Repository: MatheMatrix/zstack
Length of output: 206
🏁 Script executed:
rg -B 10 -A 10 "safeForEach" plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootManager.java | head -40Repository: MatheMatrix/zstack
Length of output: 2111
🏁 Script executed:
find plugin/kvm/src/main/java/org/zstack/kvm/tpm -name "*.java" -exec grep -l "implements TpmEncryptedResourceKeyBackend" {} \;Repository: MatheMatrix/zstack
Length of output: 44
🏁 Script executed:
cat -n utils/src/main/java/org/zstack/utils/CollectionUtils.java | grep -A 15 "void safeForEach"Repository: MatheMatrix/zstack
Length of output: 766
🏁 Script executed:
rg -A 5 "interface ForEachFunction" utils/src/main/java/org/zstack/utils/CollectionUtils.javaRepository: MatheMatrix/zstack
Length of output: 44
🏁 Script executed:
rg -t java "public void cleanEncryptedResourceKey" compute/ plugin/ -A 5Repository: MatheMatrix/zstack
Length of output: 749
🏁 Script executed:
cat -n plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java | sed -n '461,475p'Repository: MatheMatrix/zstack
Length of output: 980
🏁 Script executed:
rg "class KvmTpmManager" plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java -A 1Repository: MatheMatrix/zstack
Length of output: 195
应该使用 safeForEach() 以保持与 KvmSecureBootManager 一致的容错处理。
在 KvmSecureBootManager 中，对 cleanEncryptedResourceKey() 的调用使用 safeForEach() 进行包装，该工具方法会捕获所有异常并继续执行，符合 ZStack 的 best-effort 处理模式。但在 KvmTpmManager 的这个流程中（469-470 行）使用了原始 for 循环，缺少同样的容错保护。
建议改为：
CollectionUtils.safeForEach(backupUuidList, tpmKeyBackend::cleanEncryptedResourceKey);这样可以保证即使某个备份文件的密钥清理失败，也不会中断整个流程。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java` around lines
461 - 474, The loop in KvmTpmManager's flow uses a raw for-loop to call
tpmKeyBackend.cleanEncryptedResourceKey for each backupUuid, lacking the
fault-tolerant behavior used in KvmSecureBootManager; replace the for-loop with
CollectionUtils.safeForEach(backupUuidList,
tpmKeyBackend::cleanEncryptedResourceKey) so failures cleaning one
VmHostBackupFileVO backupUuid won't abort the flow, keeping the detach step
(tpmKeyBackend.detachKeyProviderFromTpm with context.tpmUuid) and the query that
builds backupUuidList (Q.New on VmHostBackupFileVO /
VmHostBackupFileVO_.resourceUuid and VmHostBackupFileVO_.type using
context.vmInstanceUuid and VmHostFileType.TpmState) intact.
|
Comment from yaohua.wu: Review: MR !9765 — ZSV-11845结论：REVISION_REQUIRED — 修复方向正确（在 TPM 删除/重置链路上补齐 🔴 Critical1. 完整 handle 体如下： .handle(trigger -> {
List<String> backupUuidList = Q.New(VmHostBackupFileVO.class)
.eq(VmHostBackupFileVO_.resourceUuid, vmUuid)
.eq(VmHostBackupFileVO_.type, VmHostFileType.TpmState)
.select(VmHostBackupFileVO_.uuid)
.listValues();
if (isEmpty(backupUuidList)) {
trigger.next();
return;
}
new While<>(backupUuidList).each((uuid, whileCompletion) -> {
VmHostBackupFileDeletionMsg deletionMsg = new VmHostBackupFileDeletionMsg();
deletionMsg.setUuid(uuid);
deletionMsg.setForceDelete(true);
bus.makeLocalServiceId(deletionMsg, VmInstanceConstant.SECURE_BOOT_SERVICE_ID);
bus.send(deletionMsg, new CloudBusCallBack(whileCompletion) { ... });
}).run(new WhileDoneCompletion(trigger) {
@Override public void done(ErrorCodeList errorCodeList) {
...
trigger.next(); // ← 异步路径
}
});
new SQLBatch() { // ← 同步立即执行
@Override protected void scripts() {
sql(VmHostFileVO.class)
.eq(VmHostFileVO_.uuid, context.hostFileToDeleteLast.getUuid())
.delete();
sql(VmHostBackupFileVO.class)
.eq(VmHostBackupFileVO_.resourceUuid, vmUuid)
.eq(VmHostBackupFileVO_.type, VmHostFileType.TpmState)
.delete();
}
}.execute();
trigger.next(); // ← 同步路径，与上面 WhileDoneCompletion 重复
})非空
修复建议（最小改动）： new While<>(backupUuidList).each(...).run(new WhileDoneCompletion(trigger) {
@Override public void done(ErrorCodeList errorCodeList) {
if (errorCodeList.hasError()) {
String details = String.join("\n",
transform(errorCodeList.getCauses(), ErrorCode::getReadableDetails));
logger.warn("failed to clean backup files but still continue:\n" + details);
}
trigger.next();
}
});
// 删除下面整段 SQLBatch 与多余的 trigger.next()即彻底删除 🟡 Warning2. EncryptedResourceKey 在同一个流程里被清理两次 —
List<String> backupUuidList = Q.New(VmHostBackupFileVO.class)
.eq(VmHostBackupFileVO_.resourceUuid, context.vmInstanceUuid)
.eq(VmHostBackupFileVO_.type, VmHostFileType.TpmState)
.select(VmHostBackupFileVO_.uuid)
.listValues();
for (String backupUuid : backupUuidList) {
tpmKeyBackend.cleanEncryptedResourceKey(backupUuid);
}而后续走 public void clean() {
logger.warn(...);
resourceKeyBackend.cleanEncryptedResourceKey(self.getUuid());
super.clean();
}如果 注意：这条与 Critical #1 是耦合的——SQLBatch 一删，删除路径就只剩消息一条线，clean() 必然会被调，重复调用问题就暴露出来了。 3. logger.warn(String.format("clean TpmState VmBackupFileVO[uuid=%s]", self.getUuid()));
4. 通配符 import — import org.zstack.header.vm.additions.*;原本 8 条显式 import 被合成一条通配符。zstack 代码库一致使用显式 import，建议保留显式列表（仅新增 5. 验证不充分，长青脚本未跑过 ZSV-11845 在 4/15 与 4/26 反复出现"还有场景遗漏"。Jira 评论里张鹏翊 4/23 给出了完整 7 步 CLI 长青脚本（创建 nkp1 → vm1/vm2 各种快照/备份/模板 → 删 nkp1 → 各种克隆 → 彻底删除 → 创建 nkp2 → 全局 rekey 应零脏数据），4/29 又给了清理残留数据的 SQL。本 MR 没有附带任何自动化 case，premium 侧 !13707 / !13708 已经把 Tpm/EncryptedResourceKeyRefVO 的 clean 放开，恰好可以把这条长青补成 zsv-system-test：
合入前请至少在测试环境（如 ZSV-11845 评论里那台 172.24.251.247）人工跑完一遍并把结果贴回 issue。 6.
.then(Flow.of("remove-db-records")
.handle(trigger -> {
new SQLBatch() {
@Override protected void scripts() {
sql(TpmVO.class).eq(TpmVO_.uuid, context.tpmUuid).delete();
sql(VmHostFileVO.class).eq(VmHostFileVO_.vmInstanceUuid, ...).delete();
sql(VmHostBackupFileVO.class).eq(VmHostBackupFileVO_.resourceUuid, ...).delete();
}
}.execute();
trigger.next();
})按 ZSV-11845 4/28 wenhao.zhang 的整体方向（"大量清理 SQL.delete 原生调用，改用 cascade 和 VmHostFileDeletionMsg 体系"），删除路径理应一并迁移；这里继续用 SQL.delete 等于绕过 如果本次范围有限不打算改，请至少在 PR 描述里挂一个跟进 issue/MR，避免下一次又重蹈"还有场景遗漏"。 🟢 Suggestion7. 注释表达可读性 — deletionMsg.setForceDelete(true); // set true when VmHostFileVO deleted读起来像"当 VmHostFileVO 被删除时设为 true"。建议改成： 8. 现在它既 detach TPM key provider，又遍历清理所有 TpmState backup 的 EncryptedResourceKey。建议拆成两个 step（如 兼容性 / Upstream Freshness
VerdictREVISION_REQUIRED — Critical #1 必须修；建议同时把 Warning #2、#3、#4 一并收敛，并补一次完整长青验证（Warning #5）后再合入。 🤖 Robot Reviewer |
MatheMatrix
force-pushed
the
sync/wenhao.zhang/c-4
branch
from
db04a1a to
cb6591a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java (3)
829-831: 建议将日志级别从warn调整为info或debug。此处的清理失败是被预期并容忍的（代码继续执行），使用
logger.warn级别可能导致运维告警噪音。根据 PR 评审意见 (Warning#3)，建议改为info或debug级别。♻️ 建议的修改
if (errorCodeList.hasError()) { String details = String.join("\n", transform(errorCodeList.getCauses(), ErrorCode::getReadableDetails)); - logger.warn("failed to clean backup files but still continue:\n" + details); + logger.info("failed to clean backup files but still continue:\n" + details); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java` around lines 829 - 831, The cleanup failure in KvmTpmManager is expected and tolerated, so change the logging level from logger.warn to a lower level (logger.info or logger.debug) in the block that checks errorCodeList.hasError(); specifically update the call that currently logs "failed to clean backup files but still continue:\n" + details (constructed from transform(errorCodeList.getCauses(), ErrorCode::getReadableDetails)) to use logger.info or logger.debug so it doesn’t generate unnecessary alerts.
815-815: 建议完善注释说明。当前注释
// set true when VmHostFileVO deleted较为简略。建议补充说明：由于父级VmHostFileVO已在上一步被删除，备份文件的清理失败应被容忍以避免残留数据。📝 建议的注释
- deletionMsg.setForceDelete(true); // set true when VmHostFileVO deleted + // Parent VmHostFileVO was already deleted; tolerate cleanup failures to avoid orphan data + deletionMsg.setForceDelete(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java` at line 815, 在 KvmTpmManager.java 中补充 deletionMsg.setForceDelete(true) 的注释，说明为什么在 VmHostFileVO 已被删除的情况下要将 forceDelete 设为 true：由于父对象 VmHostFileVO 在前一步已被删除，后续对备份/主机文件的删除操作即便失败也应被容忍以避免留下未预期的数据或阻塞流程；请在 deletionMsg.setForceDelete(true) 旁加入一条清晰注释，引用 VmHostFileVO、deletionMsg.setForceDelete(true) 和相关上下文以便维护者理解其原因和意图。
48-48: 建议使用显式导入替代通配符导入。通配符导入
org.zstack.header.vm.additions.*不符合代码库的一致性要求。根据 PR 评审意见 (Warning#4)，应使用显式导入以保持与其他代码的风格一致。♻️ 建议的修改
-import org.zstack.header.vm.additions.*; +import org.zstack.header.vm.additions.VmHostBackupFileDeletionMsg; +import org.zstack.header.vm.additions.VmHostBackupFileVO; +import org.zstack.header.vm.additions.VmHostBackupFileVO_; +import org.zstack.header.vm.additions.VmHostFileDeletionMsg; +import org.zstack.header.vm.additions.VmHostFileInventory; +import org.zstack.header.vm.additions.VmHostFileOperation; +import org.zstack.header.vm.additions.VmHostFileType; +import org.zstack.header.vm.additions.VmHostFileVO; +import org.zstack.header.vm.additions.VmHostFileVO_;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java` at line 48, 当前文件在 KvmTpmManager.java 中使用了通配符导入 org.zstack.header.vm.additions.*；请将该通配符替换为对本文件实际使用到的每个具体类型的显式导入（在 KvmTpmManager 类中查找使用的类型名并为每个添加单独的 import 语句），确保删除 org.zstack.header.vm.additions.* 并仅保留具体类的导入以符合项目风格一致性要求。
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java`:
- Around line 829-831: The cleanup failure in KvmTpmManager is expected and
tolerated, so change the logging level from logger.warn to a lower level
(logger.info or logger.debug) in the block that checks errorCodeList.hasError();
specifically update the call that currently logs "failed to clean backup files
but still continue:\n" + details (constructed from
transform(errorCodeList.getCauses(), ErrorCode::getReadableDetails)) to use
logger.info or logger.debug so it doesn’t generate unnecessary alerts.
- Line 815: 在 KvmTpmManager.java 中补充 deletionMsg.setForceDelete(true) 的注释，说明为什么在
VmHostFileVO 已被删除的情况下要将 forceDelete 设为 true：由于父对象 VmHostFileVO
在前一步已被删除，后续对备份/主机文件的删除操作即便失败也应被容忍以避免留下未预期的数据或阻塞流程；请在
deletionMsg.setForceDelete(true) 旁加入一条清晰注释，引用
VmHostFileVO、deletionMsg.setForceDelete(true) 和相关上下文以便维护者理解其原因和意图。
- Line 48: 当前文件在 KvmTpmManager.java 中使用了通配符导入
org.zstack.header.vm.additions.*；请将该通配符替换为对本文件实际使用到的每个具体类型的显式导入（在 KvmTpmManager
类中查找使用的类型名并为每个添加单独的 import 语句），确保删除 org.zstack.header.vm.additions.*
并仅保留具体类的导入以符合项目风格一致性要求。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 991b4222-9f2c-424e-8748-707282934bc2
📒 Files selected for processing (2)
plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.javaplugin/kvm/src/main/java/org/zstack/kvm/tpm/TpmStateVmHostBackupFileBase.java
🚧 Files skipped from review as they are similar to previous changes (1)
- plugin/kvm/src/main/java/org/zstack/kvm/tpm/TpmStateVmHostBackupFileBase.java
MatheMatrix
force-pushed
the
sync/wenhao.zhang/c-4
branch
from
cb6591a to
4a8bc62
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java (1)
461-470:⚠️ Potential issue | 🟠 Major这里的备份密钥清理不应该让删 TPM 流程中途失败。
这和上一轮在同一段提到的问题一致：
detachKeyProviderFromTpm()已经先执行了，如果某个cleanEncryptedResourceKey(backupUuid)抛异常，后面的remove-db-records就不会执行，最终会留下“provider 已解绑，但 TPM / 备份记录还在”的半清理状态。这里更适合做成 best-effort，逐个记录失败并继续。♻️ 建议修改
- for (String backupUuid : backupUuidList) { - tpmKeyBackend.cleanEncryptedResourceKey(backupUuid); - } + CollectionUtils.safeForEach(backupUuidList, tpmKeyBackend::cleanEncryptedResourceKey);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java` around lines 461 - 470, 在 detach-resource-key 的处理器里，cleanEncryptedResourceKey(backupUuid) 不能让整个流程因单个备份失败而中断：把对 tpmKeyBackend.cleanEncryptedResourceKey 的调用改为对每个 backupUuid 分别做 best-effort（在循环内用 try/catch 捕获异常并记录错误，继续下一个），确保 detachKeyProviderFromTpm(context.tpmUuid) 已执行后即便某些清理失败也不会阻止后续的 remove-db-records 或后续流程运行；引用符号：detachKeyProviderFromTpm, cleanEncryptedResourceKey, context.vmInstanceUuid, Flow.of("detach-resource-key")。
🧹 Nitpick comments (1)
plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java (1)
48-48: 避免新增通配符导入。这里实际只新增了少量
vm.additions类型，继续使用org.zstack.header.vm.additions.*会让依赖面不透明，也和本类其余导入的显式风格不一致，建议改回按需导入。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java` at line 48, Replace the wildcard import "org.zstack.header.vm.additions.*" in KvmTpmManager.java with explicit imports: remove the star import and add individual import statements for each class from org.zstack.header.vm.additions that this file actually uses (identify those types by scanning KvmTpmManager.java for references to classes from that package and import them explicitly). Keep the rest of the file's import style consistent and ensure no unused imports remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java`:
- Around line 461-470: 在 detach-resource-key
的处理器里，cleanEncryptedResourceKey(backupUuid) 不能让整个流程因单个备份失败而中断：把对
tpmKeyBackend.cleanEncryptedResourceKey 的调用改为对每个 backupUuid 分别做
best-effort（在循环内用 try/catch 捕获异常并记录错误，继续下一个），确保
detachKeyProviderFromTpm(context.tpmUuid) 已执行后即便某些清理失败也不会阻止后续的 remove-db-records
或后续流程运行；引用符号：detachKeyProviderFromTpm, cleanEncryptedResourceKey,
context.vmInstanceUuid, Flow.of("detach-resource-key")。
---
Nitpick comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java`:
- Line 48: Replace the wildcard import "org.zstack.header.vm.additions.*" in
KvmTpmManager.java with explicit imports: remove the star import and add
individual import statements for each class from org.zstack.header.vm.additions
that this file actually uses (identify those types by scanning
KvmTpmManager.java for references to classes from that package and import them
explicitly). Keep the rest of the file's import style consistent and ensure no
unused imports remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: aee8e04b-9cb1-4ab3-9901-322e5e896d2b
📒 Files selected for processing (2)
plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.javaplugin/kvm/src/main/java/org/zstack/kvm/tpm/TpmStateVmHostBackupFileBase.java
🚧 Files skipped from review as they are similar to previous changes (1)
- plugin/kvm/src/main/java/org/zstack/kvm/tpm/TpmStateVmHostBackupFileBase.java
MatheMatrix
force-pushed
the
sync/wenhao.zhang/c-4
branch
from
4a8bc62 to
97f95b6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java (1)
479-481:⚠️ Potential issue | 🟠 Major备份密钥清理应改为容错继续，避免单点失败中断整条 TPM 删除链路。
这里逐个直接调用
tpmKeyBackend.cleanEncryptedResourceKey(backupUuid)，任一异常都可能导致后续remove-db-records不执行，产生“部分清理”状态。建议改为 best-effort（不中断主流程）并记录失败。建议修改
- for (String backupUuid : backupUuidList) { - tpmKeyBackend.cleanEncryptedResourceKey(backupUuid); - } + CollectionUtils.safeForEach(backupUuidList, backupUuid -> { + try { + tpmKeyBackend.cleanEncryptedResourceKey(backupUuid); + } catch (Exception e) { + logger.warn(String.format("failed to clean encrypted key for backup[uuid:%s], continue tpm removal", backupUuid), e); + } + });Based on learnings: In ZStack, calls to
VmInstanceResourceMetadataManager.deleteVmResourceMetadata(...)commonly treat failures as best‑effort and do not block the primary operation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java` around lines 479 - 481, The loop in KvmTpmManager that calls tpmKeyBackend.cleanEncryptedResourceKey(backupUuid) must be made best‑effort: wrap each call in a try/catch so a thrown exception is logged (including backupUuid and exception details) but not rethrown, allowing the loop to continue and the subsequent remove-db-records step to run; optionally collect/log aggregate failures after the loop for visibility but do not abort the TPM delete flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java`:
- Around line 810-811: In KvmTpmManager remove the premature branch skip that
checks `.skipIf(data -> context.hostFileToDeleteLast == null)` so backup-file
deletion does not get skipped when only backup remnants (VmHostBackupFileVO)
exist; instead let the branch execute and let the existing
`isEmpty(backupUuidList)` early-return logic handle no-op cases; update the
pipeline where `.skipIf(data -> context.hostFileToDeleteLast == null)` appears
(and the similar block at the 812-820 region) to eliminate that skip and keep
the subsequent `.handle(trigger -> { ... isEmpty(backupUuidList) ... })`
behavior unchanged.
---
Duplicate comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java`:
- Around line 479-481: The loop in KvmTpmManager that calls
tpmKeyBackend.cleanEncryptedResourceKey(backupUuid) must be made best‑effort:
wrap each call in a try/catch so a thrown exception is logged (including
backupUuid and exception details) but not rethrown, allowing the loop to
continue and the subsequent remove-db-records step to run; optionally
collect/log aggregate failures after the loop for visibility but do not abort
the TPM delete flow.
🪄 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: 9fc56e92-b35b-4515-81d1-a5052fb9352e
📒 Files selected for processing (2)
plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.javaplugin/kvm/src/main/java/org/zstack/kvm/tpm/TpmStateVmHostBackupFileBase.java
✅ Files skipped from review due to trivial changes (1)
- plugin/kvm/src/main/java/org/zstack/kvm/tpm/TpmStateVmHostBackupFileBase.java
| .skipIf(data -> context.hostFileToDeleteLast == null) | ||
| .handle(trigger -> { |
There was a problem hiding this comment.
备份删除不应依赖 hostFileToDeleteLast，否则会漏清理“仅备份残留”场景。
当前 .skipIf(data -> context.hostFileToDeleteLast == null) 会在没有 last host file 时直接跳过整个备份删除分支，但这时仍可能存在 VmHostBackupFileVO 残留。建议移除该 skip，继续用后面的 isEmpty(backupUuidList) 早退控制。
建议修改
.then(Flow.of("remove-backups-db-records-for-remains")
- .skipIf(data -> context.hostFileToDeleteLast == null)
.handle(trigger -> {
List<String> backupUuidList = Q.New(VmHostBackupFileVO.class)
.eq(VmHostBackupFileVO_.resourceUuid, vmUuid)
.eq(VmHostBackupFileVO_.type, VmHostFileType.TpmState)
.select(VmHostBackupFileVO_.uuid)
.listValues();Also applies to: 812-820
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java` around lines
810 - 811, In KvmTpmManager remove the premature branch skip that checks
`.skipIf(data -> context.hostFileToDeleteLast == null)` so backup-file deletion
does not get skipped when only backup remnants (VmHostBackupFileVO) exist;
instead let the branch execute and let the existing `isEmpty(backupUuidList)`
early-return logic handle no-op cases; update the pipeline where `.skipIf(data
-> context.hostFileToDeleteLast == null)` appears (and the similar block at the
812-820 region) to eliminate that skip and keep the subsequent `.handle(trigger
-> { ... isEmpty(backupUuidList) ... })` behavior unchanged.
…pmVO Resolves: ZSV-11845 Change-Id: I6f6e707368797a796662736d68666d62697a796f
MatheMatrix
force-pushed
the
sync/wenhao.zhang/c-4
branch
from
97f95b6 to
e3f138a
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java (1)
474-481:⚠️ Potential issue | 🟠 Major单个备份 key 清理异常会中断整个 TPM 删除链。
这里是同步裸循环；只要某个
cleanEncryptedResourceKey()抛异常，后面的remove-db-records就不会执行，最终会留下“已 detach 但未删库”的半清理状态。建议把这段改成逐个容错的 best-effort 处理，不要让单个 backup 失败阻塞主删除路径。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java` around lines 474 - 481, The loop in KvmTpmManager that iterates backupUuidList and calls tpmKeyBackend.cleanEncryptedResourceKey(backupUuid) must be made best-effort per-item: wrap each cleanEncryptedResourceKey call in its own try-catch, log the failure (include backupUuid and exception) and continue so a single failure doesn't abort the whole TPM delete path; ensure subsequent remove-db-records logic that follows the loop still runs regardless of individual cleanup failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java`:
- Around line 474-481: The loop in KvmTpmManager that iterates backupUuidList
and calls tpmKeyBackend.cleanEncryptedResourceKey(backupUuid) must be made
best-effort per-item: wrap each cleanEncryptedResourceKey call in its own
try-catch, log the failure (include backupUuid and exception) and continue so a
single failure doesn't abort the whole TPM delete path; ensure subsequent
remove-db-records logic that follows the loop still runs regardless of
individual cleanup failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0ce9b742-bfc6-44d5-97d0-450c032c3a72
📒 Files selected for processing (2)
plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.javaplugin/kvm/src/main/java/org/zstack/kvm/tpm/TpmStateVmHostBackupFileBase.java
✅ Files skipped from review due to trivial changes (1)
- plugin/kvm/src/main/java/org/zstack/kvm/tpm/TpmStateVmHostBackupFileBase.java
2 participants
Resolves: ZSV-11845
Change-Id: I6f6e707368797a796662736d68666d62697a796f
sync from gitlab !9765