fix(instances): a snapshot needs free space in the VM's storage pools

A disk-only snapshot was taken without any space check, though its
overlays grow as the guest writes and a later delete merges them down;
a full pool pauses the guest and can leave the snapshot undeletable.
Only a memory snapshot was checked, for the memory alone.

Before a snapshot, each storage pool with a disk of the VM must have
free a share of those disks' virtual size (10%, at least 1 GiB), and
the pool of the memory file the VM's memory on top. Both values are on
the Settings page (Snapshot Free Percent, Snapshot Free Min GiB; 0
turns a rule off, an invalid value keeps the default). The pools are
refreshed first, and a failed refresh or an unreadable volume refuses
the snapshot instead of skipping the check; a disk in no pool is not
checked. Nothing is reserved: later writes can still fill a pool.

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 or, on a running VM with a connected QEMU guest agent, freeze the guest's file systems (the default there), 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.
+- 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 or, on a running VM with a connected QEMU guest agent, freeze the guest's file systems (the default there), and a revert keeps disks added later (it detaches them). A snapshot is refused when a storage pool with the VM's disks has less free space than a share of those disks (10%, at least 1 GiB; both on the Settings page) plus, for the pool of the memory file, the VM's memory; this is checked before the snapshot only, nothing is reserved. 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.
 - A storage volume a VM still needs (a disk, a backing file or a snapshot's file) is no longer deleted from the storage page or the API; the delete is refused with the reason. Only VMs on the same compute are checked.
 - Docker no longer publishes port 6080; the console goes through nginx on the panel's port (`WS_PUBLIC_PORT`, default 80).
 

appsettings/migrations/0017_snapshot_free_space_settings.py

@@ -0,0 +1,35 @@
+from django.core.management.color import no_style
+from django.db import migrations
+
+
+SETTINGS = [
+    ("Snapshot Free Percent", "SNAPSHOT_FREE_SPACE_PERCENT", "10",
+     "Free space a storage pool keeps for a snapshot, in percent of the VM's disks in it (0: none)"),
+    ("Snapshot Free Min GiB", "SNAPSHOT_FREE_SPACE_MIN_GIB", "1",
+     "Free space a storage pool keeps for a snapshot at least, in GiB (0: none)"),
+]
+
+
+def add_settings(apps, schema_editor):
+    setting = apps.get_model("appsettings", "AppSettings")
+    db_alias = schema_editor.connection.alias
+    # see 0014: move a PostgreSQL id sequence past the fixed ids first
+    with schema_editor.connection.cursor() as cursor:
+        for sql in schema_editor.connection.ops.sequence_reset_sql(no_style(), [setting]):
+            cursor.execute(sql)
+    for name, key, value, description in SETTINGS:
+        setting.objects.using(db_alias).get_or_create(
+            key=key, defaults={"name": name, "value": value, "choices": "", "description": description}
+        )
+
+
+def del_settings(apps, schema_editor):
+    setting = apps.get_model("appsettings", "AppSettings")
+    db_alias = schema_editor.connection.alias
+    setting.objects.using(db_alias).filter(key__in=[key for _, key, _, _ in SETTINGS]).delete()
+
+
+class Migration(migrations.Migration):
+    dependencies = [("appsettings", "0016_nic_type_rtl8139")]
+
+    operations = [migrations.RunPython(add_settings, del_settings)]

appsettings/tests/test_migrations.py

@@ -40,3 +40,14 @@ class NicTypeTestCase(TestCase):
         nic_type.fix_rtl8139(apps, SimpleNamespace(connection=connection))
         setting.refresh_from_db()
         self.assertEqual((setting.value, setting.choices), ("rtl8139", "default,e1000,e1000e,rtl8139,virtio"))
+
+
+class SnapshotFreeSpaceSettingsTestCase(TestCase):
+    def test_the_rows_fit_their_columns(self):
+        # PostgreSQL refuses a value longer than the column, sqlite does not
+        for key in ("SNAPSHOT_FREE_SPACE_PERCENT", "SNAPSHOT_FREE_SPACE_MIN_GIB"):
+            setting = AppSettings.objects.get(key=key)
+            for field in ("name", "key", "value", "description"):
+                limit = AppSettings._meta.get_field(field).max_length
+                if limit:
+                    self.assertLessEqual(len(getattr(setting, field)), limit, f"{key} {field}")

instances/tests/test_live_snapshots.py

@@ -176,3 +176,15 @@ class LiveExternalSnapshotTestCase(SimpleTestCase):
                 with self.assertRaisesRegex(util.OperationError, "guest agent is not connected"):
                     vm.create_external_snapshot("q", quiesce=True)
                 self.assertEqual(dom.snapshotNum(0), 0)
+
+    def test_a_pool_without_room_for_the_overlays_is_refused(self):
+        # a sparse disk ten times the pool: a tenth of it is more than the pool has free
+        pool = self.conn.storagePoolLookupByName(livetest.POOL)
+        big = livetest.create_volume(self.conn, P + "snap-big", size_mib=pool.info()[1] * 10 // 1024**2)
+        dom = livetest.define_vm(self.conn, P + "snap-big", [big])
+        vm = wvmInstance(*self.args, None, uuid=dom.UUIDString())
+        with self.assertRaisesRegex(util.OperationError, f"Storage pool {livetest.POOL} has .* GiB free; the snapshot needs"):
+            vm.create_external_snapshot("x")
+        self.assertEqual(dom.snapshotNum(0), 0)
+        vm.create_external_snapshot("x", free_percent=0, free_min_gib=0)
+        vm.delete_external_snapshot("x")

instances/tests/test_snapshot_views.py

@@ -12,6 +12,7 @@ from django.urls import reverse
 from lxml import html as lxml_html
 from vrtManager import util
 
+from appsettings.models import AppSettings
 from computes.models import Compute
 from instances.models import Instance
 
@@ -48,11 +49,22 @@ class SnapshotViewsTestCase(TestCase):
         self.assertEqual(response.status_code, 302)
         return proxy, method, [str(m) for m in get_messages(response.wsgi_request)]
 
-    def test_create_passes_name_description_memory_and_quiesce(self):
+    def test_create_passes_name_description_memory_quiesce_and_free_space(self):
+        free = {"free_percent": 10.0, "free_min_gib": 1.0}  # the Settings page defaults
         _, method, _ = self.post("create_external_snapshot", {"name": "a", "description": "d", "memory": "on"})
-        method.assert_called_once_with("a", "d", True, False)
+        method.assert_called_once_with("a", "d", True, False, **free)
         _, method, _ = self.post("create_external_snapshot", {"name": "a", "description": "", "quiesce": "on"})
-        method.assert_called_once_with("a", "", False, True)
+        method.assert_called_once_with("a", "", False, True, **free)
+
+    def test_the_free_space_comes_from_the_settings_page(self):
+        for percent, minimum, expected in (("20", "0.5", {"free_percent": 20.0, "free_min_gib": 0.5}),
+                                           ("0", "0", {"free_percent": 0.0, "free_min_gib": 0.0}),
+                                           ("-1", "x", {}), ("nan", "inf", {})):  # invalid: the backend default
+            with self.subTest(percent=percent, minimum=minimum):
+                AppSettings.objects.filter(key="SNAPSHOT_FREE_SPACE_PERCENT").update(value=percent)
+                AppSettings.objects.filter(key="SNAPSHOT_FREE_SPACE_MIN_GIB").update(value=minimum)
+                _, method, _ = self.post("create_external_snapshot", {"name": "a"})
+                method.assert_called_once_with("a", "", False, False, **expected)
 
     def test_revert_and_delete_call_the_backend_once_and_leave_power_alone(self):
         for view in ("revert_external_snapshot", "delete_external_snapshot"):

instances/views.py

@@ -4,6 +4,7 @@ except ImportError:
     import crypt
 import functools
 import json
+import math
 import os
 import re
 import socket
@@ -1391,6 +1392,20 @@ def _warn_left_behind(request, files):
         )
 
 
+def _snapshot_free_space():
+    """The free space each storage pool keeps for a snapshot, from the
+    Settings page; an invalid value leaves the backend's default."""
+    values = {}
+    for key, argument in (("SNAPSHOT_FREE_SPACE_PERCENT", "free_percent"), ("SNAPSHOT_FREE_SPACE_MIN_GIB", "free_min_gib")):
+        try:
+            value = float(getattr(app_settings, key))
+        except (AttributeError, TypeError, ValueError):
+            continue
+        if math.isfinite(value) and value >= 0:
+            values[argument] = value
+    return values
+
+
 @require_POST
 @serialize_instance_mutation
 def create_external_snapshot(request, pk):
@@ -1402,7 +1417,7 @@ def create_external_snapshot(request, pk):
     memory = request.POST.get("memory") == "on"
     quiesce = request.POST.get("quiesce") == "on"
     try:
-        instance.proxy.create_external_snapshot(name, desc, memory, quiesce)
+        instance.proxy.create_external_snapshot(name, desc, memory, quiesce, **_snapshot_free_space())
     except libvirtError as err:
         messages.error(request, _("Snapshot was not created: %(err)s") % {"err": error_text(request, err)})
     else:

vrtManager/instance.py

@@ -1576,12 +1576,14 @@ class wvmInstance(wvmConnect):
                   </domainsnapshot>"""
         self._snapshotCreateXML(xml, 0)
 
-    def create_external_snapshot(self, name, desc="", memory=False, quiesce=False):
+    def create_external_snapshot(self, name, desc="", memory=False, quiesce=False, free_percent=10, free_min_gib=1):
         """A disk-only snapshot, or with memory too (running VMs only). Every
         disk gets a libvirt-named overlay next to it; the memory file goes
         next to the first disk. quiesce: the guest agent freezes the guest's
         file systems for a disk-only snapshot of a running VM (libvirt
-        accepts it only without memory, which keeps the whole state anyway)."""
+        accepts it only without memory, which keeps the whole state anyway).
+        free_percent, free_min_gib: the free space each pool keeps (see
+        _check_snapshot_space)."""
         if not SNAPSHOT_NAME.fullmatch(name):
             raise util.OperationError(
                 "Snapshot names are 1-64 letters, digits, '.', '_' or '-' and do not start with '.'"
@@ -1612,7 +1614,17 @@ class wvmInstance(wvmConnect):
                     raise util.OperationError(f"Eject the media from {dev} first")
             elif disk.get("type") != "file":
                 raise util.OperationError(f"Disk {dev} is not a file; external snapshots need file disks")
-        memory_xml = self._snapshot_memory_xml(name, tree) if memory else ""
+        disks = list(self._disk_sources(tree).values())
+        # free space, and the top overlays, are known to the pools only after
+        # a refresh; nothing has changed yet when it fails
+        try:
+            self.refresh_instance_pools()
+        except libvirtError as err:
+            raise util.OperationError(
+                f"The storage pools could not be refreshed, so their free space is unknown: {err}"
+            ) from None
+        memory_xml, memory_pool = self._snapshot_memory(name, disks) if memory else ("", None)
+        self._check_snapshot_space(disks, memory_pool, free_percent, free_min_gib)
         xml = (
             f"<domainsnapshot><name>{name}</name>"
             f"<description>{util.xml_escape(desc)}</description>{memory_xml}</domainsnapshot>"
@@ -1625,24 +1637,50 @@ class wvmInstance(wvmConnect):
         self._snapshotCreateXML(xml, flags)
         self._refresh_pools_after_snapshot()
 
-    def _snapshot_memory_xml(self, name, tree):
-        """The <memory> element of a snapshot that saves memory: a file in the
-        storage pool of the first disk, which must have room for it."""
+    def _snapshot_memory(self, name, disks):
+        """The <memory> element of a snapshot that saves memory, and the
+        storage pool its file goes into: the pool of the first disk."""
         if self.get_status() != 1:
             raise util.OperationError("Memory is saved only from a running VM")
-        disks = list(self._disk_sources(tree).values())
         if not disks:
             raise util.OperationError("The VM has no disk to keep its memory next to")
-        # the top overlay is known to its pool only after a refresh
-        self.refresh_instance_pools()
         try:
             pool = self.get_volume_by_path(disks[0]).storagePoolLookupByVolume()
         except libvirtError:
             raise util.OperationError(f"{disks[0]} is not in a storage pool; memory is saved only into one") from None
-        if pool.info()[3] < self.get_cur_memory() * 1024 * 1024:
-            raise util.OperationError(f"Storage pool {pool.name()} has less free space than the VM's memory")
         path = os.path.join(os.path.dirname(disks[0]), f"{self.get_uuid()}.{name}.mem")
-        return f"<memory snapshot='external' file='{util.xml_escape(path)}'/>"
+        return f"<memory snapshot='external' file='{util.xml_escape(path)}'/>", pool.name()
+
+    def _check_snapshot_space(self, disks, memory_pool, free_percent, free_min_gib):
+        """Each storage pool with a disk of the VM keeps free_percent of the
+        virtual size of those disks free, at least free_min_gib GiB, and the
+        pool of the memory file the VM's memory on top. The
+        overlays grow as the guest writes, and a delete merges them down.
+        This is checked before the snapshot only; nothing is reserved. A
+        disk in no pool is not checked."""
+        needed, pools = {}, {}
+        for path in disks:
+            try:
+                vol = self.get_volume_by_path(path)
+            except libvirtError as err:
+                if err.get_error_code() != VIR_ERR_NO_STORAGE_VOL:
+                    raise
+                continue
+            pool = vol.storagePoolLookupByVolume()
+            pools[pool.name()] = pool
+            needed[pool.name()] = needed.get(pool.name(), 0) + vol.info()[1] * free_percent / 100
+        for name in needed:
+            needed[name] = max(needed[name], free_min_gib * 1024**3)
+        if memory_pool:
+            needed[memory_pool] += self.get_cur_memory() * 1024 * 1024
+        for name, size in needed.items():
+            free = pools[name].info()[3]
+            if free < size:
+                raise util.OperationError(
+                    f"Storage pool {name} has {free / 1024**3:.1f} GiB free; the snapshot needs {size / 1024**3:.1f} GiB "
+                    f"({free_percent:g}% of the VM's disks in it, at least {free_min_gib:g} GiB, "
+                    "and the memory if it is saved)"
+                )
 
     def revert_external_snapshot(self, name):
         """libvirt returns the disks and the definition to the snapshot's. A

vrtManager/tests/test_external_snapshots.py

@@ -49,6 +49,29 @@ def fake_snap(name, memory=False, disks=("vda",), current=False, children=0):
     return snap
 
 
+GiB = 1024**3
+
+
+def pools(p, disks, free):
+    """disks: path -> (pool name, virtual size); free: pool name -> free
+    bytes. A path not in disks is in no pool."""
+    by_name = {}
+    for name, available in free.items():
+        by_name[name] = MagicMock(**{"name.return_value": name, "info.return_value": [2, 0, 0, available]})
+
+    def lookup(path):
+        if path not in disks:
+            error = libvirtError("no volume")
+            error.err = (libvirt.VIR_ERR_NO_STORAGE_VOL,)
+            raise error
+        name, size = disks[path]
+        return MagicMock(**{"info.return_value": [0, size, 0],
+                            "storagePoolLookupByVolume.return_value": by_name[name]})
+
+    p.get_volume_by_path = MagicMock(side_effect=lookup)
+    return by_name
+
+
 def proxy(status=1, internal=(), external=(), xml=DOMAIN, jobs=None, managed_save=False):
     """A wvmInstance without a connection. internal/external are fake snapshots."""
     p = wvmInstance.__new__(wvmInstance)
@@ -80,6 +103,8 @@ def proxy(status=1, internal=(), external=(), xml=DOMAIN, jobs=None, managed_sav
     dom.blockJobInfo.side_effect = lambda dev, flags: (jobs or {}).get(dev, {})
     dom.XMLDesc.side_effect = lambda flags: xml
     p.instance = dom
+    pools(p, {"/pool/a.qcow2": ("default", 20 * GiB), "/pool/later.qcow2": ("default", 20 * GiB)},
+          {"default": 100 * GiB})
     return p
 
 
@@ -190,7 +215,8 @@ class CreateTestCase(unittest.TestCase):
         self.assertEqual(flags, DISK_ONLY_ATOMIC)
         self.assertIn("<name>snap-1</name>", xml)
         self.assertNotIn("<memory", xml)
-        p.refresh_instance_pools.assert_called_once()
+        # before, for the free space; after, for the new overlays
+        self.assertEqual(p.refresh_instance_pools.call_count, 2)
 
     def test_description_is_escaped(self):
         p = proxy()
@@ -201,12 +227,8 @@ class CreateTestCase(unittest.TestCase):
 
     def test_memory_snapshot_goes_into_the_first_disks_pool(self):
         p = proxy()
-        p.get_volume_by_path = MagicMock()
-        pool = p.get_volume_by_path.return_value.storagePoolLookupByVolume.return_value
-        pool.info.return_value = [2, 0, 0, 2 * 1024**3]
         p.create_external_snapshot("m", memory=True)
         xml, flags = created(p)
-        p.get_volume_by_path.assert_called_once_with("/pool/a.qcow2")
         self.assertEqual(flags, libvirt.VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC)
         self.assertIn(
             "<memory snapshot='external' file='/pool/11111111-2222-3333-4444-555555555555.m.mem'/>", xml
@@ -280,12 +302,55 @@ class CreateTestCase(unittest.TestCase):
         p.get_volume_by_path = MagicMock(side_effect=libvirtError("no volume"))
         self.refused(p, "not in a storage pool", memory=True)
 
-    def test_memory_needs_room_in_the_pool(self):
+    def space(self, disks, free, memory=False):
+        """Creates a snapshot of a VM with these disks (path -> (pool, size)) and free space per pool."""
+        xml = "<domain><devices>%s</devices></domain>" % "".join(
+            f"<disk type='file' device='disk'><source file='{path}'/><target dev='vd{chr(97 + i)}'/></disk>"
+            for i, path in enumerate(disks)
+        )
+        p = proxy(xml=xml)
+        pools(p, {path: disk for path, disk in disks.items() if disk}, free)
+        p.create_external_snapshot("s", memory=memory)
+        return p
+
+    def test_each_pool_keeps_a_tenth_of_its_disks_free(self):
+        # a disk-only overlay grows as the guest writes, and a delete merges it down
+        disks = {"/fast/a.qcow2": ("fast", 20 * GiB), "/slow/b.qcow2": ("slow", 50 * GiB)}
+        self.space(disks, {"fast": 2 * GiB, "slow": 5 * GiB})
+        with self.assertRaisesRegex(util.OperationError, r"Storage pool slow has 4\.9 GiB free; the snapshot needs 5\.0 GiB"):
+            self.space(disks, {"fast": 2 * GiB, "slow": int(4.9 * GiB)})
+
+    def test_at_least_one_gib_is_kept_free(self):
+        self.space({"/pool/small.qcow2": ("default", 2 * GiB)}, {"default": GiB})
+        with self.assertRaisesRegex(util.OperationError, "needs 1.0 GiB"):
+            self.space({"/pool/small.qcow2": ("default", 2 * GiB)}, {"default": GiB - 1})
+
+    def test_the_memory_pool_also_needs_room_for_the_memory(self):
+        # get_cur_memory: 1024 MiB
+        disks = {"/pool/a.qcow2": ("default", 20 * GiB)}
+        self.space(disks, {"default": 3 * GiB}, memory=True)
+        with self.assertRaisesRegex(util.OperationError, "needs 3.0 GiB"):
+            self.space(disks, {"default": 3 * GiB - 1}, memory=True)
+
+    def test_the_share_and_the_minimum_can_be_changed(self):
+        disks = {"/pool/a.qcow2": ("default", 20 * GiB)}
         p = proxy()
-        p.get_volume_by_path = MagicMock()
-        pool = p.get_volume_by_path.return_value.storagePoolLookupByVolume.return_value
-        pool.info.return_value = [2, 0, 0, 512 * 1024**2]
-        self.refused(p, "less free space", memory=True)
+        pools(p, disks, {"default": 4 * GiB - 1})
+        with self.assertRaisesRegex(util.OperationError, r"needs 4\.0 GiB \(20% of the VM's disks in it, at least 2 GiB"):
+            p.create_external_snapshot("s", free_percent=20, free_min_gib=2)
+        p.create_external_snapshot("s", free_percent=0, free_min_gib=0)  # no check
+        p._snapshotCreateXML.assert_called_once()
+
+    def test_a_volume_that_cannot_be_read_is_refused_not_skipped(self):
+        p = proxy()
+        p.get_volume_by_path = MagicMock(side_effect=libvirtError("permission denied"))
+        with self.assertRaises(libvirtError):
+            p.create_external_snapshot("s")
+        p._snapshotCreateXML.assert_not_called()
+
+    def test_a_disk_in_no_pool_is_not_checked(self):
+        p = self.space({"/pool/a.qcow2": ("default", 20 * GiB), "/elsewhere/b.qcow2": None}, {"default": 2 * GiB})
+        p._snapshotCreateXML.assert_called_once()
 
 
 REVERT_RUNNING = libvirt.VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING
@@ -542,14 +607,23 @@ class LeftBehindTestCase(unittest.TestCase):
 
 class PoolRefreshTestCase(unittest.TestCase):
     """The pools are refreshed after libvirt made, reverted or deleted the
-    snapshot; a refresh error does not turn that into a failure."""
+    snapshot; a refresh error does not turn that into a failure. A create
+    also refreshes them before, to read their free space."""
 
-    def test_create_succeeds_when_the_refresh_fails(self):
+    def test_create_succeeds_when_the_refresh_after_it_fails(self):
         p = proxy()
-        p.refresh_instance_pools.side_effect = libvirtError("pool busy")
+        p.refresh_instance_pools.side_effect = [None, libvirtError("pool busy")]
         p.create_external_snapshot("one")
         p._snapshotCreateXML.assert_called_once()
 
+    def test_create_is_refused_when_the_refresh_before_it_fails(self):
+        # the free space is not known then
+        p = proxy()
+        p.refresh_instance_pools.side_effect = libvirtError("pool busy")
+        with self.assertRaisesRegex(util.OperationError, "free space is unknown: pool busy"):
+            p.create_external_snapshot("one")
+        p._snapshotCreateXML.assert_not_called()
+
     def test_delete_and_revert_succeed_and_report_nothing_when_the_refresh_fails(self):
         for action in ("delete_external_snapshot", "revert_external_snapshot"):
             with self.subTest(action):