diff --git a/instances/api/viewsets.py b/instances/api/viewsets.py index fd299c4c..6c34b686 100644 --- a/instances/api/viewsets.py +++ b/instances/api/viewsets.py @@ -17,6 +17,7 @@ from instances.views import ( suspend, ) from drf_spectacular.utils import extend_schema +from libvirt import libvirtError from rest_framework import permissions, status, viewsets from rest_framework.decorators import action from rest_framework.response import Response @@ -266,49 +267,65 @@ class CreateInstanceViewSet(viewsets.ViewSet): compute.type, ) - path = conn.create_volume( - serializer.validated_data["storage"], - serializer.validated_data["name"], - serializer.validated_data["hdd_size"], - default_disk_format, - serializer.validated_data["meta_prealloc"], - default_disk_owner_uid, - default_disk_owner_gid, - ) - volume = {} - volume["device"] = "disk" - volume["path"] = path - volume["type"] = conn.get_volume_format_type(path) - volume["cache_mode"] = serializer.validated_data["cache_mode"] - volume["bus"] = default_bus - if volume["bus"] == "scsi": - volume["scsi_model"] = default_scsi_disk_model - volume["discard_mode"] = default_discard - volume["detect_zeroes_mode"] = default_zeroes - volume["io_mode"] = default_io - - volume_list.append(volume) - - with utils.libvirt_compute_lock(compute): - conn.create_instance( - name=serializer.validated_data["name"], - memory=serializer.validated_data["memory"], - vcpu=serializer.validated_data["vcpu"], - vcpu_mode=serializer.validated_data["vcpu_mode"], - uuid=util.randomUUID(), - arch=arch, - machine=machine, - firmware={}, - volumes=volume_list, - networks=serializer.validated_data["networks"], - nwfilter=serializer.validated_data["nwfilter"], - virtio=serializer.validated_data["virtio"], - listener_addr=serializer.validated_data["listener_addr"], - video=serializer.validated_data["video"], - console_pass=serializer.validated_data["console_pass"], - mac=serializer.validated_data["mac"], - qemu_ga=serializer.validated_data["qemu_ga"], + path = None # the volume this request allocated + try: + path = conn.create_volume( + serializer.validated_data["storage"], + serializer.validated_data["name"], + serializer.validated_data["hdd_size"], + default_disk_format, + serializer.validated_data["meta_prealloc"], + default_disk_owner_uid, + default_disk_owner_gid, ) + volume = {} + volume["device"] = "disk" + volume["path"] = path + volume["type"] = conn.get_volume_format_type(path) + volume["cache_mode"] = serializer.validated_data["cache_mode"] + volume["bus"] = default_bus + if volume["bus"] == "scsi": + volume["scsi_model"] = default_scsi_disk_model + volume["discard_mode"] = default_discard + volume["detect_zeroes_mode"] = default_zeroes + volume["io_mode"] = default_io + + volume_list.append(volume) + + with utils.libvirt_compute_lock(compute): + conn.create_instance( + name=serializer.validated_data["name"], + memory=serializer.validated_data["memory"], + vcpu=serializer.validated_data["vcpu"], + vcpu_mode=serializer.validated_data["vcpu_mode"], + uuid=util.randomUUID(), + arch=arch, + machine=machine, + firmware={}, + volumes=volume_list, + networks=serializer.validated_data["networks"], + nwfilter=serializer.validated_data["nwfilter"], + virtio=serializer.validated_data["virtio"], + listener_addr=serializer.validated_data["listener_addr"], + video=serializer.validated_data["video"], + console_pass=serializer.validated_data["console_pass"], + mac=serializer.validated_data["mac"], + qemu_ga=serializer.validated_data["qemu_ga"], + ) + except Exception as err: + # R-08: a failed definition must not leave the new disk behind + if path: + try: + conn.delete_volume(path) + except libvirtError: + return Response( + {"status": f"{err}; remove {path} by hand"}, status=status.HTTP_400_BAD_REQUEST + ) + if not isinstance(err, libvirtError): + raise + return Response({"status": str(err)}, status=status.HTTP_400_BAD_REQUEST) + # After the definition: the VM uses the volume, so no rollback here. + with utils.libvirt_compute_lock(compute): utils.refresh_instance_database(compute) msg = f"Instance {serializer.validated_data['name']} is created" return Response({"status": msg}) diff --git a/instances/test_create_rollback.py b/instances/test_create_rollback.py new file mode 100644 index 00000000..7c6f717d --- /dev/null +++ b/instances/test_create_rollback.py @@ -0,0 +1,48 @@ +from unittest.mock import patch + +from computes.models import Compute +from django.contrib.auth import get_user_model +from django.test import TestCase +from django.urls import reverse +from libvirt import libvirtError + +FORM = { + "name": "rb-vm", "firmware": "BIOS", "vcpu": 1, "vcpu_mode": "host-model", "memory": 128, + "hdd_size": 1, "storage": "default", "mac": "52:54:00:aa:04:01", "networks": "default", + "nwfilter": "", "net_model": "virtio", "cache_mode": "none", "video": "vga", + "listener_addr": "0.0.0.0", "console_pass": "", "add_cdrom": "None", "add_input": "None", + "create": True, +} + + +class CreateRollbackTestCase(TestCase): + """R-08: volumes are removed only while the VM is not defined yet.""" + + def setUp(self): + admin = get_user_model().objects.create_superuser("rb_admin", "r@example.com", "pw") + self.client.force_login(admin) + self.client.raise_request_exception = False + self.compute = Compute.objects.create(name="rb", hostname="127.0.0.1", login="", password="", type=4) + + def post(self, **conn_setup): + with patch("instances.views.wvmCreate") as wvm: + conn = wvm.return_value + conn.get_cache_modes.return_value = {"none": "Disable cache"} + conn.get_instances.return_value = [] + conn.get_storages.return_value = ["default"] + conn.get_networks.return_value = ["default"] + conn.create_volume.return_value = "/pool/rb-vm.qcow2" + for attr, value in conn_setup.items(): + getattr(conn, attr).side_effect = value + self.client.post(reverse("instances:create_instance", args=[self.compute.id, "x86_64", "q35"]), FORM) + return conn + + def test_failed_definition_removes_the_new_volume(self): + conn = self.post(create_instance=libvirtError("rejected")) + conn.delete_volume.assert_called_once_with("/pool/rb-vm.qcow2") + + def test_failure_after_the_definition_keeps_the_volume(self): + with patch("instances.views.Instance.objects.get_or_create", side_effect=RuntimeError("db down")): + conn = self.post() + conn.create_instance.assert_called_once() + conn.delete_volume.assert_not_called() diff --git a/instances/test_live_create.py b/instances/test_live_create.py new file mode 100644 index 00000000..3cf50944 --- /dev/null +++ b/instances/test_live_create.py @@ -0,0 +1,142 @@ +"""Live checks that a failed VM creation leaves no volumes behind (R-08). + +Runs only with TEST_LIBVIRT_HOST set (see instances/livetest.py). +""" + +import os +import unittest + +import libvirt +from computes.models import Compute +from django.contrib.auth import get_user_model +from django.test import TestCase +from django.urls import reverse +from vrtManager.connection import connection_manager + +from . import livetest + +P = livetest.PREFIX + + +class LiveCreateTestCase(TestCase): + @classmethod + def setUpClass(cls): + if not livetest.enabled(): + raise unittest.SkipTest("Set TEST_LIBVIRT_HOST to run the live libvirt tests") + cls.compute_args = dict( + hostname=os.environ["TEST_LIBVIRT_HOST"], + login=os.environ.get("TEST_LIBVIRT_LOGIN", ""), + password=os.environ.get("TEST_LIBVIRT_PASSWORD", ""), + type=int(os.environ.get("TEST_LIBVIRT_TYPE", 4)), + ) + cls.conn = connection_manager.get_connection(*cls.compute_args.values()) + livetest.cleanup(cls.conn) + livetest.ensure_pool(cls.conn) + cls.inventory = livetest.inventory(cls.conn) + super().setUpClass() + + @classmethod + def tearDownClass(cls): + try: + livetest.remove_pool(cls.conn) + finally: + super().tearDownClass() + if livetest.inventory(cls.conn) != cls.inventory: + raise AssertionError("Live tests changed objects outside the test namespace") + + def setUp(self): + self.compute = Compute.objects.create(name="live-create", details="test", **self.compute_args) + admin = get_user_model().objects.create_superuser("create_admin", "c@example.com", "pw") + self.client.force_login(admin) + self.client.raise_request_exception = False + + def tearDown(self): + livetest.cleanup(self.conn) + + def create(self, name, **overrides): + data = { + "name": P + name, + "firmware": "BIOS", + "vcpu": 1, + "vcpu_mode": "host-model", + "memory": 128, + "hdd_size": 1, + "storage": livetest.POOL, + "mac": "52:54:00:aa:03:01", + "networks": "default", + "nwfilter": "", + "net_model": "virtio", + "cache_mode": "none", + "video": "vga", + "listener_addr": "0.0.0.0", + "console_pass": "", + "add_cdrom": "None", + "add_input": "None", + "create": True, + } + data.update(overrides) + return self.client.post( + reverse("instances:create_instance", args=[self.compute.id, "x86_64", "q35"]), data + ) + + def volumes(self): + pool = self.conn.storagePoolLookupByName(livetest.POOL) + pool.refresh(0) + return set(pool.listVolumes()) + + def domain_exists(self, name): + try: + self.conn.lookupByName(name) + return True + except libvirt.libvirtError: + return False + + def assert_nothing_left(self, name, response): + self.assertLess(response.status_code, 500) + self.assertEqual(self.volumes(), set(), "a volume was left behind") + self.assertFalse(self.domain_exists(P + name)) + + def test_invalid_cache_mode_allocates_nothing(self): + self.assert_nothing_left("cr-cache", self.create("cr-cache", cache_mode="bogus")) + + def test_malformed_uefi_firmware_allocates_nothing(self): + self.assert_nothing_left("cr-uefi", self.create("cr-uefi", firmware="UEFI")) + + def test_rejected_definition_removes_the_new_disk(self): + self.assert_nothing_left("cr-define", self.create("cr-define", video="no-such-model")) + + def test_successful_creation(self): + response = self.create("cr-ok") + self.assertEqual(response.status_code, 302) + self.assertTrue(self.domain_exists(P + "cr-ok")) + self.assertEqual(len(self.volumes()), 1) + + def test_api_rejected_definition_removes_the_new_disk(self): + response = self.client.post( + f"/api/v1/computes/{self.compute.id}/instances/create/x86_64/q35/", + { + "name": P + "cr-api", + "vcpu": 1, + "vcpu_mode": "host-model", + "memory": 128, + "networks": "default", + "mac": "52:54:00:aa:03:02", + "nwfilter": "", + "storage": livetest.POOL, + "hdd_size": 1, + "cache_mode": "none", + "meta_prealloc": False, + "virtio": True, + "qemu_ga": False, + "console_pass": "", + "listener_addr": "0.0.0.0", + "video": "no-such-model", + "add_cdrom": "None", + "add_input": "None", + }, + content_type="application/json", + ) + self.assertEqual(response.status_code, 400, response.content) + self.assertEqual(self.volumes(), set(), "a volume was left behind") + self.assertFalse(self.domain_exists(P + "cr-api")) + diff --git a/instances/views.py b/instances/views.py index 55c55256..43da4f54 100755 --- a/instances/views.py +++ b/instances/views.py @@ -2106,121 +2106,37 @@ def create_instance(request, compute_id, arch, machine): if request.method == "POST": if "create" in request.POST: - firmware = dict() - volume_list = list() - is_disk_created = False - clone_path = "" form = NewVMForm(request.POST) if form.is_valid(): data = form.cleaned_data - if data["meta_prealloc"]: - meta_prealloc = True - if instances: - if data["name"] in instances: - raise util.OperationError( - _("A virtual machine with this name already exists") - ) - if Instance.objects.filter(name__exact=data["name"]): - raise util.OperationError( - _( - "There is an instance with same name. Remove it and try again!" - ) - ) + meta_prealloc = bool(data["meta_prealloc"]) - if data["hdd_size"]: - if not data["mac"]: - raise util.OperationError( - _("No Virtual Machine MAC has been entered") - ) - else: - path = conn.create_volume( - data["storage"], - data["name"], - data["hdd_size"], - default_disk_format, - meta_prealloc, - default_disk_owner_uid, - default_disk_owner_gid, - ) - volume = dict() - volume["device"] = "disk" - volume["path"] = path - volume["type"] = conn.get_volume_format_type(path) - volume["cache_mode"] = data["cache_mode"] - volume["bus"] = default_bus - if volume["bus"] == "scsi": - volume["scsi_model"] = default_scsi_disk_model - volume["discard_mode"] = default_discard - volume["detect_zeroes_mode"] = default_zeroes - volume["io_mode"] = default_io - - volume_list.append(volume) - is_disk_created = True - - elif data["template"]: - templ_path = conn.get_volume_path(data["template"]) - dest_vol = conn.get_volume_path( - data["name"] + ".img", data["storage"] + # Everything is checked before any storage is allocated (R-08). + if data["name"] in instances: + raise util.OperationError(_("A virtual machine with this name already exists")) + if Instance.objects.filter(name__exact=data["name"]): + raise util.OperationError( + _("There is an instance with same name. Remove it and try again!") + ) + if data["hdd_size"] and not data["mac"]: + raise util.OperationError(_("No Virtual Machine MAC has been entered")) + if not (data["hdd_size"] or data["template"] or data["images"]): + raise util.OperationError(_("First you need to create or select an image")) + if ( + not data["hdd_size"] + and data["template"] + and conn.get_volume_path(data["name"] + ".img", data["storage"]) + ): + raise util.OperationError( + _("Image has already exist. Please check volumes or change instance name") ) - if dest_vol: - raise util.OperationError( - _( - "Image has already exist. Please check volumes or change instance name" - ) - ) - else: - clone_path = conn.clone_from_template( - data["name"], - templ_path, - data["storage"], - meta_prealloc, - default_disk_owner_uid, - default_disk_owner_gid, - ) - volume = dict() - volume["path"] = clone_path - volume["type"] = conn.get_volume_format_type(clone_path) - volume["device"] = "disk" - volume["cache_mode"] = data["cache_mode"] - volume["bus"] = default_bus - if volume["bus"] == "scsi": - volume["scsi_model"] = default_scsi_disk_model - volume["discard_mode"] = default_discard - volume["detect_zeroes_mode"] = default_zeroes - volume["io_mode"] = default_io - - volume_list.append(volume) - is_disk_created = True - else: - if not data["images"]: - raise util.OperationError( - _("First you need to create or select an image") - ) - else: - for idx, vol in enumerate(data["images"].split(",")): - path = conn.get_volume_path(vol) - volume = dict() - volume["path"] = path - volume["type"] = conn.get_volume_format_type(path) - volume["device"] = request.POST.get( - "device" + str(idx), "" - ) - volume["bus"] = request.POST.get( - "bus" + str(idx), "" - ) - if volume["bus"] == "scsi": - volume["scsi_model"] = default_scsi_disk_model - volume["cache_mode"] = data["cache_mode"] - volume["discard_mode"] = default_discard - volume["detect_zeroes_mode"] = default_zeroes - volume["io_mode"] = default_io - - volume_list.append(volume) if data["cache_mode"] not in conn.get_cache_modes(): - error_msg = _("Invalid cache mode") - raise libvirtError + raise util.OperationError(_("Invalid cache mode")) + firmware = dict() if "UEFI" in data["firmware"]: + if ":" not in data["firmware"]: + raise util.OperationError(_("Invalid firmware")) firmware["loader"] = data["firmware"].split(":")[1].strip() firmware["secure"] = "no" firmware["readonly"] = "yes" @@ -2237,8 +2153,55 @@ def create_instance(request, compute_id, arch, machine): if data["net_model"] == "default": data["net_model"] = "virtio" + def volume(path, device="disk", bus=default_bus): + return { + "path": path, + "type": conn.get_volume_format_type(path), + "device": device, + "bus": bus, + "scsi_model": default_scsi_disk_model if bus == "scsi" else None, + "cache_mode": data["cache_mode"], + "discard_mode": default_discard, + "detect_zeroes_mode": default_zeroes, + "io_mode": default_io, + } + + created = [] # volumes this request allocated + defined = False + volume_list = [] uuid = util.randomUUID() try: + if data["hdd_size"]: + path = conn.create_volume( + data["storage"], + data["name"], + data["hdd_size"], + default_disk_format, + meta_prealloc, + default_disk_owner_uid, + default_disk_owner_gid, + ) + created.append(path) + volume_list.append(volume(path)) + elif data["template"]: + path = conn.clone_from_template( + data["name"], + conn.get_volume_path(data["template"]), + data["storage"], + meta_prealloc, + default_disk_owner_uid, + default_disk_owner_gid, + ) + created.append(path) + volume_list.append(volume(path)) + else: + for idx, vol in enumerate(data["images"].split(",")): + volume_list.append(volume( + conn.get_volume_path(vol), + request.POST.get("device" + str(idx), ""), + request.POST.get("bus" + str(idx), ""), + )) + with libvirt_compute_lock(compute): conn.create_instance( name=data["name"], @@ -2262,12 +2225,29 @@ def create_instance(request, compute_id, arch, machine): add_cdrom=data["add_cdrom"], add_input=data["add_input"], ) + defined = True # from here on the VM uses the volumes create_instance = Instance.objects.get_or_create( compute_id=compute_id, uuid=uuid, defaults={"name": data["name"]} )[0] if create_instance.name != data["name"]: create_instance.name = data["name"] create_instance.save(update_fields=["name"]) + except Exception as err: + leftovers = [] + for path in [] if defined else created: + try: + conn.delete_volume(path) + except libvirtError: + leftovers.append(path) + if leftovers: + messages.error( + request, + _("Remove these volumes by hand: %(paths)s") % {"paths": ", ".join(leftovers)}, + ) + if not isinstance(err, libvirtError): + raise # unexpected: the volumes are gone, the error stays visible + messages.error(request, err) + else: msg = _("Instance is created") messages.success(request, msg) addlogmsg( @@ -2276,15 +2256,7 @@ def create_instance(request, compute_id, arch, machine): create_instance.name, msg, ) - return redirect( - reverse("instances:instance", args=[create_instance.id]) - ) - except libvirtError as lib_err: - if data["hdd_size"] or len(volume_list) > 0: - if is_disk_created: - for vol in volume_list: - conn.delete_volume(vol["path"]) - messages.error(request, lib_err) + return redirect(reverse("instances:instance", args=[create_instance.id])) conn.close() except libvirtError as lib_err: messages.error(request, lib_err)