<fix>[kvm]: remove TPM from VM individually instead of batch delete#3688
<fix>[kvm]: remove TPM from VM individually instead of batch delete#3688MatheMatrix wants to merge 1 commit intofeature-zsv-5.0.0-vm-support-vtpm-and-secucebootfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughKvmTpmManager 在 Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant Caller
end
rect rgba(200,255,200,0.5)
participant KvmTpmManager
participant WhileLoop as While
end
rect rgba(255,200,200,0.5)
participant VMService as removeTpmFromVm
participant DB
end
Caller->>KvmTpmManager: 触发 rollback for "persist-TPM-VO"
KvmTpmManager->>WhileLoop: 遍历 reply.getInventories()
WhileLoop->>VMService: removeTpmFromVm(RemoveTpmFromVmContext{vmUuid,tpmUuid,force=true})
VMService->>VMService: skip "check-vm-status" if context.force
VMService->>DB: 删除/更新 TPM 关联记录
DB-->>VMService: 返回 成功/失败
VMService-->>WhileLoop: completion.done()（成功或失败均完成）
WhileLoop-->>KvmTpmManager: 所有 TPM 操作完成
KvmTpmManager->>Caller: trigger.rollback()
预估代码审查工作量🎯 3 (Moderate) | ⏱️ ~20 分钟 诗句
🚥 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)
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
🤖 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-475: The rollback path can leave orphaned TpmVO/VmHostFile*
records because removeTpmFromVm(RemoveTpmFromVmContext) performs VM state checks
and detachKeyProviderFromTpm() and returns on failure; modify KvmTpmManager to
perform a best-effort cleanup when that call fails in the While.each fail()
handler: add a new helper (e.g., forceRemoveTpmFromVm or forceDeleteTpmRecords)
that accepts the same RemoveTpmFromVmContext and deletes the TpmVO and related
VmHostFile* rows directly (skip VM state validation and key-detach logic), or
alternatively execute the equivalent SQL delete/update statements here in the
fail() block as a best-effort cleanup, log any errors but do not propagate them,
and call whileCompletion.done() after attempting the force-delete so rollback
does not leave orphan records.
🪄 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: fbd0b81e-824d-4cd4-b231-8be364c84249
📒 Files selected for processing (1)
plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java
|
Comment from yaohua.wu: Review: MR !9550 — ZSV-11439
变更概述将 Critical1. [KvmTpmManager.java:~461] VM 状态校验会阻断 rollback 清理，导致孤儿记录
当 VM 状态校验失败时：
影响: 克隆失败后数据库中残留孤儿 TpmVO 记录，对应的 VM 可能已经被清理，导致数据不一致。 建议:
Warning1. [KvmTpmManager.java:~461-475]
建议: 确认 2. [KvmTpmManager.java:~460] rollback 路径执行了不必要的 host 操作 Clone 流程中 进一步佐证 rollback 应该使用更轻量级的清理路径。 Suggestion1. [KvmTpmManager.java:~460] 考虑使用 每个 TPM 属于不同的目标 VM（ 架构思考这个变更的意图是好的 —— rollback 时做完整清理（而非仅删 DB 记录）更加健壮。但 核心问题是 正常路径 和 rollback 路径 对清理的要求不同：
建议拆分或参数化 Verdict: REVISION_REQUIRED🤖 Robot Reviewer |
MatheMatrix
force-pushed
the
sync/wenhao.zhang/zsv-ldap-3
branch
2 times, most recently
from
b8c8bb9 to
d2feb84
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 305-309: The VM-state check in RemoveTpmFromVmContext is inverted:
change the condition that currently uses context.ignoreError to require the
check only when ignoreError is false (i.e. use !context.ignoreError && <vm state
check>) so rollback and normal RemoveTpmMsg paths correctly enforce VM-state
validation; also consider renaming the boolean field ignoreError to
skipVmStateCheck (and update all references) if its purpose is only to bypass VM
state validation to avoid future misuse.
🪄 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: 8b6c33c4-742a-40c5-a4e4-44946d5e4f37
📒 Files selected for processing (1)
plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java
| static class RemoveTpmFromVmContext { | ||
| String vmInstanceUuid; | ||
| String tpmUuid; | ||
| boolean ignoreError; | ||
|
|
There was a problem hiding this comment.
ignoreError 的判断写反了，这个回滚修复实际上没有生效。
Line 328 现在只有 ignoreError=true 才会因为 VM 状态不支持而 fail()，而默认 false 的普通 RemoveTpmMsg 路径反而完全跳过了状态校验。这样 Line 467 的 clone rollback 在 transient/Creating 之类的状态下仍会失败，而外层 fail() 只记录 warning 就继续，刚创建的 TpmVO 还是会残留；同时正常删除路径也丢掉了原有的状态保护。这里应该改成 !context.ignoreError && ...。另外，这个标志如果只是给 rollback 跳过状态检查用，名字也建议收窄成 skipVmStateCheck 之类，避免后续再次误用。
🛠️ 建议修改
static class RemoveTpmFromVmContext {
String vmInstanceUuid;
String tpmUuid;
- boolean ignoreError;
+ boolean skipVmStateCheck;
@@
- if (context.ignoreError && !SUPPORT_VM_STATES_FOR_TPM_OPERATION.contains(vm.getState())) {
+ if (!context.skipVmStateCheck && !SUPPORT_VM_STATES_FOR_TPM_OPERATION.contains(vm.getState())) {
trigger.fail(err(VM_STATE_ERROR,
"The current VM state does not support removing TPM operations")
.withOpaque("support.vm.state", SUPPORT_VM_STATES_FOR_TPM_OPERATION));
return;
}
@@
- removeContext.ignoreError = true;
+ removeContext.skipVmStateCheck = true;Also applies to: 328-333, 463-468
🤖 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
305 - 309, The VM-state check in RemoveTpmFromVmContext is inverted: change the
condition that currently uses context.ignoreError to require the check only when
ignoreError is false (i.e. use !context.ignoreError && <vm state check>) so
rollback and normal RemoveTpmMsg paths correctly enforce VM-state validation;
also consider renaming the boolean field ignoreError to skipVmStateCheck (and
update all references) if its purpose is only to bypass VM state validation to
avoid future misuse.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java (1)
475-480:⚠️ Potential issue | 🟠 Major回滚失败时仍缺少兜底 DB 清理，可能留下孤儿记录。
当
removeTpmFromVm失败时（例如detachKeyProviderFromTpm在 key 未绑定时抛异常），流程会在到达remove-db-records步骤之前中断。当前fail()回调仅记录日志并继续，不会执行任何兜底清理，导致刚创建的TpmVO记录残留。Clone 场景下，
persist-TPM-VO只创建了 DB 记录，尚未进行 key 绑定。如果后续步骤失败触发回滚，detachKeyProviderFromTpm可能会因无绑定记录而抛异常或失败，从而跳过 DB 删除。建议在
fail()回调中添加 best-effort 的 DB 清理：🛠️ 建议的修复方案
`@Override` public void fail(ErrorCode errorCode) { logger.warn(String.format("failed to delete tpm for VM[%s] but still continue: %s", tpm.getVmInstanceUuid(), errorCode.getReadableDetails())); + // Best-effort fallback: ensure DB records are cleaned up + try { + SQL.New(TpmVO.class).eq(TpmVO_.uuid, tpm.getUuid()).delete(); + SQL.New(VmHostFileVO.class) + .eq(VmHostFileVO_.vmInstanceUuid, tpm.getVmInstanceUuid()) + .eq(VmHostFileVO_.type, VmHostFileType.TpmState) + .delete(); + } catch (Exception e) { + logger.warn(String.format("fallback DB cleanup for TPM[%s] also failed: %s", + tpm.getUuid(), e.getMessage())); + } whileCompletion.done(); }🤖 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 475 - 480, The fail() callback in removeTpmFromVm currently only logs and returns, which can leave a persisted TpmVO orphan when later steps (e.g., detachKeyProviderFromTpm) fail; update the fail() implementation to perform a best-effort DB cleanup: lookup and remove the TpmVO created for the VM (use tpm.getVmInstanceUuid() or tpm.getUuid() to find the record) inside a try/catch, handle and log any exceptions but do not rethrow, then finally call whileCompletion.done(); ensure the cleanup is idempotent so it won’t fail if the record is already removed.
🧹 Nitpick comments (2)
plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java (2)
305-309: 字段命名建议改进：ignoreError→skipVmStateCheck
ignoreError这个名字暗示会忽略所有错误，但实际上它只用于跳过 VM 状态检查（Line 323 的skipIf）。根据编码规范，建议使用更精确的命名如skipVmStateCheck，以避免后续误用。♻️ 建议重命名以提高代码自解释性
static class RemoveTpmFromVmContext { String vmInstanceUuid; String tpmUuid; - boolean ignoreError; + boolean skipVmStateCheck; List<VmHostFileVO> hostFiles;同时更新 Line 323 和 Line 468 的引用。
As per coding guidelines: "避免使用布尔型参数造成含义不明确。"
🤖 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 305 - 309, The boolean field ignoreError on class RemoveTpmFromVmContext is misleading because it only controls skipping the VM state check; rename the field to skipVmStateCheck and update all usages to match (e.g., the condition passed into skipIf and any other references such as where RemoveTpmFromVmContext is constructed or read) so the intent is explicit and conforms to the guideline about boolean parameter clarity; ensure getters/setters or direct accesses to ignoreError are replaced with skipVmStateCheck consistently.
464-464: 可考虑使用While.all()并行执行以加速回滚。当前使用
While.each()串行删除每个 TPM，各 VM 的 TPM 删除操作相互独立，可以改用While.all()并行执行以减少回滚耗时。♻️ 使用并行执行
-new While<>(reply.getInventories()).each((tpm, whileCompletion) -> { +new While<>(reply.getInventories()).all((tpm, whileCompletion) -> {🤖 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 464, The rollback currently iterates serially using new While<>(reply.getInventories()).each(...) which deletes TPMs one-by-one; change this to use While.all(reply.getInventories()) with the same per-item lambda so TPM deletions run in parallel, keeping the existing completion/exception handling (referencing reply.getInventories(), While.each -> replace with While.all, and the lambda used in KvmTpmManager) to reduce rollback time.
🤖 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 475-480: The fail() callback in removeTpmFromVm currently only
logs and returns, which can leave a persisted TpmVO orphan when later steps
(e.g., detachKeyProviderFromTpm) fail; update the fail() implementation to
perform a best-effort DB cleanup: lookup and remove the TpmVO created for the VM
(use tpm.getVmInstanceUuid() or tpm.getUuid() to find the record) inside a
try/catch, handle and log any exceptions but do not rethrow, then finally call
whileCompletion.done(); ensure the cleanup is idempotent so it won’t fail if the
record is already removed.
---
Nitpick comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java`:
- Around line 305-309: The boolean field ignoreError on class
RemoveTpmFromVmContext is misleading because it only controls skipping the VM
state check; rename the field to skipVmStateCheck and update all usages to match
(e.g., the condition passed into skipIf and any other references such as where
RemoveTpmFromVmContext is constructed or read) so the intent is explicit and
conforms to the guideline about boolean parameter clarity; ensure
getters/setters or direct accesses to ignoreError are replaced with
skipVmStateCheck consistently.
- Line 464: The rollback currently iterates serially using new
While<>(reply.getInventories()).each(...) which deletes TPMs one-by-one; change
this to use While.all(reply.getInventories()) with the same per-item lambda so
TPM deletions run in parallel, keeping the existing completion/exception
handling (referencing reply.getInventories(), While.each -> replace with
While.all, and the lambda used in KvmTpmManager) to reduce rollback time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 37055ff3-afb8-405f-8f17-fd67f69cc799
📒 Files selected for processing (1)
plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java
Change the TPM deletion logic from batch SQL delete to individual deletion using While loop. This ensures proper cleanup for each TPM by calling removeTpmFromVm method. Added error handling to continue even if deletion fails for individual TPM. Changes: - Replace batch SQL delete with While loop iteration - Call removeTpmFromVm for each TPM to ensure proper cleanup - Add error handling to continue on individual failures - Log warnings for failed TPM deletions Resolves: ZSV-11439 Related: ZSV-11310 Change-Id: I616d746d6a6f677a6772796f63676e6177676371
MatheMatrix
force-pushed
the
sync/wenhao.zhang/zsv-ldap-3
branch
from
d2feb84 to
ab338a6
Compare
2 participants
Change the TPM deletion logic from batch SQL delete to individual
deletion using While loop. This ensures proper cleanup for each TPM
by calling removeTpmFromVm method. Added error handling to continue
even if deletion fails for individual TPM.
Changes:
Resolves: ZSV-11439
Related: ZSV-11310
Change-Id: I616d746d6a6f677a6772796f63676e6177676371
sync from gitlab !9550