apply: make sure that networkd is restarted when needed

[alfonsosanchezbeato]

This PR fixes these two problems:

• It restarts networkd instead of reload/reconfigure
  Due to systemd-networkd bugs, reload/reconfigure was not working in
  some cases. Instead, restart the service if the configuration has
  changed (LP#2058976).

• It compares full configuration to decide to restart or not networkd.
  Copy the current networkd configuration and then call generate so we
  know more accurately if systemd-networkd needs to be restarted.

[slyon]

alfonsosanchezbeato: to be clear, the networkctl reload/reconfigure commands seem to work, but after 6-8 minutes the state goes back to the one expressed by the old .network files
alfonsosanchezbeato: you can even force the revert to happen sooner with sudo udevadm trigger, with that it tries to get control back too

[daniloegea]

Do you know how safe a networkd restart is? I mean, I'm trying to determine what kind of change in behavior it would introduce from the point of view of the user. I'm concerned it would cause this kind of problem: https://discuss.linuxcontainers.org/t/systemd-update-networkd-restart-breaks-routed-containers/9979

[slyon]

This is going backwards wrt. #200

[alfonsosanchezbeato]

This is going backwards wrt. #200

Right, as it looks like networkd is not really respecting the reload/reconfigure commands, at least on 20.04.

[alfonsosanchezbeato]

Do you know how safe a networkd restart is? I mean, I'm trying to determine what kind of change in behavior it would introduce from the point of view of the user. I'm concerned it would cause this kind of problem: https://discuss.linuxcontainers.org/t/systemd-update-networkd-restart-breaks-routed-containers/9979

Restarting it should be in theory fine, this is how it was being done in the past. The issue you shared seemed to happen only on 18.04 I think.

[alfonsosanchezbeato]

I have tested this through different UC versions, and I see:

• 20.04 systemd-networkd has the bug
• 22.04 systemd-networkd has the bug
• 24.04 systemd-networkd has apparently fixed the bug

Therefore, it would make sense to backport b9ef830 to netplan in 20/22, and remove it from this PR, but keep 8dabb54. Are there branches where the commit could be proposed for 20/22?

An alternative could be to find out the patch fixing the issue in systemd-networkd and SRU to 20/22, but being systemd it might take longer than doing an SRU of netplan, I think.

[slyon]

Thanks a lot for your continued investigation @alfonsosanchezbeato !

An alternative could be to find out the patch fixing the issue in systemd-networkd and SRU to 20/22, but being systemd it might take longer than doing an SRU of netplan, I think.

Do we have a clean reproducer? I could bisect systemd to find the commit fixing the behaviour. I'd prefer fixing it properly in systemd that backporting a workaround.

Therefore, it would make sense to backport b9ef830 to netplan in 20/22, and remove it from this PR, but keep 8dabb54. Are there branches where the commit could be proposed for 20/22?

I very much like what you did with dircmp there, this makes the code so much cleaner! We do not have upstream branches for Ubuntu releases, but ship changeset as distro patches in the packages directly. Alternatively, we could consider picking it into the next point-release, which might be SRUed to Jammy (UC22) eventually, but not Focal (UC20).

[alfonsosanchezbeato]

Thanks a lot for your continued investigation @alfonsosanchezbeato !

An alternative could be to find out the patch fixing the issue in systemd-networkd and SRU to 20/22, but being systemd it might take longer than doing an SRU of netplan, I think.

Do we have a clean reproducer? I could bisect systemd to find the commit fixing the behaviour. I'd prefer fixing it properly in systemd that backporting a workaround.

Create a UC22 image, then install network-manager snap, then run sudo udevadm trigger. If the networkctl command shows something like:

IDX LINK TYPE     OPERATIONAL SETUP    
  1 lo   loopback carrier     unmanaged
  2 ens3 ether    routable    failed   

2 links listed.

then you have the bug (if not, the interfaces should be still unmanaged).

[slyon]

We had/have some similar quirks for Netplan in Focal already:

• 0.103: https://git.launchpad.net/~ubuntu-core-dev/netplan/+git/ubuntu/tree/debian/patches/0002-Revert-cli-apply-reload-reconfigure-networkd-instead.patch?h=ubuntu/0.103-0ubuntu5_20.04.6
• 0.104: https://git.launchpad.net/~ubuntu-core-dev/netplan/+git/ubuntu/tree/debian/patches/0006-cli-apply-give-some-extra-time-for-networkctl-reload.patch?h=ubuntu/0.104-0ubuntu2_20.04.2

So I guess I'd be fine SRUing it for Focal/UC20 in Netplan. While for Jammy/UC22 I'd still like to find the proper fix in systemd-networkd.

[slyon]

Create a UC22 image, then install network-manager snap, then run sudo udevadm trigger. If the networkctl command shows something like:

I wonder if there is any reproducer to show this issue on Ubuntu classic?

[daniloegea]

An alternative (instead of changing the current default behavior of apply) would be to add a new parameter to netplan apply (something like --force-backend-restart) to enable consumers of Netplan to force the restart of the backend in cases where the backend doesn't behave as expected or introduces a bug.

[slyon]

Therefore, it would make sense to backport b9ef830 to netplan in 20/22, and remove it from this PR, but keep 8dabb54.

So I've reverted the first commit in this PR (6874d4a), but kept the "apply: restart networkd instead of reload/reconfigure" commit as suggested (ac0d615).

Additionally, I've added a small fix that was failing autopkgtests when we have no networkd configuration at all in /run/systemd/networkd/ (directory doesn't exist) before or after the generator run.

With the workaround moved to canonical/core-base#214 I think this PR (just the cleaner networkd-config comparison logic) should now be good for a (squash) merge!

@daniloegea WDYT? (CC @alfonsosanchezbeato)

[slyon]

Rebased.

[daniloegea]

I think it looks fine.

So, effectively now we are going to continue to reload (not restart) networkd only when we detect changes in the generated configuration. It's sounds like a nice improvement.

The problem is that it doesn't fix the issue for UC and now they need to maintain a patch (PR#214) that will force the restart.

Wouldn't it help them if we introduce a way to force-restart the backends via a CLI parameter?

[alfonsosanchezbeato]

Therefore, it would make sense to backport b9ef830 to netplan in 20/22, and remove it from this PR, but keep 8dabb54.

So I've reverted the first commit in this PR (6874d4a), but kept the "apply: restart networkd instead of reload/reconfigure" commit as suggested (ac0d615).

Additionally, I've added a small fix that was failing autopkgtests when we have no networkd configuration at all in /run/systemd/networkd/ (directory doesn't exist) before or after the generator run.

With the workaround moved to snapcore/core-base#214 I think this PR (just the cleaner networkd-config comparison logic) should now be good for a (squash) merge!

@daniloegea WDYT? (CC @alfonsosanchezbeato)

This is fine by me too, thank you!