<feature>[storage]: ZSV-12577 add volume encryption conversion host lease#4509
Conversation
Resolves: ZSV-12469 Change-Id: I7940bd9f7ac4207a0694a3b52e8c8dafbae538c2
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 42 seconds 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 (8)
Walkthrough新增 VolumeEncryptionConversionHostLeaseHelper 实现基于主机的加密转换分布式租约机制（预占、续租、释放），VolumeBase 接入该租约以协调转换流程；Ceph、LocalStorage、NFS 三个主存储插件的加密转换/回滚处理均改为基于消息携带的 hostUuid 精确解析并校验目标主机。 Changes主机租约与精确主机解析
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant VolumeBase
participant LeaseHelper as VolumeEncryptionConversionHostLeaseHelper
participant PrimaryStorage as 主存储(Ceph/Local/NFS)
participant HostVO as 数据库
VolumeBase->>LeaseHelper: reserve(volume, trigger, hostLease)
LeaseHelper->>HostVO: 查询/创建租约标签
LeaseHelper-->>VolumeBase: 预占成功，返回hostUuid
VolumeBase->>PrimaryStorage: ConvertVolumeEncryptionOnPrimaryStorageMsg(hostUuid)
PrimaryStorage->>HostVO: 校验主机连接/启用/归属
PrimaryStorage-->>VolumeBase: 转换结果
VolumeBase->>LeaseHelper: release(hostLease)
LeaseHelper->>HostVO: 删除租约标签
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
MatheMatrix
force-pushed
the
sync/zstackio/dev/encrypted-volume-conversion-host-lock
branch
from
c24b7a2 to
0f3b9df
Compare
![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
🧹 Nitpick comments (1)
plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageBase.java (1)
988-1054: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win建议合并重复的解析方法。
resolveHostForConvertVolumeEncryption与resolveHostForRollbackVolumeEncryption逻辑几乎完全相同，仅错误文案与消息类型不同，属于明显的复制粘贴。参照 NFS 后端将两条路径统一为单个resolveHostForConvertVolumeEncryption(inv, hostUuid)的做法，这里也可以抽出一个仅接收volumeUuid与hostUuid的私有方法，减少重复并避免后续两处逻辑分叉。As per coding guidelines: 单一职责原则 / 避免 Duplicate code（DRY）。
♻️ 建议的重构示例
private String resolveConvertHost(String volumeUuid, String hostUuid, String action) { if (StringUtils.isBlank(hostUuid)) { throw new OperationFailureException(operr( "%s volume encryption on local primary storage[uuid:%s] requires hostUuid for volume[uuid:%s]", action, self.getUuid(), volumeUuid)); } String actualHostUuid = Q.New(LocalStorageResourceRefVO.class) .eq(LocalStorageResourceRefVO_.resourceUuid, volumeUuid) .eq(LocalStorageResourceRefVO_.resourceType, VolumeVO.class.getSimpleName()) .eq(LocalStorageResourceRefVO_.primaryStorageUuid, self.getUuid()) .select(LocalStorageResourceRefVO_.hostUuid) .findValue(); if (StringUtils.isBlank(actualHostUuid) || !hostUuid.equals(actualHostUuid)) { throw new OperationFailureException(operr( "volume[uuid:%s] on local primary storage[uuid:%s] belongs to host[uuid:%s], cannot %s encryption on host[uuid:%s]", volumeUuid, self.getUuid(), actualHostUuid, action, hostUuid)); } boolean hostReady = Q.New(HostVO.class) .eq(HostVO_.uuid, hostUuid) .eq(HostVO_.status, HostStatus.Connected) .eq(HostVO_.state, HostState.Enabled) .isExists(); if (!hostReady) { throw new OperationFailureException(operr( "host[uuid:%s] is not enabled and connected for %s volume[uuid:%s] encryption on local primary storage[uuid:%s]", hostUuid, action, volumeUuid, self.getUuid())); } return hostUuid; }🤖 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/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageBase.java` around lines 988 - 1054, The two private helpers, resolveHostForConvertVolumeEncryption and resolveHostForRollbackVolumeEncryption, duplicate the same host validation and lookup flow with only message text differences. Extract the shared logic into one private resolver that takes the volume UUID, host UUID, and an action/verb parameter, then have both existing methods delegate to it while preserving their specific error wording. Keep the LocalStorageResourceRefVO and HostVO checks in the shared method to avoid future drift and reduce copy-paste.Source: Coding guidelines
🤖 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/VolumeEncryptionConversionHostLeaseHelper.java`:
- Around line 154-166: The host-lease flow in
VolumeEncryptionConversionHostLeaseHelper is treating detached-volume
conversions as per-volume rather than per-host, so concurrent conversions on the
same host do not share the same queue/lease and the second one fails
immediately. Update resolveVmUuid()/lease handling so the detached-volume path
reuses host-scoped serialization or wait logic instead of falling back to
volume.getUuid(), and adjust the OperationFailureException message in the same
block to report a host-level conflict rather than misleading vm[uuid:%s]
ownership.
---
Nitpick comments:
In
`@plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageBase.java`:
- Around line 988-1054: The two private helpers,
resolveHostForConvertVolumeEncryption and
resolveHostForRollbackVolumeEncryption, duplicate the same host validation and
lookup flow with only message text differences. Extract the shared logic into
one private resolver that takes the volume UUID, host UUID, and an action/verb
parameter, then have both existing methods delegate to it while preserving their
specific error wording. Keep the LocalStorageResourceRefVO and HostVO checks in
the shared method to avoid future drift and reduce copy-paste.
🪄 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: 53baf6ff-06ce-4d34-8db1-6764592daf5d
⛔ Files ignored due to path filters (1)
conf/springConfigXml/VolumeManager.xmlis excluded by!**/*.xml
📒 Files selected for processing (5)
plugin/ceph/src/main/java/org/zstack/storage/ceph/primary/CephPrimaryStorageBase.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageBase.javaplugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageKVMBackend.javastorage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptionConversionHostLeaseHelper.javastorage/src/main/java/org/zstack/storage/volume/VolumeBase.java
Resolves: ZSV-12577 Change-Id: Ic4e9204a2ff04621ac28454d48da670b
MatheMatrix
force-pushed
the
sync/zstackio/dev/encrypted-volume-conversion-host-lock
branch
from
0f3b9df to
1722f5e
Compare
Resolves: ZSV-12577 Change-Id: I5a0a15950d0342949fdd156753a24d14
MatheMatrix
force-pushed
the
sync/zstackio/dev/encrypted-volume-conversion-host-lock
branch
from
1722f5e to
7c3b043
Compare
MatheMatrix
force-pushed
the
dev/encrypted-volume-snapshot-conversion-rollback@@3
branch
from
763f2db to
ab4f2b5
Compare
1 participant
Summary
为硬盘加密属性转换增加 MN 侧 host lease，避免多个 MN 或同一节点上的转换请求并发进入同一 host。备份级联加密转换分支会基于该锁分支继续开发。
Changes
VolumeEncryptionConversionHostLeaseHelper，使用 DB label + GLock 做跨 MN 可见的 host lease，并支持 TTL/续租/释放。VolumeBase在加密属性转换前预留 host，并把选中的 hostUuid 下发给主存储 convert/rollback。Testing
Resolves: ZSV-12577
sync from gitlab !10470