<fix>[storage]: ZSTAC-85462 prepare zbs hosts on attach#4092
<fix>[storage]: ZSTAC-85462 prepare zbs hosts on attach#4092ZStack-Robot wants to merge 1 commit into
Conversation
|
Warning Review limit reached
More reviews will be available in 22 minutes and 33 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Walkthrough向 Changes外部主存储附加流程实现
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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/haidong.pang/fix/ZSTAC-85462-zbs-attach-hosts
branch
7 times, most recently
from
210b728 to
6495ac6
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java`:
- Around line 122-127: The host filter currently selects hosts by Connected
status and KVM hypervisor but still includes Disabled hosts; update the
Q.New(HostVO.class) query (the chain that uses HostVO_.clusterUuid,
HostVO_.status, HostVO_.hypervisorType and HostVO_.state) to exclude Disabled as
well—e.g. extend the notIn/state check to include HostState.Disabled (or add
.ne(HostVO_.state, HostState.Disabled)) so only Connected + Enabled + KVM hosts
are returned for attach/deploy operations.
🪄 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: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: 77e386bb-7404-4730-a5fc-95e6768c0553
📒 Files selected for processing (2)
storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.javatest/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsPrimaryStorageCase.groovy
🚧 Files skipped from review as they are similar to previous changes (1)
- test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsPrimaryStorageCase.groovy
| List<HostVO> hosts = Q.New(HostVO.class) | ||
| .eq(HostVO_.clusterUuid, clusterUuid) | ||
| .eq(HostVO_.status, HostStatus.Connected) | ||
| .eq(HostVO_.hypervisorType, VmInstanceConstant.KVM_HYPERVISOR_TYPE) | ||
| .notIn(HostVO_.state, Arrays.asList(HostState.PreMaintenance, HostState.Maintenance)) | ||
| .list(); |
There was a problem hiding this comment.
筛选条件应排除 Disabled 主机，避免被“不可调度主机”误阻塞附加流程。
当前仅排除了 PreMaintenance/Maintenance，Disabled 仍会进入部署列表；若其 deployClient 失败，会直接导致 attach 失败，即使集群里还有可用主机。建议将可用主机收敛为 Connected + Enabled + KVM。
建议修改
List<HostVO> hosts = Q.New(HostVO.class)
.eq(HostVO_.clusterUuid, clusterUuid)
.eq(HostVO_.status, HostStatus.Connected)
+ .eq(HostVO_.state, HostState.Enabled)
.eq(HostVO_.hypervisorType, VmInstanceConstant.KVM_HYPERVISOR_TYPE)
- .notIn(HostVO_.state, Arrays.asList(HostState.PreMaintenance, HostState.Maintenance))
.list();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java`
around lines 122 - 127, The host filter currently selects hosts by Connected
status and KVM hypervisor but still includes Disabled hosts; update the
Q.New(HostVO.class) query (the chain that uses HostVO_.clusterUuid,
HostVO_.status, HostVO_.hypervisorType and HostVO_.state) to exclude Disabled as
well—e.g. extend the notIn/state check to include HostState.Disabled (or add
.ne(HostVO_.state, HostState.Disabled)) so only Connected + Enabled + KVM hosts
are returned for attach/deploy operations.
There was a problem hiding this comment.
Comment from haidong.pang:
Disable的主机也应当deploy。避免该主机enable后无法连接存储。
There was a problem hiding this comment.
@MatheMatrix，感谢 haidong.pang 的解释！这个设计考量是合理的——Disabled 状态只是管理员主动禁用调度，主机仍处于 Connected 状态，提前部署好 ZBS client 可以确保主机重新 Enabled 后立即能连接存储，不需要再等重连触发。与此同时 deployClient 是幂等操作，多次调用无副作用。
我之前建议排除 Disabled 主机是错误的，请忽略该建议。
✏️ Learnings added
Learnt from: MatheMatrix
Repo: MatheMatrix/zstack PR: 4092
File: storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java:122-126
Timestamp: 2026-05-28T07:21:00.466Z
Learning: In ExternalPrimaryStorage.attachHook() (storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java), the host filter intentionally includes Disabled hosts (only PreMaintenance and Maintenance are excluded). This is by design: Disabled hosts are still Connected and should have the ZBS client deployed proactively so that when they are re-enabled they can immediately connect to storage without waiting for a reconnect. deployClient is idempotent so deploying to Disabled hosts is safe. Do NOT suggest filtering out Disabled hosts from this query.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: ZStack-Robot
Repo: MatheMatrix/zstack PR: 3826
File: compute/src/main/java/org/zstack/compute/allocator/HostAllocatorChain.java:261-265
Timestamp: 2026-04-22T04:38:00.864Z
Learning: 在 ZStack 的 `Platform.err()` / `Platform.toRawString()` 调用中，传入用于格式化的 `%s` 参数若实现了 `org.zstack.header.core.I18nMessage`，框架会自动通过 `getDetails()` 取得原始文本、并通过 `getI18nDetails()` 取得国际化文本。因此，在代码审查中不应将“直接把实现了 `I18nMessage` 的对象作为 `%s` 参数传入而可能打印出对象地址/不安全”当作问题；此用法是正确且应当被视为安全的（例如将 `HostCandidate.RejectedCandidate` 这类实现了 `I18nMessage` 的对象作为 `%s` 参数传入）。
7b4fbc5 to
09d138e
Compare
Resolves: ZSTAC-85462 Change-Id: Iba5d8a7f144cdaf367030867d202887f99eef77e
MatheMatrix
force-pushed
the
sync/haidong.pang/fix/ZSTAC-85462-zbs-attach-hosts
branch
from
09d138e to
9022e98
Compare
MatheMatrix
deleted the
sync/haidong.pang/fix/ZSTAC-85462-zbs-attach-hosts
branch
3 participants
Summary
Fix ZBS external primary storage attach so existing connected, non-maintenance KVM hosts are prepared before attach succeeds. This refreshes host-side ZBS client configuration and ensures the heartbeat volume without running host-storage
check-host-statusinattachHook.Changes
ExternalPrimaryStorage.attachHook()to prepare usable KVM hosts during attach.deployClient -> activateHeartbeatVolumeand fail attach if preparation fails for a usable host.check-host-status, and preparation failure.Testing
rtk git -C zstack diff --checkrtk scripts/backend-build-container/container-run java-package zstack/storagertk scripts/backend-build-container/container-run java-package zstack/plugin/zbsrtk scripts/backend-build-container/container-run test-zstack ZbsPrimaryStorageCaseblocked before the case runs by unrelatedpremium/test-premiumcompile errors (VmOvsNicConstant,ExternalService.getServiceType()). Directzstack/testMaven execution is also blocked by unrelated existingKVMHostUtilsTestandCoalesceQueueCasecompile errors.Resolves: ZSTAC-85462
sync from gitlab !9991