network: set link state configuring before setting addresses #11274
Conversation
|
It's a bit tricky. We are forcefully making state to be unconfigured before going to the function . Should we do this ? I am still thinking about this. |
|
Indeed. I will try to rework this. Thank you for the comment. |
|
The cause of the original bug doesn't make sense to me. We shouldn't be able to set LINK_STATE_CONFIGURED until we have set all of the addresses, neighbors, and routes as the conditions for making it through link_check_ready() requires all of those to be set. |
|
Is this not a bug caused by not resetting the *_configured booleans to false before running through the configure routines again? |
|
@wkennington and @ssahani Thank you for the comments. I have updated this. PTAL. @aroig If possible, please test the new version. Thank you. |
|
|
|
I confirm that the last changes in this PR fix #11272 for me. |
|
I think this should be OK |
src/network/networkd-link.c
Outdated
| link->neighbors_configured = true; | ||
| link_check_ready(link); |
There was a problem hiding this comment.
You shouldn't get rid of this in case we re-arrange the logic for setting neighbors. All of the other functions perform the same check which should be safe regardless of where it is executed.
There was a problem hiding this comment.
which should be safe regardless of where it is executed.
Good point.
| @@ -794,6 +794,8 @@ static int link_set_routing_policy_rule(Link *link) { | |||
| assert(link); | |||
| assert(link->network); | |||
|
|
|||
| link->routing_policy_rules_configured = false; | |||
There was a problem hiding this comment.
These should all be set to false at the same location in the codebase, likely when networkd determines that we need to reconfigure a link. Otherwise the invariant for link_check_ready() might not hold.
src/network/networkd-link.c
Outdated
| @@ -902,7 +906,7 @@ static int link_request_set_neighbors(Link *link) { | |||
| assert(link->network); | |||
| assert(link->state != _LINK_STATE_INVALID); | |||
|
|
|||
| link_set_state(link, LINK_STATE_CONFIGURING); | |||
There was a problem hiding this comment.
You should be able to leave this once we correct the invariants on link_check_ready(). Just in case we decide to re-arrange the ordering of the link_set_* functions in the future. There is no reason why link_set_neighbors should depend on link_set_addresses setting this state.
yuwata
added
the
reviewed/needs-rework 🔨
|
Force-pushed the 3rd version. I hope all comments by @wkennington are addressed. PTAL. Thank you. |
yuwata
removed
the
reviewed/needs-rework 🔨
|
This looks functionally good, but I'm not terribly happy with having the configuration functions needing to know exactly what to deconfigure. It would be nice if the caller which is performing the link configuration just called |
|
@wkennington Hmm, I'd like to keep setting |
|
@wkennington So, anyway, I would like to leave further cleanups in other PRs. Because, initially, this is intended to fix issue #11272. What do you think about? |
|
Just rebased and update comments. |
…k_set_routing_policy()
…uting_policy_rule() For consistency to other functions.
|
Seems fine to me |
|
OK. Let's merge this. |
|
Can this be backported to stable? It's not ok to have networkd crashing repeatedly. |
|
Ohhh please provide the dump. |
|
@nfnty you mean including this PR networkd crashing ? If so please provide your minimal conf so that we can fix this in current git. |
|
@ssahani No, I just want this PR to be backported to https://github.com/systemd/systemd-stable, so that distros that use the stable branch get this issue fixed. |
As
link_request_set_neighbors()may set link state 'configured'.Fixes #11272.
cc @wkennington and @ssahani.