diff --git a/networks/tests/test_network_in_use.py b/networks/tests/test_network_in_use.py new file mode 100644 index 00000000..cb95fae3 --- /dev/null +++ b/networks/tests/test_network_in_use.py @@ -0,0 +1,58 @@ +"""A network a VM needs is not stopped or deleted: stopping one a running VM +is on would cut that VM off, deleting one a VM's definition names would keep +that VM from starting. Runs on a private libvirt test driver: "test:///default" +is shared by the whole process.""" + +import tempfile +from unittest.mock import patch + +import libvirt +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 + +NODE = """ + net-user1024hvm + + lab +""" + + +class NetworkInUseTests(TestCase): + def setUp(self): + node = tempfile.NamedTemporaryFile("w", suffix=".xml") + self.addCleanup(node.close) + node.write(NODE) + node.flush() + self.conn = libvirt.open(f"test://{node.name}") + self.addCleanup(self.conn.close) + patcher = patch("vrtManager.connection.connection_manager.get_connection", return_value=self.conn) + patcher.start() + self.addCleanup(patcher.stop) + compute = Compute.objects.create(name="net-use", hostname="localhost", login="", password="", type=4) + self.client.force_login(get_user_model().objects.create_superuser("net-use", "nu@example.com", "pw")) + self.url = reverse("network", args=[compute.id, "lab"]) + + def post(self, action): + response = self.client.post(self.url, {action: ""}, HTTP_REFERER=f"http://testserver{self.url}") + return [str(m) for m in get_messages(response.wsgi_request)] + + def test_a_network_a_running_vm_is_on_is_not_stopped(self): + messages = self.post("stop") + self.assertTrue(any("running VMs net-user" in m for m in messages), messages) + self.assertTrue(self.conn.networkLookupByName("lab").isActive()) + self.conn.lookupByName("net-user").destroy() + self.post("stop") + self.assertFalse(self.conn.networkLookupByName("lab").isActive()) + + def test_a_network_a_vm_definition_names_is_not_deleted(self): + self.conn.lookupByName("net-user").destroy() + self.conn.networkLookupByName("lab").destroy() + messages = self.post("delete") + self.assertTrue(any("net-user" in m and "would no longer start" in m for m in messages), messages) + self.assertIn("lab", self.conn.listDefinedNetworks()) + self.conn.lookupByName("net-user").undefine() + self.post("delete") + self.assertNotIn("lab", self.conn.listDefinedNetworks()) diff --git a/storages/tests/test_pool_in_use.py b/storages/tests/test_pool_in_use.py new file mode 100644 index 00000000..28e341d2 --- /dev/null +++ b/storages/tests/test_pool_in_use.py @@ -0,0 +1,77 @@ +"""A pool a VM needs is not stopped or deleted: stopping one that holds a +running VM's disk would take that disk away, deleting one a VM's definition +names (a disk of type volume) would keep that VM from starting. A disk given +by its file path keeps working without the pool's definition. Runs on a +private libvirt test driver: "test:///default" is shared by the whole process.""" + +import tempfile +from unittest.mock import patch + +import libvirt +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 rest_framework.test import APIClient + +NODE = "use-pool/use-pool" +VOLUME = "v1.img1048576" + + +def domain(name, disk): + return (f"{name}1024hvm" + f"") + + +class PoolInUseTests(TestCase): + def setUp(self): + node = tempfile.NamedTemporaryFile("w", suffix=".xml") + self.addCleanup(node.close) + node.write(NODE) + node.flush() + self.conn = libvirt.open(f"test://{node.name}") + self.addCleanup(self.conn.close) + patcher = patch("vrtManager.connection.connection_manager.get_connection", return_value=self.conn) + patcher.start() + self.addCleanup(patcher.stop) + self.pool = self.conn.storagePoolLookupByName("use-pool") + self.path = self.pool.createXML(VOLUME, 0).path() + self.compute = Compute.objects.create(name="pool-use", hostname="localhost", login="", password="", type=4) + self.admin = get_user_model().objects.create_superuser("pool-use", "pu@example.com", "pw") + self.client.force_login(self.admin) + self.url = reverse("storage", args=[self.compute.id, "use-pool"]) + + def post(self, action): + response = self.client.post(self.url, {action: ""}, HTTP_REFERER=f"http://testserver{self.url}") + return [str(m) for m in get_messages(response.wsgi_request)] + + def test_a_pool_that_holds_a_running_vm_disk_is_not_stopped(self): + self.conn.createXML(domain("disk-user", f"type='file'>"), 0) + messages = self.post("stop") + self.assertTrue(any(m.startswith("Pool use-pool holds disks of the running VMs disk-user") for m in messages), messages) + self.assertTrue(self.pool.isActive()) + self.conn.lookupByName("disk-user").destroy() + self.post("stop") + self.assertFalse(self.pool.isActive()) + + def test_the_api_does_not_stop_it_either(self): + self.conn.createXML(domain("api-user", f"type='file'>"), 0) + api = APIClient() + api.force_login(self.admin) + response = api.post(reverse("compute-storage-stop", kwargs={"compute_pk": self.compute.id, "pk": "use-pool"})) + self.assertEqual(response.status_code, 400) + self.assertIn("running VMs api-user", response.json()["detail"]) + self.assertTrue(self.pool.isActive()) + + def test_a_pool_a_vm_definition_names_is_not_deleted(self): + self.conn.defineXML(domain("by-name", "type='volume'>")) + self.conn.defineXML(domain("by-path", f"type='file'>")) + self.pool.destroy() + messages = self.post("delete") + self.assertTrue(any("by-name" in m and "would no longer start" in m for m in messages), messages) + self.assertFalse(any("by-path" in m for m in messages), messages) + self.assertIn("use-pool", self.conn.listDefinedStoragePools()) + self.conn.lookupByName("by-name").undefine() + self.post("delete") + self.assertNotIn("use-pool", self.conn.listDefinedStoragePools()) diff --git a/storages/views.py b/storages/views.py index 210faf94..c6957bfa 100644 --- a/storages/views.py +++ b/storages/views.py @@ -185,10 +185,17 @@ def storage(request, compute_id, pool): conn.start() return HttpResponseRedirect(request.get_full_path()) if "stop" in request.POST: - conn.stop() + try: + conn.stop() + except OperationError as err: # a running VM's disk is in it + messages.error(request, str(err)) return HttpResponseRedirect(request.get_full_path()) if "delete" in request.POST: - conn.delete() + try: + conn.delete() + except OperationError as err: # a VM names it for a disk + messages.error(request, str(err)) + return HttpResponseRedirect(request.get_full_path()) return HttpResponseRedirect(reverse("storages", args=[compute_id])) if "set_autostart" in request.POST: conn.set_autostart(1) diff --git a/virtsecrets/tests/test_secret_in_use.py b/virtsecrets/tests/test_secret_in_use.py new file mode 100644 index 00000000..2786b63b --- /dev/null +++ b/virtsecrets/tests/test_secret_in_use.py @@ -0,0 +1,64 @@ +"""A secret a VM disk or a pool needs is not deleted: without it that VM or +pool no longer starts. A disk or pool names its secret by UUID or by usage. +libvirt's test driver has no secrets, so the connection is a mock.""" + +from unittest.mock import MagicMock, 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 SimpleTestCase, TestCase +from django.urls import reverse +from vrtManager.util import OperationError +from vrtManager.virtsecrets import wvmSecrets + +UUID = "8f2c0000-0000-4000-8000-000000000001" + + +def libvirt_object(name, xml): + obj = MagicMock() + obj.name.return_value = name + obj.XMLDesc.return_value = xml + return obj + + +def disk_vm(name, secret): + return libvirt_object(name, f"" + f"{secret}") + + +class SecretInUseTests(SimpleTestCase): + def delete(self, domains=(), pools=()): + with patch("vrtManager.connection.connection_manager.get_connection") as get: + wvm = get.return_value + wvm.listAllDomains.return_value = list(domains) + wvm.listAllStoragePools.return_value = list(pools) + secret = wvm.secretLookupByUUIDString.return_value + secret.usageID.return_value = "client.libvirt secret" + wvmSecrets("host", "login", "pw", 1).delete_secret(UUID) + return secret + + def test_a_secret_a_vm_disk_or_a_pool_names_is_not_deleted(self): + pool = libvirt_object("ceph-rbd", "" + "") + with self.assertRaises(OperationError) as refused: + self.delete([disk_vm("db-2", f"")], [pool]) + self.assertIn("VMs db-2", str(refused.exception)) + self.assertIn("pools ceph-rbd", str(refused.exception)) + + def test_an_unused_secret_is_deleted(self): + other = disk_vm("web-1", "") + self.delete([other]).undefine.assert_called_once_with() + + +class SecretInUsePageTests(TestCase): + def test_the_page_says_who_needs_it(self): + compute = Compute.objects.create(name="secret-use", hostname="localhost", login="", password="", type=4) + self.client.force_login(get_user_model().objects.create_superuser("secret-use", "su@example.com", "pw")) + url = reverse("virtsecrets", args=[compute.id]) + with patch("virtsecrets.views.wvmSecrets") as conn: + conn.return_value.get_secrets.return_value = [] + conn.return_value.delete_secret.side_effect = OperationError(f"Secret {UUID} is used by VMs db-2") + response = self.client.post(url, {"delete": "", "uuid": UUID}) + self.assertRedirects(response, url, fetch_redirect_response=False) + self.assertEqual([str(m) for m in get_messages(response.wsgi_request)], [f"Secret {UUID} is used by VMs db-2"]) diff --git a/virtsecrets/views.py b/virtsecrets/views.py index bfe62776..3eeece37 100644 --- a/virtsecrets/views.py +++ b/virtsecrets/views.py @@ -11,6 +11,7 @@ from libvirt import ( VIR_SECRET_USAGE_TYPE_VOLUME, libvirtError, ) +from vrtManager.util import OperationError from vrtManager.virtsecrets import wvmSecrets from virtsecrets.forms import AddSecret @@ -68,7 +69,10 @@ def secrets(request, compute_id): messages.error(request, msg_err.as_text()) if "delete" in request.POST: uuid = request.POST.get("uuid", "") - conn.delete_secret(uuid) + try: + conn.delete_secret(uuid) + except OperationError as err: # a VM disk or a pool needs it + messages.error(request, str(err)) return HttpResponseRedirect(request.get_full_path()) if "set_value" in request.POST: uuid = request.POST.get("uuid", "") diff --git a/vrtManager/connection.py b/vrtManager/connection.py index 7f8c7159..45b8036b 100644 --- a/vrtManager/connection.py +++ b/vrtManager/connection.py @@ -955,6 +955,17 @@ class wvmConnect(object): instances.append(name) return instances + def domains_using(self, match, running_only=False): + """Names of the domains with a definition, parsed, that match(tree) + holds for: the live and the persistent one of each domain, or with + running_only the live one of each running domain.""" + listing = libvirt.VIR_CONNECT_LIST_DOMAINS_ACTIVE if running_only else 0 + flags = (0,) if running_only else (0, libvirt.VIR_DOMAIN_XML_INACTIVE) + return sorted( + dom.name() for dom in self.wvm.listAllDomains(listing) + if any(match(etree.fromstring(dom.XMLDesc(f))) for f in flags) + ) + 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, diff --git a/vrtManager/network.py b/vrtManager/network.py index 784799ce..646333e7 100644 --- a/vrtManager/network.py +++ b/vrtManager/network.py @@ -161,10 +161,25 @@ class wvmNetwork(wvmConnect): def start(self): self.net.create() + def _vms_on_it(self, running_only): + name = self.net.name() + return self.domains_using( + lambda domain: domain.xpath("devices/interface[@type='network']/source[@network=$name]", name=name), + running_only, + ) + def stop(self): + """Refused while a running VM is on it: that VM would be cut off.""" + if vms := self._vms_on_it(running_only=True): + raise util.OperationError(f"Network {self.net.name()} is in use by the running VMs {', '.join(vms)}; " + "shut them down or remove their network cards first") self.net.destroy() def delete(self): + """Refused while a VM's definition names it: that VM would no longer start.""" + if vms := self._vms_on_it(running_only=False): + raise util.OperationError(f"VMs {', '.join(vms)} use network {self.net.name()} and would no longer " + "start; remove their network cards first") self.net.undefine() def update(self, command, section, xml, parentIndex, flags=0): diff --git a/vrtManager/storage.py b/vrtManager/storage.py index e7b5a0bd..4b2a1ce0 100644 --- a/vrtManager/storage.py +++ b/vrtManager/storage.py @@ -155,9 +155,27 @@ class wvmStorage(wvmConnect): return self.pool.create(0) def stop(self): + """Refused while it holds a disk of a running VM, named by the pool + or by a volume's path: that VM would lose the disk.""" + name = self.pool.name() + paths = {vol.path() for vol in self.pool.listAllVolumes()} + vms = self.domains_using( + lambda domain: domain.xpath("devices/disk/source[@pool=$name]", name=name) + or paths.intersection(self._source_paths(domain)), + running_only=True, + ) + if vms: + raise util.OperationError(f"Pool {name} holds disks of the running VMs {', '.join(vms)}; " + "shut them down first") return self.pool.destroy() def delete(self): + """Refused while a VM's definition names it for a disk: that VM would + no longer start. A disk given by its file path does not need it.""" + name = self.pool.name() + if vms := self.domains_using(lambda domain: domain.xpath("devices/disk/source[@pool=$name]", name=name)): + raise util.OperationError(f"VMs {', '.join(vms)} have disks in pool {name} by its name and would no " + "longer start; detach those disks first") self.pool.undefine() def get_autostart(self): diff --git a/vrtManager/virtsecrets.py b/vrtManager/virtsecrets.py index 683e1122..346da177 100644 --- a/vrtManager/virtsecrets.py +++ b/vrtManager/virtsecrets.py @@ -1,6 +1,7 @@ import base64 from lxml import etree +from vrtManager import util from vrtManager.connection import wvmConnect # the element of that holds the value, per usage type @@ -22,5 +23,15 @@ class wvmSecrets(wvmConnect): secrt.setValue(base64.b64decode(value, validate=True)) def delete_secret(self, uuid): + """Refused while a VM disk or a pool names it, by UUID or by usage: + without it that VM or pool would no longer start.""" secrt = self.get_secret(uuid) + refs = "//secret[@uuid=$uuid or @usage=$usage]" + usage = secrt.usageID() or "" + vms = self.domains_using(lambda domain: domain.xpath(f"devices/disk{refs}", uuid=uuid, usage=usage)) + pools = sorted(pool.name() for pool in self.wvm.listAllStoragePools() + if etree.fromstring(pool.XMLDesc(0)).xpath(f"source{refs}", uuid=uuid, usage=usage)) + if vms or pools: + users = "; ".join(f"{kind} {', '.join(names)}" for kind, names in (("VMs", vms), ("pools", pools)) if names) + raise util.OperationError(f"Secret {uuid} is used by {users}; remove it from them first") secrt.undefine()