<feature>[storage]: support cleanAllVmMetadata param#3760
Closed
MatheMatrix wants to merge 1 commit intofeature-zsv-5.0.0-vm-support-vtpm-and-secucebootfrom
Closed
<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
Walkthrough该 PR 为 VM 实例元数据清理引入 Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant API
participant LocalStorageBase
participant HostResolver
participant KVMBackend
participant Agent
Client->>API: APICleanupVmInstanceMetadataMsg\n(cleanAllVmMetadata=true)
API->>LocalStorageBase: CleanupVmInstanceMetadataOnPrimaryStorageMsg
LocalStorageBase->>HostResolver: queryConnectedHosts()
HostResolver-->>LocalStorageBase: list of hosts
loop for each host
LocalStorageBase->>KVMBackend: CleanupVmMetadataCmd\n(cleanAllVmMetadata=true)
KVMBackend->>Agent: invoke cleanup
Agent-->>KVMBackend: CleanupVmMetadataRsp\n(cleanedCount)
KVMBackend-->>LocalStorageBase: reply with cleanedCount / error
end
LocalStorageBase->>LocalStorageBase: aggregate cleanedCount\n(record failures)
LocalStorageBase-->>API: CleanupVmInstanceMetadataOnPrimaryStorageReply\n(cleanedCount, error if all failed)
API-->>Client: APICleanupVmInstanceMetadataEvent\n(totalCleaned, failedPrimaryStorageUuids)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
Comment |
![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.
MatheMatrix
force-pushed
the
sync/tao.gan/ZSV-11867@@3
branch
from
a956bcd to
51b904d
Compare
Resolves: ZSV-11867 Change-Id: I6e6b616875706f67706d726d617367637863717a
MatheMatrix
force-pushed
the
sync/tao.gan/ZSV-11867@@3
branch
from
51b904d to
d239611
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.java (1)
3955-3957: 建议对cleanedCount增加非负保护。Line 3956 直接信任 agent 返回值，若异常返回负数会污染上层清理统计；建议在赋值前做边界收敛。
可选修正示例
public void success(CleanupVmMetadataRsp rsp) { CleanupVmInstanceMetadataOnPrimaryStorageReply reply = new CleanupVmInstanceMetadataOnPrimaryStorageReply(); - reply.setCleanedCount(rsp.cleanedCount); + reply.setCleanedCount(Math.max(rsp.cleanedCount, 0)); completion.success(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/LocalStorageKvmBackend.java` around lines 3955 - 3957, The code sets reply.setCleanedCount(rsp.cleanedCount) directly and should guard against negative values from the agent; change the assignment in the block that constructs CleanupVmInstanceMetadataOnPrimaryStorageReply so you sanitize rsp.cleanedCount (e.g., use a non-negative clamp like Math.max(0, rsp.cleanedCount) or check for null/invalid and default to 0) before calling reply.setCleanedCount(...), then call completion.success(reply).
🤖 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 3636-3688: The code only queries Connected hosts
(connectedHostUuids) and treats “no connected host” as a successful
cleanedCount=0, which hides metadata left on disconnected hosts; change to query
all host UUIDs attached to this primary storage, separate them into connected vs
disconnected, and treat disconnected hosts as failures (add errors into the
While's ErrorCodeList or immediately add to com via com.addError) instead of
returning cleanedCount=0; keep using getHypervisorBackendFactoryByHostUuid and
LocalStorageHypervisorBackend.handle for connected hosts, accumulate
totalCleaned from successful callbacks, and if any host (including disconnected
ones) failed or was skipped then set reply.setError(...) (or include failed host
list in the reply) rather than reporting success via reply.setCleanedCount.
---
Nitpick comments:
In
`@plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.java`:
- Around line 3955-3957: The code sets reply.setCleanedCount(rsp.cleanedCount)
directly and should guard against negative values from the agent; change the
assignment in the block that constructs
CleanupVmInstanceMetadataOnPrimaryStorageReply so you sanitize rsp.cleanedCount
(e.g., use a non-negative clamp like Math.max(0, rsp.cleanedCount) or check for
null/invalid and default to 0) before calling reply.setCleanedCount(...), then
call completion.success(reply).
🪄 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: eac075fa-47dd-4685-acab-b350f44d6415
📒 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 (2)
- header/src/main/java/org/zstack/header/storage/primary/CleanupVmInstanceMetadataOnPrimaryStorageMsg.java
- sdk/src/main/java/org/zstack/sdk/CleanupVmInstanceMetadataResult.java
🚧 Files skipped from review as they are similar to previous changes (4)
- plugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageKVMBackend.java
- header/src/main/java/org/zstack/header/vm/APICleanupVmInstanceMetadataEvent.java
- sdk/src/main/java/org/zstack/sdk/CleanupVmInstanceMetadataAction.java
- header/src/main/java/org/zstack/header/vm/APICleanupVmInstanceMetadataMsg.java
Comment on lines
+3636
to
+3688
| 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; | ||
| } | ||
|
|
||
| AtomicInteger totalCleaned = new AtomicInteger(0); | ||
| new While<>(connectedHostUuids).all((hostUuid, com) -> { | ||
| final LocalStorageHypervisorBackend bkd; | ||
| try { | ||
| LocalStorageHypervisorFactory f = getHypervisorBackendFactoryByHostUuid(hostUuid); | ||
| bkd = f.getHypervisorBackend(self); | ||
| } catch (Exception e) { | ||
| logger.warn(String.format("[MetadataCleanup] cleanAll: failed to prepare backend for host[uuid:%s] on ps[uuid:%s]: %s", | ||
| hostUuid, self.getUuid(), e.getMessage())); | ||
| com.addError(operr("failed to prepare backend for host[uuid:%s]: %s", hostUuid, e.getMessage())); | ||
| com.done(); | ||
| return; | ||
| } | ||
| bkd.handle(msg, hostUuid, new ReturnValueCompletion<CleanupVmInstanceMetadataOnPrimaryStorageReply>(com) { | ||
| @Override | ||
| public void success(CleanupVmInstanceMetadataOnPrimaryStorageReply returnValue) { | ||
| totalCleaned.addAndGet(returnValue.getCleanedCount()); | ||
| com.done(); | ||
| } | ||
|
|
||
| @Override | ||
| public void fail(ErrorCode errorCode) { | ||
| logger.warn(String.format("[MetadataCleanup] cleanAll: failed on host[uuid:%s] on ps[uuid:%s]: %s", | ||
| hostUuid, self.getUuid(), errorCode)); | ||
| com.addError(errorCode); | ||
| com.done(); | ||
| } | ||
| }); | ||
| }).run(new WhileDoneCompletion(msg) { | ||
| @Override | ||
| public void done(ErrorCodeList errorCodeList) { | ||
| if (!errorCodeList.getCauses().isEmpty() && errorCodeList.getCauses().size() == connectedHostUuids.size()) { | ||
| reply.setError(operr("failed to cleanup all vm metadata from all hosts on local primary storage[uuid:%s], causes: %s", | ||
| self.getUuid(), errorCodeList)); | ||
| } else { | ||
| reply.setCleanedCount(totalCleaned.get()); | ||
| } | ||
| bus.reply(msg, reply); |
There was a problem hiding this comment.
不要把断连主机默认为“已全部清理”。
这里仅遍历 Connected 主机。只要 local primary storage 上还挂着断连主机，cleanAllVmMetadata=true 就可能在实际未清完的情况下返回成功；尤其是 Line 3644-3648，当前会在“没有任何可连主机”时直接返回 cleanedCount=0。这会让调用方误以为“全部 VM metadata 已清理”，但断连主机上的 metadata 其实还保留着。
建议和本类的 handle(ScanVmInstanceMetadataFromPrimaryStorageMsg) 保持一致：先拿到全部 host，再把断连 host 计入失败集合；只要存在未处理 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 3636 - 3688, The code only queries Connected hosts
(connectedHostUuids) and treats “no connected host” as a successful
cleanedCount=0, which hides metadata left on disconnected hosts; change to query
all host UUIDs attached to this primary storage, separate them into connected vs
disconnected, and treat disconnected hosts as failures (add errors into the
While's ErrorCodeList or immediately add to com via com.addError) instead of
returning cleanedCount=0; keep using getHypervisorBackendFactoryByHostUuid and
LocalStorageHypervisorBackend.handle for connected hosts, accumulate
totalCleaned from successful callbacks, and if any host (including disconnected
ones) failed or was skipped then set reply.setError(...) (or include failed host
list in the reply) rather than reporting success via reply.setCleanedCount.
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 !9629