From 2e1766139009163019c64ff4299cd31db0956710 Mon Sep 17 00:00:00 2001 From: catborise Date: Sat, 31 Oct 2026 17:05:20 +0300 Subject: [PATCH] fix(instances): a network device keeps its source unless another is chosen The Edit dialog of a VM's network device selected the listed network or interface whose name matched the device's source, and the first one when none did: saving only a new model moved a macvtap device or a bridge the host's lists did not show onto the first network, a bridge named like a network became that network, and a macvtap device chosen again lost its mode. The dialog now offers the device's own source first, ", as it is", and posts nothing for it; the view passes no source and the device's source element is left as it is. Choosing another source changes it as before. --- .../templates/instances/settings_tab.html | 6 ++++-- instances/tests/test_change_network.py | 4 ++++ instances/tests/test_pending_edits.py | 10 ++++++++-- instances/views.py | 4 +++- vrtManager/instance.py | 20 ++++++++++--------- vrtManager/tests/test_change_network.py | 15 +++++++++++++- 6 files changed, 44 insertions(+), 15 deletions(-) diff --git a/instances/templates/instances/settings_tab.html b/instances/templates/instances/settings_tab.html index e8a12997..4f213508 100644 --- a/instances/templates/instances/settings_tab.html +++ b/instances/templates/instances/settings_tab.html @@ -359,14 +359,16 @@
diff --git a/instances/tests/test_change_network.py b/instances/tests/test_change_network.py index 0e5cbae1..8bcca934 100644 --- a/instances/tests/test_change_network.py +++ b/instances/tests/test_change_network.py @@ -34,6 +34,10 @@ class ChangeNetworkViewTestCase(TestCase): }) change.assert_called_once_with("52:54:00:00:00:02", "52:54:00:00:00:03", "default", "net", "e1000", "") + def test_no_source_keeps_the_nics_own(self): + change = self.post({"net-old-mac-1": "52:54:00:00:00:02", "net-source-1": "", "net-model-1": "e1000"}) + change.assert_called_once_with("52:54:00:00:00:02", "52:54:00:00:00:02", None, None, "e1000", "") + def test_a_shut_off_vm_is_told_the_change_is_made(self): with patch("instances.models.wvmInstance") as wvm: wvm.return_value.get_status.return_value = 5 diff --git a/instances/tests/test_pending_edits.py b/instances/tests/test_pending_edits.py index b118784d..62d4a817 100644 --- a/instances/tests/test_pending_edits.py +++ b/instances/tests/test_pending_edits.py @@ -93,10 +93,16 @@ class PendingEditsTestCase(TestCase): # the Edit dialog picks the source by kind and posts it as before (source,) = doc.xpath("//select[@name='net-source-0']") self.assertEqual(source.xpath("./optgroup/@label"), ["Networks", "Interfaces"]) - self.assertEqual(source.xpath(".//option/@value"), ["net:default", "net:isolated", "iface:br0"]) - self.assertEqual(source.xpath(".//option[@selected]/@value"), ["net:default"]) + self.assertEqual(source.xpath(".//option/@value"), ["", "net:default", "net:isolated", "iface:br0"]) + # the NIC keeps its source unless another is chosen + self.assertEqual(source.xpath(".//option[@selected]/text()"), ["default, as it is"]) self.assertEqual(doc.xpath("//div[@id='editInstanceNetwork0']//input[@name='net-old-mac-0']/@value"), [nic["mac"]]) self.assertTrue(doc.xpath("//div[@id='editInstanceNetwork0']//button[@name='change_network']")) + # so does one the host's lists do not show, or one whose name is in both + doc = self.page(nics=([{**nic, "type": "bridge", "nic": "br0"}],) * 2, + get_networks=(["br0"],) * 2, get_ifaces=(["br0"],) * 2) + (source,) = doc.xpath("//select[@name='net-source-0']") + self.assertEqual(source.xpath(".//option[@selected]/@value"), [""]) def test_the_disks_are_rows_of_one_list(self): disks = [disk("vda", "default", "") | {"used": 1 << 29}, disk("vdb", "default", "") | {"used": (1 << 30) + (1 << 20)}] diff --git a/instances/views.py b/instances/views.py index 7b7a6bc4..f7a0b927 100755 --- a/instances/views.py +++ b/instances/views.py @@ -1856,7 +1856,9 @@ def change_network(request, pk): messages.error(request, err) return redirect(back) - (source, source_type) = utils.get_network_tuple(request.POST.get(f"net-source-{num}", "")) + # no source: the NIC keeps the one it has, which the host's lists may not show + raw_source = request.POST.get(f"net-source-{num}", "") + (source, source_type) = utils.get_network_tuple(raw_source) if raw_source else (None, None) try: with audit_vm(request, instance, f"change interface {old_mac}"): if source_type == "iface": diff --git a/vrtManager/instance.py b/vrtManager/instance.py index 99aeea86..d84eaa53 100644 --- a/vrtManager/instance.py +++ b/vrtManager/instance.py @@ -2706,7 +2706,8 @@ class wvmInstance(wvmConnect): """Edit the NIC with old_mac in the persistent definition, in place. Settings this form does not edit (QoS, link state, driver, PCI address) - stay. A running VM gets the change at its next start. + stay, and so does the source when source is None. A running VM gets + the change at its next start. """ tree = etree.fromstring(self._XMLDesc(PERSISTENT_XML)) found = tree.xpath("./devices/interface[mac/@address=$mac]", mac=old_mac) @@ -2716,14 +2717,15 @@ class wvmInstance(wvmConnect): if iface.get("type") == "hostdev": raise util.OperationError("A network interface that is a host device is changed in the XML editor") - iface_type = {"net": "network", "bridge": "bridge"}.get(source_type, "direct") - iface.set("type", iface_type) - for old_source in iface.findall("source"): - iface.remove(old_source) - source_attrs = {"network": {"network": source}, "bridge": {"bridge": source}}.get( - iface_type, {"dev": source, "mode": "bridge"} - ) - iface.insert(1, etree.Element("source", **source_attrs)) + if source is not None: + iface_type = {"net": "network", "bridge": "bridge"}.get(source_type, "direct") + iface.set("type", iface_type) + for old_source in iface.findall("source"): + iface.remove(old_source) + source_attrs = {"network": {"network": source}, "bridge": {"bridge": source}}.get( + iface_type, {"dev": source, "mode": "bridge"} + ) + iface.insert(1, etree.Element("source", **source_attrs)) iface.find("mac").set("address", mac) for old_model in iface.findall("model"): diff --git a/vrtManager/tests/test_change_network.py b/vrtManager/tests/test_change_network.py index 025f48ca..10621558 100644 --- a/vrtManager/tests/test_change_network.py +++ b/vrtManager/tests/test_change_network.py @@ -14,6 +14,7 @@ DOMAIN_XML = """
+ """ @@ -34,7 +35,7 @@ class TestChangeNetwork(unittest.TestCase): inst.change_network("52:54:00:00:00:02", "52:54:00:00:00:03", "br0", "bridge", "e1000", "clean-traffic") defined = etree.fromstring(inst.wvm.defineXML.call_args[0][0]) - first, second = defined.findall("devices/interface") + first, second, _direct = defined.findall("devices/interface") self.assertEqual(first.find("mac").get("address"), "52:54:00:00:00:01") self.assertEqual(second.get("type"), "bridge") self.assertEqual(second.find("mac").get("address"), "52:54:00:00:00:03") @@ -46,6 +47,18 @@ class TestChangeNetwork(unittest.TestCase): self.assertEqual(second.find("link").get("state"), "down") self.assertEqual(second.find("address").get("bus"), "0x02") + def defined_nic(self, *args): + from lxml import etree + + inst = stopped_instance() + inst.change_network(*args) + return etree.fromstring(inst.wvm.defineXML.call_args[0][0]).findall("devices/interface")[2] + + def test_no_source_keeps_the_one_the_nic_has(self): + nic = self.defined_nic("52:54:00:00:00:04", "52:54:00:00:00:04", None, None, "e1000", "") + self.assertEqual((nic.get("type"), nic.find("source").attrib), ("direct", {"dev": "eth0", "mode": "vepa"})) + self.assertEqual(nic.find("model").get("type"), "e1000") + def test_unknown_old_mac_changes_nothing(self): inst = stopped_instance() with self.assertRaises(libvirtError):