fix(instances): a running VM's boot order, video and console forms show the pending settings

On an active VM the boot order editor, the video model select and the
console form showed the running VM: a pending boot order, video model or
console setting looked unset, and the boot order editor offered devices
of the running VM only. Choosing one of those cleared the pending order,
since the setter dropped the old order and skipped devices it could not
find. They now read the persistent definition (get_bootmenu,
get_bootorder, get_media_devices, get_video_model and the console
getters take config=), and set_bootorder refuses a device that is not in
it before it changes anything.

README.md

@@ -540,7 +540,7 @@ A rollback leaves the clone on a detached HEAD at the restored commit; the updat
 - A VM's stats are polled every 5 s like the host graph, the first point as soon as the tab opens; each point is the average since the previous one, disk rates in MB/s (they were MiB/s labelled Mb/s); a counter the host does not report shows a gap instead of 0.
 - An ISO uploads to an SSH compute about 3.5 times faster (SFTP writes no longer wait for each 32 KiB); a failed write still fails the upload.
 - On a host whose libvirt cannot change network interfaces (the udev backend: RHEL 9 and later, Debian 12), the interface pages offer no create, start, stop or delete; they only failed there.
-- On a running VM the disk list, its edit form, the QoS table, the title, description and CPU fields show the settings as they will be at the next start, so saving an edit no longer reverts another pending change. A QoS edit keeps the inbound floor. Buttons shown as disabled while the VM runs can no longer be submitted with Enter.
+- On a running VM the disk list, its edit form, the QoS table, the title, description, CPU, boot order, video and console fields show the settings as they will be at the next start, so saving an edit no longer reverts another pending change. A boot order that names a device missing from that definition is refused instead of clearing the order. A QoS edit keeps the inbound floor. Buttons shown as disabled while the VM runs can no longer be submitted with Enter.
 - The VM page shows each NIC's own QoS and each CD-ROM's own pool (they showed the previous device's), and a NIC without a source or model no longer breaks it.
 - A volume of an LVM or iSCSI pool can be attached to a VM, and bridge and direct NICs can be boot devices. The NIC model `default` lets libvirt choose, and `rt18139` is `rtl8139`: a VM with either did not start; a migration corrects the default NIC type setting.
 - A paused VM's memory is resized as a running VM's: the current memory changes and the guest takes it once resumed, the maximum stays (both only reached the VM at its next boot).

instances/models.py

@@ -185,6 +185,38 @@ class Instance(models.Model):
     def config_description(self) -> str:
         return self.proxy.get_description(config=True)
 
+    @cached_property
+    def config_media(self) -> list[dict]:
+        """CD-ROMs, boot menu and boot order of the persistent definition,
+        which the boot order editor changes."""
+        return self.proxy.get_media_devices(config=True)
+
+    @cached_property
+    def config_bootmenu(self) -> bool:
+        return self.proxy.get_bootmenu(config=True)
+
+    @cached_property
+    def config_boot_order(self) -> dict:
+        return self.proxy.get_bootorder(config=True)
+
+    @cached_property
+    def config_video_model(self) -> str | None:
+        """Video model and console settings of the persistent definition,
+        which their forms change."""
+        return self.proxy.get_video_model(config=True)
+
+    @cached_property
+    def config_console_listener_address(self) -> str:
+        return self.proxy.get_console_listener_addr(config=True)
+
+    @cached_property
+    def config_console_passwd(self):
+        return self.proxy.get_console_passwd(config=True)
+
+    @cached_property
+    def config_console_keymap(self) -> str:
+        return self.proxy.get_console_keymap(config=True)
+
     @cached_property
     def telnet_port(self):
         return self.proxy.get_telnet_port()

instances/templates/instance.html

@@ -255,21 +255,21 @@
     });
     $(document).ready(function () {
         // set current console keymap or fall back to default
-        var keymap = "{{ instance.console_keymap }}";
+        var keymap = "{{ instance.config_console_keymap }}";
         if (keymap != '') {
             $("#console_select_keymap option[value='" + keymap + "']").prop('selected', true);
         }
     });
     $(document).ready(function () {
         // set current console listen address or fall back to default
-        var console_listener_address = "{{ instance.console_listener_address }}";
+        var console_listener_address = "{{ instance.config_console_listener_address }}";
         if (console_listener_address != '') {
             $("#console_select_listener_address option[value='" + console_listener_address + "']").prop('selected', true);
         }
     });
     $(document).ready(function () {
         // get video model or fall back to default
-        let video_model = "{{ instance.video_model }}";
+        let video_model = "{{ instance.config_video_model }}";
         if (video_model != '') {
             $("#video_model_select option[value='" + video_model + "']").prop('selected', true);
         }

instances/templates/instances/settings_tab.html

@@ -99,7 +99,7 @@
                     <div class="col-sm-12 text-center">
                     {% if instance.status == 5 %}
                         <p>{% trans "Enable Boot Menu for your instance when it starts up " %}
-                        {% if instance.bootmenu == 0 %}
+                        {% if instance.config_bootmenu == 0 %}
                             <form action="{% url 'instances:set_bootmenu' instance.id %}" method="post" role="form" aria-label="Enable/disable instance boot order form">{% csrf_token %}
                                 <input type="submit" class="btn btn-success" name="set_bootmenu" title="{% trans 'Show boot menu' %}" value="{% trans "Enable" %}">
                             </form>
@@ -109,17 +109,17 @@
                             </form>
                         {% endif %}
                     {% else %}
-                        {% if instance.bootmenu == 0  %}
+                        {% if instance.config_bootmenu == 0  %}
                             <p>**** {% trans "Please shutdown instance to modify boot menu" %} ****</p>
                         {% endif %}
                     {% endif %}
                     </div>
                 </div>
 
-            {% if instance.bootmenu == 1  %}
+            {% if instance.config_bootmenu == 1  %}
                 <div class="d-flex justify-content-center">
                     <div class="col-sm-6 bg-light rounded shadow-sm">
-                        {% for idx, val in instance.boot_order.items %}
+                        {% for idx, val in instance.config_boot_order.items %}
                             <label>{{ idx|add:1 }}:{{ val.target }}, </label>
                         {% endfor %}
                     </div>
@@ -128,13 +128,13 @@
                     <input id="bootorder" name="bootorder" hidden>
                     <div class="d-flex justify-content-center">
                         <div id="b_order" class="multipleselect border-0">
-                            {% for disk in instance.disks %}
+                            {% for disk in instance.config_disks %}
                                 <label><input type="checkbox" name="disk:{{ disk.dev }}" value="disk:{{ disk.dev }}" onclick="set_orderlist($('#bootorder'))" />{{ disk.dev }} - {{ disk.image }}</label>
                             {% endfor %}
-                            {% for cd in instance.media %}
+                            {% for cd in instance.config_media %}
                                 <label><input type="checkbox" name="cdrom:{{ cd.dev }}" value="cdrom:{{ cd.dev }}" onclick="set_orderlist($('#bootorder'))"/>{{ cd.dev }} - {{ cd.image }}</label>
                             {% endfor %}
-                            {% for net in instance.networks %}
+                            {% for net in instance.config_networks %}
                                 <label><input type="checkbox" name="network:{{ net.mac }}" value="network:{{ net.mac }}" onclick="set_orderlist($('#bootorder'))"/>NIC - {{ net.mac|slice:"9:" }}</label>
                             {% endfor %}
                         </div>
@@ -857,7 +857,7 @@
                             <select id="video_model_select" class="form-select" name="video_model">
                                 <option class="fw-bold" value="">{% trans "please choose" %}</option>
                                 {% for vmodel in instance.video_models %}
-                                    <option value="{{ vmodel }}"{% if vmodel == instance.video_model %} selected{% endif %}>{{ vmodel }}</option>
+                                    <option value="{{ vmodel }}"{% if vmodel == instance.config_video_model %} selected{% endif %}>{{ vmodel }}</option>
                                 {% endfor %}
                             </select>
                             {% if instance.status == 5 %}

instances/test_live_dataloss.py

@@ -401,6 +401,34 @@ class LiveDataLossTestCase(TestCase):
                          ("wvc-title", "wvc-description", "2"))
         self.assertEqual(dom.XMLDesc(0), live)
 
+    def test_pending_boot_order_and_video_are_shown_and_a_live_only_choice_changes_nothing(self):
+        from lxml import etree, html
+
+        nic = "<interface type='network'><mac address='52:54:00:aa:05:21'/><source network='default'/></interface>"
+        graphics = "<graphics type='vnc' autoport='yes' listen='127.0.0.1'/>"
+        dom, inst = self.vm("boot-pending", [livetest.create_volume(self.conn, P + "boot-pending")],
+                            extra_devices=nic + graphics)
+        self.post("set_bootmenu", inst)
+        dom.create()
+        # pending order; a NIC the running VM has but its next start will not
+        self.post("set_bootorder", inst, {"bootorder": "network:52:54:00:aa:05:21,disk:vda"})
+        dom.attachDeviceFlags(nic.replace("05:21", "05:22"), libvirt.VIR_DOMAIN_AFFECT_LIVE)
+        persistent = lambda: etree.fromstring(dom.XMLDesc(libvirt.VIR_DOMAIN_XML_INACTIVE))  # noqa: E731
+        video = "virtio" if persistent().find("devices/video/model").get("type") != "virtio" else "vga"
+        self.post("set_video_model", inst, {"video_model": video})
+        live = dom.XMLDesc(0)
+
+        page = html.fromstring(self.client.get(reverse("instances:instance", args=[inst.id])).content)
+        (boot,) = page.xpath("//form[@action=$a]", a=reverse("instances:set_bootorder", args=[inst.id]))
+        self.assertEqual(boot.xpath(".//input[@type='checkbox']/@value"), ["disk:vda", "network:52:54:00:aa:05:21"])
+        self.assertEqual(page.xpath("//select[@name='video_model']/option[@selected]/@value"), [video])
+
+        self.post("set_bootorder", inst, {"bootorder": "network:52:54:00:aa:05:22,disk:vda"})
+        tree = persistent()
+        self.assertEqual(tree.xpath("devices/interface/boot/@order"), ["1"])
+        self.assertEqual(tree.xpath("devices/disk[target/@dev='vda']/boot/@order"), ["2"])
+        self.assertEqual(dom.XMLDesc(0), live)
+
     def test_pending_qos_change_is_shown_and_kept_by_the_next_edit(self):
         from lxml import etree, html
 

instances/test_pending_edits.py

@@ -6,10 +6,12 @@ from unittest.mock import PropertyMock, patch
 
 from computes.models import Compute
 from django.contrib.auth import get_user_model
+from django.contrib.messages import get_messages
 from django.test import TestCase
 from django.urls import reverse
 from instances.models import Instance
 from lxml import html
+from vrtManager import util
 
 
 def disk(dev, cache, serial):
@@ -48,6 +50,7 @@ class PendingEditsTestCase(TestCase):
             proxy.get_disk_devices.side_effect = lambda **kw: config if kw.get("config") else live
             proxy.get_all_qos.side_effect = lambda **kw: qos[1] if kw.get("config") else qos[0]
             proxy.get_console_type.return_value = "vnc"
+            proxy.get_video_models.return_value = ["vga", "virtio"]
             proxy.get_cache_modes.return_value = {"default": "Default", "none": "Disabled"}
             response = self.client.get(reverse("instances:instance", args=[self.instance.id]))
         self.assertEqual(response.status_code, 200)
@@ -102,3 +105,28 @@ class PendingEditsTestCase(TestCase):
             with self.subTest(action):
                 (form,) = doc.xpath("//form[@action=$a]", a=reverse(f"instances:{action}", args=[self.instance.id]))
                 self.assertTrue(all(b.get("disabled") is not None for b in form.xpath(".//button")))
+
+    def test_the_boot_video_and_console_forms_show_the_pending_definition(self):
+        cd = {"dev": "sda", "image": "pending.iso", "storage": "default", "path": "/pool/pending.iso", "bus": "sata"}
+        with patch("instances.models.Instance.config_networks", new_callable=PropertyMock, return_value=[]):
+            doc = self.page(
+                live=[disk("vda", "default", "")], config=[disk("vda", "default", ""), disk("vdb", "default", "")],
+                get_bootmenu=(False, True), get_bootorder=({}, {0: {"type": "file", "dev": "cdrom", "target": "sda"}}),
+                get_media_devices=([], [cd]), get_video_model=("vga", "virtio"),
+                get_console_listener_addr=("127.0.0.1", "0.0.0.0"), get_console_keymap=("", "de"),
+                get_console_passwd=("live-pw", "pending-pw"),
+            )
+        (boot,) = doc.xpath("//form[@action=$a]", a=reverse("instances:set_bootorder", args=[self.instance.id]))
+        self.assertEqual(boot.xpath(".//input[@type='checkbox']/@value"), ["disk:vda", "disk:vdb", "cdrom:sda"])
+        self.assertIn("1:sda", doc.xpath("string(//div[label[contains(., ':')]])"))
+        self.assertEqual(doc.xpath("//select[@name='video_model']/option[@selected]/@value"), ["virtio"])
+        self.assertEqual(doc.xpath("//select[@name='keymap']/option[@selected]/@value"), ["de"])
+        self.assertEqual(doc.xpath("//select[@name='listen_on']/option[@selected]/@value"), ["0.0.0.0"])
+        self.assertEqual(doc.xpath("//input[@name='password']/@value"), ["pending-pw"])
+
+    def test_a_boot_order_choice_not_in_the_definition_is_refused(self):
+        with patch("instances.models.wvmInstance") as wvm:
+            wvm.return_value.set_bootorder.side_effect = util.OperationError("Boot device vdz is not in the definition")
+            response = self.client.post(reverse("instances:set_bootorder", args=[self.instance.id]),
+                                        {"bootorder": "disk:vda,disk:vdz"})
+        self.assertEqual([str(m) for m in get_messages(response.wsgi_request)], ["Boot device vdz is not in the definition"])

instances/views.py

@@ -121,9 +121,9 @@ def _instance_page(request, pk, instance, compute):
     if can_manage_console:
         console_form = ConsoleForm(
             initial={
-                "listen_on": instance.console_listener_address,
-                "password": instance.console_passwd,
-                "keymap": instance.console_keymap,
+                "listen_on": instance.config_console_listener_address,
+                "password": instance.config_console_passwd,
+                "keymap": instance.config_console_keymap,
             },
             can_set_listener=request.user.is_superuser,
         )
@@ -1501,7 +1501,11 @@ def set_bootorder(request, pk):
         for idx, val in enumerate(bootorder.split(",")):
             dev_type, dev = val.split(":", 1)
             order_list[idx] = {"type": dev_type, "dev": dev}
-        instance.proxy.set_bootorder(order_list)
+        try:
+            instance.proxy.set_bootorder(order_list)
+        except util.OperationError as err:
+            messages.error(request, err)
+            return _back(request, pk, "boot_opt")
         msg = _("Set boot order")
 
         if not instance.proxy.get_status() == 5:

vrtManager/instance.py

@@ -732,7 +732,7 @@ class wvmInstance(wvmConnect):
 
         return self._cached(("volume", path), read)
 
-    def get_media_devices(self):
+    def get_media_devices(self, config=False):
         def disks(doc):
             result = []
             for media in doc.xpath("/domain/devices/disk[@device='cdrom']"):
@@ -753,10 +753,10 @@ class wvmInstance(wvmConnect):
                 )
             return result
 
-        return util.get_xml_path(self._XMLDesc(0), func=disks)
+        return util.get_xml_path(self._XMLDesc(PERSISTENT_XML if config else 0), func=disks)
 
-    def get_bootmenu(self):
-        menu = util.get_xml_path(self._XMLDesc(0), "/domain/os/bootmenu/@enable")
+    def get_bootmenu(self, config=False):
+        menu = util.get_xml_path(self._XMLDesc(PERSISTENT_XML if config else 0), "/domain/os/bootmenu/@enable")
         return menu == "yes"
 
     def set_bootmenu(self, flag):
@@ -783,10 +783,10 @@ class wvmInstance(wvmConnect):
         xmldom = etree.tostring(tree).decode()
         self._defineXML(xmldom)
 
-    def get_bootorder(self):
+    def get_bootorder(self, config=False):
         boot_order = {}
         dev_type = target = None
-        tree = ElementTree.fromstring(self._XMLDesc(0))
+        tree = ElementTree.fromstring(self._XMLDesc(PERSISTENT_XML if config else 0))
         os = tree.find("os")
         boot = os.findall("boot")
 
@@ -858,7 +858,10 @@ class wvmInstance(wvmConnect):
             elif dev["type"] == "network":  # any NIC type: network, bridge, direct
                 devices = tree.xpath("./devices/interface[mac/@address=$mac]", mac=dev["dev"])
             else:
-                raise Exception("Invalid Device Type for boot order")
+                raise util.OperationError("Invalid Device Type for boot order")
+            # nothing is defined until every choice is found: the order above is cleared
+            if not devices:
+                raise util.OperationError(f"Boot device {dev['dev']} is not in the definition")
             for d in devices:
                 d.append(order)
         self._defineXML(etree.tostring(tree).decode())
@@ -1252,13 +1255,13 @@ class wvmInstance(wvmConnect):
                                 telnet_port = service_port
         return telnet_port
 
-    def get_console_listener_addr(self):
+    def get_console_listener_addr(self, config=False):
         listener_addr = util.get_xml_path(
-            self._XMLDesc(0), "/domain/devices/graphics/@listen"
+            self._XMLDesc(PERSISTENT_XML if config else 0), "/domain/devices/graphics/@listen"
         )
         if listener_addr is None:
             listener_addr = util.get_xml_path(
-                self._XMLDesc(0), "/domain/devices/graphics/listen/@address"
+                self._XMLDesc(PERSISTENT_XML if config else 0), "/domain/devices/graphics/listen/@address"
             )
             if listener_addr is None:
                 return "127.0.0.1"
@@ -1341,9 +1344,9 @@ class wvmInstance(wvmConnect):
         )
         return websocket_port
 
-    def get_console_passwd(self):
+    def get_console_passwd(self, config=False):
         return util.get_xml_path(
-            self._XMLDesc(VIR_DOMAIN_XML_SECURE), "/domain/devices/graphics/@passwd"
+            self._XMLDesc(PERSISTENT_XML if config else VIR_DOMAIN_XML_SECURE), "/domain/devices/graphics/@passwd"
         )
 
     def set_console_passwd(self, passwd):
@@ -1374,17 +1377,17 @@ class wvmInstance(wvmConnect):
         newxml = etree.tostring(root).decode()
         self._defineXML(newxml)
 
-    def get_console_keymap(self):
+    def get_console_keymap(self, config=False):
         return (
             util.get_xml_path(
-                self._XMLDesc(VIR_DOMAIN_XML_SECURE), "/domain/devices/graphics/@keymap"
+                self._XMLDesc(PERSISTENT_XML if config else VIR_DOMAIN_XML_SECURE), "/domain/devices/graphics/@keymap"
             )
             or ""
         )
 
-    def get_video_model(self):
+    def get_video_model(self, config=False):
         """:return only primary video card"""
-        xml = self._XMLDesc(VIR_DOMAIN_XML_SECURE)
+        xml = self._XMLDesc(PERSISTENT_XML if config else VIR_DOMAIN_XML_SECURE)
         tree = etree.fromstring(xml)
         video_models = tree.xpath("/domain/devices/video/model")
         for model in video_models:

vrtManager/test_device_edits.py

@@ -1,6 +1,7 @@
 """Device edits of a VM: each definition is changed as it describes the
 device, every NIC type can be a boot device, posted values stay values, and
-definition changes go through _defineXML, a QoS edit keeps the floor, and the CPU, title and description are read from either definition. An active VM's memory is resized
+definition changes go through _defineXML, a QoS edit keeps the floor, and what the edit forms show is read from either definition. A boot order with a
+missing device changes nothing. An active VM's memory is resized
 by balloon, a stopped one's in its definition. Enabling vCPU hot plug keeps
 the vCPUs enabled at boot."""
 
@@ -18,15 +19,17 @@ from vrtManager import util
 from vrtManager.instance import PERSISTENT_XML, wvmInstance
 from libvirt import VIR_DOMAIN_AFFECT_CONFIG, VIR_DOMAIN_AFFECT_LIVE
 
-PERSISTENT = """<domain><title>pending</title><vcpu current='2'>4</vcpu><os><type>hvm</type></os><devices>
+PERSISTENT = """<domain><title>pending</title><vcpu current='2'>4</vcpu><os><type>hvm</type><bootmenu enable='yes'/></os><devices>
 <disk type='file' device='disk'><source file='/pool/a.qcow2'/><target dev='vda' bus='virtio'/></disk>
 <disk type='file' device='disk'><source file='/pool/b.qcow2'/><target dev='vdb' bus='virtio'/></disk>
 <interface type='bridge'><mac address='52:54:10:00:00:02'/><source bridge='br0'/></interface>
 <interface type='network'><mac address='52:54:10:00:00:01'/><source network='default'/>
 <bandwidth><inbound average='1' peak='2' burst='3' floor='4'/></bandwidth></interface>
+<disk type='file' device='cdrom'><target dev='sda' bus='sata'/><boot order='1'/></disk>
 <video><model type='vga'/></video>
+<graphics type='vnc' passwd='pending' keymap='de'><listen type='address' address='0.0.0.0'/></graphics>
 </devices></domain>"""
-LIVE = """<domain><vcpu>1</vcpu><devices>
+LIVE = """<domain><vcpu>1</vcpu><os><type>hvm</type></os><devices>
 <disk type='file' device='disk'><source file='/pool/a.qcow2'/><target dev='vda' bus='virtio'/><alias name='virtio-disk0'/></disk>
 </devices></domain>"""
 
@@ -120,6 +123,25 @@ class DeviceEditsTestCase(unittest.TestCase):
         self.assertEqual((p.get_vcpu(), p.get_cur_vcpu(), p.get_title(), p.get_description()), (1, 1, "", ""))
         self.assertEqual((p.get_vcpu(config=True), p.get_cur_vcpu(config=True), p.get_title(config=True)), (4, 2, "pending"))
 
+    def test_boot_video_and_console_are_read_from_either_definition(self):
+        p = proxy()
+        live = (p.get_bootmenu(), p.get_bootorder(), p.get_media_devices(), p.get_video_model(),
+                p.get_console_listener_addr(), p.get_console_passwd(), p.get_console_keymap())
+        self.assertEqual(live, (False, {}, [], None, "127.0.0.1", None, ""))
+        self.assertEqual((p.get_bootmenu(config=True), p.get_bootorder(config=True), p.get_video_model(config=True)),
+                         (True, {0: {"type": "file", "dev": "cdrom", "target": "sda"}}, "vga"))
+        self.assertEqual([m["dev"] for m in p.get_media_devices(config=True)], ["sda"])
+        self.assertEqual((p.get_console_listener_addr(config=True), p.get_console_passwd(config=True),
+                          p.get_console_keymap(config=True)), ("0.0.0.0", "pending", "de"))
+
+    def test_a_boot_order_with_a_device_not_in_the_definition_changes_nothing(self):
+        p = proxy()
+        for choice in ({"type": "disk", "dev": "vdz"}, {"type": "network", "dev": "52:54:10:00:00:99"},
+                       {"type": "floppy", "dev": "fda"}):
+            with self.subTest(choice), self.assertRaises(util.OperationError):
+                p.set_bootorder({0: {"type": "disk", "dev": "vda"}, 1: choice})
+        p._defineXML.assert_not_called()
+
     def test_the_default_nic_model_leaves_the_choice_to_libvirt(self):
         p = proxy()
         p.change_network("52:54:10:00:00:01", "52:54:10:00:00:01", "default", "net", "default", "")