<feature>[storage]: support cleanAllVmMetadata param#3795
Open
MatheMatrix wants to merge 1 commit intozsv_5.0.0from
Open
<feature>[storage]: support cleanAllVmMetadata param#3795MatheMatrix wants to merge 1 commit intozsv_5.0.0from
MatheMatrix wants to merge 1 commit intozsv_5.0.0from
Conversation
概览本次变更为虚拟机元数据清理功能添加了批量清理能力。引入了新的布尔标志 变更
时序图sequenceDiagram
participant Client as 客户端
participant API as API层
participant LocalStorage as 本地存储
participant Host as 连接的主机
participant Backend as KVM后端
Client->>API: 发送APICleanupVmInstanceMetadataMsg<br/>(cleanAllVmMetadata=true)
API->>LocalStorage: CleanupVmInstanceMetadataOnPrimaryStorageMsg
LocalStorage->>LocalStorage: 检测 cleanAllVmMetadata=true
LocalStorage->>LocalStorage: 查询所有连接的主机
LocalStorage->>Host: 遍历每个主机
Host->>Backend: 调用KVM后端处理
Backend->>Backend: 执行CleanupVmMetadataCmd<br/>(cleanAllVmMetadata=true)
Backend-->>Host: 返回CleanupVmMetadataRsp<br/>(cleanedCount)
Host-->>LocalStorage: 聚合结果
LocalStorage->>LocalStorage: 累积cleanedCount
LocalStorage-->>API: CleanupVmInstanceMetadataOnPrimaryStorageReply<br/>(cleanedCount)
API-->>Client: APICleanupVmInstanceMetadataEvent<br/>(totalCleaned, failedPrimaryStorageUuids)
代码审查工作量估计🎯 3 (中等) | ⏱️ ~20 分钟 诗
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Resolves: ZSV-11867 Change-Id: I6e6b616875706f67706d726d617367637863717a
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves: ZSV-11867
Change-Id: I6e6b616875706f67706d726d617367637863717a
sync from gitlab !9664