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-generate-config: observe device label if present #339995

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

acobster
Copy link

@acobster acobster commented Sep 6, 2024

Partially fixes #27789 by using matching devices found in /dev/disk/by-label/* before using any found in /dev/disk/by-uuid/*.

Wil T's install tutorial explains the motivation: https://www.youtube.com/watch?v=axOxLJ4BWmY&t=1295s

Description of changes

This doesn't change any packages; only newly generated hardware configurations are affected.

This does NOT fix #27789 as originally reported (I was not able to repro that myself), but does address this comment.

At some point the automatic detection of device labels broke and device UUIDs began taking precedent over labels. This makes setting up a new computer a more manual and error-prone process than it needs to be.

With this change, I can instead use the devices generated as-is in hardware-configuration.nix on reinstall.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 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
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

Partially fixes NixOS#27789 by using matching devices found in `/dev/disk/by-label/*` before using any found in `by-uuid`.

Wil T's install tutorial explains the motivation: https://www.youtube.com/watch?v=axOxLJ4BWmY&t=1295s
@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 Sep 6, 2024
@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Sep 6, 2024
@roberth
Copy link
Member

roberth commented Sep 8, 2024

I'm not all that knowledgeable about this topic, but I guess I can review the process.

  • Could you document the rationale for these priorities in a comment?

  • This should - presumably - break a test case. Let's see:

@ofborg build nixos-install-tools.tests

Copy link
Member

@Aleksanaa Aleksanaa left a comment

Choose a reason for hiding this comment

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

Yes this should be preferred. Further change would be also using by-partuuid in favor of by-uuid, but that's not tested

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Sep 17, 2024
@geovex
Copy link

geovex commented Oct 12, 2024

by-partlabel will also be nice to have (very handy for exotic configs)

@roberth
Copy link
Member

roberth commented Oct 13, 2024

This doesn't change any packages; only newly generated hardware configurations are affected.

Automated installations may still be affected by this, so this is worth adding to the release notes. Could you do that?

@sigprof
Copy link
Contributor

sigprof commented Nov 19, 2024

The choice between various device ID types may be complicated:

  • User-specified filesystem or partition labels have some chance of collisions if a “foreign” storage device originally from another machine is connected during boot for some reason (e.g., for data recovery purposes). UUIDs are safer in that respect.
    • One option to avoid some collisions is to include the hostname in the label; however, some filesystems have severe restrictions on the label size which may interfere with that (e.g., XFS supports at most 12 characters).
  • However, if LVM is used, filesystem UUIDs should not be used to identify devices, because LVM supports snapshots, which would appear to have the same UUIDs as the origin filesystems. Currently nixos-generate-config ignores this issue and uses /dev/disk/by-uuid/* in preference to /dev/mapper/*.
    • Swapping from /dev/disk/by-uuid/* to /dev/mapper/* could potentially introduce the same increased collision chance as using filesystem or partition labels. However, LVM volume group names often include the hostname, which partially mitigates the problem, and LVM would also detect duplicate volume group names with different UUIDs and refuse to work until the collision is resolved.
    • Also the /dev/mapper/* namespace is not exclusively used by LVM, therefore some extra checks might be needed.

@FliegendeWurst FliegendeWurst added the awaiting_changes (old Marvin label, do not use) label Dec 4, 2024
@wegank wegank added 12.approvals: 2 This PR was reviewed and approved by two reputable people and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Dec 23, 2024
@SigmaSquadron SigmaSquadron removed the awaiting_changes (old Marvin label, do not use) label Jan 5, 2025
@github-actions github-actions bot added 8.has: documentation This PR adds or changes documentation 8.has: changelog labels Jan 5, 2025
@acobster
Copy link
Author

acobster commented Jan 5, 2025

@roberth done.

@roberth
Copy link
Member

roberth commented Jan 6, 2025

@ofborg build nixos-install-tools.tests

@wegank wegank removed the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Jan 6, 2025
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Feb 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nixos-generate-config: use uuid instead of device if possible
10 participants