Handling of interface renaming by kernel. - #171
Open
joeysk2012 wants to merge 1 commit into
Open
Conversation
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
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.
Issue #, if available:
#166
Description of changes:
Perform cleanup of systemd interface configs, when the udev rule remove is triggered. There were orphaned systemd unit files which could potentially affect the operation of the network. In this specific case, an interface ens6 was detached and attached again as ens7 (due to Hypervisor selecting a different PCI slot) with the same MAC. From there, any new changes from the control plane would only affect the config for ens6 as it is lower in alpha-numeric order, when ens6 does not even exist anymore.
Other options for the fix were considered but they were deemed too risky:
Option 1:
NamePolicy=mac(systemd.linkfile)Ship a new
.linkfile scopingNamePolicy=mac(or similar) to ENA/ixgbevfinterfaces, so udev derives the interface name from the MAC address instead of
the PCI slot. Since the MAC is stable for a given ENI (confirmed by test
evidence — same MAC across the
ens6/ens7rename), this prevents the renameat the source.
Benefits
re-attach, so no downstream config (ours or anyone else's) is ever orphaned.
Risks
AL2023 instance gets a new naming scheme. Any customer automation, monitoring,
or scripts hardcoding slot-based names (
ens5,ens6, ...) would seedifferent names after this ships.
.link-file-driven renames only fully take effect after a networkd reload +udev "move" event. amazon-ec2-net-utils' own
add_altnames()explicitlyavoids this mechanism today for the (much smaller) altname use case, citing
that overhead — the same concern applies here, at a larger scale, and could
introduce a new timing race with
setup-policy-routes.sh start'swait-for-sysfs-node loop.
naming scheme (v252) or other higher-priority
.link/naming rules that mightalready claim these interfaces.
Verdict: Highest risk / highest blast radius. Worth prototyping and testing
in isolation before proposing upstream, but not implemented in this pass.
Option 2: Match
[Match]stanza by interfaceName=instead ofMACAddress=Considered switching the generated
.networkconfig's[Match]stanza fromMACAddress=${ether}toName=${iface}, on the theory that "matching by name"might sidestep the rename problem.
Correction during discussion: the
[Match]stanza already usesMACAddress=${ether}today — only the file path / directory name / systemdunit instance (
70-<iface>.network,policy-routes@<iface>.service) is keyedby interface name, not the match rule itself.
Benefits
Name=ens6rule stops matching anything the instant the ENI renames toens7), so the underlying instability is unaddressed.Risks
designed to prevent (see
cd0f361, "Don't reconfigure interfaces on service'stop'"): if a different ENI later reuses the same slot/name, a
Name=ens6-matched leftover config (secondary IPs, routes, policy rules) fromthe previous, unrelated ENI would incorrectly apply to it.
MACAddress=matching prevents this by construction;
Name=matching would not.config at least keeps functioning correctly across a rename (only the on-disk
label is orphaned); with
Name=matching the config would stop matchinganything the moment the rename occurs.
Verdict: Rejected. Not implemented — this would reopen a closed correctness
issue for no benefit.
(Note: a related, narrower idea — keying the config file path by MAC
instead of interface name, while keeping
[Match] MACAddress=as-is — was alsodiscussed. It would prevent orphaning more structurally than Option 3 below, but
requires touching ~36 call sites across
lib.sh, breaks the documentednetworkctl status ens5→/run/systemd/network/70-ens5.networkconvention,and touches the systemd unit instantiation key. Assessed as moderate-to-high
risk and deprioritized in favor of Option 3.)
Option 3: Clean up stale config on interface removal (implemented)
Wire the udev
removeevent to deterministically delete the departinginterface's
70-<iface>.networkfile and drop-in directory, scoped to exactlythat interface name — no broad scan, no reload.
History: this cleanup logic existed once (pre-
cd0f361) viaExecStop=setup-policy-routes %i stop, triggering a globalnetworkctl reloadon every stop. That reload was found to reset conntrack state forunrelated live interfaces, causing 100% packet loss for established
connections on Docker-bridge-networking hosts.
cd0f361removed the wiringentirely to fix that regression, leaving the
removecleanup logic in thescript but orphaned (nothing calls it).
Benefits
70-<oldname>.networkfiles after a rename.ExecStop=onpolicy-routes@.serviceto call the existingremove)branch, and removesthat branch's call to
register_networkd_reloader/touch "$reload_flag"soit never triggers a reload.
cd0f361regression: the fix deliberately does notcall
networkctl reloadorreconfigure. This is safe because by the timeudev's
removeaction fires, the kernel has already destroyed the link — itsroutes/rules/addresses are already torn down independently of these config
files, so there is nothing live left to reconfigure and no reload-driven
conntrack risk for other interfaces.
OS just reported as removed. (An earlier draft additionally scanned all
70-*.networkfiles and reaped any whose name no longer resolved viaip link show— this broader sweep was deliberately dropped as unnecessarilyrisky; the scoped, event-driven deletion is sufficient and does not depend on
a live-interface heuristic that could misfire.)
Risks
ens6/ens7will still occur; this onlyensures the old name's config doesn't linger.
ExecStop=firing reliably for aType=oneshot, RemainAfterExit=yesunit stopped via
systemctl disable --now(the exact mechanism the udevremoverule already uses) — not yet validated on a live host.Verdict: Implemented on
iface-name-inconsistent. Lowest risk of the threeoptions, directly addresses the observed gap, and does not touch the MAC-based
[Match]matching that is already correct.