<refactor>[kvm]: introduce VM host file base classes for backup tracking#3615
Closed
ZStack-Robot wants to merge 2 commits into
Closed
<refactor>[kvm]: introduce VM host file base classes for backup tracking#3615ZStack-Robot wants to merge 2 commits into
ZStack-Robot wants to merge 2 commits into
Conversation
Walkthrough引入 VM 主机文件的抽象基类与工厂，新增 NV-RAM 与 TPM 状态的文件/备份实现；在备份流程中记录源对象并在持久化后调用备份对象的后置钩子以复制 TPM key-provider 名称系统标签；对部分 API 字段添加 Changes
Sequence Diagram(s)sequenceDiagram
participant Manager as KvmSecureBootManager
participant Factory as KvmVmHostFileFactory
participant BackupBase as AbstractVmHostBackupFileBase
participant TpmBackend as TpmEncryptedResourceKeyBackend
Manager->>Manager: backupVmHostFile() — 生成备份对象并记录 backupFromMap
Manager->>Manager: 持久化备份文件与内容
loop 对每个已持久化的备份文件
Manager->>Factory: createBackupBase(backup)
Factory-->>Manager: BackupBase 实例（如 TpmStateVmHostBackupFileBase）
Manager->>BackupBase: afterBackup(sourceObject)
alt 源为 VmHostBackupFileVO
BackupBase->>BackupBase: 从源系统标签读取 TPM_KEY_PROVIDER_NAME
BackupBase-->>BackupBase: 在目标上创建继承/可重建的系统标签（若存在）
else 源为 VmHostFileVO
BackupBase->>TpmBackend: findKeyProviderNameByTpm(tpmUuid)
TpmBackend-->>BackupBase: 返回 providerName 或 null
BackupBase-->>BackupBase: 在目标上创建继承/可重建的系统标签（若有）
end
end
Estimated code review effort🎯 3 (中等) | ⏱️ ~25 分钟 诗歌
🚥 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
🧹 Nitpick comments (2)
plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootManager.java (1)
579-588:afterBackup钩子在事务外执行可能导致数据不一致。
afterBackup在SQLBatch事务完成后执行。若TpmStateVmHostBackupFileBase.afterBackup中创建TPM_KEY_PROVIDER_NAME系统标签失败，备份记录已持久化但缺少密钥提供者信息，可能影响快照恢复流程。基于 learnings 中提到的 ZStack 弹性设计原则（单个文件同步失败应被容忍），建议至少添加 try-catch 和日志记录，避免异常传播中断整个流程。
♻️ 建议添加异常处理
for (VmHostBackupFileVO backup : filesNeedPersists) { final Object backupFrom = backupFromMap.get(backup); - if (backupFrom instanceof VmHostBackupFileVO) { - vmHostFileFactory.createBackupBase(backup).afterBackup((VmHostBackupFileVO) backupFrom); - } else if (backupFrom instanceof VmHostFileVO) { - vmHostFileFactory.createBackupBase(backup).afterBackup((VmHostFileVO) backupFrom); + try { + if (backupFrom instanceof VmHostBackupFileVO) { + vmHostFileFactory.createBackupBase(backup).afterBackup((VmHostBackupFileVO) backupFrom); + } else if (backupFrom instanceof VmHostFileVO) { + vmHostFileFactory.createBackupBase(backup).afterBackup((VmHostFileVO) backupFrom); + } + } catch (Exception e) { + logger.warn(String.format("failed to execute afterBackup hook for VmHostBackupFileVO[uuid:%s]: %s", + backup.getUuid(), e.getMessage()), e); } }Based on learnings: "In ZStack's KvmSecureBootExtensions.syncVmHostFilesFromHost method... individual host file sync failures should be tolerated and must not cause the entire completion to fail."
🤖 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 579 - 588, The loop invoking vmHostFileFactory.createBackupBase(...).afterBackup(...) (in the filesNeedPersists processing) can throw from TpmStateVmHostBackupFileBase.afterBackup (e.g., when creating TPM_KEY_PROVIDER_NAME system tag) and currently runs outside the SQLBatch transaction; wrap each call in a per-item try-catch that logs the error (including backup id/context and exception) and swallows it so a single failing afterBackup does not abort the whole sync; ensure you reference vmHostFileFactory.createBackupBase, the afterBackup overloads (afterBackup(VmHostBackupFileVO) and afterBackup(VmHostFileVO)), and include clear log messages indicating which backup failed.plugin/kvm/src/main/java/org/zstack/kvm/tpm/TpmStateVmHostBackupFileBase.java (1)
20-25: 建议将字段声明移至构造函数之前。按照 Java 惯例和可读性，成员字段应声明在构造函数之前。当前
resourceKeyBackend字段（第 27-28 行）放置在构造函数（第 23-25 行）之后。♻️ 建议的字段顺序调整
public class TpmStateVmHostBackupFileBase extends AbstractVmHostBackupFileBase { private static final CLogger logger = Utils.getLogger(TpmStateVmHostBackupFileBase.class); + `@Autowired` + private TpmEncryptedResourceKeyBackend resourceKeyBackend; + public TpmStateVmHostBackupFileBase(VmHostBackupFileVO self) { super(self); } - - `@Autowired` - private TpmEncryptedResourceKeyBackend resourceKeyBackend;Also applies to: 27-28
🤖 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 20 - 25, In TpmStateVmHostBackupFileBase, move the member field declaration 'resourceKeyBackend' so it is declared before the constructor 'public TpmStateVmHostBackupFileBase(VmHostBackupFileVO self)', following Java conventions; ensure its visibility and any initialization remain unchanged when you relocate the declaration above the constructor in the class body.
🤖 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/AbstractVmHostBackupFileBase.java`:
- Around line 9-23: The AbstractVmHostBackupFileBase class is missing the
`@Configurable` annotation so Spring won't inject `@Autowired` fields in subclasses
(e.g., TpmStateVmHostBackupFileBase.resourceKeyBackend) when instances are
created with new in KvmVmHostFileFactory.createBackupBase(); add the
`@Configurable` annotation to AbstractVmHostBackupFileBase (and import the
appropriate org.springframework.beans.factory.annotation.Configurable) so Spring
performs dependency injection for its subclasses and prevents resourceKeyBackend
from being null when afterBackup(VmHostFileVO) calls
resourceKeyBackend.findKeyProviderNameByTpm().
---
Nitpick comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootManager.java`:
- Around line 579-588: The loop invoking
vmHostFileFactory.createBackupBase(...).afterBackup(...) (in the
filesNeedPersists processing) can throw from
TpmStateVmHostBackupFileBase.afterBackup (e.g., when creating
TPM_KEY_PROVIDER_NAME system tag) and currently runs outside the SQLBatch
transaction; wrap each call in a per-item try-catch that logs the error
(including backup id/context and exception) and swallows it so a single failing
afterBackup does not abort the whole sync; ensure you reference
vmHostFileFactory.createBackupBase, the afterBackup overloads
(afterBackup(VmHostBackupFileVO) and afterBackup(VmHostFileVO)), and include
clear log messages indicating which backup failed.
In
`@plugin/kvm/src/main/java/org/zstack/kvm/tpm/TpmStateVmHostBackupFileBase.java`:
- Around line 20-25: In TpmStateVmHostBackupFileBase, move the member field
declaration 'resourceKeyBackend' so it is declared before the constructor
'public TpmStateVmHostBackupFileBase(VmHostBackupFileVO self)', following Java
conventions; ensure its visibility and any initialization remain unchanged when
you relocate the declaration above the constructor in the class body.
🪄 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: 39d0b9fb-6514-4e33-aea5-debe07691946
⛔ Files ignored due to path filters (2)
conf/springConfigXml/Kvm.xmlis excluded by!**/*.xmltest/src/test/resources/springConfigXml/Kvm.xmlis excluded by!**/*.xml
📒 Files selected for processing (9)
plugin/kvm/src/main/java/org/zstack/kvm/KVMSystemTags.javaplugin/kvm/src/main/java/org/zstack/kvm/efi/AbstractVmHostBackupFileBase.javaplugin/kvm/src/main/java/org/zstack/kvm/efi/AbstractVmHostFileBase.javaplugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootManager.javaplugin/kvm/src/main/java/org/zstack/kvm/efi/KvmVmHostFileFactory.javaplugin/kvm/src/main/java/org/zstack/kvm/efi/NvRamVmHostBackupFileBase.javaplugin/kvm/src/main/java/org/zstack/kvm/efi/NvRamVmHostFileBase.javaplugin/kvm/src/main/java/org/zstack/kvm/tpm/TpmStateVmHostBackupFileBase.javaplugin/kvm/src/main/java/org/zstack/kvm/tpm/TpmStateVmHostFileBase.java
MatheMatrix
force-pushed
the
sync/wenhao.zhang/zsv-ldap-2
branch
from
7c37a4c to
f0e4fa5
Compare
Collaborator
|
Comment from yaohua.wu: Review: MR !9476 — ZSV-11441MR: 总体评价架构设计合理：引入 Factory + Abstract Base + afterBackup hook 模式为 VM host file 的备份流程提供了良好的扩展点。 以下是 beyond-the-diff 全文 review 后的发现： Warning1. [KvmSecureBootManager.java:579-588]
问题影响：
建议修复：在循环内为每个 backup 文件的 for (VmHostBackupFileVO backup : filesNeedPersists) {
final Object backupFrom = backupFromMap.get(backup);
try {
if (backupFrom instanceof VmHostBackupFileVO) {
vmHostFileFactory.createBackupBase(backup).afterBackup((VmHostBackupFileVO) backupFrom);
} else if (backupFrom instanceof VmHostFileVO) {
vmHostFileFactory.createBackupBase(backup).afterBackup((VmHostFileVO) backupFrom);
}
} catch (Exception e) {
logger.warn(String.format("failed to execute afterBackup hook for VmHostBackupFileVO[uuid:%s, type:%s]: %s",
backup.getUuid(), backup.getType(), e.getMessage()), e);
}
}符合 ZStack 弹性设计原则：单个文件的 tag 同步失败不应中断整个备份流程。 2. [KvmVmHostFileFactory.java:11]
Suggestion1. [KvmSecureBootManager.java:517]
// 或简单地用两个 Map 分别记录
Map<VmHostBackupFileVO, VmHostBackupFileVO> backupFromBackup = new HashMap<>();
Map<VmHostBackupFileVO, VmHostFileVO> backupFromFile = new HashMap<>();2. [TpmStateVmHostBackupFileBase.java:27-28] 字段声明位置
Verdict: APPROVED变更架构清晰，扩展性好。Warning #1（afterBackup 异常保护）建议在合并前修复，防止生产环境因 tag 创建异常导致备份流程不必要的失败。其余为非阻塞建议。 🤖 Robot Reviewer |
added 2 commits
March 27, 2026 12:50
* Create abstract base classes for VM host file and backup file to provide extensible structure for different file types. * Add factory class KvmVmHostFileFactory to create appropriate base instances based on file type. * Implement base classes for NvRam and TpmState files. * Integrate factory into KvmSecureBootManager to call afterBackup hooks for tracking TPM key provider information. * Add TPM_KEY_PROVIDER_NAME system tag to record key provider association with backup files for snapshot restore scenarios. Resolves: ZSV-11441 Related: ZSV-11310 Change-Id: I7a7061647a6f6d636871707261656a6a6875626c
Add @APINoSee annotation to backupFileUuid field in TpmSpec and NvRamSpec classes to hide these internal fields from API responses. These fields are used internally for TPM and NvRam state restore and should not be exposed to API clients. Related: ZSV-11310 Change-Id: I71666a6677657376727a6f7a736171736c67676e
MatheMatrix
force-pushed
the
sync/wenhao.zhang/zsv-ldap-2
branch
from
1040948 to
2e3193e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootManager.java (1)
517-518: 建议将backupFromMap改为强类型结构，减少运行时分支。当前
Map<VmHostBackupFileVO, Object>+instanceof可工作，但类型约束弱，后续扩展文件类型时更容易引入分支遗漏。建议拆成两个强类型 map（或等价的强类型封装）并把createBackupBase提前为单次调用。♻️ 可选改造示例
-// value is VmHostBackupFileVO or VmHostFileVO -Map<VmHostBackupFileVO, Object> backupFromMap = new HashMap<>(); +Map<VmHostBackupFileVO, VmHostFileVO> backupFromHostFileMap = new HashMap<>(); +Map<VmHostBackupFileVO, VmHostBackupFileVO> backupFromBackupFileMap = new HashMap<>(); @@ -backupFromMap.put(file, vmHostFile == null ? vmHostBackupFile : vmHostFile); +if (vmHostFile != null) { + backupFromHostFileMap.put(file, vmHostFile); +} else { + backupFromBackupFileMap.put(file, vmHostBackupFile); +} @@ -for (VmHostBackupFileVO backup : filesNeedPersists) { - final Object backupFrom = backupFromMap.get(backup); +for (VmHostBackupFileVO backup : filesNeedPersists) { + VmHostFileVO hostFrom = backupFromHostFileMap.get(backup); + VmHostBackupFileVO backupFrom = backupFromBackupFileMap.get(backup); + AbstractVmHostBackupFileBase backupBase = vmHostFileFactory.createBackupBase(backup); try { - if (backupFrom instanceof VmHostBackupFileVO) { - vmHostFileFactory.createBackupBase(backup).afterBackup((VmHostBackupFileVO) backupFrom); - } else if (backupFrom instanceof VmHostFileVO) { - vmHostFileFactory.createBackupBase(backup).afterBackup((VmHostFileVO) backupFrom); + if (backupFrom != null) { + backupBase.afterBackup(backupFrom); + } else if (hostFrom != null) { + backupBase.afterBackup(hostFrom); }Also applies to: 543-543, 580-587
🤖 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 517 - 518, The Map<VmHostBackupFileVO, Object> backupFromMap in KvmSecureBootManager is weakly typed and forces instanceof branching; replace it with two strongly-typed collections (e.g. Map<VmHostBackupFileVO, BackupMeta> and Map<VmHostFileVO, BackupMeta> or a small sealed wrapper class) so each branch handles its concrete type without instanceof checks, and move the call to createBackupBase so it’s invoked once per file entry before branching (make createBackupBase return the shared BackupBase/metadata used by both maps); update all usages around backupFromMap (including code interacting with VmHostBackupFileVO and VmHostFileVO at the sites noted) to consume the appropriate strong-type map or wrapper.
🤖 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 517-518: The Map<VmHostBackupFileVO, Object> backupFromMap in
KvmSecureBootManager is weakly typed and forces instanceof branching; replace it
with two strongly-typed collections (e.g. Map<VmHostBackupFileVO, BackupMeta>
and Map<VmHostFileVO, BackupMeta> or a small sealed wrapper class) so each
branch handles its concrete type without instanceof checks, and move the call to
createBackupBase so it’s invoked once per file entry before branching (make
createBackupBase return the shared BackupBase/metadata used by both maps);
update all usages around backupFromMap (including code interacting with
VmHostBackupFileVO and VmHostFileVO at the sites noted) to consume the
appropriate strong-type map or wrapper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 36ab4523-4b80-4c00-be87-7b57c6f8a6de
⛔ Files ignored due to path filters (2)
conf/springConfigXml/Kvm.xmlis excluded by!**/*.xmltest/src/test/resources/springConfigXml/Kvm.xmlis excluded by!**/*.xml
📒 Files selected for processing (11)
header/src/main/java/org/zstack/header/tpm/entity/TpmSpec.javaheader/src/main/java/org/zstack/header/vm/devices/NvRamSpec.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMSystemTags.javaplugin/kvm/src/main/java/org/zstack/kvm/efi/AbstractVmHostBackupFileBase.javaplugin/kvm/src/main/java/org/zstack/kvm/efi/AbstractVmHostFileBase.javaplugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootManager.javaplugin/kvm/src/main/java/org/zstack/kvm/efi/KvmVmHostFileFactory.javaplugin/kvm/src/main/java/org/zstack/kvm/efi/NvRamVmHostBackupFileBase.javaplugin/kvm/src/main/java/org/zstack/kvm/efi/NvRamVmHostFileBase.javaplugin/kvm/src/main/java/org/zstack/kvm/tpm/TpmStateVmHostBackupFileBase.javaplugin/kvm/src/main/java/org/zstack/kvm/tpm/TpmStateVmHostFileBase.java
✅ Files skipped from review due to trivial changes (6)
- header/src/main/java/org/zstack/header/tpm/entity/TpmSpec.java
- plugin/kvm/src/main/java/org/zstack/kvm/efi/NvRamVmHostFileBase.java
- plugin/kvm/src/main/java/org/zstack/kvm/efi/NvRamVmHostBackupFileBase.java
- plugin/kvm/src/main/java/org/zstack/kvm/efi/AbstractVmHostBackupFileBase.java
- plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmVmHostFileFactory.java
- plugin/kvm/src/main/java/org/zstack/kvm/efi/AbstractVmHostFileBase.java
🚧 Files skipped from review as they are similar to previous changes (3)
- header/src/main/java/org/zstack/header/vm/devices/NvRamSpec.java
- plugin/kvm/src/main/java/org/zstack/kvm/tpm/TpmStateVmHostFileBase.java
- plugin/kvm/src/main/java/org/zstack/kvm/KVMSystemTags.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.
provide extensible structure for different file types.
instances based on file type.
afterBackup hooks for tracking TPM key provider information.
association with backup files for snapshot restore scenarios.
Resolves: ZSV-11441
Related: ZSV-11310
Change-Id: I7a7061647a6f6d636871707261656a6a6875626c
sync from gitlab !9476