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

Add background option to dhcpcd #58526

Closed
wants to merge 0 commits into from
Closed

Conversation

bene1618
Copy link
Contributor

@bene1618 bene1618 commented Mar 29, 2019

Motivation for this change

DHCP waits for an IP address to be assigned before forking to background. This causes a lag during startup. I removed the -w option from the .service file, and added it, optionally, to the dhcpcd.config file. One can now make dhcpcd fork to background automatically by adding "networking.dhcpcd.background = true;" in the NixOS configuration. On my system, this reduced userspace startup time from about 11s to about 3s.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • [ x ] NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • [ x ] Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Mar 29, 2019

If dhcpcd forks immediately and systemd thinks the network is ready, isn't it possibile that programs needing to bind to an address may randomly fail if the interface is not up yet?
I'd say it's better to just disable ARP checking, this saves a significant number of seconds and should be safe in most cases.

@bene1618
Copy link
Contributor Author

I am not really an expert, but ArchLinux starts dhcpcd with the -b flag by default: Here is the ArchLinux dhcpcd.service file. Additionally, I kept the standard behaviour - I only added the option of forking to the background, which actually might be helpful in many cases.

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Mar 30, 2019

There is a relevant discussion at #50930

Copy link
Contributor

@rnhmjoj rnhmjoj left a comment

Choose a reason for hiding this comment

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

I have done some testing and I can't find any issue with it. Also all nixos networking and related tests pass.

@gloaming
Copy link
Contributor

gloaming commented May 3, 2019

dhcpcd actually has various other possible configurations regarding waiting:

man dhcpcd:

If any interface reports a working carrier then dhcpcd will try and obtain a lease before forking to the background, otherwise it will fork right away. This behaviour can be modified with the -b, --background and -w, --waitip options.

--waitip=[4 | 6]
Wait for an address to be assigned before forking to the background. 4 means wait for an IPv4 address to be assigned. 6 means wait for an IPv6 address to be assigned. If no argument is given, dhcpcd will wait for any address protocol to be assigned. It is possible to wait for more than one address protocol and dhcpcd will only fork to the background when all waiting conditions are satisfied.

So perhaps we should have a multiway option e.g.

dhcpcd.wait = one of {"never", "if-network-up", "any", "ipv4", "ipv6", "both"}

See also #44524 (comment)

@Mic92 The dhcpcd unit already activates network-online.target if a lease has been obtained but unfortunately does this on IPv6 or IPv4. So network-online.target may be activated without an IPv4 default route if IPv6 succeeds first.

Dhcpcd can be configured to either wait for IPv4 or IPv6 but this will break IPv4-only or IPv6-only systems. Not sure what we should do here without introducing new options and rethinking the NixOS networking autoconfiguration logic.

@gloaming
Copy link
Contributor

gloaming commented May 3, 2019

See also #35567 (comment)

@worldofpeace
Copy link
Contributor

FTR, been using this for a while. There may be other issues around this but startups are much better on certain machines.

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Aug 26, 2019

Yeah, can we marge this please?

@gloaming
Copy link
Contributor

gloaming commented Aug 26, 2019

There has been no response to my feedback above. I suggest not adding half-baked options since it causes user friction if they are removed later :)

Note also that the unit ordering issue has been fixed, so you shouldn't be experiencing delayed starts anyway unless you're still on 19.03, in which case you could employ any of various workarounds.

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Sep 4, 2019

ping @bene1618

@bene1618
Copy link
Contributor Author

bene1618 commented Sep 5, 2019

I tried to implement @gloaming 's suggestion. I do not really understand if the standard behaviour (without any of the options) differs from "waitip", so I left out the "if-network-up" option.

@gloaming
Copy link
Contributor

gloaming commented Sep 7, 2019

As I understand it, the default behaviour is equivalent to background if your ethernet is unplugged and your wifi is unpowered, and equivalent to waitip otherwise. The point is that you can't hope to get a connection if you're not plugged in, so there's no point waiting.

Now that we have dhcpcd bound to network-online.target, defaulting to waitip is sensible.

if-network-up was silly of me, it should have been if-carrier-up or just default. Since the option is an enum, I guess we ought to include it or a user won't be able to select the default behaviour at all.

IPv6 = "waitip 6";
both = "waitip 4\nwaitip 6";
default = "";
}.${cfg.wait}}
Copy link
Member

Choose a reason for hiding this comment

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

The default here doesn't achieve anything. If you wanted a default you'd have to use { ... }.${cfg.wait} or "". But I don't think there's any reasonable default here anyways, so it should be okay to just not have one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems I was a bit tired. The dhcpcd default behaviour is selected with the option "if-carrier-up". And due to @gloaming's comment, this behaviour should differ from the other options available. The relevant part in the dhcpcd man page is:

If any interface reports a working carrier then dhcpcd will try and obtain a lease before forking to the background, otherwise it will fork right away.

Copy link
Member

@fpletz fpletz left a comment

Choose a reason for hiding this comment

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

Although I'm not a fan of effectively disabling network-online.target, exposing this configuration option explicitly does make sense to me instead of hardcoding it into the commandline. Thanks!

nixos/modules/services/networking/dhcpcd.nix Outdated Show resolved Hide resolved
@rnhmjoj
Copy link
Contributor

rnhmjoj commented Jan 27, 2020

Updates?

Copy link
Member

@infinisil infinisil 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 to me once the commits are squashed

@bene1618
Copy link
Contributor Author

I seem to have closed the pull request accidentally by doing something wrong with the push of the squashed commit. Now I don't see how I can reopen it again. Github claims that there are no new commits on the bene1618:master branch, but there is one: bene1618@0767de3

Could someone maybe help me what I have to do now?

@infinisil
Copy link
Member

Not sure, but you can also just open a new PR. Generally I also suggest creating a new branch for every PR

@worldofpeace
Copy link
Contributor

Not sure, but you can also just open a new PR. Generally I also suggest creating a new branch for every PR

Yep 👍

@bene1618 You really shouldn't use the master branch as per our contributing docs.

git checkout -b new-branch-name

and PR.

@bene1618
Copy link
Contributor Author

Yes, I also realized that shortly after I had opened this pull request. I have now opened a new one at #78745.

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

Successfully merging this pull request may close these issues.

None yet

8 participants