<feature>[kvm]: add lastSyncReason to track VM host file sync#3753
Open
ZStack-Robot wants to merge 216 commits intozsv_5.0.0from
Open
<feature>[kvm]: add lastSyncReason to track VM host file sync#3753ZStack-Robot wants to merge 216 commits intozsv_5.0.0from
ZStack-Robot wants to merge 216 commits intozsv_5.0.0from
Conversation
Related: ZSV-10444 Change-Id: I7a6864656668797a6c6a62786a7467626b73797a
VM edk tag use to save the EDK version the VM used Resolves: ZSV-11010 Change-Id: I797770686172776472746b79767876697062747a
…and-secuceboot' <fix>[kvm]: add VM edk tags See merge request zstackio/zstack!9019
V5.0.0__schema will move to conf/db/zsv, and ref files will move to conf/db/zsv_ref Related: ZSV-11310 Change-Id: I6d756c7a65746f646575726a61686c6d6f6a6a62
* add TPM related APIs / entities / configs * SysErrors add error of operation not supported Resolves: ZSV-11310 Resolves: ZSPHER-14 Change-Id: I746861687076626e70627a777469696e67786b66
…and-secuceboot' <fix>[conf]: add V5.0.0__schema.sql See merge request zstackio/zstack!9117
Resolves: ZSV-11310 Change-Id: I6b746e6a666d7267626d647777677a77716a6261
Resolves: ZSV-11310 Resolves: ZSPHER-1 Change-Id: I6d787a7375636c77656176616478717872676478
…and-secuceboot' <feature>[compute]: support create VM with TPM See merge request zstackio/zstack!9121
This patch is for zsv_5.0.0 Related: ZSV-11338 Change-Id: I666c7763796f7070796762767669786e6e75736b
DBImpact Resolves: ZSV-11331 Change-Id: I786f686371626e6674636772676c68747768716a
Resolves: ZSV-11340 Related: ZSV-11010 Change-Id: I63646f6264697877707967746f69756e776f6c71
…nd-secuceboot' <feature>[kms]: support kms See merge request zstackio/zstack!9128
…uceboot' <chore>[storage]: improve error message in VolumeSnapshotApiInterceptor See merge request zstackio/zstack!9148
…ecuceboot' <fix>[kvm]: update KVMGlobalConfig default value to "None" See merge request zstackio/zstack!9150
Resolves: ZSV-11340 Related: ZSV-11010 Change-Id: I63646f6264697877707967746f69756e776f6c73
…-secuceboot' <fix>[kvm]: continue to update KVMGlobalConfig default value to "None" See merge request zstackio/zstack!9157
In previous versions, the global config "enable.uefi.secure.boot" had no practical effect, as its default value was "true". In the current version, since it affects numerous configurations, its default value has been changed to "false" to maintain consistency with VM startup settings from previous versions and minimize the risk of VM configuration changes. Related: ZSV-11310 Change-Id: I616c796e6e796a726b6f7a77656975717a65726c
If TPM or secure boot is enabled, MN will prepare NvRam volume and instantiate volume to host. Add delete extension to delete NvRam volume after related VM deleted. Resolves: ZSV-11310 Related: ZSPHER-1 Change-Id: I787672786e6873696c6273647778737364767563
Libvirt 8.0.0 only support raw type NVRAW (ext is '.fd'). Resolves: ZSV-11310 Related: ZSPHER-1 Change-Id: I6c65797078616c706d6c716c796b63737a716375
…d-secuceboot' <fix>[compute]: introduce NVRAM type volume See merge request zstackio/zstack!9176
Related: ZSV-11310 Change-Id: I666370616e627a616378676f796971657662726f
…-secuceboot' <fix>[localstorage]: remame CreateEmptyVolumeCmd.format to volumeFormat See merge request zstackio/zstack!9238
Resolves: ZSV-11331 Change-Id: I63646d7974756278777565696276797066796f68
…nd-secuceboot' <feature>[kms]: support kms trust API See merge request zstackio/zstack!9245
* introduces a management infrastructure for VM-host file (ex: NvRam and TPM state), encompassing a data model KVM agent communication protocol, persistent storage and integration with Secure Boot and TPM extensions. * New global configuration properties have been adde to control the enablement behavior of NvRam volumes. Resolves: ZSV-11310 Change-Id: I716c766875616b79736c69666d7863767276706c
…d-secuceboot' <feature>[header]: introduce VmHostFileVO See merge request zstackio/zstack!9262
Related: ZSV-11310 Change-Id: I646d6f65756f67686c7a766b7361796768677163
…ecuceboot' <fix>[conf]: update VmHostFileContentVO.content default value See merge request zstackio/zstack!9273
* Removes database tables and corresponding entity classes related to TPM host references * Introducing a more generic VM-host inventory class as a replacement This is patch for feature "vTPM and Secure Boot" Resolves: ZSV-11310 Change-Id: I7a686a77686f63637974786a74656a776472686e
…-secuceboot' <feature>[kvm]: support TPM revert without KMS See merge request zstackio/zstack!9558
…d-secuceboot' <feature>[kvm]: periodic check and sync for VM host files See merge request zstackio/zstack!9555
Resolves: ZSV-11771 Change-Id: I801a5f4d8ba84e47a642be4043043af88979dd4e
…nd-secuceboot' <fix>[metadata]: extractOldPrefix use lastIndexOf See merge request zstackio/zstack!9560
Let APIScanVmInstanceMetadataFromPrimaryStorageMsg extend APISyncCallMessage APIImpact Resolves: ZSV-11790 Change-Id: I6b796372747178757767646b77676d646361636f
…secuceboot' <fix>[storage]: update APIScanVmInstanceMetadataFromPrimaryStorageMsg See merge request zstackio/zstack!9563
Move NVRAM preparation logic from preVmMigration to preMigrateVm to avoid handling twice during VM migration. Remove migration type checks since the logic should apply to all migration scenarios. Resolves: ZSV-11438 Related: ZSV-11310 Change-Id: I6578706d6f79696578787079626f6b72616a6a7a
Resolves: ZSV-11631 Change-Id: I616a776e78666179637976616c776162616c6533
…nd-secuceboot' <fix>[kvm]: clone resource ref when revert snapshot See merge request zstackio/zstack!9564
…ecuceboot' <fix>[kvm]: fix NVRAM migration handler duplicate call See merge request zstackio/zstack!9565
Sync SDK rekey result schema to providerResults and add RekeyProviderResult model. Update premium rekey test assertions for provider-level counts and unbound-provider split behavior. Resolves: ZSV-11757 Change-Id: I68676e706f6a786773627074636f6e6a617a6e44
Resolves: ZSV-11630 Change-Id: I616a776e78666179637976616c776162616c6577
Implement VmJustBeforeDeleteFromDbExtensionPoint to clean up TPM resources when VM is removed from database. This prevents TPM resource leakage and ensures proper cleanup. Related: ZSV-11310 Change-Id: I647a6d676377646b6f7773757363646b6977766b
Resolves: ZSV-11630
Resolves: ZSV-11630
…ecuceboot' <fix>[kvm]: remove TPM when VM deleted from DB See merge request zstackio/zstack!9570
…upport-vtpm-and-secuceboot' <fix>[sdk]: align sdk and tests See merge request zstackio/zstack!9568
Resolves: ZSV-11630
When all getLastSyncDate() values in the stream are null, the min() operation with Comparator.nullsFirst() throws NPE because Optional.of() doesn't accept null. Filter null values before calling min() to avoid this issue. Resolves: ZSV-11779 Related: ZSV-11310 Change-Id: I646a65776a717465636e75616c70746763727862
…ecuceboot' <fix>[kvm]: fix NPE in VmHostFileTracker sync See merge request zstackio/zstack!9583
…and-secuceboot' <fix>[crypto]: secret get in vm pre instantiate See merge request zstackio/zstack!9582
Add disableHostStatusCheck() method to KvmCommandSender to allow skipping host status validation. Apply it in KvmSecureBootManager when syncing VM host files, so the sync can proceed regardless of host connection state. Resolves: ZSV-11767 Related: ZSV-11310 Change-Id: I6c68756c7378636a6c6669656869717077716268
…-secuceboot' <fix>[kvm]: disable host status check when syncing VM host files See merge request zstackio/zstack!9591
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
…ecuceboot' <fix>[kvm]: mark TPM VM host files changed on start/shutdown See merge request zstackio/zstack!9595
also: - cleanup EncryptedResourceKeyRefVO when VM creation failed Resolves: ZSV-11729 Resolves: ZSV-11845 Change-Id: I616a776e78666179637976616c776162626c6533
…t-vtpm-and-secuceboot' <fix>[kvm]: define secret on migrate destination See merge request zstackio/zstack!9596
Walkthrough本次变更新增并贯通了“VM 元数据”与“TPM/vTPM/安全启动”相关能力：引入大量消息、全局配置、系统标签、DB 表及扩展点；在KVM/本地存储/NFS主存储实现写入/读取/扫描/清理元数据与主机侧文件同步；补充TPM密钥管理与迁移/克隆/重置流程。 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant API as API 调用方
participant VM as 计算服务
participant PR as 插件注册表
participant PS as 主存储服务
participant H as 主机Agent
API->>VM: 触发云主机删除/清理流程
VM->>PR: 按主存储类型获取 VmMetadataPathBuild 扩展
VM->>PR: 计算 metadataPath/是否需host
alt 需要主机
VM->>PS: CleanupVmInstanceMetadataOnPrimaryStorageMsg(psUuid, path, hostUuid)
else 不需要主机
VM->>PS: CleanupVmInstanceMetadataOnPrimaryStorageMsg(psUuid, path)
end
PS->>H: CLEANUP_VM_METADATA_PATH
H-->>PS: CleanupVmMetadataRsp(success/err)
PS-->>VM: CleanupVmInstanceMetadataOnPrimaryStorageReply
alt 失败
VM->>VM: 提交去重GC(Cleanup...GC)
end
sequenceDiagram
autonumber
participant U as 用户/API
participant TM as KvmTpmManager
participant DB as 数据库
participant KP as KeyProviderBackend
participant KH as KVM Host
U->>TM: APIAddTpmMsg(vmUuid, keyProviderUuid?)
TM->>DB: 创建 TpmVO(关联VM)
alt 指定或默认KeyProvider
TM->>KP: 关联/查询KeyProvider
end
TM-->>U: APIAddTpmEvent(inventory)
U->>TM: 启动/迁移前置
TM->>KP: getOrCreateKey(vm/tpm 上下文)
TM->>KH: SecretHostDefineMsg(封装DEK后下发)
KH-->>TM: SecretHostDefineReply(secretUuid/err)
TM-->>U: 完成或错误
Estimated code review effort🎯 5 (Critical) | ⏱️ ~150 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (6)
header/src/main/java/org/zstack/header/keyprovider/KeyProviderRekeyAssociationExtensionPoint.java-5-10 (1)
5-10:⚠️ Potential issue | 🟡 Minor请为接口方法补充 Javadoc。
这是公共扩展点接口，建议为 3 个方法补齐语义说明，避免实现方理解偏差。
As per coding guidelines: “接口方法不应有多余的修饰符（例如 public），且必须配有有效的 Javadoc 注释。”建议修改
public interface KeyProviderRekeyAssociationExtensionPoint { + /** + * 返回扩展点类型标识。 + */ String getType(); + /** + * 返回关联资源类型。 + */ String getAssociatedResourceType(); + /** + * 根据输入资源 UUID 列表返回关联资源 UUID 列表。 + */ List<String> getAssociatedResourceUuids(List<String> resourceUuids); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/keyprovider/KeyProviderRekeyAssociationExtensionPoint.java` around lines 5 - 10, 为接口 KeyProviderRekeyAssociationExtensionPoint 及其三个方法补充 Javadoc：在接口级别写明该扩展点的职责和使用场景；为 getType()、getAssociatedResourceType() 和 getAssociatedResourceUuids(List<String> resourceUuids) 分别说明语义、返回值含义和可能的取值范围；为 getAssociatedResourceUuids 的参数 resourceUuids 描述输入预期、返回列表与输入顺序/长度关系以及在无关联时的返回约定（例如空列表）；确保注释不包含多余修饰符并遵循项目注释风格（简洁描述 + `@param/`@return 说明）。header/src/main/java/org/zstack/header/tpm/api/TpmMessage.java-3-8 (1)
3-8:⚠️ Potential issue | 🟡 Minor请为接口方法补充 Javadoc。
当前接口方法缺少文档说明，不利于 API 语义与拦截器自动补全行为的长期维护。
✍️ 建议修复
public interface TpmMessage { + /** + * 获取 TPM 绑定的虚拟机 UUID。 + */ String getVmInstanceUuid(); + + /** + * 设置 TPM 绑定的虚拟机 UUID。 + */ void setVmInstanceUuid(String vmInstanceUuid); + + /** + * 获取 TPM UUID。 + */ String getTpmUuid(); + + /** + * 设置 TPM UUID。 + */ void setTpmUuid(String tpmUuid); }As per coding guidelines, “接口方法不应有多余的修饰符（例如 public），且必须配有有效的 Javadoc 注释。”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/tpm/api/TpmMessage.java` around lines 3 - 8, 为接口 TpmMessage 和它的四个方法（getVmInstanceUuid、setVmInstanceUuid、getTpmUuid、setTpmUuid）补充完整的 Javadoc 注释，分别说明每个方法的语义、参数和返回值（例如：getVmInstanceUuid 返回关联虚拟机的 UUID；setVmInstanceUuid 设置该 UUID；getTpmUuid 返回 TPM 设备 UUID；setTpmUuid 设置 TPM UUID），确保注释语言清晰且与拦截器/自动补全逻辑一致；不要添加多余的访问修饰符，保持方法签名为接口默认的隐式 public。compute/src/main/java/org/zstack/compute/vm/devices/TpmMessageAutoCompleter.java-68-74 (1)
68-74:⚠️ Potential issue | 🟡 Minor当
tpmUuid在数据库中不存在时，vmInstanceUuid会被设置为 null。在 Line 68-73 的分支中，如果提供的
tpmUuid在TpmVO表中不存在，查询会返回 null，然后msg.setVmInstanceUuid(null)会将vmInstanceUuid设置为 null。这可能导致下游处理逻辑出现问题。建议增加对
tpmUuid存在性的校验：🛡️ 建议添加 tpmUuid 存在性校验
} else { vmUuid = Q.New(TpmVO.class) .select(TpmVO_.vmInstanceUuid) .eq(TpmVO_.uuid, tpmUuid) .findValue(); + if (vmUuid == null && (!(msg instanceof APIDeleteMessage))) { + throw new ApiMessageInterceptionException(err(TPM_NOT_FOUND, "tpm[uuid:%s] does not exist", tpmUuid)); + } msg.setVmInstanceUuid(vmUuid); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@compute/src/main/java/org/zstack/compute/vm/devices/TpmMessageAutoCompleter.java` around lines 68 - 74, The code in TpmMessageAutoCompleter sets vmInstanceUuid to null when the tpmUuid lookup returns no row; change the logic in the branch referencing tpmUuid, Q.New(TpmVO.class) and vmUuid so that after querying TpmVO_.vmInstanceUuid you validate the result (vmUuid) and handle the missing TPM explicitly instead of calling msg.setVmInstanceUuid(null): either throw a clear IllegalArgumentException/OperationFailure (or return an error response) indicating the tpmUuid does not exist, or skip setting vmInstanceUuid only when present; ensure the error path includes tpmUuid in the message so downstream callers won’t receive a null vmInstanceUuid.plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java-5340-5366 (1)
5340-5366:⚠️ Potential issue | 🟡 Minor构造器存在且合法，不会产生双重回复，但传递
reply作为 backup 参数是冗余的。经验证，
JsonAsyncRESTCallback<>(msg, reply)的构造合法因为 msg 和 reply 都实现AsyncBackup接口。AbstractCompletion只存储 backups 列表但不会自动处理它们，所以代码中手动调用bus.reply(msg, reply)不会导致双重回复。但这与其他常见做法不符。标准做法有：
- 单参数：
new JsonAsyncRESTCallback<T>(completion)并在回调中调用completion.success()/fail()- 两参数（有意义的情况）：当第二参数需要在回调中调用时，如虚拟路由的
(msg, chain)中需要chain.next()在本代码中，
reply已在闭包作用域存在，无需作为 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/KVMHost.java` around lines 5340 - 5366, The JsonAsyncRESTCallback is being constructed with (msg, reply) redundantly passing reply as a backup even though reply is available in the closure and AbstractCompletion does not auto-reply; change the instantiation to the single-parameter form new JsonAsyncRESTCallback<KVMAgentCommands.SecretHostGetResponse>(msg) (or new JsonAsyncRESTCallback<>(completion) pattern used elsewhere) and keep the explicit bus.reply(msg, reply) calls in the fail and success overrides; update the constructor call in the restf.asyncJsonPost invocation accordingly so only msg (or the completion) is passed and remove the extra backup argument.plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageBase.java-3499-3551 (1)
3499-3551:⚠️ Potential issue | 🟡 Minor
successHosts的日志统计会算错。
failedHosts前面已经预先塞进了disconnectedHostUuids，但这里的成功数却是按connectedHostUuids.size() - failedHosts.size()算的。只要存在断连主机，successHosts就会少算，甚至可能变成负数，排障日志会被误导。💡 建议修正
- logger.warn(String.format("partial scan failure on local primary storage[uuid:%s], " + - "failedHosts=%s, successHosts=%d/%d", self.getUuid(), failedHosts, - connectedHostUuids.size() - failedHosts.size(), connectedHostUuids.size())); + logger.warn(String.format("partial scan failure on local primary storage[uuid:%s], " + + "failedHosts=%s, successHosts=%d/%d", self.getUuid(), failedHosts, + connectedHostUuids.size() - errorCodeList.getCauses().size(), connectedHostUuids.size()));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageBase.java` around lines 3499 - 3551, The log computes successHosts incorrectly because failedHosts was pre-seeded with disconnectedHostUuids; update the success count calculation used in the logger.warn to exclude the pre-seeded disconnected hosts. Concretely, in LocalStorageBase where you build failedHosts (which initially contains disconnectedHostUuids) and later log the partial scan, compute failedDuringScan = Math.max(0, failedHosts.size() - disconnectedHostUuids.size()) (or deduplicate and count only failed host UUIDs that are in connectedHostUuids) and then set successHosts = connectedHostUuids.size() - failedDuringScan, and use that successHosts in the logger.warn message instead of connectedHostUuids.size() - failedHosts.size(); ensure you dedupe counts if failedHosts can contain duplicates.plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootExtensions.java-712-774 (1)
712-774:⚠️ Potential issue | 🟡 Minor在第 726 行添加对
getLockedVmInstanceUuids()的防御性检查。
getLockedVmInstanceUuids()返回的列表可能为 null 或空，直接调用.get(0)会抛出 IndexOutOfBoundsException 或 NullPointerException。添加防御性检查以确保安全访问。🛡️ 建议的修复
`@Override` public void afterVolumeLiveSnapshotGroupCreatedOnBackend(CreateVolumesSnapshotOverlayInnerMsg msg, TakeVolumesSnapshotOnKvmReply treply, Completion completion) { if (treply == null || !treply.isSuccess()) { completion.success(); return; } if (!msg.isBackupHostFileIfNeeded()) { completion.success(); return; } + List<String> lockedVmUuids = msg.getLockedVmInstanceUuids(); + if (lockedVmUuids == null || lockedVmUuids.isEmpty()) { + logger.debug("skip backup host files: no locked VM instance UUIDs"); + completion.success(); + return; + } - String vmUuid = msg.getLockedVmInstanceUuids().get(0); + String vmUuid = lockedVmUuids.get(0); String hostUuid = Q.New(VmInstanceVO.class)🤖 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 712 - 774, In KvmSecureBootExtensions.afterVolumeLiveSnapshotGroupCreatedOnBackend, add a defensive null/empty check for msg.getLockedVmInstanceUuids() before calling .get(0); if the list is null or empty, call completion.success() and return to avoid NPE/IndexOutOfBoundsException, otherwise proceed to read vmUuid as before (use getLockedVmInstanceUuids() to locate the VM UUID).
🧹 Nitpick comments (26)
header/src/main/java/org/zstack/header/vm/additions/VmHostFileOperation.java (1)
4-6: 建议将枚举常量改为全大写命名Line 4-6 的
Write/Prepare/Delete不符合仓库常量命名规范，建议改为WRITE/PREPARE/DELETE，避免后续风格不一致。建议修改
public enum VmHostFileOperation { - Write, - Prepare, - Delete, + WRITE, + PREPARE, + DELETE, }As per coding guidelines “常量命名：全部大写，使用下划线分隔单词。”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/vm/additions/VmHostFileOperation.java` around lines 4 - 6, The enum VmHostFileOperation defines constants as Write/Prepare/Delete which violate the repository constant naming convention; rename them to WRITE, PREPARE, DELETE in the VmHostFileOperation enum and update all usages (switches, comparisons, serialization/deserialization references, config or persistence code, tests) to the new names so compilation and behavior remain correct, then run the build/tests to ensure no breakage.plugin/ceph/src/main/java/org/zstack/storage/ceph/primary/CephPrimaryStorageBase.java (1)
1733-1739: 删除未被使用的type查询，或补充基于卷类型的格式分支逻辑。查询
VolumeVO_.type并赋值给本地变量type，但该变量在cmd.format的赋值逻辑中未被使用。类型兼容性无误（VolumeAO_.type声明为SingularAttribute<VolumeAO, VolumeType>），但这是一次冗余的数据库查询。若卷类型不影响格式选择（格式完全由系统标签
FORMAT_QCOW2决定），应删除该查询。若卷类型应参与格式判断（如根卷与数据卷的格式策略不同），需补充分支逻辑。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/ceph/src/main/java/org/zstack/storage/ceph/primary/CephPrimaryStorageBase.java` around lines 1733 - 1739, The local variable 'type' is set from VolumeVO_.type but never used; either remove the redundant DB query (delete the Q.New(...).select(...).findValue() line and the 'type' variable) if format is solely determined by the system tag, or incorporate 'type' into the format decision by adding a branch before setting cmd.format that examines the retrieved VolumeType (the 'type' variable) and sets cmd.format accordingly (e.g., if type == VolumeType.Root then choose one format else another), keeping the existing msg.hasSystemTag(VolumeSystemTags.FORMAT_QCOW2.getTagFormat()) check as appropriate. Ensure references: variable 'type', VolumeVO_.type, cmd.format, and msg.hasSystemTag(...) are updated consistently.header/src/main/java/org/zstack/header/vm/ApplianceVmInstanceCreateExtensionPoint.java (1)
4-4: 建议为扩展点方法补充 Javadoc。这是对外扩展点，建议明确回调时机与实现约束，避免插件实现歧义。
参考修改
public interface ApplianceVmInstanceCreateExtensionPoint { + /** + * 在 Appliance VM 对应的 VmInstanceVO 持久化后触发。 + * `@param` vo 已持久化的 VmInstanceVO + */ default void afterPersistApplianceVmInstanceVO(VmInstanceVO vo) {} }As per coding guidelines "接口方法不应有多余的修饰符（例如 public），且必须配有有效的 Javadoc 注释。"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/vm/ApplianceVmInstanceCreateExtensionPoint.java` at line 4, Add a Javadoc comment on the extension point interface ApplianceVmInstanceCreateExtensionPoint and its method afterPersistApplianceVmInstanceVO(VmInstanceVO vo) that clearly states when this callback is invoked (immediately after the Appliance VM instance VO is persisted), documents the vo parameter, expected return/exception behavior, threading/concurrency guarantees (if any), allowed side effects (e.g., no blocking long-running work), and lifecycle constraints for implementors so plugin authors know the contract and limitations.header/src/main/java/org/zstack/header/vm/devices/VmDevicesSpec.java (1)
28-31:__example__()与“默认值必须为 null”的语义建议再对齐。当前注释强调默认值为
null，但示例里直接填充了tpm，容易让调用方误解“默认会带 TPM 配置”。建议补一句“仅用于示例展示”，或让示例保持tpm为null。可选修改示例
public static VmDevicesSpec __example__() { VmDevicesSpec spec = new VmDevicesSpec(); - spec.setTpm(TpmSpec.__example__()); + // keep null by default; tpm config should be explicitly provided by caller return spec; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/vm/devices/VmDevicesSpec.java` around lines 28 - 31, The VmDevicesSpec.__example__ currently sets a non-null TPM via spec.setTpm(TpmSpec.__example__()) which contradicts the declared semantic that default values must be null; update VmDevicesSpec.__example__ to align with that rule by either leaving tpm unset (keep it null) or add an explicit clarifying JavaDoc/comment in the __example__ method stating the non-null value is shown for demonstration only—refer to VmDevicesSpec.__example__ and the TpmSpec.__example__ usage when making this change.header/src/main/java/org/zstack/header/vm/metadata/MetadataImpact.java (1)
11-27: 建议在消费端强制校验CONFIG/STORAGE的resolver/field。这里用注释声明“必填”，但注解本身允许空字符串，后续很容易被误用。建议在解析
@MetadataImpact的入口统一做 fail-fast 校验。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/vm/metadata/MetadataImpact.java` around lines 11 - 27, The MetadataImpact annotation currently documents that resolver() and field() are required for CONFIG or STORAGE but allows empty strings; add a fail-fast validation where the annotation is parsed (the central annotation-processing or metadata-parsing entry) to throw a clear exception if MetadataImpact has value CONFIG or STORAGE and either resolver() or field() is blank; reference the MetadataImpact annotation and its resolver() and field() accessors when performing this check and ensure updateOnFailure() is unchanged.header/src/main/java/org/zstack/header/vm/metadata/VmMetadataResourcePersistExtensionPoint.java (1)
6-15: 请为扩展点接口方法补齐 Javadoc。当前接口方法（Line [7], Line [9]-[13], Line [15]）无注释，后续实现方难以准确把握参数语义与回调时机。
✍️ 建议补充注释
public interface VmMetadataResourcePersistExtensionPoint { + /** + * 返回当前扩展支持的主存储类型。 + */ String getPrimaryStorageType(); + /** + * 卷元数据持久化完成后回调。 + */ void afterVolumePersist(String primaryStorageUuid, String resourceUuid, String resourceType, String hostUuid, long size, Timestamp now); + /** + * 快照元数据持久化完成后回调。 + */ void afterSnapshotPersist(String primaryStorageUuid, String resourceUuid, String resourceType, String hostUuid, long size, Timestamp now); + /** + * 注册流程回滚后的补偿清理。 + */ default void afterRegistrationRollback(List<String> resourceUuids) {} }As per coding guidelines: “接口方法不应有多余的修饰符（例如 public），且必须配有有效的 Javadoc 注释。”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/vm/metadata/VmMetadataResourcePersistExtensionPoint.java` around lines 6 - 15, Add Javadoc for the interface VmMetadataResourcePersistExtensionPoint and for each method (getPrimaryStorageType, afterVolumePersist, afterSnapshotPersist, afterRegistrationRollback) describing purpose, callback timing, parameter semantics (primaryStorageUuid, resourceUuid, resourceType, hostUuid, size, now, resourceUuids) and expected behavior (e.g., that methods are invoked after a resource/snapshot is persisted on the given primary storage, units for size, meaning of now timestamp, and that afterRegistrationRollback is a no-op default invoked on rollback), and ensure no redundant access modifiers are added to the method declarations.header/src/main/java/org/zstack/header/vm/metadata/VmMetadataPathReplacementExtensionPoint.java (1)
22-24: 保持metadataToCurrentPathMap非空，避免下游重复空判断。Line 22-24 允许传入 null 会破坏 Line 11 的默认非空约定，建议在 setter 中回退为空 Map。
建议修改
public void setMetadataToCurrentPathMap(Map<String, String> metadataToCurrentPathMap) { - this.metadataToCurrentPathMap = metadataToCurrentPathMap; + this.metadataToCurrentPathMap = metadataToCurrentPathMap == null ? new HashMap<>() : metadataToCurrentPathMap; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/vm/metadata/VmMetadataPathReplacementExtensionPoint.java` around lines 22 - 24, 在 VmMetadataPathReplacementExtensionPoint 中，setMetadataToCurrentPathMap 方法当前接受 null，会破坏类中对 metadataToCurrentPathMap 非空默认约定；请在该方法内对传入参数做空值保护（例如当参数为 null 时回退为 Collections.emptyMap() 或 new HashMap<>()）并赋值给字段 metadataToCurrentPathMap，以确保 downstream 无需反复做空判断；保留方法名 setMetadataToCurrentPathMap 和字段 metadataToCurrentPathMap 不变以便定位。plugin/kvm/src/main/java/org/zstack/kvm/KVMConstant.java (1)
214-215: 可选：提取 UUID 连字符化逻辑，避免重复实现。Line [214] 与 Line [233] 的正则转换逻辑重复，后续若调整规则容易出现漂移。建议提炼为单一 helper。
♻️ 参考重构
+ public static final String UUID_TO_HYPHENATED_REGEX = "(\\w{8})(\\w{4})(\\w{4})(\\w{4})(\\w{12})"; + + public static String hyphenateUuid(String vmUuid) { + return vmUuid.replaceFirst(UUID_TO_HYPHENATED_REGEX, "$1-$2-$3-$4-$5"); + } + public static String buildTpmStateFilePath(String vmUuid) { - String vmUuidWithHyphen = vmUuid.replaceFirst("(\\w{8})(\\w{4})(\\w{4})(\\w{4})(\\w{12})", "$1-$2-$3-$4-$5"); + String vmUuidWithHyphen = hyphenateUuid(vmUuid); return String.format(TPM_STATE_FILE_PATH_FORMAT, vmUuidWithHyphen); } @@ public static String buildTpmStateSnapshotBackupFilePath(String vmUuid) { - String vmUuidWithHyphen = vmUuid.replaceFirst("(\\w{8})(\\w{4})(\\w{4})(\\w{4})(\\w{12})", "$1-$2-$3-$4-$5"); + String vmUuidWithHyphen = hyphenateUuid(vmUuid); return String.format(TPM_STATE_SNAPSHOT_BACKUP_FILE_PATH_FORMAT, vmUuidWithHyphen); }Also applies to: 233-234
🤖 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/KVMConstant.java` around lines 214 - 215, 提取重复的 UUID 连字符化逻辑为一个单独 helper 方法（例如 addHyphensToUuid或formatUuidWithHyphens），将当前在 KVMConstant 中使用的 vmUuid.replaceFirst(...) 调用替换为该 helper，并在返回 TPM_STATE_FILE_PATH_FORMAT 的逻辑（当前生成 vmUuidWithHyphen 的位置）以及另一个重复出现于第233-234行的地方统一使用该 helper，确保方法命名清晰且接受原始 vmUuid 字符串并返回带连字符的 UUID。build/deploydb.sh (1)
45-45: 建议修复 Line [45] 的未加引号变量展开。这里会有路径分词风险，建议只保留通配符展开、其余变量加引号。
建议修改
-cp ${base}/../conf/db/zsv/* ${flyway_sql} +cp "${base}"/../conf/db/zsv/* "${flyway_sql}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build/deploydb.sh` at line 45, 在 cp 命令中为避免路径分词风险，将变量展开改为对变量加引号但保留通配符展开；具体修改 cp ${base}/../conf/db/zsv/* ${flyway_sql} 时，引用来源路径和目标变量（即 base 和 flyway_sql）——确保来源路径的变量用引号包裹且通配符在引号外以允许扩展（保留 * 的功能），目标路径用引号包裹，更新涉及的命令为 cp 对应的行（使用 base 和 flyway_sql 标识符定位）。header/src/main/java/org/zstack/header/tpm/TpmErrors.java (1)
12-16: 建议将code声明为final，增强枚举不可变语义。当前实现可用，这里是可选的可维护性优化。
♻️ 建议修改
- private String code; + private final String code;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/tpm/TpmErrors.java` around lines 12 - 16, The enum field "code" in TpmErrors should be made final to enforce immutability: change the declaration of the field "code" to be final and keep assignment in the TpmErrors(int id) constructor (String.format("TPM.%s", id)); update any usages if necessary but no behavior change otherwise. Ensure the final field compiles correctly with the existing constructor in the TpmErrors enum.header/src/main/java/org/zstack/header/storage/primary/ScanVmInstanceMetadataFromPrimaryStorageReply.java (1)
11-13: 建议避免在 getter 中直接暴露内部可变集合。Line 11-13 直接返回内部
List，外部可修改对象状态；建议返回只读视图或副本，和 setter 的防御性拷贝保持一致。可选修改示例
+import java.util.Collections; @@ public List<VmMetadataScanEntry> getVmInstanceMetadata() { - return vmInstanceMetadata; + return Collections.unmodifiableList(vmInstanceMetadata); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/storage/primary/ScanVmInstanceMetadataFromPrimaryStorageReply.java` around lines 11 - 13, The getter ScanVmInstanceMetadataFromPrimaryStorageReply.getVmInstanceMetadata currently returns the internal mutable List directly; change it to return an unmodifiable view or a defensive copy (e.g., Collections.unmodifiableList(vmInstanceMetadata) or new ArrayList<>(vmInstanceMetadata)) to avoid exposing internal state, and ensure this behavior is consistent with the setter (e.g., setVmInstanceMetadata) which should also perform defensive copying.plugin/kvm/src/main/java/org/zstack/kvm/vmfiles/message/BackupVmHostFileMsg.java (1)
28-33: 建议对toResourceUuidList做防御性拷贝，避免可变引用外泄。Line 28-33 当前直接返回/持有外部
List引用，后续可能被调用方意外修改。可选修改示例
+import java.util.ArrayList; +import java.util.Collections; +import java.util.List; @@ public List<String> getToResourceUuidList() { - return toResourceUuidList; + return toResourceUuidList == null ? Collections.emptyList() : new ArrayList<>(toResourceUuidList); } @@ public void setToResourceUuidList(List<String> toResourceUuidList) { - this.toResourceUuidList = toResourceUuidList; + this.toResourceUuidList = toResourceUuidList == null ? null : new ArrayList<>(toResourceUuidList); }🤖 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/vmfiles/message/BackupVmHostFileMsg.java` around lines 28 - 33, The getter and setter for the mutable field toResourceUuidList currently expose and assign the incoming List reference directly; change getToResourceUuidList to return a defensive copy (e.g., new ArrayList<>(toResourceUuidList) or Collections.emptyList() if null) and change setToResourceUuidList to defensively copy the incoming list (or set to null/empty if input is null) so external callers cannot mutate the internal list; update references to the field in the class to assume copies are used.header/src/main/java/org/zstack/header/vm/metadata/VmMetadataDirtyService.java (1)
28-28: 建议避免布尔参数，提升接口语义可读性。Line 28 的
boolean storageStructureChange在调用处容易出现“true/false 语义不自解释”的问题，建议改为枚举或语义化方法名（如markDirtyWithStorageStructureChange(...)）。As per coding guidelines: “避免使用布尔型参数造成含义不明确。”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/vm/metadata/VmMetadataDirtyService.java` at line 28, The method VmMetadataDirtyService.markDirty(String vmInstanceUuid, boolean storageStructureChange) uses a non-descriptive boolean flag; replace it with a semantic alternative such as an enum (e.g., DirtyKind { STORAGE_STRUCTURE_CHANGE, METADATA_CHANGE }) or two clearly named methods (e.g., markDirtyWithStorageStructureChange(...) and markDirtyMetadataOnly(...)); update the interface signature and all callers of markDirty to use the new enum or method names, adjust implementations and tests accordingly, and ensure javadoc/comments reflect the new semantic parameter to preserve backwards-compatible behavior where needed.compute/src/main/java/org/zstack/compute/vm/VmInstanceBase.java (1)
160-166: 这里不要捕获Throwable。
Line 162会把OutOfMemoryError、LinkageError这类 JVM 级故障也吞成普通告警，删除流程会在不可恢复状态下继续执行。这里保留容错语义即可，但建议只 catchException（或 backend 的预期异常层次）。♻️ 建议修改
- } catch (Throwable t) { + } catch (Exception e) { logger.warn(String.format( "failed to detach key provider from TPM[uuid:%s]: %s", - tpmUuid, t.getMessage()), t); + tpmUuid, e.getMessage()), e); }As per coding guidelines, "捕获异常时需严格匹配期望的异常类型或其父类，避免捕获错误的异常类型。"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@compute/src/main/java/org/zstack/compute/vm/VmInstanceBase.java` around lines 160 - 166, The catch in VmInstanceBase around tpmKeyBackend.detachKeyProviderFromTpm(tpmUuid) currently catches Throwable; change it to catch Exception (or the specific backend exception type if available) so JVM errors like OutOfMemoryError/LinkageError are not swallowed, and keep the existing logger.warn call (including the exception) to preserve the fault-tolerant logging behavior; update the try-catch that surrounds detachKeyProviderFromTpm to catch Exception (or the backend's expected exception class) instead of Throwable.header/src/main/java/org/zstack/header/vm/metadata/VmMetadataPathBuildExtensionPoint.java (1)
4-10: 请为扩展点接口方法补充 Javadoc 契约说明。当前接口方法缺少说明，后续实现方难以准确把握参数含义、返回约束和异常语义，建议在每个方法上补齐简洁 Javadoc。
As per coding guidelines "接口方法不应有多余的修饰符（例如
public），且必须配有有效的 Javadoc 注释。"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/vm/metadata/VmMetadataPathBuildExtensionPoint.java` around lines 4 - 10, 为接口 VmMetadataPathBuildExtensionPoint 的每个方法补充简洁 Javadoc，分别说明方法语义、参数含义、返回值约束和异常语义：对 getPrimaryStorageType() 说明返回的主存储类型标识格式和不为 null 的保证；对 buildVmMetadataPath(String primaryStorageUuid, String vmInstanceUuid) 注明 primaryStorageUuid 与 vmInstanceUuid 的预期格式、返回的完整元数据路径格式（例如以斜杠分隔或以特定前缀开头）、以及在输入无效时抛出的异常或返回空的语义；对 buildMetadataDir(String primaryStorageUuid) 说明返回的目录路径格式及 null/空处理规则；对 validateMetadataPath(String primaryStorageUuid, String path) 说明校验规则、返回值含义（例如返回规范化路径或错误信息）、以及何时抛出异常；对 requireHostForCleanup() 注明默认行为、何时应返回 true 以及调用方应如何根据其值调整清理流程。确保 Javadoc 使用标准标签（@param、@return、@throws）并保持简短明确，遵循“接口方法不应有多余修饰符且必须配有有效的 Javadoc”规范。plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmVmHostFileFactory.java (1)
19-20:resourceConfigFacade字段已注入但未使用。该字段在类中没有被引用，可能是遗留代码或为未来功能预留。如果不需要，建议移除以保持代码整洁。
♻️ 建议移除未使用的字段
- `@Autowired` - private ResourceConfigFacade resourceConfigFacade;🤖 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/KvmVmHostFileFactory.java` around lines 19 - 20, The field resourceConfigFacade in class KvmVmHostFileFactory is injected but never used; remove the declaration "@Autowired private ResourceConfigFacade resourceConfigFacade;" from KvmVmHostFileFactory (and any now-unused imports) to clean up dead code, or if it is intentionally reserved for future use, replace it with a commented TODO or mark it as `@SuppressWarnings`("unused") with a brief comment instead.header/src/main/java/org/zstack/header/vm/additions/VmHostFileInventory.java (1)
127-138:__example__()中的lastSyncReason建议使用枚举常量根据 PR 描述，已引入
VmHostFileSyncReason枚举来替代硬编码的同步原因字符串。建议在示例方法中使用枚举值以保持一致性。♻️ 建议的修改
-ref.setLastSyncReason("on libvirt shutdown event"); +ref.setLastSyncReason(VmHostFileSyncReason.LibvirtShutdown.toString());需要添加对应的 import 语句。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/vm/additions/VmHostFileInventory.java` around lines 127 - 138, In VmHostFileInventory.__example__, replace the hard-coded lastSyncReason string passed to setLastSyncReason with the appropriate VmHostFileSyncReason enum constant (e.g., VmHostFileSyncReason.<RELEVANT_CONSTANT>.toString()) and add an import for VmHostFileSyncReason; update the call in __example__ (the setLastSyncReason(...) line) to use the enum value instead of "on libvirt shutdown event".header/src/main/java/org/zstack/header/tpm/api/APIUpdateTpmMsg.java (1)
56-61:__example__()方法可考虑补充tpmUuid当前
__example__()方法设置了keyProviderUuid和vmInstanceUuid，但未设置tpmUuid。虽然TpmMessageAutoCompleter会在运行时自动补全缺失的参数，但为了 API 文档的完整性，建议同时设置tpmUuid。📝 建议的修改
public static APIUpdateTpmMsg __example__() { APIUpdateTpmMsg msg = new APIUpdateTpmMsg(); msg.setKeyProviderUuid(StringDSL.createFixedUuid("keyProviderUuid")); msg.setVmInstanceUuid(DocUtils.createFixedUuid(VmInstanceVO.class)); + msg.setTpmUuid(DocUtils.createFixedUuid(TpmVO.class)); return msg; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/tpm/api/APIUpdateTpmMsg.java` around lines 56 - 61, The example generator APIUpdateTpmMsg.__example__() currently sets keyProviderUuid and vmInstanceUuid but omits tpmUuid; update __example__() to also populate the tpmUuid field (e.g., call the setter like setTpmUuid(...) with a fixed UUID using StringDSL.createFixedUuid("tpmUuid") or a DocUtils.createFixedUuid(...) helper) so the generated API documentation includes a representative tpmUuid value.header/src/main/java/org/zstack/header/host/HostKeyIdentityVO.java (1)
28-29: 考虑使用原始类型boolean替代包装类型Boolean
verified字段使用Boolean包装类型，虽然初始化为false，但在某些反序列化或 ORM 映射场景下可能产生null值，导致潜在的 NPE 风险。根据 ZStack 代码库惯例，对于带默认值的布尔字段，建议使用原始类型。♻️ 建议的修改
`@Column` -private Boolean verified = false; +private boolean verified = false;同时更新 getter/setter：
-public Boolean getVerified() { +public boolean isVerified() { return verified; } -public void setVerified(Boolean verified) { +public void setVerified(boolean verified) { this.verified = verified; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/host/HostKeyIdentityVO.java` around lines 28 - 29, Change the verified field in HostKeyIdentityVO from the wrapper type to a primitive by replacing "private Boolean verified = false;" with a primitive boolean and update its accessor methods (e.g., getVerified()/setVerified(...) or isVerified()/setVerified(...)) to use boolean return/parameter types so the field cannot be null and matches the default initialization.header/src/main/java/org/zstack/header/tpm/entity/TpmCapabilityView.java (1)
24-26: 存在 TODO 注释：keyProvider 相关字段待实现代码中标记了
keyProviderUuid / keyProviderType / keyProviderName / keyProviderKeyVersion字段待添加。请确认这些字段是否需要在本 PR 中实现，或者是否应创建后续 issue 跟踪。需要我帮助生成这些字段的实现代码，或者创建一个新的 issue 来跟踪这个任务吗？
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/tpm/entity/TpmCapabilityView.java` around lines 24 - 26, The file contains a TODO for keyProvider fields that must either be implemented now or tracked as an issue; add four new fields to the TpmCapabilityView class: keyProviderUuid, keyProviderType, keyProviderName, and keyProviderKeyVersion (use appropriate types, e.g., String), annotate each with the JPA mapping used in this class (e.g., `@Column`) and generate public getters/setters consistent with existing style, and update any equals/hashCode/toString or DTO mapping methods in TpmCapabilityView if present; if you decide not to implement in this PR, create a new issue referencing TpmCapabilityView and list these four fields with expected types and mapping so it can be implemented later.compute/src/main/java/org/zstack/compute/vm/VmExpungeMetadataFlow.java (1)
61-61: 建议：移除多余的空行Line 61 处有多余的空行，建议移除以保持代码整洁。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@compute/src/main/java/org/zstack/compute/vm/VmExpungeMetadataFlow.java` at line 61, Remove the redundant blank line at Line 61 in the VmExpungeMetadataFlow class to keep formatting consistent; locate the class/file org.zstack.compute.vm.VmExpungeMetadataFlow (or the surrounding method in VmExpungeMetadataFlow) and delete the extra empty line so the code follows the project's style conventions.plugin/kvm/src/main/java/org/zstack/kvm/HostKeyIdentityHelper.java (1)
25-25: 建议：使用统一的日志工具方法其他 ZStack 类通常使用
Utils.getLogger(Class)来获取日志记录器。此处直接使用CLoggerImpl.getLogger虽然功能正确，但与项目惯例不一致。♻️ 可选的修改建议
- private static final CLogger logger = org.zstack.utils.logging.CLoggerImpl.getLogger(HostKeyIdentityHelper.class); + private static final CLogger logger = Utils.getLogger(HostKeyIdentityHelper.class);🤖 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/HostKeyIdentityHelper.java` at line 25, Replace the direct use of CLoggerImpl.getLogger in HostKeyIdentityHelper by obtaining the logger via the project's unified helper: change the static logger initialization (the variable named logger in class HostKeyIdentityHelper) to call Utils.getLogger(HostKeyIdentityHelper.class) instead of org.zstack.utils.logging.CLoggerImpl.getLogger(...) so the class follows the same logging convention as other ZStack classes.plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageSimulator.java (1)
375-383: 建议：记录prefixRebaseBackingFiles命令到配置其他模拟器端点都会将
cmd记录到config中以便测试验证，但prefixRebaseBackingFiles没有记录。建议保持一致性，将命令添加到配置列表中。♻️ 可选的修改建议
`@RequestMapping`(value=LocalStorageKvmBackend.PREFIX_REBASE_BACKING_FILES_PATH, method= RequestMethod.POST) public `@ResponseBody` String prefixRebaseBackingFiles(HttpEntity<String> entity) { PrefixRebaseBackingFilesCmd cmd = JSONObjectUtil.toObject(entity.getBody(), PrefixRebaseBackingFilesCmd.class); + config.prefixRebaseBackingFilesCmds.add(cmd); PrefixRebaseBackingFilesRsp rsp = new PrefixRebaseBackingFilesRsp(); rsp.rebasedCount = cmd.filePaths == null ? 0 : cmd.filePaths.size(); reply(entity, rsp); return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageSimulator.java` around lines 375 - 383, The prefixRebaseBackingFiles handler in LocalStorageSimulator doesn't record the incoming PrefixRebaseBackingFilesCmd into the simulator config like other endpoints do; update the method (prefixRebaseBackingFiles in class LocalStorageSimulator) to append or store the parsed cmd object into the simulator configuration/request list (the same config field other handlers use for test verification), then keep constructing PrefixRebaseBackingFilesRsp and calling reply(entity, rsp) as before so tests can assert the recorded command.plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootManager.java (1)
665-668: 这里应该使用VmHostBackupFileVO_.type。当前查询实体是
VmHostBackupFileVO，但条件字段用了VmHostFileVO_.type。即使今天字段名一致，这里也丢掉了元模型的类型约束，后面字段一旦分叉会很难排查。🛠️ 建议修改
context.backupFiles.addAll(Q.New(VmHostBackupFileVO.class) .eq(VmHostBackupFileVO_.resourceUuid, msg.getSrcVmUuid()) - .in(VmHostFileVO_.type, missingTypes) + .in(VmHostBackupFileVO_.type, missingTypes) .list());🤖 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 665 - 668, The query uses the wrong metamodel field: replace VmHostFileVO_.type with the correct VmHostBackupFileVO_.type in the call that populates context.backupFiles so the JPA criteria targets the VmHostBackupFileVO entity; update the predicate in the Q.New(VmHostBackupFileVO.class)... .in(...) expression (which currently uses VmHostFileVO_.type) to use VmHostBackupFileVO_.type alongside the existing VmHostBackupFileVO_.resourceUuid, msg.getSrcVmUuid(), and missingTypes references.plugin/kvm/src/main/java/org/zstack/kvm/KVMAgentCommands.java (1)
408-473: 建议对encryptedDek字段添加@NoLogging注解。虽然 DEK 已加密，但作为安全相关的敏感数据，建议添加
@NoLogging注解以防止在日志中泄露。🔒 建议的修复
public static class SecretHostDefineCmd extends AgentCommand { /** Base64 envelope of DEK; agent expects this field name (encryptedDek). */ + `@NoLogging` private String encryptedDek;🤖 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/KVMAgentCommands.java` around lines 408 - 473, SecretHostDefineCmd currently exposes the encryptedDek field via getter/setter and should be marked to prevent accidental logging; add the `@NoLogging` annotation to the encryptedDek field declaration (and import the annotation) in class SecretHostDefineCmd so that encryptedDek is excluded from logs while leaving getters/setters intact.plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootExtensions.java (1)
228-248: 冗余的成功状态检查。在
sender.send的 wrapper 回调中已经检查了writeRsp.isSuccess()并返回错误，因此success回调中的再次检查是冗余的。当 wrapper 返回非 null 错误时，不会进入success回调。♻️ 建议简化
sender.send(cmd, WRITE_VM_HOST_FILE_PATH, wrapper -> { KVMAgentCommands.WriteVmHostFileContentResponse writeRsp = wrapper.getResponse(KVMAgentCommands.WriteVmHostFileContentResponse.class); return writeRsp.isSuccess() ? null : operr("failed to write file content response").withException(writeRsp.getError()); }, new ReturnValueCompletion<KvmResponseWrapper>(completion) { `@Override` public void success(KvmResponseWrapper wrapper) { - KVMAgentCommands.WriteVmHostFileContentResponse writeRsp = wrapper.getResponse(KVMAgentCommands.WriteVmHostFileContentResponse.class); - if (!writeRsp.isSuccess()) { - completion.fail(operr("failed to write file content response").withException(writeRsp.getError())); - return; - } completion.success(); } `@Override` public void fail(ErrorCode errorCode) { completion.fail(errorCode); } });🤖 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 228 - 248, The success path redundantly re-checks writeRsp.isSuccess() inside the ReturnValueCompletion.success callback even though the wrapper callback already returns an operr when writeRsp.isSuccess() is false; remove the duplicate check in the success override of the ReturnValueCompletion<KvmResponseWrapper> and simply call completion.success() there, leaving error handling to the wrapper lambda that returns operr(...). Ensure you keep the wrapper lambda that returns operr("failed to write file content response").withException(writeRsp.getError()) for failure and retain completion.fail(errorCode) in fail().
|
|
||
| cp $base/db/V0.6__schema.sql $flyway_sql | ||
| cp $base/db/upgrade/* $flyway_sql | ||
| cp $base/db/zsv/* $flyway_sql |
There was a problem hiding this comment.
保护 Line [45] 的 glob 复制，避免部署脚本被空目录/分词中断。
当前写法在 set -e 下对空目录和路径分词不安全，可能导致初始化流程直接失败。
🛠️ 建议修复
-cp $base/db/zsv/* $flyway_sql
+if compgen -G "$base/db/zsv/*" > /dev/null; then
+ cp "$base"/db/zsv/* "$flyway_sql"
+fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| cp $base/db/zsv/* $flyway_sql | |
| if compgen -G "$base/db/zsv/*" > /dev/null; then | |
| cp "$base"/db/zsv/* "$flyway_sql" | |
| fi |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 45-45: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 45-45: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@conf/deploydb.sh` at line 45, The cp invocation cp $base/db/zsv/* $flyway_sql
is brittle under set -e and unquoted variables (fails on empty dirs or
word-splitting); change it to a safe guarded copy: quote variables and only run
cp when the glob matches (e.g., use a guard like if compgen -G "$base/db/zsv/*"
> /dev/null; then cp -- "$base/db/zsv/"* "$flyway_sql"; fi) or replace with a
robust alternative (rsync -a "$base/db/zsv/" "$flyway_sql"/) so that $base and
$flyway_sql are quoted and the command no longer errors on an empty source
directory.
Comment on lines
+102
to
+105
| } else if (annotation.type().longText()) { | ||
| if (raw == null) | ||
| return null; | ||
| return (raw.length() < 20) ? raw : raw.substring(0, 10) + "..." + raw.substring(raw.length() - 10); |
There was a problem hiding this comment.
LongText 在短字符串场景会输出明文
@NoLogging 字段在 Line 105 遇到短文本时返回原文，可能导致敏感内容写入日志。建议短文本也做掩码。
建议修改
} else if (annotation.type().longText()) {
- if (raw == null)
+ if (raw == null) {
return null;
- return (raw.length() < 20) ? raw : raw.substring(0, 10) + "..." + raw.substring(raw.length() - 10);
+ }
+ if (raw.length() <= 20) {
+ return "*****";
+ }
+ return raw.substring(0, 10) + "..." + raw.substring(raw.length() - 10);
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } else if (annotation.type().longText()) { | |
| if (raw == null) | |
| return null; | |
| return (raw.length() < 20) ? raw : raw.substring(0, 10) + "..." + raw.substring(raw.length() - 10); | |
| } else if (annotation.type().longText()) { | |
| if (raw == null) { | |
| return null; | |
| } | |
| if (raw.length() <= 20) { | |
| return "*****"; | |
| } | |
| return raw.substring(0, 10) + "..." + raw.substring(raw.length() - 10); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/src/main/java/org/zstack/core/log/LogSafeGson.java` around lines 102 -
105, In LogSafeGson's handling of annotation.type().longText() the code
currently returns the raw string for short inputs (raw.length() < 20), which
exposes sensitive data; change this branch so short strings are also masked
instead of returned verbatim — update the logic around the raw variable in the
annotation.type().longText() block (inside class LogSafeGson) to return a masked
representation for short values (e.g., a fixed masked token or the same
prefix/.../suffix masking strategy used for long strings) rather than raw,
ensuring null handling remains intact.
Comment on lines
+264
to
+268
| DebugUtils.Assert(!asyncBackups.isEmpty(), | ||
| "propagateExceptionTo() must be called before error(Consumer<ErrorCode>)." | ||
| + " Call .propagateExceptionTo(completion) or .propagateExceptionTo((AsyncBackup) null) if you don't need async backup"); | ||
| AsyncBackup firstAsyncBackup = this.asyncBackups.get(0); | ||
| AsyncBackup[] otherAsyncBackups = this.asyncBackups.subList(1, this.asyncBackups.size()).toArray(new AsyncBackup[0]); |
There was a problem hiding this comment.
不要把 done/error 的便利重载改成必须先调用 propagateExceptionTo() 的破坏性前置条件。
Line 264-268 和 Line 348-352 现在把 asyncBackups 为空视为非法状态，并马上 get(0) 取首元素。这样 done(Runnable) / error(Consumer<ErrorCode>) 就不再是“无 AsyncBackup 也能用”的快捷入口，而是必须额外插入 propagateExceptionTo(...) 的隐藏协议；现有只调用这两个重载的链会在运行时失败。建议继续兼容空 asyncBackups，只在显式配置了 backup 时再拆 first/rest。
🔧 建议修改
@@
`@SuppressWarnings`("rawtypes")
public SimpleFlowChain error(Consumer<ErrorCode> handler) {
- DebugUtils.Assert(!asyncBackups.isEmpty(),
- "propagateExceptionTo() must be called before error(Consumer<ErrorCode>)."
- + " Call .propagateExceptionTo(completion) or .propagateExceptionTo((AsyncBackup) null) if you don't need async backup");
- AsyncBackup firstAsyncBackup = this.asyncBackups.get(0);
- AsyncBackup[] otherAsyncBackups = this.asyncBackups.subList(1, this.asyncBackups.size()).toArray(new AsyncBackup[0]);
+ AsyncBackup firstAsyncBackup = this.asyncBackups.isEmpty() ? null : this.asyncBackups.get(0);
+ AsyncBackup[] otherAsyncBackups = this.asyncBackups.size() <= 1
+ ? new AsyncBackup[0]
+ : this.asyncBackups.subList(1, this.asyncBackups.size()).toArray(new AsyncBackup[0]);
@@
`@SuppressWarnings`("rawtypes")
public SimpleFlowChain done(Runnable runnable) {
- DebugUtils.Assert(!asyncBackups.isEmpty(),
- "propagateExceptionTo() must be called before done(Runnable)."
- + " Call .propagateExceptionTo(completion) or .propagateExceptionTo((AsyncBackup) null) if you don't need async backup");
- AsyncBackup firstAsyncBackup = this.asyncBackups.get(0);
- AsyncBackup[] otherAsyncBackups = this.asyncBackups.subList(1, this.asyncBackups.size()).toArray(new AsyncBackup[0]);
+ AsyncBackup firstAsyncBackup = this.asyncBackups.isEmpty() ? null : this.asyncBackups.get(0);
+ AsyncBackup[] otherAsyncBackups = this.asyncBackups.size() <= 1
+ ? new AsyncBackup[0]
+ : this.asyncBackups.subList(1, this.asyncBackups.size()).toArray(new AsyncBackup[0]);Also applies to: 348-352
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/src/main/java/org/zstack/core/workflow/SimpleFlowChain.java` around
lines 264 - 268, The change made SimpleFlowChain to treat asyncBackups as
illegal when empty by immediately calling asyncBackups.get(0) (in the block
around lines creating firstAsyncBackup/otherAsyncBackups), which breaks the
convenient overloads done(Runnable) and error(Consumer<ErrorCode>); revert to
tolerant behavior by guarding the split logic: check asyncBackups.isEmpty() and
if so keep the existing no-backup path (do not access get(0) or subList),
otherwise extract AsyncBackup firstAsyncBackup = asyncBackups.get(0) and
otherAsyncBackups = asyncBackups.subList(1,...). Ensure the same guard is
applied in both locations referenced (the blocks that construct
firstAsyncBackup/otherAsyncBackups) so propagateExceptionTo(...) remains
optional for callers of done(...) and error(...).
Comment on lines
+25
to
+30
| public static final List<VmInstanceState> ALLOW_TAKE_SNAPSHOTS_VM_STATES = Arrays.asList( | ||
| VmInstanceState.Running, VmInstanceState.Stopped, VmInstanceState.Paused | ||
| ); | ||
| public static final List<VmInstanceState> ALLOW_TAKE_MEMORY_SNAPSHOTS_VM_STATES = Arrays.asList( | ||
| VmInstanceState.Stopped | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=java -C2 'ALLOW_TAKE_(MEMORY_)?SNAPSHOTS_VM_STATES\s*\.\s*(add|remove|set|clear|replaceAll|sort)\s*\('Repository: MatheMatrix/zstack
Length of output: 44
🏁 Script executed:
# 查找并验证文件
find . -type f -name "VolumeSnapshotConstant.java" 2>/dev/null | head -5Repository: MatheMatrix/zstack
Length of output: 147
🏁 Script executed:
# 验证常量声明的具体位置和内容
if [ -f "header/src/main/java/org/zstack/header/storage/snapshot/VolumeSnapshotConstant.java" ]; then
cat -n "header/src/main/java/org/zstack/header/storage/snapshot/VolumeSnapshotConstant.java" | sed -n '1,50p'
fiRepository: MatheMatrix/zstack
Length of output: 1388
🏁 Script executed:
# 检查这两个常量被使用的位置
rg -t java "ALLOW_TAKE_(MEMORY_)?SNAPSHOTS_VM_STATES" --max-count=20Repository: MatheMatrix/zstack
Length of output: 808
避免将可修改的 List 作为公共常量暴露。
这里使用 Arrays.asList(...) 初始化后，外部可通过 set() 修改元素。虽然当前代码中的使用都是只读操作（.contains() 调用和参数传递），但这种设计存在风险，会让全局"允许状态常量"在运行期被污染。建议改为不可变列表。
💡 建议修复
import org.zstack.header.vm.VmInstanceState;
import java.util.Arrays;
+import java.util.Collections;
import java.util.List;
@@
- public static final List<VmInstanceState> ALLOW_TAKE_SNAPSHOTS_VM_STATES = Arrays.asList(
+ public static final List<VmInstanceState> ALLOW_TAKE_SNAPSHOTS_VM_STATES = Collections.unmodifiableList(Arrays.asList(
VmInstanceState.Running, VmInstanceState.Stopped, VmInstanceState.Paused
- );
- public static final List<VmInstanceState> ALLOW_TAKE_MEMORY_SNAPSHOTS_VM_STATES = Arrays.asList(
+ ));
+ public static final List<VmInstanceState> ALLOW_TAKE_MEMORY_SNAPSHOTS_VM_STATES = Collections.unmodifiableList(Arrays.asList(
VmInstanceState.Stopped
- );
+ ));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@header/src/main/java/org/zstack/header/storage/snapshot/VolumeSnapshotConstant.java`
around lines 25 - 30, The public constants ALLOW_TAKE_SNAPSHOTS_VM_STATES and
ALLOW_TAKE_MEMORY_SNAPSHOTS_VM_STATES are backed by Arrays.asList(...) which
returns a mutable list; change their initialization to produce an immutable list
(e.g. use Collections.unmodifiableList(new ArrayList<>(Arrays.asList(...))) or
Java 9+ List.of(...) / List.copyOf(...)) so callers cannot modify the global
constants; update imports if necessary and ensure the symbols
ALLOW_TAKE_SNAPSHOTS_VM_STATES, ALLOW_TAKE_MEMORY_SNAPSHOTS_VM_STATES and the
Arrays.asList usage in VolumeSnapshotConstant are replaced accordingly.
Comment on lines
+485
to
+498
| public void setDevices(Map<String, Object> devices) { | ||
| this.devices = devices; | ||
| } | ||
|
|
||
| public VmDevicesSpec getDevicesSpec() { | ||
| if (devicesSpec == null && devices != null) { | ||
| devicesSpec = JSONObjectUtil.rehashObject(devices, VmDevicesSpec.class); | ||
| } | ||
| return devicesSpec; | ||
| } | ||
|
|
||
| public void setDevicesSpec(VmDevicesSpec devicesSpec) { | ||
| this.devicesSpec = devicesSpec; | ||
| } |
There was a problem hiding this comment.
修复 devicesSpec 缓存失效问题，避免返回陈旧设备配置。
Line [485]-[487] 更新 devices 时没有清理缓存；之后 Line [489]-[494] 可能继续返回旧的 devicesSpec，导致创建 VM 设备参数与请求不一致。
✅ 建议修复
public void setDevices(Map<String, Object> devices) {
this.devices = devices;
+ this.devicesSpec = null;
}
+@SuppressWarnings("unchecked")
public void setDevicesSpec(VmDevicesSpec devicesSpec) {
this.devicesSpec = devicesSpec;
+ this.devices = devicesSpec == null ? null : JSONObjectUtil.rehashObject(devicesSpec, Map.class);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@header/src/main/java/org/zstack/header/vm/APICreateVmInstanceMsg.java` around
lines 485 - 498, When setDevices(Map<String, Object> devices) is called it
updates the raw devices but doesn’t clear the cached devicesSpec, so
getDevicesSpec() can return stale data; modify setDevices to reset/clear the
cached devicesSpec (e.g., set devicesSpec to null) after assigning devices so
subsequent getDevicesSpec() will rehydrate from the new devices map; also
consider mirroring this behavior in setDevicesSpec(VmDevicesSpec) if needed to
keep devices and devicesSpec consistent.
Comment on lines
+319
to
+324
| static RemoveTpmFromVmContext valueOf(RemoveTpmMsg msg) { | ||
| RemoveTpmFromVmContext context = new RemoveTpmFromVmContext(); | ||
| context.vmInstanceUuid = msg.getVmInstanceUuid(); | ||
| context.tpmUuid = msg.getTpmUuid(); | ||
| return context; | ||
| } |
There was a problem hiding this comment.
force 没有从消息透传到 context。
skipIf(data -> context.force) 因此永远是 false，强制删除路径实际不会生效，删除/回滚场景仍会被 VM 状态校验拦住。
🛠️ 建议修改
static RemoveTpmFromVmContext valueOf(RemoveTpmMsg msg) {
RemoveTpmFromVmContext context = new RemoveTpmFromVmContext();
context.vmInstanceUuid = msg.getVmInstanceUuid();
context.tpmUuid = msg.getTpmUuid();
+ context.force = msg.isForce();
return context;
}Also applies to: 329-343
🤖 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
319 - 324, The context's force flag isn't being set from the incoming message,
so skipIf(data -> context.force) is always false and forced-delete path never
runs; update RemoveTpmFromVmContext.valueOf(RemoveTpmMsg msg) to copy
msg.getForce() (or the correct boolean getter) into context.force, and apply the
same fix to the other similar factory/mapper method referenced (the block around
the other occurrence) so both RemoveTpmFromVmContext instances propagate the
force flag from RemoveTpmMsg into the context.
Comment on lines
+913
to
+918
| private void handle(APIRemoveTpmMsg msg) { | ||
| APIRemoveTpmEvent event = new APIRemoveTpmEvent(msg.getId()); | ||
|
|
||
| RemoveTpmMsg inner = RemoveTpmMsg.valueOf(msg); | ||
| bus.makeTargetServiceIdByResourceUuid(inner, SERVICE_ID, msg.getTpmUuid()); | ||
| bus.send(inner, new CloudBusCallBack(msg) { |
There was a problem hiding this comment.
按 tpmUuid 路由会绕过这里按 VM 的串行队列。
这个服务用 tpmQueueSyncSignature(vmInstanceUuid) 做串行化，但 API remove 这里却按 tpmUuid 做 makeTargetServiceIdByResourceUuid()。同一 VM 的 add/remove/reset 很可能被打到不同的 MN，上面的 ChainTask 就失去保护了。这里应统一按 vmInstanceUuid 路由。
🛠️ 建议修改
RemoveTpmMsg inner = RemoveTpmMsg.valueOf(msg);
- bus.makeTargetServiceIdByResourceUuid(inner, SERVICE_ID, msg.getTpmUuid());
+ bus.makeTargetServiceIdByResourceUuid(inner, SERVICE_ID, inner.getVmInstanceUuid());
bus.send(inner, new CloudBusCallBack(msg) {🤖 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
913 - 918, APIRemoveTpmMsg is currently routed by tpmUuid which bypasses the
VM-level serial queue (tpmQueueSyncSignature(vmInstanceUuid)) and can let
concurrent add/remove/reset on the same VM run on different MNs; update the
routing in handle(APIRemoveTpmMsg) so the inner RemoveTpmMsg is targeted by the
VM UUID instead of the TPM UUID (use the vmInstanceUuid available on
RemoveTpmMsg/APIRemoveTpmMsg populated by TpmMessageAutoCompleter) when calling
bus.makeTargetServiceIdByResourceUuid(inner, SERVICE_ID, ...), ensuring
ChainTask VM-level serialization (tpmQueueSyncSignature(vmInstanceUuid)) is
preserved.
| private String vmUuid; | ||
| private String nvRamPath; | ||
| private String tpmStateFolder; | ||
| private String syncReason; |
There was a problem hiding this comment.
syncReason 缺少必填约束，可能写入空审计原因。
为保证“每次同步都有原因”的审计目标，建议在 setter 中至少做非空校验（并最好拒绝空白字符串）。
🔧 建议修复
+import java.util.Objects;
+
public class SyncVmHostFilesFromHostMsg extends NeedReplyMessage {
@@
public void setSyncReason(String syncReason) {
- this.syncReason = syncReason;
+ this.syncReason = Objects.requireNonNull(syncReason, "syncReason must not be null");
+ if (this.syncReason.isBlank()) {
+ throw new IllegalArgumentException("syncReason must not be blank");
+ }
}Also applies to: 50-51
🤖 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/vmfiles/message/SyncVmHostFilesFromHostMsg.java`
at line 10, The field syncReason on class SyncVmHostFilesFromHostMsg can be set
to null/blank; add a defensive non-null and non-blank check in the setter
(setSyncReason) so empty reasons are rejected (e.g., throw
IllegalArgumentException when syncReason is null or blank) and optionally
document the requirement; ensure the same validation is applied to any other
places that set syncReason (constructors or builders) to enforce the "every sync
must have a reason" audit rule.
Comment on lines
+264
to
+280
| boolean hasChanged = group.stream().anyMatch(f -> f.getChangeDate() != null); | ||
| String syncReason; | ||
| if (hasChanged) { | ||
| syncReason = VmHostFileSyncReason.PeriodicDirtyCheck.reason(); | ||
| } else { | ||
| // check if force sync is needed based on lastSyncDate | ||
| Timestamp oldestLastSync = group.stream() | ||
| .map(VmHostFileVO::getLastSyncDate) | ||
| .filter(Objects::nonNull) | ||
| .min(Comparator.naturalOrder()) | ||
| .orElse(null); | ||
|
|
||
| if (oldestLastSync != null && (now - oldestLastSync.getTime()) < forceSyncThresholdMs) { | ||
| whileCompletion.done(); | ||
| return; | ||
| } | ||
| syncReason = VmHostFileSyncReason.PeriodicForceSync.reason(); |
There was a problem hiding this comment.
把 lastSyncDate == null 视为“立即需要 force sync”。
这里先过滤了 null 再取 oldestLastSync。只要同一个 vm+host 分组里有一个文件最近同步过，另一个从未同步过的文件就会被一起跳过，新增的 NvRam/TpmState 可能要等到 forceSyncThresholdMs 之后才首次拉取。
参考修正
if (hasChanged) {
syncReason = VmHostFileSyncReason.PeriodicDirtyCheck.reason();
} else {
+ boolean hasNeverSynced = group.stream().anyMatch(f -> f.getLastSyncDate() == null);
// check if force sync is needed based on lastSyncDate
Timestamp oldestLastSync = group.stream()
.map(VmHostFileVO::getLastSyncDate)
.filter(Objects::nonNull)
.min(Comparator.naturalOrder())
.orElse(null);
- if (oldestLastSync != null && (now - oldestLastSync.getTime()) < forceSyncThresholdMs) {
+ if (!hasNeverSynced && oldestLastSync != null
+ && (now - oldestLastSync.getTime()) < forceSyncThresholdMs) {
whileCompletion.done();
return;
}
syncReason = VmHostFileSyncReason.PeriodicForceSync.reason();
}🤖 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/vmfiles/VmHostFileTracker.java`
around lines 264 - 280, The grouping logic currently ignores VmHostFileVO
entries with lastSyncDate == null which causes new files to be skipped from
immediate force sync; update the check in the method that computes
oldestLastSync (used with hasChanged, forceSyncThresholdMs, VmHostFileSyncReason
and whileCompletion.done()) so that any null lastSyncDate is treated as “needs
immediate force sync” (e.g., if any VM host file in the group has lastSyncDate
== null, short-circuit to perform force sync and set syncReason to
VmHostFileSyncReason.PeriodicForceSync.reason() rather than filtering out nulls
and using the min date).
Comment on lines
1150
to
1152
| volume.setInstallPath(cmd.getInstallUrl()); | ||
| volume.setFormat(VolumeConstant.VOLUME_FORMAT_QCOW2); | ||
| volume.setFormat(cmd.getVolumeFormat()); | ||
| volume.setActualSize(rsp.actualSize); |
There was a problem hiding this comment.
这里回填的是 cmd 里的格式，不是 agent 实际创建出来的格式。
当前方法里没有给 CreateEmptyVolumeCmd 显式设置 volumeFormat，所以 cmd.getVolumeFormat() 只能得到未初始化/默认值，至少拿不到 host 端真实落盘格式。把这个值写回 volume.setFormat(...) 后，后续依赖卷格式分支的逻辑会读到错误元数据。这里应从 CreateEmptyVolumeResponse 回填，或者在发送前明确设置请求格式。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@plugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageKVMBackend.java`
around lines 1150 - 1152, The code in NfsPrimaryStorageKVMBackend currently
writes volume.setFormat(cmd.getVolumeFormat()), but CreateEmptyVolumeCmd isn't
guaranteed to have the actual host-created format, so this can store wrong
metadata; instead use the agent response's format (e.g.,
CreateEmptyVolumeResponse/ rsp.format or rsp.getVolumeFormat()) to set
volume.setFormat(...), or ensure the request (CreateEmptyVolumeCmd) explicitly
sets the desired volumeFormat before sending; update the code paths that set
volume format to read from the response object (CreateEmptyVolumeResponse) or
populate CreateEmptyVolumeCmd.volumeFormat prior to send to guarantee correct
metadata.
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
3 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.
Add lastSyncReason column to VmHostFileVO table to track
the reason for each VM host file sync operation. This helps
with debugging and auditing.
Update SyncVmHostFilesFromHostMsg to include syncReason
parameter and set it at various sync triggers:
Update DB schema and all sync operation calls to properly
record the sync reason for better traceability.
Related: ZSV-11310
Change-Id: I7978696275717271667076676b79706a646c6a66
sync from gitlab !9622