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/udev: systemd initrd improvements #171021

Conversation

dasJ
Copy link
Member

@dasJ dasJ commented Apr 30, 2022

First, add the builtin udev rules to /etc/udev/rules.d so they are used.
Then, add all networkd .link units to the initrd. This is done in the
old stage 1 as well so I assume this is needed even when networkd is not
used. I assume this is for things like changing the MAC address.

Also limit the number of udev/lib binaries that is put into the initrd
because the old initrd doesn't use all units either.

Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

First, add the builtin udev rules to /etc/udev/rules.d so they are used.
Then, add all networkd .link units to the initrd. This is done in the
old stage 1 as well so I assume this is needed even when networkd is not
used. I assume this is for things like changing the MAC address.

Also limit the number of udev/lib binaries that is put into the initrd
because the old initrd doesn't use all units either.
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Apr 30, 2022
@arianvp
Copy link
Member

arianvp commented Apr 30, 2022 via email

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Apr 30, 2022
@dasJ
Copy link
Member Author

dasJ commented Apr 30, 2022

We'd probably have to move these to the network module if it exists because boot.initrd.systemd.contents doesn't merge directories so the derivation with the .link files would conflict with the one carrying the .network files :/

# This must be done in stage 1 to avoid race conditions between udev and
# network daemons.
# TODO move this into the initrd-network module when it exists
initrdLinkUnits = pkgs.runCommand "initrd-link-units" {} ''
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this. We don't have to do this in the old initrd, why should we have to with this one?

Copy link
Member

Choose a reason for hiding this comment

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

I just don't think we did link files in the old initrd. i.e. predictible interface names were not used.

Copy link
Member

Choose a reason for hiding this comment

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

But can't we use the initrd.systemd.units logic for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

@arianvp That would place units in /etc/systemd/system, not in /etc/systemd/network.

Copy link
Member

Choose a reason for hiding this comment

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

ah yes; but we have similar logic in the networkd module in stage-2. Perhaps we want to copy that to stage-1 under initrd.systemd.network.links ? (https://search.nixos.org/options?channel=21.11&show=systemd.network.links&from=0&size=50&sort=relevance&type=packages&query=systemd.network.links)

Copy link
Contributor

Choose a reason for hiding this comment

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

@arianvp see: #169116 :)

Copy link
Member

Choose a reason for hiding this comment

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

So. we can keep this workaround for now; and then clean it up later once that PR is ready? Must just make sure to not forget.

Copy link
Member Author

@dasJ dasJ May 4, 2022

Choose a reason for hiding this comment

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

@ElvishJerricco we do this because the code was actually in the old stage 1 as well ;) It's not a new invention.
Old code:

linkUnits = pkgs.runCommand "link-units" {
allowedReferences = [ extraUtils ];
preferLocalBuild = true;
} (''
mkdir -p $out
cp -v ${udev}/lib/systemd/network/*.link $out/
'' + (
let
links = filterAttrs (n: v: hasSuffix ".link" n) config.systemd.network.units;
files = mapAttrsToList (n: v: "${v.unit}/${n}") links;
in
concatMapStringsSep "\n" (file: "cp -v ${file} $out/") files
));

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, heh cool. So yea, keep this, and revisit whenever we actually want to take that networkd patch seriously.

@Artturin Artturin added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label May 4, 2022
@lovesegfault lovesegfault merged commit 9a41fab into NixOS:master May 4, 2022
@dasJ dasJ deleted the feat/systemd-stage-1-udev-improvements branch May 4, 2022 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 12.approvals: 2 This PR was reviewed and approved by two reputable people
Projects
Development

Successfully merging this pull request may close these issues.

5 participants