From c73faac7f95ec837da6f964d15cc05ff9ffd2de6 Mon Sep 17 00:00:00 2001 From: Christian Breunig Date: Wed, 7 Oct 2026 19:56:37 +0200 Subject: interfaces: T3871: name interfaces from a slot-anchored mapping store Names used to be derived from 'hw-id' in config.boot. A node carrying an address but no hw-id had to be matched to some NIC, and nothing available at boot could say which one - every heuristic could bind a configured address to the wrong physical wire. Naming authority moves to a mapping store binding a name to the slot the hardware occupies. The slot is the identity, so a replacement card in that slot is that interface again. The store renders systemd .link files, leaving udev to apply names as devices appear. --- src/tests/test_ifname_store.py | 572 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 572 insertions(+) create mode 100644 src/tests/test_ifname_store.py (limited to 'src/tests/test_ifname_store.py') diff --git a/src/tests/test_ifname_store.py b/src/tests/test_ifname_store.py new file mode 100644 index 000000000..3f12594d4 --- /dev/null +++ b/src/tests/test_ifname_store.py @@ -0,0 +1,572 @@ +# Copyright VyOS maintainers and contributors +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License version 2 or later as +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . +# +# Coverage for T3871: slot-anchored persistent interface naming. +# +# The regression classes at the bottom replay, verbatim, the field +# reproductions from the review of vyos-1x PR #5417 - the cases where the +# config-tree-driven resolver bound a configured address to the wrong +# physical wire. Under the store-based design none of them have a code path +# to travel, and these tests exist to keep it that way. + +import os +import shutil +import tempfile +import unittest +from pathlib import Path +from unittest import mock + +from vyos.ifconfig.ifname_store import STORE_VERSION +from vyos.ifconfig.ifname_store import canonical_sort_key +from vyos.ifconfig.ifname_store import device_matches +from vyos.ifconfig.ifname_store import ambiguous_keys +from vyos.ifconfig.ifname_store import hardware_key +from vyos.ifconfig.ifname_store import discover_devices +from vyos.ifconfig.ifname_store import empty_store +from vyos.ifconfig.ifname_store import load_store +from vyos.ifconfig.ifname_store import permanent_mac +from vyos.ifconfig.ifname_store import render_link +from vyos.ifconfig.ifname_store import resolve +from vyos.ifconfig.ifname_store import sync_link_files +from vyos.ifconfig.ifname_store import save_store + + +def dev(name, path, mac, wireless=False, bus='pci'): + """A device sitting at 'path'. Its key is what udev would report for it - + here the raw ID_PATH, which is what a plain virtio or e1000e NIC with no + firmware slot index ends up keyed on.""" + properties = {'ID_PATH': path, 'ID_BUS': bus} if path else {} + return {'name': name, 'mac': mac, 'wireless': wireless, + 'properties': properties, + 'key': hardware_key(properties)} + + +def store_of(mapping, hardware=None): + """mapping: {name: path} - the slot each name is recorded against""" + return { + 'version': STORE_VERSION, + 'interfaces': {n: f'ID_PATH={p}' for n, p in mapping.items()}, + 'hardware': dict(hardware or {}), + } + + +class TestCanonicalOrder(unittest.TestCase): + def test_pci_sorted_numerically_not_lexically(self): + # 0000:09:00.0 must precede 0000:10:00.0 - string order gets this wrong + a = dev('x', 'pci-0000:00:09.0', 'aa:00:00:00:00:01') + b = dev('y', 'pci-0000:00:10.0', 'aa:00:00:00:00:02') + self.assertLess(canonical_sort_key(a), canonical_sort_key(b)) + + def test_pci_before_usb(self): + pci = dev('x', 'pci-0000:00:1f.6', 'aa:00:00:00:00:01') + usb = dev('y', 'usb-0:1:1.0', 'aa:00:00:00:00:00', bus='usb') + self.assertLess(canonical_sort_key(pci), canonical_sort_key(usb)) + + def test_order_is_independent_of_mac(self): + # the whole point: a numerically higher MAC in a lower slot still wins + low_slot = dev('x', 'pci-0000:00:02.0', 'ff:ff:ff:ff:ff:ff') + high_slot = dev('y', 'pci-0000:00:05.0', '00:00:00:00:00:01') + self.assertLess(canonical_sort_key(low_slot), canonical_sort_key(high_slot)) + + +class TestReplacedHardware(unittest.TestCase): + """A slot keeps its name when its card is swapped - which hands that + interface's addresses to different hardware. resolve() reports the change + so vyos-router can warn instead of letting it happen silently. + """ + + PATH = 'pci-0000:00:12.0' + + def _device(self, mac): + return dev('eth0', self.PATH, mac) + + def test_same_card_reports_nothing(self): + store = store_of({'eth0': self.PATH}, {'eth0': 'aa:aa:aa:aa:aa:01'}) + _, _, report = resolve([self._device('aa:aa:aa:aa:aa:01')], store) + self.assertEqual(report['replaced'], {}) + + def test_swapped_card_is_reported_with_its_new_mac(self): + store = store_of({'eth0': self.PATH}, {'eth0': 'aa:aa:aa:aa:aa:01'}) + _, new_store, report = resolve([self._device('bb:bb:bb:bb:bb:02')], store) + self.assertEqual(report['replaced'], {'eth0': 'bb:bb:bb:bb:bb:02'}) + # the name is still the slot's, and the new card is now what is on record + self.assertEqual(new_store['interfaces']['eth0'], f'ID_PATH={self.PATH}') + self.assertEqual(new_store['hardware']['eth0'], 'bb:bb:bb:bb:bb:02') + + def test_reported_once_not_on_every_later_boot(self): + store = store_of({'eth0': self.PATH}, {'eth0': 'aa:aa:aa:aa:aa:01'}) + _, store, _ = resolve([self._device('bb:bb:bb:bb:bb:02')], store) + _, _, report = resolve([self._device('bb:bb:bb:bb:bb:02')], store) + self.assertEqual(report['replaced'], {}) + + def test_absent_card_keeps_its_record_so_a_later_swap_is_caught(self): + store = store_of({'eth0': self.PATH}, {'eth0': 'aa:aa:aa:aa:aa:01'}) + _, store, _ = resolve([], store) + self.assertEqual(store['hardware']['eth0'], 'aa:aa:aa:aa:aa:01') + _, _, report = resolve([self._device('bb:bb:bb:bb:bb:02')], store) + self.assertEqual(report['replaced'], {'eth0': 'bb:bb:bb:bb:bb:02'}) + + def test_new_hardware_is_not_a_replacement(self): + _, _, report = resolve([self._device('aa:aa:aa:aa:aa:01')], empty_store()) + self.assertEqual(report['replaced'], {}) + self.assertIn('eth0', report['bootstrapped']) + + def test_store_written_before_this_field_reports_nothing(self): + # nothing to compare against yet - and no interface may be renamed + legacy = {'version': STORE_VERSION, + 'interfaces': {'eth0': f'ID_PATH={self.PATH}'}} + plan, new_store, report = resolve([self._device('aa:aa:aa:aa:aa:01')], + legacy) + self.assertEqual(report['replaced'], {}) + self.assertEqual(plan, {}) + self.assertEqual(new_store['interfaces']['eth0'], f'ID_PATH={self.PATH}') + + +class TestStoreIO(unittest.TestCase): + @staticmethod + def _config_dir(d): + """A directory that looks like a properly mounted config volume.""" + (Path(d) / '.vyatta_config').touch() + return Path(d) / 'interface-mapping.json' + + def test_roundtrip(self): + with tempfile.TemporaryDirectory() as d: + p = self._config_dir(d) + s = store_of({'eth0': 'pci-0000:00:12.0'}) + self.assertTrue(save_store(s, p)) + self.assertEqual(load_store(p), s) + + def test_refuses_to_write_when_config_volume_is_not_mounted(self): + # TPM/LUKS unlock failed: vyos-router continues, but persisting here + # would write a shadow store that the real volume masks later + with tempfile.TemporaryDirectory() as d: + p = Path(d) / 'interface-mapping.json' # no .vyatta_config marker + self.assertFalse(save_store(store_of({'eth0': 'x'}), p)) + self.assertFalse(p.exists()) + + def test_missing_file_is_empty_store(self): + with tempfile.TemporaryDirectory() as d: + self.assertEqual(load_store(Path(d) / 'nope.json'), empty_store()) + + def test_corrupt_file_is_empty_store(self): + with tempfile.TemporaryDirectory() as d: + p = self._config_dir(d) + p.write_text('{ this is not json') + self.assertEqual(load_store(p), empty_store()) + + def test_write_is_atomic_no_leftovers(self): + with tempfile.TemporaryDirectory() as d: + p = self._config_dir(d) + save_store(store_of({'eth0': 'pci-0000:00:12.0'}), p) + self.assertEqual(sorted(f.name for f in Path(d).iterdir()), + ['.vyatta_config', 'interface-mapping.json']) + + +def final_names(report): + """Which name each MAC ended up with, read back off the resolve report.""" + out = {} + for bucket in report.values(): + for name, mac in bucket.items(): + out[mac] = name + return out + + +class TestDeviceMatches(unittest.TestCase): + """A stored key names the property it was taken from, so matching a + device is comparing that one property - never a guess about which + identifier was meant.""" + + def test_matches_the_named_property(self): + self.assertTrue(device_matches({'ID_NET_NAME_SLOT': 'ens18'}, + 'ID_NET_NAME_SLOT=ens18')) + + def test_same_value_under_a_different_property_does_not_match(self): + self.assertFalse(device_matches({'ID_NET_NAME_PATH': 'ens18'}, + 'ID_NET_NAME_SLOT=ens18')) + + def test_device_without_the_property_does_not_match(self): + self.assertFalse(device_matches({'ID_PATH': 'pci-0000:00:12.0'}, + 'ID_NET_NAME_SLOT=ens18')) + + def test_empty_or_malformed_key_never_matches(self): + self.assertFalse(device_matches({'ID_NET_NAME_SLOT': 'ens18'}, '')) + self.assertFalse(device_matches({'ID_NET_NAME_SLOT': 'ens18'}, 'ens18')) + + +class TestResolveBasics(unittest.TestCase): + def test_first_boot_bootstraps_in_slot_order(self): + devices = [ + dev('eth0', 'pci-0000:00:15.0', '00:00:5e:00:53:03'), + dev('eth1', 'pci-0000:00:12.0', '00:00:5e:00:53:00'), + dev('eth2', 'pci-0000:00:13.0', '00:00:5e:00:53:01'), + ] + _, new_store, _ = resolve(devices, empty_store()) + self.assertEqual(new_store['interfaces']['eth0'], 'ID_PATH=pci-0000:00:12.0') + self.assertEqual(new_store['interfaces']['eth1'], 'ID_PATH=pci-0000:00:13.0') + self.assertEqual(new_store['interfaces']['eth2'], 'ID_PATH=pci-0000:00:15.0') + + def test_established_names_are_stable_regardless_of_probe_order(self): + store = store_of({'eth0': 'pci-0000:00:12.0', 'eth1': 'pci-0000:00:13.0'}) + # drivers finished in the opposite order this boot + devices = [ + dev('eth0', 'pci-0000:00:13.0', '00:00:5e:00:53:01'), + dev('eth1', 'pci-0000:00:12.0', '00:00:5e:00:53:00'), + ] + plan, _, _ = resolve(devices, store) + self.assertEqual(plan, {'eth0': 'eth1', 'eth1': 'eth0'}) + + def test_replacement_nic_in_same_slot_inherits_name(self): + # the slot is the identity, so this needs no special rule: whatever + # card is in eth1's slot is eth1, along with its configuration + store = store_of({'eth1': 'pci-0000:00:13.0'}) + devices = [dev('eth0', 'pci-0000:00:13.0', 'bb:bb:bb:bb:bb:bb')] + plan, new_store, report = resolve(devices, store) + self.assertEqual(plan, {'eth0': 'eth1'}) + self.assertEqual(report['matched'], {'eth1': 'bb:bb:bb:bb:bb:bb'}) + self.assertEqual(new_store['interfaces']['eth1'], 'ID_PATH=pci-0000:00:13.0') + + def test_a_card_moved_to_another_slot_takes_that_slot_s_name(self): + # the consequence of slot identity, stated plainly: move a card and it + # becomes whatever the new slot is called, not what it used to be + store = store_of({'eth1': 'pci-0000:00:13.0', 'eth2': 'pci-0000:00:14.0'}) + devices = [dev('eth9', 'pci-0000:00:14.0', 'aa:aa:aa:aa:aa:aa')] + plan, _, report = resolve(devices, store) + self.assertEqual(plan, {'eth9': 'eth2'}) + self.assertEqual(final_names(report)['aa:aa:aa:aa:aa:aa'], 'eth2') + + def test_absent_hardware_keeps_its_name_reserved(self): + store = store_of({'eth0': 'pci-0000:00:12.0', 'eth1': 'pci-0000:00:13.0'}) + # eth0's slot is empty; a brand new card appeared elsewhere + devices = [ + dev('eth0', 'pci-0000:00:13.0', '00:00:5e:00:53:01'), + dev('eth1', 'pci-0000:00:1c.0', 'cc:cc:cc:cc:cc:cc'), + ] + _, new_store, report = resolve(devices, store) + # the new card must not inherit eth0's name, and with it eth0's config + self.assertNotEqual(final_names(report)['cc:cc:cc:cc:cc:cc'], 'eth0') + self.assertEqual(new_store['interfaces']['eth0'], 'ID_PATH=pci-0000:00:12.0') + + def test_hardware_with_no_usable_path_falls_back_to_its_mac(self): + # some paravirtual buses give udev nothing to build a path from; the + # MAC-derived name is the last step of the key chain so the interface + # is still stable instead of being renamed every boot + properties = {'ID_NET_NAME_MAC': 'enxaabbccddeeff'} + device = {'name': 'eth0', 'mac': 'aa:bb:cc:dd:ee:ff', 'wireless': False, + 'properties': properties, 'key': hardware_key(properties)} + _, new_store, _ = resolve([device], empty_store()) + self.assertEqual(new_store['interfaces']['eth0'], + 'ID_NET_NAME_MAC=enxaabbccddeeff') + + def test_a_path_two_devices_share_is_not_used_to_identify_either(self): + # a modem can put several interfaces on one parent device, and they + # then report the same path. Keying both on it would give them one + # name between them, so fall through to something which tells them + # apart. + first = {'ID_PATH': 'pci-0000:00:12.0', + 'ID_NET_NAME_MAC': 'enxaabbccddee01'} + second = {'ID_PATH': 'pci-0000:00:12.0', + 'ID_NET_NAME_MAC': 'enxaabbccddee02'} + ambiguous = ambiguous_keys([first, second]) + self.assertEqual(hardware_key(first, ambiguous), + 'ID_NET_NAME_MAC=enxaabbccddee01') + self.assertEqual(hardware_key(second, ambiguous), + 'ID_NET_NAME_MAC=enxaabbccddee02') + + def test_a_path_only_one_device_has_is_still_used(self): + first = {'ID_PATH': 'pci-0000:00:12.0'} + second = {'ID_PATH': 'pci-0000:00:13.0'} + ambiguous = ambiguous_keys([first, second]) + self.assertEqual(hardware_key(first, ambiguous), + 'ID_PATH=pci-0000:00:12.0') + + def test_firmware_slot_is_preferred_over_a_bare_pci_path(self): + # a firmware slot index survives PCI renumbering, a bus address does not + properties = {'ID_NET_NAME_SLOT': 'ens18', + 'ID_NET_NAME_PATH': 'enp0s18', + 'ID_PATH': 'pci-0000:00:12.0'} + self.assertEqual(hardware_key(properties), 'ID_NET_NAME_SLOT=ens18') + + def test_wireless_uses_its_own_namespace(self): + devices = [dev('eth0', 'pci-0000:00:12.0', 'aa:aa:aa:aa:aa:aa'), + dev('wlan5', 'pci-0000:00:14.0', 'cc:cc:cc:cc:cc:cc', + wireless=True)] + _, new_store, _ = resolve(devices, empty_store()) + self.assertIn('eth0', new_store['interfaces']) + self.assertIn('wlan0', new_store['interfaces']) + + +class TestPR5417Regressions(unittest.TestCase): + """Each test replays a reproduction from the review of PR #5417, where the + config-tree-driven resolver put a configured address on the wrong wire. + The assertions are about physical correctness - which MAC ends up behind + which name - not about log output. + """ + + def test_multi_nic_cloud_init_first_boot(self): + """2026-08-25 #1: three NICs, cloud-init synthesized a hw-id for eth0 + only. eth1/eth2 were pushed to eth3/eth4 and both data addresses were + unreachable, persisting across reboot. + """ + devices = [ + dev('eth0', 'pci-0000:00:03.0', '52:54:00:00:00:10'), + dev('eth1', 'pci-0000:00:04.0', '52:54:00:ff:00:21'), + dev('eth2', 'pci-0000:00:05.0', '52:54:00:00:00:22'), + ] + _, new_store, report = resolve(devices, empty_store()) + names = final_names(report) + self.assertEqual(names['52:54:00:00:00:10'], 'eth0') + self.assertEqual(names['52:54:00:ff:00:21'], 'eth1') + self.assertEqual(names['52:54:00:00:00:22'], 'eth2') + self.assertEqual(set(new_store['interfaces']), {'eth0', 'eth1', 'eth2'}) + + def test_names_follow_slots_not_mac_magnitude(self): + """2026-08-25 #2: the resolver swapped eth1/eth2 by MAC order, so each + address landed on the other's physical network, and the swap was + persisted with no warning. eth1 sits in the lower slot but has the + numerically HIGHER MAC. + """ + devices = [ + dev('eth1', 'pci-0000:00:04.0', '52:54:00:ff:00:21'), + dev('eth2', 'pci-0000:00:05.0', '52:54:00:00:00:22'), + ] + _, _, report = resolve(devices, empty_store()) + names = final_names(report) + self.assertEqual(names['52:54:00:ff:00:21'], 'eth0') + self.assertEqual(names['52:54:00:00:00:22'], 'eth1') + + def test_box_named_out_of_slot_order_is_not_reordered(self): + """2026-09-02 / 2026-09-09: a box first named by an earlier release in + MAC order - eth1 on the slot-5 NIC, eth4 on the slot-2 NIC. Deleting + hw-id entries made the resolver re-derive names in PCI slot order and + put eth1 and eth4 on each other's cable, silently and permanently. + + The store records the slot each name was actually on, so nothing is + ever re-derived and the box keeps the names it had. + """ + store = store_of({ + 'eth0': 'pci-0000:00:01.0', + 'eth1': 'pci-0000:00:05.0', + 'eth2': 'pci-0000:00:03.0', + 'eth3': 'pci-0000:00:04.0', + 'eth4': 'pci-0000:00:02.0', + }) + devices = [ + dev('eth0', 'pci-0000:00:01.0', '00:00:5e:00:54:01'), + dev('eth1', 'pci-0000:00:05.0', '00:00:5e:00:54:12'), + dev('eth2', 'pci-0000:00:03.0', '00:00:5e:00:54:13'), + dev('eth3', 'pci-0000:00:04.0', '00:00:5e:00:54:14'), + dev('eth4', 'pci-0000:00:02.0', '00:00:5e:00:54:19'), + ] + plan, _, report = resolve(devices, store) + self.assertEqual(plan, {}, 'nothing may be renamed') + names = final_names(report) + self.assertEqual(names['00:00:5e:00:54:12'], 'eth1') + self.assertEqual(names['00:00:5e:00:54:19'], 'eth4') + + def test_spare_nic_does_not_displace_configured_interfaces(self): + """2026-09-02 #2: a fourth, unconfigured NIC was enough to make the + three configured interfaces fail to get their hardware - eth1/eth2 + stayed bare while eth3/eth4/eth5 appeared holding the NICs. + """ + devices = [ + dev('eth0', 'pci-0000:00:03.0', '00:00:5e:00:54:31'), + dev('eth1', 'pci-0000:00:04.0', '00:00:5e:00:54:39'), + dev('eth2', 'pci-0000:00:05.0', '00:00:5e:00:54:32'), + dev('eth3', 'pci-0000:00:06.0', '00:00:5e:00:54:3c'), + ] + _, new_store, report = resolve(devices, empty_store()) + self.assertEqual(set(new_store['interfaces']), + {'eth0', 'eth1', 'eth2', 'eth3'}) + names = final_names(report) + self.assertEqual(names['00:00:5e:00:54:31'], 'eth0') + self.assertEqual(names['00:00:5e:00:54:39'], 'eth1') + self.assertEqual(names['00:00:5e:00:54:32'], 'eth2') + + def test_config_node_without_hardware_does_not_block_the_others(self): + """2026-09-09: eth0/eth1/eth2 configured with no hw-id on a two-NIC + VM. Every interface was left pending, both NICs parked on eth3/eth4, + and the box was unreachable from the network. + """ + devices = [ + dev('eth0', 'pci-0000:00:03.0', '00:00:5e:00:54:a1'), + dev('eth1', 'pci-0000:00:04.0', '00:00:5e:00:54:a9'), + ] + plan, new_store, report = resolve(devices, empty_store()) + # the two NICs present take eth0 and eth1; a configured eth2 node + # simply has no hardware, which is a reporting matter not a naming one + self.assertEqual(plan, {}) + names = final_names(report) + self.assertEqual(names['00:00:5e:00:54:a1'], 'eth0') + self.assertEqual(names['00:00:5e:00:54:a9'], 'eth1') + self.assertNotIn('eth3', new_store['interfaces']) + + +class TestLinkFiles(unittest.TestCase): + """The .link layer is what removes the rename window: udev applies the + name as the device is added, so no probe-order name is ever observable. + """ + + def test_matches_the_same_property_the_store_keyed_on(self): + # udev must apply exactly the mapping recorded in the store - if the + # .link matched on anything else the two could disagree about what + # identifies an interface + body = render_link('eth1', 'ID_NET_NAME_SLOT=ens18') + self.assertIn('Property=ID_NET_NAME_SLOT=ens18', body) + self.assertIn('Name=eth1', body) + self.assertIn('Type=ether', body) + + def test_link_for_a_radio_matches_the_wireless_type(self): + # a .link naming a type the device is not never applies, so a radio + # rendered as 'ether' would silently keep its probe-order name + body = render_link('wlan0', 'ID_PATH=pci-0000:00:14.0') + self.assertIn('Type=wlan', body) + self.assertNotIn('Type=ether', body) + self.assertIn('Name=wlan0', body) + + def test_radio_named_by_resolve_gets_a_wireless_link(self): + # end to end: resolve() names by type, and the file written for that + # name has to agree with it + devices = [dev('wlan5', 'pci-0000:00:14.0', 'aa:aa:aa:aa:aa:01', + wireless=True), + dev('eth9', 'pci-0000:00:12.0', 'bb:bb:bb:bb:bb:02')] + _, store, _ = resolve(devices, empty_store()) + self.assertIn('wlan0', store['interfaces']) + with tempfile.TemporaryDirectory() as d: + written = sync_link_files(store, Path(d)) + bodies = {p.name: p.read_text() for p in written} + self.assertIn('Type=wlan', bodies['10-vyos-wlan0.link']) + self.assertIn('Type=ether', bodies['10-vyos-eth0.link']) + + def test_one_file_per_interface(self): + with tempfile.TemporaryDirectory() as d: + written = sync_link_files( + store_of({'eth1': 'pci-0000:00:13.0'}), Path(d)) + self.assertEqual([p.name for p in written], ['10-vyos-eth1.link']) + + def test_stale_files_are_removed(self): + with tempfile.TemporaryDirectory() as d: + link_dir = Path(d) + sync_link_files(store_of({'eth1': 'p1', + 'eth2': 'p2'}), link_dir) + sync_link_files(store_of({'eth1': 'p1'}), link_dir) + remaining = sorted(p.name for p in link_dir.glob('*.link')) + self.assertEqual(remaining, ['10-vyos-eth1.link']) + + def test_operator_files_are_left_alone(self): + with tempfile.TemporaryDirectory() as d: + link_dir = Path(d) + custom = link_dir / '99-my-tuning.link' + custom.write_text('[Match]\nType=ether\n\n[Link]\nMTUBytes=9000\n') + sync_link_files(store_of({'eth1': 'p1'}), link_dir) + self.assertTrue(custom.exists()) + + +class TestPermanentMac(unittest.TestCase): + """Reading the hardware's own address matters wherever a custom MAC may + be in effect: the current address is then the custom one. This used to + come from the hw-id node, which could also simply be stale. + """ + + def test_ethtool_permanent_address(self): + with mock.patch('vyos.ifconfig.ifname_store.rc_cmd', + return_value=(0, 'Permanent address: aa:bb:cc:dd:ee:ff\n')): + self.assertEqual(permanent_mac('eth0'), 'aa:bb:cc:dd:ee:ff') + + def test_zero_address_falls_back_to_sysfs(self): + # some drivers report an all-zero "unsupported" permanent address + # instead of failing outright + with mock.patch('vyos.ifconfig.ifname_store.rc_cmd', + return_value=(0, 'Permanent address: 00:00:00:00:00:00\n')), \ + mock.patch('pathlib.Path.read_text', return_value='11:22:33:44:55:66\n'): + self.assertEqual(permanent_mac('eth0'), '11:22:33:44:55:66') + + def test_ethtool_unsupported_falls_back_to_sysfs(self): + with mock.patch('vyos.ifconfig.ifname_store.rc_cmd', + return_value=(1, 'Operation not supported')), \ + mock.patch('pathlib.Path.read_text', return_value='11:22:33:44:55:66\n'): + self.assertEqual(permanent_mac('eth0'), '11:22:33:44:55:66') + + def test_no_mac_available_returns_empty(self): + with mock.patch('vyos.ifconfig.ifname_store.rc_cmd', return_value=(1, '')), \ + mock.patch('pathlib.Path.read_text', side_effect=OSError): + self.assertEqual(permanent_mac('eth0'), '') + + +class TestDiscoverDevices(unittest.TestCase): + """Only real, independently-addressable hardware is a naming candidate. + Virtual interfaces have no bus device; enslaved interfaces, SR-IOV VFs + and hypervisor VF datapaths are acceleration children that share another + interface's MAC, so naming them would invent a second interface for one + physical port. + """ + + def setUp(self): + self.tmp = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, self.tmp, ignore_errors=True) + + for patch in (mock.patch('vyos.ifconfig.ifname_store.read_properties', + return_value={}), + mock.patch('vyos.ifconfig.ifname_store.rc_cmd', + return_value=(1, ''))): + patch.start() + self.addCleanup(patch.stop) + + def make(name, mac=None, has_device=True, enslaved=False, is_vf=False): + path = os.path.join(self.tmp, name) + os.mkdir(path) + if has_device: + os.mkdir(os.path.join(path, 'device')) + if is_vf: + pf = os.path.join(self.tmp, f'{name}_pf') + os.mkdir(pf) + os.symlink(pf, os.path.join(path, 'device', 'physfn')) + if enslaved: + master = os.path.join(self.tmp, f'{name}_master') + os.mkdir(master) + os.symlink(master, os.path.join(path, 'master')) + if mac: + with open(os.path.join(path, 'address'), 'w') as f: + f.write(mac + '\n') + + make('eth0', mac='aa:aa:aa:aa:aa:00') + make('eth1', mac='aa:aa:aa:aa:aa:01') + make('br0', mac='aa:aa:aa:aa:aa:99', has_device=False) + make('lo', has_device=False) + make('eth8', mac='aa:aa:aa:aa:aa:08', enslaved=True) + make('eth9', mac='aa:aa:aa:aa:aa:09', is_vf=True) + make('vf_eth0', mac='aa:aa:aa:aa:aa:00') + + def _names(self): + return {d['name'] for d in discover_devices(self.tmp)} + + def test_only_physical_independent_interfaces(self): + self.assertEqual(self._names(), {'eth0', 'eth1'}) + + def test_virtual_interfaces_excluded(self): + self.assertNotIn('br0', self._names()) + self.assertNotIn('lo', self._names()) + + def test_enslaved_interface_excluded(self): + self.assertNotIn('eth8', self._names()) + + def test_sriov_virtual_function_excluded(self): + # the general marker, independent of hypervisor - the previous + # exclusion relied on udev rules scoped to a Microsoft DMI vendor + # string and would miss a bare-metal Mellanox VF + self.assertNotIn('eth9', self._names()) + + def test_hypervisor_vf_datapath_excluded(self): + # parked as vf_* by 63-hyperv-vf-net.rules; shares its synthetic + # parent's MAC and must not be taken for a second interface + self.assertNotIn('vf_eth0', self._names()) -- cgit v1.2.3 From 0b9903a85ccbef75b653a4d4349db5710ee9638d Mon Sep 17 00:00:00 2001 From: Christian Breunig Date: Wed, 7 Oct 2026 19:56:47 +0200 Subject: interfaces: T3871: persist interface names on the first boot of a pre-built image The check guarding that write looked for a marker which an activation script creates only after interface naming has already run. On a deployed qcow2 the marker is absent on the first boot, so nothing was recorded and the next boot named the hardware from scratch. Removing a card then moved every name below it onto the neighbouring port, taking that interface's address with it. Only an encrypted configuration has a volume which may still be locked, so ask about that instead - everywhere else the directory is already the persistent one. --- python/vyos/ifconfig/ifname_store.py | 44 +++++++++++++++++++++++++++++++----- src/tests/test_ifname_store.py | 35 +++++++++++++++++++++++----- 2 files changed, 67 insertions(+), 12 deletions(-) (limited to 'src/tests/test_ifname_store.py') diff --git a/python/vyos/ifconfig/ifname_store.py b/python/vyos/ifconfig/ifname_store.py index e6a0876fd..9c00810db 100644 --- a/python/vyos/ifconfig/ifname_store.py +++ b/python/vyos/ifconfig/ifname_store.py @@ -218,15 +218,47 @@ def load_store(path: Path = None) -> dict: 'hardware': hardware} -def config_dir_is_mounted(path: Path = None) -> bool: - """Is the config directory the real one, or a bare rootfs overlay? +def encrypted_config_volume() -> bool: + """Does this image keep its configuration on a LUKS volume? + + Mirrors mount_encrypted_config() in src/init/vyos-router - a container + named after the running image, below the persistence path. Anything that + cannot be determined (no persistence, no version file, not a VyOS system + at all) means there is no volume waiting to be mounted over the config + directory. + """ + try: + from vyos.system.image import get_running_image + + code, out = rc_cmd('/opt/vyatta/sbin/vyos-persistpath') + if code != 0 or not out.strip(): + return False + image = get_running_image() + return bool(image) and (Path(out.strip()) / 'luks' / image).is_file() + except Exception: + return False - A failed TPM/LUKS unlock does not stop the boot. Writing to the overlay - underneath would be masked once the real volume mounts, leaving two - stores which silently diverge. + +def config_dir_is_mounted(path: Path = None) -> bool: + """Will a store written here still be there on the next boot? + + Only an encrypted configuration has a volume which may still be locked: + a failed TPM/LUKS unlock does not stop the boot, and writing to the + directory underneath would be masked once the real volume mounts, + leaving two stores which silently diverge. Everywhere else the directory + is already the persistent one. + + Deliberately not keyed on '.vyatta_config'. That marker is an + install-time hint, written by an activation script which runs *after* + interface naming - so on the first boot of a pre-built image it does not + exist yet, no store is ever written, and the next boot names the + hardware from scratch. Removing a card then shifts every name below it + onto the wrong port (T3871). """ path = path or store_path - return (path.parent / '.vyatta_config').exists() + if not encrypted_config_volume(): + return True + return os.path.ismount(path.parent) def save_store(store: dict, path: Path = None) -> bool: diff --git a/src/tests/test_ifname_store.py b/src/tests/test_ifname_store.py index 3f12594d4..7f6aa7852 100644 --- a/src/tests/test_ifname_store.py +++ b/src/tests/test_ifname_store.py @@ -29,6 +29,7 @@ from unittest import mock from vyos.ifconfig.ifname_store import STORE_VERSION from vyos.ifconfig.ifname_store import canonical_sort_key +from vyos.ifconfig.ifname_store import config_dir_is_mounted from vyos.ifconfig.ifname_store import device_matches from vyos.ifconfig.ifname_store import ambiguous_keys from vyos.ifconfig.ifname_store import hardware_key @@ -136,8 +137,7 @@ class TestReplacedHardware(unittest.TestCase): class TestStoreIO(unittest.TestCase): @staticmethod def _config_dir(d): - """A directory that looks like a properly mounted config volume.""" - (Path(d) / '.vyatta_config').touch() + """An ordinary, unencrypted config directory - the common case.""" return Path(d) / 'interface-mapping.json' def test_roundtrip(self): @@ -147,14 +147,37 @@ class TestStoreIO(unittest.TestCase): self.assertTrue(save_store(s, p)) self.assertEqual(load_store(p), s) - def test_refuses_to_write_when_config_volume_is_not_mounted(self): + def test_refuses_to_write_when_the_encrypted_volume_is_not_mounted(self): # TPM/LUKS unlock failed: vyos-router continues, but persisting here # would write a shadow store that the real volume masks later - with tempfile.TemporaryDirectory() as d: - p = Path(d) / 'interface-mapping.json' # no .vyatta_config marker + with tempfile.TemporaryDirectory() as d, \ + mock.patch('vyos.ifconfig.ifname_store.encrypted_config_volume', + return_value=True): + p = Path(d) / 'interface-mapping.json' # a plain dir, not a mount self.assertFalse(save_store(store_of({'eth0': 'x'}), p)) self.assertFalse(p.exists()) + def test_writes_on_the_first_boot_of_a_prebuilt_image(self): + # T3871: the check used to key on '.vyatta_config', which an + # activation script writes only AFTER interface naming has run. On a + # qcow2 that marker is absent on the first boot, so no store was ever + # written and the next boot named the hardware from scratch - pull a + # card and every name below it moved onto the wrong port. + with tempfile.TemporaryDirectory() as d: + p = Path(d) / 'interface-mapping.json' + self.assertFalse((Path(d) / '.vyatta_config').exists()) + self.assertTrue(save_store(store_of({'eth0': 'pci-0000:00:12.0'}), p)) + self.assertTrue(p.exists()) + + def test_unencrypted_config_needs_no_mount_point(self): + # the config directory of an ordinary install is not a mount of its + # own - only an encrypted one is + with tempfile.TemporaryDirectory() as d, \ + mock.patch('vyos.ifconfig.ifname_store.encrypted_config_volume', + return_value=False): + self.assertTrue(config_dir_is_mounted( + Path(d) / 'interface-mapping.json')) + def test_missing_file_is_empty_store(self): with tempfile.TemporaryDirectory() as d: self.assertEqual(load_store(Path(d) / 'nope.json'), empty_store()) @@ -170,7 +193,7 @@ class TestStoreIO(unittest.TestCase): p = self._config_dir(d) save_store(store_of({'eth0': 'pci-0000:00:12.0'}), p) self.assertEqual(sorted(f.name for f in Path(d).iterdir()), - ['.vyatta_config', 'interface-mapping.json']) + ['interface-mapping.json']) def final_names(report): -- cgit v1.2.3 From 035d0260f65ad327c5a6a512a37157e96ca50fcf Mon Sep 17 00:00:00 2001 From: Christian Breunig Date: Wed, 7 Oct 2026 19:56:49 +0200 Subject: interfaces: T3871: name a card by the card, not only by the slot it sits in The map now records which card each name sat on. A card moved to another slot keeps its name rather than leaving it behind for whatever turns up next, and the slot it vacated does not lend that name out. Where the map and the hardware disagree, the map wins: a card the store already knows, found in a name's slot, takes that name. Only a card nothing knows about, sitting in a slot on record, is treated as a replacement - the one case which can quietly move a configured address onto another cable. --- python/vyos/ifconfig/ifname_store.py | 51 +++++++++++++++++++---- src/system/vyos-net-name-resolve.py | 8 +++- src/tests/test_ifname_store.py | 80 ++++++++++++++++++++++++++++++++++++ 3 files changed, 130 insertions(+), 9 deletions(-) (limited to 'src/tests/test_ifname_store.py') diff --git a/python/vyos/ifconfig/ifname_store.py b/python/vyos/ifconfig/ifname_store.py index 9c00810db..f2131a55a 100644 --- a/python/vyos/ifconfig/ifname_store.py +++ b/python/vyos/ifconfig/ifname_store.py @@ -314,7 +314,7 @@ def resolve(devices: list, store: dict) -> tuple: seen = store.get('hardware', {}) assigned = {} # final name -> device - report = {'matched': {}, 'bootstrapped': {}} + report = {'matched': {}, 'moved': {}, 'replaced': {}, 'bootstrapped': {}} # A recorded name stays off-limits even while its slot is empty, so new # hardware cannot inherit an absent interface's name and configuration. @@ -326,13 +326,53 @@ def resolve(devices: list, store: dict) -> tuple: report[bucket][name] = device['mac'] remaining.remove(device) - # whatever card occupies the recorded slot is this interface + # the recorded card in its recorded slot. A name with no card on record + # counts too - a store written before addresses were kept has nothing to + # disagree with, and this is as certain as it gets. for name, key in entries.items(): for device in remaining: - if device_matches(device['properties'], key): + if (device_matches(device['properties'], key) + and seen.get(name, device['mac']) == device['mac']): claim(device, name, 'matched') break + # a card the store already knows about, sitting in this name's slot. The + # cards were rearranged among themselves, or the map was edited by hand - + # either way the map is what decides names, so the slot wins and the + # address on record simply follows. + known = {mac for mac in seen.values() if mac} + for name, key in entries.items(): + if name in assigned: + continue + for device in remaining: + if (device_matches(device['properties'], key) + and device['mac'] in known): + claim(device, name, 'matched') + break + + # the card moved, and nothing the store knows took its place. Its address + # identifies it, so the name follows the card rather than being handed to + # a stranger that happens to have landed in the slot it left. + for name in entries: + if name in assigned or not seen.get(name): + continue + for device in remaining: + if device['mac'] == seen[name]: + claim(device, name, 'moved') + break + + # a different card in the recorded slot: an in-place replacement, or an + # unrelated card that happened to land there once the original was pulled. + # Nothing on the device tells those apart, so the slot decides and it is + # reported and warned about rather than taken silently. + for name, key in entries.items(): + if name in assigned: + continue + for device in remaining: + if device_matches(device['properties'], key): + claim(device, name, 'replaced') + break + # unknown hardware: lowest free name of its type, in hardware order for device in sorted(remaining, key=canonical_sort_key): name = _next_free(reserved | set(assigned), _prefix_for(device)) @@ -351,11 +391,6 @@ def resolve(devices: list, store: dict) -> tuple: if device['mac']: new_hardware[name] = device['mac'] - # a slot which kept its name but changed card hands that interface's - # addresses to different hardware - never silently - report['replaced'] = {name: mac for name, mac in report['matched'].items() - if seen.get(name) and seen[name] != mac} - plan = {device['name']: name for name, device in assigned.items() if device['name'] != name} diff --git a/src/system/vyos-net-name-resolve.py b/src/system/vyos-net-name-resolve.py index b7fe052e6..df1f10f72 100755 --- a/src/system/vyos-net-name-resolve.py +++ b/src/system/vyos-net-name-resolve.py @@ -149,7 +149,13 @@ def write_status(store: dict, devices: list, applied: dict, report: dict, if key in absent }, 'renamed': applied, - 'matched': report.get('matched', {}), + # everything recognised from the store, however it was recognised + 'matched': {**report.get('matched', {}), + **report.get('moved', {}), + **report.get('replaced', {})}, + # the card was found in a different slot - certain, it is the same card + 'moved': report.get('moved', {}), + # a different card took the slot - the one case that needs verifying 'matched_by_path': report.get('replaced', {}), 'new_hardware': report.get('bootstrapped', {}), 'persisted': persisted, diff --git a/src/tests/test_ifname_store.py b/src/tests/test_ifname_store.py index 7f6aa7852..2112094a6 100644 --- a/src/tests/test_ifname_store.py +++ b/src/tests/test_ifname_store.py @@ -134,6 +134,86 @@ class TestReplacedHardware(unittest.TestCase): self.assertEqual(new_store['interfaces']['eth0'], f'ID_PATH={self.PATH}') +class TestMovedHardware(unittest.TestCase): + """A card carries its name with it. Matching on the slot alone would give + the name - and the address configured under it - to whatever took the old + position, which is how a card added for another purpose ended up on + another segment's address. + """ + + A = 'aa:aa:aa:aa:aa:01' + B = 'bb:bb:bb:bb:bb:02' + C = 'cc:cc:cc:cc:cc:03' + + def setUp(self): + self.store = store_of({'eth0': 'pci-0000:00:02.0', + 'eth1': 'pci-0000:00:03.0'}, + {'eth0': self.A, 'eth1': self.B}) + + @staticmethod + def _at(slot, mac): + return dev('?', f'pci-0000:00:{slot:02x}.0', mac) + + def test_card_moved_to_another_slot_keeps_its_name(self): + _, store, report = resolve([self._at(4, self.A), self._at(3, self.B)], + self.store) + self.assertEqual(report['moved'], {'eth0': self.A}) + self.assertEqual(report['replaced'], {}) + self.assertEqual(store['interfaces']['eth0'], 'ID_PATH=pci-0000:00:04.0') + + def test_a_new_card_in_the_vacated_slot_does_not_inherit_the_name(self): + # the shape reported from the field, with the original card still in + # the box: it keeps eth0, and the newcomer is named as new hardware + _, store, report = resolve( + [self._at(4, self.A), self._at(3, self.B), self._at(2, self.C)], + self.store) + self.assertEqual(report['moved'], {'eth0': self.A}) + self.assertEqual(report['bootstrapped'], {'eth2': self.C}) + self.assertEqual(report['replaced'], {}) + self.assertEqual(store['interfaces']['eth0'], 'ID_PATH=pci-0000:00:04.0') + + def test_two_known_cards_swapping_slots_stay_with_their_slots(self): + # Both cards are on record, so this is a rearrangement within the set + # the store already describes - and the store is what decides names. + # The same path covers an operator swapping two entries by hand, which + # has to take effect or the map would not be authoritative at all. + _, store, report = resolve([self._at(3, self.A), self._at(2, self.B)], + self.store) + self.assertEqual(report['moved'], {}) + self.assertEqual(report['matched'], {'eth0': self.B, 'eth1': self.A}) + self.assertEqual(store['interfaces']['eth0'], 'ID_PATH=pci-0000:00:02.0') + self.assertEqual(store['hardware']['eth0'], self.B) + + def test_editing_the_slot_map_by_hand_takes_effect(self): + # what `make test-ifname` does: swap the two entries in 'interfaces' + # and leave 'hardware' alone. Following the recorded address here + # would quietly undo the edit. + store = store_of({'eth0': 'pci-0000:00:03.0', + 'eth1': 'pci-0000:00:02.0'}, + {'eth0': self.A, 'eth1': self.B}) + _, _, report = resolve([self._at(2, self.A), self._at(3, self.B)], + store) + self.assertEqual(report['matched'], {'eth0': self.B, 'eth1': self.A}) + self.assertEqual(report['moved'], {}) + + def test_identity_wins_over_the_slot(self): + # both rules could fire: the card that owns the name sits elsewhere + # while another card occupies its recorded slot. The card wins. + _, _, report = resolve( + [self._at(4, self.A), self._at(2, self.C), self._at(3, self.B)], + self.store) + self.assertIn('eth0', report['moved']) + self.assertNotIn('eth0', report['replaced']) + + def test_gone_card_still_falls_back_to_the_slot_and_is_reported(self): + # the original is nowhere to be found, so nothing distinguishes an + # in-place replacement from an unrelated card - the slot decides, loudly + _, _, report = resolve([self._at(2, self.C), self._at(3, self.B)], + self.store) + self.assertEqual(report['replaced'], {'eth0': self.C}) + self.assertEqual(report['moved'], {}) + + class TestStoreIO(unittest.TestCase): @staticmethod def _config_dir(d): -- cgit v1.2.3 From 5d63400b3d5e7a95014577fdb3b18a78ac57cafe Mon Sep 17 00:00:00 2001 From: Christian Breunig Date: Wed, 7 Oct 2026 19:56:54 +0200 Subject: interfaces: T3871: leave wireless names to the configuration Every interface created on a radio carries that phy's address, so they are indistinguishable here. A name recorded against one of them was applied to whichever appeared next: configuring a second interface on a phy renamed it on top of the first, and the smoketests lost the interface they had just asked for. Nothing anchors a radio, and nothing needs to - wireless names come from 'physical-device'. Stop discovering them, stop writing a .link for them, and drop what an earlier version recorded so the entry and its file go on upgrade. --- python/vyos/ifconfig/ifname_store.py | 17 ++++++++++-- src/tests/test_ifname_store.py | 54 +++++++++++++++++++----------------- 2 files changed, 43 insertions(+), 28 deletions(-) (limited to 'src/tests/test_ifname_store.py') diff --git a/python/vyos/ifconfig/ifname_store.py b/python/vyos/ifconfig/ifname_store.py index f2131a55a..2145b8ce0 100644 --- a/python/vyos/ifconfig/ifname_store.py +++ b/python/vyos/ifconfig/ifname_store.py @@ -150,6 +150,13 @@ def discover_devices(sys_class_net: str = '/sys/class/net') -> list: Skips virtual interfaces, enslaved ones, SR-IOV virtual functions and hypervisor VF datapaths - the latter share another interface's MAC. + + Wireless is skipped too, and not because it is uninteresting: a radio's + interfaces are created from the configuration on a given phy, and every + one of them carries that phy's address. They are indistinguishable here, + so a name recorded against one would be applied to whichever appeared + next - renaming an interface the operator had just asked for by name. + Wireless names come from the configuration; nothing to anchor. """ devices = [] net_dir = Path(sys_class_net) @@ -165,6 +172,8 @@ def discover_devices(sys_class_net: str = '/sys/class/net') -> list: continue if entry.name.startswith('vf_'): continue + if is_wireless(entry.name, sys_class_net): + continue mac = permanent_mac(entry.name, sys_class_net) if not mac: @@ -204,7 +213,8 @@ def load_store(path: Path = None) -> dict: return empty_store() interfaces = {name: key for name, key in data['interfaces'].items() - if isinstance(key, str) and key} + if isinstance(key, str) and key + and not name.startswith('wlan')} # the card each name last sat on. Absent from a store written before this # was recorded, which just means the first pass has nothing to compare @@ -431,7 +441,10 @@ def sync_link_files(store: dict, link_dir: Path = None) -> list: written = {} for name, key in store.get('interfaces', {}).items(): - if key: + # never for a radio: the key is the phy's address, which every + # interface created on that phy shares, so the file would rename + # whichever of them udev saw next + if key and not name.startswith('wlan'): written[link_dir / f'{LINK_PREFIX}{name}.link'] = render_link(name, key) for path, body in written.items(): diff --git a/src/tests/test_ifname_store.py b/src/tests/test_ifname_store.py index 2112094a6..720f4b6c8 100644 --- a/src/tests/test_ifname_store.py +++ b/src/tests/test_ifname_store.py @@ -20,6 +20,7 @@ # physical wire. Under the store-based design none of them have a code path # to travel, and these tests exist to keep it that way. +import json import os import shutil import tempfile @@ -400,13 +401,23 @@ class TestResolveBasics(unittest.TestCase): 'ID_PATH': 'pci-0000:00:12.0'} self.assertEqual(hardware_key(properties), 'ID_NET_NAME_SLOT=ens18') - def test_wireless_uses_its_own_namespace(self): - devices = [dev('eth0', 'pci-0000:00:12.0', 'aa:aa:aa:aa:aa:aa'), - dev('wlan5', 'pci-0000:00:14.0', 'cc:cc:cc:cc:cc:cc', - wireless=True)] - _, new_store, _ = resolve(devices, empty_store()) - self.assertIn('eth0', new_store['interfaces']) - self.assertIn('wlan0', new_store['interfaces']) + def test_a_radio_recorded_by_an_older_version_is_dropped(self): + # Wireless is no longer named here: every interface on a phy carries + # that phy's address, so an entry recorded against one of them would + # be applied to whichever appeared next - renaming an interface the + # operator had just created by name. Drop what an older version wrote + # so the entry, and the .link file made from it, go away on upgrade. + with tempfile.TemporaryDirectory() as d: + p = Path(d) / 'interface-mapping.json' + p.write_text(json.dumps({ + 'version': STORE_VERSION, + 'interfaces': {'eth0': 'ID_PATH=pci-0000:00:12.0', + 'wlan0': 'ID_NET_NAME_MAC=wlx020000000000'}, + 'hardware': {'eth0': 'aa:aa:aa:aa:aa:aa', + 'wlan0': '02:00:00:00:00:00'}})) + store = load_store(p) + self.assertEqual(list(store['interfaces']), ['eth0']) + self.assertNotIn('wlan0', store['hardware']) class TestPR5417Regressions(unittest.TestCase): @@ -529,27 +540,18 @@ class TestLinkFiles(unittest.TestCase): self.assertIn('Name=eth1', body) self.assertIn('Type=ether', body) - def test_link_for_a_radio_matches_the_wireless_type(self): - # a .link naming a type the device is not never applies, so a radio - # rendered as 'ether' would silently keep its probe-order name - body = render_link('wlan0', 'ID_PATH=pci-0000:00:14.0') - self.assertIn('Type=wlan', body) - self.assertNotIn('Type=ether', body) - self.assertIn('Name=wlan0', body) - - def test_radio_named_by_resolve_gets_a_wireless_link(self): - # end to end: resolve() names by type, and the file written for that - # name has to agree with it - devices = [dev('wlan5', 'pci-0000:00:14.0', 'aa:aa:aa:aa:aa:01', - wireless=True), - dev('eth9', 'pci-0000:00:12.0', 'bb:bb:bb:bb:bb:02')] - _, store, _ = resolve(devices, empty_store()) - self.assertIn('wlan0', store['interfaces']) + def test_no_link_file_is_written_for_a_radio(self): + # a .link keyed on a phy's address matches every interface created on + # that phy, so writing one renamed the interface the operator had just + # asked for. Wireless is named from the configuration instead. + store = {'version': STORE_VERSION, + 'interfaces': {'eth0': 'ID_PATH=pci-0000:00:12.0', + 'wlan0': 'ID_NET_NAME_MAC=wlx020000000000'}, + 'hardware': {}} with tempfile.TemporaryDirectory() as d: written = sync_link_files(store, Path(d)) - bodies = {p.name: p.read_text() for p in written} - self.assertIn('Type=wlan', bodies['10-vyos-wlan0.link']) - self.assertIn('Type=ether', bodies['10-vyos-eth0.link']) + names = [p.name for p in written] + self.assertEqual(names, ['10-vyos-eth0.link']) def test_one_file_per_interface(self): with tempfile.TemporaryDirectory() as d: -- cgit v1.2.3 From 9d44e93638ccc189c99305b2b423222b2dab2b9c Mon Sep 17 00:00:00 2001 From: Christian Breunig Date: Wed, 7 Oct 2026 19:56:56 +0200 Subject: wwan: T3871: name a modem in the namespace the CLI reads Anything that was not wireless was named ethN, so an LTE module came up as an ethernet port: bound, working and invisible, because 'interfaces wwan' looks at wwanN and nothing else. Give it its own namespace, keyed on DEVTYPE, and drop what an earlier version filed under an ethernet name - that entry still matches the modem by path and would capture it again. --- python/vyos/ifconfig/ifname_store.py | 38 +++++++++++++++++- src/tests/test_ifname_store.py | 74 ++++++++++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+), 1 deletion(-) (limited to 'src/tests/test_ifname_store.py') diff --git a/python/vyos/ifconfig/ifname_store.py b/python/vyos/ifconfig/ifname_store.py index 2145b8ce0..eadd3994e 100644 --- a/python/vyos/ifconfig/ifname_store.py +++ b/python/vyos/ifconfig/ifname_store.py @@ -133,6 +133,18 @@ def is_wireless(ifname: str, sys_class_net: str = '/sys/class/net') -> bool: return (Path(sys_class_net) / ifname / 'phy80211').exists() +def is_wwan(ifname: str, sys_class_net: str = '/sys/class/net') -> bool: + """A modem, which the CLI addresses as wwanN. An ethernet port carries no + DEVTYPE at all, so this is exact - and it is the same property systemd + matches on in a .link file. + """ + try: + uevent = (Path(sys_class_net) / ifname / 'uevent').read_text() + except OSError: + return False + return 'DEVTYPE=wwan' in uevent.split() + + def read_properties(ifname: str) -> dict: """udev properties for an interface.""" try: @@ -183,6 +195,7 @@ def discover_devices(sys_class_net: str = '/sys/class/net') -> list: 'name': entry.name, 'mac': mac, 'wireless': is_wireless(entry.name, sys_class_net), + 'wwan': is_wwan(entry.name, sys_class_net), 'properties': read_properties(entry.name), }) @@ -303,6 +316,8 @@ def save_store(store: dict, path: Path = None) -> bool: def _prefix_for(device: dict) -> str: + if device.get('wwan'): + return 'wwan' return 'wlan' if device.get('wireless') else 'eth' @@ -323,6 +338,20 @@ def resolve(devices: list, store: dict) -> tuple: entries = store.get('interfaces', {}) seen = store.get('hardware', {}) + # a modem an earlier version filed under an ethernet name. The entry still + # matches it by path, so it would be captured again and stay invisible to + #'interfaces wwan' - drop it and let it be named as what it is. The name + # cannot be recognised on its own, and neither can the key: this one reads + # ID_NET_NAME_PATH=wwp..., but a device falling back to ID_PATH reads + # usb-... like anything else on that bus. + misfiled = {name for name, key in entries.items() + if not name.startswith('wwan') + and any(d.get('wwan') and device_matches(d['properties'], key) + for d in devices)} + if misfiled: + entries = {n: k for n, k in entries.items() if n not in misfiled} + seen = {n: m for n, m in seen.items() if n not in misfiled} + assigned = {} # final name -> device report = {'matched': {}, 'moved': {}, 'replaced': {}, 'bootstrapped': {}} @@ -412,6 +441,13 @@ LINK_DIR = Path('/etc/systemd/network') LINK_PREFIX = '10-vyos-' +def _link_type_for(name: str) -> str: + for prefix in ('wlan', 'wwan'): + if name.startswith(prefix): + return prefix + return 'ether' + + def render_link(name: str, key: str) -> str: """Render a systemd .link file body, matching on the stored property. @@ -424,7 +460,7 @@ def render_link(name: str, key: str) -> str: '# Generated by VyOS - do not edit.\n' "# Interface names are managed via 'interface-mapping.json'.\n" '[Match]\n' - f"Type={'wlan' if name.startswith('wlan') else 'ether'}\n" + f'Type={_link_type_for(name)}\n' f'Property={key}\n' '\n' '[Link]\n' diff --git a/src/tests/test_ifname_store.py b/src/tests/test_ifname_store.py index 720f4b6c8..2b31f93ca 100644 --- a/src/tests/test_ifname_store.py +++ b/src/tests/test_ifname_store.py @@ -215,6 +215,80 @@ class TestMovedHardware(unittest.TestCase): self.assertEqual(report['moved'], {}) +class TestWwanNaming(unittest.TestCase): + """A modem is addressed as wwanN by the CLI - 'interfaces wwan' looks at + nothing else. Naming it ethN like any other port left an HP lt4132 bound, + working and completely invisible on a DUT. + """ + + WWP = 'wwp0s20f0u2u4c3' + MODEM_MAC = 'b6:53:dd:66:c2:98' + + def _modem(self, name='eth6'): + properties = {'ID_NET_NAME_PATH': self.WWP, 'ID_BUS': 'usb'} + return {'name': name, 'mac': self.MODEM_MAC, 'wireless': False, + 'wwan': True, 'properties': properties, + 'key': f'ID_NET_NAME_PATH={self.WWP}'} + + def _nic(self, name='eth0', path='enp3s0', mac='00:f0:cb:fe:ba:99'): + properties = {'ID_NET_NAME_PATH': path, 'ID_BUS': 'pci'} + return {'name': name, 'mac': mac, 'wireless': False, 'wwan': False, + 'properties': properties, 'key': f'ID_NET_NAME_PATH={path}'} + + def test_a_modem_is_named_in_its_own_namespace(self): + _, store, _ = resolve([self._nic(), self._modem()], empty_store()) + self.assertEqual(store['interfaces']['wwan0'], + f'ID_NET_NAME_PATH={self.WWP}') + # and it must not have eaten an ethernet number on the way + self.assertEqual(store['interfaces']['eth0'], + 'ID_NET_NAME_PATH=enp3s0') + self.assertNotIn('eth1', store['interfaces']) + + def test_the_link_file_names_the_type_the_device_is(self): + # a .link naming a type the device is not simply never applies + body = render_link('wwan0', f'ID_NET_NAME_PATH={self.WWP}') + self.assertIn('Type=wwan', body) + self.assertNotIn('Type=ether', body) + + def test_a_modem_filed_under_an_ethernet_name_is_reclaimed(self): + # what an earlier version left on the DUT: the entry still matches the + # modem by path, so without dropping it the modem is captured again and + # stays invisible to 'interfaces wwan' + store = {'version': STORE_VERSION, + 'interfaces': {'eth0': 'ID_NET_NAME_PATH=enp3s0', + 'eth6': f'ID_NET_NAME_PATH={self.WWP}'}, + 'hardware': {'eth0': '00:f0:cb:fe:ba:99', + 'eth6': self.MODEM_MAC}} + plan, new_store, _ = resolve([self._nic(), self._modem()], store) + + self.assertNotIn('eth6', new_store['interfaces']) + self.assertNotIn('eth6', new_store['hardware']) + self.assertEqual(new_store['interfaces']['wwan0'], + f'ID_NET_NAME_PATH={self.WWP}') + self.assertEqual(plan, {'eth6': 'wwan0'}) + + def test_the_stale_link_file_goes_with_it(self): + store = {'version': STORE_VERSION, + 'interfaces': {'eth6': f'ID_NET_NAME_PATH={self.WWP}'}, + 'hardware': {'eth6': self.MODEM_MAC}} + _, new_store, _ = resolve([self._modem()], store) + with tempfile.TemporaryDirectory() as d: + (Path(d) / '10-vyos-eth6.link').write_text('left over') + sync_link_files(new_store, Path(d)) + names = sorted(p.name for p in Path(d).iterdir()) + self.assertEqual(names, ['10-vyos-wwan0.link']) + + def test_an_ordinary_port_on_the_usb_bus_is_not_a_modem(self): + # the key alone cannot tell them apart - a device falling back to + # ID_PATH reads usb-... whether it is a modem or not + usb_nic = self._nic('eth1', 'enp0s20f0u3', 'aa:bb:cc:dd:ee:01') + usb_nic['properties'] = {'ID_PATH': 'usb-0:2:1.0', 'ID_BUS': 'usb'} + usb_nic['key'] = 'ID_PATH=usb-0:2:1.0' + _, store, _ = resolve([usb_nic], empty_store()) + self.assertIn('eth0', store['interfaces']) + self.assertNotIn('wwan0', store['interfaces']) + + class TestStoreIO(unittest.TestCase): @staticmethod def _config_dir(d): -- cgit v1.2.3 From ccac4ac565796934fa217dbcb6f470819ffb2d36 Mon Sep 17 00:00:00 2001 From: Christian Breunig Date: Wed, 7 Oct 2026 19:56:58 +0200 Subject: interfaces: T3871: keep bridge and bond members discoverable An interface was passed over as soon as it had a master, which covers a VF datapath but also every bridge, bond and VRF member. Those are still their own card in their own slot, so the naming pass reported them as hardware which had not appeared, on any run made while they were in use. Skip only when the master is a device itself. That is what tells the accelerated path, which belongs to the interface it is enslaved to, apart from a member, which does not. --- python/vyos/ifconfig/ifname_store.py | 10 +++++++--- src/tests/test_ifname_store.py | 28 ++++++++++++++++++++-------- 2 files changed, 27 insertions(+), 11 deletions(-) (limited to 'src/tests/test_ifname_store.py') diff --git a/python/vyos/ifconfig/ifname_store.py b/python/vyos/ifconfig/ifname_store.py index eadd3994e..5f25235ae 100644 --- a/python/vyos/ifconfig/ifname_store.py +++ b/python/vyos/ifconfig/ifname_store.py @@ -160,8 +160,8 @@ def read_properties(ifname: str) -> dict: def discover_devices(sys_class_net: str = '/sys/class/net') -> list: """Every interface backed by independently-addressable hardware. - Skips virtual interfaces, enslaved ones, SR-IOV virtual functions and - hypervisor VF datapaths - the latter share another interface's MAC. + Skips virtual interfaces, SR-IOV virtual functions and hypervisor VF + datapaths - the latter share another interface's MAC. Wireless is skipped too, and not because it is uninteresting: a radio's interfaces are created from the configuration on a given phy, and every @@ -178,7 +178,11 @@ def discover_devices(sys_class_net: str = '/sys/class/net') -> list: for entry in sorted(net_dir.iterdir()): if not (entry / 'device').exists(): continue - if (entry / 'master').exists(): + # a VF datapath is enslaved to the interface it accelerates, which is + # a device in its own right. A bridge, bond or VRF is not, and its + # members stay real hardware while they are in it. + master = entry / 'master' + if master.exists() and (master / 'device').exists(): continue if (entry / 'device' / 'physfn').exists(): continue diff --git a/src/tests/test_ifname_store.py b/src/tests/test_ifname_store.py index 2b31f93ca..664ecf2ce 100644 --- a/src/tests/test_ifname_store.py +++ b/src/tests/test_ifname_store.py @@ -684,10 +684,10 @@ class TestPermanentMac(unittest.TestCase): class TestDiscoverDevices(unittest.TestCase): """Only real, independently-addressable hardware is a naming candidate. - Virtual interfaces have no bus device; enslaved interfaces, SR-IOV VFs - and hypervisor VF datapaths are acceleration children that share another - interface's MAC, so naming them would invent a second interface for one - physical port. + Virtual interfaces have no bus device; SR-IOV VFs and hypervisor VF + datapaths are acceleration children sharing another interface's MAC, so + naming them would invent a second interface for one physical port. Being + a member of a bridge, bond or VRF says nothing about the hardware. """ def setUp(self): @@ -701,7 +701,8 @@ class TestDiscoverDevices(unittest.TestCase): patch.start() self.addCleanup(patch.stop) - def make(name, mac=None, has_device=True, enslaved=False, is_vf=False): + def make(name, mac=None, has_device=True, enslaved=False, is_vf=False, + master_is_device=False): path = os.path.join(self.tmp, name) os.mkdir(path) if has_device: @@ -713,6 +714,8 @@ class TestDiscoverDevices(unittest.TestCase): if enslaved: master = os.path.join(self.tmp, f'{name}_master') os.mkdir(master) + if master_is_device: + os.mkdir(os.path.join(master, 'device')) os.symlink(master, os.path.join(path, 'master')) if mac: with open(os.path.join(path, 'address'), 'w') as f: @@ -723,6 +726,8 @@ class TestDiscoverDevices(unittest.TestCase): make('br0', mac='aa:aa:aa:aa:aa:99', has_device=False) make('lo', has_device=False) make('eth8', mac='aa:aa:aa:aa:aa:08', enslaved=True) + make('eth7', mac='aa:aa:aa:aa:aa:07', enslaved=True, + master_is_device=True) make('eth9', mac='aa:aa:aa:aa:aa:09', is_vf=True) make('vf_eth0', mac='aa:aa:aa:aa:aa:00') @@ -730,14 +735,21 @@ class TestDiscoverDevices(unittest.TestCase): return {d['name'] for d in discover_devices(self.tmp)} def test_only_physical_independent_interfaces(self): - self.assertEqual(self._names(), {'eth0', 'eth1'}) + self.assertEqual(self._names(), {'eth0', 'eth1', 'eth8'}) def test_virtual_interfaces_excluded(self): self.assertNotIn('br0', self._names()) self.assertNotIn('lo', self._names()) - def test_enslaved_interface_excluded(self): - self.assertNotIn('eth8', self._names()) + def test_bridge_or_bond_member_still_discovered(self): + # a member is still its own card, in its own slot. Dropping it here + # would report the hardware as missing for as long as it is in use. + self.assertIn('eth8', self._names()) + + def test_vf_datapath_enslaved_to_its_parent_excluded(self): + # the accelerated path is enslaved to the interface it belongs to, + # which - unlike a bridge or a bond - is a device itself + self.assertNotIn('eth7', self._names()) def test_sriov_virtual_function_excluded(self): # the general marker, independent of hypervisor - the previous -- cgit v1.2.3 From 9f75f7bf6c143ee3ba7522d281f3a930634cbd83 Mon Sep 17 00:00:00 2001 From: Christian Breunig Date: Wed, 7 Oct 2026 19:57:00 +0200 Subject: interfaces: T3871: report a name which changed hands between known cards Cards rearranged among themselves, or a map edited by hand, leave every name matched to a card the store knows - just not the one it had. That was filed as an ordinary match, indistinguishable from a machine where nothing moved, while the addresses configured under those names had quietly changed cable. Report it separately. Which name goes to which card is unchanged, the slot still decides; only the account of what happened is. --- python/vyos/ifconfig/ifname_store.py | 8 +++++--- src/system/vyos-net-name-resolve.py | 3 +++ src/tests/test_ifname_store.py | 6 ++++-- 3 files changed, 12 insertions(+), 5 deletions(-) (limited to 'src/tests/test_ifname_store.py') diff --git a/python/vyos/ifconfig/ifname_store.py b/python/vyos/ifconfig/ifname_store.py index 5f25235ae..d30d39a7f 100644 --- a/python/vyos/ifconfig/ifname_store.py +++ b/python/vyos/ifconfig/ifname_store.py @@ -357,7 +357,8 @@ def resolve(devices: list, store: dict) -> tuple: seen = {n: m for n, m in seen.items() if n not in misfiled} assigned = {} # final name -> device - report = {'matched': {}, 'moved': {}, 'replaced': {}, 'bootstrapped': {}} + report = {'matched': {}, 'reassigned': {}, 'moved': {}, + 'replaced': {}, 'bootstrapped': {}} # A recorded name stays off-limits even while its slot is empty, so new # hardware cannot inherit an absent interface's name and configuration. @@ -382,7 +383,8 @@ def resolve(devices: list, store: dict) -> tuple: # a card the store already knows about, sitting in this name's slot. The # cards were rearranged among themselves, or the map was edited by hand - # either way the map is what decides names, so the slot wins and the - # address on record simply follows. + # address on record simply follows. The name changes hands, so it is + # reported apart from a card which simply stayed where it was. known = {mac for mac in seen.values() if mac} for name, key in entries.items(): if name in assigned: @@ -390,7 +392,7 @@ def resolve(devices: list, store: dict) -> tuple: for device in remaining: if (device_matches(device['properties'], key) and device['mac'] in known): - claim(device, name, 'matched') + claim(device, name, 'reassigned') break # the card moved, and nothing the store knows took its place. Its address diff --git a/src/system/vyos-net-name-resolve.py b/src/system/vyos-net-name-resolve.py index aa0e74c74..43d2ed8d2 100755 --- a/src/system/vyos-net-name-resolve.py +++ b/src/system/vyos-net-name-resolve.py @@ -151,8 +151,11 @@ def write_status(store: dict, devices: list, applied: dict, report: dict, 'renamed': applied, # everything recognised from the store, however it was recognised 'matched': {**report.get('matched', {}), + **report.get('reassigned', {}), **report.get('moved', {}), **report.get('replaced', {})}, + # the name changed hands between cards the store already knows + 'reassigned': report.get('reassigned', {}), # the card was found in a different slot - certain, it is the same card 'moved': report.get('moved', {}), # a different card took the slot - the one case that needs verifying diff --git a/src/tests/test_ifname_store.py b/src/tests/test_ifname_store.py index 664ecf2ce..01a46ab5a 100644 --- a/src/tests/test_ifname_store.py +++ b/src/tests/test_ifname_store.py @@ -181,7 +181,8 @@ class TestMovedHardware(unittest.TestCase): _, store, report = resolve([self._at(3, self.A), self._at(2, self.B)], self.store) self.assertEqual(report['moved'], {}) - self.assertEqual(report['matched'], {'eth0': self.B, 'eth1': self.A}) + self.assertEqual(report['matched'], {}) + self.assertEqual(report['reassigned'], {'eth0': self.B, 'eth1': self.A}) self.assertEqual(store['interfaces']['eth0'], 'ID_PATH=pci-0000:00:02.0') self.assertEqual(store['hardware']['eth0'], self.B) @@ -194,7 +195,8 @@ class TestMovedHardware(unittest.TestCase): {'eth0': self.A, 'eth1': self.B}) _, _, report = resolve([self._at(2, self.A), self._at(3, self.B)], store) - self.assertEqual(report['matched'], {'eth0': self.B, 'eth1': self.A}) + self.assertEqual(report['matched'], {}) + self.assertEqual(report['reassigned'], {'eth0': self.B, 'eth1': self.A}) self.assertEqual(report['moved'], {}) def test_identity_wins_over_the_slot(self): -- cgit v1.2.3 From 710d0a19b25262fe77a6ebd4a62a5c427a1efa6f Mon Sep 17 00:00:00 2001 From: Christian Breunig Date: Wed, 7 Oct 2026 19:57:01 +0200 Subject: interfaces: T3871: only give a name to a device of its own kind A recorded slot was enough to claim a name, whatever turned up in it. Pull a modem and put an ordinary USB NIC in the same port and the NIC was handed wwan0 - a name 'interfaces wwan' reads, so a modem's configuration would be applied to a card which can not carry it. The reverse was already handled. Require the device to belong to the namespace the name is read under. If it does not, the name stays reserved for the hardware that earned it and the newcomer is named as what it is. --- python/vyos/ifconfig/ifname_store.py | 18 ++++++++++++++---- src/tests/test_ifname_store.py | 17 +++++++++++++++++ 2 files changed, 31 insertions(+), 4 deletions(-) (limited to 'src/tests/test_ifname_store.py') diff --git a/python/vyos/ifconfig/ifname_store.py b/python/vyos/ifconfig/ifname_store.py index d30d39a7f..5a0a29398 100644 --- a/python/vyos/ifconfig/ifname_store.py +++ b/python/vyos/ifconfig/ifname_store.py @@ -325,6 +325,13 @@ def _prefix_for(device: dict) -> str: return 'wlan' if device.get('wireless') else 'eth' +def _name_fits(name: str, device: dict) -> bool: + """A name belongs to the namespace the CLI reads it under, so a modem + can not take an ethernet name nor an ethernet card a modem's. + """ + return name.startswith(_prefix_for(device)) + + def _next_free(taken: set, prefix: str) -> str: index = 0 while f'{prefix}{index}' in taken: @@ -376,7 +383,8 @@ def resolve(devices: list, store: dict) -> tuple: for name, key in entries.items(): for device in remaining: if (device_matches(device['properties'], key) - and seen.get(name, device['mac']) == device['mac']): + and seen.get(name, device['mac']) == device['mac'] + and _name_fits(name, device)): claim(device, name, 'matched') break @@ -391,7 +399,8 @@ def resolve(devices: list, store: dict) -> tuple: continue for device in remaining: if (device_matches(device['properties'], key) - and device['mac'] in known): + and device['mac'] in known + and _name_fits(name, device)): claim(device, name, 'reassigned') break @@ -402,7 +411,7 @@ def resolve(devices: list, store: dict) -> tuple: if name in assigned or not seen.get(name): continue for device in remaining: - if device['mac'] == seen[name]: + if device['mac'] == seen[name] and _name_fits(name, device): claim(device, name, 'moved') break @@ -414,7 +423,8 @@ def resolve(devices: list, store: dict) -> tuple: if name in assigned: continue for device in remaining: - if device_matches(device['properties'], key): + if (device_matches(device['properties'], key) + and _name_fits(name, device)): claim(device, name, 'replaced') break diff --git a/src/tests/test_ifname_store.py b/src/tests/test_ifname_store.py index 01a46ab5a..af8f99ad1 100644 --- a/src/tests/test_ifname_store.py +++ b/src/tests/test_ifname_store.py @@ -280,6 +280,23 @@ class TestWwanNaming(unittest.TestCase): names = sorted(p.name for p in Path(d).iterdir()) self.assertEqual(names, ['10-vyos-wwan0.link']) + def test_a_nic_in_the_modem_s_socket_does_not_take_its_name(self): + # the modem is gone and an ordinary USB NIC is in the port it used. + # The slot matches, but wwan0 is a name 'interfaces wwan' reads - give + # it to a NIC and the modem's configuration is applied to a card that + # cannot carry it. Leave the name reserved and name the NIC as a NIC. + store = {'version': STORE_VERSION, + 'interfaces': {'wwan0': f'ID_NET_NAME_PATH={self.WWP}'}, + 'hardware': {'wwan0': self.MODEM_MAC}} + nic = self._nic('eth0', self.WWP, 'aa:bb:cc:dd:ee:07') + _, new_store, report = resolve([nic], store) + + self.assertEqual(report['replaced'], {}) + self.assertEqual(new_store['hardware']['wwan0'], self.MODEM_MAC) + self.assertIn('eth0', new_store['interfaces']) + self.assertEqual(new_store['interfaces']['wwan0'], + f'ID_NET_NAME_PATH={self.WWP}') + def test_an_ordinary_port_on_the_usb_bus_is_not_a_modem(self): # the key alone cannot tell them apart - a device falling back to # ID_PATH reads usb-... whether it is a modem or not -- cgit v1.2.3 From fb0e36092529d4d6947749009c00d7ff74b0c551 Mon Sep 17 00:00:00 2001 From: Christian Breunig Date: Wed, 7 Oct 2026 19:57:08 +0200 Subject: interfaces: T3871: keep the naming tests off the base system Three of the tests which write the interface map went through the check for an encrypted configuration volume, and that check asks the running system where its persistent storage is. A build host answers "nowhere", so the tests passed - but anywhere that does have such a volume the write is refused and all three fail, for a reason none of them is about. Pin the answer. What these tests cover is what the map does with a directory, not where the directory came from. --- src/tests/test_ifname_store.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) (limited to 'src/tests/test_ifname_store.py') diff --git a/src/tests/test_ifname_store.py b/src/tests/test_ifname_store.py index af8f99ad1..51ec7eba5 100644 --- a/src/tests/test_ifname_store.py +++ b/src/tests/test_ifname_store.py @@ -315,7 +315,9 @@ class TestStoreIO(unittest.TestCase): return Path(d) / 'interface-mapping.json' def test_roundtrip(self): - with tempfile.TemporaryDirectory() as d: + with tempfile.TemporaryDirectory() as d, \ + mock.patch('vyos.ifconfig.ifname_store.encrypted_config_volume', + return_value=False): p = self._config_dir(d) s = store_of({'eth0': 'pci-0000:00:12.0'}) self.assertTrue(save_store(s, p)) @@ -337,7 +339,9 @@ class TestStoreIO(unittest.TestCase): # qcow2 that marker is absent on the first boot, so no store was ever # written and the next boot named the hardware from scratch - pull a # card and every name below it moved onto the wrong port. - with tempfile.TemporaryDirectory() as d: + with tempfile.TemporaryDirectory() as d, \ + mock.patch('vyos.ifconfig.ifname_store.encrypted_config_volume', + return_value=False): p = Path(d) / 'interface-mapping.json' self.assertFalse((Path(d) / '.vyatta_config').exists()) self.assertTrue(save_store(store_of({'eth0': 'pci-0000:00:12.0'}), p)) @@ -363,7 +367,9 @@ class TestStoreIO(unittest.TestCase): self.assertEqual(load_store(p), empty_store()) def test_write_is_atomic_no_leftovers(self): - with tempfile.TemporaryDirectory() as d: + with tempfile.TemporaryDirectory() as d, \ + mock.patch('vyos.ifconfig.ifname_store.encrypted_config_volume', + return_value=False): p = self._config_dir(d) save_store(store_of({'eth0': 'pci-0000:00:12.0'}), p) self.assertEqual(sorted(f.name for f in Path(d).iterdir()), -- cgit v1.2.3 From afd4574df20a1cf70607b5e7644773ce336ea095 Mon Sep 17 00:00:00 2001 From: Christian Breunig Date: Wed, 7 Oct 2026 19:57:17 +0200 Subject: interfaces: T3871: keep the hypervisor's adapter order on ESXi ESXi does not lay its adapters out in PCI address order. With four or more it puts the fourth on a root port sorting ahead of the first three, so naming by address made Network adapter 4 eth0 and shifted the rest. On the vmware-vsphere OVA that moved the WAN, because cloud-init takes eth0 from the OVF properties. The firmware port label carries the adapter order, and is what the VMware udev rule this series removed matched on. Order by it, ahead of the address, for hardware the hypervisor presented - recognised by its PCI vendor, or by DMI for a model whose IDs are not known here. A physical server labels its ports too and must not be reordered. The label orders, it never identifies: a relabelled adapter is still the same card. --- python/vyos/ifconfig/ifname_store.py | 60 ++++++++++++- src/tests/test_ifname_store.py | 168 ++++++++++++++++++++++++++++++++++- 2 files changed, 221 insertions(+), 7 deletions(-) (limited to 'src/tests/test_ifname_store.py') diff --git a/python/vyos/ifconfig/ifname_store.py b/python/vyos/ifconfig/ifname_store.py index b0ee3df35..e0a6b4cbe 100644 --- a/python/vyos/ifconfig/ifname_store.py +++ b/python/vyos/ifconfig/ifname_store.py @@ -54,6 +54,13 @@ _BUS_RANK = {'pci': 0, 'vmbus': 1, 'xen': 2, 'usb': 3} _BUS_RANK_UNKNOWN = 90 _PCI_BDF = re.compile(r'([0-9a-f]{4}):([0-9a-f]{2}):([0-9a-f]{2})\.([0-9a-f]+)', re.I) +# ESXi labels its adapters Ethernet0..EthernetN, in "Network adapter N" order +_PORT_LABEL = re.compile(r'^Ethernet(\d+)$') +# sorts after every real label, and must stay an int - canonical_sort_key() +# compares tuples element-wise, so a None here would raise against an int +_LABEL_UNKNOWN = 1 << 30 +_VMWARE_PCI_VENDOR = '0x15ad' +_DMI_SYS_VENDOR = Path('/sys/class/dmi/id/sys_vendor') _MAC_RE = re.compile(r'([0-9a-fA-F]{2}(:[0-9a-fA-F]{2}){5})') @@ -91,18 +98,62 @@ def device_matches(properties: dict, key: str) -> bool: def canonical_sort_key(device: dict) -> tuple: - """Total order by bus class, physical position then MAC, so it never - depends on probe order. Only used for hardware not yet in the store. + """Total order by bus class, the hypervisor's adapter order where it says + so, then physical position and MAC, so it never depends on probe order. + Only used for hardware not yet in the store. """ properties = device.get('properties') or {} path = properties.get('ID_PATH', '') rank = _BUS_RANK.get(properties.get('ID_BUS', ''), _BUS_RANK_UNKNOWN) + # a labelled adapter outranks the PCI address, which on ESXi does not + # follow the order the adapters were added in. Unlabelled hardware sorts + # after it: a passthrough NIC is not part of the hypervisor's own set and + # must not take the first name from it. + label = device.get('label_index', _LABEL_UNKNOWN) + # numeric, so 0000:09:00.0 sorts before 0000:10:00.0 segments = [tuple(int(part, 16) for part in m.groups()) for m in _PCI_BDF.finditer(path)] - return (rank, segments, path, device.get('mac') or '') + return (rank, label, segments, path, device.get('mac') or '') + + +def _sysfs(path: Path) -> str: + try: + return path.read_text().strip() + except OSError: + return '' + + +def is_vmware_nic(ifname: str, sys_class_net: str = '/sys/class/net') -> bool: + """Whether a NIC is one ESXi presents - vmxnet3 carries VMware's PCI + vendor, an emulated e1000 carries it as the subsystem vendor. + + The DMI fallback is not redundant: smbios.reflectHost makes a guest report + the physical host's vendor, and a NIC model whose IDs are not listed here + would otherwise go unrecognised. + """ + device = Path(sys_class_net) / ifname / 'device' + if _VMWARE_PCI_VENDOR in (_sysfs(device / 'vendor'), + _sysfs(device / 'subsystem_vendor')): + return True + return _sysfs(_DMI_SYS_VENDOR) == 'VMware, Inc.' + + +def port_label_index(ifname: str, sys_class_net: str = '/sys/class/net') -> int: + """The hypervisor's adapter number, from the firmware port label. + + ESXi does not lay its adapters out in PCI address order - with four or more + it places the fourth on a root port which sorts ahead of the first three - + so the label is the only thing that gives their intended order. + + Returns _LABEL_UNKNOWN when there is no such label. Physical servers label + ports too, e.g. "NIC1" or "Embedded LOM 1 Port 1", and must not be reordered + by this. + """ + m = _PORT_LABEL.match(_sysfs(Path(sys_class_net) / ifname / 'device' / 'label')) + return int(m.group(1)) if m else _LABEL_UNKNOWN def permanent_mac(ifname: str, sys_class_net: str = '/sys/class/net') -> str: @@ -200,6 +251,9 @@ def discover_devices(sys_class_net: str = '/sys/class/net') -> list: 'mac': mac, 'wireless': is_wireless(entry.name, sys_class_net), 'wwan': is_wwan(entry.name, sys_class_net), + 'label_index': (port_label_index(entry.name, sys_class_net) + if is_vmware_nic(entry.name, sys_class_net) + else _LABEL_UNKNOWN), 'properties': read_properties(entry.name), }) diff --git a/src/tests/test_ifname_store.py b/src/tests/test_ifname_store.py index 51ec7eba5..5b1fec17f 100644 --- a/src/tests/test_ifname_store.py +++ b/src/tests/test_ifname_store.py @@ -37,6 +37,9 @@ from vyos.ifconfig.ifname_store import hardware_key from vyos.ifconfig.ifname_store import discover_devices from vyos.ifconfig.ifname_store import empty_store from vyos.ifconfig.ifname_store import load_store +from vyos.ifconfig.ifname_store import port_label_index +from vyos.ifconfig.ifname_store import is_vmware_nic +from vyos.ifconfig.ifname_store import _LABEL_UNKNOWN from vyos.ifconfig.ifname_store import permanent_mac from vyos.ifconfig.ifname_store import render_link from vyos.ifconfig.ifname_store import resolve @@ -44,14 +47,17 @@ from vyos.ifconfig.ifname_store import sync_link_files from vyos.ifconfig.ifname_store import save_store -def dev(name, path, mac, wireless=False, bus='pci'): +def dev(name, path, mac, wireless=False, bus='pci', label=None): """A device sitting at 'path'. Its key is what udev would report for it - here the raw ID_PATH, which is what a plain virtio or e1000e NIC with no firmware slot index ends up keyed on.""" properties = {'ID_PATH': path, 'ID_BUS': bus} if path else {} - return {'name': name, 'mac': mac, 'wireless': wireless, - 'properties': properties, - 'key': hardware_key(properties)} + device = {'name': name, 'mac': mac, 'wireless': wireless, + 'properties': properties, + 'key': hardware_key(properties)} + if label is not None: + device['label_index'] = label + return device def store_of(mapping, hardware=None): @@ -82,6 +88,91 @@ class TestCanonicalOrder(unittest.TestCase): self.assertLess(canonical_sort_key(low_slot), canonical_sort_key(high_slot)) +class TestHypervisorAdapterOrder(unittest.TestCase): + """ESXi does not lay its adapters out in PCI address order. With four or + more it puts the fourth on a root port which sorts ahead of the first + three, so naming by PCI address alone made Network adapter 4 eth0. On the + OVA that moves the WAN: cloud-init takes eth0 from the OVF properties. + Reported against ESXi 8.0.3, vmx-21. + """ + + # the two interfaces from the report, verbatim + ADAPTER_4 = ('pci-0000:04:00.0', '00:0c:29:60:0e:5c', 3) # Ethernet3 + ADAPTER_1 = ('pci-0000:0b:00.0', '00:0c:29:60:0e:3e', 0) # Ethernet0 + + def test_the_first_adapter_sorts_first_despite_its_pci_address(self): + a4 = dev('eth0', self.ADAPTER_4[0], self.ADAPTER_4[1], label=self.ADAPTER_4[2]) + a1 = dev('eth1', self.ADAPTER_1[0], self.ADAPTER_1[1], label=self.ADAPTER_1[2]) + self.assertEqual(sorted([a4, a1], key=canonical_sort_key)[0]['mac'], + self.ADAPTER_1[1]) + + def test_four_adapters_are_named_in_adapter_order(self): + devices = [dev('?', 'pci-0000:04:00.0', 'aa:03', label=3), + dev('?', 'pci-0000:0b:00.0', 'aa:00', label=0), + dev('?', 'pci-0000:0c:00.0', 'aa:01', label=1), + dev('?', 'pci-0000:0d:00.0', 'aa:02', label=2)] + _, store, report = resolve(devices, empty_store()) + # assert on the hardware, not just that four names exist + self.assertEqual(report['bootstrapped'], + {'eth0': 'aa:00', 'eth1': 'aa:01', + 'eth2': 'aa:02', 'eth3': 'aa:03'}) + self.assertEqual(store['interfaces']['eth3'], 'ID_PATH=pci-0000:04:00.0') + + def test_a_labelled_and_an_unlabelled_device_can_be_compared(self): + # the sentinel has to be an int - sort keys are tuples compared + # element-wise, and None against int raises + labelled = dev('eth0', 'pci-0000:0b:00.0', 'aa:00', label=0) + plain = dev('eth1', 'pci-0000:03:00.0', 'bb:00') + self.assertEqual(sorted([plain, labelled], key=canonical_sort_key)[0]['mac'], + 'aa:00') + + def test_unlabelled_hardware_never_takes_the_first_name(self): + # a passthrough NIC is not in the OVF network mapping + devices = [dev('?', 'pci-0000:03:00.0', 'bb:00'), + dev('?', 'pci-0000:0b:00.0', 'aa:00', label=0)] + _, _, report = resolve(devices, empty_store()) + self.assertEqual(report['bootstrapped']['eth0'], 'aa:00') + + def test_adding_unlabelled_hardware_does_not_reshuffle_the_adapters(self): + adapters = [dev('?', 'pci-0000:0b:00.0', 'aa:00', label=0), + dev('?', 'pci-0000:04:00.0', 'aa:01', label=1)] + plain = dev('?', 'pci-0000:03:00.0', 'bb:00') + before = [d['mac'] for d in sorted(adapters, key=canonical_sort_key)] + after = [d['mac'] for d in sorted(adapters + [plain], key=canonical_sort_key)] + self.assertEqual(after[:len(before)], before) + + def test_the_order_survives_into_the_store(self): + # the names close the gap: two adapters labelled 0 and 3 become + # eth0 and eth1, the lowest free names in adapter order + devices = [dev('enp4s0', 'pci-0000:04:00.0', 'aa:03', label=3), + dev('enp11s0', 'pci-0000:0b:00.0', 'aa:00', label=0)] + plan, store, _ = resolve(devices, empty_store()) + self.assertEqual(plan, {'enp11s0': 'eth0', 'enp4s0': 'eth1'}) + + # the next boot sees them under the names it just gave them + for device in devices: + device['name'] = plan[device['name']] + + plan, _, report = resolve(devices, store) + self.assertEqual(plan, {}) + self.assertEqual(report['bootstrapped'], {}) + + def test_hardware_added_later_takes_the_next_name(self): + devices = [dev('?', 'pci-0000:0b:00.0', 'aa:00', label=0)] + _, store, _ = resolve(devices, empty_store()) + # a second adapter, labelled lower, must not displace the first + devices.append(dev('?', 'pci-0000:04:00.0', 'aa:09', label=9)) + _, store, report = resolve(devices, store) + self.assertEqual(report['bootstrapped'], {'eth1': 'aa:09'}) + + def test_the_label_is_ordering_only_never_identity(self): + # it must not reach hardware_key(), or a relabelled adapter would read + # as different hardware + properties = {'ID_PATH': 'pci-0000:0b:00.0', 'ID_BUS': 'pci'} + self.assertNotIn('Ethernet', hardware_key(properties)) + self.assertNotIn('label', hardware_key(properties)) + + class TestReplacedHardware(unittest.TestCase): """A slot keeps its name when its card is swapped - which hands that interface's addresses to different hardware. resolve() reports the change @@ -676,6 +767,75 @@ class TestLinkFiles(unittest.TestCase): self.assertTrue(custom.exists()) +class TestPortLabelGate(unittest.TestCase): + """The adapter label only orders hardware the hypervisor presented. + Physical servers label their ports too - "NIC1", "Embedded LOM 1 Port 1" - + and reordering those would be the same bug on bare metal. + """ + + def setUp(self): + self.tmp = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, self.tmp, ignore_errors=True) + self.dmi = os.path.join(self.tmp, 'sys_vendor') + patch = mock.patch('vyos.ifconfig.ifname_store._DMI_SYS_VENDOR', + Path(self.dmi)) + patch.start() + self.addCleanup(patch.stop) + self._dmi('Dell Inc.') + + def _dmi(self, vendor): + with open(self.dmi, 'w') as f: + f.write(vendor + '\n') + + def _make(self, name, label=None, vendor=None, subsystem_vendor=None): + device = os.path.join(self.tmp, name, 'device') + os.makedirs(device) + for attr, value in (('label', label), ('vendor', vendor), + ('subsystem_vendor', subsystem_vendor)): + if value is not None: + with open(os.path.join(device, attr), 'w') as f: + f.write(value + '\n') + return name + + def test_label_index_is_parsed(self): + self._make('eth0', label='Ethernet0') + self._make('eth1', label='Ethernet10') + self.assertEqual(port_label_index('eth0', self.tmp), 0) + # not lexical - Ethernet10 must sort after Ethernet9 + self.assertEqual(port_label_index('eth1', self.tmp), 10) + + def test_labels_physical_servers_use_are_not_adapter_numbers(self): + for i, label in enumerate(['NIC1', 'Integrated NIC 1', 'LAN1', + 'Embedded LOM 1 Port 1', + 'PCIe Slot 1 Port 1', 'Ethernet', + 'Ethernet0 Port 2', '']): + name = self._make(f'eth{i}', label=label) + self.assertEqual(port_label_index(name, self.tmp), _LABEL_UNKNOWN, + f'{label!r} was taken for an adapter number') + + def test_no_label_at_all(self): + self._make('eth0') + self.assertEqual(port_label_index('eth0', self.tmp), _LABEL_UNKNOWN) + + def test_a_vmware_nic_is_recognised_by_its_pci_vendor_alone(self): + # DMI says Dell - smbios.reflectHost does exactly this, and a gate on + # DMI alone would switch itself off + self._make('eth0', vendor='0x15ad') # vmxnet3 + self._make('eth1', subsystem_vendor='0x15ad') # emulated e1000 + self.assertTrue(is_vmware_nic('eth0', self.tmp)) + self.assertTrue(is_vmware_nic('eth1', self.tmp)) + + def test_dmi_still_catches_a_model_whose_pci_ids_are_unknown(self): + self._make('eth0', vendor='0x1af4') + self._dmi('VMware, Inc.') + self.assertTrue(is_vmware_nic('eth0', self.tmp)) + + def test_a_physical_nic_is_left_alone(self): + self._make('eth0', label='Ethernet0', + vendor='0x8086', subsystem_vendor='0x8086') + self.assertFalse(is_vmware_nic('eth0', self.tmp)) + + class TestPermanentMac(unittest.TestCase): """Reading the hardware's own address matters wherever a custom MAC may be in effect: the current address is then the custom one. This used to -- cgit v1.2.3 From 2c4b01409a625605009b6e6b29046723a4b2bd57 Mon Sep 17 00:00:00 2001 From: Christian Breunig Date: Wed, 7 Oct 2026 19:57:22 +0200 Subject: smoketest: T3871: run the naming tests on a VMware guest in CI too The interface naming testcases gained a second variant in vyos-build, running on a guest that presents VMware firmware - the one platform where the adapter order comes from the firmware rather than from the PCI topology, and the one the reported regression was found on. CI now runs both variants and reports them separately. The target the existing job calls was renamed along with it. --- .github/workflows/package-smoketest.yml | 40 +++++++++++++++++++++++++++++++-- src/tests/test_ifname_store.py | 2 +- 2 files changed, 39 insertions(+), 3 deletions(-) (limited to 'src/tests/test_ifname_store.py') diff --git a/.github/workflows/package-smoketest.yml b/.github/workflows/package-smoketest.yml index f06c5ffcf..19c9bc18a 100644 --- a/.github/workflows/package-smoketest.yml +++ b/.github/workflows/package-smoketest.yml @@ -347,7 +347,41 @@ jobs: id: test shell: bash run: | - if sudo make test-ifname; then + if sudo make test-interface-naming; then + echo "exit_code=success" >> $GITHUB_OUTPUT + else + echo "exit_code=fail" >> $GITHUB_OUTPUT + exit 1 + fi + + test_interface_naming_vmware: + needs: [build_iso, set_config] + if: needs.set_config.outputs.test_ifname == 'true' + runs-on: ubuntu-24.04 + timeout-minutes: 45 + container: + image: ${{ needs.set_config.outputs.container_image }} + options: --sysctl net.ipv6.conf.lo.disable_ipv6=0 --privileged + outputs: + exit_code: ${{ steps.test.outputs.exit_code }} + steps: + - name: Clone vyos-build source code + uses: actions/checkout@v6 + with: + repository: ${{ needs.set_config.outputs.vyos_build_repo }} + token: ${{ needs.set_config.outputs.use_pat == 'true' && secrets.PAT || secrets.GITHUB_TOKEN }} + ref: ${{ needs.set_config.outputs.vyos_build_ref }} + + - uses: actions/download-artifact@v8 + with: + name: vyos-${{ needs.build_iso.outputs.build_version }} + path: build + + - name: VyOS interface naming tests (VMware firmware) + id: test + shell: bash + run: | + if sudo make test-interface-naming-vmware; then echo "exit_code=success" >> $GITHUB_OUTPUT else echo "exit_code=fail" >> $GITHUB_OUTPUT @@ -511,6 +545,7 @@ jobs: - test_smoketest_cli_vpp - test_interfaces_cli - test_interface_naming + - test_interface_naming_vmware - test_config_load - test_config_load_vpp - test_raid1_install @@ -526,7 +561,7 @@ jobs: echo "${{ github.event.pull_request.number }}" > pr-report/pr_number.txt cat > pr-report/message.md << 'EOF' - CI integration ${{ (needs.test_smoketest_cli.outputs.exit_code == 'success' && needs.test_interfaces_cli.outputs.exit_code == 'success' && needs.test_config_load.outputs.exit_code == 'success' && needs.test_raid1_install.outputs.exit_code == 'success' && (needs.test_smoketest_cli_vpp.outputs.exit_code != 'fail') && (needs.test_config_load_vpp.outputs.exit_code != 'fail') && (needs.test_encrypted_config_tpm.outputs.exit_code != 'fail') && (needs.test_interface_naming.outputs.exit_code != 'fail')) && '👍 passed!' || '❌ failed!' }} + CI integration ${{ (needs.test_smoketest_cli.outputs.exit_code == 'success' && needs.test_interfaces_cli.outputs.exit_code == 'success' && needs.test_config_load.outputs.exit_code == 'success' && needs.test_raid1_install.outputs.exit_code == 'success' && (needs.test_smoketest_cli_vpp.outputs.exit_code != 'fail') && (needs.test_config_load_vpp.outputs.exit_code != 'fail') && (needs.test_encrypted_config_tpm.outputs.exit_code != 'fail') && (needs.test_interface_naming.outputs.exit_code != 'fail') && (needs.test_interface_naming_vmware.outputs.exit_code != 'fail')) && '👍 passed!' || '❌ failed!' }} ### Details @@ -540,6 +575,7 @@ jobs: * Config tests VPP ${{ needs.test_config_load_vpp.outputs.exit_code == 'success' && '👍 passed' || needs.test_config_load_vpp.outputs.exit_code == 'fail' && '❌ failed' || '⏭️ skipped' }} * TPM tests ${{ needs.test_encrypted_config_tpm.outputs.exit_code == 'success' && '👍 passed' || needs.test_encrypted_config_tpm.outputs.exit_code == 'fail' && '❌ failed' || '⏭️ skipped' }} * Interface naming tests ${{ needs.test_interface_naming.outputs.exit_code == 'success' && '👍 passed' || needs.test_interface_naming.outputs.exit_code == 'fail' && '❌ failed' || '⏭️ skipped' }} + * Interface naming tests (VMware) ${{ needs.test_interface_naming_vmware.outputs.exit_code == 'success' && '👍 passed' || needs.test_interface_naming_vmware.outputs.exit_code == 'fail' && '❌ failed' || '⏭️ skipped' }} EOF - uses: actions/upload-artifact@v7 diff --git a/src/tests/test_ifname_store.py b/src/tests/test_ifname_store.py index 5b1fec17f..b61cd0041 100644 --- a/src/tests/test_ifname_store.py +++ b/src/tests/test_ifname_store.py @@ -278,7 +278,7 @@ class TestMovedHardware(unittest.TestCase): self.assertEqual(store['hardware']['eth0'], self.B) def test_editing_the_slot_map_by_hand_takes_effect(self): - # what `make test-ifname` does: swap the two entries in 'interfaces' + # what `make test-interface-naming` does: swap the two entries in 'interfaces' # and leave 'hardware' alone. Following the recorded address here # would quietly undo the edit. store = store_of({'eth0': 'pci-0000:00:03.0', -- cgit v1.2.3 From 639d575ad258f6f6ba629a0c50a710b736e4911d Mon Sep 17 00:00:00 2001 From: Alex Kudentsov <43482574+alexk37@users.noreply.github.com> Date: Wed, 14 Oct 2026 20:45:45 +0700 Subject: interfaces: T3871: keep an accelerated-networking VF off its NIC's .link file On Azure every Hyper-V NIC shares one ID_PATH and has no slot, so it is keyed on its MAC. Its accelerated-networking VF carries the same MAC and matched the NIC's .link file, so udev tried to give the VF the NIC's name. The .link file of a Hyper-V NIC now also matches Driver=hv_netvsc, so the VF cannot match it. This holds for a lone NIC keyed on the VMBus path too. A NIC absent at a pass cannot say what it is, so its file keeps the pin it already carries. NICs that are not Hyper-V ones get no Driver line, and their files do not change. The 34-to-35 migration passes the hardware it found, so the pin is written on the first boot of an upgrade too. --- python/vyos/ifconfig/ifname_store.py | 43 ++++++++- src/migration-scripts/interfaces/34-to-35 | 2 +- src/system/vyos-net-name-resolve.py | 2 +- src/tests/test_ifname_store.py | 116 ++++++++++++++++++++++++ src/tests/test_migration_interfaces_34_to_35.py | 12 +++ 5 files changed, 170 insertions(+), 5 deletions(-) (limited to 'src/tests/test_ifname_store.py') diff --git a/python/vyos/ifconfig/ifname_store.py b/python/vyos/ifconfig/ifname_store.py index e0a6b4cbe..20731aed1 100644 --- a/python/vyos/ifconfig/ifname_store.py +++ b/python/vyos/ifconfig/ifname_store.py @@ -60,6 +60,8 @@ _PORT_LABEL = re.compile(r'^Ethernet(\d+)$') # compares tuples element-wise, so a None here would raise against an int _LABEL_UNKNOWN = 1 << 30 _VMWARE_PCI_VENDOR = '0x15ad' +# a Hyper-V NIC carries no slot, path or label of its own - only its MAC +_HYPERV_DRIVER = 'hv_netvsc' _DMI_SYS_VENDOR = Path('/sys/class/dmi/id/sys_vendor') _MAC_RE = re.compile(r'([0-9a-fA-F]{2}(:[0-9a-fA-F]{2}){5})') @@ -119,6 +121,11 @@ def canonical_sort_key(device: dict) -> tuple: return (rank, label, segments, path, device.get('mac') or '') +def is_hyperv_nic(device: dict) -> bool: + properties = device.get('properties') or {} + return properties.get('ID_NET_DRIVER') == _HYPERV_DRIVER + + def _sysfs(path: Path) -> str: try: return path.read_text().strip() @@ -529,40 +536,70 @@ def _link_type_for(name: str) -> str: return 'ether' -def render_link(name: str, key: str) -> str: +def render_link(name: str, key: str, driver: str = '') -> str: """Render a systemd .link file body, matching on the stored property. The device type has to match as well, and a radio is 'wlan' - a .link naming a type the device is not simply never applies, leaving the interface on its probe-order name. resolve() already names by type (see _prefix_for()), so the recorded name is what says which. + + A MAC is not unique to one interface: an accelerated-networking VF + carries its NIC's MAC, and matched the NIC's file - udev then tried to + give the VF the NIC's name. The driver tells them apart. """ + match_driver = f'Driver={driver}\n' if driver else '' return ( '# Generated by VyOS - do not edit.\n' "# Interface names are managed via 'interface-mapping.json'.\n" '[Match]\n' f'Type={_link_type_for(name)}\n' f'Property={key}\n' + f'{match_driver}' '\n' '[Link]\n' f'Name={name}\n' ) -def sync_link_files(store: dict, link_dir: Path = None) -> list: +def _pinned_driver(path: Path, key: str) -> str: + """The driver an existing file already pins for this key, if any""" + try: + body = path.read_text() + except OSError: + return '' + if f'Property={key}\n' not in body: + return '' + m = re.search(r'^Driver=(\S+)$', body, re.M) + return m.group(1) if m else '' + + +def sync_link_files(store: dict, link_dir: Path = None, devices: list = None) -> list: """One .link file per recorded name, so udev applies it as the device is added - before anything can observe a probe-order name. """ link_dir = link_dir or LINK_DIR link_dir.mkdir(parents=True, exist_ok=True) + # a Hyper-V NIC shares its MAC with its accelerated-networking VF, so + # its file names the driver too - also when the NIC is keyed on the VMBus + # path, as a lone one is. A NIC absent this boot cannot say which, and + # keeps the pin its file already carries. + drivers = {} + for device in devices or []: + key = device.get('key') or '' + if key and is_hyperv_nic(device): + drivers[key] = _HYPERV_DRIVER + written = {} for name, key in store.get('interfaces', {}).items(): # never for a radio: the key is the phy's address, which every # interface created on that phy shares, so the file would rename # whichever of them udev saw next if key and not name.startswith('wlan'): - written[link_dir / f'{LINK_PREFIX}{name}.link'] = render_link(name, key) + path = link_dir / f'{LINK_PREFIX}{name}.link' + driver = drivers.get(key) or _pinned_driver(path, key) + written[path] = render_link(name, key, driver) for path, body in written.items(): if path.exists() and path.read_text() == body: diff --git a/src/migration-scripts/interfaces/34-to-35 b/src/migration-scripts/interfaces/34-to-35 index 9176184f4..a09dc8cd8 100755 --- a/src/migration-scripts/interfaces/34-to-35 +++ b/src/migration-scripts/interfaces/34-to-35 @@ -64,6 +64,6 @@ def migrate(config: ConfigTree) -> None: if added and save_store(store): try: - sync_link_files(store) + sync_link_files(store, devices=list(devices.values())) except OSError: pass diff --git a/src/system/vyos-net-name-resolve.py b/src/system/vyos-net-name-resolve.py index 58966e291..d74663707 100755 --- a/src/system/vyos-net-name-resolve.py +++ b/src/system/vyos-net-name-resolve.py @@ -254,7 +254,7 @@ def main(): if persisted: # render so udev applies the names on the next boot try: - sync_link_files(store) + sync_link_files(store, devices=devices) except OSError as e: logger.error(f'could not write .link files: {e}') else: diff --git a/src/tests/test_ifname_store.py b/src/tests/test_ifname_store.py index b61cd0041..ff0f916ec 100644 --- a/src/tests/test_ifname_store.py +++ b/src/tests/test_ifname_store.py @@ -173,6 +173,122 @@ class TestHypervisorAdapterOrder(unittest.TestCase): self.assertNotIn('label', hardware_key(properties)) +def hv(name, mac): + """A Hyper-V NIC as udev reports it on Azure: every one shares the ACPI + VMBus node as its ID_PATH and has no slot, so its MAC is its key.""" + return { + 'name': name, + 'mac': mac, + 'wireless': False, + 'properties': { + 'ID_PATH': 'acpi-MSFT1000:00', + 'ID_NET_NAME_MAC': 'enx' + mac.replace(':', ''), + 'ID_NET_DRIVER': 'hv_netvsc', + }, + } + + +def keyed(devices): + """Keys as discover_devices() computes them, shared ID_PATH passed over""" + ambiguous = ambiguous_keys([d['properties'] for d in devices]) + for device in devices: + device['key'] = hardware_key(device['properties'], ambiguous) + return devices + + +class TestHyperV(unittest.TestCase): + """Azure, four NICs, measured on a fresh deployment of this series: every + Hyper-V NIC shares one ID_PATH and has no slot, so its MAC is its key. + The order Azure lists the NICs in is cloud-init's to record. + """ + + AZURE = [ + ('eth0', '00:0d:3a:47:38:1d'), # WAN, the primary NIC + ('eth1', '00:0d:3a:47:3b:29'), + ('eth2', '00:0d:3a:47:33:ad'), + ('eth3', '00:0d:3a:47:3b:f8'), + ] + + def nics(self): + return keyed([hv(name, mac) for name, mac in self.AZURE]) + + def stored(self): + """the map as cloud-init records it on Azure: the names it gave""" + store = empty_store() + for name, mac in self.AZURE: + store['interfaces'][name] = 'ID_NET_NAME_MAC=enx' + mac.replace(':', '') + store['hardware'][name] = mac + plan, store, report = resolve(self.nics(), store) + assert plan == {} and len(report['matched']) == 4 + return store + + def test_each_nic_is_keyed_on_its_own_mac(self): + store = self.stored() + self.assertEqual( + sorted(store['interfaces'].values()), + sorted( + 'ID_NET_NAME_MAC=enx' + mac.replace(':', '') for _, mac in self.AZURE + ), + ) + + def test_a_lone_hyperv_nic_keyed_on_its_path_still_pins_its_driver(self): + # one NIC has the VMBus path to itself, so it is keyed on it; its VF + # must not match its file any more than with a MAC key + devices = keyed([hv('eth0', '00:0d:3a:47:38:1d')]) + _, store, _ = resolve(devices, empty_store()) + self.assertTrue(store['interfaces']['eth0'].startswith('ID_PATH=')) + with tempfile.TemporaryDirectory() as d: + sync_link_files(store, link_dir=Path(d), devices=devices) + body = (Path(d) / '10-vyos-eth0.link').read_text() + self.assertIn('Driver=hv_netvsc\n', body) + + def test_link_file_pins_the_driver_for_a_mac_key(self): + # the accelerated-networking VF carries its NIC's MAC + body = render_link('eth0', 'ID_NET_NAME_MAC=enx000d3a47381d', 'hv_netvsc') + self.assertIn( + 'Property=ID_NET_NAME_MAC=enx000d3a47381d\nDriver=hv_netvsc\n', body + ) + + def test_link_file_without_a_driver_is_unchanged(self): + self.assertNotIn('Driver=', render_link('eth0', 'ID_PATH=pci-0000:00:03.0')) + + def link_dir(self): + tmp = Path(tempfile.mkdtemp()) + self.addCleanup(shutil.rmtree, tmp) + return tmp + + def test_sync_pins_the_driver_for_hyperv_only(self): + devices = self.nics() + usb = { + 'name': 'eth4', + 'mac': 'aa:00:00:00:00:04', + 'wireless': False, + 'properties': { + 'ID_NET_NAME_MAC': 'enxaa0000000004', + 'ID_NET_DRIVER': 'r8152', + }, + } + devices += keyed([usb]) + _, store, _ = resolve(devices, empty_store()) + tmp = self.link_dir() + sync_link_files(store, link_dir=tmp, devices=devices) + name_of = {key: name for name, key in store['interfaces'].items()} + wan = name_of['ID_NET_NAME_MAC=enx000d3a47381d'] + other = name_of['ID_NET_NAME_MAC=enxaa0000000004'] + self.assertIn('Driver=hv_netvsc', (tmp / f'10-vyos-{wan}.link').read_text()) + self.assertNotIn('Driver=', (tmp / f'10-vyos-{other}.link').read_text()) + + def test_an_absent_nic_keeps_its_driver_pin(self): + # a boot without the NIC used to rewrite its file without the pin, and + # on its return its VF matched the file again + devices = self.nics() + _, store, _ = resolve(devices, empty_store()) + tmp = self.link_dir() + sync_link_files(store, link_dir=tmp, devices=devices) + sync_link_files(store, link_dir=tmp, devices=devices[:3]) + self.assertIn('Driver=hv_netvsc', (tmp / '10-vyos-eth3.link').read_text()) + + class TestReplacedHardware(unittest.TestCase): """A slot keeps its name when its card is swapped - which hands that interface's addresses to different hardware. resolve() reports the change diff --git a/src/tests/test_migration_interfaces_34_to_35.py b/src/tests/test_migration_interfaces_34_to_35.py index 4a38d6341..ad83de424 100644 --- a/src/tests/test_migration_interfaces_34_to_35.py +++ b/src/tests/test_migration_interfaces_34_to_35.py @@ -102,6 +102,18 @@ class TestMigration34to35(unittest.TestCase): self.assertEqual(entries['eth0'], 'ID_PATH=pci-0000:00:04.0') self.assertEqual(entries['eth1'], 'ID_PATH=pci-0000:00:05.0') + def test_the_link_files_are_written_from_the_hardware_present(self): + # the files pin a Hyper-V NIC's driver, which only the device says + devices = [ + device('eth0', '00:00:5e:00:53:01', 'pci-0000:00:04.0'), + device('eth1', '00:00:5e:00:53:02', 'pci-0000:00:05.0'), + ] + with self._devices(devices): + migration.migrate(self.config) + migration.sync_link_files.assert_called_once() + self.assertEqual(migration.sync_link_files.call_args.kwargs['devices'], + devices) + def test_names_are_preserved_even_out_of_slot_order(self): # a box named by an earlier release in MAC order: eth0 sits in the # HIGHER slot. The migration must record what is, not what slot -- cgit v1.2.3 From 153dd688b327c922ec4b371db1fdd11b8ee8cf9e Mon Sep 17 00:00:00 2001 From: Alex Kudentsov <43482574+alexk37@users.noreply.github.com> Date: Wed, 14 Oct 2026 20:46:08 +0700 Subject: interfaces: T3871: give a re-created Hyper-V NIC the name of the NIC it replaced A Hyper-V NIC is known by its MAC alone. One deleted and created again on the same VM - an Azure NIC re-created, or a Hyper-V adapter removed and added - comes back with a new MAC, so nothing on it matched the store: its name stayed reserved, the new NIC got a new name, and its configuration was left on a name with no NIC. cloud-init does not run again on the same instance, so nothing else repairs it. At boot, a vacant name now goes to a new NIC only when the pairing is not in doubt: - exactly one recorded name has no NIC, and exactly one new Hyper-V NIC appeared. With more of either, the order the NICs come up in does not say which replaced which, so nothing is handed over and the names stay reserved, as before; - the name went missing at this pass. The store records which names were missing at the last pass, so a NIC removed for good does not hand its name to a NIC added at a later boot; - the name's own NIC was a Hyper-V one. The store records which names sat on a Hyper-V NIC; a MAC-keyed name can belong to other hardware, and a store written before this was recorded hands nothing over (the boot pass logs that when it holds a name back); - only at boot. On a running system the configuration under a name is not applied to the NIC that takes it, so the hotplug unit passes --hotplug, which names the new NIC like any other and records the vacant name as missing, so the next boot does not hand it over either. An adoption is logged as a warning, reported under 'adopted' in the status file (kept there across hotplug passes while it holds), and repeated by vyos-router, as a slot match is: verify the port before relying on its configuration. --- python/vyos/ifconfig/ifname_store.py | 89 +++++++++- src/init/vyos-router | 6 + src/system/vyos-net-name-resolve.py | 56 ++++++- src/systemd/vyos-net-name-resolve-hotplug.service | 2 +- src/systemd/vyos-net-name-resolve.service | 4 +- src/tests/test_ifname_store.py | 191 +++++++++++++++++++++- src/tests/test_net_name_resolve.py | 157 ++++++++++++++++++ 7 files changed, 487 insertions(+), 18 deletions(-) (limited to 'src/tests/test_ifname_store.py') diff --git a/python/vyos/ifconfig/ifname_store.py b/python/vyos/ifconfig/ifname_store.py index 20731aed1..961a81d15 100644 --- a/python/vyos/ifconfig/ifname_store.py +++ b/python/vyos/ifconfig/ifname_store.py @@ -302,8 +302,25 @@ def load_store(path: Path = None) -> dict: if isinstance(mac, str) and mac and name in interfaces } if isinstance(seen, dict) else {} - return {'version': STORE_VERSION, 'interfaces': interfaces, - 'hardware': hardware} + store = {'version': STORE_VERSION, 'interfaces': interfaces, 'hardware': hardware} + + # names whose hardware was missing at the last pass - kept only while + # there are any, so a store without them reads as it always did + absent = data.get('absent') + if isinstance(absent, list): + absent = sorted({n for n in absent if n in interfaces}) + if absent: + store['absent'] = absent + + # names whose NIC was a Hyper-V one when last seen - the only names a new + # Hyper-V NIC may take over. A store without them (written before they + # were recorded) has none. + hyperv = data.get('hyperv') + if isinstance(hyperv, list): + hyperv = sorted({n for n in hyperv if n in interfaces}) + if hyperv: + store['hyperv'] = hyperv + return store def encrypted_config_volume() -> bool: @@ -411,12 +428,15 @@ def _next_free(taken: set, prefix: str) -> str: return f'{prefix}{index}' -def resolve(devices: list, store: dict) -> tuple: +def resolve(devices: list, store: dict, adopt: bool = True) -> tuple: """Name every physical interface present this boot. Returns (plan, new_store, report); plan is {current: new} for the interfaces which have to be renamed. The slot decides the name - nothing here looks at the configuration, so there is nothing to guess. + + adopt=False keeps a vacant Hyper-V name from going to a new NIC, for a + pass on a running system (see the adoption block below). """ entries = store.get('interfaces', {}) seen = store.get('hardware', {}) @@ -436,13 +456,23 @@ def resolve(devices: list, store: dict) -> tuple: seen = {n: m for n, m in seen.items() if n not in misfiled} assigned = {} # final name -> device - report = {'matched': {}, 'reassigned': {}, 'moved': {}, - 'replaced': {}, 'bootstrapped': {}} + report = { + 'matched': {}, + 'reassigned': {}, + 'moved': {}, + 'replaced': {}, + 'adopted': {}, + 'bootstrapped': {}, + } # A recorded name stays off-limits even while its slot is empty, so new # hardware cannot inherit an absent interface's name and configuration. reserved = set(entries) remaining = list(devices) + # what the last pass recorded: names already missing then, and names + # whose NIC was a Hyper-V one + missing_before = set(store.get('absent', [])) + hyperv_before = set(store.get('hyperv', [])) def claim(device, name, bucket): assigned[name] = device @@ -500,6 +530,36 @@ def resolve(devices: list, store: dict) -> tuple: claim(device, name, 'replaced') break + # a Hyper-V NIC is known by its MAC alone, and one deleted and created + # again comes back with a new MAC, so nothing on it could match. When one + # recorded NIC is gone and one new NIC took its place the pairing is not in + # doubt, and the name goes to it. With more of either it is: the order the + # NICs come up in here does not say which replaced which, so nothing is + # handed over and the names stay reserved. + # Only on the pass the NIC first went missing: a replacement is one change + # to the VM. A name already missing at the last pass belongs to a NIC that + # was removed, and a NIC added later is not its replacement. + # Only at boot: on a running system the configuration under the name is + # not applied to the NIC that takes it, so the name would point at a port + # that is down and unaddressed until the next boot. The new NIC gets a new + # name there, and the vacant one is recorded as missing, so the next boot + # does not hand it over either. + # Only a name whose own NIC was a Hyper-V one: a MAC-keyed name can + # belong to other hardware too, and a store that does not say (written + # before this was recorded) hands nothing over. + vacant = [ + name + for name, key in entries.items() + if name not in assigned + and name not in missing_before + and name in hyperv_before + and key.startswith('ID_NET_NAME_MAC=') + ] + adoptable = [device for device in remaining if is_hyperv_nic(device)] + if (adopt and len(vacant) == 1 and len(adoptable) == 1 + and _name_fits(vacant[0], adoptable[0])): + claim(adoptable[0], vacant[0], 'adopted') + # unknown hardware: lowest free name of its type, in hardware order for device in sorted(remaining, key=canonical_sort_key): name = _next_free(reserved | set(assigned), _prefix_for(device)) @@ -521,8 +581,23 @@ def resolve(devices: list, store: dict) -> tuple: plan = {device['name']: name for name, device in assigned.items() if device['name'] != name} - return plan, {'version': STORE_VERSION, 'interfaces': new_interfaces, - 'hardware': new_hardware}, report + new_store = { + 'version': STORE_VERSION, + 'interfaces': new_interfaces, + 'hardware': new_hardware, + } + absent = sorted(name for name in new_interfaces if name not in assigned) + if absent: + new_store['absent'] = absent + # what each name's NIC was, kept for an absent one like its address + hyperv = sorted( + {name for name, device in assigned.items() if is_hyperv_nic(device)} + | (hyperv_before & set(absent)) + ) + if hyperv: + new_store['hyperv'] = hyperv + + return plan, new_store, report LINK_DIR = Path('/etc/systemd/network') diff --git a/src/init/vyos-router b/src/init/vyos-router index ff1de0584..8978d4157 100755 --- a/src/init/vyos-router +++ b/src/init/vyos-router @@ -385,6 +385,12 @@ warn_missing_interface_hardware () log_warning_msg "interface '${intf}' was matched to new hardware ${hwid} by slot - verify it is the port you expect before relying on its configuration" done + jq -r '.adopted // {} | to_entries[] | "\(.key) \(.value)"' "${status_file}" \ + 2>/dev/null | while read -r intf hwid; do + [ -n "${intf}" ] || continue + log_warning_msg "interface '${intf}' had no NIC this boot and was given to the new Hyper-V NIC ${hwid} - verify it is the port you expect before relying on its configuration" + done + if [ "$(jq -r '.persisted // true' "${status_file}" 2>/dev/null)" = "false" ]; then log_failure_msg "interface names could not be persisted - the configuration volume is not mounted" fi diff --git a/src/system/vyos-net-name-resolve.py b/src/system/vyos-net-name-resolve.py index d74663707..f3e4e62fb 100755 --- a/src/system/vyos-net-name-resolve.py +++ b/src/system/vyos-net-name-resolve.py @@ -26,6 +26,7 @@ import json import logging import logging.handlers +import sys import time from pathlib import Path @@ -182,16 +183,21 @@ def write_status(store: dict, devices: list, applied: dict, report: dict, # in use, so left alone until the next boot 'rename_refused': refused or {}, # everything recognised from the store, however it was recognised - 'matched': {**report.get('matched', {}), - **report.get('reassigned', {}), - **report.get('moved', {}), - **report.get('replaced', {})}, + 'matched': { + **report.get('matched', {}), + **report.get('reassigned', {}), + **report.get('moved', {}), + **report.get('replaced', {}), + **report.get('adopted', {}), + }, # the name changed hands between cards the store already knows 'reassigned': report.get('reassigned', {}), # the card was found in a different slot - certain, it is the same card 'moved': report.get('moved', {}), # a different card took the slot - the one case that needs verifying 'matched_by_path': report.get('replaced', {}), + # a recorded Hyper-V NIC was gone and its one replacement took its name + 'adopted': report.get('adopted', {}), 'new_hardware': report.get('bootstrapped', {}), 'persisted': persisted, } @@ -201,7 +207,25 @@ def write_status(store: dict, devices: list, applied: dict, report: dict, logger.error(f'could not write status file: {e}') -def main(): +def boot_adoptions(store: dict) -> dict: + """The adoptions the boot pass reported that still hold. A pass for + hot-plugged hardware adopts nothing, and must not wipe the record of + what this boot did.""" + try: + adopted = json.loads(status_file.read_text()).get('adopted') or {} + except (OSError, ValueError, AttributeError): + return {} + hardware = store.get('hardware', {}) + return {name: mac for name, mac in adopted.items() + if hardware.get(name) == mac} + + +def hotplug_requested(argv: list) -> bool: + """--hotplug: started for hardware added to a running system""" + return '--hotplug' in argv + + +def main(hotplug: bool = False): if is_running_as_container(): # a container owns no NIC - its veth pairs have no backing bus device logger.info('running inside a container - skipping interface naming') @@ -229,7 +253,16 @@ def main(): 'reserved.' ) - plan, store, report = resolve(devices, store) + if not hotplug and 'hyperv' not in store and any( + key.startswith('ID_NET_NAME_MAC=') and key in absent + for key in store['interfaces'].values()): + # a name is vacant, and the map was written by code that did not + # record which NICs were Hyper-V ones: the name is not handed over. + # Each pass records the Hyper-V NICs it finds, but never a vacant name + logger.info('the interface map does not say which interfaces were ' + 'Hyper-V NICs - no vacant name is given to a new NIC ' + 'this boot') + plan, store, report = resolve(devices, store, adopt=not hotplug) for name, mac in sorted(report.get('bootstrapped', {}).items()): logger.info(f"new hardware {mac} named '{name}'") @@ -242,6 +275,12 @@ def main(): f"'{name}' was matched to new hardware {mac} by slot - verify it " 'is the port you expect before relying on its configuration' ) + for name, mac in sorted(report.get('adopted', {}).items()): + logger.warning( + f"'{name}' had no NIC this boot and was given to the new Hyper-V " + f'NIC {mac}, which took its place - verify it is the port you ' + 'expect before relying on its configuration' + ) plan, refused = refuse_renames_in_use(plan) for old, target in sorted(refused.items()): logger.warning( @@ -263,6 +302,9 @@ def main(): 'resolved for this boot but could not be persisted' ) + if hotplug: + # reported once, by the boot pass; kept on record until the next boot + report = {**report, 'adopted': boot_adoptions(store)} write_status(store, devices, applied, report, persisted, refused) @@ -274,4 +316,4 @@ if __name__ == '__main__': logger.addHandler(syslog_handler) logger.setLevel(logging.DEBUG) - main() + main(hotplug=hotplug_requested(sys.argv[1:])) diff --git a/src/systemd/vyos-net-name-resolve-hotplug.service b/src/systemd/vyos-net-name-resolve-hotplug.service index c798feb99..d52fc6061 100644 --- a/src/systemd/vyos-net-name-resolve-hotplug.service +++ b/src/systemd/vyos-net-name-resolve-hotplug.service @@ -8,6 +8,6 @@ ConditionPathExists=/tmp/vyos-config-status [Service] Type=oneshot -ExecStart=/usr/libexec/vyos/system/vyos-net-name-resolve.py +ExecStart=/usr/libexec/vyos/system/vyos-net-name-resolve.py --hotplug TimeoutStartSec=90 StandardOutput=journal diff --git a/src/systemd/vyos-net-name-resolve.service b/src/systemd/vyos-net-name-resolve.service index b02d22cf7..8ad1c3418 100644 --- a/src/systemd/vyos-net-name-resolve.service +++ b/src/systemd/vyos-net-name-resolve.service @@ -4,8 +4,8 @@ Documentation=man:vyos-net-name-resolve.py(8) # Started by vyos-router once the config directory is mounted - for an # encrypted volume that is only after it has been unlocked - so this can not -# be ordered independently. Started again for hot-plugged hardware, hence no -# RemainAfterExit, which would make every later start a no-op. +# be ordered independently. This is the boot pass; hot-plugged hardware is +# named by vyos-net-name-resolve-hotplug.service, which passes --hotplug. DefaultDependencies=no [Service] diff --git a/src/tests/test_ifname_store.py b/src/tests/test_ifname_store.py index ff0f916ec..635033fff 100644 --- a/src/tests/test_ifname_store.py +++ b/src/tests/test_ifname_store.py @@ -199,7 +199,9 @@ def keyed(devices): class TestHyperV(unittest.TestCase): """Azure, four NICs, measured on a fresh deployment of this series: every Hyper-V NIC shares one ID_PATH and has no slot, so its MAC is its key. - The order Azure lists the NICs in is cloud-init's to record. + The order Azure lists the NICs in is cloud-init's to record; what is left + here is a NIC deleted and created again on the same VM, where cloud-init + does not run again. """ AZURE = [ @@ -231,6 +233,162 @@ class TestHyperV(unittest.TestCase): ), ) + def test_a_recreated_nic_takes_the_name_of_the_one_it_replaced(self): + # LAN3's NIC deleted and created again: new MAC. It came up as eth4, + # eth3 stayed reserved and the boot commit failed on it every boot. + store = self.stored() + devices = self.nics() + devices[3] = keyed([hv('eth3', '00:0d:3a:47:aa:01')] + devices[:3])[0] + plan, store, report = resolve(devices, store) + self.assertEqual(report.get('adopted', {}), {'eth3': '00:0d:3a:47:aa:01'}) + self.assertEqual(report['bootstrapped'], {}) + self.assertEqual(plan, {}) + self.assertEqual(store['interfaces']['eth3'], 'ID_NET_NAME_MAC=enx000d3a47aa01') + + def test_two_recreated_nics_are_not_paired_by_guess(self): + # nothing here says which new NIC replaced which, and pairing them by + # the order they came up in can swap two subnets' configuration + store = self.stored() + devices = self.nics()[:2] + keyed( + [hv('eth3', '00:0d:3a:99:00:11'), hv('eth2', '00:0d:3a:99:00:33')] + ) + _, store, report = resolve(devices, store) + self.assertEqual(report.get('adopted', {}), {}) + self.assertEqual(set(report['bootstrapped']), {'eth4', 'eth5'}) + self.assertIn('eth2', store['interfaces']) + self.assertIn('eth3', store['interfaces']) + + def test_two_vacancies_and_one_new_nic_are_not_paired(self): + # which of two gone NICs the one new NIC replaced is not known + store = self.stored() + devices = self.nics()[:2] + keyed( + [hv('eth3', '00:0d:3a:99:00:11')] + self.nics()[:2])[:1] + _, store, report = resolve(devices, store) + self.assertEqual(report.get('adopted', {}), {}) + self.assertEqual(report['bootstrapped'], {'eth4': '00:0d:3a:99:00:11'}) + self.assertEqual(store.get('absent'), ['eth2', 'eth3']) + + def test_a_new_nic_with_no_vacancy_takes_a_new_name(self): + store = self.stored() + devices = self.nics() + keyed([hv('eth4', '00:0d:3a:48:00:09')]) + _, _, report = resolve(devices, store) + self.assertEqual(report.get('adopted', {}), {}) + self.assertEqual(report['bootstrapped'], {'eth4': '00:0d:3a:48:00:09'}) + + def test_one_vacancy_and_two_new_nics_are_not_paired(self): + store = self.stored() + devices = self.nics()[:3] + keyed( + [hv('eth3', '00:0d:3a:48:00:01'), hv('eth4', '00:0d:3a:48:00:02')] + ) + _, _, report = resolve(devices, store) + self.assertEqual(report.get('adopted', {}), {}) + + def test_a_disk_on_all_new_nics_is_not_handed_over_here(self): + # every recorded NIC gone at once: the platform's order is cloud-init's + # to supply (it runs again on a new instance); guessing it here could + # put the WAN's configuration on a LAN + store = self.stored() + new = keyed([hv(f'eth{i}', f'00:0d:3a:48:00:0{i}') for i in range(4)]) + _, _, report = resolve(new, store) + self.assertEqual(report.get('adopted', {}), {}) + + def test_a_slot_keyed_name_is_never_adopted(self): + # a PCI card has a slot to come back to; its name stays reserved + devices = [dev('eth0', 'pci-0000:00:03.0', 'aa:00')] + _, store, _ = resolve(devices, empty_store()) + other = keyed([hv('eth1', '00:0d:3a:48:00:09')]) + _, store, report = resolve(other, store) + self.assertEqual(report.get('adopted', {}), {}) + self.assertEqual(report['bootstrapped'], {'eth1': '00:0d:3a:48:00:09'}) + + def test_a_nic_added_after_one_was_removed_is_not_its_replacement(self): + # P8, measured on Azure: LAN3's NIC deleted for good (one boot), a NIC + # on another subnet added later (the next boot). Taking the vacant + # name put LAN3's configuration on the other subnet. + store = self.stored() + devices = self.nics()[:3] + _, store, report = resolve(devices, store) + self.assertEqual(report.get('adopted', {}), {}) + later = devices + keyed([hv('eth4', '38:33:c5:cc:a3:3e')]) + _, store, report = resolve(later, store) + self.assertEqual(report.get('adopted', {}), {}) + self.assertEqual(report['bootstrapped'], {'eth4': '38:33:c5:cc:a3:3e'}) + self.assertEqual(store.get('absent'), ['eth3']) + + def test_a_nic_replaced_on_a_running_system_is_not_adopted(self): + # measured on Hyper-V: a NIC hot-added after another was hot-removed + # took its name at once, while the configuration under that name + # stayed unapplied until the next boot + store = self.stored() + devices = self.nics() + devices[3] = keyed([hv('eth3', '00:0d:3a:47:aa:01')] + devices[:3])[0] + _, store, report = resolve(devices, store, adopt=False) + self.assertEqual(report.get('adopted', {}), {}) + self.assertEqual(report['bootstrapped'], {'eth4': '00:0d:3a:47:aa:01'}) + self.assertEqual(store.get('absent'), ['eth3']) + # nor at the next boot: the name was already missing at the last pass + _, store, report = resolve(devices, store) + self.assertEqual(report.get('adopted', {}), {}) + self.assertEqual(report['matched'].get('eth4'), '00:0d:3a:47:aa:01') + + def test_a_name_whose_nic_was_not_hyperv_is_not_handed_over(self): + # a MAC-keyed name can belong to other hardware sharing the VMBus + # path; only a Hyper-V NIC's name goes to a Hyper-V replacement + other = hv('eth1', '00:0d:3a:47:3b:29') + other['properties']['ID_NET_DRIVER'] = 'mlx5_core' + devices = keyed([hv('eth0', '00:0d:3a:47:38:1d'), other]) + _, store, _ = resolve(devices, empty_store()) + self.assertEqual(store.get('hyperv'), ['eth0']) + later = keyed([hv('eth0', '00:0d:3a:47:38:1d'), + hv('eth1', '00:0d:3a:48:00:09')]) + _, _, report = resolve(later, store) + self.assertEqual(report.get('adopted', {}), {}) + self.assertEqual(report['bootstrapped'], {'eth2': '00:0d:3a:48:00:09'}) + + def test_only_a_hyperv_nic_takes_a_vacant_name(self): + store = self.stored() + new = hv('eth3', '00:0d:3a:48:00:09') + new['properties']['ID_NET_DRIVER'] = 'virtio_net' + devices = self.nics()[:3] + keyed([new] + self.nics()[:3])[:1] + _, _, report = resolve(devices, store) + self.assertEqual(report.get('adopted', {}), {}) + self.assertEqual(report['bootstrapped'], {'eth4': '00:0d:3a:48:00:09'}) + + def test_a_new_nic_takes_no_name_of_another_namespace(self): + store = self.stored() + new = keyed([hv('wlan0', '00:0d:3a:48:00:09')] + self.nics()[:3])[0] + new['wireless'] = True + _, _, report = resolve(self.nics()[:3] + [new], store) + self.assertEqual(report.get('adopted', {}), {}) + self.assertEqual(report['bootstrapped'], {'wlan0': '00:0d:3a:48:00:09'}) + + def test_a_store_that_does_not_say_which_nics_were_hyperv_hands_nothing_over(self): + # written by code that did not record it: whether the vacant name + # was a Hyper-V NIC's, or how long it has been vacant, is unknown + store = self.stored() + store.pop('hyperv') + devices = self.nics() + devices[3] = keyed([hv('eth3', '00:0d:3a:47:aa:01')] + devices[:3])[0] + _, store, report = resolve(devices, store) + self.assertEqual(report.get('adopted', {}), {}) + self.assertEqual(report['bootstrapped'], {'eth4': '00:0d:3a:47:aa:01'}) + self.assertEqual(store.get('absent'), ['eth3']) + + def test_the_hyperv_record_survives_a_load_and_follows_the_nics(self): + store = self.stored() + self.assertEqual(store['hyperv'], ['eth0', 'eth1', 'eth2', 'eth3']) + with tempfile.TemporaryDirectory() as d: + path = Path(d) / 'map.json' + with mock.patch('vyos.ifconfig.ifname_store.config_dir_is_mounted', + return_value=True), \ + mock.patch('vyos.ifconfig.ifname_store.get_cfg_group_id', + return_value=0): + save_store(store, path) + self.assertEqual(load_store(path)['hyperv'], store['hyperv']) + # an absent NIC keeps its record, so its replacement is recognised + _, store, _ = resolve(self.nics()[:3], store) + self.assertEqual(store['hyperv'], ['eth0', 'eth1', 'eth2', 'eth3']) + def test_a_lone_hyperv_nic_keyed_on_its_path_still_pins_its_driver(self): # one NIC has the VMBus path to itself, so it is keyed on it; its VF # must not match its file any more than with a MAC key @@ -242,6 +400,37 @@ class TestHyperV(unittest.TestCase): body = (Path(d) / '10-vyos-eth0.link').read_text() self.assertIn('Driver=hv_netvsc\n', body) + def test_a_store_without_the_record_still_adopts(self): + # written before names missing at a pass were recorded: nothing was + # missing then, as far as it can say, so a one-step replacement holds + store = self.stored() + store.pop('absent', None) + devices = self.nics() + devices[3] = keyed([hv('eth3', '00:0d:3a:47:aa:01')] + devices[:3])[0] + _, _, report = resolve(devices, store) + self.assertEqual(report.get('adopted', {}), {'eth3': '00:0d:3a:47:aa:01'}) + + def test_the_record_clears_when_the_nic_is_back(self): + store = self.stored() + _, store, _ = resolve(self.nics()[:3], store) + self.assertEqual(store.get('absent'), ['eth3']) + _, store, report = resolve(self.nics(), store) + self.assertNotIn('absent', store) + self.assertEqual(report['matched']['eth3'], self.AZURE[3][1]) + + def test_the_record_survives_a_load(self): + tmp = Path(tempfile.mkdtemp()) + self.addCleanup(shutil.rmtree, tmp) + path = tmp / 'interface-mapping.json' + store = self.stored() + _, store, _ = resolve(self.nics()[:3], store) + path.write_text(json.dumps(store)) + self.assertEqual(load_store(path).get('absent'), ['eth3']) + # a name the map no longer has is dropped from it + store['absent'] = ['eth3', 'eth9'] + path.write_text(json.dumps(store)) + self.assertEqual(load_store(path).get('absent'), ['eth3']) + def test_link_file_pins_the_driver_for_a_mac_key(self): # the accelerated-networking VF carries its NIC's MAC body = render_link('eth0', 'ID_NET_NAME_MAC=enx000d3a47381d', 'hv_netvsc') diff --git a/src/tests/test_net_name_resolve.py b/src/tests/test_net_name_resolve.py index 62236219a..b529c7164 100644 --- a/src/tests/test_net_name_resolve.py +++ b/src/tests/test_net_name_resolve.py @@ -18,10 +18,12 @@ # The naming decision and hardware discovery both live in # vyos.ifconfig.ifname_store and are covered by test_ifname_store.py. +import json import os import shutil import tempfile import unittest +from pathlib import Path from unittest import mock from helper import prepare_module @@ -200,6 +202,33 @@ class TestRefuseRenamesInUse(unittest.TestCase): self.assertEqual(refused, {'eth1': 'eth23'}) +class TestStatusAdopted(unittest.TestCase): + """A Hyper-V NIC re-created with a new MAC takes the vacant name. That is + neither a card found in its recorded slot nor anything the slot says, so + it is reported under its own key - vyos-router warns about it from there. + """ + + def test_an_adoption_is_reported_apart_from_slot_matches(self): + tmp = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, tmp) + status = os.path.join(tmp, 'status.json') + store = { + 'version': 1, + 'hardware': {}, + 'interfaces': {'eth3': 'ID_NET_NAME_MAC=enx000d3a47aa01'}, + } + report = {'adopted': {'eth3': '00:0d:3a:47:aa:01'}} + devices = [device('eth3', '00:0d:3a:47:aa:01')] + devices[0]['properties'] = {'ID_NET_NAME_MAC': 'enx000d3a47aa01'} + with mock.patch.object(resolver, 'status_file', Path(status)): + resolver.write_status(store, devices, {}, report, True) + with open(status) as f: + data = json.load(f) + self.assertEqual(data['adopted'], {'eth3': '00:0d:3a:47:aa:01'}) + self.assertEqual(data['matched_by_path'], {}) + self.assertIn('eth3', data['matched']) + + class TestMainInContainer(unittest.TestCase): """A container owns no NIC: its interfaces are runtime-created veth pairs with no backing bus device in sysfs and a host-assigned MAC that changes @@ -230,6 +259,134 @@ class TestMainInContainer(unittest.TestCase): self.assertFalse(resolver.status_file.exists()) +class TestHotplugPass(unittest.TestCase): + """A pass for hardware added to a running system hands no vacant name + over: the configuration under it would not be applied until the next boot. + """ + + def _unit(self, name): + with open(os.path.join(_here, '../systemd', name)) as f: + return f.read() + + def test_only_the_hotplug_unit_says_so(self): + self.assertIn('vyos-net-name-resolve.py --hotplug\n', + self._unit('vyos-net-name-resolve-hotplug.service')) + exec_start = [line for line in + self._unit('vyos-net-name-resolve.service').splitlines() + if line.startswith('ExecStart=')] + self.assertEqual(len(exec_start), 1) + self.assertNotIn('--hotplug', exec_start[0]) + + def _main(self, hotplug): + status_dir = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, status_dir, ignore_errors=True) + store = {'interfaces': {}, 'hardware': {}} + with mock.patch.object(resolver, 'is_running_as_container', + return_value=False), \ + mock.patch.object(resolver, 'status_file', + resolver.Path(status_dir) / 'status.json'), \ + mock.patch.object(resolver, 'load_store', return_value=store), \ + mock.patch.object(resolver, 'wait_for_hardware', + return_value=([], set())), \ + mock.patch.object(resolver, 'wait_for_settle', return_value=[]), \ + mock.patch.object(resolver, 'resolve', + return_value=({}, store, {})) as resolve, \ + mock.patch.object(resolver, 'save_store', return_value=False): + resolver.main(hotplug=hotplug) + return resolve.call_args + + def test_the_flag_is_read_from_the_command_line(self): + self.assertTrue(resolver.hotplug_requested(['--hotplug'])) + self.assertFalse(resolver.hotplug_requested([])) + + def test_a_hotplug_pass_keeps_the_boot_adoption_on_record(self): + # the boot pass gave eth2 to a new NIC; a later VF re-add starts a + # hotplug pass, which must neither forget nor repeat that + status_dir = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, status_dir, ignore_errors=True) + status = resolver.Path(status_dir) / 'status.json' + status.write_text(json.dumps({'adopted': {'eth2': '60:45:bd:00:00:01', + 'eth3': '60:45:bd:00:00:02'}})) + store = {'interfaces': {'eth2': 'ID_NET_NAME_MAC=enx6045bd000001', + 'eth3': 'ID_NET_NAME_MAC=enx6045bd000002'}, + 'hardware': {'eth2': '60:45:bd:00:00:01', + 'eth3': '60:45:bd:00:00:09'}} + with mock.patch.object(resolver, 'is_running_as_container', + return_value=False), \ + mock.patch.object(resolver, 'status_file', status), \ + mock.patch.object(resolver, 'load_store', return_value=store), \ + mock.patch.object(resolver, 'wait_for_hardware', + return_value=([], set())), \ + mock.patch.object(resolver, 'wait_for_settle', return_value=[]), \ + mock.patch.object(resolver, 'resolve', + return_value=({}, store, {})), \ + mock.patch.object(resolver, 'save_store', return_value=False), \ + mock.patch.object(resolver.logger, 'warning') as warning: + resolver.main(hotplug=True) + # eth3's NIC changed since: that record no longer holds + self.assertEqual(json.loads(status.read_text())['adopted'], + {'eth2': '60:45:bd:00:00:01'}) + self.assertFalse(any('had no NIC this boot' in str(c) + for c in warning.call_args_list)) + + def test_the_boot_pass_reports_only_its_own_adoptions(self): + # a status left from an earlier pass is not carried by a boot pass + status_dir = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, status_dir, ignore_errors=True) + status = resolver.Path(status_dir) / 'status.json' + status.write_text(json.dumps({'adopted': {'eth2': '60:45:bd:00:00:01'}})) + store = {'interfaces': {'eth2': 'ID_NET_NAME_MAC=enx6045bd000001'}, + 'hardware': {'eth2': '60:45:bd:00:00:01'}, 'hyperv': ['eth2']} + with mock.patch.object(resolver, 'is_running_as_container', + return_value=False), \ + mock.patch.object(resolver, 'status_file', status), \ + mock.patch.object(resolver, 'load_store', return_value=store), \ + mock.patch.object(resolver, 'wait_for_hardware', + return_value=([], set())), \ + mock.patch.object(resolver, 'wait_for_settle', return_value=[]), \ + mock.patch.object(resolver, 'resolve', + return_value=({}, store, {})), \ + mock.patch.object(resolver, 'save_store', return_value=False): + resolver.main(hotplug=False) + self.assertEqual(json.loads(status.read_text())['adopted'], {}) + + def _unrecorded_map_logs(self, present): + store = {'interfaces': {'eth0': 'ID_NET_NAME_MAC=enx000d3a000001'}, + 'hardware': {'eth0': '00:0d:3a:00:00:01'}} + devices = [{'properties': {'ID_NET_NAME_MAC': 'enx000d3a000001'}, + 'mac': '00:0d:3a:00:00:01', 'name': 'eth0'}] if present else [] + status_dir = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, status_dir, ignore_errors=True) + with mock.patch.object(resolver, 'is_running_as_container', + return_value=False), \ + mock.patch.object(resolver, 'status_file', + resolver.Path(status_dir) / 'status.json'), \ + mock.patch.object(resolver, 'load_store', return_value=store), \ + mock.patch.object(resolver, 'wait_for_hardware', + return_value=(devices, set())), \ + mock.patch.object(resolver, 'wait_for_settle', + return_value=devices), \ + mock.patch.object(resolver, 'resolve', + return_value=({}, store, {})), \ + mock.patch.object(resolver, 'save_store', return_value=False), \ + mock.patch.object(resolver.logger, 'info') as info: + resolver.main(hotplug=False) + return any('does not say which interfaces' in str(c) + for c in info.call_args_list) + + def test_an_unrecorded_map_is_mentioned_only_when_a_name_is_vacant(self): + # a fresh Azure deployment: cloud-init wrote the map, every NIC is + # there - nothing is held back, so nothing is said (5479.0.9 G1) + self.assertFalse(self._unrecorded_map_logs(present=True)) + self.assertTrue(self._unrecorded_map_logs(present=False)) + + def test_the_boot_pass_may_adopt(self): + self.assertTrue(self._main(False).kwargs['adopt']) + + def test_the_hotplug_pass_does_not(self): + self.assertFalse(self._main(True).kwargs['adopt']) + + class TestBootOrdering(unittest.TestCase): """The store lives in the config directory, which does not exist until vyos-router mounts (and, for an encrypted config volume, decrypts) it. So -- cgit v1.2.3 From 015663a3188727e2a16900cf5496c931770979a2 Mon Sep 17 00:00:00 2001 From: Christian Breunig Date: Wed, 28 Oct 2026 22:16:59 +0100 Subject: wwan: T3871: stop reporting a modem as replaced hardware every boot A modem that reports no permanent address is given a random one by its driver at every probe. That address was read as if it identified the hardware, so it never matched what was recorded and the modem was taken for a different card in the same socket on each boot, warning the operator to check a port nobody had touched. The map also kept being rewritten with the address of the boot before, so it could never settle. Keep no address for a modem, and compare one only where there is one. An address recorded by an earlier version is dropped on the next pass, so a machine already carrying one needs nothing done to it. A card whose address really did change is still reported. --- python/vyos/ifconfig/ifname_store.py | 14 +++++++--- src/tests/test_ifname_store.py | 52 +++++++++++++++++++++++++++++++++--- 2 files changed, 58 insertions(+), 8 deletions(-) (limited to 'src/tests/test_ifname_store.py') diff --git a/python/vyos/ifconfig/ifname_store.py b/python/vyos/ifconfig/ifname_store.py index 961a81d15..e9c204e6c 100644 --- a/python/vyos/ifconfig/ifname_store.py +++ b/python/vyos/ifconfig/ifname_store.py @@ -249,15 +249,18 @@ def discover_devices(sys_class_net: str = '/sys/class/net') -> list: if is_wireless(entry.name, sys_class_net): continue - mac = permanent_mac(entry.name, sys_class_net) - if not mac: + # a modem is given a random address at each probe, so it says nothing + # about which modem this is - there is nothing here worth recording + wwan = is_wwan(entry.name, sys_class_net) + mac = '' if wwan else permanent_mac(entry.name, sys_class_net) + if not mac and not wwan: continue devices.append({ 'name': entry.name, 'mac': mac, 'wireless': is_wireless(entry.name, sys_class_net), - 'wwan': is_wwan(entry.name, sys_class_net), + 'wwan': wwan, 'label_index': (port_label_index(entry.name, sys_class_net) if is_vmware_nic(entry.name, sys_class_net) else _LABEL_UNKNOWN), @@ -485,7 +488,8 @@ def resolve(devices: list, store: dict, adopt: bool = True) -> tuple: for name, key in entries.items(): for device in remaining: if (device_matches(device['properties'], key) - and seen.get(name, device['mac']) == device['mac'] + and (not device['mac'] + or seen.get(name, device['mac']) == device['mac']) and _name_fits(name, device)): claim(device, name, 'matched') break @@ -577,6 +581,8 @@ def resolve(devices: list, store: dict, adopt: bool = True) -> tuple: for name, device in assigned.items(): if device['mac']: new_hardware[name] = device['mac'] + else: + new_hardware.pop(name, None) plan = {device['name']: name for name, device in assigned.items() if device['name'] != name} diff --git a/src/tests/test_ifname_store.py b/src/tests/test_ifname_store.py index 635033fff..62eb4dc49 100644 --- a/src/tests/test_ifname_store.py +++ b/src/tests/test_ifname_store.py @@ -623,8 +623,9 @@ class TestWwanNaming(unittest.TestCase): MODEM_MAC = 'b6:53:dd:66:c2:98' def _modem(self, name='eth6'): + # no address: discovery keeps none for a modem properties = {'ID_NET_NAME_PATH': self.WWP, 'ID_BUS': 'usb'} - return {'name': name, 'mac': self.MODEM_MAC, 'wireless': False, + return {'name': name, 'mac': '', 'wireless': False, 'wwan': True, 'properties': properties, 'key': f'ID_NET_NAME_PATH={self.WWP}'} @@ -683,16 +684,41 @@ class TestWwanNaming(unittest.TestCase): # cannot carry it. Leave the name reserved and name the NIC as a NIC. store = {'version': STORE_VERSION, 'interfaces': {'wwan0': f'ID_NET_NAME_PATH={self.WWP}'}, - 'hardware': {'wwan0': self.MODEM_MAC}} + 'hardware': {}} nic = self._nic('eth0', self.WWP, 'aa:bb:cc:dd:ee:07') _, new_store, report = resolve([nic], store) self.assertEqual(report['replaced'], {}) - self.assertEqual(new_store['hardware']['wwan0'], self.MODEM_MAC) self.assertIn('eth0', new_store['interfaces']) self.assertEqual(new_store['interfaces']['wwan0'], f'ID_NET_NAME_PATH={self.WWP}') + def test_a_modem_is_not_reported_as_replaced(self): + # its address changes at every probe, so comparing it reported the + # modem as a different card in the same socket on every boot + store = {'version': STORE_VERSION, + 'interfaces': {'wwan0': f'ID_NET_NAME_PATH={self.WWP}'}, + 'hardware': {'wwan0': self.MODEM_MAC}} + plan, new_store, report = resolve([self._modem('wwan0')], store) + + self.assertEqual(plan, {}) + self.assertEqual(report['replaced'], {}) + self.assertIn('wwan0', report['matched']) + # and an address an earlier version recorded is dropped, so a store + # already carrying one settles on the next pass + self.assertNotIn('wwan0', new_store['hardware']) + + def test_a_card_whose_address_changed_is_still_replaced(self): + # the modem must not have weakened this: a card in a recorded slot + # with a different address is the case worth warning about + store = {'version': STORE_VERSION, + 'interfaces': {'eth0': 'ID_NET_NAME_PATH=enp3s0'}, + 'hardware': {'eth0': '00:f0:cb:fe:ba:99'}} + nic = self._nic('eth0', 'enp3s0', 'aa:bb:cc:dd:ee:07') + _, _, report = resolve([nic], store) + + self.assertIn('eth0', report['replaced']) + def test_an_ordinary_port_on_the_usb_bus_is_not_a_modem(self): # the key alone cannot tell them apart - a device falling back to # ID_PATH reads usb-... whether it is a modem or not @@ -1192,7 +1218,7 @@ class TestDiscoverDevices(unittest.TestCase): self.addCleanup(patch.stop) def make(name, mac=None, has_device=True, enslaved=False, is_vf=False, - master_is_device=False): + master_is_device=False, wwan=False): path = os.path.join(self.tmp, name) os.mkdir(path) if has_device: @@ -1210,7 +1236,11 @@ class TestDiscoverDevices(unittest.TestCase): if mac: with open(os.path.join(path, 'address'), 'w') as f: f.write(mac + '\n') + if wwan: + with open(os.path.join(path, 'uevent'), 'w') as f: + f.write('DEVTYPE=wwan\n') + self.make = make make('eth0', mac='aa:aa:aa:aa:aa:00') make('eth1', mac='aa:aa:aa:aa:aa:01') make('br0', mac='aa:aa:aa:aa:aa:99', has_device=False) @@ -1227,6 +1257,20 @@ class TestDiscoverDevices(unittest.TestCase): def test_only_physical_independent_interfaces(self): self.assertEqual(self._names(), {'eth0', 'eth1', 'eth8'}) + def test_a_modem_is_discovered_without_an_address(self): + # a modem is given a random address at each probe, so recording it + # would make the same modem look like different hardware every boot + self.make('wwan0', mac='b6:53:dd:66:c2:98', wwan=True) + devices = {d['name']: d for d in discover_devices(self.tmp)} + self.assertIn('wwan0', devices) + self.assertEqual(devices['wwan0']['mac'], '') + self.assertTrue(devices['wwan0']['wwan']) + + def test_a_card_with_no_address_is_still_skipped(self): + # only a modem is kept without one + self.make('eth5') + self.assertNotIn('eth5', self._names()) + def test_virtual_interfaces_excluded(self): self.assertNotIn('br0', self._names()) self.assertNotIn('lo', self._names()) -- cgit v1.2.3 From 6bef88ed9c11210855689bd78a39ee032fb27731 Mon Sep 17 00:00:00 2001 From: Christian Breunig Date: Wed, 28 Oct 2026 22:17:13 +0100 Subject: interfaces: T3871: keep the address policy the distribution ships Only the first matching link file is applied, and ours sets just the name, so the address policy shipped with the system never ran on any interface. Hardware with a permanent address was unaffected - there was nothing to apply - but a modem, which has none, was left on whatever random address its driver had picked that boot, a different one every time. Spell the policy out again. A permanent address is still kept where there is one, and a device without one is given an address derived from the machine and the device, which is the same on every boot of that machine. An address set through the CLI is applied later and is unaffected. --- python/vyos/ifconfig/ifname_store.py | 6 ++++++ src/tests/test_ifname_store.py | 20 ++++++++++++++++++++ 2 files changed, 26 insertions(+) (limited to 'src/tests/test_ifname_store.py') diff --git a/python/vyos/ifconfig/ifname_store.py b/python/vyos/ifconfig/ifname_store.py index e9c204e6c..b527c7348 100644 --- a/python/vyos/ifconfig/ifname_store.py +++ b/python/vyos/ifconfig/ifname_store.py @@ -628,6 +628,11 @@ def render_link(name: str, key: str, driver: str = '') -> str: A MAC is not unique to one interface: an accelerated-networking VF carries its NIC's MAC, and matched the NIC's file - udev then tried to give the VF the NIC's name. The driver tells them apart. + + Only the first matching file is applied, so this one shadows the address + policy the distribution ships. Spell it out again, or a device with no + permanent address - a modem - is left on the random one its driver gave + it, which is a different address on every boot. """ match_driver = f'Driver={driver}\n' if driver else '' return ( @@ -640,6 +645,7 @@ def render_link(name: str, key: str, driver: str = '') -> str: '\n' '[Link]\n' f'Name={name}\n' + 'MACAddressPolicy=persistent\n' ) diff --git a/src/tests/test_ifname_store.py b/src/tests/test_ifname_store.py index 62eb4dc49..40a975ba5 100644 --- a/src/tests/test_ifname_store.py +++ b/src/tests/test_ifname_store.py @@ -1061,6 +1061,26 @@ class TestLinkFiles(unittest.TestCase): self.assertIn('Name=eth1', body) self.assertIn('Type=ether', body) + def test_the_address_policy_is_spelled_out(self): + # only the first matching file is applied, so this one shadows the + # policy the distribution ships. Without it a modem keeps the random + # address its driver gives it, fresh on every boot. + for name in ('eth1', 'wwan0'): + body = render_link(name, 'ID_NET_NAME_PATH=wwp0s20f0u2u4c3') + self.assertIn('MACAddressPolicy=persistent', body) + + def test_a_file_without_the_policy_is_rewritten(self): + # the upgrade path: a box already carrying files from before only + # picks this up if an existing one is replaced + store = {'version': STORE_VERSION, + 'interfaces': {'wwan0': 'ID_NET_NAME_PATH=wwp0s20f0u2u4c3'}, + 'hardware': {}} + with tempfile.TemporaryDirectory() as d: + path = Path(d) / '10-vyos-wwan0.link' + path.write_text('[Match]\nType=wwan\n\n[Link]\nName=wwan0\n') + sync_link_files(store, Path(d)) + self.assertIn('MACAddressPolicy=persistent', path.read_text()) + def test_no_link_file_is_written_for_a_radio(self): # a .link keyed on a phy's address matches every interface created on # that phy, so writing one renamed the interface the operator had just -- cgit v1.2.3