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

networkmanager: Add rc-manager option #61949

Merged
merged 2 commits into from Jul 3, 2019
Merged

Conversation

talyz
Copy link
Contributor

@talyz talyz commented May 23, 2019

Motivation for this change

Add an option to set the rc-manager parameter in NetworkManager.conf,
which controls how NetworkManager handles resolv.conf. This sets the
default rc-manager to "resolvconf", which solves #61490. It
additionally allows the user to change rc-manager without interference
from configuration activations.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • 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.

<para>
Options:
<variablelist>
<varlistentry>
Copy link
Contributor

Choose a reason for hiding this comment

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

This is pretty much documented at https://developer.gnome.org/NetworkManager/stable/NetworkManager.conf.html
"main section" "rc-manager". Can't we link there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! For consistency, we should do this for dns as well.

# by NetworkManager
${optionalString (!config.environment.etc?"resolv.conf" &&
cfg.networkmanager.enable ->
cfg.networkmanager.rc-manager == "resolvconf") ''
Copy link
Member

Choose a reason for hiding this comment

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

I'm very confused by this condition here, can you explain this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, I had the precedence messed up there. It's supposed to check that there's no "resolv.conf"-attribute in etc and that, if networkmanager is enabled, rc-manager is set to "resolvconf".

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also curious, i don't know much about network manager, but after reading the doc it seems that network manager is supposed to run itself the resolvconf program, so I'm not sure why we run here resolvconf. I would have written instead just config.environment.etc?"resolv.conf" && !cfg.networkmanager.enable. Did I missed something? Otherwise, thanks for this PR, it solves the problem of no internet after each rebuild!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NetworkManager runs resolvconf only if configured to do so via rc-manager = "resolvconf", which is why the condition is needed - it makes sure that if NetworkManager is enabled, resolvconf is only run here if NetworkManager is configured to use resolvconf.

talyz added 2 commits May 24, 2019 22:27
Add an option to set the rc-manager parameter in NetworkManager.conf,
which controls how NetworkManager handles resolv.conf. This sets the
default rc-manager to "resolvconf", which solves NixOS#61490. It
additionally allows the user to change rc-manager without interference
from configuration activations.
- Refer to external documentation for dns option
- Clean up macAddress option
- Improve references
Copy link
Contributor

@etu etu left a comment

Choose a reason for hiding this comment

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

I reproduced the issue by adding something to my system and doing a nixos-rebuild test and watched the /etc/resolv.conf loose it's nameservers. So to regain my nameservers I reconnected to my wifi.

Then I applied this patch and did another nixos-rebuild test and still had the nameservers in there.

@etu
Copy link
Contributor

etu commented Jun 17, 2019

@infinisil Is there something that you think we need to fix in this after the updates to your comments?

etu added a commit to etu/nixconfig that referenced this pull request Jun 23, 2019
@alyssais alyssais merged commit 732af03 into NixOS:master Jul 3, 2019
@talyz talyz deleted the networkmanager branch July 3, 2019 11:52
@abbradar abbradar mentioned this pull request Jul 11, 2019
10 tasks
@kampka
Copy link
Contributor

kampka commented Aug 6, 2019

Any chance we can get this backported to 19.03.
The current behavior using NetworkManager is truly annoying.

@infinisil
Copy link
Member

@kampka If you want something backported, you're always free to open a PR for it (to the release-19.03 branch)

@alyssais
Copy link
Member

alyssais commented Aug 12, 2019 via email

@lheckemann
Copy link
Member

-1 on backporting. This can be worked around effectively using

networking.networkmanager.extraConfig = ''
 [main]
 rc-manager=resolvconf
'';

as mentioned by @talyz on the issue that prompted this, or by defining networking.nameservers to be non-empty.

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