Skip to content

Create individual [email protected] units (LP: #2060311)#455

Closed
slyon wants to merge 7 commits intocanonical:mainfrom
slyon:wait-online
Closed

Create individual [email protected] units (LP: #2060311)#455
slyon wants to merge 7 commits intocanonical:mainfrom
slyon:wait-online

Conversation

@slyon
Copy link
Contributor

@slyon slyon commented Apr 11, 2024

Description

This is in favor to RequiredForOnline=yes as the behavior of upstream (pure) systemd-networkd-wait-online.service is not mean to be used in this way.

If "RequiredForOnline=no" sd-networkd-wait-online will fully ignore the corresponding interface and it will block/delay network-online.target if no interfaces are "RequiredForOnline=yes" at all.

This is as per discussions in systemd/systemd#32016

FR-7246

Checklist

  • Runs make check successfully.
  • Retains 100% 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. LP#2060311

@slyon slyon requested a review from enr0n April 11, 2024 13:51
@slyon slyon force-pushed the wait-online branch 4 times, most recently from a995dbf to 125baa3 Compare April 11, 2024 15:55
Copy link

@enr0n enr0n left a comment

Choose a reason for hiding this comment

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

This looks good so far to me.

One other thing is that we will actually need to mask systemd-networkd-wait-online.service by default I think, because otherwise e.g. a unit generated by systemd-sysv-generator could re-enable it, which is just annoying.

Since netplan would be the one enforcing this new [email protected] policy, do you think it makes sense to ship the mask (symlink to /dev/null) in netplan?

_netplan_safe_mkdir_p_dir(enablement_dir);
}

// Create an enablement link for each interface in the <ifnames> vector
Copy link

Choose a reason for hiding this comment

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

Not something that will likely be addressed in this PR (because I expect the same thing is done elsewhere in the netplan generator), but systemd generators really should not touch unit search paths outside of the directories passed as arguments to the generator (e.g. usually /run/systemd/generator{,.early,.late}).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ACK. See spec FO165

* netplan_netdef_match_interface() to see if they match the current NetDef
*/
STATIC void
_netplan_enumerate_interfaces(const NetplanNetDefinition* def, GStrvBuilder* builder, const char* rootdir)
Copy link

Choose a reason for hiding this comment

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

Is this here to work around the fact that we could have a generated file like:

# my-interface.network
[Match]
MacAddress=AA:BB:CC:DD:EE:FF

[Network]
...

but we want to enable e.g. [email protected], if ethZ is the interface with the matching MAC?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! We need to expand any Netplan match.mac|driver|name setting to the actual matching network interfaces. Therefore, we need to check those values on all of them.

@slyon slyon force-pushed the wait-online branch 5 times, most recently from f9528b7 to cf6c4c6 Compare April 15, 2024 09:58
@@ -0,0 +1,26 @@
# SPDX-License-Identifier: LGPL-2.1-or-later
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@slyon slyon marked this pull request as ready for review April 15, 2024 10:40
@slyon slyon changed the title Create individual [email protected] units Create individual [email protected] units (LP: #2060311) Apr 15, 2024
@slyon slyon changed the title Create individual [email protected] units (LP: #2060311) Create individual [email protected] units (LP: #2060311) Apr 15, 2024
@slyon slyon requested a review from daniloegea April 15, 2024 10:56
@slyon
Copy link
Contributor Author

slyon commented Apr 15, 2024

* Otherwise, systemd might wait indefinitely for the interface to
* come online.
*/
if (!(def->optional || def->activation_mode)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

question: Should we also check if IP addresses or dhcp4/dhcp6 are defined, e.g. to ignore definitions like this ethernets.encc000: {}?

We could define such scenarios as "user error", or try to be smart about it..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd probably opt to "user error" for now (the easy workaround is adding optional: true instead of {}) and in the future we might want to wait for different conditions, e.g. link-layer configuration, which would still involve some waiting, but not necessarily on routable IPs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if I understood the question. Are you suggesting we shouldn't create the link for that type of definition (empty)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Empty NetDefs will not reach the "online" state, due to lacking an IP address. I was wondering if we should implicitly mark them "optional: true". But I now feel like we shouldn't do it, as logic might change in the future with the implementation of "optional-addresses".

slyon added 7 commits April 15, 2024 16:57
This is in favor to RequiredForOnline=yes as the behavior of upstream (pure)
systemd-networkd-wait-online.service is not mean to be used in this way.

If "RequiredForOnline=no" sd-networkd-wait-online will fully ignore the
corresponding interface and it will block/delay network-online.target if
no interfaces are "RequiredForOnline=yes" at all.

FR-7246
This is to enforce individual "[email protected]"
wait-online policy.
@slyon slyon force-pushed the wait-online branch 2 times, most recently from a9e5024 to b16ca32 Compare April 16, 2024 09:10
@slyon slyon marked this pull request as draft April 16, 2024 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants