<fix>[kvm]: mark TPM VM host files changed on start/shutdown#3726
Open
MatheMatrix wants to merge 1 commit intofeature-zsv-5.0.0-vm-support-vtpm-and-secucebootfrom
Open
<fix>[kvm]: mark TPM VM host files changed on start/shutdown#3726MatheMatrix wants to merge 1 commit intofeature-zsv-5.0.0-vm-support-vtpm-and-secucebootfrom
MatheMatrix wants to merge 1 commit intofeature-zsv-5.0.0-vm-support-vtpm-and-secucebootfrom
Conversation
Walkthrough在 KVM 安全启动模块中：KvmSecureBootManager 注入 ResourceDestinationMaker 并在 libvirt 启动/关机事件用 isManagedByUs 门控；受管 VM 事件会提前更新对应主机文件的 changeDate（通过工厂决定的类型集合）；工厂新增按 VM 决定需注册的主机文件类型查询方法，扩展改为使用该工厂判定 NvRam/TpmState 处理逻辑。 Changes
Sequence Diagram(s)sequenceDiagram
participant Libvirt as "Libvirt 事件"
participant Manager as "KvmSecureBootManager"
participant Factory as "KvmVmHostFileFactory"
participant DB as "数据库 (VmHostFileVO)"
Libvirt->>Manager: VM_LIBVIRT_REPORT_START/SHUTDOWN(vmUuid)
Manager->>Manager: resourceDestinationMaker.isManagedByUs(vmUuid)?
alt Managed
Manager->>Factory: vmHostFileTypeNeedRegisterForVm(vmUuid)
Factory-->>Manager: Set<VmHostFileType> (e.g., NvRam, TpmState)
Manager->>DB: UPDATE VmHostFileVO.changeDate WHERE vmInstanceUuid=vmUuid AND hostUuid=resolvedHostUuid AND type IN (...)
DB-->>Manager: 更新结果
Manager-->>Libvirt: 继续后续事件处理（如需）
else Not managed
Manager-->>Libvirt: 早退/忽略事件
end
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)
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/efi/KvmSecureBootManager.java (1)
197-238: 可选优化：提取 hostUuid 获取逻辑以避免重复代码和重复查询。
hostUuid的获取逻辑（优先取hostUuid，若为空则取lastHostUuid）在以下位置重复出现：
markVmHostFilesChanged(lines 210-224)- shutdown 事件处理 (lines 146-157)
handle(RestoreVmHostFileMsg)(lines 816-834)此外，在 shutdown 事件路径中，
markVmHostFilesChanged和后续逻辑会对VmInstanceVO进行两次相同的查询，可以考虑传递查询结果以减少 DB 访问。♻️ 建议提取辅助方法
/** * Get the effective host UUID for a VM (hostUuid or fallback to lastHostUuid). * `@return` host UUID or null if VM not found or no host */ private String getEffectiveHostUuid(String vmUuid) { Tuple tuple = Q.New(VmInstanceVO.class) .select(VmInstanceVO_.hostUuid, VmInstanceVO_.lastHostUuid) .eq(VmInstanceVO_.uuid, vmUuid) .findTuple(); if (tuple == null) { return null; } String hostUuid = (String) tuple.get(0); return hostUuid != null ? hostUuid : (String) tuple.get(1); }然后在 shutdown 事件处理中可以复用结果：
markVmHostFilesChanged(vmUuid); String hostUuid = getEffectiveHostUuid(vmUuid); if (hostUuid == null) { return; } // ... rest of logic using hostUuid🤖 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/efi/KvmSecureBootManager.java` around lines 197 - 238, The hostUuid selection logic (prefer hostUuid, fallback to lastHostUuid) is duplicated in markVmHostFilesChanged, the shutdown handler, and handle(RestoreVmHostFileMsg); extract it into a private helper (e.g., getEffectiveHostUuid(String vmUuid)) that returns the effective host UUID or null, then replace the duplicated Tuple queries in markVmHostFilesChanged, the shutdown event path, and handle(RestoreVmHostFileMsg) to call getEffectiveHostUuid; additionally, where shutdown currently calls markVmHostFilesChanged and then re-queries VmInstanceVO, refactor to call getEffectiveHostUuid once and pass the result into markVmHostFilesChanged (or provide an overload like markVmHostFilesChanged(String vmUuid, String hostUuid)) to eliminate the double DB access.
🤖 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/efi/KvmSecureBootManager.java`:
- Around line 197-238: The hostUuid selection logic (prefer hostUuid, fallback
to lastHostUuid) is duplicated in markVmHostFilesChanged, the shutdown handler,
and handle(RestoreVmHostFileMsg); extract it into a private helper (e.g.,
getEffectiveHostUuid(String vmUuid)) that returns the effective host UUID or
null, then replace the duplicated Tuple queries in markVmHostFilesChanged, the
shutdown event path, and handle(RestoreVmHostFileMsg) to call
getEffectiveHostUuid; additionally, where shutdown currently calls
markVmHostFilesChanged and then re-queries VmInstanceVO, refactor to call
getEffectiveHostUuid once and pass the result into markVmHostFilesChanged (or
provide an overload like markVmHostFilesChanged(String vmUuid, String hostUuid))
to eliminate the double DB access.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ed16abfb-1ca2-46ac-95aa-f0d9d855a8a4
📒 Files selected for processing (1)
plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootManager.java
MatheMatrix
force-pushed
the
sync/wenhao.zhang/zsv-ldap
branch
from
3f42ee8 to
0a5630d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootManager.java (1)
208-222: 建议抽取 hostUuid 解析逻辑以减少重复。
hostUuid -> lastHostUuid的回退解析在 shutdown 回调和markVmHostFilesChanged()中重复出现，建议提取成私有 helper，减少后续改动时的分支漂移风险。🤖 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/efi/KvmSecureBootManager.java` around lines 208 - 222, Extract the duplicated "hostUuid -> lastHostUuid" fallback logic into a private helper in KvmSecureBootManager (e.g., resolveHostUuid(String vmUuid) or getEffectiveHostUuid) that performs the Q.New(VmInstanceVO.class) select for hostUuid and lastHostUuid, applies the fallback and returns null if none; then replace the duplicated blocks in the shutdown callback and markVmHostFilesChanged() with calls to this helper to centralize the resolution logic.
🤖 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/efi/KvmSecureBootManager.java`:
- Around line 233-234: 将 debug 日志中的“TPM vm”表述改为中性且准确的描述以免误导排障；在
KvmSecureBootManager 中找到使用 logger.debug 的那一行（当前格式化字符串包含 vmUuid, hostUuid,
updated）并将消息改为例如“preemptively marked VmHostFiles as changed for
secure-boot-related vm[uuid:%s] on host[uuid:%s], %d records updated”
或其它等效中性表述，确保仍包含 vmUuid、hostUuid 和 updated 三个占位符。
In `@plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmVmHostFileFactory.java`:
- Around line 53-55: The ternary in KvmVmHostFileFactory using
resourceConfig.getResourceConfigValue(vmUuid, Boolean.class) can NPE when the
Boolean is null; change the condition to compare with Boolean.TRUE (e.g.,
Boolean.TRUE.equals(resourceConfig.getResourceConfigValue(...)) or ==
Boolean.TRUE) to avoid auto-unboxing NPE and match the style used in
KvmSecureBootExtensions and VmTpmExtensions, keeping the returned new
HashSet<>(list(NvRam)) or Collections.emptySet() behavior unchanged.
---
Nitpick comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootManager.java`:
- Around line 208-222: Extract the duplicated "hostUuid -> lastHostUuid"
fallback logic into a private helper in KvmSecureBootManager (e.g.,
resolveHostUuid(String vmUuid) or getEffectiveHostUuid) that performs the
Q.New(VmInstanceVO.class) select for hostUuid and lastHostUuid, applies the
fallback and returns null if none; then replace the duplicated blocks in the
shutdown callback and markVmHostFilesChanged() with calls to this helper to
centralize the resolution logic.
🪄 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: 7ab4cfb2-44df-49e3-964e-75fb7a2aea7d
📒 Files selected for processing (2)
plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootManager.javaplugin/kvm/src/main/java/org/zstack/kvm/efi/KvmVmHostFileFactory.java
plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootManager.java
Outdated
Show resolved
Hide resolved
MatheMatrix
force-pushed
the
sync/wenhao.zhang/zsv-ldap
branch
from
0a5630d to
de81066
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootManager.java (1)
201-229: 建议按已注册的VmHostFileVO置脏，避免兜底范围和直读范围漂移。这里用
vmHostFileFactory.vmHostFileTypeNeedRegisterForVm(vmUuid)重新推导类型；但下面 shutdown 的即时同步是按 DB 里实际存在的VmHostFileVO行来读的。为了让“即时同步失败后的周期兜底”覆盖完全一致，建议这里直接基于vmUuid + hostUuid下已注册的文件行批量更新changeDate，而不是再算一遍期望类型集。♻️ 可参考的调整方向
- final Set<VmHostFileType> types = vmHostFileFactory.vmHostFileTypeNeedRegisterForVm(vmUuid); - if (types.isEmpty()) { - return; - } - Tuple tuple = Q.New(VmInstanceVO.class) .select(VmInstanceVO_.hostUuid, VmInstanceVO_.lastHostUuid) .eq(VmInstanceVO_.uuid, vmUuid) .findTuple(); @@ if (hostUuid == null) { return; } + List<VmHostFileType> types = Q.New(VmHostFileVO.class) + .eq(VmHostFileVO_.vmInstanceUuid, vmUuid) + .eq(VmHostFileVO_.hostUuid, hostUuid) + .select(VmHostFileVO_.type) + .listValues(); + if (types.isEmpty()) { + return; + } + Timestamp now = new Timestamp(timeHelper.getCurrentTimeMillis()); long updated = SQL.New(VmHostFileVO.class) .eq(VmHostFileVO_.vmInstanceUuid, vmUuid) .eq(VmHostFileVO_.hostUuid, hostUuid) .in(VmHostFileVO_.type, types)🤖 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/efi/KvmSecureBootManager.java` around lines 201 - 229, The current markVmHostFilesChanged method computes types via vmHostFileFactory.vmHostFileTypeNeedRegisterForVm(vmUuid) but then updates by that derived set, which can diverge from the actual registered VmHostFileVO rows read by the shutdown sync; change it to query the DB for existing VmHostFileVO entries for the resolved hostUuid and vmUuid and update those rows' VmHostFileVO_.changeDate directly (use SQL.New(VmHostFileVO.class) filtered by VmHostFileVO_.vmInstanceUuid and VmHostFileVO_.hostUuid to target actual rows) instead of using the types variable derived from the factory so both immediate sync and periodic fallback operate on the same set of DB rows.plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootExtensions.java (1)
339-346: 建议把VmHostFileType到消息字段的分派收敛到一个 helper。这几处现在都在手写
NvRam/TpmState -> setNvRamPath/setTpmStateFolder的分支。当前 PR 已经改到 6 处，后面如果再加类型或调整路径语义，很容易漏改，而且现在不同路径里对 unsupported type 的处理也不完全一致。♻️ 可参考的收敛方式
+ private void applyHostFilePath(SyncVmHostFilesFromHostMsg syncMsg, VmHostFileType type, String path) { + if (type == NvRam) { + syncMsg.setNvRamPath(path); + return; + } + if (type == VmHostFileType.TpmState) { + syncMsg.setTpmStateFolder(path); + return; + } + throw new CloudRuntimeException("unsupported vm host file type: " + type); + }然后把这些分支统一替换成：
- if (file.getType() == NvRam) { - syncMsg.setNvRamPath(file.getPath()); - } else if (file.getType() == VmHostFileType.TpmState) { - syncMsg.setTpmStateFolder(file.getPath()); - } else { - logger.warn(...); - } + applyHostFilePath(syncMsg, file.getType(), file.getPath());As per coding guidelines，“单一职责原则：每个模块、类、方法应只承担单一的责任。若某个逻辑过于复杂，应考虑拆分出独立类来处理。”
Also applies to: 461-465, 563-572, 622-632, 688-697, 748-754
🤖 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/efi/KvmSecureBootExtensions.java` around lines 339 - 346, Centralize the VmHostFileType -> message-field dispatch into a single helper to avoid duplicated branching: create a private helper (e.g., populateSyncMsgForVmHostFile or mapVmHostFileToSync) that takes the VmHostFile (or its type/path) and the sync message object and performs the NvRam/TpmState switch, calling setNvRamPath or setTpmStateFolder (and throwing a single consistent CloudRuntimeException for unsupported types); then replace the inline branches in KvmSecureBootExtensions (locations around vmHostFile handling including the existing blocks at the shown snippet and the other occurrences ~461-465, ~563-572, ~622-632, ~688-697, ~748-754) to call that helper so all dispatch logic is maintained in one place.
🤖 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/efi/KvmSecureBootExtensions.java`:
- Around line 339-346: Centralize the VmHostFileType -> message-field dispatch
into a single helper to avoid duplicated branching: create a private helper
(e.g., populateSyncMsgForVmHostFile or mapVmHostFileToSync) that takes the
VmHostFile (or its type/path) and the sync message object and performs the
NvRam/TpmState switch, calling setNvRamPath or setTpmStateFolder (and throwing a
single consistent CloudRuntimeException for unsupported types); then replace the
inline branches in KvmSecureBootExtensions (locations around vmHostFile handling
including the existing blocks at the shown snippet and the other occurrences
~461-465, ~563-572, ~622-632, ~688-697, ~748-754) to call that helper so all
dispatch logic is maintained in one place.
In `@plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootManager.java`:
- Around line 201-229: The current markVmHostFilesChanged method computes types
via vmHostFileFactory.vmHostFileTypeNeedRegisterForVm(vmUuid) but then updates
by that derived set, which can diverge from the actual registered VmHostFileVO
rows read by the shutdown sync; change it to query the DB for existing
VmHostFileVO entries for the resolved hostUuid and vmUuid and update those rows'
VmHostFileVO_.changeDate directly (use SQL.New(VmHostFileVO.class) filtered by
VmHostFileVO_.vmInstanceUuid and VmHostFileVO_.hostUuid to target actual rows)
instead of using the types variable derived from the factory so both immediate
sync and periodic fallback operate on the same set of DB rows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 78119598-43fb-468d-a005-d3d5d6fbabf3
📒 Files selected for processing (3)
plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootExtensions.javaplugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootManager.javaplugin/kvm/src/main/java/org/zstack/kvm/efi/KvmVmHostFileFactory.java
🚧 Files skipped from review as they are similar to previous changes (1)
- plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmVmHostFileFactory.java
Collaborator
|
Comment from ye.zou: Code Review
结论: CONDITIONAL 统计: 🔴 0 | 🟡 2 | 🔵 1 回归风险: 低-中 总体评价：
|
When a VM with TPM starts or shuts down, NvRam/TpmState data must have changed. Preemptively set changeDate on the corresponding VmHostFileVO so the periodic tracker knows to sync them, even if the direct sync fails. Also add ResourceDestinationMaker check to all VM canonical event handlers in KvmSecureBootManager to ensure only the owning management node processes each event. Resolves: ZSV-11779 Related: ZSV-11310 Change-Id: I6e6d6c6c647175716669756b75756a6f72657277
MatheMatrix
force-pushed
the
sync/wenhao.zhang/zsv-ldap
branch
from
de81066 to
9ea77b9
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 (1)
plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootManager.java (1)
150-154:⚠️ Potential issue | 🟡 Minor缺少 hostUuid 的空值检查。
shutdown 事件处理器在调用
markVmHostFilesChanged(vmUuid, hostUuid)前没有检查hostUuid是否为 null。如果VmInstanceVO.hostUuid和lastHostUuid都为 null，会传递 null 给方法。相比之下，
markVmHostFilesChanged(String vmUuid)重载版本（第 212-214 行）正确地做了 null 检查。建议保持一致性。🛡️ 建议添加空值检查
String hostUuid = (String) tuple.get(0); if (hostUuid == null) { hostUuid = (String) tuple.get(1); } +if (hostUuid == null) { + return; +} markVmHostFilesChanged(vmUuid, hostUuid);🤖 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/efi/KvmSecureBootManager.java` around lines 150 - 154, The shutdown handler extracts hostUuid from tuple but doesn't null-check it before calling markVmHostFilesChanged(vmUuid, hostUuid); add a null check on the extracted hostUuid (the value from tuple.get(0)/tuple.get(1)) and, if it is null, call the existing overload markVmHostFilesChanged(vmUuid) instead of passing null, otherwise call markVmHostFilesChanged(vmUuid, hostUuid); this keeps behavior consistent with the other overload that already performs the null check.
🤖 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/efi/KvmSecureBootManager.java`:
- Around line 150-154: The shutdown handler extracts hostUuid from tuple but
doesn't null-check it before calling markVmHostFilesChanged(vmUuid, hostUuid);
add a null check on the extracted hostUuid (the value from
tuple.get(0)/tuple.get(1)) and, if it is null, call the existing overload
markVmHostFilesChanged(vmUuid) instead of passing null, otherwise call
markVmHostFilesChanged(vmUuid, hostUuid); this keeps behavior consistent with
the other overload that already performs the null check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fbd4eff4-b5de-4ccd-935b-48ddc80cfb1b
📒 Files selected for processing (3)
plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootExtensions.javaplugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootManager.javaplugin/kvm/src/main/java/org/zstack/kvm/efi/KvmVmHostFileFactory.java
🚧 Files skipped from review as they are similar to previous changes (1)
- plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmVmHostFileFactory.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.
When a VM with TPM starts or shuts down, NvRam/TpmState
data must have changed. Preemptively set changeDate on the
corresponding VmHostFileVO so the periodic tracker knows
to sync them, even if the direct sync fails.
Also add ResourceDestinationMaker check to all VM canonical
event handlers in KvmSecureBootManager to ensure only the
owning management node processes each event.
Resolves: ZSV-11779
Related: ZSV-11310
Change-Id: I6e6d6c6c647175716669756b75756a6f72657277
sync from gitlab !9595