fix(instances): a new VM's CPU is a mode or a model the host offers, checked by libvirt

- A named CPU model is written without check='none' and with
  fallback='forbid': libvirt checks before the start that the host runs
  it, and it is not swapped for another model.
- Mode maximum is written as that mode; it was written as a model named
  maximum.
- Model names up to 64 characters: the -noTSX and -IBRS models, the ones a
  mixed set of hosts needs, were refused at 20.
- The mode or model is checked against the host's capabilities when the VM
  is defined, from the page and the API alike; no CPU element (QEMU's
  oldest default CPU) is no longer offered.
- The default CPU mode setting offers the modes the new VM dialog has;
  no-model and custom, which matched none of them, become host-model.

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),
+    ]

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

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

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"]:

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

instances/templates/create_instance_w2.html

@@ -82,8 +82,8 @@
                         {% if dom_caps.cpu_modes %}
                         <div class="col-md-6">
                             <label for="vm_vcpu_mode" class="form-label">{% trans "CPU mode" %}</label>
-                            <select id="vm_vcpu_mode" class="form-select{{ vm_form.vcpu_mode|invalid }}" name="vcpu_mode"{% error_attrs vm_form.vcpu_mode "vm_vcpu_mode" %}>
-                                <option value="">{% trans 'no-mode' %}</option>
+                            <select id="vm_vcpu_mode" class="form-select{{ vm_form.vcpu_mode|invalid }}" name="vcpu_mode" required{% error_attrs vm_form.vcpu_mode "vm_vcpu_mode" %}>
+                                {% if not vm_form.vcpu_mode.value %}<option value="" selected disabled>{% trans "Choose…" %}</option>{% endif %}
                                 {% for mode in dom_caps.cpu_modes %}
                                     {% if mode == 'custom' %}
                                         <optgroup label="{% trans "Custom CPU Models" %}">

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:

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

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(), [""])

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}
 

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

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

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

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

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 += """<smm state="on"/>"""
             xml += """</features>"""
 
-        if vcpu_mode == "host-model":
-            xml += """<cpu mode='host-model'/>"""
-        elif vcpu_mode == "host-passthrough":
-            xml += """<cpu mode='host-passthrough'/>"""
-        elif vcpu_mode != "":
-            xml += f"""<cpu mode='custom' match='exact' check='none'>
-                        <model fallback='allow'>{vcpu_mode}</model>"""
-            xml += """</cpu>"""
+        # 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"""<cpu mode='{vcpu_mode}'/>"""
+        else:
+            xml += f"""<cpu mode='custom' match='exact'><model fallback='forbid'>{vcpu_mode}</model></cpu>"""
 
         xml += """
                   <clock offset="utc"/>