<feature>[storage]: support cleanAllVmMetadata param#3760
Open
MatheMatrix wants to merge 1 commit intofeature-zsv-5.0.0-vm-support-vtpm-and-secucebootfrom
Open
<feature>[storage]: support cleanAllVmMetadata param#3760MatheMatrix wants to merge 1 commit intofeature-zsv-5.0.0-vm-support-vtpm-and-secucebootfrom
MatheMatrix wants to merge 1 commit intofeature-zsv-5.0.0-vm-support-vtpm-and-secucebootfrom
Conversation
Resolves: ZSV-11867 Change-Id: I6e6b616875706f67706d726d617367637863717a
Walkthrough该PR扩展了VM实例元数据清理功能，添加了 Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant API
participant LocalStorageBase
participant HostResolver
participant KVMBackend
participant Agent
Client->>API: APICleanupVmInstanceMetadataMsg<br/>(cleanAllVmMetadata=true)
API->>LocalStorageBase: handle(CleanupVmInstanceMetadataMsg)
alt cleanAllVmMetadata is true
LocalStorageBase->>HostResolver: queryConnectedHosts()
HostResolver-->>LocalStorageBase: List of connected hosts
loop For each connected host
LocalStorageBase->>KVMBackend: handle(msg, hostUuid)
KVMBackend->>Agent: CleanupVmMetadataCmd<br/>(cleanAllVmMetadata=true)
Agent-->>KVMBackend: CleanupVmMetadataRsp<br/>(cleanedCount)
KVMBackend->>LocalStorageBase: increment cleanedCount
end
LocalStorageBase->>LocalStorageBase: aggregate results<br/>via AtomicInteger
else cleanAllVmMetadata is false
LocalStorageBase->>LocalStorageBase: proceed with<br/>per-VM cleanup
end
LocalStorageBase-->>API: CleanupVmInstanceMetadataOnPrimaryStorageReply<br/>(cleanedCount, error if all failed)
API-->>Client: APICleanupVmInstanceMetadataEvent<br/>(totalCleaned, failedPrimaryStorageUuids)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.42.1)plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageBase.javaComment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
header/src/main/java/org/zstack/header/vm/APICleanupVmInstanceMetadataEvent.java (1)
46-54: 建议在__example__中补充failedPrimaryStorageUuids示例值。新增字段已进入事件模型，但示例未覆盖，文档示例可读性会打折。
♻️ 建议修改
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; }As per coding guidelines, "API 类需要实现 example 方法以便生成 API 文档，并确保生成对应的 Groovy API Template 与 API Markdown 文件。"
Also applies to: 56-61
🤖 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 example for APICleanupVmInstanceMetadataEvent is missing a sample value for the new field failedPrimaryStorageUuids; update the class's static __example__ method to populate failedPrimaryStorageUuids (e.g., a list with one or more UUID strings) alongside the existing example fields so generated Groovy template/Markdown include this new property; ensure the example uses realistic UUID-format strings and the method returns the event instance with failedPrimaryStorageUuids set.header/src/main/java/org/zstack/header/vm/APICleanupVmInstanceMetadataMsg.java (1)
39-42: 建议在__example__增加cleanAllVmMetadata=true的示例。当前示例未覆盖新增参数场景，补充后更利于 API 文档与 SDK 使用者理解“全量清理”用法。
As per coding guidelines
API 类需要实现 __example__ 方法以便生成 API 文档，并确保生成对应的 Groovy API Template 与 API Markdown 文件。🤖 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 39 - 42, Update the static example generator APICleanupVmInstanceMetadataMsg.__example__ to include the new boolean option by setting cleanAllVmMetadata = true in the returned msg (while keeping existing vmUuids example), so the generated API docs show the “clean all metadata” scenario; locate the __example__ method in APICleanupVmInstanceMetadataMsg and assign msg.cleanAllVmMetadata = true before returning the msg.plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageBase.java (1)
3633-3691: 建议：增加失败主机追踪以提高运维可见性与类似的
ScanVmInstanceMetadataFromPrimaryStorageMsg处理逻辑（第 3470-3557 行）相比，该方法存在以下不一致：
缺少断开连接主机的追踪：Scan 方法显式追踪了断开连接的主机并在开始前将它们加入
failedHosts列表，但此方法没有类似处理。缺少失败主机信息的反馈：当部分主机失败时，只记录了日志警告，但 reply 中没有包含失败主机的信息，不利于运维排查。
建议参考 Scan 方法的模式，追踪失败的主机列表：
♻️ 建议的改进
private void handleCleanAllVmMetadataOnLocalStorage(final CleanupVmInstanceMetadataOnPrimaryStorageMsg msg) { CleanupVmInstanceMetadataOnPrimaryStorageReply reply = new CleanupVmInstanceMetadataOnPrimaryStorageReply(); + 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); bus.reply(msg, reply); return; } + // Track disconnected hosts as failed upfront + List<String> failedHosts = Collections.synchronizedList(new ArrayList<>()); + List<String> disconnectedHostUuids = new ArrayList<>(allHostUuids); + disconnectedHostUuids.removeAll(connectedHostUuids); + failedHosts.addAll(disconnectedHostUuids); + AtomicInteger totalCleaned = new AtomicInteger(0); new While<>(connectedHostUuids).all((hostUuid, com) -> { // ... existing code ... + `@Override` + public void fail(ErrorCode errorCode) { + logger.warn(...); + failedHosts.add(hostUuid); + com.addError(errorCode); + com.done(); + } }).run(new WhileDoneCompletion(msg) { `@Override` public void done(ErrorCodeList errorCodeList) { // ... existing code ... + if (!failedHosts.isEmpty()) { + reply.setFailedStorages(failedHosts); // 如果 reply 支持此字段 + logger.warn(String.format("[MetadataCleanup] cleanAll partial failure, failedHosts=%s", failedHosts)); + } bus.reply(msg, reply); } }); }🤖 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 3633 - 3691, The handleCleanAllVmMetadataOnLocalStorage method lacks tracking/reporting of failed or disconnected hosts; update it to mirror the ScanVmInstanceMetadataFromPrimaryStorageMsg pattern by creating and maintaining a failedHosts list, add disconnected host UUIDs to it when connectedHostUuids is empty or hosts are not Connected, push host UUIDs into failedHosts when backend preparation fails in getHypervisorBackendFactoryByHostUuid or when bkd.handle(...) fails in the ReturnValueCompletion.fail callback, and include this failedHosts information in the CleanupVmInstanceMetadataOnPrimaryStorageReply (set an error or a failedHosts field) before bus.reply(msg, reply) so callers/operators can see which hosts failed.
🤖 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/APICleanupVmInstanceMetadataMsg.java`:
- Around line 17-22: Add a parameter-combination validation for
APICleanupVmInstanceMetadataMsg: implement a validate method in
VmInstanceApiInterceptor that checks the message's vmUuids and
cleanAllVmMetadata fields and rejects the request unless vmUuids is non-empty OR
cleanAllVmMetadata is true; reference APICleanupVmInstanceMetadataMsg.vmUuids
and APICleanupVmInstanceMetadataMsg.cleanAllVmMetadata in the check and return a
clear API error when neither is provided. Also update
APICleanupVmInstanceMetadataMsg.__example__() to populate the new
cleanAllVmMetadata field (e.g., set it to true or include a sample vmUuids list)
so the API docs show the new option.
---
Nitpick comments:
In
`@header/src/main/java/org/zstack/header/vm/APICleanupVmInstanceMetadataEvent.java`:
- Around line 46-54: The API example for APICleanupVmInstanceMetadataEvent is
missing a sample value for the new field failedPrimaryStorageUuids; update the
class's static __example__ method to populate failedPrimaryStorageUuids (e.g., a
list with one or more UUID strings) alongside the existing example fields so
generated Groovy template/Markdown include this new property; ensure the example
uses realistic UUID-format strings and the method returns the event instance
with failedPrimaryStorageUuids set.
In
`@header/src/main/java/org/zstack/header/vm/APICleanupVmInstanceMetadataMsg.java`:
- Around line 39-42: Update the static example generator
APICleanupVmInstanceMetadataMsg.__example__ to include the new boolean option by
setting cleanAllVmMetadata = true in the returned msg (while keeping existing
vmUuids example), so the generated API docs show the “clean all metadata”
scenario; locate the __example__ method in APICleanupVmInstanceMetadataMsg and
assign msg.cleanAllVmMetadata = true before returning the msg.
In
`@plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageBase.java`:
- Around line 3633-3691: The handleCleanAllVmMetadataOnLocalStorage method lacks
tracking/reporting of failed or disconnected hosts; update it to mirror the
ScanVmInstanceMetadataFromPrimaryStorageMsg pattern by creating and maintaining
a failedHosts list, add disconnected host UUIDs to it when connectedHostUuids is
empty or hosts are not Connected, push host UUIDs into failedHosts when backend
preparation fails in getHypervisorBackendFactoryByHostUuid or when
bkd.handle(...) fails in the ReturnValueCompletion.fail callback, and include
this failedHosts information in the
CleanupVmInstanceMetadataOnPrimaryStorageReply (set an error or a failedHosts
field) before bus.reply(msg, reply) so callers/operators can see which hosts
failed.
🪄 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: 264a09e2-bba8-4ced-bc02-b90ee1ded7cc
📒 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
+17
to
+22
| @APIParam(resourceType = VmInstanceVO.class, required = false) | ||
| private List<String> vmUuids; | ||
|
|
||
| @APIParam(required = false) | ||
| private boolean cleanAllVmMetadata; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 搜索 APICleanupVmInstanceMetadataMsg 相关校验逻辑 =="
rg -n -C3 --type=java 'APICleanupVmInstanceMetadataMsg|cleanAllVmMetadata|getVmUuids|vmUuids|intercept|validate'
echo
echo "== 定位 VmInstanceApiInterceptor 并查看上下文 =="
fd -t f 'VmInstanceApiInterceptor.java' -x sh -c '
echo ">>> {}"
rg -n -C6 "APICleanupVmInstanceMetadataMsg|cleanAllVmMetadata|getVmUuids|vmUuids|validate|intercept" "{}"
'Repository: MatheMatrix/zstack
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 搜索 VmInstanceApiInterceptor 中 APICleanupVmInstanceMetadataMsg 的处理 =="
fd -type f -name "VmInstanceApiInterceptor.java" -exec rg -n -A 20 "APICleanupVmInstanceMetadataMsg" {} +
echo ""
echo "== 在计算模块中搜索 cleanAllVmMetadata 的验证逻辑 =="
rg -n -B 3 -A 10 "cleanAllVmMetadata" --type=java compute/ 2>/dev/null | head -100
echo ""
echo "== 搜索 APICleanupVmInstanceMetadataMsg 的完整定义 =="
fd -type f -name "APICleanupVmInstanceMetadataMsg.java" -exec cat {} +Repository: MatheMatrix/zstack
Length of output: 321
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 搜索 VmInstanceApiInterceptor 中 APICleanupVmInstanceMetadataMsg 的处理 =="
fd -name "VmInstanceApiInterceptor.java" -exec rg -n -A 20 "APICleanupVmInstanceMetadataMsg" {} +
echo ""
echo "== 在计算模块中搜索 cleanAllVmMetadata 的验证逻辑 =="
rg -n -B 3 -A 10 "cleanAllVmMetadata" compute/ 2>/dev/null | head -100
echo ""
echo "== 搜索 APICleanupVmInstanceMetadataMsg 的完整定义 =="
fd -name "APICleanupVmInstanceMetadataMsg.java" -exec cat {} +Repository: MatheMatrix/zstack
Length of output: 381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 搜索 VmInstanceApiInterceptor.java 文件 =="
fd "VmInstanceApiInterceptor.java"
echo ""
echo "== 在计算模块中查找 APICleanupVmInstanceMetadataMsg 的处理 =="
rg "APICleanupVmInstanceMetadataMsg" compute/ --type java -A 10 -B 2
echo ""
echo "== 搜索 cleanAllVmMetadata 的验证逻辑 =="
rg "cleanAllVmMetadata" compute/ --type java -B 5 -A 5Repository: MatheMatrix/zstack
Length of output: 229
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 查看 VmInstanceApiInterceptor 的完整内容，检查是否有 APICleanupVmInstanceMetadataMsg 处理 =="
cat compute/src/main/java/org/zstack/compute/vm/VmInstanceApiInterceptor.java | grep -n -i "cleanup\|intercept" | head -30
echo ""
echo "== 搜索整个仓库中 APICleanupVmInstanceMetadataMsg 的使用 =="
rg "APICleanupVmInstanceMetadataMsg" --type java
echo ""
echo "== 查看 APICleanupVmInstanceMetadataMsg 的定义 =="
fd "APICleanupVmInstanceMetadataMsg.java" -exec wc -l {} + && fd "APICleanupVmInstanceMetadataMsg.java" -exec cat {} +Repository: MatheMatrix/zstack
Length of output: 4193
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 查看 APICleanupVmInstanceMetadataMsg 的定义 =="
cat header/src/main/java/org/zstack/header/vm/APICleanupVmInstanceMetadataMsg.java
echo ""
echo "== 搜索整个仓库中 APICleanupVmInstanceMetadataMsg 在 intercept 方法中的处理 =="
rg "intercept|APICleanupVmInstanceMetadataMsg" compute/src/main/java/org/zstack/compute/vm/VmInstanceApiInterceptor.java --type java -B 2 -A 5Repository: MatheMatrix/zstack
Length of output: 1841
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 在 VmInstanceApiInterceptor 的 intercept 方法中查找完整列表，确认是否缺少 APICleanupVmInstanceMetadataMsg 的处理 =="
rg "instanceof API" compute/src/main/java/org/zstack/compute/vm/VmInstanceApiInterceptor.java --type java | grep -i "cleanup\|metadata" || echo "Not found in intercept"
echo ""
echo "== 查看 VmInstanceApiInterceptor intercept 方法的完整代码范围 =="
sed -n '94,600p' compute/src/main/java/org/zstack/compute/vm/VmInstanceApiInterceptor.java | head -200
echo ""
echo "== 搜索整个 compute 模块中是否有其他拦截器处理 APICleanupVmInstanceMetadataMsg =="
rg "APICleanupVmInstanceMetadataMsg" compute/ --type javaRepository: MatheMatrix/zstack
Length of output: 11401
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 检查是否有其他地方处理 APICleanupVmInstanceMetadataMsg（如消息处理器） =="
rg "APICleanupVmInstanceMetadataMsg\|cleanAllVmMetadata" --type java | head -50
echo ""
echo "== 查找 VmInstanceApiInterceptor 完整的 intercept 方法结束位置 =="
grep -n "APIRegisterVmInstanceFromMetadataMsg" compute/src/main/java/org/zstack/compute/vm/VmInstanceApiInterceptor.java
grep -n "setServiceId" compute/src/main/java/org/zstack/compute/vm/VmInstanceApiInterceptor.java | head -5
echo ""
echo "== 统计 VmInstanceApiInterceptor 中 instanceof 检查的个数 =="
grep -c "instanceof API" compute/src/main/java/org/zstack/compute/vm/VmInstanceApiInterceptor.java || echo "0"Repository: MatheMatrix/zstack
Length of output: 119
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 搜索 CleanupVmInstanceMetadata 相关的任何处理逻辑（包括消息处理器） =="
rg -i "cleanup.*metadata\|metadata.*cleanup" compute/ --type java -B 2 -A 5
echo ""
echo "== 查找 VmInstanceApiInterceptor 是否在其他地方处理过此消息 =="
grep -n "APICleanupVmInstanceMetadataMsg" compute/src/main/java/org/zstack/compute/vm/VmInstanceApiInterceptor.java || echo "Not found in VmInstanceApiInterceptor"
echo ""
echo "== 确认 __example__ 方法的完整内容 =="
tail -10 header/src/main/java/org/zstack/header/vm/APICleanupVmInstanceMetadataMsg.javaRepository: MatheMatrix/zstack
Length of output: 115
需要添加参数组合验证。
现在 vmUuids 和 cleanAllVmMetadata 都是可选的，但代码中没有验证至少一个参数被设置。建议在 VmInstanceApiInterceptor 中为 APICleanupVmInstanceMetadataMsg 添加验证方法，确保：vmUuids 非空或 cleanAllVmMetadata=true（至少满足其一），避免发送空清理请求。
同时，__example__() 方法应覆盖新增的 cleanAllVmMetadata 参数，以提升 API 文档的可发现性。
🤖 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 - 22, Add a parameter-combination validation for
APICleanupVmInstanceMetadataMsg: implement a validate method in
VmInstanceApiInterceptor that checks the message's vmUuids and
cleanAllVmMetadata fields and rejects the request unless vmUuids is non-empty OR
cleanAllVmMetadata is true; reference APICleanupVmInstanceMetadataMsg.vmUuids
and APICleanupVmInstanceMetadataMsg.cleanAllVmMetadata in the check and return a
clear API error when neither is provided. Also update
APICleanupVmInstanceMetadataMsg.__example__() to populate the new
cleanAllVmMetadata field (e.g., set it to true or include a sample vmUuids list)
so the API docs show the new option.
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 !9629