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.

Sign up for GitHub

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

Jump to bottom

Added pciid match support #375

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Added pciid match support #375

wants to merge 3 commits into from
+101 −11

Conversation

fatihusta
Copy link

@fatihusta fatihusta commented Jul 6, 2023 •
edited
Loading

Description

Netplan doesn't match mac address of ethernet when using bond. So member ethernet doesn't up.

This PR fixes ethernet up problem, but still prints warning message(below).

I try to add PCI ID match support for better renaming to the ethernet.

[]
Cannot find unique matching interface for enp152s0f0np0
[]
Cannot find unique matching interface for eno12399np0
[]
Cannot find unique matching interface for enp152s0f1np1
[]
Cannot find unique matching interface for eno12409np1

Checklist

  • Runs make check successfully.
  • Retains 100% code coverage (make check-coverage).
  • New/changed keys in YAML format are documented.
  • (Optional) Adds example YAML for new feature.
  • (Optional) Closes an open bug in Launchpad.

Also may fixes this issue: https://bugs.launchpad.net/netplan/+bug/1608447

@slyon slyon added the schema change This PR includes a change to netplan's YAML schema, which needs sign-off label Jul 10, 2023
@slyon
Copy link
Collaborator

slyon commented Aug 16, 2023

Sorry, this is taking much longer than anticipated. I like the idea of having pciid matching capabilities. But it touches core components of Netplan and I need to find enough time to do a proper review on this.

@slyon slyon added the community This PR has been proposed by somebody outside of the Netplan team and roadmap commitments. label Aug 16, 2023
slyon
slyon requested changes Sep 4, 2024
View reviewed changes
Copy link
Collaborator

@slyon slyon left a comment

Choose a reason for hiding this comment

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

My apologies, this PR fell through the cracks. I've now done an initial high-level round of review, as this is touching quite some core parts of Netplan and the original issue seems to be fixed as of recently (#503).

Please let me know if you're still interested in driving this forward, so I can do a more detailed review and talk to our architect about the new YAML schema.

include/netplan.h
@@ -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.

src/abi.h
@@ -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.

doc/netplan-yaml.md
Comment on lines +108 to +112
- **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`
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.

src/parse.c
@@ -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.

tests/generator/test_ethernets.py
@@ -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).

netplan/cli/utils.py
@@ -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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slyon slyon slyon requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
community This PR has been proposed by somebody outside of the Netplan team and roadmap commitments. schema change This PR includes a change to netplan's YAML schema, which needs sign-off
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fatihusta @slyon