Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

networkd: make s-n-wait-online wait for at least one routable interface #482

Merged
merged 6 commits into from
Jul 4, 2024

Conversation

slyon
Copy link
Collaborator

@slyon slyon commented Jun 24, 2024

Description

wait-online: wait for 'routable' state, if corresponding IPs are defined

Generally, we should be waiting on "routable" state (instead of "degraded") when we have static IP/dhcp4/dhcp6/RA.

Our spec defines "at least one interface MUST be up on the link layer and have received layer 3 (IP) configuration" (ignoring link-local).

We now start two separate systemd-networkd-wait-online processes. One waiting for "carrier" & "degraded" interfaces (including the "routable" ones), not using "--any". Another one waiting just for "routable" interfaces, combined with "--any". Both blocking "network-online.target".

This is an extension of #456

FR-7838

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.

@slyon slyon requested review from daniloegea and enr0n June 24, 2024 15:57
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.

Nice! LGTM.

@slyon slyon force-pushed the s-n-wait-online-routable branch from 1f5c78d to ebb14e4 Compare June 25, 2024 07:54
tests/generator/test_args.py Outdated Show resolved Hide resolved
slyon added 2 commits June 27, 2024 15:58
Generally, we should be waiting on "routable" state (instead of "degraded") when we have static IP/dhcp4/dhcp6/RA.

Our spec defines "at least one interface MUST be up on the link layer and have received layer 3 (IP) configuration" (ignoring link-local).

We now start two separate "systemd-networkd-wait-online" processes. One waiting for "carrier" & "degraded" interfaces, not using "--any". Another one waiting just for "routable" interfaces, combined with "--any". Both blocking "network-online.target".
@slyon slyon force-pushed the s-n-wait-online-routable branch 3 times, most recently from a119c15 to ba03349 Compare June 27, 2024 15:00
@slyon slyon force-pushed the s-n-wait-online-routable branch from ba03349 to 56b149c Compare June 27, 2024 15:10
Copy link
Collaborator

@daniloegea daniloegea left a comment

Choose a reason for hiding this comment

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

It seems to be working as intended.

I just left one thought about bonds. I'm not sure if we should wait for carrier on all the members by default. The system doesn't need to have carrier on all the bond members to be online.

But to have it waiting for any member we'd need to create groups of members for each bond and have one call to wait-online --any for each group... maybe we could consider that in the future. Maybe we should recommend that bond members should be optional.

src/networkd.c Show resolved Hide resolved
@slyon
Copy link
Collaborator Author

slyon commented Jul 3, 2024

I'm not sure if we should wait for carrier on all the members by default. The system doesn't need to have carrier on all the bond members to be online.

Right, I think this edge cases was not considered in the spec. But issuing a log message about it is probably a good first step. Further improvements can then be done as follow-up PRs.

PTAL, I added two new isolated commits on top of the current branch (the 2nd begin some small, unrelated refactoring).

Copy link
Collaborator

@daniloegea daniloegea left a comment

Choose a reason for hiding this comment

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

Looks good. I just left a suggestion to make the log message a bit more informative.

src/networkd.c Show resolved Hide resolved
@slyon slyon requested a review from daniloegea July 3, 2024 15:31
Copy link
Collaborator

@daniloegea daniloegea left a comment

Choose a reason for hiding this comment

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

lgtm

@slyon slyon merged commit 039ea1f into canonical:main Jul 4, 2024
15 of 16 checks passed
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