diff --git a/README.md b/README.md index 26824b53..2deb8abb 100644 --- a/README.md +++ b/README.md @@ -578,7 +578,7 @@ A version from before v3.0.0 has no worker: `systemctl disable --now webvirt-wor - The admin's first-login password change survives losing `data/admin_password` (migration `accounts.0007`); run `migrate` as a user who can read that file. - "VM Clone Auto Migrate" is removed; a clone stays on its source host. - A template VM is changed or deleted only by superusers and staff owners with the matching permission; other owners can still view, open the console and clone it. Power cycle no longer starts a template. -- A VM that vanishes from its compute keeps its owners for `INSTANCE_OWNERSHIP_RETENTION_DAYS` (30) in case the same UUID comes back; superusers list and remove these records on the "Removed VMs" admin page. +- A VM that vanishes from its compute, or whose compute is deleted, keeps its owners for `INSTANCE_OWNERSHIP_RETENTION_DAYS` (30) in case the same UUID comes back; superusers list and remove these records on the "Removed VMs" admin page. - The log rotates (10 MB × 5) in `data/webvirtcloud.log`. - A disk can be grown while its VM runs or is paused, not only when it is shut off. A disk on an LVM pool cannot be grown (libvirt cannot resize its volumes): the resize says so instead of failing, and the VM's other disks still grow. - Enabling vCPU hot plug keeps the VM's current vCPU count (it enabled all of them up to the maximum). diff --git a/computes/models.py b/computes/models.py index 175e34c5..1bd4fa6e 100644 --- a/computes/models.py +++ b/computes/models.py @@ -1,5 +1,6 @@ from ipaddress import ip_address +from django.db import transaction from django.db.models import CharField, IntegerField, Model from django.utils.functional import cached_property from django.utils.translation import gettext_lazy as _ @@ -88,3 +89,17 @@ class Compute(Model): def __str__(self): return self.name + + def delete(self, *args, **kwargs): + """Its VMs keep running on the host: their owners wait in tombstones, + or follow a VM already listed on another compute. The compute is held + exclusive, so no reconcile, VM operation or job on it runs meanwhile + (Busy if one does); its row is locked before its VMs', as the + reconcile does.""" + from computes.utils import forget, libvirt_compute_lock + + with libvirt_compute_lock(self), transaction.atomic(): + Compute.objects.select_for_update().filter(pk=self.pk).first() + for inst in self.instance_set.select_for_update(): + forget(inst) + return super().delete(*args, **kwargs) diff --git a/computes/tests/test_ownership.py b/computes/tests/test_ownership.py index 390b4bdd..2d7bc53b 100644 --- a/computes/tests/test_ownership.py +++ b/computes/tests/test_ownership.py @@ -8,8 +8,12 @@ from computes.models import Compute from computes.utils import refresh_instance_database from django.contrib.auth import get_user_model from django.test import TestCase, override_settings +from django.urls import reverse from django.utils import timezone from instances.models import Instance, InstanceTombstone +from operations import claims +from operations.models import Claim +from rest_framework.test import APIClient UUID = "44444444-4444-4444-4444-444444444444" @@ -111,3 +115,52 @@ class OwnershipTestCase(TestCase): refresh_instance_database(self.host_a) self.assertEqual(self.ownership(self.host_a), (True, [])) + + # a deleted compute's VMs keep running on the host: their owners wait in + # tombstones as for a VM that disappeared, or follow a VM already on another compute + def delete(self, compute): + admin = get_user_model().objects.create_superuser("cd-admin", "cd@example.com", "pw") + self.client.force_login(admin) + self.client.post(reverse("compute_delete", args=[compute.id])) + self.assertFalse(Compute.objects.filter(pk=compute.pk).exists()) + + def test_a_host_added_again_gets_its_vms_back_with_their_owners(self): + self.owned_vm(self.host_a) + self.delete(self.host_a) + self.assertEqual(InstanceTombstone.objects.get().owners[0]["user"], self.owner.id) + + again = self.compute("host-a-again") + self.on(again, [("vm", UUID)]) + refresh_instance_database(again) + self.assertEqual(self.ownership(again), (True, [(self.owner, True, False, True)])) + + def test_a_vm_already_on_another_compute_keeps_its_owners_there(self): + self.owned_vm(self.host_a) + self.on(self.host_b, [("vm", UUID)]) + refresh_instance_database(self.host_b) # listed on B too: a copy for now + self.delete(self.host_a) + self.assertEqual(self.ownership(self.host_b), (True, [(self.owner, True, False, True)])) + self.assertFalse(InstanceTombstone.objects.exists()) + + def test_the_api_delete_keeps_the_owners_too(self): + self.owned_vm(self.host_a) + api = APIClient() + api.force_login(get_user_model().objects.create_superuser("cd-api", "cda@example.com", "pw")) + self.assertEqual(api.delete(reverse("compute-detail", args=[self.host_a.id])).status_code, 204) + self.assertEqual(InstanceTombstone.objects.get().owners[0]["user"], self.owner.id) + + def test_a_compute_in_use_is_not_deleted(self): + # a reconcile, VM operation or job holds it: the delete waits for none of them + vm = self.owned_vm(self.host_a) + claims.admit({claims.compute_key(self.host_a.id): Claim.SHARED}, holder="192.0.2.30:1:1") + admin = get_user_model().objects.create_superuser("cd-busy", "cdb@example.com", "pw") + self.client.force_login(admin) + api = APIClient() + api.force_login(admin) + url = reverse("compute_delete", args=[self.host_a.id]) + self.assertRedirects(self.client.post(url, HTTP_REFERER=f"http://testserver{url}"), url, + fetch_redirect_response=False) + self.assertEqual(api.delete(reverse("compute-detail", args=[self.host_a.id])).status_code, 409) + self.assertTrue(Compute.objects.filter(pk=self.host_a.pk).exists()) + self.assertEqual(list(vm.userinstance_set.values_list("user", flat=True)), [self.owner.id]) + self.assertFalse(InstanceTombstone.objects.exists()) diff --git a/computes/utils.py b/computes/utils.py index d5f2e10a..eb86d862 100644 --- a/computes/utils.py +++ b/computes/utils.py @@ -62,6 +62,19 @@ def _move_ownership(source, target): target.save(update_fields=["is_template"]) +def forget(inst): + """Remove an instance whose VM its compute no longer lists, or whose + compute is deleted. Its ownership goes to the same UUID on another + compute (an out-of-band move) or is kept in a tombstone in case the VM + comes back.""" + elsewhere = Instance.objects.filter(uuid=inst.uuid).exclude(compute_id=inst.compute_id).first() + if elsewhere: + _move_ownership(inst, elsewhere) + else: + _bury(inst) + inst.delete() + + def _bury(inst): owners = [ {"user": ui.user_id, "is_change": ui.is_change, "is_delete": ui.is_delete, "is_vnc": ui.is_vnc} @@ -165,17 +178,10 @@ def refresh_instance_database(compute): inst.name = new_name inst.save(update_fields=["name"]) - # 2. Instances gone from this compute. Their ownership goes to the - # same UUID on another compute (an out-of-band move) or is kept - # in a tombstone in case the VM comes back. + # 2. Instances gone from this compute for inst in db_instances: if inst.uuid not in host_domains: - elsewhere = Instance.objects.filter(uuid=inst.uuid).exclude(compute=compute).first() - if elsewhere: - _move_ownership(inst, elsewhere) - else: - _bury(inst) - inst.delete() + forget(inst) # 3. Instances created out-of-band. A UUID that also exists on another # compute is a copy and gets no owners; otherwise a tombstone's diff --git a/computes/views.py b/computes/views.py index 4a3a1427..7d4b6c87 100644 --- a/computes/views.py +++ b/computes/views.py @@ -1,6 +1,7 @@ import json from admin.decorators import superuser_only +from django.conf import settings from django.core import signing from django.http import HttpResponse from django.shortcuts import get_object_or_404, redirect, render @@ -129,8 +130,9 @@ def compute_delete(request, compute_id): request, "common/confirm_delete.html", {"object": compute, - "consequence": _("Its VMs keep running on the host, but the panel forgets them and who owns them; " - "added again, the host's VMs come back without owners.")}, + "consequence": _("Its VMs keep running on the host and leave the panel; their owners are kept for " + "%(days)s days on the Removed VMs page and come back if the host is added again.") + % {"days": getattr(settings, "INSTANCE_OWNERSHIP_RETENTION_DAYS", 30)}}, ) diff --git a/storages/tests/test_upload_job.py b/storages/tests/test_upload_job.py index 3cc2314d..682de786 100644 --- a/storages/tests/test_upload_job.py +++ b/storages/tests/test_upload_job.py @@ -303,7 +303,9 @@ class LocalUploadTests(LocalPool, UploadTestCase): def test_ending_without_the_compute(self): op = self.send(0, b"aaa") - self.compute.delete() + with self.assertRaises(claims.Busy): # the upload holds the compute + self.compute.delete() + Compute.objects.filter(pk=self.compute.pk).delete() # the row gone without Compute.delete() op = jobs.cancel(op.pk, self.admin) self.assertEqual((op.state, op.error), (Operation.FAILED, "The compute was deleted")) diff --git a/webvirtcloud/tests/test_delete_pages.py b/webvirtcloud/tests/test_delete_pages.py index 07c0ee65..90a8721a 100644 --- a/webvirtcloud/tests/test_delete_pages.py +++ b/webvirtcloud/tests/test_delete_pages.py @@ -19,7 +19,7 @@ class DeletePageTests(TestCase): compute = Compute.objects.create(name="del-host", hostname="192.0.2.9", login="u", password="p", type=1) vm = Instance.objects.create(compute=compute, name="del-vm", uuid="d0000000-0000-4000-8000-000000000001") pages = { - reverse("compute_delete", args=[compute.id]): "the panel forgets them and who owns them", + reverse("compute_delete", args=[compute.id]): "their owners are kept for 30 days on the Removed VMs page", reverse("admin:user_delete", args=[user.id]): "the VMs themselves stay", reverse("admin:group_delete", args=[Group.objects.create(name="del-group").id]): "lose the permissions", reverse("admin:tombstone_delete", args=[InstanceTombstone.objects.create(uuid=vm.uuid, name="gone").id]):