<fix>[storage]: ZSV-12657 wait zbs clients before revert

[ZStack-Robot]

Summary

Fix ZBS snapshot revert failure caused by a short window where VM stop has completed but the ZBS volume client is still active.

Changes

• Wait for active ZBS volume clients to be released before rolling back a volume snapshot.
• Add ZBS primary storage regression coverage for delayed client release before revert.

Testing

• git diff --check
• cbok zsv groovy_test --test-class org.zstack.test.integration.storage.primary.addon.zbs.ZbsPrimaryStorageCase --test-mode case
• CI pipeline

Resolves: ZSV-12657

sync from gitlab !10565

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 43 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 8d36d014-2da6-4749-b838-f86a5bd4767f

📥 Commits

Reviewing files that changed from the base of the PR and between c725911 and 74cfe66.

📒 Files selected for processing (1)

• plugin/kvm/src/main/java/org/zstack/kvm/KVMGlobalConfig.java

Walkthrough

ZBS 数据卷快照回滚新增卷客户端释放等待机制，支持轮询、超时、中断和查询失败处理；集成测试验证客户端释放后才执行一次回滚。

Changes

ZBS 快照回滚流程

Layer / File(s)	Summary
回滚前客户端释放检查 plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.java	新增轮询超时与检查间隔常量，并在快照回滚 HTTP 请求前检查活跃卷客户端，处理释放成功、超时、中断及查询失败。
客户端释放等待集成验证 test/src/main/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsPrimaryStorageCase.groovy	新增测试模拟客户端逐步释放，验证回滚等待完成后执行且仅执行一次，并清理测试资源。

Estimated code review effort: 3 (Moderate) | ~20 minutes

Poem

小兔守着快照门，
等待客户端轻轻奔。
两次查询仍有影，
第三次空才回滚。
卷已安全，胡萝卜香。

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	标题简洁且准确概括了在回滚前等待 ZBS 客户端释放这一主要改动。
Description check	✅ Passed	描述与本次修改一致，清楚说明了问题、改动内容和测试情况。
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch sync/zstackio/codex/zbs-revert-wait-client-release

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

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 `@plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.java`:
- Around line 1060-1101: Refactor waitVolumeClientsReleased to use asynchronous
scheduled callbacks instead of the current while loop and
TimeUnit.SECONDS.sleep, so revertVolumeSnapshot’s message-processing thread is
never blocked during the release wait. Preserve the existing path validation,
active-client checks, timeout failure, operation-failure propagation, and
interruption-equivalent failure behavior through the callback chain, completing
ReturnValueCompletion only when clients are released or a terminal failure
occurs.

🪄 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: bb96fbf1-8817-4cc4-a990-595e2760f440

📥 Commits

Reviewing files that changed from the base of the PR and between 81727e9 and c725911.

📒 Files selected for processing (2)

• plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.java
• test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsPrimaryStorageCase.groovy

[zstack-robot-2]

Comment from yaohua.wu:

Review: MR !10565 — ZSV-12657

Background (preserved across rounds)

• Jira: ZSV-12657 — 【虚拟机加密】zbs存储-自定义恢复快照-加密数据盘报错
• Bug summary: 问题与加密盘本身无关。VM force stop 后，管理面已进入 Stopped，但 CBD/ZBS client session 仍可能处于释放窗口；此时立即 revert 数据盘快照会收到 kFileOccupied。
• Intent & scope: 在 ZBS snapshot rollback 前异步轮询目标 volume 的 active CBD clients，全部释放后再执行 rollback；同时补充“延迟释放”和“等待过程不阻塞调用线程”的集成测试。
• Round 1 initial findings: 0 × P0, 0 × P1；1 × P2，位于 ZBS client-release 等待参数与 timeout 测试契约。
• Suggested fix direction: 无阻塞项；将等待超时/轮询间隔改为 ZBS GlobalConfig，并用短 timeout 覆盖失败分支。

---

🟡 Warning (P2)

#	File:Line	Issue	Fix	Reviewer(s)	Conf	Route
1	plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.java:111	VOLUME_CLIENT_RELEASE_TIMEOUT_SECONDS=100 与轮询间隔 1s 被硬编码。不同 ZBS 环境无法按 session 实际释放时延调优；同时当前两项测试只覆盖“最终释放成功”，timeout 失败分支若按现值验证需要等待 100 秒，事实上未被回归测试保护。	在 ZBS GlobalConfig 中新增 client-release timeout 与 poll interval；测试中临时把 timeout 调小，补充 clients 始终不释放时 comp.fail 且 ROLLBACK_SNAPSHOT_PATH 不执行的断言。	craftsmanship	0.90	gated_auto → downstream-resolver

Upstream Freshness

Check	Result
Target branch	feature-zsv-5.1.0-encryption 比 MR source 新 2 个提交
Same-file change	新提交 f78c317b61 同样修改 ZbsStorageController.java，新增 volume encryption conversion 接口
Compatibility	git merge-tree 合并成功，无文本冲突；合并结果同时包含 encryption conversion 与 client-release wait，未发现语义覆盖

Coverage

• Generated-file filter: 0 files skipped（kept 2 / total 2）
• Deep-reviewed: 1 logic file；reviewed: 1 test file；0 hunks clipped
• Verifier: 1 verified / 0 inconclusive / 0 contradicted
• 被 confidence gate 抑制：0；被 Quality Gate 抑制：0
• 已核验：Jira 触发链路、snapshot revert 调用链、AsyncSafeAspect 异常兜底、snapshot tree 串行化、目标分支合并结果
• Testing: git diff --check 通过；本机无 mvn/ cbok，无法复跑 MR 声明的 cbok zsv groovy_test --test-class org.zstack.test.integration.storage.primary.addon.zbs.ZbsPrimaryStorageCase --test-mode case

Verdict: APPROVED

未发现 P0/P1 阻塞问题；建议在合并前补齐可配置 timeout 与失败分支回归测试。

---

🤖 Robot Reviewer

[MatheMatrix]

Comment from 刘志建:

Code Review 汇总(仅本次 MR diff 范围)

范围:27aafe64..a581db92(3 个文件,+268/-11)。

意图

snapshot revert 前主动轮询 ZBS,直到 volume 的 CBD 客户端全部释放,避免在客户端仍持有 volume 时直接 revert 造成数据不一致或 revert 失败。加了两个全局配置(超时 100s、poll 间隔 1s)和两个测试用例(等待成功、超时失败)。

主要发现

🔴 中风险 · waitVolumeClientsReleased 内部调用同步阻塞 HTTP 且长时间占用调度线程

plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.java:1066-1108

getActiveClients() 是 syncHttpCall 同步 HTTP,在 submitTimeoutTask 的调度线程池里执行。每次 poll 都会:

1. 同步等待 HTTP 响应(默认 timeout 可能远大于 pollInterval=1s)
2. 拿到结果后立即再 submitTimeoutTask 排下一次

后果:

• 高并发 revert 场景下(比如批量恢复),每个 revert 都长时间占用一个调度线程,可能耗尽 ThreadFacade 的调度池
• 如果 ZBS API 响应慢(比如网络抖动 5s),nextDelay 计算基于 System.currentTimeMillis() 会变成非常小的值(甚至 1s),poll 频率被 HTTP 延迟主导,失去 pollInterval 的意义

建议:改为 httpCall 异步(如 GetVolumeClientsCmd 有异步版本),或者至少确认 ThreadFacade 的池大小对并发 revert 数量足够。

🟡 中风险 · 首次 poll 之前无 short-circuit

ZbsStorageController.java:1080-1084

waitVolumeClientsReleased 一进来就 submitTimeoutTask(..., 0) 触发第一次检查,再一次跳到调度线程里跑。绝大多数 revert 场景没有活跃客户端,理想路径应该是当前线程直接问一次,没客户端立即 success 到 comp,只有真有客户端才切到 timer。

现在的写法多了一次线程跳转,增加了正常 revert 路径的延迟(即使 delay=0,也要等调度器排队)。

建议:入口第一次同步查询,只在需要等待时才 submitTimeoutTask。

🟡 中风险 · nextDelay 计算与 pollInterval 语义偏离

ZbsStorageController.java:1103-1104

long remaining = expired - System.currentTimeMillis();
long nextDelay = Math.min(pollInterval, Math.max(1, TimeUnit.MILLISECONDS.toSeconds(remaining)));

• 当 remaining < 1000ms 时 toSeconds(remaining) 会返回 0,被 Math.max(1, 0) 抬到 1s,可能超过实际剩余时间 → 下一次进入后会立即超时失败,但延迟了 1s 才检查。可以直接跳过等待,进入 comp.fail 分支
• 相反,如果 remaining 极小(比如 100ms)本应该马上判超时,现在还要多等 1s

建议:

• 剩余时间 < pollInterval 时:直接检查一次或立即判超时,不要再 sleep
• 或者在下次进入时,循环第一步就应该判 expired,当前实现是"先查客户端再判超时",顺序也可以调整成"先判超时再查客户端"避免最后一次浪费 HTTP

🟡 中风险 · 只按 @snapshot_ 判断 install path 格式,ZBS 侧格式变化会静默出错

ZbsStorageController.java:1067-1071

int index = snapshotInstallPath.indexOf("@snapshot_");
if (index < 0) {
    comp.fail(operr("... invalid zbs snapshot install path"));
    return;
}
String volumeInstallPath = snapshotInstallPath.substring(0, index);

• 只识别 @snapshot_ 前缀,ZBS 若引入其他 snapshot 命名(如 @sn_、@snap-)会全部失败
• ZBS 侧现有代码是否已有 helper 从 snapshotInstallPath 拆出 volume path?如果有,应复用,避免重复散点解析

建议:检查 ZbsHelper 或 parseZbsCbdPath 之类的现有工具方法,统一入口。若确实没有,把这段解析抽成 parseSnapshotInstallPath 方法方便未来扩展。

🟢 低风险 · getActiveClients 只针对 CBD 协议

ZbsStorageController.java:1090

硬编码 VolumeProtocol.CBD.toString()。如果 ZBS 未来支持其他协议(iSCSI/NVMe-oF 之类),这里会漏检查。可加常量或 TODO。

🟢 低风险 · ZbsGlobalConfig 未随 spring xml 装配

plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsGlobalConfig.java(新增)

@GlobalConfigDefinition 通常会通过组件扫描自动加载,但要确认 zbs 模块的 spring config xml 有对应的 component-scan 覆盖到该包,否则运行时读默认值不会生效。相同模式的类(比如 CephGlobalConfig)已经 wired 好可以对照。

🟢 观察 · 测试覆盖良好

• testRevertVolumeSnapshotDoesNotBlockWhileWaitingForActiveClients:验证异步非阻塞 + 客户端释放后正确 rollback
• testRevertVolumeSnapshotFailsWhenActiveClientsAreNotReleasedBeforeTimeout:验证超时失败

测试写得很到位,断言粒度细(rollbackCount==0 等)。仅建议:

• 第一个用例 revertThread.join(5000) 在 finally 里,若测试卡住 5 秒后线程可能仍未退出但用例通过,建议加个 assert !revertThread.isAlive()
• 第二个用例 errorCode.get().details.contains("were not released within 1 seconds") 依赖英文措辞,如果未来 i18n 化会破坏,建议改成断言 errorCode 类型或 code

结论

逻辑方向正确,补齐了 revert-before-release 这个真实风险。🔴/🟡 项建议 merge 前处理,尤其是同步 HTTP 在调度线程池阻塞这个问题在高并发环境下容易踩坑。