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
qemu-vm: Fix useBootLoader, remove /boot
read-only restriction
#92122
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but I very rarely use this -- someone else might have a more educated opinion 😄
Also FYI @edolstra that this removes the CCing also the reviewers I put on #85895, @OmnipotentEntity, @Mic92, @symphorien, @emilazy. |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test that verifies useBootLoader? This both keeps it from breaking again and also really helps other folks working in here to make changes with confidence that they're not breaking stuff.
@@ -528,6 +567,8 @@ in | |||
|
|||
virtualisation.qemu.drives = mkMerge [ | |||
(mkIf cfg.useBootLoader [ | |||
# The order of this list determines the device names, see |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed this brittle order-determining-device-names thing as part of #72354. Sorry that I didn't aggressively enough pursue reviewers to get that merged to make this change easier. I've split out the cleanup and refactor parts into the separate PR #92205 that can hopefully be reviewed and merged faster.
I think it'd be great if we could have fewer "/dev/..." strings flying around in here, and just one diskInterface == "scsi"
test about device names.
Consider reviewing #92205? (Rebasing this atop #92205 would be an excellent way to review #92205!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, currently testing the rebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chkno With #92205 (comment) fixed, this PR rebased on top of yours passes the VM based test I wrote for #85895.
So this is all looking very good, thanks!
Just adding a note here that I also need a writable If this change is accepted, we should remove this statement from the
|
1b496d4
to
e599834
Compare
Great point, I've added that to the commit. |
010aa57
to
8d5ba86
Compare
There does not seem to be a good reason to do this, and it breaks running `nixos-rebuild boot --install-bootloader` inside the VM.
boot.loader.grub.device` was hardcoded to `bootDevice`, which is wrong, because that's the device for `/`, and with `useBootLoader` the boot loader is not on that device. This bug probably came into existence because of bad naming; `virtualisation.bootDevice` has description "The disk to be used for the root filesystem", which is very confusing; it should be `.rootDevice` then! Unfortunately, the description is right and the attribute name is wrong, so it is not easy to change this without deprecation. This commit ensures that even if you use `useBootLoader` and `diskInterface == "scsi"`, the created VM can boot through, and can run `nixos-rebuild afterwards. It also adds extra commentary to explain what's going on in this module in general in relation to `useBootLoader`.
8d5ba86
to
5b16d4c
Compare
I've addressed all feedback and re-tested, will probably merge tomorrow. |
TODO
Motivation for this change
/boot
read-only, because it is not necessary and only forbids useful things.useBootLoader
.See commit messages for details.
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)I've tested it via the
grub-test-vm
in https://github.com/nh2/nixos-vm-examples/tree/0b29937fcb23424b95aa98121c94db02fd9b920d