fix(instances): a running VM's options and CPU edits keep the pending changes

On an active VM the options and CPU forms write the persistent definition
but showed the running VM's title, description and vCPUs, so saving one
posted the live values back over a pending change. They now read the
persistent definition (get_vcpu, get_cur_vcpu, get_title and
get_description take config=).

Buttons in the settings and resize tabs that were disabled only by class
get the disabled attribute: Enter in a field no longer submits a form
the page shows as unavailable while the VM runs.

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 and the QoS table 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.
+- 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.
 - 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

@@ -166,6 +166,25 @@ class Instance(models.Model):
         """QoS of the persistent definition, which the QoS form changes."""
         return self.proxy.get_all_qos(config=True)
 
+    @cached_property
+    def config_vcpu(self) -> int:
+        """vCPUs of the persistent definition, which the CPU form changes."""
+        return self.proxy.get_vcpu(config=True)
+
+    @cached_property
+    def config_cur_vcpu(self) -> int:
+        return self.proxy.get_cur_vcpu(config=True)
+
+    @cached_property
+    def config_title(self) -> str:
+        """Title and description of the persistent definition, which the
+        options form changes."""
+        return self.proxy.get_title(config=True)
+
+    @cached_property
+    def config_description(self) -> str:
+        return self.proxy.get_description(config=True)
+
     @cached_property
     def telnet_port(self):
         return self.proxy.get_telnet_port()

instances/templates/instances/resize_tab.html

@@ -30,11 +30,7 @@
                         <div class="col-sm-4">
                             <select name="cur_vcpu" class="form-select">
                                 {% for cpu in instance.vcpu_range %}
-                                    {% if instance.cur_vcpu %}
-                                        <option value="{{ cpu }}" {% if cpu == instance.cur_vcpu %}selected{% endif %}>{{ cpu }}</option>
-                                    {% else %}
-                                        <option value="{{ cpu }}" {% if cpu == instance.vcpu %}selected{% endif %}>{{ cpu }}</option>
-                                    {% endif %}
+                                    <option value="{{ cpu }}" {% if cpu == instance.config_cur_vcpu %}selected{% endif %}>{{ cpu }}</option>
                                 {% endfor %}
                             </select>
                         </div>
@@ -44,7 +40,7 @@
                         <div class="col-sm-4">
                             <select name="vcpu" class="form-select">
                                 {% for cpu in instance.vcpu_range %}
-                                    <option value="{{ cpu }}" {% if cpu == instance.vcpu %}selected{% endif %}>{{ cpu }}</option>
+                                    <option value="{{ cpu }}" {% if cpu == instance.config_vcpu %}selected{% endif %}>{{ cpu }}</option>
                                 {% endfor %}
                             </select>
                         </div>
@@ -53,7 +49,7 @@
                     {% if instance.status == 5 %}
                         <button type="submit" class="btn btn-lg btn-success float-end" name="resizevm_cpu">{% trans "Resize" %}</button>
                     {% else %}
-                        <button class="btn btn-lg btn-success float-end disabled">{% trans "Resize" %}</button>
+                        <button class="btn btn-lg btn-success float-end disabled" disabled>{% trans "Resize" %}</button>
                     {% endif %}
                 </form>
                 <div class="clearfix"></div>
@@ -82,7 +78,7 @@
                 {% endif %}
             {% else %}
                 {% trans "You don't have permission for resizing instance" %}
-                <button class="btn btn-lg btn-success float-end disabled">{% trans "Resize" %}</button>
+                <button class="btn btn-lg btn-success float-end disabled" disabled>{% trans "Resize" %}</button>
             {% endif %}
             <div class="clearfix"></div>
         </div>
@@ -123,7 +119,7 @@
                 </form>
             {% else %}
                 {% trans "You don't have permission for resizing instance" %}
-                <button class="btn btn-lg btn-success float-end disabled">{% trans "Resize" %}</button>
+                <button class="btn btn-lg btn-success float-end disabled" disabled>{% trans "Resize" %}</button>
             {% endif %}
             <div class="clearfix"></div>
         </div>
@@ -151,12 +147,12 @@
                     {% if instance.status == 1 or instance.status == 3 or instance.status == 5 %}
                         <button type="submit" class="btn btn-lg btn-success float-end" name="resizevm_disk">{% trans "Resize" %}</button>
                     {% else %}
-                        <button class="btn btn-lg btn-success float-end disabled">{% trans "Resize" %}</button>
+                        <button class="btn btn-lg btn-success float-end disabled" disabled>{% trans "Resize" %}</button>
                     {% endif %}
                 </form>
             {% else %}
                 {% trans "You don't have permission for resizing instance" %}
-                <button class="btn btn-lg btn-success float-end disabled">{% trans "Resize" %}</button>
+                <button class="btn btn-lg btn-success float-end disabled" disabled>{% trans "Resize" %}</button>
             {% endif %}
             <div class="clearfix"></div>
         </div>

instances/templates/instances/settings_tab.html

@@ -201,7 +201,7 @@
                                     {% if instance.media_iso %}
                                         <button type="submit" class="btn btn-sm btn-success" name="mount_iso" value="{{ cd.dev }}">{% trans "Mount" %}</button>
                                     {% else %}
-                                        <button class="btn btn-sm btn-success disabled">{% trans "Mount" %}</button>
+                                        <button class="btn btn-sm btn-success disabled" disabled>{% trans "Mount" %}</button>
                                     {% endif %}
                                     {% if instance.status == 5 %}
                                         <button type="submit" class="btn btn-sm btn-danger float-end" formaction="{% url 'instances:detach_cdrom' instance.id cd.dev %}" formmethod="post" title="{% trans "Detach CD-ROM (remove device)" %}">
@@ -312,7 +312,7 @@
                                             {% bs_icon 'eject' %}
                                         </button>
                                     {% else %}
-                                        <button class="btn btn-sm btn-secondary disabled" value="{{ disk.dev }}" title="{% trans "Detach" %}" onclick="return confirm('{% trans "Are you sure? This may lead data corruption!" %}')">
+                                        <button class="btn btn-sm btn-secondary disabled" disabled value="{{ disk.dev }}" title="{% trans "Detach" %}" onclick="return confirm('{% trans "Are you sure? This may lead data corruption!" %}')">
                                             {% bs_icon 'eject' %}
                                         </button>
                                     {% endif %}
@@ -328,7 +328,7 @@
                                             {% bs_icon 'trash' %}
                                         </button>
                                     {% else %}
-                                        <button class="btn btn-sm btn-secondary disabled" title="{% trans "Delete" %}" onclick="return confirm('{% trans "Are you sure? This may lead data corruption!" %}')">
+                                        <button class="btn btn-sm btn-secondary disabled" disabled title="{% trans "Delete" %}" onclick="return confirm('{% trans "Are you sure? This may lead data corruption!" %}')">
                                             {% bs_icon 'trash' %}
                                         </button>
                                     {% endif %}
@@ -640,7 +640,7 @@
                 {% if computes_count != 1 %}
                     <button type="submit" class="btn btn-lg btn-success float-end" onclick="showPleaseWaitDialog()" name="migrate" >{% trans "Migrate" %}</button>
                 {% else %}
-                    <button class="btn btn-lg btn-success float-end disabled">{% trans "Migrate" %}</button>
+                    <button class="btn btn-lg btn-success float-end disabled" disabled>{% trans "Migrate" %}</button>
                 {% endif %}
             </form>
             <div class="clearfix"></div>
@@ -660,7 +660,7 @@
                         {% trans "Change" %}
                     </button>
                 {% else %}
-                    <button class="btn btn-lg btn-success float-end disabled">
+                    <button class="btn btn-lg btn-success float-end disabled" disabled>
                         {% trans "Change" %}
                     </button>
                 {% endif %}
@@ -804,7 +804,7 @@
                 {% if instance.status == 5 %}
                     <button type="submit" class="btn btn-lg btn-success float-end" name="clone" onclick="showPleaseWaitDialog();">{% trans "Clone" %}</button>
                 {% else %}
-                    <button class="btn btn-lg btn-success float-end disabled" name="clone">{% trans "Clone" %}</button>
+                    <button class="btn btn-lg btn-success float-end disabled" disabled name="clone">{% trans "Clone" %}</button>
                 {% endif %}
             </form>
             <div class="clearfix"></div>
@@ -817,13 +817,13 @@
                 <div class="row">
                     <label class="col-sm-3 col-form-label">{% trans "Title" %}</label>
                     <div class="col-sm-6">
-                        <input type="text" name="title" class="form-control" value="{{ instance.title }}">
+                        <input type="text" name="title" class="form-control" value="{{ instance.config_title }}">
                     </div>
                 </div>
                 <div class="row">
                     <label class="col-sm-3 col-form-label">{% trans "Description" %}</label>
                     <div class="col-sm-6">
-                        <textarea name="description" class="form-control">{{ instance.description }}</textarea>
+                        <textarea name="description" class="form-control">{{ instance.config_description }}</textarea>
                     </div>
                 </div>
                 <div class="row">
@@ -842,7 +842,7 @@
                     {% if instance.status == 5 %}
                         <button type="submit" class="btn btn-success" name="change_options">{% trans "Change" %}</button>
                     {% else %}
-                        <button class="btn btn-success disabled" name="change_options">{% trans "Change" %}</button>
+                        <button class="btn btn-success disabled" disabled name="change_options">{% trans "Change" %}</button>
                     {% endif %}
                     </div>
                 </div>
@@ -863,7 +863,7 @@
                             {% if instance.status == 5 %}
                             <button type="submit" class="btn btn-success" name="set_video_model">{% trans "Set" %}</button>
                             {% else %}
-                            <button class="btn btn-success disabled" name="set_video_model">{% trans "Set" %}</button>
+                            <button class="btn btn-success disabled" disabled name="set_video_model">{% trans "Set" %}</button>
                             {% endif %}
                         </div>
                     </div>

instances/test_live_dataloss.py

@@ -376,6 +376,31 @@ class LiveDataLossTestCase(TestCase):
         self.assertEqual((disk.find("driver").get("cache"), disk.findtext("serial")), ("none", "wvc-serial"))
         self.assertEqual(dom.XMLDesc(0), live)
 
+    def test_pending_options_and_cpu_changes_are_shown_and_kept_by_the_next_edit(self):
+        from lxml import etree, html
+
+        dom, inst = self.vm("options-pending", [])
+        dom.create()
+        live = dom.XMLDesc(0)
+
+        def form(view):
+            page = self.client.get(reverse("instances:instance", args=[inst.id])).content
+            (form,) = html.fromstring(page).xpath("//form[@action=$a]", a=reverse(f"instances:{view}", args=[inst.id]))
+            return dict(form.form_values())  # what the form would post
+
+        # the buttons are disabled while it runs; a direct post is pending
+        self.post("change_options", inst, {**form("change_options"), "title": "wvc-title"})
+        self.post("resizevm_cpu", inst, {**form("resizevm_cpu"), "vcpu": "2"})
+        options, cpu = form("change_options"), form("resizevm_cpu")
+        self.assertEqual((options["title"], cpu["vcpu"]), ("wvc-title", "2"))
+        self.post("change_options", inst, {**options, "description": "wvc-description"})
+        self.post("resizevm_cpu", inst, {**cpu, "cur_vcpu": "1"})
+
+        tree = etree.fromstring(dom.XMLDesc(libvirt.VIR_DOMAIN_XML_INACTIVE))
+        self.assertEqual((tree.findtext("title"), tree.findtext("description"), tree.findtext("vcpu")),
+                         ("wvc-title", "wvc-description", "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

@@ -26,7 +26,9 @@ class PendingEditsTestCase(TestCase):
         compute = Compute.objects.create(name="pe", hostname="127.0.0.1:1", login="root", password="", type=1)
         self.instance = Instance.objects.create(compute=compute, name="pe-vm", uuid="aaaaaaaa-1111-2222-3333-444444444444")
 
-    def page(self, live=(), config=(), qos=({}, {})):  # disks and QoS of the live and the persistent definition
+    def page(self, live=(), config=(), qos=({}, {}), **getters):
+        """live and config: disks; qos and getters (get_vcpu=(2, 4)): values of
+        the live and the persistent definition."""
         with patch("instances.models.wvmInstance") as wvm, patch.object(
             Compute, "status", new_callable=PropertyMock, return_value=True
         ):
@@ -34,8 +36,11 @@ class PendingEditsTestCase(TestCase):
             proxy.get_status.return_value = 1
             proxy.get_memory.return_value = proxy.get_cur_memory.return_value = 1024
             proxy.get_max_memory.return_value = 4096
-            proxy.get_vcpu.return_value = proxy.get_cur_vcpu.return_value = 1
-            proxy.get_max_cpus.return_value = [1, 2]
+            getters = {"get_vcpu": (1, 1), "get_cur_vcpu": (1, 1), "get_title": ("", ""),
+                       "get_description": ("", ""), **getters}
+            for getter, values in getters.items():
+                getattr(proxy, getter).side_effect = lambda config=False, values=values: values[config]
+            proxy.get_max_cpus.return_value = [1, 2, 3, 4]
             proxy.get_vcpus.return_value = {}
             for getter in ("get_networks", "get_ifaces", "get_storages", "get_media_devices", "get_net_devices",
                            "get_snapshot", "get_external_snapshots"):
@@ -78,3 +83,22 @@ class PendingEditsTestCase(TestCase):
             self.client.post(reverse("instances:delete_vol", args=[self.instance.id]), {"dev": "vda"})
         proxy.detach_disk.assert_not_called()
         storage.return_value.del_volume.assert_not_called()
+
+    def test_the_options_and_cpu_forms_show_the_pending_definition(self):
+        doc = self.page(get_vcpu=(2, 4), get_cur_vcpu=(1, 3), get_title=("old", "new"),
+                        get_description=("live text", "pending text"))
+        self.assertEqual(doc.xpath("//input[@name='title']/@value"), ["new"])
+        self.assertEqual(doc.xpath("//textarea[@name='description']/text()"), ["pending text"])
+        self.assertEqual(doc.xpath("//select[@name='vcpu']/option[@selected]/@value"), ["4"])
+        self.assertEqual(doc.xpath("//select[@name='cur_vcpu']/option[@selected]/@value"), ["3"])
+
+    def test_buttons_shown_as_disabled_cannot_submit(self):
+        # running: the CPU, options, video and XML forms wait for a shutdown
+        doc = self.page(live=[disk("vda", "default", "")], config=[disk("vda", "default", "")])
+        shown = doc.xpath("//button[contains(concat(' ', @class, ' '), ' disabled ')]")
+        self.assertGreater(len(shown), 5)
+        self.assertEqual([b for b in shown if b.get("disabled") is None], [])
+        for action in ("resizevm_cpu", "change_options", "set_video_model", "change_xml", "detach_vol", "delete_vol"):
+            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")))

vrtManager/instance.py

@@ -446,13 +446,13 @@ class wvmInstance(wvmConnect):
     def get_uuid(self):
         return self.instance.UUIDString()
 
-    def get_vcpu(self):
-        vcpu = util.get_xml_path(self._XMLDesc(0), "/domain/vcpu")
+    def get_vcpu(self, config=False):
+        vcpu = util.get_xml_path(self._XMLDesc(PERSISTENT_XML if config else 0), "/domain/vcpu")
         return int(vcpu)
 
-    def get_cur_vcpu(self):
-        cur_vcpu = util.get_xml_path(self._XMLDesc(0), "/domain/vcpu/@current")
-        return int(cur_vcpu) if cur_vcpu else self.get_vcpu()
+    def get_cur_vcpu(self, config=False):
+        cur_vcpu = util.get_xml_path(self._XMLDesc(PERSISTENT_XML if config else 0), "/domain/vcpu/@current")
+        return int(cur_vcpu) if cur_vcpu else self.get_vcpu(config)
 
     def get_arch(self):
         return util.get_xml_path(self._XMLDesc(0), "/domain/os/type/@arch")
@@ -497,8 +497,8 @@ class wvmInstance(wvmConnect):
         mem = util.get_xml_path(self._XMLDesc(0), "/domain/currentMemory")
         return int(mem) // 1024
 
-    def get_title(self):
-        title = util.get_xml_path(self._XMLDesc(0), "/domain/title")
+    def get_title(self, config=False):
+        title = util.get_xml_path(self._XMLDesc(PERSISTENT_XML if config else 0), "/domain/title")
         return title or ""
 
     def get_filterrefs(self):
@@ -512,8 +512,8 @@ class wvmInstance(wvmConnect):
 
         return util.get_xml_path(self._XMLDesc(0), func=filterrefs)
 
-    def get_description(self):
-        description = util.get_xml_path(self._XMLDesc(0), "/domain/description")
+    def get_description(self, config=False):
+        description = util.get_xml_path(self._XMLDesc(PERSISTENT_XML if config else 0), "/domain/description")
         return description or ""
 
     def get_max_memory(self):

vrtManager/test_device_edits.py

@@ -1,6 +1,6 @@
 """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. An active VM's memory is resized
+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
 by balloon, a stopped one's in its definition. Enabling vCPU hot plug keeps
 the vCPUs enabled at boot."""
 
@@ -18,7 +18,7 @@ from vrtManager import util
 from vrtManager.instance import PERSISTENT_XML, wvmInstance
 from libvirt import VIR_DOMAIN_AFFECT_CONFIG, VIR_DOMAIN_AFFECT_LIVE
 
-PERSISTENT = """<domain><os><type>hvm</type></os><devices>
+PERSISTENT = """<domain><title>pending</title><vcpu current='2'>4</vcpu><os><type>hvm</type></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>
@@ -26,7 +26,7 @@ PERSISTENT = """<domain><os><type>hvm</type></os><devices>
 <bandwidth><inbound average='1' peak='2' burst='3' floor='4'/></bandwidth></interface>
 <video><model type='vga'/></video>
 </devices></domain>"""
-LIVE = """<domain><devices>
+LIVE = """<domain><vcpu>1</vcpu><devices>
 <disk type='file' device='disk'><source file='/pool/a.qcow2'/><target dev='vda' bus='virtio'/><alias name='virtio-disk0'/></disk>
 </devices></domain>"""
 
@@ -115,6 +115,11 @@ class DeviceEditsTestCase(unittest.TestCase):
         inbound = defined(p).find("devices/interface[@type='network']/bandwidth/inbound")
         self.assertEqual(inbound.attrib, {"average": "10", "peak": "20", "burst": "30", "floor": "4"})
 
+    def test_cpu_title_and_description_are_read_from_either_definition(self):
+        p = proxy()
+        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_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", "")