<fix>[vm]: add cleanup all vm metadata api - #4639
Conversation
47eb986 to
943e03f
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough新增 VM 元数据清理屏障持久化结构,提供全量清理 API 与 SDK,并将清理请求扩展到 Local Storage、NFS Storage 及 KVM 主机代理,同时传递元数据 generation 并汇总失败主存储。 ChangesVM 元数据清理功能
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant APIClient
participant PrimaryStorage
participant HostAgent
APIClient->>PrimaryStorage: DELETE /v1/vm-instances/metadata
PrimaryStorage->>HostAgent: 调用 cleanupall 并传递元数据参数
HostAgent-->>PrimaryStorage: 返回清理结果
PrimaryStorage-->>APIClient: 返回失败主存储 UUID 列表
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
dac38e6 to
ab895a2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
header/src/main/java/org/zstack/header/vm/metadata/VmMetadataCleanupBarrierState.java (1)
3-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win枚举常量请遵循全大写命名。
Idle、Draining、Cleaning应改为IDLE、DRAINING、CLEANING,并同步更新 schema 中的状态值及所有引用,避免持久化状态名称不一致。As per path instructions: 常量名必须全部大写,并使用下划线分隔单词。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@header/src/main/java/org/zstack/header/vm/metadata/VmMetadataCleanupBarrierState.java` around lines 3 - 6, 将 VmMetadataCleanupBarrierState 中的枚举常量重命名为全大写的 IDLE、DRAINING 和 CLEANING,并同步更新 schema 状态值及所有引用,确保代码与持久化状态名称保持一致。Source: Path instructions
plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageBase.java (1)
3658-3661: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value请使用完整变量名。
新增的
bkd和idx不利于理解清理链路。
plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageBase.java#L3658-L3661: 将bkd重命名为backend。plugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorage.java#L2084-L2108: 将idx重命名为hostIndex,并同步更新递归调用。As per path instructions:不允许使用不必要的缩写,应使用完整单词提升可读性。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageBase.java` around lines 3658 - 3661, Use complete variable names in the cleanup flows: rename bkd to backend in LocalStorageBase.java at lines 3658-3661 and update all references; rename idx to hostIndex in NfsPrimaryStorage.java at lines 2084-2108 and update the recursive call and all other references.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@header/src/main/java/org/zstack/header/vm/metadata/VmMetadataCleanupBarrierVO.java`:
- Around line 48-51: 移除 VmMetadataCleanupBarrierVO 的 `@PreUpdate` 回调及其对 lastOpDate
的 null 赋值,保留数据库通过 ON UPDATE CURRENT_TIMESTAMP 自动更新时间,避免向 NOT NULL 时间字段写入空值。
---
Nitpick comments:
In
`@header/src/main/java/org/zstack/header/vm/metadata/VmMetadataCleanupBarrierState.java`:
- Around line 3-6: 将 VmMetadataCleanupBarrierState 中的枚举常量重命名为全大写的 IDLE、DRAINING
和 CLEANING,并同步更新 schema 状态值及所有引用,确保代码与持久化状态名称保持一致。
In
`@plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageBase.java`:
- Around line 3658-3661: Use complete variable names in the cleanup flows:
rename bkd to backend in LocalStorageBase.java at lines 3658-3661 and update all
references; rename idx to hostIndex in NfsPrimaryStorage.java at lines 2084-2108
and update the recursive call and all other references.
🪄 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: 740683f9-e512-47c3-b5a9-5c563018ad74
⛔ Files ignored due to path filters (2)
conf/persistence.xmlis excluded by!**/*.xmlconf/serviceConfig/vmInstance.xmlis excluded by!**/*.xml
📒 Files selected for processing (21)
conf/db/zsv/V5.1.0__schema.sqlheader/src/main/java/org/zstack/header/storage/primary/CleanupAllVmMetadataOnPrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/storage/primary/CleanupAllVmMetadataOnPrimaryStorageReply.javaheader/src/main/java/org/zstack/header/vm/APICleanupAllVmInstanceMetadataEvent.javaheader/src/main/java/org/zstack/header/vm/APICleanupAllVmInstanceMetadataEventDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/vm/APICleanupAllVmInstanceMetadataMsg.javaheader/src/main/java/org/zstack/header/vm/APICleanupAllVmInstanceMetadataMsgDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/vm/metadata/UpdateVmInstanceMetadataOnPrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/vm/metadata/VmMetadataCleanupBarrierState.javaheader/src/main/java/org/zstack/header/vm/metadata/VmMetadataCleanupBarrierVO.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageBase.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageHypervisorBackend.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.javaplugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorage.javaplugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageBackend.javaplugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageKVMBackend.javaplugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageKVMBackendCommands.javasdk/src/main/java/org/zstack/sdk/CleanupAllVmInstanceMetadataAction.javasdk/src/main/java/org/zstack/sdk/CleanupAllVmInstanceMetadataResult.javastorage/src/main/java/org/zstack/storage/primary/PrimaryStorageBase.javatestlib/src/main/java/org/zstack/testlib/ApiHelper.groovy
ab895a2 to
243e5c2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@header/src/main/java/org/zstack/header/vm/metadata/VmMetadataCleanupBarrierVO.java`:
- Around line 41-45: Update the lastOpDate mapping in VmMetadataCleanupBarrierVO
so updates preserve the database’s ON UPDATE CURRENT_TIMESTAMP behavior: either
add a `@PreUpdate` handler that clears lastOpDate before persistence, following
other lastOpDate VOs, or configure the field as database-generated using the
project’s established mapping approach. Leave createDate unchanged.
In
`@plugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageBackend.java`:
- Line 104: 为 NfsPrimaryStorageBackend 中新增的
handle(CleanupAllVmMetadataOnPrimaryStorageMsg, String, ReturnValueCompletion)
接口方法补充 Javadoc,明确说明清理所有虚拟机元数据的范围、hostUuid 用于指定相关主机,以及 completion
在操作成功或失败时分别如何回调。
🪄 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: 0467d3e3-d234-41f0-b10a-d712c9c3fb82
⛔ Files ignored due to path filters (2)
conf/persistence.xmlis excluded by!**/*.xmlconf/serviceConfig/vmInstance.xmlis excluded by!**/*.xml
📒 Files selected for processing (21)
conf/db/zsv/V5.1.0__schema.sqlheader/src/main/java/org/zstack/header/storage/primary/CleanupAllVmMetadataOnPrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/storage/primary/CleanupAllVmMetadataOnPrimaryStorageReply.javaheader/src/main/java/org/zstack/header/vm/APICleanupAllVmInstanceMetadataEvent.javaheader/src/main/java/org/zstack/header/vm/APICleanupAllVmInstanceMetadataEventDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/vm/APICleanupAllVmInstanceMetadataMsg.javaheader/src/main/java/org/zstack/header/vm/APICleanupAllVmInstanceMetadataMsgDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/vm/metadata/UpdateVmInstanceMetadataOnPrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/vm/metadata/VmMetadataCleanupBarrierState.javaheader/src/main/java/org/zstack/header/vm/metadata/VmMetadataCleanupBarrierVO.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageBase.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageHypervisorBackend.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.javaplugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorage.javaplugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageBackend.javaplugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageKVMBackend.javaplugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageKVMBackendCommands.javasdk/src/main/java/org/zstack/sdk/CleanupAllVmInstanceMetadataAction.javasdk/src/main/java/org/zstack/sdk/CleanupAllVmInstanceMetadataResult.javastorage/src/main/java/org/zstack/storage/primary/PrimaryStorageBase.javatestlib/src/main/java/org/zstack/testlib/ApiHelper.groovy
🚧 Files skipped from review as they are similar to previous changes (19)
- sdk/src/main/java/org/zstack/sdk/CleanupAllVmInstanceMetadataResult.java
- header/src/main/java/org/zstack/header/storage/primary/CleanupAllVmMetadataOnPrimaryStorageReply.java
- header/src/main/java/org/zstack/header/vm/metadata/VmMetadataCleanupBarrierState.java
- header/src/main/java/org/zstack/header/vm/APICleanupAllVmInstanceMetadataMsgDoc_zh_cn.groovy
- header/src/main/java/org/zstack/header/storage/primary/CleanupAllVmMetadataOnPrimaryStorageMsg.java
- header/src/main/java/org/zstack/header/vm/APICleanupAllVmInstanceMetadataMsg.java
- header/src/main/java/org/zstack/header/vm/APICleanupAllVmInstanceMetadataEvent.java
- header/src/main/java/org/zstack/header/vm/metadata/UpdateVmInstanceMetadataOnPrimaryStorageMsg.java
- header/src/main/java/org/zstack/header/vm/APICleanupAllVmInstanceMetadataEventDoc_zh_cn.groovy
- testlib/src/main/java/org/zstack/testlib/ApiHelper.groovy
- sdk/src/main/java/org/zstack/sdk/CleanupAllVmInstanceMetadataAction.java
- conf/db/zsv/V5.1.0__schema.sql
- plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.java
- plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageHypervisorBackend.java
- storage/src/main/java/org/zstack/storage/primary/PrimaryStorageBase.java
- plugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageKVMBackendCommands.java
- plugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageKVMBackend.java
- plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageBase.java
- plugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorage.java
|
|
||
| void handle(CleanupVmInstanceMetadataOnPrimaryStorageMsg msg, String hostUuid, ReturnValueCompletion<CleanupVmInstanceMetadataOnPrimaryStorageReply> completion); | ||
|
|
||
| void handle(CleanupAllVmMetadataOnPrimaryStorageMsg msg, String hostUuid, ReturnValueCompletion<CleanupAllVmMetadataOnPrimaryStorageReply> completion); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
为新增接口方法补充有效的 Javadoc。
Line 104 新增了主存储清理契约,但缺少接口文档。请说明清理范围、hostUuid 的用途,以及 completion 的成功和失败回调语义,避免不同后端实现产生不一致理解。
建议补充的 Javadoc
+ /**
+ * 在指定主机上清理全部 VM 元数据。
+ *
+ * `@param` msg 清理请求,包含主存储和元数据 generation
+ * `@param` hostUuid 执行清理的主机 UUID
+ * `@param` completion 清理成功或失败后的回调
+ */
void handle(CleanupAllVmMetadataOnPrimaryStorageMsg msg, String hostUuid,
ReturnValueCompletion<CleanupAllVmMetadataOnPrimaryStorageReply> completion);📝 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.
| void handle(CleanupAllVmMetadataOnPrimaryStorageMsg msg, String hostUuid, ReturnValueCompletion<CleanupAllVmMetadataOnPrimaryStorageReply> completion); | |
| /** | |
| * 在指定主机上清理全部 VM 元数据。 | |
| * | |
| * `@param` msg 清理请求,包含主存储和元数据 generation | |
| * `@param` hostUuid 执行清理的主机 UUID | |
| * `@param` completion 清理成功或失败后的回调 | |
| */ | |
| void handle(CleanupAllVmMetadataOnPrimaryStorageMsg msg, String hostUuid, | |
| ReturnValueCompletion<CleanupAllVmMetadataOnPrimaryStorageReply> completion); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@plugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageBackend.java`
at line 104, 为 NfsPrimaryStorageBackend 中新增的
handle(CleanupAllVmMetadataOnPrimaryStorageMsg, String, ReturnValueCompletion)
接口方法补充 Javadoc,明确说明清理所有虚拟机元数据的范围、hostUuid 用于指定相关主机,以及 completion
在操作成功或失败时分别如何回调。
Source: Path instructions
88ef86d to
cb73e57
Compare
APIImpact Resolves: ZSV-11867 Change-Id: I6c7767706a706e72756b7964646877676c626767
cb73e57 to
412e8bf
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
header/src/main/java/org/zstack/header/vm/metadata/VmMetadataCleanupBarrierVO.java (1)
42-45:⚠️ Potential issue | 🟠 Major不要在
@PreUpdate中把lastOpDate设为null。当前回调会让 ORM 在更新时显式写入
NULL。如果数据库字段是NOT NULL TIMESTAMP并依赖ON UPDATE CURRENT_TIMESTAMP,在启用explicit_defaults_for_timestamp的 MySQL 配置下可能直接更新失败。请移除该回调,或改用项目既有的数据库生成字段映射方式。该问题与历史审查意见重复,当前代码仍未修复。
建议修复
- `@PreUpdate` - private void preUpdate() { - lastOpDate = null; - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@header/src/main/java/org/zstack/header/vm/metadata/VmMetadataCleanupBarrierVO.java` around lines 42 - 45, Remove the `@PreUpdate` preUpdate() callback in VmMetadataCleanupBarrierVO that assigns null to lastOpDate; preserve lastOpDate for the database’s automatic ON UPDATE timestamp behavior and do not introduce explicit NULL writes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@header/src/main/java/org/zstack/header/vm/metadata/VmMetadataCleanupBarrierVO.java`:
- Around line 42-45: Remove the `@PreUpdate` preUpdate() callback in
VmMetadataCleanupBarrierVO that assigns null to lastOpDate; preserve lastOpDate
for the database’s automatic ON UPDATE timestamp behavior and do not introduce
explicit NULL writes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 68a3f115-3af9-4275-93eb-d06df34449b5
⛔ Files ignored due to path filters (2)
conf/persistence.xmlis excluded by!**/*.xmlconf/serviceConfig/vmInstance.xmlis excluded by!**/*.xml
📒 Files selected for processing (22)
conf/db/zsv/V5.1.0__schema.sqlheader/src/main/java/org/zstack/header/storage/primary/CleanupAllVmMetadataOnPrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/storage/primary/CleanupAllVmMetadataOnPrimaryStorageReply.javaheader/src/main/java/org/zstack/header/vm/APICleanupAllVmInstanceMetadataEvent.javaheader/src/main/java/org/zstack/header/vm/APICleanupAllVmInstanceMetadataEventDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/vm/APICleanupAllVmInstanceMetadataMsg.javaheader/src/main/java/org/zstack/header/vm/APICleanupAllVmInstanceMetadataMsgDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/vm/metadata/UpdateVmInstanceMetadataOnPrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/vm/metadata/VmMetadataCleanupBarrierState.javaheader/src/main/java/org/zstack/header/vm/metadata/VmMetadataCleanupBarrierVO.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageBase.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageHypervisorBackend.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.javaplugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorage.javaplugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageBackend.javaplugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageKVMBackend.javaplugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageKVMBackendCommands.javasdk/src/main/java/org/zstack/sdk/CleanupAllVmInstanceMetadataAction.javasdk/src/main/java/org/zstack/sdk/CleanupAllVmInstanceMetadataResult.javastorage/src/main/java/org/zstack/storage/primary/PrimaryStorageBase.javatestlib/src/main/java/org/zstack/testlib/ApiHelper.groovytestlib/src/main/java/org/zstack/testlib/VmMetadataCleanupBarrierDBRemaining.groovy
🚧 Files skipped from review as they are similar to previous changes (18)
- sdk/src/main/java/org/zstack/sdk/CleanupAllVmInstanceMetadataResult.java
- header/src/main/java/org/zstack/header/vm/APICleanupAllVmInstanceMetadataEventDoc_zh_cn.groovy
- header/src/main/java/org/zstack/header/vm/metadata/VmMetadataCleanupBarrierState.java
- header/src/main/java/org/zstack/header/storage/primary/CleanupAllVmMetadataOnPrimaryStorageMsg.java
- header/src/main/java/org/zstack/header/vm/APICleanupAllVmInstanceMetadataMsg.java
- header/src/main/java/org/zstack/header/vm/APICleanupAllVmInstanceMetadataMsgDoc_zh_cn.groovy
- plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageHypervisorBackend.java
- header/src/main/java/org/zstack/header/vm/APICleanupAllVmInstanceMetadataEvent.java
- testlib/src/main/java/org/zstack/testlib/ApiHelper.groovy
- plugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageKVMBackendCommands.java
- plugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorage.java
- header/src/main/java/org/zstack/header/vm/metadata/UpdateVmInstanceMetadataOnPrimaryStorageMsg.java
- plugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageKVMBackend.java
- sdk/src/main/java/org/zstack/sdk/CleanupAllVmInstanceMetadataAction.java
- conf/db/zsv/V5.1.0__schema.sql
- plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.java
- plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageBase.java
- plugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageBackend.java
sync from gitlab !10563