backport: Incremental NAS backup support for KVM (#13074) - #13877
backport: Incremental NAS backup support for KVM (#13074)#13877abh1sar wants to merge 5 commits into
Conversation
Implements incremental backup support for the NAS backup provider on KVM, using QEMU dirty bitmaps and libvirt's backup-begin API. RFC: #12899. For large VMs this reduces daily backup storage 80–95% and shortens backup windows from hours to minutes (e.g. a 500 GB VM with moderate writes goes from ~500 GB/day to ~5–15 GB/day after the initial full backup). Signed-off-by: James Peru <jmsperu@gmail.com> Co-authored-by: jmsperu <jmsperu@users.noreply.github.com> Co-authored-by: Abhisar Sinha <63767682+abh1sar@users.noreply.github.com>
boring-cyborg
Bot
added
component:api
component:backup
component:integration-test
component:kvm
component:vmware
Python
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 4.22 #13877 +/- ##
============================================
+ Coverage 17.71% 17.75% +0.04%
- Complexity 15858 15901 +43
============================================
Files 5926 5928 +2
Lines 533613 534014 +401
Branches 65285 65362 +77
============================================
+ Hits 94517 94836 +319
- Misses 428415 428457 +42
- Partials 10681 10721 +40
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:
|
* Avoid concurrent deletes modifying the same backup chain in parallel - Backup deletes are serialized by using a per-VM GlobalLock Backup delete checks if it has a live dependent backup not just a live immediate child.
|
@blueorangutan package |
|
@abh1sar a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18875 |
|
@blueorangutan package |
|
@abh1sar a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18885 |
|
|
@blueorangutan test |
There was a problem hiding this comment.
Pull request overview
This PR backports incremental NAS backup support for KVM by introducing chain-aware orchestration in the NAS backup provider (bitmaps/checkpoints, chain metadata, and cascade delete semantics), plus agent-side support in nasbackup.sh and restore-side chain flattening.
Changes:
- Add incremental-chain decisioning and metadata persistence to the NAS backup provider (including zone-scoped config keys and tombstone/GC semantics for deletes).
- Extend KVM agent backup/restore wrappers and
nasbackup.shto supportbackup-begin --checkpointxml, rebase-backed incrementals, and flatten-on-restore. - Add/extend unit + smoke integration tests to cover incremental cadence, restore, and delete semantics.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/integration/smoke/test_backup_recovery_nas.py | Adds smoke coverage for incremental chain cadence/restore/delete and enables incremental config for the suite |
| server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java | Hides tombstoned (Hidden) backups from user listing; adjusts delete flow for chain-aware providers |
| server/src/main/java/com/cloud/hypervisor/KVMGuru.java | Updates backup logging field selection (type) |
| scripts/vm/hypervisor/kvm/nasbackup.sh | Implements incremental/full/legacy-full modes, checkpoint handling, rebasing, and bitmap cleanup |
| plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java | Updates backup logging field selection (type) |
| plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapperTest.java | Extends stubbing for incremental restore detection path |
| plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java | Adds incremental args validation, script invocation flags, and fallback marker handling |
| plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java | Flattens qcow2 backing chains on restore when needed |
| plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java | Adds unit coverage for incremental enablement, chain decisions, restore clearing checkpoint, and cascade delete rules |
| plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java | Implements incremental chain orchestration, chain-aware delete + resource accounting, and VM checkpoint tracking |
| plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupChainKeys.java | Introduces constants for chain metadata stored in backup_details and VM checkpoint detail key |
| engine/schema/src/main/java/org/apache/cloudstack/backup/BackupVO.java | Renames backing field to type while keeping DB column mapping stable |
| core/src/main/java/org/apache/cloudstack/backup/TakeBackupCommand.java | Adds incremental parameters (mode/bitmaps/parent paths) to agent command |
| core/src/main/java/org/apache/cloudstack/backup/BackupAnswer.java | Returns bitmap-created + incremental-fallback markers from agent to MS |
| api/src/main/java/org/apache/cloudstack/backup/BackupProvider.java | Adds handlesChainDeleteResourceAccounting() capability flag for providers |
| api/src/main/java/org/apache/cloudstack/backup/Backup.java | Adds Hidden backup status for tombstoned chain members |
Suppressed comments (2)
test/integration/smoke/test_backup_recovery_nas.py:69
- When enabling
nas.backup.incremental.enabledfor the test, the update should also be applied at zone scope; otherwise a pre-existing zone override can keep incrementals disabled and break the new incremental-chain tests.
if cls.backup_provider != "nas":
Configurations.update(cls.api_client, 'backup.framework.provider.plugin', value='nas')
if cls.incremental_backup_enabled == "false":
Configurations.update(cls.api_client, 'nas.backup.incremental.enabled', value='true')
test/integration/smoke/test_backup_recovery_nas.py:102
- Cleanup restores
nas.backup.incremental.enabledwithoutzoneid, which may leave a zone override enabled/disabled incorrectly on shared test environments. Restore it withzoneid=cls.zone.idto match the scope of the config key.
if cls.backup_provider != "nas":
Configurations.update(cls.api_client, 'backup.framework.provider.plugin', value=cls.backup_provider)
if cls.incremental_backup_enabled == "false":
Configurations.update(cls.api_client, 'nas.backup.incremental.enabled', value="false")
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Tombstoned chain backups (Status.Hidden) are never shown to users; they exist only so the | ||
| // incremental chain GC can sweep them once their last descendant is deleted. | ||
| sb.and("statusNeq", sb.entity().getStatus(), SearchCriteria.Op.NEQ); | ||
| sb.and("backupStatus", sb.entity().getStatus(), SearchCriteria.Op.EQ); |
There was a problem hiding this comment.
Checked this against 4.22. The backupStatus condition came across from main, where #13254 added the status filter to listBackups, but 4.22 has neither ListBackupsCmd.getBackupStatus() nor the sc.setParametersIfNotNull("backupStatus", ...) call, so here it is an sb.and() whose parameter is never set. It is harmless rather than fatal: SearchCriteria.getWhereClause() skips any condition whose parameters were never set, so listing still works. It is dead code in this branch though, so I'd drop line 980 and keep only the statusNeq clause.
| # Check backup configuration values, set them to enable the nas provider | ||
| backup_enabled_cfg = Configurations.list(cls.api_client, name='backup.framework.enabled') | ||
| backup_provider_cfg = Configurations.list(cls.api_client, name='backup.framework.provider.plugin') | ||
| incremental_backup_enabled_cfg = Configurations.list(cls.api_client, name='nas.backup.incremental.enabled') | ||
| cls.backup_enabled = backup_enabled_cfg[0].value | ||
| cls.backup_provider = backup_provider_cfg[0].value | ||
| cls.incremental_backup_enabled = incremental_backup_enabled_cfg[0].value |
There was a problem hiding this comment.
This mirrors the merged test on main line for line, so it isn't something the backport introduced. Worth fixing in both places; I'll put up a small follow-up on main that passes zoneid=cls.zone.id on all three reads so the two branches stay in sync.
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
|
just as a heads up: I have already based on this PR an addition drafted for nasbackup that enables incremental backups for Linstor + libvirt pullmode (saves extra write on the nas storage) |
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
jmsperu
left a comment
jmsperu
left a comment
There was a problem hiding this comment.
Went through the backport against #13074 and the 4.22 tree. Two notes in-thread: line 980 (sb.and("backupStatus", ...)) is dead code on 4.22 because the status filter from #13254 isn't there; harmless thanks to SearchCriteria skipping unset conditions, but worth dropping. The zoneid point in the smoke test is inherited from main rather than introduced here; I'll fix it on main separately. Otherwise this matches the merged change. Happy to re-check once the conflicts with 4.22 are resolved.
Implements incremental backup support for the NAS backup provider on KVM, using QEMU dirty bitmaps and libvirt's backup-begin API.
For large VMs this reduces daily backup storage 80–95% and shortens backup windows from hours to minutes (e.g. a 500 GB VM with moderate writes goes from ~500 GB/day to ~5–15 GB/day after the initial full backup).
Description
This PR...
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
Repeated the manual testing from #13074 (comment)
And added a few more test cases.
nas.backup.full.everybackupsnas.backup.full.everyinterval)vm_instance_details; backup record removed from DB and file deleted from NFSnas.backup.incremental.enabledmid-chain, take a backup, re-enable, take backupsactive_checkpoint_iduntouched and produces a plain legacy full with no chain metadata; re-enabling forces a fresh FULL on the first backup (since the legacy full has no matching bitmap), then resumes incrementals normallyquiescevm=trueon a VM with a live QEMU Guest Agentthawedstate, no hangdetachVolumeon a VM assigned to a backup offering withbackup.enable.attach.detach.of.volumes=false(default)nas.chain_positiondetail row (DB-level corruption), then take the next backupBackedUp), then attemptrestoreBackupHOURLYand aDAILYbackup schedule on the same VM simultaneouslybackup_schedule_ids, independentmaxBackups); each fires on its own cadence without disturbing the othermaxBackupson an existing schedule below the current schedule-attributed backup countdeleteBackupcalled on an already-removed id)size/virtualsizefields against the actual on-disk backup directory size and the VM's total provisioned disk sizeHow did you try to break this feature and the system with this change?