diff --git a/README.md b/README.md
index 740b8f63..f365c109 100644
--- a/README.md
+++ b/README.md
@@ -556,7 +556,7 @@ A rollback leaves the clone on a detached HEAD at the restored commit; the updat
- gstfsd finds the guest's operating system the way libguestfs inspects it, so a root on LVM works, and it always answers (an error when it finds no or several operating systems). Update `/usr/local/bin/gstfsd` on every compute (see [Adding a Compute Node](#adding-a-compute-node), step 3).
- A theme picked on the Settings page rewrites two tracked files; the update and rollback steps put back the shipped theme. Pick yours again afterwards, also after Docker recreates the container.
- With the bundled nginx config, browsers revalidate static files on each load (304 when unchanged), so an update or a theme change shows without a hard reload. Files cached before this update can stay until a hard reload (Ctrl+F5) of each affected page, or until the site's browser cache is cleared once; an external caching proxy may need a purge.
-- External snapshots use libvirt's snapshot API and need libvirt 9.9 or newer on the compute hosts. They work on UEFI VMs and running, paused or shut-off VMs, can save memory too, and a revert keeps disks added later (it detaches them). A VM's snapshots form one chain: after a revert, delete the newer snapshots before taking another. Snapshot names no longer get an `s1.` prefix; existing ones keep theirs and can still be reverted and deleted, except that a snapshot an older version took of a UEFI VM can only be deleted. On NFS, libvirt removes merged snapshot files as the qemu user, who needs write access to the image directory; files it could not remove are named in a warning after the delete or revert.
+- External snapshots use libvirt's snapshot API and need libvirt 9.9 or newer on the compute hosts. They work on UEFI VMs and running, paused or shut-off VMs, can save memory too, and a revert keeps disks added later (it detaches them). A VM's snapshots form one chain: after a revert, delete the newer snapshots before taking another. Snapshot names no longer get an `s1.` prefix; existing ones keep theirs and can still be reverted and deleted, except that a snapshot an older version took of a UEFI VM can only be deleted. On NFS, libvirt removes merged snapshot files as the qemu user, who needs write access to the image directory; files it could not remove are named in a warning after the delete or revert. Deleting a VM with its data removes its snapshot files too, except a template it grew from and files another VM uses.
- Docker no longer publishes port 6080; the console goes through nginx on the panel's port (`WS_PUBLIC_PORT`, default 80).
## Usage
diff --git a/instances/tests/test_live_dataloss.py b/instances/tests/test_live_dataloss.py
index 7629c333..0f3485c6 100644
--- a/instances/tests/test_live_dataloss.py
+++ b/instances/tests/test_live_dataloss.py
@@ -275,6 +275,52 @@ class LiveDataLossTestCase(TestCase):
self.assertFalse(livetest.volume_exists(self.conn, overlay), "the VM's own image is still there")
self.assertTrue(livetest.volume_exists(self.conn, base), "the base image was deleted")
+ def test_destroy_deletes_the_external_snapshot_chain_but_not_its_template(self):
+ # The VM grew from a template; its snapshots added overlays above its
+ # disk and saved memory. Deleting the disks takes all of those, which
+ # nothing else uses once the VM and its snapshot metadata are gone.
+ template = livetest.create_volume(self.conn, P + "r14-ext-template")
+ pool = self.conn.storagePoolLookupByName(livetest.POOL)
+ disk = pool.createXML(
+ f"{P}r14-ext.qcow264"
+ f"{template}"
+ "",
+ 0,
+ ).path()
+ dom, inst = self.vm("r14-ext", [disk])
+ dom.create()
+ inst.proxy.create_external_snapshot("a", memory=True)
+ inst.proxy.create_external_snapshot("b")
+ memory = os.path.join(livetest.POOL_PATH, f"{dom.UUIDString()}.a.mem")
+ pool.refresh(0)
+ made = {name for name in pool.listVolumes() if name.startswith(P + "r14-ext.")} - {P + "r14-ext.qcow2"}
+ self.assertEqual(len(made), 2, made) # one overlay per snapshot
+
+ response = self.post("destroy", inst, {"delete_disk": "1"})
+
+ self.assertEqual(response.status_code, 302)
+ self.assertFalse(self.domain_exists(dom.UUIDString()), "VM is still defined")
+ pool.refresh(0)
+ left = {name for name in pool.listVolumes() if name.startswith(P + "r14-ext") and "template" not in name}
+ self.assertEqual(left, set(), "files of the VM or its snapshots stayed")
+ self.assertFalse(livetest.volume_exists(self.conn, memory), "the memory file stayed")
+ self.assertTrue(livetest.volume_exists(self.conn, template), "the template was deleted")
+ self.assertEqual([str(m) for m in get_messages(response.wsgi_request) if "kept" in str(m)], [])
+
+ def test_destroy_after_a_revert_deletes_the_newer_snapshots_files_too(self):
+ base = livetest.create_volume(self.conn, P + "r14-rev")
+ dom, inst = self.vm("r14-rev", [base])
+ inst.proxy.create_external_snapshot("a")
+ inst.proxy.create_external_snapshot("b")
+ inst.proxy.revert_external_snapshot("a") # b's overlay is off the disk's chain now
+
+ response = self.post("destroy", inst, {"delete_disk": "1"})
+
+ self.assertEqual(response.status_code, 302)
+ pool = self.conn.storagePoolLookupByName(livetest.POOL)
+ pool.refresh(0)
+ self.assertEqual({name for name in pool.listVolumes() if name.startswith(P + "r14-rev")}, set())
+
# Delete a volume only after it is detached
def _delete_attached_volume(self, pause):
diff --git a/vrtManager/instance.py b/vrtManager/instance.py
index 7cc4f44c..5a4cc822 100644
--- a/vrtManager/instance.py
+++ b/vrtManager/instance.py
@@ -1479,8 +1479,9 @@ class wvmInstance(wvmConnect):
iso.extend(name for name in names if name.lower().endswith(".iso"))
return iso
- def paths_used_by_other_domains(self):
- """Paths another domain uses as a disk or as a backing file of one."""
+ def paths_used_by_other_domains(self, own=()):
+ """Paths another domain uses as a disk or as a backing file of one.
+ The volumes in own are this VM's: their backing files do not count."""
uuid = self.instance.UUIDString()
used = set()
for dom in self.wvm.listAllDomains():
@@ -1499,19 +1500,73 @@ class wvmInstance(wvmConnect):
pool.refresh(0)
for vol in pool.listAllVolumes():
with contextlib.suppress(libvirtError):
- used.update(etree.fromstring(vol.XMLDesc(0)).xpath("./backingStore/path/text()"))
+ if vol.path() not in own:
+ used.update(etree.fromstring(vol.XMLDesc(0)).xpath("./backingStore/path/text()"))
return used
def split_disk_paths_by_use(self):
- """This VM's disk paths, split into (only this VM, also used by another domain)."""
+ """This VM's disk paths, split into (only this VM, also used by another
+ domain). With external snapshots, the files below a disk that the
+ snapshots were taken on, and their memory files, are the VM's too."""
self.refresh_instance_pools()
own = [disk["path"] for disk in self.get_disk_devices() if disk["path"]]
- used_elsewhere = self.paths_used_by_other_domains()
+ own += self._snapshot_chain_files(own)
+ used_elsewhere = self.paths_used_by_other_domains(own)
+ # a kept file keeps the files below it
+ for path in [path for path in own if path in used_elsewhere]:
+ while (path := self._backing_path(path)) and path not in used_elsewhere:
+ used_elsewhere.add(path)
return (
[path for path in own if path not in used_elsewhere],
[path for path in own if path in used_elsewhere],
)
+ def _snapshot_chain_files(self, tops):
+ """The files of this VM's external snapshots that these disk images
+ grow from or that grow from them: the backing files snapshots were
+ taken on, down the chain, the overlays of snapshots off the chain
+ (newer ones kept after a revert), and the snapshots' memory files. A
+ backing file the VM started from (a template) is no snapshot point
+ and ends the chain; an overlay that does not grow from the chain (a
+ disk detached since) stays."""
+ points, overlays, memory = set(), set(), []
+ for snap in self.instance.listAllSnapshots(VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL):
+ tree = etree.fromstring(snap.getXMLDesc(0))
+ memory += tree.xpath("memory/@file")
+ overlays.update(tree.xpath("disks/disk/source/@file"))
+ saved = self._saved_definition(tree)
+ if saved is not None:
+ points.update(self._disk_sources(saved).values())
+ files = list(tops)
+ for path in tops:
+ while (path := self._backing_path(path)) in points and path not in files:
+ files.append(path)
+ grew = True
+ while grew:
+ grew = False
+ for top in overlays.difference(files):
+ if top in files: # reached by a branch in this pass
+ continue
+ branch, path = [top], top
+ while (path := self._backing_path(path)) in points | overlays and path not in files:
+ branch.append(path)
+ if path in files:
+ files += branch
+ grew = True
+ return files[len(tops):] + memory
+
+ def _backing_path(self, path):
+ """The backing file of a volume; None if it has none or is in no pool.
+ Any other error is raised: a chain cut short would let a file a kept
+ one needs count as deletable."""
+ try:
+ vol = self.get_volume_by_path(path)
+ except libvirtError as err:
+ if err.get_error_code() != VIR_ERR_NO_STORAGE_VOL:
+ raise
+ return None
+ return util.get_xml_path(vol.XMLDesc(0), "/volume/backingStore/path")
+
def _snapshotCreateXML(self, xml, flag):
self.instance.snapshotCreateXML(xml, flag)
diff --git a/vrtManager/tests/test_external_snapshots.py b/vrtManager/tests/test_external_snapshots.py
index 38b58bfe..3dcc9ec3 100644
--- a/vrtManager/tests/test_external_snapshots.py
+++ b/vrtManager/tests/test_external_snapshots.py
@@ -551,3 +551,82 @@ class KindTestCase(unittest.TestCase):
if __name__ == "__main__":
unittest.main()
+
+
+class DestroyFilesTestCase(unittest.TestCase):
+ """Deleting a VM with its disks takes the files its external snapshots
+ made or were taken on, not a template below them or a file another VM
+ still grows from."""
+
+ TOP = ""
+
+ def split(self, other_overlay_on=None, top="/pool/a.two", extra=None):
+ one = chain_snap("one", "/pool/a.one", memory="/pool/vm.one.mem")
+ two = chain_snap("two", "/pool/a.two")
+ snaps = [(one, "/pool/a.qcow2"), (two, "/pool/a.one")] # each with the file it was taken on
+ if extra:
+ snaps.append((chain_snap("old", extra[0]), extra[1]))
+ for snap, point in snaps:
+ snap.getXMLDesc.return_value = snap.getXMLDesc.return_value.replace(
+ "", f""
+ )
+ p = proxy(external=[snap for snap, _ in snaps], xml=self.TOP)
+ p.get_disk_devices = lambda: [{"path": top}]
+ backing = {
+ "/pool/a.two": "/pool/a.one", "/pool/a.one": "/pool/a.qcow2", "/pool/a.qcow2": "/templates/base.qcow2",
+ }
+ if top == "/pool/a.reverted":
+ backing[top] = "/pool/a.one"
+ if extra:
+ backing[extra[0]] = extra[1]
+ if other_overlay_on:
+ backing["/pool/other.qcow2"] = other_overlay_on
+
+ def volume(path):
+ parent = f"{backing[path]}" if path in backing else ""
+ vol = MagicMock(**{"XMLDesc.return_value": f"{parent}"})
+ vol.path.return_value = path
+ return vol
+
+ p.get_volume_by_path = volume
+ pool = MagicMock(**{"isActive.return_value": True})
+ pool.listAllVolumes.return_value = [volume(path) for path in backing]
+ p.wvm.listAllStoragePools.return_value = [pool]
+ p.wvm.listAllDomains.return_value = []
+ return p.split_disk_paths_by_use()
+
+ def test_a_backing_file_that_cannot_be_read_is_an_error_not_the_chain_end(self):
+ p = proxy()
+ missing = libvirtError("no volume")
+ missing.err = (libvirt.VIR_ERR_NO_STORAGE_VOL,)
+ p.get_volume_by_path = MagicMock(side_effect=missing)
+ self.assertIsNone(p._backing_path("/elsewhere/base.qcow2")) # in no pool
+ p.get_volume_by_path = MagicMock(side_effect=libvirtError("connection lost"))
+ with self.assertRaises(libvirtError):
+ p._backing_path("/pool/a.one")
+
+ def test_the_snapshot_chain_and_memory_go_but_not_the_template(self):
+ delete, keep = self.split()
+ self.assertEqual(sorted(delete), ["/pool/a.one", "/pool/a.qcow2", "/pool/a.two", "/pool/vm.one.mem"])
+ self.assertEqual(keep, [])
+
+ def test_a_file_another_vm_grows_from_keeps_the_files_below_it(self):
+ delete, keep = self.split(other_overlay_on="/pool/a.one")
+ self.assertEqual(sorted(delete), ["/pool/a.two", "/pool/vm.one.mem"])
+ self.assertEqual(sorted(keep), ["/pool/a.one", "/pool/a.qcow2"])
+
+ def test_newer_snapshots_kept_after_a_revert_go_too(self):
+ # reverted to one: the disk is a new overlay on a.one, and a.two of
+ # snapshot two branches off it
+ delete, keep = self.split(top="/pool/a.reverted")
+ self.assertEqual(
+ sorted(delete), ["/pool/a.one", "/pool/a.qcow2", "/pool/a.reverted", "/pool/a.two", "/pool/vm.one.mem"]
+ )
+ self.assertEqual(keep, [])
+
+ def test_a_snapshot_file_off_the_disks_chain_stays(self):
+ # a disk detached since: its snapshot overlay grows from a file the
+ # VM's disks do not
+ delete, _ = self.split(extra=("/pool/gone.old", "/pool/gone.qcow2"))
+ self.assertNotIn("/pool/gone.old", delete)
+ self.assertNotIn("/pool/gone.qcow2", delete)