diff --git a/appsettings/migrations/0022_cpu_mode_choices.py b/appsettings/migrations/0022_cpu_mode_choices.py new file mode 100644 index 00000000..d1e3d980 --- /dev/null +++ b/appsettings/migrations/0022_cpu_mode_choices.py @@ -0,0 +1,25 @@ +from django.db import migrations + +CHOICES = "host-model,host-passthrough,maximum" + + +def fix_cpu_mode(apps, schema_editor): + """The CPU modes a new VM can have: no-model and custom matched no choice + of the new VM dialog, which then showed another one. Not undone on the way + back, as nothing needs the old choices.""" + AppSettings = apps.get_model("appsettings", "AppSettings") + for setting in AppSettings.objects.using(schema_editor.connection.alias).filter(key="INSTANCE_CPU_DEFAULT_MODE"): + setting.choices = CHOICES + if setting.value not in CHOICES.split(","): + setting.value = "host-model" + setting.save() + + +class Migration(migrations.Migration): + dependencies = [ + ("appsettings", "0021_setting_descriptions"), + ] + + operations = [ + migrations.RunPython(fix_cpu_mode, migrations.RunPython.noop), + ] diff --git a/appsettings/tests/test_migrations.py b/appsettings/tests/test_migrations.py index 98cf4f5a..30f0829f 100644 --- a/appsettings/tests/test_migrations.py +++ b/appsettings/tests/test_migrations.py @@ -99,3 +99,17 @@ class DescriptionsTestCase(TestCase): with self.subTest(key=key): self.assertLessEqual(len(text), limit) self.assertEqual(AppSettings.objects.get(key=key).description, text) + + +cpu_mode = importlib.import_module("appsettings.migrations.0022_cpu_mode_choices") + + +class CpuModeChoicesTestCase(TestCase): + def test_the_default_cpu_mode_is_one_the_new_vm_dialog_offers(self): + for old, new in (("no-model", "host-model"), ("custom", "host-model"), ("host-passthrough", "host-passthrough")): + with self.subTest(old=old): + AppSettings.objects.filter(key="INSTANCE_CPU_DEFAULT_MODE").update( + value=old, choices="no-model,host-model,host-passthrough,custom") + cpu_mode.fix_cpu_mode(apps, SimpleNamespace(connection=connection)) + setting = AppSettings.objects.get(key="INSTANCE_CPU_DEFAULT_MODE") + self.assertEqual((setting.value, setting.choices), (new, "host-model,host-passthrough,maximum")) diff --git a/instances/forms.py b/instances/forms.py index ba9247e3..010a26e3 100644 --- a/instances/forms.py +++ b/instances/forms.py @@ -67,7 +67,6 @@ class NewVMForm(forms.ModelForm): # name = forms.CharField(error_messages={'required': _('No Virtual Machine name has been entered')}, max_length=64) # firmware = forms.CharField(max_length=50, required=False) # vcpu = forms.IntegerField(error_messages={'required': _('No VCPU has been entered')}) - # vcpu_mode = forms.CharField(max_length=20, required=False) # disk = forms.IntegerField(required=False) # memory = forms.IntegerField(error_messages={'required': _('No RAM size has been entered')}) # networks = forms.CharField(error_messages={'required': _('No Network pool has been choosen')}) diff --git a/instances/jobs.py b/instances/jobs.py index a6ac5090..7bb72489 100644 --- a/instances/jobs.py +++ b/instances/jobs.py @@ -13,7 +13,7 @@ from libvirt import (VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE, VIR_DOMAIN_JOB_NONE, VIR_ERR_NO_DOMAIN, VIR_ERR_NO_STORAGE_VOL, libvirtError) from vrtManager import util from vrtManager.connection import wvmConnect -from vrtManager.create import wvmCreate +from vrtManager.create import cpu_choices, wvmCreate from vrtManager.instance import check_snapshot_name, wvmInstance from accounts.models import UserInstance, UserSSHKey @@ -665,6 +665,8 @@ class CreateInstance(Kind): raise util.OperationError(_("Image has already exist. Please check volumes or change instance name")) if data["cache_mode"] not in conn.get_cache_modes(): raise util.OperationError(_("Invalid cache mode")) + if data["vcpu_mode"] not in cpu_choices(conn.get_dom_capabilities(data["arch"], data["machine"])): + raise util.OperationError(_("This host offers no CPU mode or model %(mode)s") % {"mode": data["vcpu_mode"]}) if data["add_cdrom"] != "None" and data["add_cdrom"] not in cdrom_buses: raise util.OperationError(_("A CD-ROM cannot use the %(bus)s bus") % {"bus": data["add_cdrom"]}) if data["cloud_init"]: diff --git a/instances/models.py b/instances/models.py index 02c5083c..5f55aeda 100644 --- a/instances/models.py +++ b/instances/models.py @@ -352,7 +352,8 @@ class CreateInstance(models.Model): vcpu = models.IntegerField( error_messages={"required": _("No VCPU has been entered")}, validators=[MinValueValidator(1)] ) - vcpu_mode = models.CharField(max_length=20, blank=True) + # a mode of libvirt's (host-model, host-passthrough, maximum) or a CPU model's name + vcpu_mode = models.CharField(max_length=64) disk = models.IntegerField(blank=True) memory = models.IntegerField( error_messages={"required": _("No RAM size has been entered")}, validators=[MinValueValidator(1)] diff --git a/instances/templates/create_instance_w2.html b/instances/templates/create_instance_w2.html index 23f066b6..62ea9694 100644 --- a/instances/templates/create_instance_w2.html +++ b/instances/templates/create_instance_w2.html @@ -82,8 +82,8 @@ {% if dom_caps.cpu_modes %}
- + {% if not vm_form.vcpu_mode.value %}{% endif %} {% for mode in dom_caps.cpu_modes %} {% if mode == 'custom' %} diff --git a/instances/tests/create_page.py b/instances/tests/create_page.py index 9242ba45..961d7440 100644 --- a/instances/tests/create_page.py +++ b/instances/tests/create_page.py @@ -21,7 +21,8 @@ def render_create_page(testcase, data=None): url = reverse("instances:create_instance", args=[compute.id, "i686", "pc"]) with patch("vrtManager.connection.connection_manager.get_connection", return_value=conn), \ patch("vrtManager.connection.wvmConnect.get_machine_types", return_value=["pc"]), \ - patch("vrtManager.connection.wvmConnect.get_nwfilters", return_value=[]): + patch("vrtManager.connection.wvmConnect.get_nwfilters", return_value=[]), \ + patch("vrtManager.connection.wvmConnect.get_cpu_modes", return_value=["host-model"]): # the test driver has none if data is None: response = testcase.client.get(url) else: diff --git a/instances/tests/test_cloud_init_create.py b/instances/tests/test_cloud_init_create.py index c9a6ac15..5af7f222 100644 --- a/instances/tests/test_cloud_init_create.py +++ b/instances/tests/test_cloud_init_create.py @@ -32,6 +32,7 @@ class CloudInitCreateTestCase(TransactionTestCase): self.addCleanup(patcher.stop) self.conn.get_instances.return_value = [] self.conn.get_cache_modes.return_value = {"none": "Disable cache"} + self.conn.get_dom_capabilities.return_value = {"cpu_modes": ["host-model"]} self.conn.get_disk_bus_types.return_value = ["virtio", "sata"] self.conn.create_volume.return_value = "/srv/default/api-vm.qcow2" self.conn.create_seed.return_value = SEED diff --git a/instances/tests/test_cpu_mode.py b/instances/tests/test_cpu_mode.py new file mode 100644 index 00000000..c69e8073 --- /dev/null +++ b/instances/tests/test_cpu_mode.py @@ -0,0 +1,54 @@ +"""A new VM's CPU: one of libvirt's modes, or a named model that is that +model, checked by libvirt before the start. That the host offers it is +checked when the job is submitted (test_create_job).""" + +from unittest.mock import MagicMock, patch + +from appsettings.models import AppSettings +from appsettings.settings import get_settings +from django.test import SimpleTestCase, TestCase +from instances.tests.create_page import render_create_page +from lxml import etree, html +from vrtManager.create import wvmCreate + +DOM_CAPS = {"domain": "kvm", "os_support": "no", "cpu_modes": ["host-passthrough", "maximum", "host-model", "custom"], + "cpu_custom_models": ["Skylake-Server-noTSX-IBRS", "SandyBridge-IBRS"]} + + +def defined_cpu(vcpu_mode): + conn = wvmCreate.__new__(wvmCreate) + conn.wvm = MagicMock() + with patch.object(wvmCreate, "get_capabilities", return_value={"features": []}), \ + patch.object(wvmCreate, "get_dom_capabilities", return_value=DOM_CAPS): + conn.create_instance(name="vm-1", memory=512, vcpu=1, vcpu_mode=vcpu_mode, + uuid="00000000-0000-4000-8000-0000000000c1", arch="x86_64", machine="q35", + firmware={}, volumes=[], networks="", nwfilter="", virtio=True, listener_addr="0.0.0.0", + add_cdrom="None", add_input="None") + return etree.fromstring(conn.wvm.defineXML.call_args.args[0]).find("cpu") + + +class CpuModeTests(SimpleTestCase): + def test_a_mode_is_written_as_the_mode(self): + for mode in ("host-model", "host-passthrough", "maximum"): + with self.subTest(mode=mode): + cpu = defined_cpu(mode) + self.assertEqual((dict(cpu.attrib), len(cpu)), ({"mode": mode}, 0)) + + def test_a_named_model_is_that_model_checked_by_libvirt(self): + # a name longer than 20 characters too + cpu = defined_cpu("Skylake-Server-noTSX-IBRS") + self.assertEqual(dict(cpu.attrib), {"mode": "custom", "match": "exact"}) # no check='none' + self.assertEqual((cpu.findtext("model"), cpu.find("model").get("fallback")), ("Skylake-Server-noTSX-IBRS", "forbid")) + + +class CpuModeSelectTests(TestCase): + def selected(self): + _, response = render_create_page(self) + return html.fromstring(response.content).xpath("//select[@id='vm_vcpu_mode']/option[@selected]/@value") + + def test_the_default_mode_is_chosen_when_the_host_offers_it_else_one_is_asked_for(self): + get_settings() + self.assertEqual(self.selected(), ["host-model"]) + # not swapped for another mode silently: the browser would take the first one + AppSettings.objects.filter(key="INSTANCE_CPU_DEFAULT_MODE").update(value="maximum") + self.assertEqual(self.selected(), [""]) diff --git a/instances/tests/test_create_form_input.py b/instances/tests/test_create_form_input.py index 231d39d1..058dea62 100644 --- a/instances/tests/test_create_form_input.py +++ b/instances/tests/test_create_form_input.py @@ -20,7 +20,7 @@ from vrtManager import util def valid(**fields): """A create submission NewVMForm accepts.""" - return {"name": "vm1", "firmware": "BIOS", "vcpu": "1", "memory": "512", "networks": "default", + return {"name": "vm1", "firmware": "BIOS", "vcpu": "1", "vcpu_mode": "host-model", "memory": "512", "networks": "default", "cache_mode": "default", "add_cdrom": "None", "add_input": "None", "video": "vga", "listener_addr": "0.0.0.0", "submission_key": uuid.uuid4().hex, **fields} diff --git a/instances/tests/test_create_instance.py b/instances/tests/test_create_instance.py index cbd7287b..1d24e090 100644 --- a/instances/tests/test_create_instance.py +++ b/instances/tests/test_create_instance.py @@ -93,7 +93,7 @@ class CreateInstanceViewTests(TestCase): conn.get_nwfilters.return_value = [] conn.get_network_models.return_value = ["virtio"] conn.get_storages.return_value = ["default"] - conn.get_dom_capabilities.return_value = {"loader_enums": [], "loaders": []} + conn.get_dom_capabilities.return_value = {"loader_enums": [], "loaders": [], "cpu_modes": ["host-model"]} conn.get_capabilities.return_value = {} conn.label_for_firmware_path.return_value = None conn.find_uefi_path_for_arch.return_value = None @@ -136,7 +136,7 @@ class CreateInstanceViewTests(TestCase): conn.get_nwfilters.return_value = [] conn.get_cache_modes.return_value = {} # A host without loader support has no "loaders"/"loader_enums" keys - conn.get_dom_capabilities.return_value = {"loader_support": "no"} + conn.get_dom_capabilities.return_value = {"loader_support": "no", "cpu_modes": ["host-model"]} conn.get_capabilities.return_value = {} conn.label_for_firmware_path.return_value = None conn.find_uefi_path_for_arch.return_value = None diff --git a/instances/tests/test_create_job.py b/instances/tests/test_create_job.py index 76313ebe..5601f20c 100644 --- a/instances/tests/test_create_job.py +++ b/instances/tests/test_create_job.py @@ -32,6 +32,8 @@ class CreateJobTestCase(TransactionTestCase): self.conn.get_instances.return_value = [] self.conn.get_cache_modes.return_value = {"none": "Disable cache"} self.conn.get_disk_bus_types.return_value = ["virtio", "sata"] + self.conn.get_dom_capabilities.return_value = {"cpu_modes": ["host-model", "custom"], + "cpu_custom_models": ["Skylake-Server-noTSX-IBRS"]} self.conn.create_volume.return_value = "/srv/default/api-vm.qcow2" def test_the_api_queues_the_create_and_the_worker_defines_and_records_it(self): @@ -55,6 +57,18 @@ class CreateJobTestCase(TransactionTestCase): self.assertEqual(response.status_code, 400) self.assertFalse(Operation.objects.exists()) + def test_the_cpu_is_a_mode_or_a_model_the_host_offers(self): + url = f"/api/v1/computes/{self.compute.pk}/instances/create/x86_64/q35/" + # a model name longer than 20 characters is taken; none, or one the host lacks, is refused + response = self.client.post(url, {**BODY, "vcpu_mode": "Skylake-Server-noTSX-IBRS"}, content_type="application/json") + self.assertEqual(response.status_code, 202) + for vcpu_mode in ("", "maximum", "EPYC-Genoa", "x" * 65): + with self.subTest(vcpu_mode=vcpu_mode): + body = {**BODY, "vcpu_mode": vcpu_mode, "name": "api-vm2", "submission_key": f"key-{len(vcpu_mode):08}"} + response = self.client.post(url, body, content_type="application/json") + self.assertEqual(response.status_code, 400) + self.assertEqual(Operation.objects.count(), 1) + def test_impossible_sizes_are_refused_by_the_api(self): url = f"/api/v1/computes/{self.compute.pk}/instances/create/x86_64/q35/" for field, value in (("vcpu", 0), ("memory", -512), ("hdd_size", 0)): diff --git a/instances/tests/test_create_rollback.py b/instances/tests/test_create_rollback.py index 071f694a..f6475429 100644 --- a/instances/tests/test_create_rollback.py +++ b/instances/tests/test_create_rollback.py @@ -33,6 +33,7 @@ class CreateRollbackTestCase(TestCase): with patch("instances.jobs.wvmCreate") as wvm: conn = wvm.return_value conn.get_cache_modes.return_value = {"none": "Disable cache"} + conn.get_dom_capabilities.return_value = {"cpu_modes": ["host-model"]} conn.get_instances.return_value = [] conn.create_volume.return_value = "/pool/rb-vm.qcow2" op = jobs.submit("instance.create", self.admin, {**PARAMS, "compute": self.compute.pk}, "key-rollback1") diff --git a/instances/views.py b/instances/views.py index a5dbb8eb..17c891bb 100755 --- a/instances/views.py +++ b/instances/views.py @@ -46,7 +46,7 @@ from operations import claims, jobs from operations.utils import busy_message, summary from webvirtcloud.middleware import error_text, same_origin_referer from vrtManager import util -from vrtManager.create import wvmCreate +from vrtManager.create import cpu_choices, wvmCreate from vrtManager.instance import USAGE_MAX_WINDOW from vrtManager.interface import wvmInterface from vrtManager.storage import wvmStorage @@ -2237,7 +2237,9 @@ def create_instance(request, compute_id, arch, machine): raise util.OperationError(_("You haven't defined any network pools")) vm_form = NewVMForm(initial={ - "vcpu": 1, "memory": 512, "firmware": default_firmware, "vcpu_mode": default_cpu_mode, + # a default this host does not offer is no choice: the select asks for one + "vcpu": 1, "memory": 512, "firmware": default_firmware, + "vcpu_mode": default_cpu_mode if default_cpu_mode in cpu_choices(dom_caps) else "", "cache_mode": default_cache, "net_model": default_nic_type, "listener_addr": "0.0.0.0", "add_cdrom": default_cdrom, "add_input": default_input_device_bus, "video": default_video, "qemu_ga": True, "virtio": bool(virtio_support), diff --git a/vrtManager/create.py b/vrtManager/create.py index 6c047ec2..1e37580d 100644 --- a/vrtManager/create.py +++ b/vrtManager/create.py @@ -7,6 +7,15 @@ import pycdlib from vrtManager import util from vrtManager.connection import wvmConnect +# libvirt's CPU modes a new VM can have; any other value is a CPU model's name (mode custom) +CPU_MODES = ("host-model", "host-passthrough", "maximum") + + +def cpu_choices(dom_caps): + """The CPU modes and named models a new VM can have on a host, from its + domain capabilities (get_dom_capabilities).""" + return [mode for mode in dom_caps["cpu_modes"] if mode in CPU_MODES] + dom_caps.get("cpu_custom_models", []) + def nocloud_seed(instance_id, hostname, user_data): """A cloud-init NoCloud seed: an ISO labelled cidata with the VM's @@ -240,14 +249,11 @@ class wvmCreate(wvmConnect): xml += """""" xml += """""" - if vcpu_mode == "host-model": - xml += """""" - elif vcpu_mode == "host-passthrough": - xml += """""" - elif vcpu_mode != "": - xml += f""" - {vcpu_mode}""" - xml += """""" + # a named model is that model or none: libvirt checks before the start that the host runs it + if vcpu_mode in CPU_MODES: + xml += f"""""" + else: + xml += f"""{vcpu_mode}""" xml += """