Set IgnoreCarrierLoss= default to value of ConfigureWithoutCarrier= #15619
Conversation
|
Hmm, the CentOS on Arch failures seem to indicate the bridge there gained carrier, but it shouldn't have since it shouldn't have any slave interfaces attached...I'll look at the test to see if I can figure it out |
ddstreet
force-pushed
the
ignore_carrier_loss_default
branch
from
e16fad8
to
57f9adb
Compare
ddstreet
changed the title
ddstreet
force-pushed
the
ignore_carrier_loss_default
branch
from
c3be6ad
to
75e6212
Compare
|
Both should not be clubbed together. |
For some background on this PR, I opened this to address this ubuntu bug: Quick summary there is the networkd configuration sets up a bridge with a single slave interface, and adds static address/gateway config to the bridge. It also uses ConfigureWithoutCarrier=true, but does not include IgnoreCarrierLoss=true. (I don't know why ConfigureWithoutCarrier=true is used). The problem is during networkd start, the bridge comes up with carrier, and starts configuring itself, including adding its static address However, it then loses carrier very shortly after (due to -LOWER_UP), and that static address is removed (because IgnoreCarrierLoss defaults to false). But the initial configuration continues, and link_request_set_routes() sees that while carrier is down, configure_without_carrier is set, so it attempts to set the routes. That route setting fails, because the static address has been removed. I tested with upstream systemd (instead of Ubuntu systemd) and the problem doesn't happen, but that is because the bridge starts without initially having carrier (which, it shouldn't, until its slave is brought up). I'm not sure yet why Ubuntu's networkd shows the bridge with carrier before bringing up the slave interface, but this race condition exists even with upstream code - if carrier is lost very briefly during configuration and ConfigureWithoutCarrier=true is set, but not IgnoreCarrierLoss=true, then there's definitely a race condition in correctly configuring the interface route. This approach, to set the IgnoreCarrierLoss default to the value of ConfigureWithoutCarrier, seemed to me like the quickest and easiest way to at least work around this race condition, since it unfortunately is very reproducable in the latest released Ubuntu. And, it seems strange that anyone would want to use ConfigureWithoutCarrier=true but also want their configuration to get dropped the first time carrier is gained and then lost. Note that this does not set both config params together; this PR only changes the default value of IgnoreCarrierLoss, if not specified, to whatever the configured (or default) value is for ConfigureWithoutCarrier. I would argue that most people probably don't realize that setting ConfigureWithoutCarrier=true does not also provide the functionality of IgnoreCarrierLoss=true. Anyway, I do still think this is a good change to make, but if you don't think it makes sense (or, even if you do agree) I can look into fixing the race condition when an interface briefly drops/gains carrier during its configuration. |
|
Hmm, this is really interesting, the Centos CI running on bare metal (with CentOS 7) appears to bring up the bridge which then reports -LOWER_UP (which IIUC is the correct behavior from the kernel), but the CentOS CI running under KVM brings up the bridge which then reports +LOWER_UP, just like happens with Ubuntu. I've only testing on Ubuntu under KVM, but I'll check a bare metal system also. |
|
Ok, after talking to a kernel coworker, I understand what's going on now. When the kernel creates a new bridge interface, as soon as it's brought +UP, it has carrier (i.e. +LOWER_UP). It will stay that way, as long as it has no ports (slave interfaces).
So...what's happening is, networkd first creates the new bridge, then brings that bridge +UP, and the kernel tells networkd that the bridge has carrier (+LOWER_UP). So, networkd (correctly) starts configuring the bridge. However, as soon as networkd adds the slave interface (which is down) to the bridge, it changes the bridge to -LOWER_UP and networkd gets notification of carrier loss, and starts unconfiguring the bridge. Then, when networkd brings up the bridge slave, the bridge gains carrier, and networkd starts re-configuring the bridge. Why does this "work" in some places (e.g. the CentOS on CentOS 7 test), but "fail" in other places (e.g. the CentOS on Arch test)? Because when a new bridge is created, if its mac address is set before it's brought up, then it comes up without carrier.
This may require networkd's netdev code to specifically tweak newly-created bridges that it creates before bringing them up... |
ddstreet
force-pushed
the
ignore_carrier_loss_default
branch
from
75e6212
to
f4262f2
Compare
ddstreet
changed the title
ddstreet
force-pushed
the
ignore_carrier_loss_default
branch
from
f4262f2
to
c6e7932
Compare
|
Ok, this PR is a RFC now; I think only 1 of these 2 commits should be included: I do not like the "only honor configure_without_carrier if link never had carrier" commit; I would much prefer defaulting IgnoreCarrierLoss to the value of ConfigureWithoutCarrier, or even forcing IgnoreCarrierLoss=true if ConfigureWithoutCarrier=true. However @ssahani, I may be missing something about your earlier comment:
So, I don't really understand this use case; what can an empty bridge be used for? Reading the original issue the parameter was added for, #6645, the lxdbr0 bridge definitely will gain carrier, later, when a container veth is added to it (when a lxd container is created). I do understand the use case of wanting the bridge to be configured while empty, but that use case absolutely will want IgnoreCarrierLoss=true as well, and I suspect that all users of ConfigureWithoutCarrier=true will actually want IgnoreCarrierLoss=true, but may not actually realize they don't get it just by setting ConfigureWithoutCarrier=true. |
ddstreet
changed the title
@ssahani @yuwata any opinion on either/both of these commits?
Personally, I really think this should be included, as I believe many people using ConfigureWithoutCarrier=true don't realize they will lose that configuration as soon as carrier drops - which might be during initial configuration! Certainly the original use case the parameter was added for (#6645) will gain carrier later (and will also lose carrier later if/when all bridge ports are removed).
Personally, I'd rather not include this commit, as it adds confusion to initial configuration (having to track if the link ever had carrier is adding complexity for a use case I don't think exists); however without it, anyone who sets ConfigureWithoutCarrier=true and IgnoreCarrierLoss=false may face network configuration failure, due to losing carrier between setting the address and setting the route(s). |
ddstreet
force-pushed
the
ignore_carrier_loss_default
branch
from
c6e7932
to
0fa3093
Compare
ddstreet
changed the title
|
I've removed my patch 'network: only honor configure_without_carrier if link never had carrier' from the series, but it's still available to view here: I still believe adjusting IgnoreCarrierLoss= to default to ConfigureWithoutCarrier= is the correct thing to do here. |
There was a problem hiding this comment.
i like this. I think it does make a lot of sense to bind this together if not explicitly configured. I agree with @ssahani that people might want to use this separately and that those usecases are string, but i have the suspicion the most frequent is to use both together, hence I think this PR makes a ton of sense.
| assert_return(sd_ipv4ll_is_running(ll) == 0, -EBUSY); | ||
|
|
||
| if (sd_ipv4ll_is_running(ll)) | ||
| return 0; | ||
|
|
||
| return ipv4ll_start_internal(ll, true); |
There was a problem hiding this comment.
i think this makes sense, but I think this could use one more tweak: if we are already started we should return 1 here, but if we are just starting this we should return 0 here. i.e. two different ways of indicating success but still slightly different, to let the caller know if what they did was a NOP or not. This is in fact also hat sd_ndisc_start() does.
There was a problem hiding this comment.
One clarification, it looks like sd_ndisc_start() return values are 0 == already started, and 1 == successful start...should sd_ipv4ll_start() change to that? Or 0 == successful start and 1 == already started?
There was a problem hiding this comment.
A quick look in src/libsystemd-network/ seems to show:
sd_dhcp_client_start: start == 0, already started == -EBUSY
sd_dhcp6_client_start: start == 0, already started == -EBUSY
sd_ipv4acd_start: start == 0, already started == -EBUSY
sd_ipv4ll_start: start == 0, already started == -EBUSY
sd_lldp_start: start == 1, already started == 0
sd_ndisc_start: start == 1, already started == 0
sd_radv_start: start == 0, already started == 0
should they be unified to a consistent set of return codes?
There was a problem hiding this comment.
of course, it would be excellent if this would be unified!
but i think I would like to change my mind regarding what i wrote above. I think == 0 should mean "already started, all ok", and == 1 (or more generically > 0) should mean "started now, all ok". i.e. sd_nids_start + sd_lldp_start do it right, and the others should be changed to work the same.
i.e. our usually rule regarding return values is < 0 error, >= 0 is success. And of the latter we sometimes define a special case > 0 meaning "we actually did something to become successful" while == 0 means "we didn't do anything, but things are OK anyway".
This is followed quite often in our codebase, but not always (as we can see), but I think we should strive to make this behaviour systematic.
(Doesn't have to be in this PR, but it would of course be excellent if this unification would happen, either here or in another PR)
There was a problem hiding this comment.
i adjusted sd_ipv4ll_start() to match, and i'll look at changing the others as well, but in a separate PR so it doesn't hold this one up (and in case it's not simple). Thanks!
poettering
added
the
reviewed/needs-rework 🔨
…tCarrier It doesn't make much sense to have ConfigureWithoutCarrier set, but not IgnoreCarrierLoss; all the configuration added during initial interface bring-up will be lost at the first carrier up/down.
ddstreet
force-pushed
the
ignore_carrier_loss_default
branch
from
0fa3093
to
9673498
Compare
Instead of -EBUSY, return 0 from sd_ipv4ll_start() if it's already started, and change successful start return value to 1. This matches sd_ndisc_start() behavior; 1 indicates successful start, and 0 indicates already started.
The test currently doesn't actually test configure-without-carrier since it does have carrier for the entire test. It now forces carrier down before starting the network portion of the test. Also, it tests to verify the configuration is retained across future carrier losses/gains.
Verify configure-without-carrier works, and retains configuration across carrier losses/gains.
ddstreet
force-pushed
the
ignore_carrier_loss_default
branch
from
9673498
to
0fc0d85
Compare
yuwata
removed
the
reviewed/needs-rework 🔨
|
LGTM. |
It doesn't make much sense to have ConfigureWithoutCarrier set, but not
IgnoreCarrierLoss; all the configuration added during initial interface
bring-up will be lost at the first carrier up/down.