Skip to content

Fold ipv4ll fallback modes into normal ipv4ll addressing#17290

Closed
keszybz wants to merge 5 commits intosystemd:masterfrom
keszybz:dhcp-client
Closed

Fold ipv4ll fallback modes into normal ipv4ll addressing#17290
keszybz wants to merge 5 commits intosystemd:masterfrom
keszybz:dhcp-client

Conversation

@keszybz
Copy link
Copy Markdown
Member

@keszybz keszybz commented Oct 9, 2020

No description provided.

The code was changed in 715cedf to allow more than
six attempts and the old description stopped making sense.
…lues

They are not really boolean, because we have both ipv4 and ipv6, but
for each protocol we have either unset, no, and yes.

From systemd#13316 (comment):
LinkLocalAddressing must be a boolean option, at least for ipv4:
- LinkLocalAddressing=no => no LL at all.

- LinkLocalAddressing=yes + Static Address => invalid configuration, warn and
  interpret as LinkLocalAddressing=no, no LL at all.

(we check that during parsing and reject)

- LinkLocalAddressing=yes + DHCP => LL process should be subordinated to the
  DHCP one, an LL address must be acquired at start or after a short N
  unsuccessful DHCP attemps, and must not stop DHCP to keeping trying. When a
  DHCP address is acquired, drop the LL address. If the DHCP address is lost,
  re-adquire a new LL address.

(next patch will move in this direction)

- LinkLocalAddressing=fallback has no reason to exist, because LL address must
  always be allocated as a fallback option when using DHCP. Having both DHCP
  and LL address at the same time is an RFC violation, so
  LinkLocalAdressing=yes correctly implemented is already the "fallback"
  behavior. The fallback option must be deprecated and if present in older
  configs must be interpreted as LinkLocalAddressing=yes.

(removed)

- And for IPv6, the LinkLocalAddress option has any sense at all? IPv6-LL
  address aren't required to be always set for every IPv6 enabled interface (in
  this case, coexisting with static or dynamic address if any)? Shouldn't be
  always =yes?

(good question)

This effectively reverts 29e8108. There is no
special "fallback" mode now, so the check doesn't make sense anymore.
So far we only reported major state transitions like failure to acquire
the message. Let's report the initial failure after a few timeouts in
a new event type.

The number of timeouts is hardcoded as 3, since Windows seems to be using
that. I don't think we need to make this configurable out of the box. A
reasonable default may be enough.
@keszybz keszybz added network ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR labels Oct 9, 2020
Copy link
Copy Markdown
Member

@yuwata yuwata left a comment

Choose a reason for hiding this comment

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

I've not tested this. I may missed something, but it seems there is no logic that dropping ipv4ll after DHCPv4 address is acquired when once DHCPv4 is failed and ipv4ll is acquired:

  1. DHCPv4 fails (3 trial),
  2. IPv4LL is acquired,
  3. DHCPv4 address is acquired,
  4. dropping IPv4LL <--- is this implemented in this PR?


if (link_ipv4ll_enabled(link, ADDRESS_FAMILY_IPV4)) {
if (link_ipv4ll_enabled(link)) {
assert(link->ipv4ll);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I prefer to simply use if (link->ipv4ll).


case SD_DHCP_CLIENT_EVENT_TRANSIENT_FAILURE:
if (link_ipv4ll_enabled(link)) {
assert(link->ipv4ll);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same here. I prefer if (link->ipv4ll)

@yuwata
Copy link
Copy Markdown
Member

yuwata commented Nov 23, 2020

Replaced by #17692.

@yuwata yuwata closed this Nov 23, 2020
@BillPlunkett
Copy link
Copy Markdown

I've been following this discussion for a while, but not 100% sure I understand where things stand.

The desired behavior (for me) is:

  1. network configured in DHCP client mode
  2. DHCP server not present or unresponsive, so ipv4ll address assigned
  3. DHCP server (re)appears, so ipv4ll address removed and DHCP address assigned

From above, it seems like this behavior has been implemented in v247 with the exception that the ipv4ll address will remain after a DHCP address is assigned in step 3. Assuming that is true, what are the ramifications?

@keszybz keszybz deleted the dhcp-client branch December 7, 2020 08:13
@keszybz
Copy link
Copy Markdown
Member Author

keszybz commented Dec 7, 2020

Let's continue the discussion on the other PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR needs-rebase network

Development

Successfully merging this pull request may close these issues.

4 participants