From b1a69e3f12e233c35dc2d7307518245f3c9bef73 Mon Sep 17 00:00:00 2001 From: Simon Kobyda Date: Fri, 27 Oct 2023 01:45:54 +0200 Subject: [PATCH] Create external snapshots when supported External VM snapshots are more reliable and flexible than the internal format. They also work for raw disk images, not just for qcow2. In addition, the internal format was declared deprecated in RHEL 8 [1]. The external format is supported with libvirt 9.9.0 or later. Detect it through the capability and use it when available. If the VM is running, add a a memory snapshot file path input to the snapshot creation dialog. Co-Authored-By: Martin Pitt [1] https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/8/html/8.4_release_notes/deprecated_functionality#deprecated-functionality_virtualization --- .../vm/snapshots/vmSnapshotsCard.jsx | 8 +- .../vm/snapshots/vmSnapshotsCreateModal.jsx | 79 +++++++++++++++--- src/components/vm/vmDetailsPage.jsx | 2 +- src/helpers.js | 39 +++++++++ src/libvirt-xml-create.js | 38 ++++++++- src/libvirt-xml-parse.js | 1 + src/libvirtApi/helpers.js | 3 +- src/libvirtApi/snapshot.js | 9 ++- test/check-machines-snapshots | 81 +++++++++++++++++-- 9 files changed, 232 insertions(+), 28 deletions(-) diff --git a/src/components/vm/snapshots/vmSnapshotsCard.jsx b/src/components/vm/snapshots/vmSnapshotsCard.jsx index a7e2dcd94..24217f884 100644 --- a/src/components/vm/snapshots/vmSnapshotsCard.jsx +++ b/src/components/vm/snapshots/vmSnapshotsCard.jsx @@ -20,7 +20,7 @@ import React from 'react'; import cockpit from 'cockpit'; import { useDialogs, DialogsContext } from 'dialogs.jsx'; -import { vmId, localize_datetime } from "../../../helpers.js"; +import { vmId, localize_datetime, vmSupportsExternalSnapshots } from "../../../helpers.js"; import { CreateSnapshotModal } from "./vmSnapshotsCreateModal.jsx"; import { ListingTable } from "cockpit-components-table.jsx"; import { Button } from "@patternfly/react-core/dist/esm/components/Button"; @@ -35,12 +35,16 @@ import './vmSnapshotsCard.scss'; const _ = cockpit.gettext; -export const VmSnapshotsActions = ({ vm }) => { +export const VmSnapshotsActions = ({ vm, config, storagePools }) => { const Dialogs = useDialogs(); const id = vmId(vm.name); + const isExternal = vmSupportsExternalSnapshots(config, vm, storagePools); + function open() { Dialogs.show(); } diff --git a/src/components/vm/snapshots/vmSnapshotsCreateModal.jsx b/src/components/vm/snapshots/vmSnapshotsCreateModal.jsx index 31455f56e..1b8a1d5aa 100644 --- a/src/components/vm/snapshots/vmSnapshotsCreateModal.jsx +++ b/src/components/vm/snapshots/vmSnapshotsCreateModal.jsx @@ -28,20 +28,25 @@ import { TextInput } from "@patternfly/react-core/dist/esm/components/TextInput" import { FormHelper } from 'cockpit-components-form-helper.jsx'; import { DialogsContext } from 'dialogs.jsx'; import { ModalError } from "cockpit-components-inline-notification.jsx"; +import { FileAutoComplete } from "cockpit-components-file-autocomplete.jsx"; import { snapshotCreate, snapshotGetAll } from "../../../libvirtApi/snapshot.js"; +import { getSortedBootOrderDevices, LIBVIRT_SYSTEM_CONNECTION } from "../../../helpers.js"; const _ = cockpit.gettext; +let current_user = null; +cockpit.user().then(user => { current_user = user }); + const NameRow = ({ onValueChanged, name, validationError }) => { return ( onValueChanged("name", value)} /> - + ); }; @@ -58,6 +63,40 @@ const DescriptionRow = ({ onValueChanged, description }) => { ); }; +function getDefaultMemoryPath(vm, snapName) { + // Choosing a default path where memory snapshot should be stored might be tricky. Ideally we want + // to store it in the same directory where the primary disk (the disk which is first booted) is stored + // If howver no such disk can be found, we should fallback to libvirt's default /var/lib/libvirt + const devices = getSortedBootOrderDevices(vm).filter(d => d.bootOrder && + d.device.device === "disk" && + d.device.type === "file" && + d.device.source.file); + if (devices.length > 0) { + const primaryDiskPath = devices[0].device.source.file; + const directory = primaryDiskPath.substring(0, primaryDiskPath.lastIndexOf("/") + 1); + return directory + snapName; + } else { + if (vm.connectionName === LIBVIRT_SYSTEM_CONNECTION) + return "/var/lib/libvirt/memory/" + snapName; + else if (current_user) + return current_user.home + "/.local/share/libvirt/memory/" + snapName; + } + + return ""; +} + +const MemoryPathRow = ({ onValueChanged, memoryPath, validationError }) => { + return ( + + onValueChanged("memoryPath", value)} + superuser="try" + value={memoryPath} /> + + + ); +}; + export class CreateSnapshotModal extends React.Component { static contextType = DialogsContext; @@ -66,9 +105,11 @@ export class CreateSnapshotModal extends React.Component { // cut off seconds, subseconds, and timezone const now = new Date().toISOString() .replace(/:[^:]*$/, ''); + const snapName = props.vm.name + '_' + now; this.state = { - name: props.vm.name + '_' + now, + name: snapName, description: "", + memoryPath: getDefaultMemoryPath(props.vm, snapName), inProgress: false, }; @@ -87,8 +128,8 @@ export class CreateSnapshotModal extends React.Component { } onValidate(submitted = false) { - const { name } = this.state; - const { vm } = this.props; + const { name, memoryPath } = this.state; + const { vm, isExternal } = this.props; const validationError = {}; if (vm.snapshots.findIndex(snap => snap.name === name) > -1) @@ -96,20 +137,33 @@ export class CreateSnapshotModal extends React.Component { else if (!name && submitted) validationError.name = _("Name should not be empty"); + if (isExternal && vm.state === "running" && !memoryPath) + validationError.memory = _("Memory file can not be empty"); + return validationError; } onCreate() { const Dialogs = this.context; - const { vm } = this.props; - const { name, description } = this.state; + const { vm, isExternal, storagePools } = this.props; + const { name, description, memoryPath } = this.state; + const disks = Object.values(vm.disks); const validationError = this.onValidate(true); this.setState({ submitted: true }); if (!Object.keys(validationError).length) { this.setState({ inProgress: true }); - snapshotCreate({ connectionName: vm.connectionName, vmId: vm.id, name, description }) + snapshotCreate({ + connectionName: vm.connectionName, + vmId: vm.id, + name, + description, + memoryPath: vm.state === "running" && memoryPath, + disks, + isExternal, + storagePools + }) .then(() => { // VM Snapshots do not trigger any events so we have to refresh them manually snapshotGetAll({ connectionName: vm.connectionName, domainPath: vm.id }); @@ -124,14 +178,17 @@ export class CreateSnapshotModal extends React.Component { render() { const Dialogs = this.context; - const { idPrefix } = this.props; - const { name, description, submitted } = this.state; + const { idPrefix, isExternal, vm } = this.props; + const { name, description, memoryPath, submitted } = this.state; const validationError = this.onValidate(submitted); const body = (
e.preventDefault()} isHorizontal> - + + {isExternal && vm.state === 'running' && + } ); diff --git a/src/components/vm/vmDetailsPage.jsx b/src/components/vm/vmDetailsPage.jsx index c7f7ea281..98fbfeb12 100644 --- a/src/components/vm/vmDetailsPage.jsx +++ b/src/components/vm/vmDetailsPage.jsx @@ -175,7 +175,7 @@ export const VmDetailsPage = ({ id: cockpit.format("$0-snapshots", vmId(vm.name)), className: "snapshots-card", title: _("Snapshots"), - actions: , + actions: , body: }); } diff --git a/src/helpers.js b/src/helpers.js index 3bdbd532b..9bd71a869 100644 --- a/src/helpers.js +++ b/src/helpers.js @@ -873,3 +873,42 @@ export function getIfaceSourceName(iface) { export function canDeleteDiskFile(disk) { return disk.type === "volume" || (disk.type === "file" && disk.source.file); } + +export function getStoragePoolPath(storagePools, poolName, connectionName) { + const pool = storagePools.find(pool => pool.name === poolName && pool.connectionName === connectionName); + + return pool?.target?.path; +} + +export function vmSupportsExternalSnapshots(config, vm, storagePools) { + // External snapshot should only be used if the VM's os types/architecture allow it + // and if snapshot features are present among guest capabilities: + // https://libvirt.org/formatcaps.html#guest-capabilities + if (!config.capabilities?.guests.some(guest => guest.osType === vm.osType && + guest.arch === vm.arch && + guest.features.externalSnapshot)) { + logDebug(`vmSupportsExternalSnapshots: vm ${vm.name} has no external snapshot support`); + return false; + } + + // If at leat one disk has internal snapshot preference specified, use internal snapshot for all disk, + // as mixing internal and external is not allowed + const disks = Object.values(vm.disks); + if (disks.some(disk => disk.snapshot === "internal")) { + logDebug(`vmSupportsExternalSnapshots: vm ${vm.name} has internal snapshot preference specified`); + return false; + } + + // Currently external snapshots works only for disks where we can specify a local path of a newly created disk snapshot file + // This rules out all disks which are not file-based, or pool-based where pool is of type "dir" + const supportedPools = storagePools.filter(pool => pool.type === "dir" && pool.connectionName === vm.connectionName); + if (!disks.every(disk => + disk.type === "file" || + (disk.type === "volume" && supportedPools.some(pool => pool.name === disk.source.pool)) + )) { + logDebug(`vmSupportsExternalSnapshots: vm ${vm.name} has unsupported disk type`); + return false; + } + + return true; +} diff --git a/src/libvirt-xml-create.js b/src/libvirt-xml-create.js index d1747c583..bdcba0293 100644 --- a/src/libvirt-xml-create.js +++ b/src/libvirt-xml-create.js @@ -1,3 +1,5 @@ +import { getStoragePoolPath } from "./helpers.js"; + export function getDiskXML(type, file, device, poolName, volumeName, format, target, cacheMode, shareable, busType, serial) { const doc = document.implementation.createDocument('', '', null); @@ -216,7 +218,8 @@ export function getPoolXML({ name, type, source, target }) { return new XMLSerializer().serializeToString(doc.documentElement); } -export function getSnapshotXML(name, description) { +// see https://libvirt.org/formatsnapshot.html +export function getSnapshotXML(name, description, disks, memoryPath, isExternal, storagePools, connectionName) { const doc = document.implementation.createDocument('', '', null); const snapElem = doc.createElement('domainsnapshot'); @@ -233,6 +236,39 @@ export function getSnapshotXML(name, description) { snapElem.appendChild(descriptionElem); } + if (isExternal) { + if (memoryPath) { + const memoryElem = doc.createElement('memory'); + memoryElem.setAttribute('snapshot', 'external'); + memoryElem.setAttribute('file', memoryPath); + snapElem.appendChild(memoryElem); + } + + 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 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'); + + if (disk.type === "volume") { + const poolPath = getStoragePoolPath(storagePools, disk.source.pool, connectionName); + if (poolPath) { + const sourceElem = doc.createElement('source'); + sourceElem.setAttribute('file', `${poolPath}/${disk.source.volume}.snap`); + diskElem.appendChild(sourceElem); + } + } + + disksElem.appendChild(diskElem); + } + }); + snapElem.appendChild(disksElem); + } + doc.appendChild(snapElem); return new XMLSerializer().serializeToString(doc.documentElement); diff --git a/src/libvirt-xml-parse.js b/src/libvirt-xml-parse.js index a98582038..c2eccada6 100644 --- a/src/libvirt-xml-parse.js +++ b/src/libvirt-xml-parse.js @@ -468,6 +468,7 @@ export function parseDumpxmlForDisks(devicesElem) { }, bootOrder: bootElem?.getAttribute('order'), type: diskElem.getAttribute('type'), // i.e.: file + snapshot: diskElem.getAttribute('snapshot'), // i.e.: internal, external device: diskElem.getAttribute('device'), // i.e. cdrom, disk source: { file: sourceElem?.getAttribute('file'), // optional file name of the disk diff --git a/src/libvirtApi/helpers.js b/src/libvirtApi/helpers.js index 426e695c7..c8ec6e3d7 100644 --- a/src/libvirtApi/helpers.js +++ b/src/libvirtApi/helpers.js @@ -41,11 +41,12 @@ export const Enum = { VIR_DOMAIN_UNDEFINE_MANAGED_SAVE: 1, VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA: 2, VIR_DOMAIN_UNDEFINE_NVRAM: 4, - VIR_DOMAIN_SNAPSHOT_LIST_INTERNAL: 256, + // https://libvirt.org/html/libvirt-libvirt-domain-snapshot.html VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING: 1, VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED: 2, VIR_DOMAIN_SNAPSHOT_REVERT_FORCE: 4, VIR_DOMAIN_SNAPSHOT_REVERT_RESET_NVRAM: 8, + VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY: 16, VIR_DOMAIN_STATS_BALLOON: 4, VIR_DOMAIN_SHUTOFF: 5, VIR_DOMAIN_STATS_VCPU: 8, diff --git a/src/libvirtApi/snapshot.js b/src/libvirtApi/snapshot.js index 38970581e..671ec607d 100644 --- a/src/libvirtApi/snapshot.js +++ b/src/libvirtApi/snapshot.js @@ -29,10 +29,11 @@ import { parseDomainSnapshotDumpxml } from '../libvirt-xml-parse.js'; import { call, Enum, timeout } from './helpers.js'; import { logDebug } from '../helpers.js'; -export function snapshotCreate({ connectionName, vmId, name, description }) { - const xmlDesc = getSnapshotXML(name, description); - - return call(connectionName, vmId, 'org.libvirt.Domain', 'SnapshotCreateXML', [xmlDesc, 0], { timeout, type: 'su' }); +export function snapshotCreate({ connectionName, vmId, name, description, memoryPath, disks, isExternal, storagePools }) { + // that flag ought to be implicit for non-running VMs, see https://issues.redhat.com/browse/RHEL-22797 + const flags = (!isExternal || memoryPath) ? 0 : Enum.VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY; + const xmlDesc = getSnapshotXML(name, description, disks, memoryPath, isExternal, storagePools, connectionName); + return call(connectionName, vmId, 'org.libvirt.Domain', 'SnapshotCreateXML', [xmlDesc, flags], { timeout, type: 'su' }); } export function snapshotCurrent({ connectionName, objPath }) { diff --git a/test/check-machines-snapshots b/test/check-machines-snapshots index cd8297816..d3efbd498 100755 --- a/test/check-machines-snapshots +++ b/test/check-machines-snapshots @@ -124,22 +124,25 @@ class TestMachinesSnapshots(machineslib.VirtualMachinesCase): class SnapshotCreateDialog(object): def __init__( - self, test_obj, name=None, description=None, state="shutoff", snap_num=0, vm_name="subVmTest1", xfail=False, remove=True + self, test_obj, name=None, description=None, memory_path=None, state="shutoff", snap_num=0, + vm_name="subVmTest1", expect_external=False, xfail=None, remove=True ): self.test_obj = test_obj self.name = name self.description = description + self.memory_path = memory_path self.state = state self.snap_num = snap_num self.vm_name = vm_name self.remove = remove self.xfail = xfail + self.expect_external = expect_external def execute(self): self.open() self.fill() self.create() - if not self.xfail: + if self.xfail is None: self.verify_frontend() self.verify_backend() if self.remove: @@ -154,6 +157,8 @@ class TestMachinesSnapshots(machineslib.VirtualMachinesCase): b.set_input_text("#snapshot-create-dialog-name", self.name) if self.description: b.set_input_text("#snapshot-create-dialog-description", self.description) + if self.memory_path is not None: + b.set_input_text("#snapshot-create-dialog-memory-path input", self.memory_path) def assert_pixels(self): if self.name == 'test_snap_1': @@ -167,14 +172,20 @@ class TestMachinesSnapshots(machineslib.VirtualMachinesCase): if not self.xfail: self.assert_pixels() - b.click(".pf-v5-c-modal-box__footer button:contains(Create)") - - if not self.xfail: + if self.xfail is None: + b.click(".pf-v5-c-modal-box__footer button:contains(Create)") b.wait_not_present("#vm-subVmTest1-create-snapshot-modal") - else: + return + + if self.xfail == 'name': self.assert_pixels() b.wait_visible("#snapshot-create-dialog-name[aria-invalid=true]") - b.click(".pf-v5-c-modal-box__footer button:contains(Cancel)") + elif self.xfail == 'memory-path': + b.wait_visible("#snapshot-create-dialog-memory-path .pf-v5-c-helper-text__item-text") + else: + raise ValueError(f"Unknown xfail: {self.xfail}") + + self.cancel() def verify_frontend(self): if self.name: @@ -204,6 +215,26 @@ class TestMachinesSnapshots(machineslib.VirtualMachinesCase): if (self.state): self.test_obj.assertEqual(self.state, m.execute(xmllint_element.format(prop='state')).strip()) + try: + memory_snapshot = m.execute(xmllint_element.format(prop='memory/@snapshot')).strip() + ext_int = "external" if self.expect_external else "internal" + + if self.state == "running": + self.test_obj.assertEqual(memory_snapshot, ext_int) + else: + self.test_obj.assertEqual(memory_snapshot, 'no') + + disk_snapshot = m.execute(xmllint_element.format(prop='disks/disk/@snapshot')).strip() + self.test_obj.assertEqual(disk_snapshot, ext_int) + if self.name and self.expect_external: + disk_source = m.execute(xmllint_element.format(prop='disks/disk/source/@file')).strip() + self.test_obj.assertIn(self.name, disk_source) + except AssertionError: + print("------ snapshot XML -------") + print(m.execute(snap_xml)) + print("------ end snapshot XML -------") + raise + def cleanup(self): b.click(f"#delete-vm-subVmTest1-snapshot-{self.snap_num}") b.wait_in_text(".pf-v5-c-modal-box .pf-v5-c-modal-box__header .pf-v5-c-modal-box__title", "Delete snapshot?") @@ -214,9 +245,25 @@ class TestMachinesSnapshots(machineslib.VirtualMachinesCase): # No Snapshots present b.wait_visible("#vm-subVmTest1-add-snapshot-button") + # external memory snapshots introduced in libvirt 9.9.0 + supports_external = not ( + m.image.startswith("rhel-8") or + m.image.startswith("centos-8") or + m.image in ["debian-stable", "ubuntu-2204", "ubuntu-stable", "fedora-38", "fedora-39"]) + + # HACK: deleting external snapshots for non-running VMs is broken https://bugs.debian.org/bug=1061725 + # Work around that by temporarily disabling libvirtd's AppArmor profile. AppArmor isn't installed by + # default in Debian, and we want the rest of the test to run, thus no naughty. + apparmor_hack = m.image in ["debian-testing"] + if apparmor_hack: + # we don't install apparmor-utils, so need to emulate aa-disable + m.execute("ln -s /etc/apparmor.d/usr.sbin.libvirtd /etc/apparmor.d/disable/usr.sbin.libvirtd") + m.execute("aa-teardown; systemctl restart apparmor libvirtd") + # Test snapshot creation with pre-generated values SnapshotCreateDialog( self, + expect_external=supports_external, ).execute() # Test snapshot creation with predefined values @@ -225,6 +272,7 @@ class TestMachinesSnapshots(machineslib.VirtualMachinesCase): name="test_snap_1", description="Description of test_snap_1", state="shutoff", + expect_external=supports_external, remove=False, ).execute() @@ -233,21 +281,38 @@ class TestMachinesSnapshots(machineslib.VirtualMachinesCase): self, name="test_snap_1", state="shutoff", - xfail=True, + expect_external=supports_external, + xfail="name", ).execute() m.execute("virsh snapshot-delete subVmTest1 test_snap_1") b.click("#vm-subVmTest1-system-run") b.wait_in_text("#vm-subVmTest1-system-state", "Running") + if apparmor_hack: + # should work fine for running VMs, so re-enable AppArmor + m.execute("rm /etc/apparmor.d/disable/usr.sbin.libvirtd") + m.execute("aa-teardown; systemctl restart apparmor libvirtd") + # Test snapshot creation on running VM SnapshotCreateDialog( self, name="test_snap_2", description="Description of test_snap_2", state="running", + expect_external=supports_external, ).execute() + if supports_external: + SnapshotCreateDialog( + self, + name="test_snap_3", + state="running", + expect_external=True, + memory_path="", + xfail="memory-path", + ).execute() + def testSnapshotRevert(self): b = self.browser m = self.machine