cli:apply: fix potential race with rename/creation of netdevs and start networkd if off (LP: #1962095) #260
Conversation
slyon
force-pushed
the
slyon/fix-netplan-apply-race
branch
from
2952644
to
c14ad8a
Compare
Codecov Report
@@ Coverage Diff @@
## main #260 +/- ##
=======================================
Coverage 99.07% 99.07%
=======================================
Files 61 61
Lines 10860 10867 +7
=======================================
+ Hits 10760 10767 +7
Misses 100 100
Continue to review full report at Codecov.
|
slyon
changed the title
|
This is related to canonical/core-base#31 |
slyon
force-pushed
the
slyon/fix-netplan-apply-race
branch
3 times, most recently
from
a58fbc2
to
5286cd3
Compare
Calling networkctl_reload() before networkd_interfaces() makes sure that newly created netdevs will show up in the interface list. Making use of the index number (instead of interface name) ensures that renamed interfaces are properly reconfigured even if they changed their name.
… time When a MAC address is set for a bond, networkd will set the same MAC address to the enslaved interfaces. Therefore we cannot match on the original MAC for the ethernet device, as networkd will not find/manage that interface otherwise. https://systemd.network/systemd.netdev.html#MACAddress=
slyon
force-pushed
the
slyon/fix-netplan-apply-race
branch
from
5286cd3
to
d0e4d3e
Compare
|
@valentindavid just confirmed via PM that this PR fixes the issue they see on a real system. Tested via PPA https://launchpad.net/~slyon/+archive/ubuntu/netplan/+sourcepub/13303987/+listing-archive-extra |
| except subprocess.CalledProcessError: | ||
| # (re-)start systemd-networkd if it is not running, yet | ||
| logging.warning('Falling back to a hard restart of systemd-networkd.service') | ||
| utils.systemctl('restart', ['systemd-networkd.service'], sync=True) |
There was a problem hiding this comment.
Generally, I don't like try/except blocks, and especially when they wrap two commands without distinction if networkd is not running (hence networctl fails to connect to networkd) or if there is something broken with the config (and thus reconfigure of a device fails, but networkd is running and gets pointlessly restarted).
Can you check if systemd-networkd.service is running? If yes, execute networkctl reload & reconfigure. Otherwise restart networkd.
There was a problem hiding this comment.
Can you check if systemd-networkd.service is running? If yes, execute networkctl reload & reconfigure. Otherwise restart networkd.
That is actually how I had it implemented initially, but it does not catch cases where systemd-networkd.service is running but "stuck" (for some unkown reason), as reported in the original bug report at LP#1962095 (comment 4). Sure, networkd being stuck is not really a netplan problem... but using the try/except block we can hit two birds with one stone.
There was a problem hiding this comment.
This looks good to me, and feels sufficiently safe. Sure, as per Dimitri's comment it might lead to a case where there's a pointless restart, but I don't think that will be too frequent, and other ways might just unnecessarily make the logic more complicated. So I'd be +1 on getting this merged as-is. But I leave it up to you!
|
Thank you for the comments. As per my answer above, I'll merge this as-is. |
Description
Calling
networkctl_reload()beforenetworkd_interfaces()makes sure that newly created netdevs will show up in the interface list.Making use of the index number (instead of interface name) ensures that renamed interfaces are properly reconfigured even if they changed their name.
Starting networkd if not active is making sure we can process network changes even if systemd-networkd was stopped before (e.g. by subiquity), see LP#1962095
Checklist
make checksuccessfully.make check-coverage).