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..7ea32d075 100644 --- a/src/helpers.js +++ b/src/helpers.js @@ -873,3 +873,41 @@ 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) { + const disks = Object.values(vm.disks); + + // If at leat one disk has internal snapshot preference specified, use internal snapshot for all disk, + // as mixing internal and external is not allowed + if (disks.some(disk => disk.snapshot === "internal")) { + logDebug(`vmSupportsExternalSnapshots: vm ${vm.name} has internal snapshot preference specified, false`); + 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" + if (!disks.every(disk => { + const supportedPools = storagePools.filter(pool => pool.type === "dir" && pool.connectionName === vm.connectionName); + + return disk.type === "file" || + (disk.type === "volume" && supportedPools.some(pool => pool.name === disk.source.pool)); + })) { + logDebug(`vmSupportsExternalSnapshots: vm ${vm.name} has unsupported disk type, false`); + return false; + } + + // 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 + const support = config.capabilities?.guests.some(guest => guest.osType === vm.osType && + guest.arch === vm.arch && + guest.features.externalSnapshot); + logDebug(`vmSupportsExternalSnapshots: vm ${vm.name} has external snapshot support: ${support}`); + return support; +} 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 838921f3b..081b65bc1 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..76de91d68 100644 --- a/src/libvirtApi/helpers.js +++ b/src/libvirtApi/helpers.js @@ -41,11 +41,13 @@ 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_SNAPSHOT_LIST_INTERNAL: 256, 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..8221d9237 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,23 @@ 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 + 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 +270,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 +279,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 + 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