<feature>[storage]: support cleanAllVmMetadata param#3795
Closed
MatheMatrix wants to merge 1 commit into
Closed
Conversation
|
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:
概述在 变更
代码审查工作量🎯 1 (Trivial) | ⏱️ ~3 分钟 诗歌
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
MatheMatrix
force-pushed
the
sync/tao.gan/clean-vm-metadata-ZSV-11867@@3
branch
from
f662f09 to
1f54630
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
header/src/main/java/org/zstack/header/vm/APICleanupVmInstanceMetadataMsg.java (1)
17-37:⚠️ Potential issue | 🔴 CriticalAPICleanupVmInstanceMetadataMsg 未被注册到处理器，将被视为未知消息。
在 VmInstanceManagerImpl.handleApiMessage() 方法中，apiCleanupVmInstanceMetadataMsg 没有对应的 instanceof 检查路由。该消息最终会调用 bus.dealWithUnknownMessage() 被拒绝，整个 API 不可用。因此，原计划的两个参数之间的互斥性校验（vmUuids 和 cleanAllVmMetadata）永远不会执行。需要在 handleApiMessage() 中添加 APICleanupVmInstanceMetadataMsg 的处理分支，并在 VmInstanceApiInterceptor 中实现参数校验，确保至少指定一个清理目标。
🤖 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/APICleanupVmInstanceMetadataMsg.java` around lines 17 - 37, The APICleanupVmInstanceMetadataMsg is never handled because VmInstanceManagerImpl.handleApiMessage() lacks an instanceof branch for APICleanupVmInstanceMetadataMsg; add a branch that routes this message to the existing VM metadata cleanup handler (or a new private handler method) so it is not sent to bus.dealWithUnknownMessage(). Also implement parameter validation in VmInstanceApiInterceptor (referencing APICleanupVmInstanceMetadataMsg, vmUuids, cleanAllVmMetadata) to enforce that the request must specify at least one target and that vmUuids and cleanAllVmMetadata are not both provided (i.e., either cleanAllVmMetadata==true XOR vmUuids is non-empty), returning a proper API error if the check fails.
🧹 Nitpick comments (1)
header/src/main/java/org/zstack/header/vm/APICleanupVmInstanceMetadataEvent.java (1)
46-54: 建议同步更新__example__，避免示例与返回结构脱节。新增了
failedPrimaryStorageUuids后，示例返回建议一并体现，便于 API 文档消费方理解完整字段集。可选修改示例
public static APICleanupVmInstanceMetadataEvent __example__() { APICleanupVmInstanceMetadataEvent evt = new APICleanupVmInstanceMetadataEvent(); evt.totalCleaned = 5; evt.totalFailed = 0; evt.failedVmUuids = java.util.Collections.emptyList(); + evt.failedPrimaryStorageUuids = java.util.Collections.emptyList(); return evt; }🤖 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/APICleanupVmInstanceMetadataEvent.java` around lines 46 - 54, The API response example needs to be updated to include the new field added to APICleanupVmInstanceMetadataEvent: failedPrimaryStorageUuids (with its getter/setter present on the class), so update the "__example__" sample response to show this field (e.g., as an array of UUID strings or an empty array) alongside the existing fields so documentation consumers see the complete return structure; ensure the example key name exactly matches failedPrimaryStorageUuids and the value type matches List<String>.
🤖 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/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageBase.java`:
- Around line 3564-3567: The cleanAll branch currently returns early and
bypasses the existing per-primary-storage serialization used by thdf.chainSubmit
and the metadata update path (so cleanAll can race with per-VM updates);
instead, wrap the clean-all work in the same per-PS chain: submit a chain task
via thdf.chainSubmit using the same serialization key you use for single-VM
cleanup/metadata updates, and inside that chain task call
handleCleanAllVmMetadataOnLocalStorage (performing any host-level fan-out from
within the chain). This ensures cleanAllVmMetadata follows the same serialized
boundary as the metadata update code and single-VM cleanup.
- Around line 3638-3650: The cleanAll logic in LocalStorageBase (method cleanAll
/ helper cleanAllVmMetadata) only queries hosts with HostStatus.Connected, which
allows cleanAll to report success while offline hosts remain uncleaned; change
the host enumeration to fetch all host UUIDs mounted to this primary storage
(remove the "and host.status = :hstatus" filter or query HostVO without status),
then detect hosts whose HostStatus != Connected and mark the operation as
partial failure or return an error (e.g., set reply to failed/partial with
cleanedCount for connected hosts and include the offline host UUIDs/count);
update the analogous block around the cleanAllVmMetadata call (the code at the
other occurrence) the same way so offline hosts are counted and reported rather
than ignored.
---
Outside diff comments:
In
`@header/src/main/java/org/zstack/header/vm/APICleanupVmInstanceMetadataMsg.java`:
- Around line 17-37: The APICleanupVmInstanceMetadataMsg is never handled
because VmInstanceManagerImpl.handleApiMessage() lacks an instanceof branch for
APICleanupVmInstanceMetadataMsg; add a branch that routes this message to the
existing VM metadata cleanup handler (or a new private handler method) so it is
not sent to bus.dealWithUnknownMessage(). Also implement parameter validation in
VmInstanceApiInterceptor (referencing APICleanupVmInstanceMetadataMsg, vmUuids,
cleanAllVmMetadata) to enforce that the request must specify at least one target
and that vmUuids and cleanAllVmMetadata are not both provided (i.e., either
cleanAllVmMetadata==true XOR vmUuids is non-empty), returning a proper API error
if the check fails.
---
Nitpick comments:
In
`@header/src/main/java/org/zstack/header/vm/APICleanupVmInstanceMetadataEvent.java`:
- Around line 46-54: The API response example needs to be updated to include the
new field added to APICleanupVmInstanceMetadataEvent: failedPrimaryStorageUuids
(with its getter/setter present on the class), so update the "__example__"
sample response to show this field (e.g., as an array of UUID strings or an
empty array) alongside the existing fields so documentation consumers see the
complete return structure; ensure the example key name exactly matches
failedPrimaryStorageUuids and the value type matches List<String>.
🪄 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: 69163841-9532-43bb-ac8f-d14b683bdd28
📒 Files selected for processing (10)
header/src/main/java/org/zstack/header/storage/primary/CleanupVmInstanceMetadataOnPrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/storage/primary/CleanupVmInstanceMetadataOnPrimaryStorageReply.javaheader/src/main/java/org/zstack/header/vm/APICleanupVmInstanceMetadataEvent.javaheader/src/main/java/org/zstack/header/vm/APICleanupVmInstanceMetadataMsg.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageBase.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.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/CleanupVmInstanceMetadataAction.javasdk/src/main/java/org/zstack/sdk/CleanupVmInstanceMetadataResult.java
Comment on lines
+3564
to
+3567
| if (msg.isCleanAllVmMetadata()) { | ||
| handleCleanAllVmMetadataOnLocalStorage(msg); | ||
| return; | ||
| } |
There was a problem hiding this comment.
cleanAll 分支绕过了现有的元数据串行化队列。
这里直接 return 到新的全量清理逻辑，导致它不再经过下面单 VM cleanup 使用的 thdf.chainSubmit(...)，也和 Line 3368 的 metadata update 路径失去了同一个串行化边界。这样全量清理可以和更新/单 VM 清理并发修改同一套 metadata，结果会依赖时序。建议把 cleanAllVmMetadata=true 也放进同一个 per-PS sync chain，再在 chain 内部做 host 级 fan-out。
🤖 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 3564 - 3567, The cleanAll branch currently returns early and
bypasses the existing per-primary-storage serialization used by thdf.chainSubmit
and the metadata update path (so cleanAll can race with per-VM updates);
instead, wrap the clean-all work in the same per-PS chain: submit a chain task
via thdf.chainSubmit using the same serialization key you use for single-VM
cleanup/metadata updates, and inside that chain task call
handleCleanAllVmMetadataOnLocalStorage (performing any host-level fan-out from
within the chain). This ensures cleanAllVmMetadata follows the same serialized
boundary as the metadata update code and single-VM cleanup.
Comment on lines
+3638
to
+3650
| List<String> connectedHostUuids = SQL.New( | ||
| "select h.hostUuid from LocalStorageHostRefVO h, HostVO host" + | ||
| " where h.primaryStorageUuid = :psUuid" + | ||
| " and h.hostUuid = host.uuid" + | ||
| " and host.status = :hstatus", String.class) | ||
| .param("psUuid", self.getUuid()) | ||
| .param("hstatus", HostStatus.Connected) | ||
| .list(); | ||
| if (connectedHostUuids.isEmpty()) { | ||
| logger.warn(String.format("[MetadataCleanup] cleanAll: no connected host found for local ps[uuid:%s]", self.getUuid())); | ||
| reply.setCleanedCount(0); | ||
| bus.reply(msg, reply); | ||
| return; |
There was a problem hiding this comment.
cleanAll 现在会把“未清理离线宿主机”误报成成功。
这里仅查询 HostStatus.Connected 的宿主机；只要有一台在线宿主机成功，最终就会返回成功并带 cleanedCount。但挂载在该 local ps 上的离线宿主机上的 metadata 根本没有机会被清理，cleanAllVmMetadata 的语义就不成立了。建议像本类的扫描逻辑一样先枚举全部宿主机，把离线宿主机计入失败/部分失败，或者直接在存在离线宿主机时返回错误。
可参考的修正方向
- List<String> connectedHostUuids = SQL.New(
+ List<String> allHostUuids = SQL.New(
+ "select h.hostUuid from LocalStorageHostRefVO h, HostVO host" +
+ " where h.primaryStorageUuid = :psUuid" +
+ " and h.hostUuid = host.uuid", String.class)
+ .param("psUuid", self.getUuid())
+ .list();
+
+ List<String> connectedHostUuids = SQL.New(
"select h.hostUuid from LocalStorageHostRefVO h, HostVO host" +
" where h.primaryStorageUuid = :psUuid" +
" and h.hostUuid = host.uuid" +
" and host.status = :hstatus", String.class)
.param("psUuid", self.getUuid())
.param("hstatus", HostStatus.Connected)
.list();
- if (connectedHostUuids.isEmpty()) {
- logger.warn(String.format("[MetadataCleanup] cleanAll: no connected host found for local ps[uuid:%s]", self.getUuid()));
- reply.setCleanedCount(0);
+
+ List<String> disconnectedHostUuids = new ArrayList<>(allHostUuids);
+ disconnectedHostUuids.removeAll(connectedHostUuids);
+ if (!disconnectedHostUuids.isEmpty()) {
+ reply.setError(operr("cannot clean all vm metadata on local primary storage[uuid:%s], disconnected hosts=%s",
+ self.getUuid(), disconnectedHostUuids));
bus.reply(msg, reply);
return;
}Also applies to: 3684-3690
🤖 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 3638 - 3650, The cleanAll logic in LocalStorageBase (method
cleanAll / helper cleanAllVmMetadata) only queries hosts with
HostStatus.Connected, which allows cleanAll to report success while offline
hosts remain uncleaned; change the host enumeration to fetch all host UUIDs
mounted to this primary storage (remove the "and host.status = :hstatus" filter
or query HostVO without status), then detect hosts whose HostStatus != Connected
and mark the operation as partial failure or return an error (e.g., set reply to
failed/partial with cleanedCount for connected hosts and include the offline
host UUIDs/count); update the analogous block around the cleanAllVmMetadata call
(the code at the other occurrence) the same way so offline hosts are counted and
reported rather than ignored.
1f54630 to
87fa596
Compare
Collaborator
|
Comment from yaohua.wu: Review: MR !9664 — ZSV-11867关联 MR：
本 MR 处于"承上启下"的位置：把 🔴 Critical1.
而 zstack-utility !6954 的 if getattr(cmd, 'cleanAllVmMetadata', False):
return self._do_cleanup_all(getattr(cmd, 'metadataDir', None))JSON 反序列化后 修复方案（任一）：
强烈建议加一个 mock 集成测试，覆盖 "cleanAllVmMetadata=true → agent 端确实拿到了非空 metadataDir → 走到 rmtree/lv 删除路径"。 2.
List<String> connectedHostUuids = SQL.New(
"select h.hostUuid from LocalStorageHostRefVO h, HostVO host" +
" where h.primaryStorageUuid = :psUuid" +
" and h.hostUuid = host.uuid" +
" and host.status = :hstatus", String.class)
.param("hstatus", HostStatus.Connected)
.list();如果有挂载该 PS 的主机当前是 Disconnected（这是 local storage 集群里很常见的场景），cleanAll 直接跳过这些主机，但 reply 仍然返回成功并附 修复：枚举该 ps 上所有 host（去掉 3.
protected void handle(final CleanupVmInstanceMetadataOnPrimaryStorageMsg msg) {
if (msg.isCleanAllVmMetadata()) {
handleCleanAllVmMetadataOnLocalStorage(msg);
return;
}
thdf.chainSubmit(new ChainTask(msg) { ... });
}单 VM cleanup、metadata 写/更新都在 🟡 Warning4. API event 新增 5. 新增字段建议同步到 evt.failedPrimaryStorageUuids = java.util.Collections.emptyList();6. NFS / SharedBlock 在 cleanAll 下没有 host 级 fan-out 设计文档说明。 NFS 和 SharedBlock 是共享存储，理论上任选一台 connected 主机做一次 rmtree/LV cleanup 就能覆盖整个 PS —— 但本 MR 没有显式把"只挑一台主机"的语义落到代码注释里。后续维护者改成 fan-out 是错误的也找不到约束依据。建议在 🟢 Suggestion
🤖 Robot Reviewer |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageBase.java (2)
3564-3567:⚠️ Potential issue | 🟠 Major
cleanAll分支仍然绕过了主存级别的 metadata 串行化。这里直接
return到全量清理逻辑，不会进入现有的 sync chain；但全量清理会并行下发到多个 host，和 Line 3368 的 metadata 更新路径并发执行时，可能同时修改同一套 metadata，结果会依赖时序。建议先进入同一个 per-PSthdf.chainSubmit(...)，再在 chain 内部做 host 级 fan-out。🤖 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 3564 - 3567, The clean-all VM metadata branch bypasses the primary-storage-level serialization; instead of directly calling handleCleanAllVmMetadataOnLocalStorage and returning, submit a task into the same per-primary-storage sync chain (use thdf.chainSubmit with the primary storage uuid key) and then perform the host-level fan-out inside that chain; modify LocalStorageBase so that when msg.isCleanAllVmMetadata() is true it invokes thdf.chainSubmit(...) and inside the chain body calls handleCleanAllVmMetadataOnLocalStorage to ensure the cleanAll operation is serialized with other metadata updates (the same chain used by the metadata update path referenced elsewhere), preventing concurrent modifications across hosts.
3638-3690:⚠️ Potential issue | 🟠 Major
cleanAllVmMetadata仍会把未执行完成的宿主机误报为成功。这里只枚举
HostStatus.Connected的宿主机，而且只有“所有在线宿主机都失败”才会置error。结果是：有离线宿主机时仍可能返回成功；即使部分在线宿主机执行失败，也只会返回cleanedCount，调用方无法知道这次全量清理并未真正覆盖整个 local ps。建议先枚举该 primary storage 上的全部宿主机，并把离线/执行失败的 host 明确作为失败或部分失败回传，而不是仅在全失败时才报错。🤖 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 3638 - 3690, The code only queries HostStatus.Connected and treats the operation as success unless every connected host failed, causing offline or partially-failed hosts to be reported as successful; change the logic in LocalStorageBase (the cleanAll/cleanAllVmMetadata flow that builds connectedHostUuids, iterates using While<>, calls getHypervisorBackendFactoryByHostUuid and LocalStorageHypervisorBackend.handle, and populates reply/CleanupVmInstanceMetadataOnPrimaryStorageReply) to enumerate all hosts referenced by the primary storage (not just HostStatus.Connected), and explicitly treat non-Connected hosts as failures by adding an ErrorCode to the While's ErrorCodeList (e.g., com.addError(operr(...)) for offline hosts) before continuing; preserve existing handling of runtime failures in the LocalStorageHypervisorBackend.handle callbacks but ensure those failures are counted, and finally change the completion logic so that if any host produced an error (offline or handler fail) the reply indicates an error/partial-failure (include the failed host UUIDs or error list) rather than returning only cleanedCount; keep totalCleaned accumulation unchanged for successful hosts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageBase.java`:
- Around line 3564-3567: The clean-all VM metadata branch bypasses the
primary-storage-level serialization; instead of directly calling
handleCleanAllVmMetadataOnLocalStorage and returning, submit a task into the
same per-primary-storage sync chain (use thdf.chainSubmit with the primary
storage uuid key) and then perform the host-level fan-out inside that chain;
modify LocalStorageBase so that when msg.isCleanAllVmMetadata() is true it
invokes thdf.chainSubmit(...) and inside the chain body calls
handleCleanAllVmMetadataOnLocalStorage to ensure the cleanAll operation is
serialized with other metadata updates (the same chain used by the metadata
update path referenced elsewhere), preventing concurrent modifications across
hosts.
- Around line 3638-3690: The code only queries HostStatus.Connected and treats
the operation as success unless every connected host failed, causing offline or
partially-failed hosts to be reported as successful; change the logic in
LocalStorageBase (the cleanAll/cleanAllVmMetadata flow that builds
connectedHostUuids, iterates using While<>, calls
getHypervisorBackendFactoryByHostUuid and LocalStorageHypervisorBackend.handle,
and populates reply/CleanupVmInstanceMetadataOnPrimaryStorageReply) to enumerate
all hosts referenced by the primary storage (not just HostStatus.Connected), and
explicitly treat non-Connected hosts as failures by adding an ErrorCode to the
While's ErrorCodeList (e.g., com.addError(operr(...)) for offline hosts) before
continuing; preserve existing handling of runtime failures in the
LocalStorageHypervisorBackend.handle callbacks but ensure those failures are
counted, and finally change the completion logic so that if any host produced an
error (offline or handler fail) the reply indicates an error/partial-failure
(include the failed host UUIDs or error list) rather than returning only
cleanedCount; keep totalCleaned accumulation unchanged for successful hosts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 89c1a9ec-9cf6-46c7-a9ef-255a811279c6
📒 Files selected for processing (10)
header/src/main/java/org/zstack/header/storage/primary/CleanupVmInstanceMetadataOnPrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/storage/primary/CleanupVmInstanceMetadataOnPrimaryStorageReply.javaheader/src/main/java/org/zstack/header/vm/APICleanupVmInstanceMetadataEvent.javaheader/src/main/java/org/zstack/header/vm/APICleanupVmInstanceMetadataMsg.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageBase.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.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/CleanupVmInstanceMetadataAction.javasdk/src/main/java/org/zstack/sdk/CleanupVmInstanceMetadataResult.java
✅ Files skipped from review due to trivial changes (3)
- header/src/main/java/org/zstack/header/vm/APICleanupVmInstanceMetadataEvent.java
- plugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageKVMBackendCommands.java
- header/src/main/java/org/zstack/header/storage/primary/CleanupVmInstanceMetadataOnPrimaryStorageMsg.java
🚧 Files skipped from review as they are similar to previous changes (2)
- header/src/main/java/org/zstack/header/storage/primary/CleanupVmInstanceMetadataOnPrimaryStorageReply.java
- sdk/src/main/java/org/zstack/sdk/CleanupVmInstanceMetadataResult.java
MatheMatrix
force-pushed
the
sync/tao.gan/clean-vm-metadata-ZSV-11867@@3
branch
2 times, most recently
from
e6ae9ff to
61e5e35
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageKVMBackend.java (1)
2173-2174: 建议在 Line 2173 做cleanAllVmMetadata场景的metadataDir非空校验。当前是直接透传；若上游漏传
metadataDir，全量清理可能再次退化为“成功但未清理”。建议在这里快速失败，避免静默行为。可参考的最小改动
cmd.cleanAllVmMetadata = msg.isCleanAllVmMetadata(); cmd.metadataDir = msg.getMetadataDir(); + if (cmd.cleanAllVmMetadata && StringUtils.isBlank(cmd.metadataDir)) { + completion.fail(operr("failed to cleanup vm metadata on nfs via host[uuid:%s]: metadataDir is required when cleanAllVmMetadata is true", hostUuid)); + return; + }🤖 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 2173 - 2174, If msg.isCleanAllVmMetadata() is true, validate that msg.getMetadataDir() is non-null/non-empty before assigning to cmd.metadataDir; if it's missing, fail fast (throw IllegalArgumentException or set an error/response and return) instead of proceeding. Update the logic around cmd.cleanAllVmMetadata and cmd.metadataDir to check msg.getMetadataDir(), and produce a clear error/response when the directory is absent so the clean-all path cannot silently succeed without metadataDir.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@header/src/main/java/org/zstack/header/vm/APICleanupAllVmInstanceMetadataMsg.java`:
- Around line 10-13: The REST annotation for APICleanupAllVmInstanceMetadataMsg
uses HttpMethod.PUT but the operation is a delete/cleanup; change the
`@RestRequest` method from HttpMethod.PUT to HttpMethod.DELETE in the
APICleanupAllVmInstanceMetadataMsg declaration (path
"/vm-instances/metadata/cleanup-all") and also update any getRestInfo()
implementation or other places that return HttpMethod.PUT for this API to return
HttpMethod.DELETE so the API metadata and SDK callers remain consistent.
---
Nitpick comments:
In
`@plugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageKVMBackend.java`:
- Around line 2173-2174: If msg.isCleanAllVmMetadata() is true, validate that
msg.getMetadataDir() is non-null/non-empty before assigning to cmd.metadataDir;
if it's missing, fail fast (throw IllegalArgumentException or set an
error/response and return) instead of proceeding. Update the logic around
cmd.cleanAllVmMetadata and cmd.metadataDir to check msg.getMetadataDir(), and
produce a clear error/response when the directory is absent so the clean-all
path cannot silently succeed without metadataDir.
🪄 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: 859b4259-ec7b-4c38-84fe-698e1bb298c0
⛔ Files ignored due to path filters (1)
conf/serviceConfig/vmInstance.xmlis excluded by!**/*.xml
📒 Files selected for processing (10)
header/src/main/java/org/zstack/header/storage/primary/CleanupVmInstanceMetadataOnPrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/storage/primary/CleanupVmInstanceMetadataOnPrimaryStorageReply.javaheader/src/main/java/org/zstack/header/vm/APICleanupAllVmInstanceMetadataEvent.javaheader/src/main/java/org/zstack/header/vm/APICleanupAllVmInstanceMetadataMsg.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageBase.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.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.java
✅ Files skipped from review due to trivial changes (4)
- header/src/main/java/org/zstack/header/storage/primary/CleanupVmInstanceMetadataOnPrimaryStorageReply.java
- sdk/src/main/java/org/zstack/sdk/CleanupAllVmInstanceMetadataResult.java
- header/src/main/java/org/zstack/header/storage/primary/CleanupVmInstanceMetadataOnPrimaryStorageMsg.java
- header/src/main/java/org/zstack/header/vm/APICleanupAllVmInstanceMetadataEvent.java
MatheMatrix
force-pushed
the
sync/tao.gan/clean-vm-metadata-ZSV-11867@@3
branch
from
61e5e35 to
7133b18
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageBase.java (2)
3564-3567:⚠️ Potential issue | 🟠 Major
cleanAll仍然绕过了元数据串行化队列。这里直接返回到
handleCleanAllVmMetadataOnLocalStorage()，没有进入thdf.chainSubmit(...)。UpdateVmInstanceMetadataOnPrimaryStorageMsg在同类里是按 primary storage 串行化处理的，所以全量清理现在仍然可能和元数据写入并发，结果依赖时序。建议把 clean-all 也放进同一个 per-PS sync chain，再在 chain 内做 host fan-out。🤖 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 3564 - 3567, The clean-all VM metadata path currently bypasses the per-primary-storage serialization chain by calling handleCleanAllVmMetadataOnLocalStorage(msg) directly; change LocalStorageBase so that when msg.isCleanAllVmMetadata() is true you submit the work through the same thdf.chainSubmit(...) path used for UpdateVmInstanceMetadataOnPrimaryStorageMsg (i.e., serialize on the per-PS key) and inside that submitted chain invoke handleCleanAllVmMetadataOnLocalStorage to perform the host fan-out; this ensures cleanAll runs serialized with metadata writes for the same primary storage.
3638-3690:⚠️ Potential issue | 🟠 Major
cleanAll仍会把离线宿主机遗漏成“成功”。这里还是只查询
HostStatus.Connected的宿主机；只要在线宿主机清理完成，就会返回cleanedCount，但离线宿主机上的 metadata 根本没有机会被清理。和本文件的 scan 路径不同，这里没有先枚举全部挂载宿主机并显式处理 disconnected hosts，因此cleanAllVmMetadata的语义仍然不成立。建议存在离线宿主机时直接失败，或至少把它们明确计入失败结果。🤖 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 3638 - 3690, The cleanAll path currently only queries HostStatus.Connected (connectedHostUuids) so disconnected hosts are never processed and are implicitly treated as success; change cleanAll to first enumerate all LocalStorageHostRefVO host UUIDs for this primary (e.g., query without host.status filter), compute the set difference vs connectedHostUuids, and for any disconnected hosts either (a) fail fast by setting reply.setError(...) and replying with those host UUIDs included, or (b) add explicit ErrorCode entries for each disconnected host into the final ErrorCodeList so they count as failures; ensure you still use getHypervisorBackendFactoryByHostUuid/getHypervisorBackend and LocalStorageHypervisorBackend for connected hosts and aggregate totalCleaned and error causes before bus.reply(msg, reply).
🧹 Nitpick comments (1)
plugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageKVMBackend.java (1)
2169-2197: 建议补一行注释说明 NFScleanAll的“单宿主执行”语义。NFS 是共享存储，这里的全量清理只需要在一个已连接宿主机上执行一次即可。当前实现行为是对的，但没有把这个约束写出来，后续维护时很容易被误改成像 LocalStorage 一样的多宿主 fan-out。建议在这段处理逻辑旁边补一条简短注释。
🤖 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 2169 - 2197, Add a short explanatory comment next to the handler for CleanupVmInstanceMetadataOnPrimaryStorageMsg (the handle method that builds CleanupVmMetadataCmd, KVMHostAsyncHttpCallMsg hmsg and sends to CLEANUP_VM_METADATA_PATH) stating that when cleanAllVmMetadata is true the cleanup must be executed only once on any single host because NFS is shared storage; this clarifies the “single-host execution” semantics and prevents future changes that might incorrectly fan-out the operation to multiple hosts as done for LocalStorage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageBase.java`:
- Around line 3564-3567: The clean-all VM metadata path currently bypasses the
per-primary-storage serialization chain by calling
handleCleanAllVmMetadataOnLocalStorage(msg) directly; change LocalStorageBase so
that when msg.isCleanAllVmMetadata() is true you submit the work through the
same thdf.chainSubmit(...) path used for
UpdateVmInstanceMetadataOnPrimaryStorageMsg (i.e., serialize on the per-PS key)
and inside that submitted chain invoke handleCleanAllVmMetadataOnLocalStorage to
perform the host fan-out; this ensures cleanAll runs serialized with metadata
writes for the same primary storage.
- Around line 3638-3690: The cleanAll path currently only queries
HostStatus.Connected (connectedHostUuids) so disconnected hosts are never
processed and are implicitly treated as success; change cleanAll to first
enumerate all LocalStorageHostRefVO host UUIDs for this primary (e.g., query
without host.status filter), compute the set difference vs connectedHostUuids,
and for any disconnected hosts either (a) fail fast by setting
reply.setError(...) and replying with those host UUIDs included, or (b) add
explicit ErrorCode entries for each disconnected host into the final
ErrorCodeList so they count as failures; ensure you still use
getHypervisorBackendFactoryByHostUuid/getHypervisorBackend and
LocalStorageHypervisorBackend for connected hosts and aggregate totalCleaned and
error causes before bus.reply(msg, reply).
---
Nitpick comments:
In
`@plugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageKVMBackend.java`:
- Around line 2169-2197: Add a short explanatory comment next to the handler for
CleanupVmInstanceMetadataOnPrimaryStorageMsg (the handle method that builds
CleanupVmMetadataCmd, KVMHostAsyncHttpCallMsg hmsg and sends to
CLEANUP_VM_METADATA_PATH) stating that when cleanAllVmMetadata is true the
cleanup must be executed only once on any single host because NFS is shared
storage; this clarifies the “single-host execution” semantics and prevents
future changes that might incorrectly fan-out the operation to multiple hosts as
done for LocalStorage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 437ccb33-a220-42ca-9f8c-3306235ca901
⛔ Files ignored due to path filters (1)
conf/serviceConfig/vmInstance.xmlis excluded by!**/*.xml
📒 Files selected for processing (10)
header/src/main/java/org/zstack/header/storage/primary/CleanupVmInstanceMetadataOnPrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/storage/primary/CleanupVmInstanceMetadataOnPrimaryStorageReply.javaheader/src/main/java/org/zstack/header/vm/APICleanupAllVmInstanceMetadataEvent.javaheader/src/main/java/org/zstack/header/vm/APICleanupAllVmInstanceMetadataMsg.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageBase.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.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.java
✅ Files skipped from review due to trivial changes (4)
- sdk/src/main/java/org/zstack/sdk/CleanupAllVmInstanceMetadataResult.java
- header/src/main/java/org/zstack/header/storage/primary/CleanupVmInstanceMetadataOnPrimaryStorageReply.java
- header/src/main/java/org/zstack/header/vm/APICleanupAllVmInstanceMetadataMsg.java
- header/src/main/java/org/zstack/header/storage/primary/CleanupVmInstanceMetadataOnPrimaryStorageMsg.java
🚧 Files skipped from review as they are similar to previous changes (1)
- header/src/main/java/org/zstack/header/vm/APICleanupAllVmInstanceMetadataEvent.java
MatheMatrix
force-pushed
the
sync/tao.gan/clean-vm-metadata-ZSV-11867@@3
branch
from
7133b18 to
5116114
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sdk/src/main/java/org/zstack/sdk/zmigrate/api/GetZMigrateInfosResult.java`:
- Around line 22-44: 在 GetZMigrateInfosResult 类中，不要移除原有的 public
字段和访问器名（platforms, gateways, migrateJobs）；恢复并保留这些旧字段与其
getPlatforms()/setPlatforms(...), getGateways()/setGateways(...),
getMigrateJobs()/setMigrateJobs(...)（标记为 `@Deprecated`），并让它们内部委托到新的
platformsCount, gatewaysCount, migrateJobsCount
字段和对应的访问器，以保证二进制/源码兼容性和序列化映射不破坏现有客户端。
🪄 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: 03134256-acd3-4c96-9ab8-28f5b0da15c1
📒 Files selected for processing (1)
sdk/src/main/java/org/zstack/sdk/zmigrate/api/GetZMigrateInfosResult.java
Comment on lines
+22
to
44
| public long platformsCount; | ||
| public void setPlatformsCount(long platformsCount) { | ||
| this.platformsCount = platformsCount; | ||
| } | ||
| public long getPlatforms() { | ||
| return this.platforms; | ||
| public long getPlatformsCount() { | ||
| return this.platformsCount; | ||
| } | ||
|
|
||
| public long gateways; | ||
| public void setGateways(long gateways) { | ||
| this.gateways = gateways; | ||
| public long gatewaysCount; | ||
| public void setGatewaysCount(long gatewaysCount) { | ||
| this.gatewaysCount = gatewaysCount; | ||
| } | ||
| public long getGateways() { | ||
| return this.gateways; | ||
| public long getGatewaysCount() { | ||
| return this.gatewaysCount; | ||
| } | ||
|
|
||
| public long migrateJobs; | ||
| public void setMigrateJobs(long migrateJobs) { | ||
| this.migrateJobs = migrateJobs; | ||
| public long migrateJobsCount; | ||
| public void setMigrateJobsCount(long migrateJobsCount) { | ||
| this.migrateJobsCount = migrateJobsCount; | ||
| } | ||
| public long getMigrateJobs() { | ||
| return this.migrateJobs; | ||
| public long getMigrateJobsCount() { | ||
| return this.migrateJobsCount; | ||
| } |
There was a problem hiding this comment.
这是一个 SDK 兼容性破坏变更，建议保留旧字段/访问器别名。
Line 22-44 把 platforms/gateways/migrateJobs 全量改名为 *Count，并删除旧 public 字段与 get/set，会直接导致已有客户端编译失败或字段映射不兼容。若不是明确的破坏性版本发布，建议至少保留旧访问器（@Deprecated）并委托到新字段，同时在发布说明声明迁移窗口。
兼容性修复示例
public class GetZMigrateInfosResult {
@@
public long platformsCount;
+ /** `@deprecated` use platformsCount */
+ `@Deprecated`
+ public long platforms;
@@
public void setPlatformsCount(long platformsCount) {
this.platformsCount = platformsCount;
+ this.platforms = platformsCount;
}
@@
public long getPlatformsCount() {
return this.platformsCount;
}
+ /** `@deprecated` use setPlatformsCount(long) */
+ `@Deprecated`
+ public void setPlatforms(long platforms) {
+ setPlatformsCount(platforms);
+ }
+ /** `@deprecated` use getPlatformsCount() */
+ `@Deprecated`
+ public long getPlatforms() {
+ return getPlatformsCount();
+ }
@@
public long gatewaysCount;
+ /** `@deprecated` use gatewaysCount */
+ `@Deprecated`
+ public long gateways;
@@
public void setGatewaysCount(long gatewaysCount) {
this.gatewaysCount = gatewaysCount;
+ this.gateways = gatewaysCount;
}
@@
public long getGatewaysCount() {
return this.gatewaysCount;
}
+ /** `@deprecated` use setGatewaysCount(long) */
+ `@Deprecated`
+ public void setGateways(long gateways) {
+ setGatewaysCount(gateways);
+ }
+ /** `@deprecated` use getGatewaysCount() */
+ `@Deprecated`
+ public long getGateways() {
+ return getGatewaysCount();
+ }
@@
public long migrateJobsCount;
+ /** `@deprecated` use migrateJobsCount */
+ `@Deprecated`
+ public long migrateJobs;
@@
public void setMigrateJobsCount(long migrateJobsCount) {
this.migrateJobsCount = migrateJobsCount;
+ this.migrateJobs = migrateJobsCount;
}
@@
public long getMigrateJobsCount() {
return this.migrateJobsCount;
}
+ /** `@deprecated` use setMigrateJobsCount(long) */
+ `@Deprecated`
+ public void setMigrateJobs(long migrateJobs) {
+ setMigrateJobsCount(migrateJobs);
+ }
+ /** `@deprecated` use getMigrateJobsCount() */
+ `@Deprecated`
+ public long getMigrateJobs() {
+ return getMigrateJobsCount();
+ }
}📝 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
| public long platformsCount; | |
| public void setPlatformsCount(long platformsCount) { | |
| this.platformsCount = platformsCount; | |
| } | |
| public long getPlatforms() { | |
| return this.platforms; | |
| public long getPlatformsCount() { | |
| return this.platformsCount; | |
| } | |
| public long gateways; | |
| public void setGateways(long gateways) { | |
| this.gateways = gateways; | |
| public long gatewaysCount; | |
| public void setGatewaysCount(long gatewaysCount) { | |
| this.gatewaysCount = gatewaysCount; | |
| } | |
| public long getGateways() { | |
| return this.gateways; | |
| public long getGatewaysCount() { | |
| return this.gatewaysCount; | |
| } | |
| public long migrateJobs; | |
| public void setMigrateJobs(long migrateJobs) { | |
| this.migrateJobs = migrateJobs; | |
| public long migrateJobsCount; | |
| public void setMigrateJobsCount(long migrateJobsCount) { | |
| this.migrateJobsCount = migrateJobsCount; | |
| } | |
| public long getMigrateJobs() { | |
| return this.migrateJobs; | |
| public long getMigrateJobsCount() { | |
| return this.migrateJobsCount; | |
| } | |
| public long platformsCount; | |
| /** `@deprecated` use platformsCount */ | |
| `@Deprecated` | |
| public long platforms; | |
| public void setPlatformsCount(long platformsCount) { | |
| this.platformsCount = platformsCount; | |
| this.platforms = platformsCount; | |
| } | |
| public long getPlatformsCount() { | |
| return this.platformsCount; | |
| } | |
| /** `@deprecated` use setPlatformsCount(long) */ | |
| `@Deprecated` | |
| public void setPlatforms(long platforms) { | |
| setPlatformsCount(platforms); | |
| } | |
| /** `@deprecated` use getPlatformsCount() */ | |
| `@Deprecated` | |
| public long getPlatforms() { | |
| return getPlatformsCount(); | |
| } | |
| public long gatewaysCount; | |
| /** `@deprecated` use gatewaysCount */ | |
| `@Deprecated` | |
| public long gateways; | |
| public void setGatewaysCount(long gatewaysCount) { | |
| this.gatewaysCount = gatewaysCount; | |
| this.gateways = gatewaysCount; | |
| } | |
| public long getGatewaysCount() { | |
| return this.gatewaysCount; | |
| } | |
| /** `@deprecated` use setGatewaysCount(long) */ | |
| `@Deprecated` | |
| public void setGateways(long gateways) { | |
| setGatewaysCount(gateways); | |
| } | |
| /** `@deprecated` use getGatewaysCount() */ | |
| `@Deprecated` | |
| public long getGateways() { | |
| return getGatewaysCount(); | |
| } | |
| public long migrateJobsCount; | |
| /** `@deprecated` use migrateJobsCount */ | |
| `@Deprecated` | |
| public long migrateJobs; | |
| public void setMigrateJobsCount(long migrateJobsCount) { | |
| this.migrateJobsCount = migrateJobsCount; | |
| this.migrateJobs = migrateJobsCount; | |
| } | |
| public long getMigrateJobsCount() { | |
| return this.migrateJobsCount; | |
| } | |
| /** `@deprecated` use setMigrateJobsCount(long) */ | |
| `@Deprecated` | |
| public void setMigrateJobs(long migrateJobs) { | |
| setMigrateJobsCount(migrateJobs); | |
| } | |
| /** `@deprecated` use getMigrateJobsCount() */ | |
| `@Deprecated` | |
| public long getMigrateJobs() { | |
| return getMigrateJobsCount(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/src/main/java/org/zstack/sdk/zmigrate/api/GetZMigrateInfosResult.java`
around lines 22 - 44, 在 GetZMigrateInfosResult 类中，不要移除原有的 public
字段和访问器名（platforms, gateways, migrateJobs）；恢复并保留这些旧字段与其
getPlatforms()/setPlatforms(...), getGateways()/setGateways(...),
getMigrateJobs()/setMigrateJobs(...)（标记为 `@Deprecated`），并让它们内部委托到新的
platformsCount, gatewaysCount, migrateJobsCount
字段和对应的访问器，以保证二进制/源码兼容性和序列化映射不破坏现有客户端。
71d73f1 to
b153517
Compare
Rename platforms/gateways/migrateJobs to platformsCount/gatewaysCount/migrateJobsCount to make field semantics explicit (these are counts, not lists of entities). Resolves: ZSV-12018 Change-Id: I66667361746274616d6879666a6479796f797174
MatheMatrix
force-pushed
the
sync/tao.gan/clean-vm-metadata-ZSV-11867@@3
branch
from
b153517 to
bdc91fb
Compare
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.
Resolves: ZSV-11867
Change-Id: I6e6b616875706f67706d726d617367637863717a
sync from gitlab !9664