diff --git a/README.md b/README.md index f365c109..b566e685 100644 --- a/README.md +++ b/README.md @@ -557,6 +557,7 @@ A rollback leaves the clone on a detached HEAD at the restored commit; the updat - 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. 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). ## Usage diff --git a/instances/tests/test_destroy.py b/instances/tests/test_destroy.py index f42dd89e..1afd3505 100644 --- a/instances/tests/test_destroy.py +++ b/instances/tests/test_destroy.py @@ -3,10 +3,12 @@ from unittest.mock import patch from computes.models import Compute from django.contrib.auth import get_user_model +from django.contrib.messages import get_messages from django.test import TestCase from django.urls import reverse from instances.models import Instance from libvirt import VIR_DOMAIN_UNDEFINE_KEEP_NVRAM, VIR_DOMAIN_UNDEFINE_NVRAM, libvirtError +from vrtManager.util import OperationError class DestroyOrderTestCase(TestCase): @@ -66,6 +68,20 @@ class DestroyOrderTestCase(TestCase): self.destroy() self.assertEqual(calls, [False, False]) + def test_a_volume_the_vms_snapshots_need_is_kept_after_the_detach(self): + disk = {"dev": "vdb", "path": "/pool/data.qcow2", "storage": "default", "image": "data.qcow2"} + with patch("instances.models.wvmInstance") as wvm, patch("instances.views.wvmStorage") as storage: + proxy = wvm.return_value + proxy.get_disk_devices.return_value = [disk] + proxy.paths_used_by_other_domains.return_value = set() + proxy.wait_disk_detached.return_value = True + storage.return_value.del_volume.side_effect = OperationError("Volume data.qcow2 is in use") + response = self.client.post(reverse("instances:delete_vol", args=[self.instance.id]), {"dev": "vdb"}) + self.assertEqual(response.status_code, 302) + proxy.detach_disk.assert_called_once_with("vdb") + messages = [str(m) for m in get_messages(response.wsgi_request)] + self.assertTrue(any("data.qcow2 is in use" in m and "detached" in m for m in messages), messages) + def test_confirmation_page_takes_no_lock(self): calls = [] diff --git a/instances/tests/test_live_dataloss.py b/instances/tests/test_live_dataloss.py index 0f3485c6..33963b40 100644 --- a/instances/tests/test_live_dataloss.py +++ b/instances/tests/test_live_dataloss.py @@ -321,6 +321,33 @@ class LiveDataLossTestCase(TestCase): pool.refresh(0) self.assertEqual({name for name in pool.listVolumes() if name.startswith(P + "r14-rev")}, set()) + # Delete a volume from the storage page + + def test_the_storage_page_keeps_volumes_a_vm_needs(self): + disk = livetest.create_volume(self.conn, P + "vol-disk") + dom, inst = self.vm("vol-disk", [disk]) + inst.proxy.create_external_snapshot("a") # disk is now the file snapshot a was taken on + template = livetest.create_volume(self.conn, P + "vol-template") + pool = self.conn.storagePoolLookupByName(livetest.POOL) + pool.createXML( + f"{P}vol-clone.qcow264" + f"{template}" + "", + 0, + ) + unused = livetest.create_volume(self.conn, P + "vol-unused") + top = self.disk_sources(dom)["vda"] + url = reverse("storage", args=[self.compute.id, livetest.POOL]) + + for path in (disk, template, top): + with self.subTest(path): + response = self.client.post(url, {"del_volume": "1", "volname": os.path.basename(path)}) + self.assertEqual(response.status_code, 302) + self.assertTrue(livetest.volume_exists(self.conn, path), "a volume a VM needs was deleted") + self.assertTrue(any("is in use" in str(m) for m in get_messages(response.wsgi_request))) + self.client.post(url, {"del_volume": "1", "volname": os.path.basename(unused)}) + self.assertFalse(livetest.volume_exists(self.conn, unused), "an unused volume was kept") + # Delete a volume only after it is detached def _delete_attached_volume(self, pause): diff --git a/instances/views.py b/instances/views.py index e9564dff..2bbd84b9 100755 --- a/instances/views.py +++ b/instances/views.py @@ -1223,7 +1223,13 @@ def delete_vol(request, pk): % {"dev": dev, "vol": disk["image"]}, ) return _back(request, pk, "disks") - conn_delete.del_volume(disk["image"]) + try: + conn_delete.del_volume(disk["image"]) + except util.OperationError as err: # a file of the VM's own snapshots + messages.warning( + request, + _("Disk %(dev)s was detached, but its volume was kept: %(err)s") % {"dev": dev, "err": err}, + ) addlogmsg(request.user.username, instance.compute.name, instance.name, msg) return _back(request, pk, "disks") diff --git a/storages/api/viewsets.py b/storages/api/viewsets.py index 48c407ea..714f0b28 100644 --- a/storages/api/viewsets.py +++ b/storages/api/viewsets.py @@ -1,6 +1,7 @@ from admin.permissions import IsSuperUser from appsettings.settings import app_settings from computes.models import Compute +from computes.utils import libvirt_compute_lock from django.shortcuts import get_object_or_404 from drf_spectacular.utils import extend_schema, inline_serializer from rest_framework import serializers, status, viewsets @@ -196,7 +197,9 @@ class VolumeViewSet(viewsets.ViewSet): ) if conn.is_active(): - conn.del_volume(pk) + # no VM operation attaches it between the check and the delete + with libvirt_compute_lock(compute): + conn.del_volume(pk) conn.close() return Response({"status": "Volume: " + pk + " is deleted"}) else: diff --git a/storages/tests/test_views.py b/storages/tests/test_views.py index b4981023..e21226d6 100644 --- a/storages/tests/test_views.py +++ b/storages/tests/test_views.py @@ -6,6 +6,7 @@ from unittest.mock import MagicMock, patch from django.core.files.uploadedfile import SimpleUploadedFile from django.test import RequestFactory, TestCase +from django.contrib.messages import get_messages from django.urls import reverse from django.contrib.auth import get_user_model @@ -894,3 +895,80 @@ class GetVolumesViewTests(TestCase): response = self.client.get(self.url) self.assertEqual(response.status_code, 200) self.assertEqual(response.json()["vols"], ["a.qcow2", "b.qcow2"]) + + +class VolumeDeleteTests(TestCase): + """A volume a VM still needs is refused with its reason, on the page and in the API.""" + + def setUp(self): + User.objects.create_superuser(username="vol_admin", password="password", email="vol_admin@example.com") + self.client.login(request=RequestFactory().get("/"), username="vol_admin", password="password") + self.compute = Compute.objects.create(name="VolCompute", hostname="localhost", type=1, login="u", password="p") + + def refused(self, mock_wvm): + from vrtManager import util + + conn = mock_wvm.return_value + conn.is_active.return_value = False + conn.get_size.return_value = (1, 1) + conn.del_volume.side_effect = util.OperationError("Volume disk.qcow2 is in use") + return conn + + @patch("storages.views.wvmStorage") + def test_the_page_shows_the_refusal(self, mock_wvm): + conn = self.refused(mock_wvm) + url = reverse("storage", args=[self.compute.id, "default"]) + response = self.client.post(url, {"del_volume": "1", "volname": "disk.qcow2"}) + self.assertRedirects(response, url, fetch_redirect_response=False) + conn.del_volume.assert_called_once_with("disk.qcow2") + messages = [str(m) for m in get_messages(response.wsgi_request)] + self.assertTrue(any("disk.qcow2 is in use" in m for m in messages), messages) + self.assertFalse(any("is deleted" in m for m in messages), messages) + + def test_the_check_and_the_delete_hold_the_compute_lock(self): + """No VM operation (they share the compute lock) attaches the volume + between the check and the delete.""" + from contextlib import contextmanager + + held = [] + + @contextmanager + def lock(compute, *args, **kwargs): + held.append((compute.pk, kwargs.get("shared", False))) + yield + held.pop() + + for module, request in ( + ("storages.views", lambda: self.client.post(reverse("storage", args=[self.compute.id, "default"]), + {"del_volume": "1", "volname": "disk.qcow2"})), + ("storages.api.viewsets", lambda: self.client.delete( + f"/api/v1/computes/{self.compute.id}/storages/default/volumes/disk.qcow2/")), + ): + with self.subTest(module), patch(f"{module}.wvmStorage") as mock_wvm, \ + patch(f"{module}.libvirt_compute_lock", lock): + conn = mock_wvm.return_value + conn.is_active.return_value = True + conn.get_size.return_value = (1, 1) + inside = [] + conn.del_volume.side_effect = lambda name: inside.append(list(held)) + request() + self.assertEqual(inside, [[(self.compute.pk, False)]]) + + @patch("storages.views.wvmStorage") + def test_a_busy_compute_is_a_message(self, mock_wvm): + conn = self.refused(mock_wvm) + url = reverse("storage", args=[self.compute.id, "default"]) + with patch("storages.views.libvirt_compute_lock", side_effect=TimeoutError): + response = self.client.post(url, {"del_volume": "1", "volname": "disk.qcow2"}) + self.assertRedirects(response, url, fetch_redirect_response=False) + conn.del_volume.assert_not_called() + messages = [str(m) for m in get_messages(response.wsgi_request)] + self.assertTrue(any("Please retry" in m for m in messages), messages) + + @patch("storages.api.viewsets.wvmStorage") + def test_the_api_refuses_with_the_reason(self, mock_wvm): + conn = self.refused(mock_wvm) + conn.is_active.return_value = True + response = self.client.delete(f"/api/v1/computes/{self.compute.id}/storages/default/volumes/disk.qcow2/") + self.assertEqual(response.status_code, 400, response.content) + self.assertIn("disk.qcow2 is in use", response.json()["detail"]) diff --git a/storages/views.py b/storages/views.py index 8f8dcd73..e7085c16 100644 --- a/storages/views.py +++ b/storages/views.py @@ -4,6 +4,7 @@ import re from admin.decorators import superuser_only from appsettings.settings import app_settings from computes.models import Compute +from computes.utils import libvirt_compute_lock from django.contrib import messages from django.http import HttpResponseRedirect, JsonResponse from django.shortcuts import get_object_or_404, redirect, render @@ -12,6 +13,7 @@ from django.utils.translation import gettext_lazy as _ from libvirt import libvirtError from vrtManager.connection import CONN_TCP, CONN_TLS +from vrtManager.util import OperationError from vrtManager.storage import wvmStorage, wvmStorages from storages.forms import AddStgPool, CloneImage, CreateVolumeForm @@ -232,11 +234,18 @@ def storage(request, compute_id, pool): return HttpResponseRedirect(request.get_full_path()) if "del_volume" in request.POST: volname = request.POST.get("volname", "") - vol = conn.get_volume(volname) - vol.delete(0) - messages.success( - request, _("Volume: %(vol)s is deleted.") % {"vol": volname} - ) + try: + # no VM operation attaches it between the check and the delete + with libvirt_compute_lock(compute): + conn.del_volume(volname) + except OperationError as err: + messages.error(request, str(err)) + except TimeoutError: + messages.error(request, _("A VM operation on this host is running. Please retry.")) + else: + messages.success( + request, _("Volume: %(vol)s is deleted.") % {"vol": volname} + ) return redirect(reverse("storage", args=[compute.id, pool])) # return HttpResponseRedirect(request.get_full_path()) if "cln_volume" in request.POST: diff --git a/vrtManager/connection.py b/vrtManager/connection.py index e4b9c35b..7fec5de0 100644 --- a/vrtManager/connection.py +++ b/vrtManager/connection.py @@ -11,6 +11,7 @@ import libvirt from django.conf import settings from django.utils.functional import cached_property from libvirt import libvirtError +from lxml import etree from vrtManager import util from vrtManager.rwlock import ReadWriteLock @@ -946,6 +947,59 @@ class wvmConnect(object): instances.append(name) return instances + def paths_in_use(self, skip_uuid=None, own=()): + """Paths (names for network disks) a domain uses as a disk or a + backing file of one, or as a file of its snapshots (overlays, memory, + the disks they were taken on); and the backing files of volumes. + skip_uuid leaves out one domain, own the volumes whose backing files + do not count.""" + used = set() + for dom in self.wvm.listAllDomains(): + if dom.UUIDString() == skip_uuid: + continue + for flags in (0, libvirt.VIR_DOMAIN_XML_INACTIVE): + used.update(self._source_paths(etree.fromstring(dom.XMLDesc(flags)))) + for snap in dom.listAllSnapshots(0): + tree = etree.fromstring(snap.getXMLDesc(0)) + used.update(tree.xpath("disks/disk/source/@file|memory/@file")) + for saved in tree.xpath("domain|inactiveDomain"): + used.update(self._source_paths(saved)) + # A stopped domain's XML has no backing chain; the volumes have it. + # Any volume backed by a path keeps that path, even an unattached one. + for pool in self.wvm.listAllStoragePools(): + if not pool.isActive(): + continue + with contextlib.suppress(libvirtError): + pool.refresh(0) + for vol in pool.listAllVolumes(): + # only a volume removed meanwhile is skipped: one that cannot + # be read may still keep a backing file + try: + if vol.path() not in own: + used.update(etree.fromstring(vol.XMLDesc(0)).xpath("./backingStore/path/text()")) + except libvirtError as err: + if err.get_error_code() != libvirt.VIR_ERR_NO_STORAGE_VOL: + raise + return used + + def _source_paths(self, domain): + """The files, devices or network names of a domain definition's + disks and of their backing chains ("//" reaches a running domain's + backingStore); a pool volume source as its volume's path.""" + paths = [] + for source in domain.xpath("devices/disk//source"): + if source.get("pool") and source.get("volume"): + try: + pool = self.wvm.storagePoolLookupByName(source.get("pool")) + paths.append(pool.storageVolLookupByName(source.get("volume")).path()) + except libvirtError as err: + # a volume that is not there cannot be the one deleted + if err.get_error_code() not in (libvirt.VIR_ERR_NO_STORAGE_VOL, libvirt.VIR_ERR_NO_STORAGE_POOL): + raise + else: + paths += [v for v in (source.get("file"), source.get("dev"), source.get("name")) if v] + return paths + def get_snapshots(self): instance = [] for snap_id in self.wvm.listDomainsID(): diff --git a/vrtManager/instance.py b/vrtManager/instance.py index 5a4cc822..c5ce2614 100644 --- a/vrtManager/instance.py +++ b/vrtManager/instance.py @@ -1480,29 +1480,9 @@ class wvmInstance(wvmConnect): return iso 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(): - if dom.UUIDString() == uuid: - continue - for flags in (0, VIR_DOMAIN_XML_INACTIVE): - tree = etree.fromstring(dom.XMLDesc(flags)) - # "//" also reaches backingStore chains in a running domain's XML - used.update(tree.xpath("./devices/disk//source/@file|./devices/disk//source/@dev")) - # A stopped domain's XML has no backing chain; the volumes have it. - # Any volume backed by a path keeps that path, even an unattached one. - for pool in self.wvm.listAllStoragePools(): - if not pool.isActive(): - continue - with contextlib.suppress(libvirtError): - pool.refresh(0) - for vol in pool.listAllVolumes(): - with contextlib.suppress(libvirtError): - if vol.path() not in own: - used.update(etree.fromstring(vol.XMLDesc(0)).xpath("./backingStore/path/text()")) - return used + """Paths another domain uses (see paths_in_use). The volumes in own + are this VM's: their backing files do not count.""" + return self.paths_in_use(self.instance.UUIDString(), own) def split_disk_paths_by_use(self): """This VM's disk paths, split into (only this VM, also used by another diff --git a/vrtManager/storage.py b/vrtManager/storage.py index 0037de22..60a8bf8d 100644 --- a/vrtManager/storage.py +++ b/vrtManager/storage.py @@ -238,7 +238,10 @@ class wvmStorage(wvmConnect): return vol.XMLDesc(0) def del_volume(self, name): + """libvirt deletes any volume; one a VM still needs is refused.""" vol = self.pool.storageVolLookupByName(name) + if vol.path() in self.paths_in_use(): + raise util.OperationError(f"Volume {name} is in use: a VM's disk, a backing file or a snapshot's file") vol.delete(0) def get_volume_format_type(self, name): diff --git a/vrtManager/tests/test_volume_in_use.py b/vrtManager/tests/test_volume_in_use.py new file mode 100644 index 00000000..5c2fb738 --- /dev/null +++ b/vrtManager/tests/test_volume_in_use.py @@ -0,0 +1,138 @@ +"""A volume a VM still needs is never deleted from a storage pool: not a +disk, not a backing file, not a file of a snapshot. libvirt deletes any +volume it is asked to; the live cases are in instances/tests/test_live_dataloss.py.""" + +import unittest +from unittest.mock import MagicMock + +import libvirt +from django.conf import settings +from libvirt import libvirtError + +if not settings.configured: + settings.configure(MAC_OUI="52:54:10") + +from vrtManager import util +from vrtManager.storage import wvmStorage + + +def domain(uuid, disks=(), snapshots=()): + dom = MagicMock() + dom.UUIDString.return_value = uuid + devices = "".join(f"" for d in disks) + dom.XMLDesc.return_value = f"{devices}" + snaps = [] + for xml in snapshots: + snap = MagicMock() + snap.getXMLDesc.return_value = xml + snaps.append(snap) + dom.listAllSnapshots.return_value = snaps + return dom + + +def volume(path, backing=None): + vol = MagicMock() + vol.path.return_value = path + parent = f"{backing}" if backing else "" + vol.XMLDesc.return_value = f"{parent}" + return vol + + +def storage(domains=(), volumes=()): + conn = wvmStorage.__new__(wvmStorage) + conn.wvm = MagicMock() + conn.wvm.listAllDomains.return_value = list(domains) + pool = MagicMock(**{"isActive.return_value": True}) + pool.listAllVolumes.return_value = list(volumes) + by_name = {v.path().rsplit("/", 1)[1]: v for v in volumes} + pool.storageVolLookupByName.side_effect = lambda name: by_name[name] + conn.wvm.listAllStoragePools.return_value = [pool] + conn.pool = pool + return conn, by_name + + +SNAPSHOT = ( + "s" + "" + "" + "" +) + + +class PathsInUseTestCase(unittest.TestCase): + def test_disks_backing_files_and_snapshot_files_are_in_use(self): + conn, _ = storage( + domains=[domain("u1", ["/pool/vm.now"], [SNAPSHOT])], + volumes=[volume("/pool/clone.qcow2", backing="/pool/template.qcow2")], + ) + self.assertEqual( + conn.paths_in_use(), + {"/pool/vm.now", "/pool/vm.s", "/pool/vm.s.mem", "/pool/vm.qcow2", "/pool/template.qcow2"}, + ) + + def test_one_domain_and_the_backing_files_of_own_volumes_can_be_left_out(self): + conn, _ = storage( + domains=[domain("u1", ["/pool/a.qcow2"]), domain("u2", ["/pool/b.qcow2"])], + volumes=[volume("/pool/a.qcow2", backing="/pool/base.qcow2")], + ) + self.assertEqual(conn.paths_in_use(skip_uuid="u1", own=["/pool/a.qcow2"]), {"/pool/b.qcow2"}) + + + def test_a_network_disk_is_in_use_by_its_name(self): + dom = domain("u1") + dom.XMLDesc.return_value = ( + "" + "" + ) + conn, _ = storage(domains=[dom]) + self.assertIn("rbdpool/disk", conn.paths_in_use()) + + def test_a_pool_volume_disk_is_in_use_by_its_path(self): + for xml in ( + "{}", + "", # in a snapshot's saved definition + ): + with self.subTest(xml): + disk = "" + dom = domain("u1") + if "{}" in xml: + dom.XMLDesc.return_value = xml.format(disk) + else: + snap = MagicMock() + snap.getXMLDesc.return_value = f"{disk}" + dom.listAllSnapshots.return_value = [snap] + conn, _ = storage(domains=[dom], volumes=[volume("/pool/disk.qcow2")]) + conn.wvm.storagePoolLookupByName.return_value = conn.pool + self.assertIn("/pool/disk.qcow2", conn.paths_in_use()) + + def test_a_volume_that_cannot_be_read_is_not_taken_as_unused(self): + removed, unreadable = volume("/pool/gone.qcow2"), volume("/pool/top.qcow2") + error = libvirtError("removed") + error.err = (libvirt.VIR_ERR_NO_STORAGE_VOL,) + removed.XMLDesc.side_effect = error + conn, _ = storage(volumes=[removed]) + self.assertEqual(conn.paths_in_use(), set()) + unreadable.XMLDesc.side_effect = libvirtError("permission denied") + conn, _ = storage(volumes=[unreadable]) + with self.assertRaises(libvirtError): + conn.paths_in_use() + + +class DeleteVolumeTestCase(unittest.TestCase): + def test_a_volume_in_use_is_refused(self): + for case, domains, volumes in ( + ("disk", [domain("u1", ["/pool/disk.qcow2"])], [volume("/pool/disk.qcow2")]), + ("backing file", [], [volume("/pool/disk.qcow2"), volume("/pool/top.qcow2", backing="/pool/disk.qcow2")]), + ("snapshot point", [domain("u1", ["/pool/vm.s"], [SNAPSHOT.replace("vm.qcow2", "disk.qcow2")])], + [volume("/pool/disk.qcow2")]), + ): + with self.subTest(case): + conn, vols = storage(domains, volumes) + with self.assertRaisesRegex(util.OperationError, "disk.qcow2 is in use"): + conn.del_volume("disk.qcow2") + vols["disk.qcow2"].delete.assert_not_called() + + def test_an_unused_volume_is_deleted(self): + conn, vols = storage([domain("u1", ["/pool/other.qcow2"])], [volume("/pool/disk.qcow2")]) + conn.del_volume("disk.qcow2") + vols["disk.qcow2"].delete.assert_called_once_with(0)