Repository navigation
Conversation
rp-
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 4.22 #14194 +/- ##
============================================
+ Coverage 18.01% 18.04% +0.02%
- Complexity 16265 16298 +33
============================================
Files 5936 5936
Lines 536395 536506 +111
Branches 65701 65726 +25
============================================
+ Hits 96658 96811 +153
+ Misses 428710 428656 -54
- Partials 11027 11039 +12
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:
|
|
@rp- are the changes here relavant for 4.22 branch? if so, please re-target to 4.22. |
thanks, missed the wrong base branch |
|
@blueorangutan package |
|
@rp- 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 ✖️ debian ✖️ suse15. SL-JID 19402 |
|
@blueorangutan package |
|
@weizhouapache 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 19405 |
|
@blueorangutan test |
|
@weizhouapache a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
Also pinging for this here @sureshanaparti to be included in 4.22.2 |
|
[SF] Trillian test result (tid-17075)
|
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
1 similar comment
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
abh1sar
left a comment
abh1sar
left a comment
There was a problem hiding this comment.
I found one issue not in this PR, but related to Linstor NAS backup support.
replaceBlockDeviceWithBackup calls QemuImg with skipZeroIfSupported=true.
This means restore assumes that the target disk is zeroes and doesn't copy zeroed blocks. This doesn't work for in-place restore. In that case every zero region of the backup is skipped and the volume keeps whatever was there.
| if backup_out=$(virsh -c qemu:///system backup-begin --domain "$VM" --backupxml "$backupxml" 2>&1); then | ||
| backup_begin=1 | ||
| fi | ||
|
|
There was a problem hiding this comment.
The backup job will be left forever running if there is an interrupt or error before calling domjobabort.
Consider adding a trap and cleanup.
| * does anchor a chain so later content-incrementals can hang off it. | ||
| */ | ||
| static ChainDecision contentFull() { | ||
| return new ChainDecision(NASBackupChainKeys.TYPE_CONTENT_FULL, null, null, null, |
There was a problem hiding this comment.
Instead of overloading the mode for each ChainDecision, I suggest adding a new method field to it.
The method can be qcow2 / content / none / rbd_diff(ceph - can be added later).
The mode will still say full and incremental for each method.
#14386 also adds rbd-full and rbd-incremental. I think a new method will be a better choice for both PRs.
| * | ||
| * @since 4.22.0 | ||
| */ | ||
| public static final Property<String> NAS_BACKUP_PULL_SCRATCH_DIR = new Property<>("nas.backup.pull.scratch.dir", "/var/tmp"); |
There was a problem hiding this comment.
If the scratch dir gets full, the VM writes are stalled and it is paused. If I am not wrong, the VM is never resumed.
Consider either creating the scratch dir as a new volume on the Linstor pool itself / do a space check before starting backup / test the pause and resume workflow.
There was a problem hiding this comment.
Yes this is the real problem with the pull mode, I added a disk space check and also by default now use the old push mode (with its drawbacks), but now it is an opt-in and admins have to make sure they have enough space or configure a correct scratch dir.
| # room for the write churn during the backup. 711 lets the qemu process traverse in to | ||
| # the files libvirt creates as root. | ||
| local scratch_dir | ||
| scratch_dir=$(mktemp -d "$SCRATCH_PARENT/csbackup-scratch.XXXXXX") || { |
There was a problem hiding this comment.
what happens to the running vm if /var/tmp fills up during a long backup of a busy disk? does the backup just fail or can the vm writes start failing too?
There was a problem hiding this comment.
the check before start helps, but with the default 25 a busy vm that rewrites more than a quarter of its disk still gets paused and stays paused. can we stop the backup and resume the vm when the scratch area runs low?
There was a problem hiding this comment.
Well its a setting and should be set to the users usage scenarios, even 50% can be too much if you think of a VM with a really large data disk like 20TB which maybe doesn't do much writes, but still you would need then a scratch space of 10TB...
There was a problem hiding this comment.
fair, with pull mode opt-in and the size up to the admin thats fine
| // Those VMs take the content-based pull-mode path instead. It is chosen regardless of the | ||
| // master switch, because even a full backup benefits: pull mode writes a sparse qcow2 in a | ||
| // single pass rather than a fully allocated one needing a second re-convert pass. | ||
| if (allVolumesOnContentDiffCapableStorage(vm)) { |
There was a problem hiding this comment.
should linstor vms stay on the old full backup when incremental is turned off? right now they switch to the new method after upgrade even if the admin never opted in
There was a problem hiding this comment.
Yes it is probably the safer bet to keep the push mode by default and let people opt in to the pull mode (then they should also be aware to configure a proper scratch dir
There was a problem hiding this comment.
makes sense, checking the incremental setting first in 3c1820b covers it
Raw block-device disks such as LINSTOR/DRBD cannot carry QEMU persistent dirty
bitmaps -- persistence is a qcow2-only feature -- so libvirt checkpoints, and
with them the push-mode incremental path, are unavailable there. Those VMs were
therefore pinned to full-only backups, and push mode additionally wrote a fully
allocated qcow2 that needed a second re-convert pass to reclaim space.
Add a content-based path driven by libvirt pull mode. libvirt exposes a
point-in-time NBD view of each disk, held consistent by copy-before-write into
local fleecing scratch images, and the backup is derived from that export:
content-full qemu-img convert -> sparse qcow2 in a single pass, so the
bloated push output and its re-convert pass both go away.
content-incremental a qcow2 overlay backed by the NBD export, then a safe
rebase onto the parent, merging in exactly the clusters
that differ -> a delta qcow2 backed by the parent.
The delta has the same chain shape the push-mode incremental produces (relative
backing path), so the existing restore and cascade-delete logic applies
unchanged. A missing or short parent list degrades to a full and emits
INCREMENTAL_FALLBACK=true, mirroring the push-mode fallback.
Unlike a storage-snapshot based diff there is no trailing DRBD metadata to clip:
the DRBD device, and therefore the NBD export, is already net-sized.
The scratch dir is host-local and configurable with -S, defaulting to /var/tmp;
it must not sit on the NAS share since copy-before-write runs inline with guest
writes. NBD socket paths are validated against the kernel's 108-byte limit up
front to avoid an opaque qemu-img failure later.
The incremental NAS backup feature gates on QEMU dirty bitmaps, which can only be persisted inside a qcow2. LINSTOR volumes are raw DRBD block devices, so allVolumesOnCheckpointCapableStorage() rejected them and every LINSTOR VM was pinned to full-only backups regardless of the configured cadence. Route those VMs to the content-based path instead, which derives the delta by comparing the disk's point-in-time NBD export against the parent backup rather than by tracking dirty blocks: - decideContentChain() mirrors the bitmap cadence logic (master switch, stopped VMs, nas.backup.full.every, chain length) but anchors on backup history alone. There is no host-side bitmap to invalidate, so unlike the bitmap chain a VM restart, live migration or restore cannot break it -- content comparison is stateless and self-correcting. - The path is taken even when the incremental master switch is off, because a content-full is still a single-pass sparse write, strictly better than the fully allocated push-mode output it replaces. - ChainDecision carries the parent backup uuid directly, since a content chain has no bitmap name to resolve the parent by. - Mixed-storage VMs stay off the content path: one backup run uses one mode. The agent wrapper accepts the two new modes -- content-incremental requires only its per-disk parent paths, no bitmap -- and passes the host-local pull-mode scratch directory from the new nas.backup.pull.scratch.dir agent property. Adds 6 unit tests covering the routing and cadence decisions. All 21 pre-existing NASBackupProviderTest cases and the KVM wrapper tests still pass.
A pull-mode backup job never finishes on its own; nasbackup.sh ends it with domjobabort once the data is read. Any exit the script did not plan for -- an unexpected error under "set -e", SIGINT/SIGTERM/SIGHUP -- left the job running, keeping the NBD server and the growing fleecing images alive and blocking every later backup or migration of the VM. Arm an EXIT trap once the scratch dir exists: it aborts the job (through apache#14305's BACKUP_JOB_ACTIVE/abort_backup_job), removes the scratch dir and runs cleanup(). This replaces the per-error-path abort/cleanup copies. The agent kills a timed-out script with SIGKILL, which no trap can catch, so LibvirtTakeBackupCommandWrapper aborts the job itself after a pull-mode timeout.
The content-based path overloaded ChainDecision.mode with content-full and content-incremental, and apache#14386 is adding rbd-full and rbd-incremental the same way. Split the two concerns as suggested in review: mode stays full or incremental, and a new method field says how the backup is taken -- qcow2 (push mode with dirty bitmaps) or content (pull mode). Further methods such as an RBD diff can be added without new mode tokens. TakeBackupCommand gets a method field and nasbackup.sh a --method option. Both default to qcow2 when absent, so a management server that does not send a method keeps working against an upgraded agent. legacy-full is unchanged: rewriting it as full plus a "none" method would break incremental-disabled backups on agents that have not been upgraded yet.
Copy-before-write saves every guest block that is overwritten during a pull-mode backup into the host-local scratch area. If that fills up, the guest's writes fail, QEMU pauses the VM (the default werror=enospc policy) and nothing resumes it. Each block is saved at most once, so the VM's total disk size is the worst case. Refuse to start the backup unless the scratch area has the share of that set by the new agent.properties option nas.backup.pull.scratch.min.free.percent. The default of 25 assumes the guest overwrites at most a quarter of its disks while a backup runs; 100 rules out a full scratch area, 0 disables the check. The check does not account for concurrent pull-mode backups sharing the same scratch filesystem.
… off LINSTOR VMs took the content-based pull-mode path even with nas.backup.incremental.enabled off, so an upgrade switched existing users to the new mechanism, with its host-local scratch space requirement, without them opting in. Apply the master switch before the storage routing: with incrementals disabled every VM, LINSTOR included, gets the legacy push-mode full as before. Enabling incrementals is the opt-in to the content-based path.
replaceBlockDeviceWithBackup created its QemuImg with skipZeroIfSupported, so for an existing target device qemu-img convert ran with --target-is-zero and skipped every zero region of the backup -- at qcow2 cluster granularity, not the -S 1M chunk size. On an in-place restore the volume keeps its old data there, silently corrupting the restored disk. A newly created LINSTOR volume is only safe on thin or ZFS backed pools; thick LVM hands out stale data too. Add KVMStoragePool.newVolumeReadsAsZero(), false by default; LinstorStoragePool answers it with LinstorUtil.resourceSupportZeroBlocks(), the check LinstorStorageAdaptor.copyPhysicalDisk already uses. The restore now skips zero regions only on a volume it just created and the pool reports as zeroed, and requests -n explicitly for an existing target, which it previously got only through the zero-skipping branch. RBD targets are unaffected: --target-is-zero never applied to them.
d509516 to
1d9b64c
Compare
This branch has not been deployed
7 participants
Description
This PR adds incremental NAS backup support for KVM VMs whose disks live on raw block-device storage such as LINSTOR/DRBD.
#13877 (backport of #13074) implements incremental NAS backups with QEMU dirty bitmaps and libvirt checkpoints. Bitmap persistence is a qcow2-only feature, so
allVolumesOnCheckpointCapableStorage()excludes raw block devices and every LINSTOR VM stays pinned to full-only backups regardless of the configured cadence. On top of that, the push-mode full backup of a raw device writes a fully allocated qcow2 which then needs a secondqemu-img convertpass to reclaim space.This PR adds a content-based backup path driven by libvirt pull mode and routes VMs on content-diff-capable storage (currently LINSTOR) to it:
nasbackup.sh– newcontentbackup methodThe script gets a
--methodoption next to-M|--mode:qcow2(default, the existing push-mode path with dirty bitmaps) orcontent. With--method content, libvirt is asked for a pull-mode backup: it starts an NBD server exposing a point-in-time view of each disk, held consistent by copy-before-write into host-local fleecing scratch images. The backup is then derived from that export:full:qemu-img convertfrom the NBD export straight into a sparse qcow2 in a single pass. This replaces the bloated push-mode output and its re-convert pass.incremental: a qcow2 overlay backed by the NBD export, then a safeqemu-img rebaseonto the parent backup. This merges in exactly the clusters that differ and yields a delta qcow2 backed by the parent, with a relative backing path. The chain has the same shape as a bitmap incremental, so the existing restore and cascade-delete logic applies unchanged.A missing or short parent list degrades to a full and emits
INCREMENTAL_FALLBACK=true, mirroring the push-mode fallback. Also handles the raw/dev/drbdNdevice paths of long-running VMs the same way the push/stopped paths do (viaget_linstor_uuid_from_device).The scratch dir is host-local and configurable with
-S(default/var/tmp). It must not be on the NAS share, since copy-before-write runs inline with guest writes. NBD socket paths are validated against the kernel's 108-byte UNIX socket limit up front instead of failing opaquely inqemu-imglater.If the scratch area fills up during a backup, copy-before-write fails the guest's writes and QEMU pauses the VM. Before starting, the script therefore checks that the scratch filesystem has free space for a share of the VM's total disk size, the worst case since each block is saved at most once (
--scratch-min-free-percent, default 25, 0 disables the check). Otherwise the backup fails up front with a message naming the required and available space. Concurrent pull-mode backups sharing one scratch filesystem are not accounted for together.A pull-mode job never ends on its own, so an EXIT/INT/TERM/HUP trap ends it (via
abort_backup_job()from #14305), removes the scratch dir and cleans up on any failed exit. The freeze/thaw handling uses the helpers from #14305 as well.NASBackupProvider– routing and cadencedecideContentChain()mirrors the bitmap cadence logic (stopped VMs,nas.backup.full.every, chain length) but anchors on backup history alone. There is no host-side bitmap that can be invalidated, so a VM restart, live migration or restore cannot break the chain; content comparison is stateless and self-correcting.nas.backup.incremental.enabledapplies first: with incrementals disabled every VM, LINSTOR included, keeps the legacy push-mode full backup. Enabling incrementals is the opt-in to the content-based path.ChainDecisionseparates the two concerns:modeisfullorincremental, a newmethodfield isqcow2orcontent. Further methods (e.g. an RBD diff) can be added without new mode tokens.ChainDecisioncarries the parent backup uuid directly, since a content chain has no bitmap name to resolve the parent by.LibvirtTakeBackupCommandWrapper/ agentTakeBackupCommandcarries the newmethodfield, passed to the script as--method. When absent the agent assumesqcow2, so a management server that does not send it keeps working. Acontentincremental requires only its per-disk parent paths, no bitmap. Newagent.propertieskeys:nas.backup.pull.scratch.dir(default/var/tmp): host-local scratch area.nas.backup.pull.scratch.min.free.percent(default25): free space the scratch area needs before a pull-mode backup starts, as a percentage of the VM's total disk size.The agent kills a timed-out script with SIGKILL, which no trap can catch, so after a pull-mode timeout the wrapper aborts the backup job itself.
Restore onto LINSTOR: do not skip zero regions (bug fix, independent of the above)
replaceBlockDeviceWithBackupranqemu-img convertwith--target-is-zerowhenever the target device existed, skipping every zero/unallocated qcow2 cluster of the backup. On an in-place restore those clusters kept the volume's current data, and a new LINSTOR volume on thick LVM is not zeroed either. A newKVMStoragePool.newVolumeReadsAsZero()hook (false by default,LinstorStoragePoolanswers it withLinstorUtil.resourceSupportZeroBlocks()) now limits zero skipping to freshly created volumes on thin or ZFS backed pools. RBD targets were never affected.Behaviour for VMs on qcow2/NFS/Ceph storage is unchanged; they keep using the bitmap path from #13877.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
Unit tests
Six new
NASBackupProviderTestcases cover the routing and cadence decisions:decideChainReturnsLegacyFullForLinstorWhenIncrementalDisableddecideChainReturnsContentFullForStoppedLinstorVmdecideChainReturnsContentIncrementalForLinstorWithValidParentdecideChainReturnsContentFullForLinstorAtChainEndallVolumesOnContentDiffCapableStorageFalseForMixedStorageallVolumesOnContentDiffCapableStorageFalseForNoVolumesLibvirtTakeBackupCommandWrapperTestcovers the--method/ scratch arguments, the method validation and the job abort after a pull-mode timeout.LibvirtRestoreBackupCommandWrapperTestcovers zero skipping for in-place, new thick and new thin LINSTOR restores.All
NASBackupProviderTest(29),LibvirtTakeBackupCommandWrapperTest(10) andLibvirtRestoreBackupCommandWrapperTest(23) cases pass, as do the full core, KVM plugin and NAS plugin suites, after rebasing onto current 4.22.End-to-end on a 3-host KVM/LINSTOR cluster, run on the initial revision, before the review changes above (EXIT trap,
methodfield, scratch space check, legacy full while incrementals are off, restore fix); not yet repeated on the current one (management server + 3 agents, NFS backup repository,nas.backup.incremental.enabled=true,nas.backup.full.every=3):full.every=3qemu-img checkHow did you try to break this feature and the system with this change?
nas.backup.pull.scratch.dirat a deep directory so the NBD socket path exceeded 108 bytes. The script fails up front with a clear message and a hint to shorten the dir, instead of an opaqueqemu-imgconnection error mid-backup; cleanup runs and no partial backup dir is left behind.INCREMENTAL_FALLBACK=true, and the DB records a full.qemu-imgduring the pull): the backup job is aborted, scratch dir and partial output removed, mount released; the next backup succeeds./dev/drbdNinstead of/dev/drbd/by-res/…in live XML): resolved via udev like the push path; if the UUID cannot be derived the backup fails hard rather than producing a file restore cannot find.