Check if the name to set to an interface is already an altname (LP: #1994822) #292
Conversation
This prevents an exception caused by trying to set a name that already exists in an interface
Also call static method using class name instead of self instance
To comply with codestyle regulations
slyon
changed the title
There was a problem hiding this comment.
Thank you very much for contributing to Netplan and providing this clean patch + unit-test!
CI is looking good!
I left a few comments inline. One big question mark is if skipping the rename is the correct solution at all, or if we should rather delete the existing altname and execute the rename afterwards? What do you think?
| @@ -237,6 +243,9 @@ def command_apply(self, run_generate=True, sync=False, exit_on_error=True, state | |||
| if iface in devices and new_name in devices_after_udev: | |||
| logging.debug('Interface rename {} -> {} already happened.'.format(iface, new_name)) | |||
| continue # re-name already happened via 'udevadm test' | |||
| if new_name in NetplanApply.get_alt_names(iface): | |||
| logging.debug('Interface name {} already present as altname on {}.'.format(new_name, iface)) | |||
There was a problem hiding this comment.
I think this should at least be at "logging.warning" level, as Netplan was asked to rename the interface, but won't do it.
But I wonder if this is the correct thing to do at all (i.e. skipping the rename)?
Netplan was asked to rename the interface, so we probably shouldn't just skip it.
Maybe the better approach would be to delete the altname, then rename the interface to that name afterwards.
ip link property del altname NEW_NAME dev IFACE
ip link set dev IFACE name NEW_NAME
What do you think?
There was a problem hiding this comment.
In theory the altname works seamlessly as if it where the interface name. You can run commands using both names and should be the same. They exist to allow having names longer that 16 as the "main" name can't grow more than that without breaking compatibility. See here for the comments on the kernel patch that added them: https://lwn.net/Articles/794289/
That is why I think having the name in the altname should be essentially the same as having it in the name. Any commands that rely on the name of the interface being the string in the set-name parameter should work anyway. That's also why I treated it as the other case where the desired name was already present.
There was a problem hiding this comment.
Thanks for your comment and further reference/context. After consulting some other netplan devs for a 2nd opinion, I feel like we should really go the extra mile here, i.e. drop the alt name and actually re-name the interface properly.
"working seamlessly in theory" isn't really enough, as we don't know the side-effects for edge cases. On the kernel commit (and during my testing) it showed that e.g. /sys/class/net/IFNAME/* is not yet supported/available, so we couldn't rely on the interface name anymore to query sysfs data. Also, the state of a running system vs re-booted system could diverge (netplan apply ignoring an interface rename at runtime, because an altname already exists VS the interface being actually renamed at the next reboot).
In addition to that, if netplan would add support for altnames in the future, we would have a looming conflict of real interface name vs altname here if we accept to ignore renaming interfaces if they're covered by an altname.
For those reasons I'd like to ask for changing the code to actually drop the altname and do the proper interface rename, instead.
There was a problem hiding this comment.
The issue there is that the "rename the interface properly" will fail on case for which the altname was specifically created: interface names in excess of 16 characters.
The real purpose of the altname is to provide an interface name that raises the IFNAMSIZ (16) character limit of the "regular" name, because some interface names generated by the systemd persistent naming exceed 16 characters. If netplan attempts to transfer one of those names from altname to "regular" name, it will fail.
Also, what would netplan do for the case of multiple altnames?
There was a problem hiding this comment.
Adding to the discussion, I tried changing this patch to delete the altname in lines 250-251 instead of just skipping the step of the loop. This works fine running netplan apply after booting but the interface does not get the altname as the "regular" name on boot. After booting the interface remains as before with the desired name as altname and a different "real" name. So this will also be a case where the could be differences between a running vs re-booted system
There was a problem hiding this comment.
The issue there is that the "rename the interface properly" will fail on case for which the altname was specifically created: interface names in excess of 16 characters.
The real purpose of the altname is to provide an interface name that raises the IFNAMSIZ (16) character limit of the "regular" name, because some interface names generated by the systemd persistent naming exceed 16 characters. If netplan attempts to transfer one of those names from altname to "regular" name, it will fail.
I don't think so, as we're not trying to transfer an arbitrary altname to a regular name, but we're only trying to rename an interface to the new name configured in Netplan's "set-name" setting, which is supposed to be a proper interface name and Netplan SHOULD actually validate this value fits the IFNAMESIZ (16) character limit (in validation.c); it doesn't currently validate that, though.
Also, what would netplan do for the case of multiple altnames?
We have the current interface name "OLD_NAME", the requested new interface name "NEW_NAME" and can have multiple altnames. We want to drop OLD_NAME and replace it by NEW_NAME. If NEW_NAME is already taken by an altname, we drop that specific altname and do the OLD_NAME -> NEW_NAME rename of the interface. We do not touch any other altnames, and keep them as is.
There was a problem hiding this comment.
Adding to the discussion, I tried changing this patch to delete the altname in lines 250-251 instead of just skipping the step of the loop. This works fine running
netplan applyafter booting but the interface does not get the altname as the "regular" name on boot. After booting the interface remains as before with the desired name as altname and a different "real" name. So this will also be a case where the could be differences between a running vs re-booted system
How are those altnames set-up in your case? Are they set by the kernel itself, i.e. are available directly at boot time?
The patch proposed in this PR handles the "rename at runtime" case only, via netplan apply cli. Renaming of interfaces at boot time happens through udev and we might need to update that code path accordingly as well (see networkd.c, write_rules_file())
There was a problem hiding this comment.
I believe the altname in question is coming from systemd (udev), as /lib/systemd/network/99-default.link contains
NamePolicy=keep kernel database onboard slot path
AlternativeNamesPolicy=database onboard slot path
Maybe netplan is just a victim of the AlternativeNamesPolicy here?
There was a problem hiding this comment.
As Jay said the altnames are added because of the AlternativeNamesPolicy. But systemd seems to take the same path as this patch when netplan's .link file specifies a name that is also an altname because of the policy (ie. it leaves the name as an altname). When a different name is configured in the .link file (one that is not an altname) the interface ¨regular" name gets updated.
There was a problem hiding this comment.
Yes, sounds like Netplan is a victim of the AlternativeNamesPolicy. I need to read a bit more about the naming policies... I wonder if we could change those when writing a .link file for renaming the interface (but I remember vaguely those can only be applied once during bootup...)?
@drencrom Are you sure about systemd/udev behaving the same way? I think this might just be their "error case", see:
https://github.com/systemd/systemd/blob/main/src/libsystemd/sd-netlink/netlink-util.c#L33
What they're actually trying to do is to delete the AlternativeName, then renaming the interface (as suggested above). But this works only if the interface is DOWN and they are not supposed to bring down the interface themselves... So they keep the name in case the interface is UP already.
Netplan OTOH is in control of the networking stack and can cycle an interface DOWN/UP if needed, so should do the right think and clear the alternative name, to set it as a proper new interface name.
For reference, see also: systemd/systemd#25221 (Ubuntu Foundations currently working on a similar issue with upstream systemd)
Fix unit test to adapt to new implementation
slyon
added
the
community
When netplan has a set-name configuration option and that name is already defined as an altname in the interface an exception is thrown and the apply process ends prematurely. This patch checks this case and prevents that exception from being thrown. A debug message is written instead.
Closes-Bug: LP#1994822