cli: drop python-netifaces (LP: #2065870, LP: #2017585)#503
Merged
slyon merged 3 commits intocanonical:mainfrom Aug 28, 2024
Merged
cli: drop python-netifaces (LP: #2065870, LP: #2017585)#503slyon merged 3 commits intocanonical:mainfrom
slyon merged 3 commits intocanonical:mainfrom
Conversation
daniloegea
changed the title
daniloegea
force-pushed
the
get_rid_of_netifaces
branch
2 times, most recently
from
31ec163 to
79d1b6a
Compare
daniloegea
changed the title
daniloegea
force-pushed
the
get_rid_of_netifaces
branch
2 times, most recently
from
4e39aee to
6a93964
Compare
Contributor
slyon
changed the title
slyon
reviewed
Aug 21, 2024
Contributor
slyon
left a comment
slyon
left a comment
There was a problem hiding this comment.
Thank you very much. I generally like those changes, especially as netifaces is not maintained upstream anymore: al45tair/netifaces#78
I left a bunch of inline comments, some asking for some additional work (nothing big).
Also note, that after this change Netplan grows a new runtime dependency on ethtool.
daniloegea
force-pushed
the
get_rid_of_netifaces
branch
4 times, most recently
from
734b363 to
8f6d9a8
Compare
netifaces doesn't return the permanent MAC address of physical interfaces. This is causing issues when we try to find interfaces that are matched by MAC address but their MACs changed (because they were added to a bond for example). We only use it to retrieve the list of interfaces and their MACs so let's drop it and implement this functionalities in the CLI. get_interfaces() will simply call "ip link" and return a list with the interface names. get_interface_macaddress() will try to get the permanent MAC address and fallback to the transient address if it can't be found.
Skipping members of bonds when searching for interfaces that need to be renamed was done as a workaround for LP: #1802322. The actual problem was that members of bonds will have their MAC addresses changed to a common address when they are added as members. The renaming process looked for interfaces based on their transient MAC addresses not the permanent one. Because of that, it would confuse one interface by another and try to rename it. The process was changed to look for the permanent MAC address so this problem shouldn't happen. It also searches only for physical interfaces, which should have a permanent MAC address. Skipping the renaming of members of bonds will cause the backend to "forget" about it. Once the configuration is generated with a different name, the backend will look for the new name. As the interface will continue to have the previous name, it will not be managed anymore. Lets fix this by allowing the renaming these interfaces. If the system administrator changed the "set-name" setting, they probably want to get the interface renamed. This might cause a quick loss of connectivity on the interface being renamed. Drop is_composite_member(). This method is not needed anymore.
daniloegea
force-pushed
the
get_rid_of_netifaces
branch
from
8f6d9a8 to
6bcae2f
Compare
Contributor
Author
|
Thanks, Lukas. I addressed your comments and added some extra unit tests. |
slyon
approved these changes
Aug 28, 2024
Contributor
slyon
left a comment
slyon
left a comment
There was a problem hiding this comment.
Kudos! LGTM. Let's get it merged.
I prepared the new ethtool dependency in the Debian packaging, so we won't forget about it: https://salsa.debian.org/debian/netplan.io/-/commit/d6405858d585b833eb5d1c58cf43eff8568a16ed
(We still need to remember python-netifaces, though.)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
netifaces doesn't return the permanent MAC address of physical interfaces.
This causes a problem with bonds for example where all the interfaces in the bond will share the same MAC address. When
netplan applytries to find a unique matching based on the MAC returned by netifaces and the MAC set in the YAML, it will not be found.The
is_composite_member()method wasn't working properly as well. It was always returning False. In the end I stopped using it.Regarding skipping bond/bridge members during renames, that seems to be put in place as a workaround for LP: #1802322. The problem it fixed was happening because the code was matching interfaces by their non-permanent MAC address. When they are added to a bond, they will share the same MAC. The code was confusing one interface with another and trying to rename them.
By not renaming them the backend will "forget" about the interface. We will emit the new name to the backend config files but keep the interface with the old name in the system so it will not be found anymore. This is worse than causing a quick disconnection during the rename process as further changes to the interface will not be applied. The user would need to either rename it manually (which will cause a disconnection anyway) or reboot. I also found that networkd <= 255 was triggering a rename anyway when we called
udevadm testfromnetplan applyso we've been unintentionally allowing renames anyway...Fixes: LP#2065870
Description
Checklist
make checksuccessfully.make check-coverage).