<fix>[storage]: exclude deleted volumes from recalculate and sync physical capacity on reconnect#3677
Open
zstack-robot-2 wants to merge 6 commits intomasterfrom
Open
<fix>[storage]: exclude deleted volumes from recalculate and sync physical capacity on reconnect#3677zstack-robot-2 wants to merge 6 commits intomasterfrom
zstack-robot-2 wants to merge 6 commits intomasterfrom
Conversation
…sical capacity on reconnect Resolves: ZSTAC-80937 Change-Id: I5380a3cb223c675c20418831fffe5c107089b5ea
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough连接成功时发送的容量消息由 Changes
sequenceDiagram
participant Client as "请求/调度"
participant VolumeService as "VolumeBase"
participant Backend as "PrimaryStorage Backend"
participant MessageBus as "MessageBus"
participant PrimaryStorage as "PrimaryStorage(处理容量消息)"
participant DB as "数据库"
Note left of VolumeService: syncVolumeVolumeSize 高层流程
Client->>VolumeService: 触发 syncVolumeSize(volumeUuid)
VolumeService->>Backend: 查询卷实际 size
Backend-->>VolumeService: 返回 actualSize (newSize)
VolumeService->>DB: 读取 VolumeVO(oldSize)
DB-->>VolumeService: 返回 oldSize
alt newSize > oldSize
VolumeService->>MessageBus: 发送 DecreasePrimaryStorageCapacityMsg(diskSize = newSize - oldSize)
else newSize < oldSize
VolumeService->>MessageBus: 发送 IncreasePrimaryStorageCapacityMsg(diskSize = oldSize - newSize)
end
MessageBus->>PrimaryStorage: 转发容量调整消息
PrimaryStorage->>DB: 更新 PrimaryStorageCapacityVO.availableCapacity
DB-->>PrimaryStorage: 持久化完成
VolumeService->>DB: 更新并持久化 VolumeVO.size = newSize
DB-->>VolumeService: 持久化完成
代码审查工作量评估🎯 4 (Complex) | ⏱️ ~45 分钟 庆祝诗
🚥 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 |
…g handler Resolves: ZSTAC-80789 Change-Id: Ic9d89a477688248e24ba9099e8ca198cb04c9d92
…during syncVolumeSize Resolves: ZSTAC-80937 Change-Id: Idb146c176e50dd3748ae408c0ae1dab5a999935c
storage Resolves: ZSTAC-80937 Change-Id: Ie3a540bd1091e143c62dc4a3cc0c9557cc27e078
![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 the current code and only fix it if needed.
Inline comments:
In
`@storage/src/test/java/org/zstack/storage/addon/primary/ExternalPrimaryStorageSelectBsTest.java`:
- Line 47: Rename the test method testDefensiveCopyPreventsBeaMutation to
testDefensiveCopyPreventsBeanMutation to correct the typo; update the method
declaration in ExternalPrimaryStorageSelectBsTest (and any internal references
or reflective usages in the same file) so all occurrences match the new name and
the test continues to run under the framework.
🪄 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: 0d869a78-1918-47a5-80f8-2e95cbd964f0
⛔ Files ignored due to path filters (1)
storage/pom.xmlis excluded by!**/*.xml
📒 Files selected for processing (4)
storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.javastorage/src/main/java/org/zstack/storage/volume/VolumeBase.javastorage/src/test/java/org/zstack/storage/addon/primary/ExternalPrimaryStorageSelectBsTest.javatest/src/test/groovy/org/zstack/test/integration/storage/primary/nfs/capacity/NfsSyncVolumeSizeCapacityCase.groovy
✅ Files skipped from review due to trivial changes (1)
- storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java
| } | ||
|
|
||
| @Test | ||
| public void testDefensiveCopyPreventsBeaMutation() { |
There was a problem hiding this comment.
测试方法名有拼写错误，建议改为 Bean。
Line [47] 的 testDefensiveCopyPreventsBeaMutation 中 Bea 应为 Bean，建议修正以提升可读性和检索一致性。
✏️ 建议修改
- public void testDefensiveCopyPreventsBeaMutation() {
+ public void testDefensiveCopyPreventsBeanMutation() {As per coding guidelines, “命名应尽量用完整的单词组合表达意图”。
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public void testDefensiveCopyPreventsBeaMutation() { | |
| public void testDefensiveCopyPreventsBeanMutation() { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@storage/src/test/java/org/zstack/storage/addon/primary/ExternalPrimaryStorageSelectBsTest.java`
at line 47, Rename the test method testDefensiveCopyPreventsBeaMutation to
testDefensiveCopyPreventsBeanMutation to correct the typo; update the method
declaration in ExternalPrimaryStorageSelectBsTest (and any internal references
or reflective usages in the same file) so all occurrences match the new name and
the test continues to run under the framework.
Resolves: ZSTAC-80937 Change-Id: I7566666e79706c6c66726f616d706965687a7474
Resolves: ZSTAC-80937 Change-Id: I7566666e79706c6c66726f616d706965687a7474 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
storage/src/main/java/org/zstack/storage/volume/VolumeBase.java (1)
2172-2215:⚠️ Potential issue | 🔴 Critical避免并发
syncVolumeSize把同一笔容量变化重复结算。Line 2172 先缓存了
oldSize，但这条路径本身没有像其他 volume 变更一样走syncThreadId串行化。这样两个并发的 sync 都可能拿到同一个旧值，并在 Line 2200 各自发送一次容量增减消息，最终把一次 size 变化重复记到主存储 availableCapacity 上。建议把
syncVolumeVolumeSize()的落库和容量调整放进按 volume 串行的 task 里，或者至少基于数据库当前size做原子 compare-and-update 后再发送 delta 消息。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/src/main/java/org/zstack/storage/volume/VolumeBase.java` around lines 2172 - 2215, Concurrent syncVolume operations read the cached oldSize and can both send duplicate primary-storage capacity deltas; in VolumeBase (the syncVolumeSize/syncVolumeVolumeSize path around refreshVO(), self.setSize(), and the DecreasePrimaryStorageCapacityMsg/IncreasePrimaryStorageCapacityMsg sends) either (A) move the DB write of the new size and the logic that sends the capacity adjustment into the volume-level serialized task (use the same syncThreadId/serialization used by other volume updates), or (B) perform an atomic compare-and-update against the database current size before sending a delta: read current size from DB, compute sizeDiff = newSize - currentDbSize, do an UPDATE ... WHERE uuid=? AND size=? to CAS the size, and only if the update affected a row send the corresponding DecreasePrimaryStorageCapacityMsg/IncreasePrimaryStorageCapacityMsg; apply this change around the code that currently caches oldSize and sends the bus messages to avoid double-counting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@storage/src/main/java/org/zstack/storage/volume/VolumeBase.java`:
- Around line 2172-2215: Concurrent syncVolume operations read the cached
oldSize and can both send duplicate primary-storage capacity deltas; in
VolumeBase (the syncVolumeSize/syncVolumeVolumeSize path around refreshVO(),
self.setSize(), and the
DecreasePrimaryStorageCapacityMsg/IncreasePrimaryStorageCapacityMsg sends)
either (A) move the DB write of the new size and the logic that sends the
capacity adjustment into the volume-level serialized task (use the same
syncThreadId/serialization used by other volume updates), or (B) perform an
atomic compare-and-update against the database current size before sending a
delta: read current size from DB, compute sizeDiff = newSize - currentDbSize, do
an UPDATE ... WHERE uuid=? AND size=? to CAS the size, and only if the update
affected a row send the corresponding
DecreasePrimaryStorageCapacityMsg/IncreasePrimaryStorageCapacityMsg; apply this
change around the code that currently caches oldSize and sends the bus messages
to avoid double-counting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 44baf68a-ae08-4124-9811-21082b930e7f
⛔ Files ignored due to path filters (1)
docs/cache/fix-artifacts/ZSTAC-80937/phase-5.jsonis excluded by!**/*.json
📒 Files selected for processing (1)
storage/src/main/java/org/zstack/storage/volume/VolumeBase.java
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.
Root Cause
PrimaryStorageCapacityRecalculator.recalculate() 第78行将 VolumeStatus.Deleted 计入 needCountVolumeStates，Deleted 卷在删除时已归还虚拟容量，但 recalculate() 又将其计入占用，导致双重扣减，存储池分配率异常增长。
次要问题：doConnect() 成功后直接发 RecalculatePrimaryStorageCapacityMsg，未先同步物理容量。
Solution
Testing
Jira
Resolves: ZSTAC-80937
sync from gitlab !9538