j3d1/webvirtcloud
1
0
Fork 0
mirror of https://github.com/retspen/webvirtcloud synced 2026-11-08 19:26:48 +00:00
Code Issues Projects Releases Wiki Activity

fix(instances): a failed VM creation leaves no volumes behind

Browse source 
Web form: the cache mode and the UEFI firmware value were checked only
after the disk volume had been created; an invalid one answered 500 and
left the volume. All checks now run before any allocation. Volumes the
request creates are removed on any failure up to the definition; ones
that cannot be removed are reported. After the definition the VM uses
them, so a later failure keeps them.

API: the new volume was never removed when the definition failed, and
the error was a 500. It is removed now and a libvirt error returns 400.
This commit is contained in:
catborise 2026-10-04 00:27:55 +03:00
parent 5def4e41fd
commit abdd229a1f
4 changed files with 337 additions and 158 deletions
Download patch file Download diff file Expand all files Collapse all files

101
instances/api/viewsets.py
View file

@ -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})

48
instances/test_create_rollback.py Normal file
View file

@ -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()

142
instances/test_live_create.py Normal file
View file

@ -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"))

204
instances/views.py
View file

@ -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)