<fix>[storage]: ZSTAC-84163 local PS capacity excludes non-local data disk#4364
Open
ZStack-Robot wants to merge 1 commit into
Open
<fix>[storage]: ZSTAC-84163 local PS capacity excludes non-local data disk#4364ZStack-Robot wants to merge 1 commit into
ZStack-Robot wants to merge 1 commit into
Conversation
LocalStorageAllocatorFactory summed root+all data disks against local host capacity, wrongly raising NO_AVAILABLE_HOST in mixed storage where data disks land on NFS. Carry localStorageDiskSize (root + data only when data PS is local) so the local check counts only volumes on that local PS. Test CreateVmDataDiskOnNfsLocalCapacityCase covers it. Resolves: ZSTAC-84163 Change-Id: I3908f006e8aaf04ee9ee57d81fc09fabeda1c197
|
Warning
|
| Layer / File(s) | Summary |
|---|---|
AllocateHostMsg 与 HostAllocatorSpec 新增字段 header/.../AllocateHostMsg.java, header/.../HostAllocatorSpec.java |
新增 localStorageDiskSize 私有字段及其 getter/setter；HostAllocatorSpec.fromAllocationMsg 同步从消息中赋值；getter 在字段为正时返回自身值，否则回退到 diskSize。 |
VmAllocateHostFlow 容量计算拆分 compute/.../VmAllocateHostFlow.java |
新增 getDataDiskSizeOnLocalStorage 方法，根据数据卷所在主存储类型决定是否计入本地容量；prepareMsg 拆分 rootDiskSize，分别设置 msg.setDiskSize（总量）与 msg.setLocalStorageDiskSize（仅本地部分）。 |
LocalStorageAllocatorFactory 使用 localStorageDiskSize plugin/localstorage/.../LocalStorageAllocatorFactory.java |
filterHostCandidates 中引入 requiredSize = spec.getLocalStorageDiskSize()，将容量校验、debug 日志和异常消息统一替换为该值，不再直接使用 spec.getDiskSize()。 |
集成回归测试 test/.../CreateVmDataDiskOnNfsLocalCapacityCase.groovy |
新增测试用例，搭建本地（50G）+ NFS（1000G）双存储环境，创建根卷在本地（40G）、数据卷在 NFS（80G×2）的 VM，断言不产生 NO_AVAILABLE_HOST 错误。 |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
🐇 兔子跑过存储园，
根盘落在本地间，
数据盘飞去 NFS 天，
容量计算不再乱，
NO_AVAILABLE_HOST 烟消散～
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% 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 | 标题准确概括了本次变更：本地存储容量统计排除了非本地数据盘。 |
| Description check | ✅ Passed | 描述与代码变更一致，清楚说明了问题、修复思路、改动点和测试结果。 |
| 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 docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
sync/jin.ma/fix/ZSTAC-84163
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: 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 `@compute/src/main/java/org/zstack/compute/vm/VmAllocateHostFlow.java`:
- Around line 66-98: In VmAllocateHostFlow.prepareMsg(), the
localStorageDiskSize calculation is still using
getDataDiskSizeOnLocalStorage(spec), which only checks
spec.getRequiredPrimaryStorageUuidForDataVolume() and cannot handle
per-data-disk primary storage selection. Update the logic to inspect each data
disk’s target PrimaryStorageUuid individually (including the
APICreateVmInstanceMsg.DiskAO.primaryStorageUuid path already supported
elsewhere in this flow) and sum only the portions that are actually on
LOCAL_STORAGE_TYPE before setting msg.setLocalStorageDiskSize().
🪄 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: 15c878d3-ff36-486e-b0d5-c468d23f763c
📒 Files selected for processing (5)
compute/src/main/java/org/zstack/compute/vm/VmAllocateHostFlow.javaheader/src/main/java/org/zstack/header/allocator/AllocateHostMsg.javaheader/src/main/java/org/zstack/header/allocator/HostAllocatorSpec.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageAllocatorFactory.javatest/src/test/groovy/org/zstack/test/integration/storage/primary/local_nfs/allocator/host/CreateVmDataDiskOnNfsLocalCapacityCase.groovy
Comment on lines
+66
to
+98
| private long getDataDiskSizeOnLocalStorage(VmInstanceSpec spec) { | ||
| String dataPs = spec.getRequiredPrimaryStorageUuidForDataVolume(); | ||
| if (dataPs == null) { | ||
| return getTotalDataDiskSize(spec); | ||
| } | ||
| boolean dataOnLocal = PrimaryStorageConstants.LOCAL_STORAGE_TYPE.equals( | ||
| org.zstack.core.db.Q.New(PrimaryStorageVO.class) | ||
| .eq(PrimaryStorageVO_.uuid, dataPs) | ||
| .select(PrimaryStorageVO_.type) | ||
| .findValue()); | ||
| return dataOnLocal ? getTotalDataDiskSize(spec) : 0; | ||
| } | ||
|
|
||
| protected AllocateHostMsg prepareMsg(VmInstanceSpec spec) { | ||
| DesignatedAllocateHostMsg msg = new DesignatedAllocateHostMsg(); | ||
|
|
||
| List<DiskOfferingInventory> diskOfferings = new ArrayList<>(); | ||
| ImageInventory image = spec.getImageSpec().getInventory(); | ||
| long diskSize; | ||
| long rootDiskSize; | ||
| if (image == null || (image.getMediaType() != null && image.getMediaType().equals(ImageMediaType.ISO.toString()))) { | ||
| DiskOfferingVO dvo = dbf.findByUuid(spec.getRootDiskOffering().getUuid(), DiskOfferingVO.class); | ||
| diskSize = dvo.getDiskSize(); | ||
| rootDiskSize = dvo.getDiskSize(); | ||
| diskOfferings.add(DiskOfferingInventory.valueOf(dvo)); | ||
| } else { | ||
| diskSize = image.getSize(); | ||
| rootDiskSize = image.getSize(); | ||
| } | ||
| diskSize += getTotalDataDiskSize(spec); | ||
| long diskSize = rootDiskSize + getTotalDataDiskSize(spec); | ||
| diskOfferings.addAll(spec.getDataDiskOfferings()); | ||
| msg.setSoftAvoidHostUuids(spec.getSoftAvoidHostUuids()); | ||
| msg.setAvoidHostUuids(spec.getAvoidHostUuids()); | ||
| msg.setDiskOfferings(diskOfferings); | ||
| msg.setDiskSize(diskSize); | ||
| msg.setLocalStorageDiskSize(rootDiskSize + getDataDiskSizeOnLocalStorage(spec)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
localStorageDiskSize 仍然漏掉了“按数据盘分别选主存储”的路径。
Line 67 这里只看单个 spec.getRequiredPrimaryStorageUuidForDataVolume()，但同一个 prepareMsg() 在 Line 139-154 已经支持多候选 data PS 以及 APICreateVmInstanceMsg.DiskAO.primaryStorageUuid。这样一来，只要一次创建里部分数据盘落在 local、部分落在 NFS，这里的本地容量还是会被按“全算/全不算”处理，继续把 mixed-storage 请求误判为 NO_AVAILABLE_HOST（或反向少算）。这里需要按每块数据盘的目标 PS 逐个累加本地容量，而不是依赖单个 data-volume PS 提示。
🤖 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 `@compute/src/main/java/org/zstack/compute/vm/VmAllocateHostFlow.java` around
lines 66 - 98, In VmAllocateHostFlow.prepareMsg(), the localStorageDiskSize
calculation is still using getDataDiskSizeOnLocalStorage(spec), which only
checks spec.getRequiredPrimaryStorageUuidForDataVolume() and cannot handle
per-data-disk primary storage selection. Update the logic to inspect each data
disk’s target PrimaryStorageUuid individually (including the
APICreateVmInstanceMsg.DiskAO.primaryStorageUuid path already supported
elsewhere in this flow) and sum only the portions that are actually on
LOCAL_STORAGE_TYPE before setting msg.setLocalStorageDiskSize().
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.
Summary
创建云主机携带数据盘时，LocalStorageAllocatorFactory 容量检查使用根盘+全部数据盘总大小，混合存储场景（根盘 local、数据盘落 NFS）误判 local 容量不足。
Root Cause
spec.getDiskSize()累加根盘+所有数据盘，但数据盘可能落非 local 存储。新增localStorageDiskSize：仅累加将分配到该 local PS 的卷（根盘 + 目标 PS 为 local 的数据盘），其余不计入。Changes
Testing
Resolves: ZSTAC-84163
sync from gitlab !10324