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

nixos/stage-1: fix predictable interface names in initrd #79532

Merged
merged 5 commits into from Mar 2, 2020

Conversation

@fpletz
Copy link
Member

@fpletz fpletz commented Feb 8, 2020

Motivation for this change

This makes predictable interfaces names available as soon as possible with udev by adding the default network link units to initrd which are read by udev. Also adds some udev rules that are needed but which would normally loaded from the udev store path which is not included in the initrd.

Also ensures that the interfaces are unconfigured before switching to stage 2, switches to klibc's ipconfig due to more features and make boot.initrd.network.postCommands run regardless if network setup has succeeded.

Fixes #74328 #75314 #71447.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@fpletz fpletz requested review from flokli and arianvp Feb 8, 2020
@fpletz fpletz added this to the 20.03 milestone Feb 8, 2020
@fpletz
Copy link
Member Author

@fpletz fpletz commented Feb 8, 2020

@GrahamcOfBorg test predictable-interface-names initrdNetwork initrd-network-ssh

fpletz added 2 commits Sep 22, 2019
This makes predictable interfaces names available as soon as possible
with udev by adding the default network link units to initrd which are read
by udev. Also adds some udev rules that are needed but which would normally
loaded from the udev store path which is not included in the initrd.
Depending on the network management backend being used, if the interface
configuration in stage 1 is not cleared, there might still be some old
addresses or routes from stage 1 present in stage 2 after network
configuration has finished.
@fpletz fpletz force-pushed the fix-predictable-ifnames-in-initrd branch from ab02ac4 to ea7d024 Feb 8, 2020
@fpletz fpletz requested review from Ma27, elseym and worldofpeace Feb 8, 2020
@flokli
Copy link
Contributor

@flokli flokli commented Feb 8, 2020

From a quick glace, this looks like the right way to do (at least until we have systemd in initrd).

Can somebody with a "push luks keys over ssh in initrd" setup test this? I don't currently have such a setup around, but we should make sure it now works as intended.

Also, we should add something to the release notes, so people who are using such a setup and implemented their workarounds are aware names have changed and don't lock themselves out.

@Ma27
Ma27 approved these changes Feb 8, 2020
Copy link
Member

@Ma27 Ma27 left a comment

I have a setup which is fairly similar to nixos/tests/initrd-network-ssh and tested the first commit on multiple servers, didn't find any regressions :)

@flokli
Copy link
Contributor

@flokli flokli commented Feb 8, 2020

@petabyteboy @schmittlauch @erikarvstedt @emilazy could you also give this a very firm look?

fpletz added 3 commits Feb 8, 2020
This apparently has features that the version from Arch's
mkinitcpio-nfs-utils does not have. Fixes #75314.
As outlined in #71447, postCommands should always be run if networking
in initrd is enabled. regardless if the configuration actually
succeeded.
@fpletz
Copy link
Member Author

@fpletz fpletz commented Feb 8, 2020

cc @mweinelt for #71447

@emilazy
Copy link
Member

@emilazy emilazy commented Feb 8, 2020

I'll try and take a look at this soon -- for what it's worth, I personally successfully use this module and it's the closest thing I've seen to a Correct™ solution for this issue.

@fpletz
Copy link
Member Author

@fpletz fpletz commented Feb 8, 2020

@emilazy Those changes are actually included here but with some more fixes for stuff I encountered while reviewing the code. Just didn't notice @arianvp's PR earlier unfortunately.

@emilazy
Copy link
Member

@emilazy emilazy commented Feb 8, 2020

I'm referring to @ju1m's module here, not @arianvp's PR, which IIRC didn't quite work for me.

@flokli
Copy link
Contributor

@flokli flokli commented Feb 8, 2020

Marked this as a blocker, we should really try to get this fixed for 20.03.

@Ma27 Ma27 merged commit 70325e6 into master Mar 2, 2020
16 checks passed
16 checks passed
klibc on x86_64-darwin No attempt
Details
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
klibc on aarch64-linux Success
Details
klibc on x86_64-linux Success
Details
@Ma27 Ma27 deleted the fix-predictable-ifnames-in-initrd branch Mar 2, 2020
@Ma27
Copy link
Member

@Ma27 Ma27 commented Mar 2, 2020

Currently preparing the backport :)

@flokli
Copy link
Contributor

@flokli flokli commented Mar 2, 2020

@Ma27
Copy link
Member

@Ma27 Ma27 commented Mar 2, 2020

Backported the following patches:

  • 332d731 (nixos/stage-1: fix predictable interfaces names)
  • 3206aa9 (klibc: 2.0.4 -> 2.0.7)
  • 173c771 (nixos/initrd-network: use ipconfig from klibc)
  • 0bdf352 (nixos/release-notes: mention fix for predictable network-interfaces in initrd)

After re-reading @fpletz previous comment, I figured that the ipconfig-related fix is also part of the bugfixes and the main breaking change that should not be backported is the flush of the interfaces after stage-1.

@mweinelt
Copy link
Member

@mweinelt mweinelt commented Mar 2, 2020

Is 5897899 out of scope for a backport? It has the fix for #71447.

@flokli
Copy link
Contributor

@flokli flokli commented Mar 2, 2020

@Ma27
Copy link
Member

@Ma27 Ma27 commented Mar 2, 2020

and I'd also have preferred if those were part of the initial PR

Sorry for that, hit the PR-button too early :/

I'd personally say that's fine to backport as well

You're right, just missed it!

@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Mar 2, 2020

Backported the following patches:

* [332d731](https://github.com/NixOS/nixpkgs/commit/332d731a7a48e79acda2e4ce79f1245999a5a2e4) (nixos/stage-1: fix predictable interfaces names)

* [3206aa9](https://github.com/NixOS/nixpkgs/commit/3206aa985af95d48fde39c3613461882106cc7d4) (klibc: 2.0.4 -> 2.0.7)

* [173c771](https://github.com/NixOS/nixpkgs/commit/173c7715dedc9c88f54e970d70464319fe93c04d) (nixos/initrd-network: use ipconfig from klibc)

* [0bdf352](https://github.com/NixOS/nixpkgs/commit/0bdf352a0589252cf63ebbf50944d213f4cd6447) (nixos/release-notes: mention fix for predictable network-interfaces in initrd)

After re-reading @fpletz previous comment, I figured that the ipconfig-related fix is also part of the bugfixes and the main breaking change that should not be backported is the flush of the interfaces after stage-1.

Btw, please PR larger backports.

@Ma27
Copy link
Member

@Ma27 Ma27 commented Mar 4, 2020

Sorry for the delay, currently preparing a PR to backport 5897899.

Ma27 added a commit to Ma27/nixpkgs that referenced this pull request Mar 4, 2020
As outlined in NixOS#71447, postCommands should always be run if networking
in initrd is enabled. regardless if the configuration actually
succeeded.

(cherry picked from commit 5897899)

The backport of this patch has been requested in NixOS#79532[1]. The diff is
slightly off the original commit since some changes from
ea7d024 were needed, however this
commit shouldn't be backported as it potentially breaks existing setups.

[1] NixOS#79532 (comment)
@Ma27 Ma27 mentioned this pull request Mar 4, 2020
4 of 10 tasks complete
@Frostman
Copy link
Member

@Frostman Frostman commented May 12, 2020

@fpletz does it move building bridges to stage-1 as well? I have .useDHCP=true only for br0 and trying to understand if it'll continue working after switching to 20.03. Thanks.

@Nadrieril
Copy link
Contributor

@Nadrieril Nadrieril commented Jul 10, 2020

Hum, this seems to impose predictable interface names in stage-1 even if the main OS has predictableInterfaceNames = false. This has just cause my server to fail to get the interface up in initrd because I expected eth0 to exist. This should at least be mentioned in the release notes I think, it's really breaking

@flokli
Copy link
Contributor

@flokli flokli commented Jul 11, 2020

@Nadrieril can you show us a minimal NixOS configuration reproducing this?

There's nixos/tests/predictable-interface-names.nix which should discover exactly this, and it's not failing…

@Nadrieril
Copy link
Contributor

@Nadrieril Nadrieril commented Jul 14, 2020

Hmm, this happened on two of my servers so it's not just a fluke and probably not hardware-related either. But it's difficult to minimize, I can't just break my servers.
The one thing that's weird is that I did not have usePredictableInterfaceNames = false, yet the servers used to use eth0-like interfaces in normal use. So it could also be that I was relying on an old bug that has been fixed in the past year. If no one else has that problem then I won't bother trying to understand this mess.

@Ma27
Copy link
Member

@Ma27 Ma27 commented Jul 14, 2020

That's most likely the case: without this PR, NixOS had eth0-like interfaces when configuring a network in the initrd.

@Nadrieril
Copy link
Contributor

@Nadrieril Nadrieril commented Jul 14, 2020

Ohh, you mean the interface set up in initrd would carry on to the booted system? That explains a lot!

@ju1m
Copy link
Contributor

@ju1m ju1m commented Jul 14, 2020

@Nadrieril yes the interface names are kept from stage-1 to stage-2, AFAIU there's no more than a pivot_root between the two, no rmmod and modprobe that would reset the interface names. I've explained this in detail in this proposal #68953 (comment) ; note that I'm still using this code and it still works for me.

@flokli
Copy link
Contributor

@flokli flokli commented Jul 14, 2020

So this PR actually fixed a bug we didn't know about - even with networking.usePredictableInterfaceNames set to true (the default), doing things with them in initrd caused them to stick with their non-predictable name.

I don't think that's a bug we should consider a feature and apply workarounds for ;-)

We should however update the release note entry explaining the possibly changed behaviour, so people reading those are made aware.

@flokli
Copy link
Contributor

@flokli flokli commented Jul 14, 2020

Ah, nvm - we already refer to this PR in the release notes, and as this landed in 20.03 already, the amount of people still running into it while upgrading should be somewhat small.

@Ma27
Copy link
Member

@Ma27 Ma27 commented Jul 14, 2020

So this PR actually fixed a bug we didn't know about - even with networking.usePredictableInterfaceNames set to true (the default), doing things with them in initrd caused them to stick with their non-predictable name.

Well, actually this was one of the main issues before this PR (and one of the biggest challenges I had while trying to upgrade to 19.09).

The problem was that [they] did not have usePredictableInterfaceNames = false, so AFAICS everything works as expected now, right?

@leo60228
Copy link
Contributor

@leo60228 leo60228 commented Aug 22, 2020

Am I doing something wrong or does this break setting interface names with systemd link units?

@mweinelt
Copy link
Member

@mweinelt mweinelt commented Aug 22, 2020

A common pitfall with using network interfaces in initrd is that you need to set them admin down, because you cannot rename interfaces that are admin up. Is the interface properly set to down?

Does enabling this block fix your issue? https://github.com/NixOS/nixpkgs/pull/79532/files#diff-5c6eca5d6ffea3065ed16561bec3ca9dR139

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.