kvm: restore backups into the volume they were taken from - #8
Open
calvix wants to merge 28 commits into
Open
Conversation
An in-place restore of a KVM instance with more than one data disk can write a backed up disk into a different volume than the one it came from. NASBackupProvider.restoreVMBackup pairs the backed up volumes with the instance's current volumes by position in two lists that are each sorted by device id, and LibvirtRestoreBackupCommandWrapper.restoreVolumesOfExistingVM consumes those lists index by index. The volume UUID is carried along but only used to build error messages. That pairing is only correct while the instance's device ids still match the ones recorded in the backup, and the restore itself breaks that: on completion KVMGuru.importVirtualMachineFromBackup re-attaches every data disk with getNextAvailableDeviceId(). The volumes are never detached first, so their current ids still count as in use and the helper cannot return the id a volume already holds - every data disk is shifted by one slot on each restore. The root disk is unaffected because it is always re-attached at device id 0. A second restore therefore maps the backup files onto the wrong volumes. With equally sized disks the contents are silently exchanged and the API reports success. With disks of different sizes the first volume is overwritten and the restore then fails converting an image into a smaller volume, leaving that volume half rewritten with no rollback. Three changes: - KVMGuru: re-attach data disks with the device id recorded in the backup, falling back to the next free id only when the backup has none. - LibvirtRestoreBackupCommandWrapper: select the target volume by UUID instead of by list position, and fail explicitly when a backed up volume is no longer attached to the instance. - NASBackupProvider: derive the UUID list and the backup file list from the same sorted collection so the two cannot drift apart. The create-instance-from-backup path (restoreVolumesOfDestroyedVMs) is left alone: it provisions new volumes with new UUIDs, so it has nothing to match on.
LibvirtRestoreBackupCommandWrapperTest: restoring an instance whose volumes arrive in a different order than the backed up volumes must still write each backup into the volume it was taken from, and must fail explicitly when a backed up volume is no longer attached to the instance. KVMGuruTest: importing an instance from a backup must re-attach its data disks on the device ids recorded in the backup rather than allocating new ones.
calvix
force-pushed
the
fix/nas-backup-restore-volume-mapping
branch
from
August 19, 2026 12:00
8f011e1 to
4722bd4
Compare
… uuid Matching backed up volumes to the instance's volumes by uuid breaks creating an instance from a backup: that instance gets brand new volumes, so none of the uuids recorded in the backup can ever match and the restore always failed with the volume 'is not attached to the instance any more' error. When none of the recorded uuids match, the volumes are new and the device id ordering both lists already carry is the only mapping available, so fall back to it. When some of them do match, a missing one really is a detached volume and is still rejected, which is what the uuid matching was added for. The fallback also refuses to run if the two lists differ in length.
…e#13779) * Add NULL check during restoreVM operation when host is removed (apache#571) * Add NULL check during restore VM operation when host is not available/removed * fix line ending pre commit failure * update logging with details of removed host and vm --------- Co-authored-by: Sachin R Doddaguni <s_rudrappadoddagu@apple.com> (cherry picked from commit 1dbda12ccdca1aaf86ef8f9b185986c9d22b567e) * Handle null host in VM restore to prevent NPE on deleted host records * Fix build * Fix unit test --------- Co-authored-by: Sachin R <32716246+sachindoddaguni@users.noreply.github.com> Co-authored-by: Sachin R Doddaguni <s_rudrappadoddagu@apple.com> Co-authored-by: mprokopchuk <mprokopchuk@apple.com>
…ues (apache#12419) Co-authored-by: dahn <daan@onecht.net>
…s list (apache#13931) Co-authored-by: mprokopchuk <mprokopchuk@apple.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…#13502) Co-authored-by: dahn <daan@onecht.net>
…VM (apache#13558) Co-authored-by: Sachin R <32716246+sachindoddaguni@users.noreply.github.com> Co-authored-by: dahn <daan@onecht.net>
Co-authored-by: Daan Hoogland <dahn@apache.org> Co-authored-by: Suresh Kumar Anaparti <sureshkumar.anaparti@gmail.com>
…3144) Co-authored-by: toni.zamparetti <toni.zamparetti@scclouds.com.br>
…etworks with dedicated vlan (apache#13379) Co-authored-by: Pearl Dsilva <pearl1954@gmail.com>
…pache#13611) When createVolumeAsync fails, createVolumeCallback resets the volume's pool_id only when volume.getPodId() != null. Zone-wide primary storage pools have no pod, so such a volume keeps a stale pool_id while it is reverted to Allocated. On the next create/attach, findStoragePool then returns no suitable pool because storagePoolCompatibleWithVolumePool rejects the volume (its state is not Ready), and the operation fails with "Unable to find suitable primary storage" even though the pool has plenty of capacity. Guard the reset on the field that is actually being cleared (getPoolId() != null) instead of getPodId(), so it also applies to zone-wide (and local) storage. The same guard is fixed in destroyAndReallocateManagedVolume. ensureVolumeIsExpungeReady is left unchanged as it legitimately clears pod_id. Regression from apache#10757.
…emoval race (apache#13700) removeNicFromVmThroughJobQueue looked up pending work jobs by (vmType, vmId, commandName) only, so the nic was not part of the dedup key. A second removeNicFromVirtualMachine request for a different nic on the same vm matched the first still-pending VmWorkRemoveNicFromVm job and joined it instead of submitting its own. That job removes only the nic it was created for, yet both callers wait on the same job id and both receive its success, leaving the second nic silently attached while its API call reports success. Make the nic uuid part of the lookup key, mirroring addVmToNetworkThroughJobQueue which was fixed the same way in apache#5658: - look up pending jobs with the 4-arg listPendingWorkJobs(Instance, vmId, cmd, nic.getUuid()) - fail fast with CloudRuntimeException if more than one job matches - stamp new jobs with setSecondaryObjectIdentifier(nic.getUuid()) before submitting Genuine duplicates, two requests for the same nic, still dedup as before. Adds three regression tests to VirtualMachineManagerImplTest covering the cross-nic race, same-nic dedup, and the multiple-pending-jobs guard. Fixes: apache#13699 Generated-by: Claude Code (Anthropic) Signed-off-by: Ayush Sinha <ayushsinha3199@gmail.com>
…pache#13835) * kvm: fix RBD exclusive-lock leak that breaks revertSnapshot on Ceph takeRbdVolumeSnapshotOfStoppedVm() called image.snapCreate(snapshotName) twice. The first call creates the RBD snapshot, the second one always throws RbdException ("Failed to create snapshot <uuid>") because the snapshot already exists. The duplicate is a merge artifact: 30d3066 ("Merge branch '4.20' into 4.22") resolved a conflict by keeping the call from both sides - each parent had exactly one. Because there was no finally block, that exception skipped rbd.close(image) and r.ioCtxDestroy(io), so the agent kept the image open and held its RBD exclusive-lock indefinitely. The exception is only logged, so the snapshot job still reported success and the fault stayed invisible. Consequences observed on a KVM + Ceph/RBD cluster: - revertSnapshot fails with "com.ceph.rbd.RbdException: Failed to rollback snapshot <uuid>". librbd returns EROFS because a live peer holds the exclusive-lock; 'rbd snap rollback' only succeeds once that client dies and librbd can break the lock, which makes the failure look intermittent. - getRbdSnapshotSize() is never reached, so every snapshot is reported with physical size 0 when snapshot.backup.to.secondary is false. - The leaked watchers keep the image busy, so 'rbd rm' fails and the volume cannot be expunged - it stays stuck in state Destroy. Note the method also runs for RUNNING VMs: createSnapshot() branches on "RUNNING && !primaryPool.isExternalSnapshot()", and RBD is an external-snapshot pool, so every RBD volume snapshot took this path. Remove the duplicated call and move the image/IO-context cleanup into a finally block so the lock is released even if the snapshot itself fails. * kvm: release RBD handles on every path when cloning a volume from a snapshot createRBDvolumeFromRBDSnapshot() closed the source image, the cloned image and the RADOS IO context only on the success path, and called snapUnprotect() only there too. Two paths escaped that cleanup: - the early "Could not find snapshot ... on RBD" return, and - any RadosException/RbdException from clone(), resize() or flatten(), which is caught and turned into a null disk. Both leave the images open, so this client keeps the RBD exclusive-lock. That later makes 'rbd snap rollback' (revertSnapshot) fail with EROFS from another host, and keeps the image busy so 'rbd rm' cannot remove it - the volume then stays stuck in state Destroy. The failure paths after snapProtect() are worse: the snapshot stays protected, and a protected snapshot can be deleted neither on its own nor together with its volume. Move the cleanup into a finally block, tracking whether the snapshot was actually protected so it is unprotected exactly when it needs to be. Failures during cleanup are logged and never mask the original outcome; a failed snapUnprotect is logged at ERROR since it needs manual intervention. This is the same class of defect as the leak fixed in takeRbdVolumeSnapshotOfStoppedVm(); no behaviour changes on the success path. * kvm: add regression tests for the RBD snapshot handle leak Two tests around takeRbdVolumeSnapshotOfStoppedVm, using the MockedConstruction pattern already used in this test class (the Rbd instance is created inside the method under test, so it cannot be injected): - createsSnapshotExactlyOnce guards the duplicated snapCreate call from coming back, and checks the image and IO context are released. - releasesHandlesWhenSnapshotFails makes snapCreate throw and asserts the image is still closed and the IO context destroyed, so a future failure cannot leak the RBD exclusive-lock again. takeRbdVolumeSnapshotOfStoppedVm, radosConnect and getRbdSnapshotSize widened from private to protected so the test can stub the Ceph interactions. * kvm: extract RBD handle teardown into null-safe helper methods Address review feedback: the nested try-catch teardown blocks in the finally clauses of takeRbdVolumeSnapshotOfStoppedVm and createRBDvolumeFromRBDSnapshot are extracted into two reusable, null-safe helpers that log but never throw: - closeRbdImage (3 call sites) - destroyRadosIoCtx (2 call sites) No behavior change. * kvm: extract RBD snapshot unprotect into a helper method Follow-up to the teardown helpers: the snapUnprotect block in the finally clause of createRBDvolumeFromRBDSnapshot moves into a never-throwing unprotectRbdSnapshot helper, so the finally clause is now free of inline try-catch constructions entirely. No behavior change. --------- Co-authored-by: calvix <7136358+calvix@users.noreply.github.com>
Restoring a volume from a backup and attaching it to a VM has been broken since the restore commands were changed to run without a shell, in three independent ways. getDeviceToAttachDisk pipes virsh domblklist through awk, but passes the awk program still wrapped in the single quotes a shell would have stripped. Run directly, awk fails with "invalid char" and returns nothing, so the device name is empty and charAt throws StringIndexOutOfBoundsException before any attach is attempted. This affects every storage type. The exit value was also never checked, and the output not trimmed, so even a working awk would leave the trailing line separator and increment that instead of the device letter. The RBD branch passes the literal string "<<EOF%sEOF" as a virsh argument. The placeholder is never substituted with the disk XML, and a here-document cannot work without a shell, so virsh is handed a bogus argument and fails. The XML is now written to a temporary file that virsh reads. The Linstor branch declares "--subdriver qcow2", inverting the previous behaviour where Linstor got a raw attach and every other pool got qcow2. A Linstor volume is a raw DRBD block device, so libvirt rejects it with "Image is not in qcow2 format". The condition is restored, along with the "--driver qemu" that was dropped.
…ache#14064) The "Migrate instance to another host" wizard lets an operator pick a destination host and, optionally, a destination primary storage. Today the storage list is not filtered by the selected host, so it offers primary storages the host cannot reach. This filters that list down to the storages actually accessible to the selected host.
Signed-off-by: kunal.behbudzade <kunal.behbudzade@btsgrp.com> Co-authored-by: kunal.behbudzade <kunal.behbudzade@btsgrp.com>
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
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.
Description
restoreBackupof a KVM instance with more than one data disk can write abacked up disk into a different volume than the one it was taken from. This happens only on a second or further restore of a VM from a backup that was taken. ie: create backup of VM, restore 1, then restore 2 again.
success: true, but the appearance of the bug is not 100% because the enumeration can randomly be the same as before. I recommend trying instances with 5 or 7 data disks for testing where occurrence is very likely.fails converting the larger image into the smaller volume, and returns an error after the data is already
destroyed, with no rollback
createVMFromBackupandrestoreVolumeFromBackupAndAttachToVMare not affectedReproduced on 4.22.1.0 with KVM + Ceph/RBD primary storage and the NAS backup provider.
At first, this can be a very silent error, especially if the VM uses mount by LABEL/UUID, as the VM will mount the data to the right mount paths, but volume A now contains the data of volume B ( including LABELs) and volume B contains data for volume A.
Logs
case 1 - different disk sizes
The logs show output showing this bug on the VM, with 2 data volumes
diska,diskb- all stored onCeph primary storageand backed up via backup and recovery plugin tocephfsstorage.diskcheckis a simple bash util that prints useful data from the disks on a remote machine and connects via ssh.If the disk sizes are the same, the second restore works but disk can be swapped depending on the scenario how disk are renumbered.
Root cause
NASBackupProvider.restoreVMBackupbuilds two lists the backed up volumes and the instance's currentvolumes, each sorted by device id, and
LibvirtRestoreBackupCommandWrapper.restoreVolumesOfExistingVMconsumes them index by index:
BackupManagerImpl.restoreBackup-importRestoredVM-KVMGuru.importVirtualMachineFromBackupre-attaches every data disk with
getNextAvailableDeviceId(). The volumes are never detached first, sotheir current ids still count as "in use" and the helper can never return the id a volume already holds —
every data disk is shifted by one slot on every restore.
A restore is therefore only correct while the instance's device ids still match the ones recorded in the
backup. The first restore satisfies that and then breaks it for the next one.
Behaviour, restore by restore
The first restore is always correct. It is the restore itself that can plant the fault, so the damage only
appears on second restore.
Restore #1 — succeeds, but renumbers the disks
The device ids recorded in the backup still match the instance, so the two sorted lists line up and every
backup lands in its own volume. Verified on a 2-data-disk instance by reading the volumes directly from
primary storage — each still held its own filesystem.
But on completion
KVMGuru.importVirtualMachineFromBackupre-attaches the data disks withgetNextAvailableDeviceId():t
After the first restore, diskA gets id 4 because ids 1 and 2 are taken; id 3 is cdrom, so the next free id is 4. diskb gets id 1 because the id was just freed.
Restore #2 — the damage, and what it looks like depends on the disk sizes
Sorted by device id the instance is now
[ROOT, demo-disk-b, demo-disk-a]while the backup is[ROOT, demo-disk-a, demo-disk-b], so index 1 and 2 point at each other's volumes.Equal-sized data disks (1 GB + 1 GB) — silent corruption. Both images fit their new targets, so
qemu-imgsucceeds and the API returnssuccess: true. The volume contents are simply exchanged:A guest that mounts by
LABEL=/UUID=still mounts everything at the right paths, because the labelstravel with the filesystems — so nothing looks wrong from inside the instance, while CloudStack's
volume-to-content mapping is now wrong for every per-volume operation (detach, delete, snapshot, resize,
restore single volume).
Differently sized data disks (1 GB + 2 GB), data loss, reported as a failure.
The loop processes the disks in order and only aborts on the second pairing, so the first one has already been written.
The API returns:
and the agent log shows the real cause — the 2 GB backup being written into the 1 GB volume:
State afterwards — the restore is reported as failed, but one volume is already gone, with no rollback:
Both volumes still report
Readyin CloudStack. An operator seeing error 530 would reasonably assumenothing happened.
How to reproduce
Instance with two data disks, a NAS backup offering, KVM primary storage. Use two different sizes to get
the loud failure, if you want to reproduce the disk swap, I recommend using more data disks, as there is a chance that reumbering accdientaly get right and the bug does not occur. So trying like 5 or 7 data disks should most likely show the bug.
Types of changes
How Has This Been Tested?
Unit tests — three added, all of which fail without the patch:
KVMGuruTestLibvirtRestoreBackupCommandWrapperTestEvery pre-existing test in both classes passes either way.
LibvirtRestoreBackupCommandWrapperTest.testRestoreOfExistingVmMapsBackupsToVolumesByUuid— theinstance's volumes arrive in a different order than the backed up volumes; each backup must still be
written into the volume it was taken from. Also asserts with
never()that neither data disk's backup iswritten into the other one.
LibvirtRestoreBackupCommandWrapperTest.testRestoreOfExistingVmFailsWhenBackedUpVolumeIsNoLongerAttachedKVMGuruTest.testImportVirtualMachineFromBackupReinstatesRecordedDeviceIds— a data disk recorded ondevice id 5 must be re-attached on device id 5, and
getNextAvailableDeviceId()must not be consulted.Live testing — a patched build was deployed to a KVM + Ceph/RBD zone (management server and KVM
agents). The same 2-data-disk instance was restored three times in place:
0,1,2on every restore (unpatched:1,2→4,1after the first)rbd exportand from inside the guestcreateVMFromBackupverified unaffected