kvm: restore backups into the volume they were taken from - #13922
Open
calvix wants to merge 2 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.
Member
|
@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. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.22 #13922 +/- ##
=========================================
Coverage 17.77% 17.78%
- Complexity 15982 15988 +6
=========================================
Files 5928 5928
Lines 534301 534310 +9
Branches 65382 65385 +3
=========================================
+ Hits 94987 95021 +34
+ Misses 428568 428539 -29
- Partials 10746 10750 +4
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:
|
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18917 |
Member
|
@blueorangutan test |
|
@weizhouapache a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian Build Failed (tid-16786) |
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
4 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.
Description
This is an issue I noticed when using the Recovery and backup plugin on KVM + CEPH, but might be related to all KVM volume storage.
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 with same disk sizes, 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