fix(instances): NIC edits keep their settings; edits keep metadata

- change_network detached the NIC and attached one built from scratch,
  dropping its QoS, link state and other settings. It now edits the NIC
  of the persistent definition in place and defines once; a running VM
  gets the change at its next start, as the UI already says.
- add_network, delete_network and set_link_state did nothing on a
  paused VM, and set_link_state rewrote the persistent NIC from the live
  XML. They now update the running definition when the VM is active and
  the persistent one always, each from its own XML.
- Boot and console edits used xml.etree, which renamed the namespace
  prefixes of other tools' metadata (<app:info> became <ns0:info>); they
  use lxml now.
- The NIC table and the clone form are filled from the persistent
  definition (Instance.config_networks, config_disks), so a pending NIC
  change is shown and can be edited again.

A live test runs 18 domain edits on stopped, running and paused VMs
whose definition has elements WebVirtCloud does not manage, and checks
for each the effect, that nothing else in the persistent definition
changed, and that persistent-only edits leave the running VM alone.

instances/models.py

@@ -135,6 +135,21 @@ class Instance(models.Model):
     def networks(self) -> list[dict]:
         return self.proxy.get_net_devices()
 
+    @cached_property
+    def config_networks(self) -> list[dict]:
+        """NICs of the persistent definition, which the edit and clone forms
+        change; target is the running VM's device name, if any."""
+        live_targets = {net["mac"]: net.get("target", "") for net in self.networks}
+        networks = self.proxy.get_net_devices(config=True)
+        for net in networks:
+            net["target"] = live_targets.get(net["mac"], "")
+        return networks
+
+    @cached_property
+    def config_disks(self) -> list[dict]:
+        """Disks of the persistent definition, which a clone copies."""
+        return self.proxy.get_disk_devices(config=True)
+
     @cached_property
     def qos(self):
         return self.proxy.get_all_qos()

instances/templates/instances/settings_tab.html

@@ -360,7 +360,7 @@
                         </tr>
                     </thead>
                     <tbody>
-                    {% for network in instance.networks %}
+                    {% for network in instance.config_networks %}
                         <tr>
                             <td rowspan="2">eth{{ forloop.counter0 }}({{ network.target|default:"no target" }})
                                 <form action="{% url 'instances:set_link_state' instance.id %}" method="post" aria-label="set instance link state form">
@@ -762,7 +762,7 @@
                 </div>
                 {% if request.user.is_superuser %}
                     <label>{% trans "Network devices" %}:</label>
-                    {% for network in instance.networks %}
+                    {% for network in instance.config_networks %}
                     <p>
                         <div class="row">
                             <label class="col-sm-2 col-form-label offset-1">eth{{ forloop.counter0 }} ({{ network.nic }})</label>
@@ -779,13 +779,13 @@
                     </p>
                     {% endfor %}
                 {% else %}
-                    {% for network in instance.networks %}
+                    {% for network in instance.config_networks %}
                         <input type="hidden" class="form-control" name="clone-net-mac-{{ forloop.counter0 }}" value="{{ network.mac }}"/>
                     {% endfor %}
                 {% endif %}
                 {% if request.user.is_superuser %}
                     <label>{% trans "Storage devices" %}:</label>
-                    {% for disk in instance.disks %}
+                    {% for disk in instance.config_disks %}
                         <div class="row">
                             <label class="col-sm-2 col-form-label offset-1">{{ disk.dev }} ({{ disk.storage }})</label>
                             <div class="col-sm-6">

instances/test_live_dataloss.py

@@ -63,8 +63,8 @@ class LiveDataLossTestCase(TestCase):
 
     # helpers
 
-    def vm(self, name, disks, uefi=False):
-        dom = livetest.define_vm(self.conn, P + name, disks, uefi=uefi)
+    def vm(self, name, disks, uefi=False, extra_devices=""):
+        dom = livetest.define_vm(self.conn, P + name, disks, uefi=uefi, extra_devices=extra_devices)
         refr(self.compute)
         return dom, Instance.objects.get(compute=self.compute, uuid=dom.UUIDString())
 
@@ -268,3 +268,23 @@ class LiveDataLossTestCase(TestCase):
         self.post("change_options", inst, {"title": "for-old", "description": ""})
 
         self.assertNotIn("<title>for-old</title>", dom_new.XMLDesc(0))
+
+    # NIC edits of a running VM are pending until restart; the form must show them
+
+    def test_pending_nic_change_is_shown_and_can_be_edited_again(self):
+        nic = "<interface type='network'><mac address='52:54:00:aa:05:01'/><source network='default'/></interface>"
+        dom, inst = self.vm("nic-pending", [], extra_devices=nic)
+        dom.create()
+
+        def change(old, new):
+            return self.post("change_network", inst, {
+                "net-old-mac-0": old, "net-mac-0": new, "net-source-0": "net:default", "net-model-0": "virtio",
+            })
+
+        change("52:54:00:aa:05:01", "52:54:00:aa:05:02")
+        page = self.client.get(reverse("instances:instance", args=[inst.id])).content.decode()
+        self.assertIn('name="net-old-mac-0" value="52:54:00:aa:05:02"', page)
+
+        change("52:54:00:aa:05:02", "52:54:00:aa:05:03")
+        self.assertIn("52:54:00:aa:05:03", dom.XMLDesc(libvirt.VIR_DOMAIN_XML_INACTIVE))
+

instances/test_live_xml_effects.py

@@ -0,0 +1,265 @@
+"""Live checks that each domain edit does what it says and nothing else.
+
+For every edit the persistent definition (inactive, with secrets) is read
+before and after. The edit must show its effect, and once the nodes it is
+allowed to touch are removed from both sides, the rest of the definition
+must be identical: unknown elements, secrets and pending changes survive.
+On a running VM, edits of the persistent definition must leave the live
+one alone. Runs only with TEST_LIBVIRT_HOST set (see instances/livetest.py).
+"""
+
+import os
+import unittest
+
+import libvirt
+from django.test import SimpleTestCase
+from lxml import etree
+from vrtManager.connection import connection_manager
+from vrtManager.instance import wvmInstance
+
+from . import livetest
+
+P = livetest.PREFIX
+MAC = "52:54:00:aa:02:01"
+PERSISTENT = libvirt.VIR_DOMAIN_XML_INACTIVE | libvirt.VIR_DOMAIN_XML_SECURE
+PARSER = etree.XMLParser(remove_blank_text=True)
+
+
+def domain_xml(name, disk, iso):
+    """A VM with elements WebVirtCloud does not manage, which edits must keep."""
+    return f"""
+<domain type='kvm'>
+  <name>{name}</name>
+  <title>original title</title>
+  <metadata><app:info xmlns:app='http://example.com/wvc-test'>keep-metadata</app:info></metadata>
+  <memory unit='MiB'>128</memory>
+  <currentMemory unit='MiB'>128</currentMemory>
+  <memoryBacking><nosharepages/></memoryBacking>
+  <vcpu placement='static'>1</vcpu>
+  <cputune><shares>2048</shares></cputune>
+  <os><type arch='x86_64' machine='q35'>hvm</type><boot dev='hd'/></os>
+  <features><acpi/><apic/></features>
+  <on_crash>restart</on_crash>
+  <devices>
+    <disk type='file' device='disk'>
+      <driver name='qemu' type='qcow2' cache='none'/>
+      <source file='{disk}'/>
+      <target dev='vda' bus='virtio'/>
+      <serial>keep-serial</serial>
+      <iotune><total_bytes_sec>10485760</total_bytes_sec></iotune>
+    </disk>
+    <disk type='file' device='cdrom'>
+      <driver name='qemu' type='raw'/>
+      <source file='{iso}'/>
+      <target dev='sda' bus='sata'/>
+      <readonly/>
+    </disk>
+    <interface type='network'>
+      <mac address='{MAC}'/>
+      <source network='default'/>
+      <model type='virtio'/>
+      <bandwidth><outbound average='1000' peak='2000' burst='1024'/></bandwidth>
+    </interface>
+    <graphics type='vnc' autoport='yes' passwd='keep1234' keymap='en-us'>
+      <listen type='address' address='0.0.0.0'/>
+    </graphics>
+    <video><model type='vga' primary='yes'/></video>
+    <serial type='pty'><target port='0'/></serial>
+    <rng model='virtio'><backend model='random'>/dev/urandom</backend></rng>
+    <watchdog model='i6300esb' action='reset'/>
+  </devices>
+</domain>"""
+
+
+def first(tree, path):
+    found = tree.xpath(path)
+    return found[0] if found else None
+
+
+# name -> (edit, check of the new persistent tree, paths the edit may change,
+#          allowed while running, changes the running VM too)
+EDITS = {
+    "set_bootmenu": (
+        lambda vm: vm.set_bootmenu(1),
+        lambda t: first(t, "os/bootmenu/@enable") == "yes",
+        ["os/bootmenu"], True, False,
+    ),
+    "set_bootorder": (
+        lambda vm: vm.set_bootorder({0: {"type": "disk", "dev": "vda"}}),
+        lambda t: first(t, "devices/disk[target/@dev='vda']/boot/@order") == "1" and not t.xpath("os/boot"),
+        ["os/boot", "devices/*/boot"], True, False,
+    ),
+    "set_vcpu_hotplug": (
+        lambda vm: vm.set_vcpu_hotplug(True, 1),
+        lambda t: bool(t.xpath("vcpus/vcpu")),
+        ["vcpus"], False, False,
+    ),
+    "set_console_keymap": (
+        lambda vm: vm.set_console_keymap("de"),
+        lambda t: first(t, "devices/graphics/@keymap") == "de",
+        ["devices/graphics/@keymap"], True, False,
+    ),
+    "set_console_listener_addr": (
+        lambda vm: vm.set_console_listener_addr("127.0.0.1"),
+        lambda t: first(t, "devices/graphics/listen/@address") == "127.0.0.1",
+        ["devices/graphics/@listen", "devices/graphics/listen/@address"], True, False,
+    ),
+    "set_console_passwd": (
+        lambda vm: vm.set_console_passwd("new12345"),
+        lambda t: first(t, "devices/graphics/@passwd") == "new12345",
+        ["devices/graphics/@passwd"], True, False,
+    ),
+    "set_video_model": (
+        lambda vm: vm.set_video_model("virtio"),
+        lambda t: first(t, "devices/video/model/@type") == "virtio",
+        ["devices/video"], True, False,
+    ),
+    "resize_cpu": (
+        lambda vm: vm.resize_cpu("1", "2"),
+        lambda t: t.findtext("vcpu") == "2" and first(t, "vcpu/@current") == "1",
+        ["vcpu"], True, False,
+    ),
+    "resize_mem": (
+        lambda vm: vm.resize_mem(128, 160),
+        lambda t: t.findtext("memory") == str(160 * 1024),
+        ["memory", "currentMemory"], False, False,
+    ),
+    "set_options": (
+        lambda vm: vm.set_options({"title": "new title", "description": "new description"}),
+        lambda t: t.findtext("title") == "new title" and t.findtext("description") == "new description",
+        ["title", "description"], True, False,
+    ),
+    "set_qos": (
+        lambda vm: vm.set_qos(MAC, "inbound", 100, 200, 300),
+        lambda t: first(t, "devices/interface/bandwidth/inbound/@average") == "100"
+        and first(t, "devices/interface/bandwidth/outbound/@average") == "1000",
+        ["devices/interface/bandwidth/inbound"], True, False,
+    ),
+    "unset_qos": (
+        lambda vm: vm.unset_qos(MAC, "outbound"),
+        lambda t: not t.xpath("devices/interface/bandwidth/outbound"),
+        ["devices/interface/bandwidth"], True, False,
+    ),
+    "umount_iso": (
+        lambda vm: vm.umount_iso("sda"),
+        lambda t: not t.xpath("devices/disk[target/@dev='sda']/source"),
+        ["devices/disk[target/@dev='sda']/source"], True, True,
+    ),
+    "set_link_state": (
+        lambda vm: vm.set_link_state(MAC, "down"),
+        lambda t: first(t, "devices/interface/link/@state") == "down",
+        ["devices/interface/link"], True, True,
+    ),
+    "change_network (same MAC, new model)": (
+        lambda vm: vm.change_network(MAC, MAC, "default", "net", "e1000e", ""),
+        lambda t: first(t, "devices/interface/model/@type") == "e1000e"
+        and first(t, "devices/interface/bandwidth/outbound/@average") == "1000",
+        ["devices/interface/model"], True, False,
+    ),
+    "add_network": (
+        lambda vm: vm.add_network("52:54:00:aa:02:09", "default", "net", "virtio"),
+        lambda t: len(t.xpath("devices/interface")) == 2,
+        ["devices/interface[mac/@address='52:54:00:aa:02:09']"], True, True,
+    ),
+    "delete_network": (
+        lambda vm: vm.delete_network(MAC),
+        lambda t: not t.xpath("devices/interface"),
+        ["devices/interface"], True, True,
+    ),
+    "edit_disk (cache mode)": (
+        lambda vm: vm.edit_disk("vda", vm.get_disk_devices(config=True)[0]["path"], False, False, "virtio",
+                                "keep-serial", "qcow2", "writeback", None, None, None),
+        lambda t: first(t, "devices/disk[target/@dev='vda']/driver/@cache") == "writeback",
+        ["devices/disk[target/@dev='vda']/driver/@cache"], True, False,
+    ),
+}
+
+
+def canonical(tree, paths):
+    tree = etree.fromstring(etree.tostring(tree), PARSER)
+    for path in paths:
+        for node in tree.xpath(path):
+            if isinstance(node, etree._ElementUnicodeResult):
+                node.getparent().attrib.pop(node.attrname, None)
+            else:
+                node.getparent().remove(node)
+    return etree.tostring(tree, method="c14n").decode()
+
+
+class LiveXmlEffectsTestCase(SimpleTestCase):
+    @classmethod
+    def setUpClass(cls):
+        if not livetest.enabled():
+            raise unittest.SkipTest("Set TEST_LIBVIRT_HOST to run the live libvirt tests")
+        cls.args = (
+            os.environ["TEST_LIBVIRT_HOST"],
+            os.environ.get("TEST_LIBVIRT_LOGIN", ""),
+            os.environ.get("TEST_LIBVIRT_PASSWORD", ""),
+            int(os.environ.get("TEST_LIBVIRT_TYPE", 4)),
+        )
+        cls.conn = connection_manager.get_connection(*cls.args)
+        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 vm(self, running, paused=False):
+        disk = livetest.create_volume(self.conn, P + "fx")
+        iso = self.conn.storagePoolLookupByName(livetest.POOL).createXML(
+            f"<volume><name>{P}fx.iso</name><capacity unit='MiB'>1</capacity><target><format type='raw'/></target></volume>", 0
+        ).path()
+        dom = self.conn.defineXML(domain_xml(P + "fx", disk, iso))
+        if running:
+            dom.create()
+            # A pending change only in the persistent definition
+            tree = self.persistent(dom)
+            tree.find("on_crash").text = "destroy"
+            self.conn.defineXML(etree.tostring(tree).decode())
+            if paused:
+                dom.suspend()
+        return dom, wvmInstance(*self.args, None, uuid=dom.UUIDString())
+
+    def persistent(self, dom):
+        return etree.fromstring(dom.XMLDesc(PERSISTENT), PARSER)
+
+    def live(self, dom):
+        return etree.fromstring(dom.XMLDesc(libvirt.VIR_DOMAIN_XML_SECURE), PARSER)
+
+    def check(self, running, paused=False):
+        for name, (edit, effect, touched, allowed_running, changes_live) in EDITS.items():
+            if running and not allowed_running:
+                continue
+            with self.subTest(name):
+                try:
+                    dom, vm = self.vm(running, paused)
+                    before, live_before = self.persistent(dom), dom.isActive() and self.live(dom)
+                    edit(vm)
+                    after = self.persistent(dom)
+                    self.assertTrue(effect(after), "edit had no effect")
+                    self.assertEqual(canonical(after, touched), canonical(before, touched), "unrelated change")
+                    if running and not changes_live:
+                        self.assertEqual(
+                            canonical(self.live(dom), ["devices/graphics/@port"]),
+                            canonical(live_before, ["devices/graphics/@port"]),
+                            "persistent-only edit changed the running VM",
+                        )
+                finally:
+                    livetest.cleanup(self.conn)
+
+    def test_edits_of_a_stopped_vm(self):
+        self.check(running=False)
+
+    def test_edits_of_a_running_vm(self):
+        self.check(running=True)
+
+    def test_edits_of_a_paused_vm(self):
+        self.check(running=True, paused=True)

instances/views.py

@@ -1748,11 +1748,11 @@ def clone(request, pk):
             clone_data[disk_dev] = disk_name
 
     if not request.user.is_superuser:
-        for disk in instance.disks:
+        for disk in instance.config_disks:
             clone_data[f"disk-{disk['dev']}"] = utils.get_clone_disk_name(
                 disk, instance.name, clone_data["name"]
             )
-        for num in range(max(1, len(instance.networks))):
+        for num in range(max(1, len(instance.config_networks))):
             key = f"clone-net-mac-{num}"
             if not clone_data.get(key):
                 clone_data[key] = (