<fix>[storage]: ZSTAC-86038 support typed storage resource stats#4548
<fix>[storage]: ZSTAC-86038 support typed storage resource stats#4548zstack-robot-2 wants to merge 1 commit into
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
Warning
|
| Layer / File(s) | Summary |
|---|---|
资源统计契约 header/src/main/java/org/zstack/header/storage/addon/StorageResource.java, header/src/main/java/org/zstack/header/storage/addon/primary/PrimaryStorageControllerSvc.java, header/src/main/java/org/zstack/header/storage/snapshot/VolumeSnapshotStats.java, header/src/main/java/org/zstack/header/volume/VolumeStats.java |
新增 StorageResource 基类，卷和快照统计类继承该基类，服务接口将统计结果改为 StorageResource。 |
卷与快照统计实现 plugin/expon/src/main/java/org/zstack/expon/ExponStorageController.java, plugin/xinfini/src/main/java/org/zstack/xinfini/XInfiniStorageController.java, plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.java |
控制器根据安装路径区分卷和快照，并返回 VolumeStats 或 VolumeSnapshotStats。 |
主存储资源消费与错误处理 storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java, plugin/xinfini/src/main/java/org/zstack/xinfini/XInfiniApiHelper.java |
主存储使用统一资源统计读取实际大小和父链信息；缓存复用前增加统计校验，并处理资源不存在错误。 |
卷快照统计集成验证 test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/xinfini/XinfiniPrimaryStorageCase.groovy |
新增卷与快照统计查询及断言，验证返回类型、安装路径和大小字段。 |
Estimated code review effort: 4 (Complex) | ~45 minutes
Sequence Diagram(s)
sequenceDiagram
participant TestCase
participant PrimaryStorageControllerSvc
participant XInfiniStorageController
participant StorageResource
TestCase->>PrimaryStorageControllerSvc: 查询卷或快照 stats
PrimaryStorageControllerSvc->>XInfiniStorageController: 转发 installPath
XInfiniStorageController->>XInfiniStorageController: 判断卷路径或快照路径
XInfiniStorageController->>StorageResource: 创建对应统计对象
XInfiniStorageController-->>TestCase: 返回 StorageResource
TestCase->>TestCase: 校验类型、路径和大小
Poem
我是小兔，抱来资源新篮，
卷与快照，都有统一名牌。
路径一分，统计便分流，
失效缓存，轻轻清走。
测试萝卜，咔嚓咔嚓全通过！
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 2.86% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | 标题准确概括了将存储资源统计泛化为类型化 StorageResource 的主要改动。 |
| Description check | ✅ Passed | 描述与变更一致，说明了 stats 泛化、子类复用及相关存储控制器调整。 |
| 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/jin.ma/fix/ZSTAC-86038-storage-resource-stats
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
plugin/expon/src/main/java/org/zstack/expon/ExponStorageController.java (1)
1149-1172: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚖️ Poor tradeoff补齐 Expon 统计结果的
parentUri：plugin/expon/src/main/java/org/zstack/expon/ExponStorageController.java:1149-1172的卷/快照统计分支都没有可回填祖先链的字段，而VolumeModule/VolumeSnapshotModule也没有等价于bsSnapId/bsVolumeId的引用；如果上层会走通用父链解析，这里会断链。可补一个父资源标识，或明确声明 Expon 不支持祖先链。🤖 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 `@plugin/expon/src/main/java/org/zstack/expon/ExponStorageController.java` around lines 1149 - 1172, 补齐 Expon 的统计结果父资源信息：更新 ExponStorageController.stats 及其 getVolumeStats 分支，为卷和快照统计结果设置可供通用父链解析的 parentUri 或等价父资源标识；若 Expon 的 VolumeModule 和 VolumeSnapshotModule 无法提供该标识，则在相关统计结果处理路径中明确声明不支持祖先链，避免返回看似可解析但实际断链的数据。
🤖 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 `@header/src/main/java/org/zstack/header/storage/addon/StorageResource.java`:
- Around line 4-44: 更新 VolumeStats.getActualSize()，在 actualSize 未设置时返回
0L，保留已设置值的返回行为，使 ExternalPrimaryStorage 中的 Collectors.toMap 能安全处理所有
VolumeStats；可参考 VolumeSnapshotStats 的空值兜底方式。
In
`@plugin/xinfini/src/main/java/org/zstack/xinfini/XInfiniStorageController.java`:
- Around line 854-868: Update the snapshot branch of
XInfiniStorageController.stats so parentUri points directly to the backing
volume identified by snapshot.getSpec().getBsVolumeId(), rather than passing
that volume through getParentUri. Preserve the existing size and installPath
handling.
In
`@storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java`:
- Around line 1432-1436: 修正 ExternalPrimaryStorage 中该 completion.fail 的 operr
调用，补上本模块现有的全局错误码常量作为首个参数，并保持后续提示文本及 imageUuid、primaryStorageUuid
参数顺序不变，确保错误码和格式化信息正确对应。
- Around line 1402-1454: 统一 ExternalPrimaryStorage 中 statsCompletion.fail
的资源缺失判断，不要只匹配 SysErrors.RESOURCE_NOT_FOUND；让 ZbsStorageController.httpCall 和
ExponStorageController.stats/errorOut 将底层资源不存在统一归类为可触发缓存删除重建的错误，同时保留瞬时失败直接返回给
completion 的处理。
---
Nitpick comments:
In `@plugin/expon/src/main/java/org/zstack/expon/ExponStorageController.java`:
- Around line 1149-1172: 补齐 Expon 的统计结果父资源信息：更新 ExponStorageController.stats 及其
getVolumeStats 分支，为卷和快照统计结果设置可供通用父链解析的 parentUri 或等价父资源标识；若 Expon 的 VolumeModule
和 VolumeSnapshotModule 无法提供该标识，则在相关统计结果处理路径中明确声明不支持祖先链，避免返回看似可解析但实际断链的数据。
🪄 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: 92a82b00-c5f9-4f41-8382-38198fb667af
📒 Files selected for processing (10)
header/src/main/java/org/zstack/header/storage/addon/StorageResource.javaheader/src/main/java/org/zstack/header/storage/addon/primary/PrimaryStorageControllerSvc.javaheader/src/main/java/org/zstack/header/storage/snapshot/VolumeSnapshotStats.javaheader/src/main/java/org/zstack/header/volume/VolumeStats.javaplugin/expon/src/main/java/org/zstack/expon/ExponStorageController.javaplugin/xinfini/src/main/java/org/zstack/xinfini/XInfiniApiHelper.javaplugin/xinfini/src/main/java/org/zstack/xinfini/XInfiniStorageController.javaplugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.javastorage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.javatest/src/test/groovy/org/zstack/test/integration/storage/primary/addon/xinfini/XinfiniPrimaryStorageCase.groovy
| public void stats(String installPath, ReturnValueCompletion<StorageResource> comp) { | ||
| if (installPath.contains("@")) { | ||
| VolumeSnapshotModule snapshot = apiHelper.getVolumeSnapshot(getSnapIdFromPath(installPath)); | ||
| VolumeSnapshotStats stats = new VolumeSnapshotStats(); | ||
| long size = SizeUnit.MEGABYTE.toByte(snapshot.getSpec().getSizeMb()); | ||
| stats.setInstallPath(installPath); | ||
| stats.setSize(size); | ||
| stats.setActualSize(size); | ||
| stats.setParentUri(getParentUri(apiHelper.getVolume(snapshot.getSpec().getBsVolumeId()))); | ||
| comp.success(stats); | ||
| return; | ||
| } | ||
|
|
||
| comp.success(getVolumeStats(installPath)); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'class VolumeSnapshotModule|class VolumeSnapshotSpec' plugin/xinfini/src/main/java/org/zstack/xinfini
rg -n 'getDataSize|getAllocatedSize|DataSize' plugin/xinfini/src/main/java/org/zstack/xinfiniRepository: MatheMatrix/zstack
Length of output: 1490
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant methods and surrounding context
sed -n '840,900p' plugin/xinfini/src/main/java/org/zstack/xinfini/XInfiniStorageController.java
echo '--- getParentUri / buildXInfiniPath / getParentChain ---'
rg -n 'getParentUri\\(|buildXInfiniPath\\(|getParentChain\\(' plugin/xinfini/src/main/java/org/zstack/xinfini -A 12 -B 6
echo '--- VolumeSnapshotModule / VolumeSnapshotSpec ---'
sed -n '1,180p' plugin/xinfini/src/main/java/org/zstack/xinfini/sdk/volume/VolumeSnapshotModule.javaRepository: MatheMatrix/zstack
Length of output: 3134
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'buildXInfiniSnapshotPath|getParentChain|parentUri' plugin/xinfini/src/main/java/org/zstack/xinfini -A 6 -B 6
echo '--- VolumeSnapshotStats definition ---'
sed -n '1,220p' plugin/xinfini/src/main/java/org/zstack/xinfini/sdk/volume/VolumeSnapshotStats.javaRepository: MatheMatrix/zstack
Length of output: 5606
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find every consumer of parentUri and the stats classes involved.
rg -n 'getParentUri\\(|setParentUri\\(|parentUri' plugin/xinfini/src/main/java -A 4 -B 4
echo '--- stats interfaces / models ---'
rg -n 'class .*Stats|interface .*Stats|extends .*Stats|implements .*Stats' plugin/xinfini/src/main/java/org/zstack/xinfini -A 3 -B 3Repository: MatheMatrix/zstack
Length of output: 279
快照统计的 parentUri 需要直接指向 backing volume
stats() 这里把 snapshot.getSpec().getBsVolumeId() 对应的 volume 再传给 getParentUri()，拿到的是该 volume 的父快照路径，结果会跳过当前快照的直接父节点。这里应改为返回 backing volume 自身的路径。
🤖 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
`@plugin/xinfini/src/main/java/org/zstack/xinfini/XInfiniStorageController.java`
around lines 854 - 868, Update the snapshot branch of
XInfiniStorageController.stats so parentUri points directly to the backing
volume identified by snapshot.getSpec().getBsVolumeId(), rather than passing
that volume through getParentUri. Preserve the existing size and installPath
handling.
| if (cache.getInstallUrl().startsWith(ImageConstant.SNAPSHOT_REUSE_IMAGE_SCHEMA)) { | ||
| completion.fail(operr("image cache[imageUuid:%s, primaryStorageUuid:%s] refers to a missing reused volume snapshot", | ||
| cache.getImageUuid(), cache.getPrimaryStorageUuid())); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'public static ErrorCode operr' core/src/main/java/org/zstack/core/Platform.javaRepository: MatheMatrix/zstack
Length of output: 357
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== ExternalPrimaryStorage excerpt ==="
sed -n '1418,1442p' storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java
echo
echo "=== operr implementation ==="
sed -n '1738,1760p' core/src/main/java/org/zstack/core/Platform.java
echo
echo "=== nearby operr usages in ExternalPrimaryStorage ==="
rg -n 'operr\(' storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.javaRepository: MatheMatrix/zstack
Length of output: 4107
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'ORG_ZSTACK_STORAGE_ADDON_PRIMARY_10041|ORG_ZSTACK_STORAGE_ADDON_PRIMARY_1004[0-9]|ORG_ZSTACK_STORAGE_ADDON_PRIMARY_100[0-9]+' storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.javaRepository: MatheMatrix/zstack
Length of output: 1466
operr(...) 少传了全局错误码参数。
这里把提示文本当成了 globalErrorCode，后面的 %s 也不会按预期格式化；最终错误码和错误信息都会错位。按本类其它调用补上一个模块内错误码常量即可。
🤖 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 1432 - 1436, 修正 ExternalPrimaryStorage 中该 completion.fail 的 operr
调用，补上本模块现有的全局错误码常量作为首个参数，并保持后续提示文本及 imageUuid、primaryStorageUuid
参数顺序不变，确保错误码和格式化信息正确对应。
MatheMatrix
force-pushed
the
sync/jin.ma/fix/ZSTAC-86038-storage-resource-stats
branch
from
d5c3054 to
5b28e20
Compare
Generalize stats for volume and snapshot resources and rely on AsyncSafeAspect for completion errors. Resolves: ZSTAC-86038 Change-Id: I8660e9884c76cac94aedc8736f99b488779b7897
MatheMatrix
force-pushed
the
sync/jin.ma/fix/ZSTAC-86038-storage-resource-stats
branch
from
5b28e20 to
b808517
Compare
2 participants
问题
外部主存储的
stats只能返回VolumeStats，无法同时表达卷和快照，导致镜像缓存快照缺失时不能通过已有 stats 逻辑正确识别并重建。修改
stats，返回抽象StorageResourceVolumeStats、VolumeSnapshotStats作为子类，共享 installPath/size/actualSize/parentUri验证
mvn -Dmaven.test.skip=true clean install：56/56 modules successXinfiniPrimaryStorageCase：1 passed, 0 failed/error/skippedZbsPrimaryStorageCase：1 passed, 0 failed/error/skippedStorageResource，AJ weaving 生效Jira: http://jira.zstack.io/browse/ZSTAC-86038
sync from gitlab !10512