Repository navigation
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #14484 +/- ##
=============================================
- Coverage 20.76% 3.86% -16.91%
=============================================
Files 6496 494 -6002
Lines 591612 42875 -548737
Branches 72585 8136 -64449
=============================================
- Hits 122851 1657 -121194
+ Misses 455424 40986 -414438
+ Partials 13337 232 -13105
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@blueorangutan package |
|
@Pearl1594 a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19694 |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
3 open issues
What changed in this PR
Adds incremental volume snapshot support for CLVM-NG by treating it similarly to KVM/file-based incremental flows and enforcing secondary-storage requirements in the KVM storage processor.
Changes:
- Enable incremental snapshot payload/flow for
StoragePoolType.CLVM_NGwhenkvmIncrementalSnapshotis enabled. - Adjust snapshot-to-secondary handling to avoid double-backup for CLVM-NG incremental snapshots.
- Add CLVM-NG incremental validation in KVM snapshot creation (requires secondary storage; supports live/paused cases).
| File | Description |
|---|---|
| server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java | Enables CLVM-NG to participate in the incremental snapshot/secondary-store allocation flow and adjusts backup behavior accordingly. |
| plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java | Adds CLVM-NG incremental snapshot checks and routes live/stopped snapshot handling through incremental paths using secondary storage. |
Review effort: Lite
| if (isClvmNgIncremental) { | ||
| logger.info("Taking incremental snapshot of CLVM_NG volume [{}] using QCOW2 backup to secondary storage.", volume.getName()); | ||
| newSnapshot = takeIncrementalVolumeSnapshotOfStoppedVm(snapshotTO, primaryPool, secondaryPool, | ||
| imageStoreTo.getUrl(), snapshotName, volume, conn, cmd.getWait()); |
|
|
||
| StoragePoolType poolType = volume.getStoragePoolType(); | ||
| boolean isClvmNgIncrementalCandidate = StoragePoolType.CLVM_NG == poolType && kvmIncrementalSnapshot.valueIn(clusterId); | ||
|
|
| } | ||
| boolean isLiveClvmNgIncremental = isClvmNgIncremental && (DomainInfo.DomainState.VIR_DOMAIN_RUNNING.equals(state) || DomainInfo.DomainState.VIR_DOMAIN_PAUSED.equals(state)); | ||
| if ((DomainInfo.DomainState.VIR_DOMAIN_RUNNING.equals(state) && !primaryPool.isExternalSnapshot()) || isLiveClvmNgIncremental) { | ||
| if (snapshotTO.isKvmIncrementalSnapshot()) { |
JoaoJandre
left a comment
JoaoJandre
left a comment
There was a problem hiding this comment.
CLGTM, did not test it
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
|
Hi @Pearl1594 can you please address the conflicts? |
There was a problem hiding this comment.
🟡 Changes recommended
CLVM-NG incremental snapshots can retain incorrect full-volume charges, omit requested replicas with asynchronous backup, and fail in primary-only configurations.
6 open issues
imageStoreTo.getUrl()can throw a NullPointerException ifimageStoreTois null. The earlier…takeIncrementalVolumeSnapshotOfRunningVm(...)is now used for theVIR_DOMAIN_PAUSEDstate as…
Review effort: Balanced
|
|
||
| if (archiveSnapshotToSecondary) { | ||
| if (!isKvmAndFileBasedStorage) { | ||
| if (!isKvmAndFileBasedStorage && !isClvmNgIncrementalCandidate) { |
| if (!isKvmAndFileBasedStorage && !isClvmNgIncrementalCandidate) { | ||
| backupSnapshotToSecondary(payload.getAsyncBackup(), snapshotStrategy, snapshotOnPrimary, payload.getZoneIds(), payload.getStoragePoolIds()); |
| } | ||
|
|
||
| if (isKvmAndFileBasedStorage && kvmIncrementalSnapshot.valueIn(clusterId)) { | ||
| if ((isKvmAndFileBasedStorage || StoragePoolType.CLVM_NG == poolType) && kvmIncrementalSnapshot.valueIn(clusterId)) { |
|
@blueorangutan package |
|
@Pearl1594 a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
🔴 Test Coverage Grade:
|
| Metric | Value |
|---|---|
| Line coverage | 25.86% |
| Branch coverage | 20.19% |
Grade Scale
| Grade | Line Coverage | Meaning |
|---|---|---|
| 🟢 A | ≥ 80% | Excellent - this code sleeps well at night 😴 |
| 🟡 B | 60-79% | Good - almost there, don't stop now 😉 |
| 🟠 C | 40-59% | Acceptable - your code is wearing a seatbelt, but no airbags 😬 |
| 🔴 D | 20-39% | Marginal - boldly shipping where no test has gone before 🖖 |
| ⛔ F | < 20% | Failing - tests? what tests? 🔥 |
Branch coverage is shown as a secondary signal. Grade is determined by line coverage.
View full Actions run
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✖️ suse15. SL-JID 19869 |
|
@blueorangutan package |
|
@RosiKyu a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19989 |
RosiKyu
left a comment
RosiKyu
left a comment
There was a problem hiding this comment.
LGTM
QA test results: CLVM_NG incremental snapshots
Build: 24.0.0-shapeblue22703 (PR head 2f1249b), upgraded from 4.23.0.0-shapeblue0. Management server and 4 KVM hosts on Oracle Linux 8. Pools: two CLVM_NG pools and one CLVM pool (lvmlockd + sanlock shared VGs on iSCSI LUNs), one NFS primary pool, NFS secondary storage. Guest: CentOS 5.5.
Result: 14 of 15 tests passed. 1 test failed (minor, error message only, details below).
How each test was checked
Before every snapshot, write a marker file in the guest, e.g. echo "M3 written $(date -u) before snapshot 3" > /root/m3; sync. On data disks, also write a 20 MB random file and note its md5sum. To check a snapshot, create a volume from it, attach it to a second VM, and look for the markers on the raw disk: tr -c "[:print:]" "\n" < /dev/vdX | grep -oE "M[0-9] written .* before snapshot [0-9]". For data disks, mount the disk read only and compare the md5 sums. A restore of snapshot N must contain markers 1 to N and nothing newer. To see whether a snapshot is full or incremental, check snapshot_store_ref: parent_snapshot_id = 0 means full, otherwise it is incremental on that parent (incrementals are also much smaller in physical_size).
Global settings for all tests unless the test says otherwise: kvm.incremental.snapshot=true, snapshot.delta.max=16, snapshot.backup.to.secondary=true.
| # | Test | Steps | Expected | Result |
|---|---|---|---|---|
| UPG-01 | Upgrade with existing snapshots | On 4.23, take 2 ROOT snapshots and 1 DATA snapshot of a running VM on CLVM_NG. Upgrade the management server and agents to the PR build. Set kvm.incremental.snapshot=true and take 3 more ROOT snapshots. Restore all 5. Delete the oldest 4.23 snapshot. |
The first snapshot after the upgrade is a new full, the next 2 are incrementals. All 5 restores are correct. Deleting the old snapshot does not affect the new chain. | PASS |
| INC-01 | Running VM, guest writing | Running VM on CLVM_NG: take 4 ROOT snapshots (marker before each), then a 5th while a loop in the guest keeps writing files. Restore all 5. | 1 full, then incrementals. Each restore has the markers up to its own snapshot. | PASS |
| INC-02 | Stopped VM, ROOT and DATA | 3 rounds of: start the VM, write markers on ROOT and DATA, stop the VM, snapshot ROOT and DATA. Restore all 6. | Chains correct. Markers and DATA md5 sums match in every restore. | PASS |
| INC-03 | Revert to the middle of a chain | Stop the VM, revert ROOT to the 2nd of 3 snapshots, start the VM, check files. Write a marker, snapshot, write a marker, snapshot. Restore both. | The guest has the data of snapshot 2 only. The next snapshot is a new full, the one after is incremental. Restores have no data from the reverted snapshot 3. | PASS |
| INC-04 | Volume and template from an incremental | VM stopped: create a volume from the latest incremental DATA snapshot, attach it and the original DATA disk to another VM, compare with md5sum /dev/vdX /dev/vdY. Create a template from an incremental ROOT snapshot and deploy a VM from it. |
The disks are byte-identical. The VM from the template boots and has the data up to that snapshot. | PASS |
| INC-05 | Delete a snapshot in the middle | Delete the 3rd snapshot of a 5-snapshot chain. Restore the 4th and the 5th. | The 3rd snapshot disappears from listSnapshots, and in the DB it shows as Destroyed / Hidden. The 4th and 5th restore with all markers, including the one from the deleted 3rd. |
PASS |
| INC-06 | Live migration | Take 2 snapshots, live migrate the VM to another host, take 2 more. Restore the last 2. | The chain continues after the migration. Restores are correct. | PASS |
| INC-07 | Disk moved to the other CLVM_NG pool | Running VM: migrateVirtualMachineWithVolume to another host, with the DATA disk moved to the other CLVM_NG pool. Take a snapshot before the move and 2 after. Restore all 3. |
The VM stays up and its files are intact. The first snapshot after the move is a new full, then incremental. All md5 sums match. | PASS |
| INC-08 | snapshot.delta.max rollover |
Set snapshot.delta.max=3, take 5 snapshots, restore the 3rd and the 5th. |
Full, 2 incrementals, then a new full. Restores are correct. | PASS |
| INC-09 | Paused VM | virsh suspend the VM on its host, take a ROOT snapshot, check virsh domstate, virsh resume. Restore the snapshot. |
The snapshot succeeds as an incremental, and the VM stays paused until it is resumed. The restore has the full history. | PASS |
| INC-10 | Backup to secondary off | Set snapshot.backup.to.secondary=false, restart the management server, take a ROOT snapshot of a VM on CLVM_NG. |
A clear error that names the cause, or a clear fallback. No broken snapshot. | FAILED (see below) |
| INC-11 | Recurring hourly policy | Create an HOURLY snapshot policy on a DATA disk. A cron job in the guest writes a marker 2 minutes before each run. Wait 2 hours, then restore the 2nd snapshot. | The 2nd hourly snapshot is incremental on the 1st. The restore has both hourly markers. | PASS |
| INC-12 | Regression: plain CLVM pool | VM on a CLVM (not CLVM_NG) pool, incremental on: take 3 snapshots, restore 2. | All 3 are full snapshots with no checkpoint, as before the PR. Restores are correct. | PASS |
| INC-13 | Regression: incremental off on CLVM_NG | Set kvm.incremental.snapshot=false, take 3 snapshots. Set it back to true and take 2 more. Restore one from each part. |
When off: all full, no checkpoint. When back on: a new full, then incremental. Restores are correct. | PASS |
| INC-14 | Regression: NFS primary | VM on NFS primary, snapshot.delta.max=3: take 4 snapshots, restore the 3rd. |
Same chain behaviour as before (full, 2 incrementals, new full). Restore is correct. | PASS |
Failed: INC-10, a CLVM_NG snapshot with backup to secondary off fails with only "internal error"
Severity: Minor. The snapshot is correctly refused and nothing broken is left behind. The snapshot row goes to Error, and no new LV or libvirt checkpoint is created. But the user cannot tell why it failed.
Steps to reproduce
- Set
kvm.incremental.snapshot=trueandsnapshot.backup.to.secondary=false, then restart the management server (this setting is not dynamic). - Take a snapshot of the ROOT disk of a running VM on a CLVM_NG pool:
createSnapshot volumeid={volume}. - For comparison, do the same for a VM on an NFS primary pool.
Expected result: an error that tells the user what to do (for example: turn snapshot.backup.to.secondary back on, or turn off kvm.incremental.snapshot).
Actual result: the API and UI only show "Failed to create Snapshot due to an internal error". The real reason is only in the management server log, at DEBUG level. That message also says "Please configure secondary storage", but secondary storage is already configured; the actual cause is the snapshot.backup.to.secondary setting. On NFS, the same settings work and the snapshot stays on primary storage.
API response, step 2
inc10-primary-s3: jobstatus 2 {"errorcode": 530, "errortext": "Failed to create Snapshot due to an internal error creating Snapshot for volume cb3ddeb9-9d4b-488f-a129-7613b5ad19ff"}
Management server log, step 2
2026-11-12 15:47:35,402 DEBUG [o.a.c.s.s.DefaultSnapshotStrategy] (Work-Job-Executor-1:[ctx-e248fc59, job-386/job-387, ctx-a8e32c65]) (logid:798c2822) Failed to take snapshot: Incremental snapshots for CLVM_NG require secondary storage. Please configure secondary storage or disable incremental snapshots for volume [ROOT-9].
2026-11-12 15:47:35,631 ERROR [o.a.c.a.c.u.s.CreateSnapshotCmd] (API-Job-Executor-1:[ctx-55582e63, job-386, ctx-51d0aec5]) (logid:798c2822) Failed to create Snapshot due to an internal error creating Snapshot for volume cb3ddeb9-9d4b-488f-a129-7613b5ad19ff java.lang.RuntimeException: Unexpected exception
2026-11-12 15:47:35,635 DEBUG [o.a.c.f.j.i.AsyncJobManagerImpl] (API-Job-Executor-1:[ctx-55582e63, job-386]) (logid:798c2822) Complete async job-386, jobStatus: FAILED, resultCode: 530, result: org.apache.cloudstack.api.response.ExceptionResponse/null/{"uuidList":[],"errorcode":"530","errortext":"Failed to create Snapshot due to an internal error creating Snapshot for volume cb3ddeb9-9d4b-488f-a129-7613b5ad19ff"}
Step 3: NFS volume with the same settings, the snapshot succeeds and stays on primary
| inc10-nfs-primary-only | BackedUp | Primary | Ready | 0 | 47 | 1874853888 | /mnt/309c71fd-166b-38dd-88b0-bb02aac6a416/snapshots/b8f49ba6-aeab-432c-9c59-7952b129c9ac |
Notes (existing behaviour, not caused by the PR)
snapshot.delta.maxcounts the full snapshot as part of the chain, so 3 gives a full plus 2 incrementals. NFS behaves the same way, and the code is unchanged from main.- Moving a VM together with its storage ends the snapshot chain for all of its disks, so the next snapshot of each disk is a full. Same code as in main.
migrateVolumewithlivemigrate=trueis refused on KVM by design;migrateVirtualMachineWithVolumehas to be used instead.
RosiKyu
left a comment
RosiKyu
left a comment
There was a problem hiding this comment.
Approving. My earlier "Request changes" was a mistake; my full test results are in the review above.
|
@blueorangutan test |
|
@Pearl1594 a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
@RosiKyu I found an issue with bitmap removal for a stopped VM and when the disks have moved: |
|
@blueorangutan package |
|
@Pearl1594 a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 20040 |
|
[SF] Trillian test result (tid-17568)
|
Description
This PR adds support for incremental volume snapshots for CLVM-NG
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Screenshots (if appropriate):
How Has This Been Tested?
snapshot.delta.maxchain rolloverHow did you try to break this feature and the system with this change?