<feature>[storage]: ZSV-12577 support backup encryption change extension#4367
Open
zstack-robot-2 wants to merge 2 commits into
Open
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 33 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 (8)
Walkthrough新增卷备份加密转换扩展点接口，Ceph/Local/NFS 三种主存储的加密转换消息处理改为基于消息携带的 hostUuid 解析并校验目标宿主，Ceph 命令新增 volumeUuid 字段。VolumeBase 引入新的 VolumeEncryptionConversionHostLeaseHelper 实现宿主互斥租约的预留/释放，并在转换流程中新增后置备份加密扩展点调用。 Changes卷加密转换宿主选择与租约
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant VolumeBase
participant VolumeEncryptionConversionHostLeaseHelper
participant PrimaryStorage
participant VolumeBackupEncryptionConversionExtensionPoint
VolumeBase->>VolumeEncryptionConversionHostLeaseHelper: reserve(volume)
VolumeEncryptionConversionHostLeaseHelper-->>VolumeBase: hostLease
VolumeBase->>PrimaryStorage: ConvertVolumeEncryptionOnPrimaryStorageMsg(hostUuid)
PrimaryStorage->>PrimaryStorage: resolveHostForConvertVolumeEncryption/resolveHostForCephLuks
PrimaryStorage-->>VolumeBase: 转换结果
VolumeBase->>VolumeBackupEncryptionConversionExtensionPoint: convertVolumeBackupEncryption(volume, encrypted)
VolumeBase->>VolumeEncryptionConversionHostLeaseHelper: release(hostLease)
VolumeBase-->>VolumeBase: completion.success/fail
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
MatheMatrix
force-pushed
the
sync/zstackio/dev/encrypted-volume-backup-linked-encryption@@4
branch
2 times, most recently
from
8fed9b5 to
3d4a11f
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: 2
🧹 Nitpick comments (1)
header/src/main/java/org/zstack/header/volume/VolumeAfterChangeEncryptionExtensionPoint.java (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win接口方法缺少 Javadoc 注释。
新增的公开扩展点方法未提供 Javadoc，且
encrypted布尔参数语义（表示变更后的目标加密状态）不够直观，建议补充说明。As per path instructions（“接口方法不应有多余的修饰符（例如 public），且必须配有有效的 Javadoc 注释。”）。📝 建议补充 Javadoc
public interface VolumeAfterChangeEncryptionExtensionPoint { - void volumeAfterChangeEncryption(VolumeInventory volume, boolean encrypted, Completion completion); + /** + * 卷加密状态变更完成后的回调扩展点。 + * + * `@param` volume 发生加密状态变更的卷 + * `@param` encrypted 变更后的目标加密状态：{`@code` true} 表示已加密，{`@code` false} 表示未加密 + * `@param` completion 异步完成回调 + */ + void volumeAfterChangeEncryption(VolumeInventory volume, boolean encrypted, Completion completion); }🤖 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/volume/VolumeAfterChangeEncryptionExtensionPoint.java` at line 6, 新增的扩展点方法 volumeAfterChangeEncryption 缺少必要的 Javadoc，且 encrypted 参数语义需要明确说明。请在该接口方法上补充有效的 Javadoc，说明它在 Volume 加密状态变更完成后触发，并清楚标注 encrypted 表示变更后的目标加密状态；同时检查该接口方法签名，避免显式 public 等多余修饰符。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.
Inline comments:
In `@storage/src/main/java/org/zstack/storage/volume/VolumeBase.java`:
- Around line 3670-3683: The host lease created in
createVolumeEncryptionConversionHostLease() is persisted via JsonLabelVO but
only removed on normal release paths, so crashes can leave a permanent stale
lease. Add an expiration/recovery mechanism to the lease model (for example
include a TTL or creation timestamp in labelValue or a dedicated field) and make
reserve()/lease acquisition prune expired leases before blocking. Also update
releaseVolumeEncryptionConversionHost() and any lease lookup logic to treat
expired or unreachable leases as reclaimable so later conversions on the same
host do not stall indefinitely.
- Around line 3629-3667: The reserveVolumeEncryptionConversionHost() loop blocks
the flow worker thread with repeated sleep while waiting for the host lease,
which can exhaust the SimpleFlowChain/_syncpool workers. Refactor this wait path
to avoid synchronous polling inside the flow: move the retry to an async
scheduled retry/requeue, or use a non-blocking backoff with a much shorter
maximum wait. Keep the lease checks and ownership validation in
reserveVolumeEncryptionConversionHost(), but ensure the thread is released
between attempts instead of holding it in the while(true) loop.
---
Nitpick comments:
In
`@header/src/main/java/org/zstack/header/volume/VolumeAfterChangeEncryptionExtensionPoint.java`:
- Line 6: 新增的扩展点方法 volumeAfterChangeEncryption 缺少必要的 Javadoc，且 encrypted
参数语义需要明确说明。请在该接口方法上补充有效的 Javadoc，说明它在 Volume 加密状态变更完成后触发，并清楚标注 encrypted
表示变更后的目标加密状态；同时检查该接口方法签名，避免显式 public 等多余修饰符。
🪄 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: d6040e8c-07cf-4689-a6f7-af3e68b9b779
📒 Files selected for processing (6)
header/src/main/java/org/zstack/header/volume/VolumeAfterChangeEncryptionExtensionPoint.javaplugin/ceph/src/main/java/org/zstack/storage/ceph/primary/CephPrimaryStorageBase.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.javaplugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageKVMBackend.javaplugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageKVMBackendCommands.javastorage/src/main/java/org/zstack/storage/volume/VolumeBase.java
MatheMatrix
force-pushed
the
sync/zstackio/dev/encrypted-volume-backup-linked-encryption@@4
branch
7 times, most recently
from
1d5be06 to
1539449
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
storage/src/main/java/org/zstack/storage/volume/VolumeBase.java (1)
3353-3358: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift避免在 Flow 线程中同步等待宿主租约。
reserve(self)内部会按 3 秒间隔阻塞等待，最长 2 小时；这里在Flow.run()中同步调用，会长期占用卷操作链路线程。建议改为异步重试/调度，或让 helper 暴露非阻塞 reserve 尝试接口。🤖 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 3353 - 3358, The Flow.run() implementation in VolumeBase is synchronously calling volumeEncryptionConversionHostLeaseHelper.reserve(self), which can block a Flow thread for a long time. Change this step to avoid blocking by using an asynchronous retry/scheduling approach, or add a non-blocking try-reserve API on VolumeEncryptionConversionHostLeaseHelper and have the flow continue or reschedule based on that result. Keep the host lease handling around hostLease.set(...) and trigger.next() in the updated async/non-blocking path.storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptionConversionHostLeaseHelper.java (1)
76-91: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift为持久化租约增加过期/回收机制。
当前
labelValue只保存vmUuid::volumeUuid，且只有正常release()才删除；管理节点在持有租约期间异常退出后，同宿主其他 VM 会持续失败，同 VM 会反复等到超时。建议写入过期时间/心跳并在reserve()时回收过期租约。Also applies to: 151-157
🤖 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/encrypt/VolumeEncryptionConversionHostLeaseHelper.java` around lines 76 - 91, The lease stored by VolumeEncryptionConversionHostLeaseHelper has no expiration or reclaim path, so stale JsonLabelVO entries can block other VMs after a manager crash. Update the reservation flow in reserve()/createLease() to persist an expiry or heartbeat in the labelValue, and in reserve() detect and reclaim expired leases before deciding ownership; also ensure release() still clears the label normally. Use the existing leaseKey(), leaseVmUuid(), and createLease()/release() paths to keep the ownership check consistent.
🤖 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
`@plugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsPrimaryStorageKVMBackend.java`:
- Around line 2414-2423: The NfsPrimaryStorageKVMBackend host-state check only
rejects PrimaryStorageHostStatus.Disconnected, so null and Connecting can still
pass; update the validation around the PrimaryStorageHostRefVO lookup to
explicitly require PrimaryStorageHostStatus.Connected. Use the status check in
NfsPrimaryStorageKVMBackend where inv.getUuid() and hostUuid are used, and throw
OperationFailureException for any non-Connected result with a message that
clearly states the host must be connected to the NFS primary storage.
In `@storage/src/main/java/org/zstack/storage/volume/VolumeBase.java`:
- Around line 3434-3473: The current NoRollbackFlow in
VolumeBase.convert-volume-backup-encryption updates backup encryption before the
volume DB change and has no rollback, so partial extension success or later
failure can leave state inconsistent. Refactor this step to use a compensable or
idempotent recovery path: either move the encryption conversion to a
post-DB-success task with explicit retry handling, or add a matching
rollback/revert mechanism around VolumeBackupEncryptionConversionExtensionPoint
so failures can restore prior backup encryption state cleanly.
---
Duplicate comments:
In
`@storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptionConversionHostLeaseHelper.java`:
- Around line 76-91: The lease stored by
VolumeEncryptionConversionHostLeaseHelper has no expiration or reclaim path, so
stale JsonLabelVO entries can block other VMs after a manager crash. Update the
reservation flow in reserve()/createLease() to persist an expiry or heartbeat in
the labelValue, and in reserve() detect and reclaim expired leases before
deciding ownership; also ensure release() still clears the label normally. Use
the existing leaseKey(), leaseVmUuid(), and createLease()/release() paths to
keep the ownership check consistent.
In `@storage/src/main/java/org/zstack/storage/volume/VolumeBase.java`:
- Around line 3353-3358: The Flow.run() implementation in VolumeBase is
synchronously calling volumeEncryptionConversionHostLeaseHelper.reserve(self),
which can block a Flow thread for a long time. Change this step to avoid
blocking by using an asynchronous retry/scheduling approach, or add a
non-blocking try-reserve API on VolumeEncryptionConversionHostLeaseHelper and
have the flow continue or reschedule based on that result. Keep the host lease
handling around hostLease.set(...) and trigger.next() in the updated
async/non-blocking path.
🪄 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: e13d3414-f565-4052-a202-b9d77b36b307
⛔ Files ignored due to path filters (1)
conf/springConfigXml/VolumeManager.xmlis excluded by!**/*.xml
📒 Files selected for processing (6)
header/src/main/java/org/zstack/header/volume/VolumeBackupEncryptionConversionExtensionPoint.javaplugin/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
✅ Files skipped from review due to trivial changes (1)
- header/src/main/java/org/zstack/header/volume/VolumeBackupEncryptionConversionExtensionPoint.java
MatheMatrix
force-pushed
the
sync/zstackio/dev/encrypted-volume-backup-linked-encryption@@4
branch
5 times, most recently
from
6a9f0a0 to
c97b5e7
Compare
Owner
|
Comment from yaohua.wu: Review: MR !10328 — ZSV-12577Background
关联 MR
本仓结论
跨仓阻塞
Coverage
Verdict: REVISION_REQUIRED跨仓 backup chain 完整性问题修复前，本组 MR 不建议合入。 🤖 Robot Reviewer |
MatheMatrix
force-pushed
the
sync/zstackio/dev/encrypted-volume-backup-linked-encryption@@4
branch
from
c97b5e7 to
05c0e65
Compare
Collaborator
|
Comment from 周众: kvmagent/kvmagent/plugins/vm_plugin.py:9709 source secret 和 target secret肯定是同一个。 |
MatheMatrix
force-pushed
the
sync/zstackio/dev/encrypted-volume-backup-linked-encryption@@4
branch
from
05c0e65 to
6172697
Compare
added 2 commits
July 4, 2026 17:42
Resolves: ZSV-12469 Change-Id: I5ed4f9f00e700e4818a5c440b6dc5b04edee9c79
Resolves: ZSV-12577 Change-Id: I224e974f90f555267e06174179023c449eae1cd3
MatheMatrix
force-pushed
the
sync/zstackio/dev/encrypted-volume-backup-linked-encryption@@4
branch
from
6172697 to
f6e5ab8
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
Update volume backup encryption when a volume encryption attribute changes.
Changes
Testing
Resolves: ZSV-12577
sync from gitlab !10328