<refactor>[kvm]: improve TPM removal by cleaning host files before DB deletion#3667
Closed
MatheMatrix wants to merge 1 commit into
Closed
Conversation
Walkthrough
Changes
Sequence Diagram(s)sequenceDiagram
participant Manager as KvmTpmManager
participant DB as Database
participant Host as KVM Host Agent
Manager->>DB: 查询 `VmHostFileVO` (type=TpmState) for VM
DB-->>Manager: 返回 hostFiles 列表
Manager->>Manager: 按 hostUuid 分组 hostFiles
loop 每个 host
Manager->>Host: WriteVmHostFileContentCmd(delete paths)
alt host 成功
Host-->>Manager: WriteVmHostFileContentRsp(success)
else host 失败
Host-->>Manager: WriteVmHostFileContentRsp(failure)
Manager-->>Manager: 记录警告，不中断流程
end
end
Manager->>DB: 删除 `TpmVO` 与 `VmHostFileVO`/`VmHostBackupFileVO` (type=TpmState)
DB-->>Manager: 删除完成
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java (1)
356-359: 可选优化：使用 Stream API 进行分组。当前使用
for循环配合computeIfAbsent进行分组，可以考虑使用 Java Stream 的Collectors.groupingBy使代码更简洁。♻️ 可选重构建议
- Map<String, List<VmHostFileVO>> filesByHost = new HashMap<>(); - for (VmHostFileVO file : context.hostFiles) { - filesByHost.computeIfAbsent(file.getHostUuid(), k -> new ArrayList<>()).add(file); - } + Map<String, List<VmHostFileVO>> filesByHost = context.hostFiles.stream() + .collect(java.util.stream.Collectors.groupingBy(VmHostFileVO::getHostUuid));🤖 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 356 - 359, Replace the manual grouping loop in KvmTpmManager that builds filesByHost from context.hostFiles using computeIfAbsent with a Stream-based collect: use context.hostFiles.stream().collect(Collectors.groupingBy(VmHostFileVO::getHostUuid)) to produce Map<String, List<VmHostFileVO>> filesByHost; add the necessary import for java.util.stream.Collectors and ensure the variable type remains Map<String, List<VmHostFileVO>> so existing code using filesByHost continues to work.
🤖 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 356-359: Replace the manual grouping loop in KvmTpmManager that
builds filesByHost from context.hostFiles using computeIfAbsent with a
Stream-based collect: use
context.hostFiles.stream().collect(Collectors.groupingBy(VmHostFileVO::getHostUuid))
to produce Map<String, List<VmHostFileVO>> filesByHost; add the necessary import
for java.util.stream.Collectors and ensure the variable type remains Map<String,
List<VmHostFileVO>> so existing code using filesByHost continues to work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8b6689c6-e594-4943-9fb5-ad03cc5286dc
📒 Files selected for processing (1)
plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java
Collaborator
|
Comment from ye.zou: Code Review
结论: CONDITIONAL |
… deletion Refactor the removeTpmFromVm method to split the DB removal flow into three steps for better resource cleanup: 1. Collect VmHostFileVO and VmHostBackupFileVO for TPM state files 2. Send delete commands to hosts to clean up files on disk 3. Remove database records regardless of command success This ensures host files are cleaned up before DB records are deleted. Removed NvRam file deletion to avoid conflicts with secure boot. Related: ZSV-11310 Resolves: ZSV-11622 Change-Id: I7865626a687376746c746a706b74676b6b7a6a71
MatheMatrix
force-pushed
the
sync/wenhao.zhang/zsv-ldap-4
branch
from
8663f8b to
efc4fc2
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java (2)
387-413:⚠️ Potential issue | 🟠 Major关于 detach-resource-key 与主机删除命令失败的场景。
当前流程顺序：
send-delete-commands-to-hosts→detach-resource-key→remove-db-records。潜在风险：如果所有主机删除命令都失败（如主机不可达），流程仍会继续执行：
- 解除密钥绑定（
detach-resource-key）- 删除数据库记录
这会导致 TPM 状态文件残留在主机上但失去密钥保护。
建议评估以下方案之一：
- 在
detach-resource-key步骤添加回滚逻辑，如果后续失败可重新绑定密钥；- 如果认为这是可接受的 trade-off（优先保证 DB 一致性），请添加注释说明设计意图。
🤖 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 387 - 413, The detach-resource-key step (Flow.of("detach-resource-key") using tpmKeyBackend.detachKeyProviderFromTpm(context.tpmUuid)) executes unconditionally after send-delete-commands-to-hosts and before remove-db-records, which can leave TPM state files on hosts unprotected if all host delete commands failed; either implement rollback binding (capture result of host deletes and, on downstream failure, call tpmKeyBackend.attachKeyProviderToTpm or equivalent during error/compensation) so detach is reversible, or if the intended design is to prefer DB consistency over host state, add a clear comment near Flow.of("detach-resource-key") and Flow.of("remove-db-records") referencing send-delete-commands-to-hosts to document the trade-off and why detach proceeds despite host delete failures.
299-310:⚠️ Potential issue | 🟡 MinorVmHostBackupFileVO 处理不一致。
context.hostFiles仅收集VmHostFileVO，但在remove-db-records流程中同时删除了VmHostBackupFileVO的数据库记录（第 405-408 行）。然而，send-delete-commands-to-hosts流程仅对hostFiles发送删除命令，未包含备份文件。这会导致备份文件的主机端文件成为孤立文件，建议：
- 在
collect-vm-host-files中同时收集VmHostBackupFileVO并在主机端删除；或- 如果备份文件由其他机制清理（如
VmHostFileTracker），请移除对VmHostBackupFileVO的查询和删除，或添加注释说明。🤖 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 299 - 310, The RemoveTpmFromVmContext currently only collects VmHostFileVO into context.hostFiles but remove-db-records also deletes VmHostBackupFileVO entries, leading to orphaned host files; update the collect-vm-host-files flow to also query and include VmHostBackupFileVO into the context (e.g., add a separate list like backupHostFiles on RemoveTpmFromVmContext) and extend send-delete-commands-to-hosts to send delete commands for those backup files as well, or alternatively remove the VmHostBackupFileVO query/deletion in remove-db-records or add a clear comment explaining that backup files are cleaned by another mechanism (refer to RemoveTpmFromVmContext, hostFiles, collect-vm-host-files, send-delete-commands-to-hosts, remove-db-records, VmHostBackupFileVO, VmHostFileVO).
🧹 Nitpick comments (1)
plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java (1)
339-386: 建议添加注释说明 "best-effort" 语义。当前实现在主机删除命令失败时仅记录警告并继续（第 373-377 行），这是预期的容错设计。但代码中缺少对此语义的说明，后续维护者可能误解为遗漏错误处理。
建议在流程开头或失败处理处添加简短注释：
.then(Flow.of("send-delete-commands-to-hosts") + // Best-effort: host command failures are tolerated to ensure DB cleanup proceeds. .skipIf(data -> context.hostFiles.isEmpty()) .handle(trigger -> {🤖 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 339 - 386, Add a short, clear comment in KvmTpmManager describing that the "send-delete-commands-to-hosts" flow performs best-effort cleanup: when KvmCommandSender.send(...) for WriteVmHostFileContentCmd fails the code intentionally logs a warning in the ReturnValueCompletion.fail(...) block and continues DB cleanup; place the comment either at the start of the Flow.of("send-delete-commands-to-hosts") block or immediately above the fail(...) handler to document that failures are non-fatal and expected behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java`:
- Around line 387-413: The detach-resource-key step
(Flow.of("detach-resource-key") using
tpmKeyBackend.detachKeyProviderFromTpm(context.tpmUuid)) executes
unconditionally after send-delete-commands-to-hosts and before
remove-db-records, which can leave TPM state files on hosts unprotected if all
host delete commands failed; either implement rollback binding (capture result
of host deletes and, on downstream failure, call
tpmKeyBackend.attachKeyProviderToTpm or equivalent during error/compensation) so
detach is reversible, or if the intended design is to prefer DB consistency over
host state, add a clear comment near Flow.of("detach-resource-key") and
Flow.of("remove-db-records") referencing send-delete-commands-to-hosts to
document the trade-off and why detach proceeds despite host delete failures.
- Around line 299-310: The RemoveTpmFromVmContext currently only collects
VmHostFileVO into context.hostFiles but remove-db-records also deletes
VmHostBackupFileVO entries, leading to orphaned host files; update the
collect-vm-host-files flow to also query and include VmHostBackupFileVO into the
context (e.g., add a separate list like backupHostFiles on
RemoveTpmFromVmContext) and extend send-delete-commands-to-hosts to send delete
commands for those backup files as well, or alternatively remove the
VmHostBackupFileVO query/deletion in remove-db-records or add a clear comment
explaining that backup files are cleaned by another mechanism (refer to
RemoveTpmFromVmContext, hostFiles, collect-vm-host-files,
send-delete-commands-to-hosts, remove-db-records, VmHostBackupFileVO,
VmHostFileVO).
---
Nitpick comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java`:
- Around line 339-386: Add a short, clear comment in KvmTpmManager describing
that the "send-delete-commands-to-hosts" flow performs best-effort cleanup: when
KvmCommandSender.send(...) for WriteVmHostFileContentCmd fails the code
intentionally logs a warning in the ReturnValueCompletion.fail(...) block and
continues DB cleanup; place the comment either at the start of the
Flow.of("send-delete-commands-to-hosts") block or immediately above the
fail(...) handler to document that failures are non-fatal and expected behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d1a84fec-1771-4dac-a9f4-efc186c3c56b
📒 Files selected for processing (1)
plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refactor the removeTpmFromVm method to split the DB removal flow
into three steps for better resource cleanup:
This ensures host files are cleaned up before DB records are deleted.
Removed NvRam file deletion to avoid conflicts with secure boot.
Related: ZSV-11310
Resolves: ZSV-11622
Change-Id: I7865626a687376746c746a706b74676b6b7a6a71
sync from gitlab !9527