Skip to content

Commit

Permalink
snapshots: Skip empty media drives for external snapshots
Browse files Browse the repository at this point in the history
Instead of falling back to internal ones. This makes external
snapshots work in more cases.

Cherry-picked from main commit 914e1e1
  • Loading branch information
mvollmer authored and martinpitt committed May 1, 2024
1 parent 852a3c4 commit 0fe1062
Show file tree
Hide file tree
Showing 3 changed files with 23 additions and 14 deletions.
4 changes: 2 additions & 2 deletions src/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -899,8 +899,8 @@ export function vmSupportsExternalSnapshots(config, vm, storagePools) {
return false;
}

// Currently external snapshots work only for disks of type "file" with a source.
if (!disks.every(disk => (disk.type === "file" && disk.source?.file))) {
// Currently external snapshots work only for disks of type "file"
if (!disks.every(disk => disk.type === "file")) {
logDebug(`vmSupportsExternalSnapshots: vm ${vm.name} has unsupported disk type`);
return false;
}
Expand Down
27 changes: 18 additions & 9 deletions src/libvirt-xml-create.js
Original file line number Diff line number Diff line change
Expand Up @@ -263,15 +263,24 @@ export function getSnapshotXML(name, description, disks, memoryPath, isExternal,

const disksElem = doc.createElement('disks');
disks.forEach(disk => {
// Disk can have attribute "snapshot" set to "no", which means no snapshot should be created of the said disk
// This cannot be configured through cockpit, but we should uphold it nevertheless
// see "snapshot" attribute of <disk> element at https://libvirt.org/formatdomain.html#hard-drives-floppy-disks-cdroms
if (disk.snapshot !== "no") {
const diskElem = doc.createElement('disk');
diskElem.setAttribute('name', disk.target);
diskElem.setAttribute('snapshot', 'external');
disksElem.appendChild(diskElem);
}
// Disk can have attribute "snapshot" set to "no", which
// means no snapshot should be created of the said disk
// This cannot be configured through cockpit, but we
// should uphold it nevertheless.
//
// See "snapshot" attribute of <disk> element at
// https://libvirt.org/formatdomain.html#hard-drives-floppy-disks-cdroms
if (disk.snapshot == "no")
return;

// Skip disks without source, such as empty media drives.
if (isExternal && disk.type == "file" && !disk.source.file)
return;

const diskElem = doc.createElement('disk');
diskElem.setAttribute('name', disk.target);
diskElem.setAttribute('snapshot', 'external');
disksElem.appendChild(diskElem);
});
snapElem.appendChild(disksElem);
}
Expand Down
6 changes: 3 additions & 3 deletions test/check-machines-snapshots
Original file line number Diff line number Diff line change
Expand Up @@ -241,8 +241,9 @@ class TestMachinesSnapshots(machineslib.VirtualMachinesCase):

def revert(self):
b.click(f"#vm-subVmTest1-snapshot-{self.snap_num}-revert")
b.wait_visible(".pf-v5-c-modal-box")
b.click('.pf-v5-c-modal-box__footer button:contains("Revert")')
b.wait_not_present('.pf-v5-c-modal-box__footer button:contains("Revert")')
b.wait_not_present(".pf-v5-c-modal-box")

def cleanup(self):
b.click(f"#delete-vm-subVmTest1-snapshot-{self.snap_num}")
Expand Down Expand Up @@ -352,7 +353,6 @@ class TestMachinesSnapshots(machineslib.VirtualMachinesCase):
).execute()

# With CD-ROM drive without media
# doesn't support external snapshots yet
b.click("#vm-subVmTest1-disks-sdd-eject-button")
b.click(".pf-v5-c-modal-box__footer button:contains(Eject)")
b.wait_not_present(".pf-v5-c-modal-box")
Expand All @@ -361,7 +361,7 @@ class TestMachinesSnapshots(machineslib.VirtualMachinesCase):
self,
name="nomedia",
state="shutoff",
expect_external=False,
expect_external=supports_external,
).execute()

# delete CD-DROM again so that the next test can use external snapshot again
Expand Down

0 comments on commit 0fe1062

Please sign in to comment.