Skip to content

Revert PR 449 (LP: #2078009)#518

Merged
daniloegea merged 2 commits intocanonical:mainfrom
daniloegea:revert_pr_449
Sep 23, 2024
Merged

Revert PR 449 (LP: #2078009)#518
daniloegea merged 2 commits intocanonical:mainfrom
daniloegea:revert_pr_449

Conversation

@daniloegea
Copy link
Contributor

@daniloegea daniloegea commented Sep 16, 2024 •
edited by slyon
Loading

Description

This PR essentially reverts #449

The only conflict was due to an import that was removed (netifaces).

On #449 netplan apply was changed to only apply if there were changes in the backend config files.

While this is a nice approach to avoid unnecessary network disruptions, backend configuration can be generated without apply be aware of it (via the generator (such as when daemon-reload is called) or netplan generate).

In this cases (if the generator or netplan generate runs first), when apply compares the previous configuration with the new one, there will be no changes.

See LP: #2078009. Here, when a new interface is added to the container, a daemon-reload happens (twice apparently). Because of that, the netplan generator is called, new backend configuration is generated and netplan apply wouldn't do any action after that.

Journal from the container at the moment the interface is plugged in:

Sep 16 12:59:22 doh systemd-networkd[441]: veth28d5cf88: Interface name change detected, renamed to eth2.
Sep 16 12:59:22 doh systemd[1]: Starting cloud-init-hotplugd.service - Cloud-init: Hotplug Hook...
Sep 16 12:59:22 doh cloud-init-hotplugd[712]: args=--subsystem=net handle --devpath=/devices/virtual/net/veth28d5cf88 --udevaction=add
Sep 16 12:59:23 doh systemd[1]: Reload requested from client PID 719 ('systemctl') (unit cloud-init-hotplugd.service)...
Sep 16 12:59:23 doh systemd[1]: Reloading...
Sep 16 12:59:23 doh systemd[1]: Configuration file /run/systemd/system/systemd-networkd-wait-online.service.d/10-netplan.conf is marked world-inaccessible. This has no effect as configuration data is accessible via APIs without restrictions. Proceeding anyway.
Sep 16 12:59:23 doh systemd[1]: Configuration file /run/systemd/system/netplan-ovs-cleanup.service is marked world-inaccessible. This has no effect as configuration data is accessible via APIs without restrictions. Proceeding anyway.
Sep 16 12:59:23 doh systemd[1]: Reloading finished in 114 ms.
Sep 16 12:59:23 doh systemd[1]: Reload requested from client PID 769 ('systemctl') (unit cloud-init-hotplugd.service)...
Sep 16 12:59:23 doh systemd[1]: Reloading...
Sep 16 12:59:23 doh systemd[1]: Configuration file /run/systemd/system/systemd-networkd-wait-online.service.d/10-netplan.conf is marked world-inaccessible. This has no effect as configuration data is accessible via APIs without restrictions. Proceeding anyway.
Sep 16 12:59:23 doh systemd[1]: Configuration file /run/systemd/system/netplan-ovs-cleanup.service is marked world-inaccessible. This has no effect as configuration data is accessible via APIs without restrictions. Proceeding anyway.
Sep 16 12:59:23 doh systemd[1]: Reloading finished in 106 ms.
Sep 16 12:59:23 doh systemd[1]: cloud-init-hotplugd.service: Deactivated successfully.
Sep 16 12:59:23 doh systemd[1]: Finished cloud-init-hotplugd.service - Cloud-init: Hotplug Hook.
Sep 16 12:59:33 doh systemd-journald[57]: Forwarding to syslog missed 2162 messages.
Sep 16 12:59:33 doh systemd[1]: systemd-timedated.service: Deactivated successfully.
Sep 16 12:59:33 doh systemd[1]: systemd-hostnamed.service: Deactivated successfully.

I created a PPA with these patches for testing: https://launchpad.net/~danilogondolfo/+archive/ubuntu/bugfixes

Autopkgtests on this PPA.

https://autopkgtest.ubuntu.com/results/autopkgtest-oracular-danilogondolfo-bugfixes/oracular/arm64/n/netplan.io/20240916_144524_6b1ba@/log.gz
https://autopkgtest.ubuntu.com/results/autopkgtest-oracular-danilogondolfo-bugfixes/oracular/ppc64el/n/netplan.io/20240916_144805_5f233@/log.gz
https://autopkgtest.ubuntu.com/results/autopkgtest-oracular-danilogondolfo-bugfixes/oracular/s390x/n/netplan.io/20240916_142627_dc92a@/log.gz
https://autopkgtest.ubuntu.com/results/autopkgtest-oracular-danilogondolfo-bugfixes/oracular/armhf/n/netplan.io/20240916_145550_526ff@/log.gz
https://autopkgtest.ubuntu.com/results/autopkgtest-oracular-danilogondolfo-bugfixes/oracular/amd64/n/netplan.io/20240916_145623_713d6@/log.gz

Checklist

  • Runs make check successfully.
  • Retains code coverage (make check-coverage).
  • New/changed keys in YAML format are documented.
  • (Optional) Adds example YAML for new feature.
  • (Optional) Closes an open bug in Launchpad.

@daniloegea daniloegea requested a review from slyon September 16, 2024 15:00
@slyon
Copy link
Contributor

slyon commented Sep 17, 2024

CC @alfonsosanchezbeato would this revert break any of your usecases?

slyon
slyon approved these changes Sep 17, 2024
View reviewed changes
Copy link
Contributor

@slyon slyon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. It matches the code from before #449 except for cases where it evolved over time, such as the replacement for netifaces and unused is_composite_member method, in addition to the new udevadm control --reload & udevadm trigger --action=move --subsystem-match=net --settle handling.

Let's give Alfonso some time to chime in, before merging.

@daniloegea daniloegea mentioned this pull request Sep 17, 2024
Draft
5 tasks
@alfonsosanchezbeato
Copy link
Member

alfonsosanchezbeato commented Sep 17, 2024 •
edited
Loading

I'm fine with this, but just to double check, will this restart/reload config for networkd/network-manager as a minimum in the same cases as before? We have had many issues related to this in the past and I'd like to make sure that things like moving from some config to no config, etc. work as expected.

@daniloegea
Copy link
Contributor Author

daniloegea commented Sep 18, 2024

Yes, it's essentially reverting the behavior to what it was before until we come up with a better solution. netplan apply will not work properly in some situations because of this.

I think you have a local patch that will restart networkd? I suppose it should continue to work.

@alfonsosanchezbeato
Copy link
Member

alfonsosanchezbeato commented Sep 18, 2024

Yes, it's essentially reverting the behavior to what it was before until we come up with a better solution. netplan apply will not work properly in some situations because of this.

I think you have a local patch that will restart networkd? I suppose it should continue to work.

I have a patch for core22, but not for core24 where some networkd bug was not present anymore.

@daniloegea
Copy link
Contributor Author

daniloegea commented Sep 18, 2024

I think it will not change things for you then. I still think we should have a command line parameter to force the restart of backends so wouldn't need to maintain a patch for that. We probably should consider adding one.

@daniloegea daniloegea merged commit 1ab755c into canonical:main Sep 23, 2024
@slyon slyon added the stable Might be merged in a stable branch label Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@slyon slyon slyon approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

stable Might be merged in a stable branch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@daniloegea @slyon @alfonsosanchezbeato