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/qemu-vm: use persistent block device names #236656

Merged

Conversation

nikstur
Copy link
Contributor

@nikstur nikstur commented Jun 8, 2023

Description of changes

This change removes the bespoke logic around identifying block devices. Instead of trying to find the right device by iterating over qemu.drives and guessing the right partition number (e.g. /dev/vda{1,2}), devices are now identified by persistent names provided by udev in /dev/disk/by-*.

Before this change, the root device was formatted on demand in the initrd. However, this makes it impossible to use filesystem identifiers to identify devices. Now, the formatting step is performed before the VM is started. Because some tests, however, rely on this behaviour, a utility function to replace this behaviour in added in /nixos/tests/common/auto-format-root-device.nix.

Devices that contain neither a partition table nor a filesystem are identified by their hardware serial number which is injecetd via QEMU (and is thus persistent and predictable). PCI paths are not a reliably way to identify devices because their availability and numbering depends on the QEMU machine type.

This change makes the module more robust against changes in QEMU and the kernel (non-persistent device naming) and by decoupling abstractions (i.e. rootDevice, bootPartition, and bootLoaderDevice) enables further improvement down the line.

For all the tests to succeed, depends on #234030.

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/)
  • 23.11 Release Notes (or backporting 23.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.

@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 Jun 8, 2023
@nikstur nikstur requested review from roberth and alyssais June 8, 2023 13:55
@RaitoBezarius
Copy link
Member

This is beautiful. :)

@nikstur nikstur force-pushed the qemu-vm-persistent-block-device-names branch 3 times, most recently from 87c80f3 to d711d69 Compare June 8, 2023 19:26
@nikstur
Copy link
Contributor Author

nikstur commented Jun 8, 2023

Could this solve the issue trying to be solved in #233847 ?

@RaitoBezarius
Copy link
Member

Could this solve the issue trying to be solved in #233847 ?

Only partially, here's the issue is related to Nix closures I believe

@roberth
Copy link
Member

roberth commented Jun 9, 2023

I'm not familiar with this part and I can't do a deep dive now, but the idea seems good and I don't see any problems besides breaking tests that depend on these details, which you've already identified and I think is worth breaking.

@roberth roberth removed their request for review June 9, 2023 10:44
@RaitoBezarius
Copy link
Member

Once we are done with directBoot, I can push your branch to our Hydra jobset.

@RaitoBezarius
Copy link
Member

Pushing 532ea30 as a baseline evaluation.

@RaitoBezarius
Copy link
Member

Hello hello, I bring the failures: https://hydra.nixos.org/eval/1796336#tabs-now-fail — :D

@nikstur nikstur force-pushed the qemu-vm-persistent-block-device-names branch from d711d69 to 2c8fce2 Compare June 12, 2023 21:36
@nikstur
Copy link
Contributor Author

nikstur commented Jun 12, 2023

I think I fixed all deterministic failures. @RaitoBezarius can you push another eval?

@nikstur nikstur force-pushed the qemu-vm-persistent-block-device-names branch from 2c8fce2 to ee60d30 Compare June 12, 2023 22:55
@RaitoBezarius
Copy link
Member

Pushed ee60d30

@nikstur
Copy link
Contributor Author

nikstur commented Jun 15, 2023

Looks like only unrelated failures are left: https://hydra.nixos.org/eval/1796429?compare=-184800&full=0

@RaitoBezarius
Copy link
Member

nods

@RaitoBezarius
Copy link
Member

RaitoBezarius commented Jun 15, 2023

Please write a release note for this, maybe introduce a nixpkgs internal change section in 23.11 based on what we have done for 23.05.

This change removes the bespoke logic around identifying block devices.
Instead of trying to find the right device by iterating over
`qemu.drives` and guessing the right partition number (e.g.
/dev/vda{1,2}), devices are now identified by persistent names provided
by udev in /dev/disk/by-*.

Before this change, the root device was formatted on demand in the
initrd. However, this makes it impossible to use filesystem identifiers
to identify devices. Now, the formatting step is performed before the VM
is started. Because some tests, however, rely on this behaviour, a
utility function to replace this behaviour in added in
/nixos/tests/common/auto-format-root-device.nix.

Devices that contain neither a partition table nor a filesystem are
identified by their hardware serial number which is injecetd via QEMU
(and is thus persistent and predictable). PCI paths are not a reliably
way to identify devices because their availability and numbering depends
on the QEMU machine type.

This change makes the module more robust against changes in QEMU and the
kernel (non-persistent device naming) and by decoupling abstractions
(i.e. rootDevice, bootPartition, and bootLoaderDevice) enables further
improvement down the line.
@nikstur nikstur force-pushed the qemu-vm-persistent-block-device-names branch from ee60d30 to 0bdba6c Compare June 16, 2023 17:36
@nikstur
Copy link
Contributor Author

nikstur commented Jun 16, 2023

Please write a release note for this

Done

@RaitoBezarius RaitoBezarius merged commit 3d941b6 into NixOS:master Jun 17, 2023
@emilazy
Copy link
Member

emilazy commented Jun 19, 2023

This broke darwin.builder; mkfs.ext4 cannot be run on Darwin hosts. I don't see any way to make it work with this approach off the top of my head (other than maybe making it not depend on disk images at all?).

cc @Gabriella439

@emilazy
Copy link
Member

emilazy commented Jun 19, 2023

Ah, after reading the commit more, maybe it's as simple as turning off useDefaultFilesystems and using autoFormat? I thought the mkfs.ext4 was being added unconditionally.

@alyssais
Copy link
Member

Why can't mkfs.ext4 run on Darwin?

@emilazy
Copy link
Member

emilazy commented Jun 19, 2023

Hmm, I wasn't expecting e2fsprogs to be portable enough to build on Darwin but apparently it is, except that all the tests fail horribly. Not sure if it's just a matter of skipping the tests on Darwin or if something is more fundamentally broken there.

The actual observed issue is that the VM run script references a Linux mkfs.ext4 (but Darwin versions of all other tools), I guess because pkgs here is for the guest system?

@emilazy
Copy link
Member

emilazy commented Jun 19, 2023

Unfortunately it is not that simple:

emily@yuyuko ~> /nix/store/66wby349s40sp4pyzj40s01kzrxhpc7f-e2fsprogs-1.47.0-bin/bin/mkfs.ext4 test.img
mke2fs 1.47.0 (5-Feb-2023)
dyld[13680]: missing symbol called
fish: Job 1, '/nix/store/66wby349s40sp4pyzj40…' terminated by signal SIGABRT (Abort)

I don't really have any idea where to start debugging it. Homebrew has a package that apparently works, so it should be possible to fix. I'll try to take a look tomorrow.

@nikstur nikstur mentioned this pull request Jun 19, 2023
12 tasks
@nikstur
Copy link
Contributor Author

nikstur commented Jun 19, 2023

I opened #238596 which should at least use the right e2fsprogs package from the host pkgs. Can't really help with mkfs.ext4 not working properly on darwin since I don't have anything to test.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/darwin-builder-on-an-m1-mac-problems-with-mkfs-ext4/29523/3

@alyssais
Copy link
Member

Apparently e2fsprogs built for Darwin until the recent upgrade, so bisecting might be an option?

@alyssais alyssais mentioned this pull request Jun 25, 2023
12 tasks
@emilazy
Copy link
Member

emilazy commented Jun 25, 2023

I tracked it down to #238791 which will be fixed with the upcoming Darwin stdenv rework; sorry for not linking back.

chkno added a commit to chkno/nixos-qemu-vm-isolation that referenced this pull request Jul 21, 2023
NixOS/nixpkgs#236656 changed NixOS's qemu-vm
disk-finding mechanism to use filesystem labels.

squashfs doesn't support filesystem labels (see
plougher/squashfs-tools#59 ).

So we can't use squashfs anymore.  :(

The simple test's nix store image is 240M as squashfs and 1.3G as ext4.
@symphorien
Copy link
Member

This broke nixos-rebuild build-vm: #292156

@symphorien
Copy link
Member

My bad, I need to remove the old nixos.qcow2 image in the current directory. The new code does not handle old images.

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: changelog 8.has: documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10
Projects
Development

Successfully merging this pull request may close these issues.

7 participants