Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added pciid match support #375

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions doc/netplan-yaml.md
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,12 @@ Match devices by MAC when setting options like: `wakeonlan` or `*-offload`.
> A sequence of globs is supported, any of which must match.
> Matching on driver is *only* supported with networkd.

- **pciid** (scalar) – since **0.107**

> The PCI ID, corresponding to the `ID_PATH` udev properity. (`0000:98:00.1`)
> Matching on pci id is *only* supported with networkd.
> `udevadm info /sys/class/net/DEVICE_NAME`
Comment on lines +108 to +112
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick (non-blocking): Version needs to be bumped.

question: Also, is there any possibility of doing the same for the NetworkManager backend? The match stanza is a very central piece of Netplan's configuration and we should try not to diverge between the backends here.


Examples:

- All cards on second PCI bus:
Expand Down Expand Up @@ -145,6 +151,15 @@ Match devices by MAC when setting options like: `wakeonlan` or `*-offload`.
name: en*
```

- Matching with PCI ID `0000:98:00.1`:
```yaml
network:
ethernets:
nic0:
match:
pciid: 0000:98:00.1
```

- **set-name** (scalar)

> When matching on unique properties such as path or MAC, or with additional
Expand Down
2 changes: 1 addition & 1 deletion include/netplan.h
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ NETPLAN_PUBLIC gboolean
netplan_netdef_has_match(const NetplanNetDefinition* netdef);

NETPLAN_PUBLIC gboolean
netplan_netdef_match_interface(const NetplanNetDefinition* netdef, const char* name, const char* mac, const char* driver_name);
netplan_netdef_match_interface(const NetplanNetDefinition* netdef, const char* name, const char* mac, const char* driver_name, const char* pciid);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

todo: We cannot just change public API like this, but would rather need to introduce a new netplan_netdef_match_interface2() function. Keeping the original function backwards compatible.


/********** Old API below this ***********/

Expand Down
30 changes: 29 additions & 1 deletion netplan/cli/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -174,14 +174,42 @@ def get_interface_macaddress(interface):
return link.get('addr', '')


def get_interface_pciid(interface): # pragma: nocover (covered in autopkgtest)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: Can we apply some test-mocking to get this net method covered by unit tests?

devdir = os.path.join('/sys/class/net', interface)
pciid = ''
try:
uevent = os.path.realpath(os.path.join(devdir, 'device', 'uevent'))
if os.path.exists(uevent):
with open(uevent) as f:
contents = f.read().splitlines()
f.close()
else:
return None

for line in contents:
if line.split('=')[0] == 'PCI_SLOT_NAME':
pciid = line.split('=')[1] # 0000:4b:00.0
break
except IOError as e:
logging.debug('Cannot replug %s: cannot read link %s: %s', interface, devdir, str(e))
return None

# 0000:4b:00.0
if re.match('[0-9a-f]+:[0-9a-f]+:[0-9a-f]+.+', pciid, re.IGNORECASE) is None:
return None

return pciid


def find_matching_iface(interfaces: list, netdef):
assert isinstance(netdef, np.NetDefinition)
assert netdef.has_match

matches = list(filter(lambda itf: netdef.match_interface(
itf_name=itf,
itf_driver=get_interface_driver_name(itf),
itf_mac=get_interface_macaddress(itf)), interfaces))
itf_mac=get_interface_macaddress(itf),
itf_pciid=get_interface_pciid(itf)), interfaces))

# Return current name of unique matched interface, if available
if len(matches) != 1:
Expand Down
5 changes: 3 additions & 2 deletions netplan/libnetplan.py
Original file line number Diff line number Diff line change
Expand Up @@ -598,12 +598,13 @@ def embedded_switch_mode(self):
def delay_virtual_functions_rebind(self):
return bool(lib.netplan_netdef_get_delay_virtual_functions_rebind(self._ptr))

def match_interface(self, itf_name=None, itf_driver=None, itf_mac=None):
def match_interface(self, itf_name=None, itf_driver=None, itf_mac=None, itf_pciid=None):
return bool(lib.netplan_netdef_match_interface(
self._ptr,
itf_name and itf_name.encode('utf-8'),
itf_mac and itf_mac.encode('utf-8'),
itf_driver and itf_driver.encode('utf-8')))
itf_driver and itf_driver.encode('utf-8'),
itf_pciid and itf_pciid.encode('utf-8')))

@property
def vf_count(self):
Expand Down
1 change: 1 addition & 0 deletions src/abi.h
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,7 @@ struct netplan_net_definition {
char* driver;
char* mac;
char* original_name;
char* pciid;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

todo: This has the potential of breaking ABI and we'd need to move the new field to the very end of the netplan_net_definition struct, e.g. called match_pciid.

} match;
gboolean has_match;
gboolean wake_on_lan;
Expand Down
5 changes: 3 additions & 2 deletions src/generate.c
Original file line number Diff line number Diff line change
Expand Up @@ -174,13 +174,14 @@ find_interface(gchar* interface, GHashTable* netdefs)
}
else {
const NetplanNetDefinition *nd = (NetplanNetDefinition *)g_ptr_array_index (found, 0);
g_printf("id=%s, backend=%s, set_name=%s, match_name=%s, match_mac=%s, match_driver=%s\n",
g_printf("id=%s, backend=%s, set_name=%s, match_name=%s, match_mac=%s, match_driver=%s, match_pciid=%s\n",
nd->id,
netplan_backend_name(nd->backend),
nd->set_name,
nd->match.original_name,
nd->match.mac,
nd->match.driver);
nd->match.driver,
nd->match.pciid);
}

ret = EXIT_SUCCESS;
Expand Down
1 change: 1 addition & 0 deletions src/netplan.c
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ write_match(yaml_event_t* event, yaml_emitter_t* emitter, const NetplanNetDefini
YAML_MAPPING_OPEN(event, emitter);
YAML_NONNULL_STRING(event, emitter, "name", def->match.original_name);
YAML_NONNULL_STRING(event, emitter, "macaddress", def->match.mac)
YAML_NONNULL_STRING(event, emitter, "pciid", def->match.pciid)
if (def->match.driver && strchr(def->match.driver, '\t')) {
gchar **split = g_strsplit(def->match.driver, "\t", 0);
YAML_SCALAR_PLAIN(event, emitter, "driver");
Expand Down
10 changes: 9 additions & 1 deletion src/networkd.c
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,11 @@ append_match_section(const NetplanNetDefinition* def, GString* s, gboolean match
*/
g_string_append_printf(s, "PermanentMACAddress=%s\n", def->match.mac);
}

// Append pciid to match interface
if (def->match.pciid)
g_string_append_printf(s, "Path=pci-%s\n", def->match.pciid);

/* name matching is special: if the .link renames the interface, the
* .network has to use the renamed one, otherwise the original one */
if (!match_rename && def->match.original_name)
Expand Down Expand Up @@ -997,7 +1002,7 @@ write_rules_file(const NetplanNetDefinition* def, const char* rootdir)
* renamed name. If you match by the unstable kernel name, the
* device no longer has that name when udevd reads the file, so
* the rule doesn't fire. So only support mac and driver. */
if (!def->set_name || (!def->match.mac && !def->match.driver))
if (!def->set_name || (!def->match.mac && !def->match.driver && !def->match.pciid))
return;

/* build file contents */
Expand All @@ -1014,6 +1019,9 @@ write_rules_file(const NetplanNetDefinition* def, const char* rootdir)
if (def->match.mac)
g_string_append_printf(s, "ATTR{address}==\"%s\", ", def->match.mac);

if (def->match.pciid)
g_string_append_printf(s, "ENV{ID_PATH}==\"pci-%s\", ", def->match.pciid);

g_string_append_printf(s, "NAME=\"%s\"\n", def->set_name);

orig_umask = umask(022);
Expand Down
9 changes: 7 additions & 2 deletions src/nm.c
Original file line number Diff line number Diff line change
Expand Up @@ -1018,7 +1018,7 @@ netplan_state_finish_nm_write(
guint unmanaged = nd->backend == NETPLAN_BACKEND_NM ? 0 : 1;

/* Special case: manage or ignore any device of given type on empty "match: {}" stanza */
if (nd->has_match && !nd->match.driver && !nd->match.mac && !nd->match.original_name) {
if (nd->has_match && !nd->match.driver && !nd->match.mac && !nd->match.original_name && !nd->match.pciid) {
nm_type = type_str(nd);
g_assert(nm_type);
g_string_append_printf(nm_conf, "[device-netplan.%s.%s]\nmatch-device=type:%s\n"
Expand Down Expand Up @@ -1050,7 +1050,7 @@ netplan_state_finish_nm_write(
* from the "match" stanza (e.g. original_name/mac/drivers)
* This will match the "old" interface (i.e. original MAC and/or
* interface name) if it got changed */
if (nd->has_match && (nd->match.original_name || nd->match.mac || nd->match.driver)) {
if (nd->has_match && (nd->match.original_name || nd->match.mac || nd->match.driver || nd->match.pciid)) {
// match on original name glob
// TODO: maybe support matching on multiple name globs in the future (like drivers)
g_string_append(udev_rules, prefix);
Expand All @@ -1077,6 +1077,11 @@ netplan_state_finish_nm_write(
g_string_append_printf(udev_rules, " ENV{ID_NET_DRIVER}==\"%s\",", drivers);
g_free(drivers);
}

// match pciid
if (nd->match.pciid)
g_string_append_printf(udev_rules, " ENV{ID_PATH}==\"%s\",", nd->match.pciid);

g_string_append(udev_rules, suffix);
}
}
Expand Down
1 change: 1 addition & 0 deletions src/parse.c
Original file line number Diff line number Diff line change
Expand Up @@ -857,6 +857,7 @@ static const mapping_entry_handler match_handlers[] = {
{"driver", YAML_NO_NODE, {.variable=handle_match_driver}},
{"macaddress", YAML_SCALAR_NODE, {.generic=handle_netdef_mac}, netdef_offset(match.mac)},
{"name", YAML_SCALAR_NODE, {.generic=handle_netdef_id}, netdef_offset(match.original_name)},
{"pciid", YAML_SCALAR_NODE, {.generic=handle_netdef_str}, netdef_offset(match.pciid)},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

todo: We'd probably want to have a more specific handler here, that validates the input string is an actual PCI ID.

{NULL}
};

Expand Down
1 change: 1 addition & 0 deletions src/types.c
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,7 @@ reset_netdef(NetplanNetDefinition* netdef, NetplanDefType new_type, NetplanBacke
FREE_AND_NULLIFY(netdef->match.driver);
FREE_AND_NULLIFY(netdef->match.mac);
FREE_AND_NULLIFY(netdef->match.original_name);
FREE_AND_NULLIFY(netdef->match.pciid);
netdef->has_match = FALSE;
netdef->wake_on_lan = FALSE;
netdef->wowlan = 0;
Expand Down
7 changes: 6 additions & 1 deletion src/util.c
Original file line number Diff line number Diff line change
Expand Up @@ -824,7 +824,7 @@ netplan_copy_string(const char* input, char* out_buffer, size_t out_size)
}

gboolean
netplan_netdef_match_interface(const NetplanNetDefinition* netdef, const char* name, const char* mac, const char* driver_name)
netplan_netdef_match_interface(const NetplanNetDefinition* netdef, const char* name, const char* mac, const char* driver_name, const char* pciid)
{
if (!netdef->has_match)
return !g_strcmp0(name, netdef->id);
Expand Down Expand Up @@ -855,6 +855,11 @@ netplan_netdef_match_interface(const NetplanNetDefinition* netdef, const char* n
return matches_driver;
}

if (netdef->match.pciid) {
if (g_strcmp0(netdef->match.pciid, pciid))
return FALSE;
}

return TRUE;
}

Expand Down
2 changes: 2 additions & 0 deletions tests/generator/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@
[Service]\nType=oneshot\nTimeoutStartSec=10s\nExecStart=/usr/sbin/netplan apply --only-ovs-cleanup\n'
UDEV_MAC_RULE = 'SUBSYSTEM=="net", ACTION=="add", DRIVERS=="%s", ATTR{address}=="%s", NAME="%s"\n'
UDEV_NO_MAC_RULE = 'SUBSYSTEM=="net", ACTION=="add", DRIVERS=="%s", NAME="%s"\n'
UDEV_PCIID_RULE = 'SUBSYSTEM=="net", ACTION=="add", DRIVERS=="%s", ENV{ID_PATH}=="pci-%s", NAME="%s"\n'
UDEV_SRIOV_RULE = 'ACTION=="add", SUBSYSTEM=="net", ATTRS{sriov_totalvfs}=="?*", RUN+="/usr/sbin/netplan apply --sriov-only"\n'
ND_WITHIPGW = '[Match]\nName=%s\n\n[Network]\nLinkLocalAddressing=ipv6\nAddress=%s\nAddress=%s\nGateway=%s\n\
ConfigureWithoutCarrier=yes\n'
Expand Down Expand Up @@ -102,6 +103,7 @@
NM_UNMANAGED_MAC = 'SUBSYSTEM=="net", ACTION=="add|change|move", ATTR{address}=="%s", ENV{NM_UNMANAGED}="1"\n'
NM_MANAGED_DRIVER = 'SUBSYSTEM=="net", ACTION=="add|change|move", ENV{ID_NET_DRIVER}=="%s", ENV{NM_UNMANAGED}="0"\n'
NM_UNMANAGED_DRIVER = 'SUBSYSTEM=="net", ACTION=="add|change|move", ENV{ID_NET_DRIVER}=="%s", ENV{NM_UNMANAGED}="1"\n'
NM_UNMANAGED_PCIID = 'SUBSYSTEM=="net", ACTION=="add|change|move", ENV{ID_PATH}=="%s", ENV{NM_UNMANAGED}="1"\n'

WOKE_REPLACE_REGEX = ' +# wokeignore:rule=[a-z]+'

Expand Down
23 changes: 22 additions & 1 deletion tests/generator/test_ethernets.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@

from .base import TestBase, ND_DHCP4, UDEV_MAC_RULE, UDEV_NO_MAC_RULE, UDEV_SRIOV_RULE, \
NM_MANAGED, NM_UNMANAGED, NM_MANAGED_MAC, NM_UNMANAGED_MAC, \
NM_MANAGED_DRIVER, NM_UNMANAGED_DRIVER
NM_MANAGED_DRIVER, NM_UNMANAGED_DRIVER, NM_UNMANAGED_PCIID, UDEV_PCIID_RULE


class TestNetworkd(TestBase):
Expand Down Expand Up @@ -164,6 +164,27 @@ def test_eth_match_by_mac_rename(self):
self.assert_nm(None)
self.assert_nm_udev(NM_UNMANAGED % 'lom1' + NM_UNMANAGED_MAC % '11:22:33:44:55:66')

def test_eth_match_by_pciid_rename(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: Thanks for providing some test cases! I'd love to see an integration test along the lines of tests/integration/ethernets.py:test_rename_interfaces(), too (if possible).

self.generate('''network:
version: 2
ethernets:
def1:
match:
pciid: 0000:98:00.1
set-name: lom1''')

self.assert_networkd({'def1.link': '[Match]\nPath=pci-0000:98:00.1\n\n[Link]\nName=lom1\nWakeOnLan=off\n',
'def1.network': '''[Match]
Path=pci-0000:98:00.1
Name=lom1

[Network]
LinkLocalAddressing=ipv6
'''})
self.assert_networkd_udev({'def1.rules': (UDEV_PCIID_RULE % ('?*', '0000:98:00.1', 'lom1'))})
self.assert_nm(None)
self.assert_nm_udev(NM_UNMANAGED % 'lom1' + NM_UNMANAGED_PCIID % '0000:98:00.1')

# https://bugs.launchpad.net/netplan/+bug/1848474
def test_eth_match_by_mac_infiniband(self):
self.generate('''network:
Expand Down