<fix>[vm]: filter zbs for encrypted auto ps#4464
Open
zstack-robot-2 wants to merge 1 commit into
Open
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 13 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 ignored due to path filters (1)
📒 Files selected for processing (15)
Walkthrough新增 Changes加密卷主存储分配
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Flow as VM allocation flow
participant Allocator as EncryptedVolumePrimaryStorageAllocatorExtension
participant StorageDB as ExternalPrimaryStorageVO
participant CloudBus
Flow->>Allocator: 过滤 root/data 主存储候选
Allocator->>StorageDB: 查询不支持加密卷的外部主存储
StorageDB-->>Allocator: 返回主存储 UUID
Allocator-->>Flow: 返回过滤后的候选
Flow->>CloudBus: 发送主存储分配请求
CloudBus-->>Flow: 返回成功或无候选失败
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
test/src/test/java/org/zstack/test/unittest/storage/encrypt/EncryptedVolumePrimaryStorageAllocatorExtensionTest.java (1)
15-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value建议补充
filterPrimaryStorageCandidates与shouldAllocatePrimaryStorageForTemplateDataVolume的用例。当前仅覆盖
beforeAllocatePrimaryStorage。另外两个 override 方法涉及 DB 查询分支与短路逻辑，是本次过滤行为的核心路径，建议补充测试（可考虑 mockQ）以覆盖候选过滤与模板数据卷分配判定。🤖 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 `@test/src/test/java/org/zstack/test/unittest/storage/encrypt/EncryptedVolumePrimaryStorageAllocatorExtensionTest.java` around lines 15 - 36, Current tests only cover beforeAllocatePrimaryStorage, but the core filtering logic also lives in filterPrimaryStorageCandidates and shouldAllocatePrimaryStorageForTemplateDataVolume. Add test cases for these two methods in EncryptedVolumePrimaryStorageAllocatorExtensionTest, including the DB-query branch in filterPrimaryStorageCandidates and the short-circuit behavior in shouldAllocatePrimaryStorageForTemplateDataVolume; use mocking for Q where needed so both candidate filtering and template data volume allocation decisions are verified.compute/src/main/java/org/zstack/compute/vm/VmAllocatePrimaryStorageFlow.java (1)
258-264: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff扩展点调用循环在多个 Flow 中重复。
相同的
pluginRgty.getExtensionList(VmAllocatePrimaryStorageExtensionPoint.class)遍历回调逻辑在VmAllocatePrimaryStorageFlow、VmInstantiateOtherDiskFlow（beforeAllocatePrimaryStorage）以及VmAllocateHostAndPrimaryStorageFlow（filterPrimaryStorageCandidates）中重复出现。可考虑参照 ZStack 既有的*ExtensionPointEmitter模式抽出统一的 emitter，减少重复并集中管理契约。🤖 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/VmAllocatePrimaryStorageFlow.java` around lines 258 - 264, The extension-point iteration logic for VmAllocatePrimaryStorageExtensionPoint is duplicated across multiple flows, so extract it into a shared emitter following the existing *ExtensionPointEmitter pattern. Create a centralized emitter for the pluginRgty.getExtensionList(VmAllocatePrimaryStorageExtensionPoint.class) callback sequence and use it from VmAllocatePrimaryStorageFlow.beforeAllocatePrimaryStorage, VmInstantiateOtherDiskFlow.beforeAllocatePrimaryStorage, and VmAllocateHostAndPrimaryStorageFlow.filterPrimaryStorageCandidates to keep the contract in one place and remove repeated loops.header/src/main/java/org/zstack/header/vm/VmAllocatePrimaryStorageExtensionPoint.java (1)
8-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win建议在扩展点接口上明确
spec/disk的可空契约。当前多个调用方会传入 null：
VmInstantiateOtherDiskFlow传入spec=null，而VmAllocatePrimaryStorageFlow/VmAllocateHostAndPrimaryStorageFlow在自动分配分支可能传入disk=null（getRootDisk()可能为 null）。默认实现不解引用这些参数因而安全，但后续新增的实现容易在未察觉的情况下触发 NPE。建议在 Javadoc 中说明各参数何时可能为 null，提示实现方做空值防护。🤖 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 `@header/src/main/java/org/zstack/header/vm/VmAllocatePrimaryStorageExtensionPoint.java` around lines 8 - 18, Clarify the nullable contract for the VmAllocatePrimaryStorageExtensionPoint methods so implementers know that spec and disk may be null in some call paths. Update the interface Javadoc around beforeAllocatePrimaryStorage, filterPrimaryStorageCandidates, and shouldAllocatePrimaryStorageForTemplateDataVolume to state when VmInstanceSpec or DiskAO can be absent, referencing the extension point methods directly so future implementations add null checks and avoid NPEs.storage/src/main/java/org/zstack/storage/volume/InstantiateVolumeForNewCreatedVmExtension.java (2)
307-315: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value可用 Stream
anyMatch替代显式循环。该判断逻辑适合用 Stream 表达。As per coding guidelines：“使用 Java Stream 或 Lambda 表达式代替冗长的循环与条件判断”。
♻️ 建议的写法
- private boolean shouldAllocatePrimaryStorageForTemplateDataVolume(VmInstanceSpec spec, DiskAO diskAO, String defaultPsUuid) { - for (VmAllocatePrimaryStorageExtensionPoint ext : - pluginRgty.getExtensionList(VmAllocatePrimaryStorageExtensionPoint.class)) { - if (ext.shouldAllocatePrimaryStorageForTemplateDataVolume(spec, diskAO, defaultPsUuid)) { - return true; - } - } - return false; - } + private boolean shouldAllocatePrimaryStorageForTemplateDataVolume(VmInstanceSpec spec, DiskAO diskAO, String defaultPsUuid) { + return pluginRgty.getExtensionList(VmAllocatePrimaryStorageExtensionPoint.class).stream() + .anyMatch(ext -> ext.shouldAllocatePrimaryStorageForTemplateDataVolume(spec, diskAO, defaultPsUuid)); + }🤖 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/volume/InstantiateVolumeForNewCreatedVmExtension.java` around lines 307 - 315, Replace the explicit loop in shouldAllocatePrimaryStorageForTemplateDataVolume with a Stream anyMatch chain over pluginRgty.getExtensionList(VmAllocatePrimaryStorageExtensionPoint.class), using the same predicate ext.shouldAllocatePrimaryStorageForTemplateDataVolume(spec, diskAO, defaultPsUuid) to preserve behavior while following the Stream/Lambda guideline.Source: Path instructions
276-278: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value建议简化嵌套三元表达式以提升可读性。
psUuid使用了嵌套三元，语义不直观。可改用早返回或普通条件赋值表达清楚意图。As per coding guidelines：“if 条件表达不宜过长或过于复杂，必要时可以将条件抽成 boolean 变量描述”，并尽量减少难以阅读的分支结构。♻️ 建议的写法
- boolean allocatePrimaryStorage = shouldAllocatePrimaryStorageForTemplateDataVolume(spec, diskAO, defaultPsUuid); - String psUuid = diskAO.getPrimaryStorageUuid() != null ? diskAO.getPrimaryStorageUuid() : - allocatePrimaryStorage ? null : defaultPsUuid; + boolean allocatePrimaryStorage = shouldAllocatePrimaryStorageForTemplateDataVolume(spec, diskAO, defaultPsUuid); + String psUuid; + if (diskAO.getPrimaryStorageUuid() != null) { + psUuid = diskAO.getPrimaryStorageUuid(); + } else { + psUuid = allocatePrimaryStorage ? null : defaultPsUuid; + }🤖 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/volume/InstantiateVolumeForNewCreatedVmExtension.java` around lines 276 - 278, The psUuid assignment in InstantiateVolumeForNewCreatedVmExtension uses a nested ternary that is hard to read; replace it with clearer conditional logic, such as an explicit if/else or early-return style, while preserving the same behavior based on diskAO.getPrimaryStorageUuid(), allocatePrimaryStorage, and defaultPsUuid. Keep the boolean from shouldAllocatePrimaryStorageForTemplateDataVolume() as the descriptive condition and use it to make the storage selection flow obvious.Source: Path instructions
🤖 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.
Nitpick comments:
In
`@compute/src/main/java/org/zstack/compute/vm/VmAllocatePrimaryStorageFlow.java`:
- Around line 258-264: The extension-point iteration logic for
VmAllocatePrimaryStorageExtensionPoint is duplicated across multiple flows, so
extract it into a shared emitter following the existing *ExtensionPointEmitter
pattern. Create a centralized emitter for the
pluginRgty.getExtensionList(VmAllocatePrimaryStorageExtensionPoint.class)
callback sequence and use it from
VmAllocatePrimaryStorageFlow.beforeAllocatePrimaryStorage,
VmInstantiateOtherDiskFlow.beforeAllocatePrimaryStorage, and
VmAllocateHostAndPrimaryStorageFlow.filterPrimaryStorageCandidates to keep the
contract in one place and remove repeated loops.
In
`@header/src/main/java/org/zstack/header/vm/VmAllocatePrimaryStorageExtensionPoint.java`:
- Around line 8-18: Clarify the nullable contract for the
VmAllocatePrimaryStorageExtensionPoint methods so implementers know that spec
and disk may be null in some call paths. Update the interface Javadoc around
beforeAllocatePrimaryStorage, filterPrimaryStorageCandidates, and
shouldAllocatePrimaryStorageForTemplateDataVolume to state when VmInstanceSpec
or DiskAO can be absent, referencing the extension point methods directly so
future implementations add null checks and avoid NPEs.
In
`@storage/src/main/java/org/zstack/storage/volume/InstantiateVolumeForNewCreatedVmExtension.java`:
- Around line 307-315: Replace the explicit loop in
shouldAllocatePrimaryStorageForTemplateDataVolume with a Stream anyMatch chain
over pluginRgty.getExtensionList(VmAllocatePrimaryStorageExtensionPoint.class),
using the same predicate
ext.shouldAllocatePrimaryStorageForTemplateDataVolume(spec, diskAO,
defaultPsUuid) to preserve behavior while following the Stream/Lambda guideline.
- Around line 276-278: The psUuid assignment in
InstantiateVolumeForNewCreatedVmExtension uses a nested ternary that is hard to
read; replace it with clearer conditional logic, such as an explicit if/else or
early-return style, while preserving the same behavior based on
diskAO.getPrimaryStorageUuid(), allocatePrimaryStorage, and defaultPsUuid. Keep
the boolean from shouldAllocatePrimaryStorageForTemplateDataVolume() as the
descriptive condition and use it to make the storage selection flow obvious.
In
`@test/src/test/java/org/zstack/test/unittest/storage/encrypt/EncryptedVolumePrimaryStorageAllocatorExtensionTest.java`:
- Around line 15-36: Current tests only cover beforeAllocatePrimaryStorage, but
the core filtering logic also lives in filterPrimaryStorageCandidates and
shouldAllocatePrimaryStorageForTemplateDataVolume. Add test cases for these two
methods in EncryptedVolumePrimaryStorageAllocatorExtensionTest, including the
DB-query branch in filterPrimaryStorageCandidates and the short-circuit behavior
in shouldAllocatePrimaryStorageForTemplateDataVolume; use mocking for Q where
needed so both candidate filtering and template data volume allocation decisions
are verified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ecc85355-2f22-4d1d-8987-4cf43115be05
⛔ Files ignored due to path filters (1)
conf/springConfigXml/VolumeManager.xmlis excluded by!**/*.xml
📒 Files selected for processing (7)
compute/src/main/java/org/zstack/compute/vm/VmAllocateHostAndPrimaryStorageFlow.javacompute/src/main/java/org/zstack/compute/vm/VmAllocatePrimaryStorageFlow.javacompute/src/main/java/org/zstack/compute/vm/VmInstantiateOtherDiskFlow.javaheader/src/main/java/org/zstack/header/vm/VmAllocatePrimaryStorageExtensionPoint.javastorage/src/main/java/org/zstack/storage/encrypt/EncryptedVolumePrimaryStorageAllocatorExtension.javastorage/src/main/java/org/zstack/storage/volume/InstantiateVolumeForNewCreatedVmExtension.javatest/src/test/java/org/zstack/test/unittest/storage/encrypt/EncryptedVolumePrimaryStorageAllocatorExtensionTest.java
MatheMatrix
force-pushed
the
sync/zstackio/codex/filter-zhps-encrypted-auto-ps
branch
7 times, most recently
from
29fed71 to
478e59b
Compare
Owner
|
Comment from yaohua.wu: Review: MR !10426 — ZSV-12607Background (preserved across rounds)
🔴 Critical
🟡 Warning
🟢 Suggestion
Coverage
Verdict: REVISION_REQUIRED2 条 Critical 都是修复不完整路径，可能让 🤖 Robot Reviewer |
Collaborator
|
Comment from 周众: 已修复：attach 未实例化加密盘补充 encryptedVolumeAutoAllocation；模板数据盘加密且自动选择时不再继承 defaultPsUuid，改为先通过 allocator dry-run 选择非 ZHPS，再继续原创建流程。过滤逻辑仍集中在 PrimaryStorageMainAllocatorFlow 的 filterPrimaryStorageCandidates 扩展点。 |
MatheMatrix
force-pushed
the
sync/zstackio/codex/filter-zhps-encrypted-auto-ps
branch
5 times, most recently
from
2baeca6 to
0de0ff1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
compute/src/main/java/org/zstack/compute/vm/VmAllocatePrimaryStorageFlow.java (1)
149-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win加密自动分配判断逻辑在多个文件中重复。
Boolean.TRUE.equals(x.getEncrypted()) && x.getPrimaryStorageUuid() == null这一表达式在本文件（根卷/数据卷）、VmAllocatePrimaryStorageForAttachingDiskFlow、VmInstantiateOtherDiskFlow（两处）共出现 5 次。本 PR 的迭代历史本身就说明了这种重复的风险——review 过程中曾遗漏了附加磁盘与模板数据卷两处，需要额外补丁修复。建议抽取为统一的静态工具方法（例如EncryptedVolumeAllocationUtils.isEncryptedAutoAllocation(Boolean encrypted, String primaryStorageUuid)），避免未来新增分配入口再次遗漏。♻️ 建议的重构方向
+public class EncryptedVolumeAllocationUtils { + public static boolean isEncryptedAutoAllocation(Boolean encrypted, String primaryStorageUuid) { + return Boolean.TRUE.equals(encrypted) && primaryStorageUuid == null; + } +}- rmsg.setEncryptedVolumeAutoAllocation(disk != null && Boolean.TRUE.equals(disk.getEncrypted())); + rmsg.setEncryptedVolumeAutoAllocation(disk != null + && EncryptedVolumeAllocationUtils.isEncryptedAutoAllocation(disk.getEncrypted(), disk.getPrimaryStorageUuid()));As per path instructions（
## 6. 设计原则和模块划分中的"单一职责原则"），应避免重复逻辑分散在多处、造成后续维护和一致性风险。Also applies to: 184-186
🤖 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/VmAllocatePrimaryStorageFlow.java` at line 149, The encrypted auto-allocation check is duplicated across multiple VM primary-storage flows, so refactor it into one shared helper and reuse it everywhere. Extract the repeated Boolean.TRUE.equals(encrypted) && primaryStorageUuid == null logic into a static utility such as EncryptedVolumeAllocationUtils.isEncryptedAutoAllocation, then update VmAllocatePrimaryStorageFlow, VmAllocatePrimaryStorageForAttachingDiskFlow, and VmInstantiateOtherDiskFlow to call that helper instead of inlining the condition. Keep the existing behavior identical while centralizing the rule to prevent future misses in new allocation paths.Source: Path instructions
test/src/test/java/org/zstack/test/unittest/storage/encrypt/EncryptedVolumePrimaryStorageAllocatorExtensionTest.java (1)
16-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win缺少"加密根卷 + 自动分配主存储"场景的用例。
现有 4 个测试仅覆盖了
PrimaryStorageAllocationSpec直接标记及VmInstanceSpec中非根盘（DiskAO.nonRootDisk()+ 模板盘）两类场景，EncryptedVolumePrimaryStorageAllocatorExtension.isEncryptedAutoAllocatedRootVolume（根卷加密且getCandidatePrimaryStorageUuidsForRootVolume()为空）这条分支未被任何用例触发。考虑到根卷是加密自动分配最常见的入口之一，建议补充一个设置spec.getRootDisk()为加密盘、且未指定候选主存储的用例，以覆盖该分支并验证getCandidatePrimaryStorageUuidsForRootVolume()默认值的空安全性。✅ 建议补充的测试用例示例
`@Test` public void testFilterZhpsForEncryptedRootDiskWithAutoPrimaryStorage() { VmInstanceSpec spec = new VmInstanceSpec(); DiskAO rootDisk = DiskAO.rootDisk(); // 视实际工厂方法调整 rootDisk.setEncrypted(true); spec.setRootDisk(rootDisk); List<PrimaryStorageVO> candidates = new ArrayList<>(Arrays.asList(primaryStorage("normal-ps-uuid"), primaryStorage(ZHPS_PS_UUID))); extension.filterPrimaryStorageCandidates(spec, candidates); Assert.assertEquals(1, candidates.size()); Assert.assertEquals("normal-ps-uuid", candidates.get(0).getUuid()); }🤖 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 `@test/src/test/java/org/zstack/test/unittest/storage/encrypt/EncryptedVolumePrimaryStorageAllocatorExtensionTest.java` around lines 16 - 92, Add a test for the missing “encrypted root volume + auto primary storage” path in EncryptedVolumePrimaryStorageAllocatorExtensionTest. Introduce a case using VmInstanceSpec with an encrypted root disk via DiskAO.rootDisk() (or the equivalent root-disk factory in this test) and no explicit candidate primary storage, then call filterPrimaryStorageCandidates and assert ZHPS_PS_UUID is filtered out. This should directly exercise EncryptedVolumePrimaryStorageAllocatorExtension.isEncryptedAutoAllocatedRootVolume and verify getCandidatePrimaryStorageUuidsForRootVolume() is safely handled when unset.
🤖 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/encrypt/EncryptedVolumePrimaryStorageAllocatorExtension.java`:
- Around line 56-65: `hasEncryptedAutoAllocatedVolume(VmInstanceSpec)` is using
the VM-wide `candidatePrimaryStorageUuidsForDataVolume` field inside a per-disk
check, which can incorrectly skip ZBS filtering in multi-disk cases. Refactor
the logic in `hasEncryptedAutoAllocatedVolume` so each deprecated disk is
evaluated independently: keep checking `isEncrypted(disk)` and
`disk.getPrimaryStorageUuid() == null`, but move the candidate-primary-storage
condition to disk-specific handling instead of relying on
`spec.getCandidatePrimaryStorageUuidsForDataVolume()` globally. Ensure the
helper still correctly detects auto-allocated encrypted root volumes via
`isEncryptedAutoAllocatedRootVolume` and only lets disks through when that
specific disk’s primary-storage selection warrants it.
---
Nitpick comments:
In
`@compute/src/main/java/org/zstack/compute/vm/VmAllocatePrimaryStorageFlow.java`:
- Line 149: The encrypted auto-allocation check is duplicated across multiple VM
primary-storage flows, so refactor it into one shared helper and reuse it
everywhere. Extract the repeated Boolean.TRUE.equals(encrypted) &&
primaryStorageUuid == null logic into a static utility such as
EncryptedVolumeAllocationUtils.isEncryptedAutoAllocation, then update
VmAllocatePrimaryStorageFlow, VmAllocatePrimaryStorageForAttachingDiskFlow, and
VmInstantiateOtherDiskFlow to call that helper instead of inlining the
condition. Keep the existing behavior identical while centralizing the rule to
prevent future misses in new allocation paths.
In
`@test/src/test/java/org/zstack/test/unittest/storage/encrypt/EncryptedVolumePrimaryStorageAllocatorExtensionTest.java`:
- Around line 16-92: Add a test for the missing “encrypted root volume + auto
primary storage” path in EncryptedVolumePrimaryStorageAllocatorExtensionTest.
Introduce a case using VmInstanceSpec with an encrypted root disk via
DiskAO.rootDisk() (or the equivalent root-disk factory in this test) and no
explicit candidate primary storage, then call filterPrimaryStorageCandidates and
assert ZHPS_PS_UUID is filtered out. This should directly exercise
EncryptedVolumePrimaryStorageAllocatorExtension.isEncryptedAutoAllocatedRootVolume
and verify getCandidatePrimaryStorageUuidsForRootVolume() is safely handled when
unset.
🪄 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: 052ff466-0332-4f2a-b612-3063562652a8
⛔ Files ignored due to path filters (1)
conf/springConfigXml/VolumeManager.xmlis excluded by!**/*.xml
📒 Files selected for processing (11)
compute/src/main/java/org/zstack/compute/vm/VmAllocateHostAndPrimaryStorageFlow.javacompute/src/main/java/org/zstack/compute/vm/VmAllocatePrimaryStorageFlow.javacompute/src/main/java/org/zstack/compute/vm/VmAllocatePrimaryStorageForAttachingDiskFlow.javacompute/src/main/java/org/zstack/compute/vm/VmInstantiateOtherDiskFlow.javaheader/src/main/java/org/zstack/header/storage/primary/AllocatePrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/storage/primary/PrimaryStorageAllocationSpec.javaheader/src/main/java/org/zstack/header/vm/VmAllocatePrimaryStorageExtensionPoint.javastorage/src/main/java/org/zstack/storage/encrypt/EncryptedVolumePrimaryStorageAllocatorExtension.javastorage/src/main/java/org/zstack/storage/primary/PrimaryStorageMainAllocatorFlow.javastorage/src/main/java/org/zstack/storage/primary/PrimaryStorageManagerImpl.javatest/src/test/java/org/zstack/test/unittest/storage/encrypt/EncryptedVolumePrimaryStorageAllocatorExtensionTest.java
MatheMatrix
force-pushed
the
sync/zstackio/codex/filter-zhps-encrypted-auto-ps
branch
2 times, most recently
from
dd70533 to
0af5fe5
Compare
Owner
|
Comment from yaohua.wu: Review: MR !10426 — ZSV-12607Background
🔴 Critical
Coverage
Verdict: REVISION_REQUIRED当前实现会在混合 root/data 场景中过度过滤 ZHPS，导致合法的非加密卷放置被拒绝，需要按卷粒度修正后再合入。 🤖 Robot Reviewer |
MatheMatrix
force-pushed
the
sync/zstackio/codex/filter-zhps-encrypted-auto-ps
branch
10 times, most recently
from
2a98582 to
62968b9
Compare
MatheMatrix
force-pushed
the
sync/zstackio/codex/filter-zhps-encrypted-auto-ps
branch
3 times, most recently
from
8f57445 to
1c435db
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
compute/src/main/java/org/zstack/compute/vm/VmAllocatePrimaryStorageForAttachingDiskFlow.java (1)
66-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win逻辑正确，但与其他文件存在重复模式。
该「加密且未指定主存储则追加
ENCRYPTED_VOLUME特性」的判断逻辑，在VolumeManagerImpl.java（364-366行）及VmAllocatePrimaryStorageFlow.java中重复出现。建议抽取为AllocatePrimaryStorageMsg/AllocatePrimaryStorageSpaceMsg上的统一辅助方法，减少重复并降低未来修改遗漏的风险（详见文末合并意见）。🤖 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/VmAllocatePrimaryStorageForAttachingDiskFlow.java` around lines 66 - 68, 将“卷已加密且未指定主存储时追加 ENCRYPTED_VOLUME 特性”的重复判断抽取到 AllocatePrimaryStorageMsg 或 AllocatePrimaryStorageSpaceMsg 的统一辅助方法中；更新 VmAllocatePrimaryStorageForAttachingDiskFlow 及 VolumeManagerImpl、VmAllocatePrimaryStorageFlow 使用该方法，并移除各处重复条件逻辑，保持现有行为不变。test/src/test/groovy/org/zstack/test/integration/storage/primary/EncryptedVolumePrimaryStorageAllocationCase.groovy (1)
59-93: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win缺少针对 VM 级根/数据盘混合场景的回归测试。
当前测试仅覆盖了
AllocatePrimaryStorageSpaceMsg+PrimaryStorageFeatureAllocatorExtensionPoint.allocatePrimaryStorage这一条路径，未覆盖VmAllocatePrimaryStorageExtensionPoint.filterPrimaryStorageCandidates（VM 级 root/data 候选过滤）路径，也未覆盖 PR 目标中提到的混合场景（非加密根盘显式放在 ZBS，加密数据盘放在非 ZBS）。建议补充一个通过完整 VM 创建流程触发filterPrimaryStorageCandidates的集成测试用例，以验证 root 与 data 候选列表按卷角色独立过滤，而不是全部包含关系。🤖 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 `@test/src/test/groovy/org/zstack/test/integration/storage/primary/EncryptedVolumePrimaryStorageAllocationCase.groovy` around lines 59 - 93, 在现有测试类中新增一个通过完整 VM 创建流程触发 VmAllocatePrimaryStorageExtensionPoint.filterPrimaryStorageCandidates 的集成测试，覆盖非加密根盘显式放在 ZBS、加密数据盘放在非 ZBS 的混合场景。分别断言 root 与 data 卷候选按各自卷角色独立过滤，确保不会错误地使用所有卷候选的整体包含关系；保留现有 testFilterZhpsForEncryptedVolumeAutoAllocation 的覆盖范围不变。storage/src/main/java/org/zstack/storage/volume/VolumeManagerImpl.java (1)
364-366: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win逻辑正确，但与其他文件存在重复模式。
同
VmAllocatePrimaryStorageForAttachingDiskFlow.java66-68行的重复问题，见文末合并意见。🤖 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/volume/VolumeManagerImpl.java` around lines 364 - 366, 将 VolumeManagerImpl 中针对加密卷且未指定主存储时添加 PrimaryStorageFeature.ENCRYPTED_VOLUME 的逻辑，与 VmAllocatePrimaryStorageForAttachingDiskFlow 中的重复实现提取为共享方法或复用已有公共方法，并让两处调用该实现；保持现有判断条件和 amsg.addRequiredFeature 行为不变。
🤖 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/vm/VmAllocatePrimaryStorageExtensionPoint.java`:
- Around line 5-8: 为公共接口方法
VmAllocatePrimaryStorageExtensionPoint.filterPrimaryStorageCandidates 补充有效
Javadoc，说明其调用时机、spec 参数用途、rootPrimaryStorageUuids 与 dataPrimaryStorageUuids
分别表示的候选列表，以及实现应原地修改列表的约定。
In `@storage/src/main/java/org/zstack/storage/volume/VolumeBase.java`:
- Around line 3414-3420: 修改 refreshVO() 的异常处理：刷新失败时不要仅记录警告后继续执行
trigger.next()，应按现有流程重试刷新，或继续向上抛出异常使流程失败。只有 refreshVO() 成功后才允许调用
trigger.next()，确保后续结果不会使用过期的 self。
- Around line 3466-3473: 严格处理加密转换后的清理失败：更新
detachKeyProviderFromVolume、detachKeyProviderFromSnapshot 及 trash.createTrash
所在流程，失败时执行可重试处理或持久化待清理任务，不要仅记录日志后继续成功返回；确保密钥引用解绑和旧数据位回收状态可被可靠完成或追踪。
---
Nitpick comments:
In
`@compute/src/main/java/org/zstack/compute/vm/VmAllocatePrimaryStorageForAttachingDiskFlow.java`:
- Around line 66-68: 将“卷已加密且未指定主存储时追加 ENCRYPTED_VOLUME 特性”的重复判断抽取到
AllocatePrimaryStorageMsg 或 AllocatePrimaryStorageSpaceMsg 的统一辅助方法中；更新
VmAllocatePrimaryStorageForAttachingDiskFlow 及
VolumeManagerImpl、VmAllocatePrimaryStorageFlow 使用该方法，并移除各处重复条件逻辑，保持现有行为不变。
In `@storage/src/main/java/org/zstack/storage/volume/VolumeManagerImpl.java`:
- Around line 364-366: 将 VolumeManagerImpl 中针对加密卷且未指定主存储时添加
PrimaryStorageFeature.ENCRYPTED_VOLUME 的逻辑，与
VmAllocatePrimaryStorageForAttachingDiskFlow
中的重复实现提取为共享方法或复用已有公共方法，并让两处调用该实现；保持现有判断条件和 amsg.addRequiredFeature 行为不变。
In
`@test/src/test/groovy/org/zstack/test/integration/storage/primary/EncryptedVolumePrimaryStorageAllocationCase.groovy`:
- Around line 59-93: 在现有测试类中新增一个通过完整 VM 创建流程触发
VmAllocatePrimaryStorageExtensionPoint.filterPrimaryStorageCandidates
的集成测试，覆盖非加密根盘显式放在 ZBS、加密数据盘放在非 ZBS 的混合场景。分别断言 root 与 data
卷候选按各自卷角色独立过滤，确保不会错误地使用所有卷候选的整体包含关系；保留现有
testFilterZhpsForEncryptedVolumeAutoAllocation 的覆盖范围不变。
🪄 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: 4a5412c8-388d-4cc9-9877-e937c0d719d6
⛔ Files ignored due to path filters (1)
conf/springConfigXml/VolumeManager.xmlis excluded by!**/*.xml
📒 Files selected for processing (12)
compute/src/main/java/org/zstack/compute/vm/VmAllocateHostAndPrimaryStorageFlow.javacompute/src/main/java/org/zstack/compute/vm/VmAllocatePrimaryStorageFlow.javacompute/src/main/java/org/zstack/compute/vm/VmAllocatePrimaryStorageForAttachingDiskFlow.javacompute/src/main/java/org/zstack/compute/vm/VmInstantiateOtherDiskFlow.javaheader/src/main/java/org/zstack/header/storage/primary/AllocatePrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/storage/primary/PrimaryStorageFeature.javaheader/src/main/java/org/zstack/header/vm/VmAllocatePrimaryStorageExtensionPoint.javastorage/src/main/java/org/zstack/storage/encrypt/EncryptedVolumePrimaryStorageAllocatorExtension.javastorage/src/main/java/org/zstack/storage/volume/InstantiateVolumeForNewCreatedVmExtension.javastorage/src/main/java/org/zstack/storage/volume/VolumeBase.javastorage/src/main/java/org/zstack/storage/volume/VolumeManagerImpl.javatest/src/test/groovy/org/zstack/test/integration/storage/primary/EncryptedVolumePrimaryStorageAllocationCase.groovy
🚧 Files skipped from review as they are similar to previous changes (1)
- compute/src/main/java/org/zstack/compute/vm/VmInstantiateOtherDiskFlow.java
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🧹 Nitpick comments (3)
compute/src/main/java/org/zstack/compute/vm/VmAllocatePrimaryStorageForAttachingDiskFlow.java (1)
66-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win逻辑正确，但与其他文件存在重复模式。
该「加密且未指定主存储则追加
ENCRYPTED_VOLUME特性」的判断逻辑，在VolumeManagerImpl.java（364-366行）及VmAllocatePrimaryStorageFlow.java中重复出现。建议抽取为AllocatePrimaryStorageMsg/AllocatePrimaryStorageSpaceMsg上的统一辅助方法，减少重复并降低未来修改遗漏的风险（详见文末合并意见）。🤖 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/VmAllocatePrimaryStorageForAttachingDiskFlow.java` around lines 66 - 68, 将“卷已加密且未指定主存储时追加 ENCRYPTED_VOLUME 特性”的重复判断抽取到 AllocatePrimaryStorageMsg 或 AllocatePrimaryStorageSpaceMsg 的统一辅助方法中；更新 VmAllocatePrimaryStorageForAttachingDiskFlow 及 VolumeManagerImpl、VmAllocatePrimaryStorageFlow 使用该方法，并移除各处重复条件逻辑，保持现有行为不变。test/src/test/groovy/org/zstack/test/integration/storage/primary/EncryptedVolumePrimaryStorageAllocationCase.groovy (1)
59-93: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win缺少针对 VM 级根/数据盘混合场景的回归测试。
当前测试仅覆盖了
AllocatePrimaryStorageSpaceMsg+PrimaryStorageFeatureAllocatorExtensionPoint.allocatePrimaryStorage这一条路径，未覆盖VmAllocatePrimaryStorageExtensionPoint.filterPrimaryStorageCandidates（VM 级 root/data 候选过滤）路径，也未覆盖 PR 目标中提到的混合场景（非加密根盘显式放在 ZBS，加密数据盘放在非 ZBS）。建议补充一个通过完整 VM 创建流程触发filterPrimaryStorageCandidates的集成测试用例，以验证 root 与 data 候选列表按卷角色独立过滤，而不是全部包含关系。🤖 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 `@test/src/test/groovy/org/zstack/test/integration/storage/primary/EncryptedVolumePrimaryStorageAllocationCase.groovy` around lines 59 - 93, 在现有测试类中新增一个通过完整 VM 创建流程触发 VmAllocatePrimaryStorageExtensionPoint.filterPrimaryStorageCandidates 的集成测试，覆盖非加密根盘显式放在 ZBS、加密数据盘放在非 ZBS 的混合场景。分别断言 root 与 data 卷候选按各自卷角色独立过滤，确保不会错误地使用所有卷候选的整体包含关系；保留现有 testFilterZhpsForEncryptedVolumeAutoAllocation 的覆盖范围不变。storage/src/main/java/org/zstack/storage/volume/VolumeManagerImpl.java (1)
364-366: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win逻辑正确，但与其他文件存在重复模式。
同
VmAllocatePrimaryStorageForAttachingDiskFlow.java66-68行的重复问题，见文末合并意见。🤖 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/volume/VolumeManagerImpl.java` around lines 364 - 366, 将 VolumeManagerImpl 中针对加密卷且未指定主存储时添加 PrimaryStorageFeature.ENCRYPTED_VOLUME 的逻辑，与 VmAllocatePrimaryStorageForAttachingDiskFlow 中的重复实现提取为共享方法或复用已有公共方法，并让两处调用该实现；保持现有判断条件和 amsg.addRequiredFeature 行为不变。
🤖 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/vm/VmAllocatePrimaryStorageExtensionPoint.java`:
- Around line 5-8: 为公共接口方法
VmAllocatePrimaryStorageExtensionPoint.filterPrimaryStorageCandidates 补充有效
Javadoc，说明其调用时机、spec 参数用途、rootPrimaryStorageUuids 与 dataPrimaryStorageUuids
分别表示的候选列表，以及实现应原地修改列表的约定。
In `@storage/src/main/java/org/zstack/storage/volume/VolumeBase.java`:
- Around line 3414-3420: 修改 refreshVO() 的异常处理：刷新失败时不要仅记录警告后继续执行
trigger.next()，应按现有流程重试刷新，或继续向上抛出异常使流程失败。只有 refreshVO() 成功后才允许调用
trigger.next()，确保后续结果不会使用过期的 self。
- Around line 3466-3473: 严格处理加密转换后的清理失败：更新
detachKeyProviderFromVolume、detachKeyProviderFromSnapshot 及 trash.createTrash
所在流程，失败时执行可重试处理或持久化待清理任务，不要仅记录日志后继续成功返回；确保密钥引用解绑和旧数据位回收状态可被可靠完成或追踪。
---
Nitpick comments:
In
`@compute/src/main/java/org/zstack/compute/vm/VmAllocatePrimaryStorageForAttachingDiskFlow.java`:
- Around line 66-68: 将“卷已加密且未指定主存储时追加 ENCRYPTED_VOLUME 特性”的重复判断抽取到
AllocatePrimaryStorageMsg 或 AllocatePrimaryStorageSpaceMsg 的统一辅助方法中；更新
VmAllocatePrimaryStorageForAttachingDiskFlow 及
VolumeManagerImpl、VmAllocatePrimaryStorageFlow 使用该方法，并移除各处重复条件逻辑，保持现有行为不变。
In `@storage/src/main/java/org/zstack/storage/volume/VolumeManagerImpl.java`:
- Around line 364-366: 将 VolumeManagerImpl 中针对加密卷且未指定主存储时添加
PrimaryStorageFeature.ENCRYPTED_VOLUME 的逻辑，与
VmAllocatePrimaryStorageForAttachingDiskFlow
中的重复实现提取为共享方法或复用已有公共方法，并让两处调用该实现；保持现有判断条件和 amsg.addRequiredFeature 行为不变。
In
`@test/src/test/groovy/org/zstack/test/integration/storage/primary/EncryptedVolumePrimaryStorageAllocationCase.groovy`:
- Around line 59-93: 在现有测试类中新增一个通过完整 VM 创建流程触发
VmAllocatePrimaryStorageExtensionPoint.filterPrimaryStorageCandidates
的集成测试，覆盖非加密根盘显式放在 ZBS、加密数据盘放在非 ZBS 的混合场景。分别断言 root 与 data
卷候选按各自卷角色独立过滤，确保不会错误地使用所有卷候选的整体包含关系；保留现有
testFilterZhpsForEncryptedVolumeAutoAllocation 的覆盖范围不变。
🪄 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: 4a5412c8-388d-4cc9-9877-e937c0d719d6
⛔ Files ignored due to path filters (1)
conf/springConfigXml/VolumeManager.xmlis excluded by!**/*.xml
📒 Files selected for processing (12)
compute/src/main/java/org/zstack/compute/vm/VmAllocateHostAndPrimaryStorageFlow.javacompute/src/main/java/org/zstack/compute/vm/VmAllocatePrimaryStorageFlow.javacompute/src/main/java/org/zstack/compute/vm/VmAllocatePrimaryStorageForAttachingDiskFlow.javacompute/src/main/java/org/zstack/compute/vm/VmInstantiateOtherDiskFlow.javaheader/src/main/java/org/zstack/header/storage/primary/AllocatePrimaryStorageMsg.javaheader/src/main/java/org/zstack/header/storage/primary/PrimaryStorageFeature.javaheader/src/main/java/org/zstack/header/vm/VmAllocatePrimaryStorageExtensionPoint.javastorage/src/main/java/org/zstack/storage/encrypt/EncryptedVolumePrimaryStorageAllocatorExtension.javastorage/src/main/java/org/zstack/storage/volume/InstantiateVolumeForNewCreatedVmExtension.javastorage/src/main/java/org/zstack/storage/volume/VolumeBase.javastorage/src/main/java/org/zstack/storage/volume/VolumeManagerImpl.javatest/src/test/groovy/org/zstack/test/integration/storage/primary/EncryptedVolumePrimaryStorageAllocationCase.groovy
🚧 Files skipped from review as they are similar to previous changes (1)
- compute/src/main/java/org/zstack/compute/vm/VmInstantiateOtherDiskFlow.java
🛑 Comments failed to post (3)
header/src/main/java/org/zstack/header/vm/VmAllocatePrimaryStorageExtensionPoint.java (1)
5-8: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
为公共扩展点方法补充 Javadoc。
filterPrimaryStorageCandidates是跨模块扩展契约，但目前没有说明调用时机、root/data 两组候选列表的含义，以及实现是否应原地修改列表，容易导致扩展实现产生不一致行为。As per path instructions：接口方法必须配有有效的 Javadoc 注释。
建议补充
public interface VmAllocatePrimaryStorageExtensionPoint { + /** + * 在生成主存储组合前过滤 root/data 主存储候选。 + * + * `@param` spec VM 分配规格 + * `@param` rootPrimaryStorageUuids root 卷候选主存储 UUID，允许原地过滤 + * `@param` dataPrimaryStorageUuids data 卷候选主存储 UUID，允许原地过滤 + */ default void filterPrimaryStorageCandidates(VmInstanceSpec spec, List<String> rootPrimaryStorageUuids, List<String> dataPrimaryStorageUuids) {📝 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.public interface VmAllocatePrimaryStorageExtensionPoint { /** * 在生成主存储组合前过滤 root/data 主存储候选。 * * `@param` spec VM 分配规格 * `@param` rootPrimaryStorageUuids root 卷候选主存储 UUID，允许原地过滤 * `@param` dataPrimaryStorageUuids data 卷候选主存储 UUID，允许原地过滤 */ default void filterPrimaryStorageCandidates(VmInstanceSpec spec, List<String> rootPrimaryStorageUuids, List<String> dataPrimaryStorageUuids) { }🤖 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 `@header/src/main/java/org/zstack/header/vm/VmAllocatePrimaryStorageExtensionPoint.java` around lines 5 - 8, 为公共接口方法 VmAllocatePrimaryStorageExtensionPoint.filterPrimaryStorageCandidates 补充有效 Javadoc，说明其调用时机、spec 参数用途、rootPrimaryStorageUuids 与 dataPrimaryStorageUuids 分别表示的候选列表，以及实现应原地修改列表的约定。Source: Coding guidelines
storage/src/main/java/org/zstack/storage/volume/VolumeBase.java (2)
3414-3420: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
不要吞掉
refreshVO()失败。Line [3414] 之后即使刷新失败仍执行
trigger.next()；但数据库更新已经完成，最终 Line [3501] 会用过期的self返回成功结果，可能继续报告旧的encrypted状态或旧安装路径。应重试刷新，或让流程失败而不是返回过期 inventory。As per path instructions：异常不能被静默转换为正常流程，必要时应继续向上抛出。
🤖 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/volume/VolumeBase.java` around lines 3414 - 3420, 修改 refreshVO() 的异常处理：刷新失败时不要仅记录警告后继续执行 trigger.next()，应按现有流程重试刷新，或继续向上抛出异常使流程失败。只有 refreshVO() 成功后才允许调用 trigger.next()，确保后续结果不会使用过期的 self。Source: Coding guidelines
3466-3473: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
不要把密钥引用和旧位清理失败转成成功。
数据库在 Line [3763] 已更新卷的加密状态；如果
detachKeyProviderFromVolume、快照密钥解绑或trash.createTrash失败，当前代码只记录日志并继续，最终会出现“数据库显示转换成功，但密钥引用仍存在或旧数据位未纳入回收”的不一致状态。请改为失败重试，或持久化待清理任务。As per path instructions：应严格处理异常，不能将关键资源清理失败当作正常流程。
Also applies to: 3484-3493
🤖 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/volume/VolumeBase.java` around lines 3466 - 3473, 严格处理加密转换后的清理失败：更新 detachKeyProviderFromVolume、detachKeyProviderFromSnapshot 及 trash.createTrash 所在流程，失败时执行可重试处理或持久化待清理任务，不要仅记录日志后继续成功返回；确保密钥引用解绑和旧数据位回收状态可被可靠完成或追踪。Source: Coding guidelines
MatheMatrix
force-pushed
the
sync/zstackio/codex/filter-zhps-encrypted-auto-ps
branch
2 times, most recently
from
dfee7fa to
feacb8d
Compare
Filter zhps in the allocator extension point. Only applies to encrypted auto primary storage allocation. Test: git diff --check Test: cbok zsv compile --no-deploy passed Test: cbok zsv groovy_test EncryptedVolumePrimaryStorageAllocationCase passed Resolves: ZSV-12607 Change-Id: If4078641c7a42c67552373fcc9d7581cf4f97b1a
MatheMatrix
force-pushed
the
sync/zstackio/codex/filter-zhps-encrypted-auto-ps
branch
from
feacb8d to
92b7973
Compare
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
3 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
Filter ZBS primary storage from encrypted VM disk automatic primary storage allocation.
Changes
Testing
sync from gitlab !10426