nixos/qemu-vm: refactor bootDisk using make-disk-image

[RaitoBezarius]

Description of changes

Depends on #207038.

A small cover letter on the changes:

We remove the bootDisk ad-hoc generation and rewire it up with make-disk-image, this unlocks various features:

• perform bootloader operations that have an impact on EFI variables (SecureBoot, etc.)
• factorization (everything is controlled by make-disk-image)
• clarification of the "current" boot configuration: https://github.com/NixOS/nixpkgs/pull/207039/files#diff-5f468553a33ed484ce0a34ef2f404fb7f37bd981ba95fa5ec666bcbdaeb3d6d9R253-R263 -- we have various mode of operations and this module is becoming confusing on what bunch of booleans realizes a specific configuration, pushing for an explicit string makes the code more maintainable and readable.
• exposes rootDevice, bootDevice and bootPartition which clarify further the different roles of the disk in the VM world we use, to cater for legacy (disk-level) and UEFI (partition-level) needs.
• undo persistBootDevice introduced in 678eed3

Testing is done on https://hydra.nixos.org/jobset/nixos/python-test-refactoring on a recent enough: nixos-unstable-small and treewide cleanups are done.

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:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• 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.05 Release Notes (or backporting 22.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.

[blitz]

@RaitoBezarius The comments sound like you still want to work on this. What's the status here?

[RaitoBezarius]

@RaitoBezarius The comments sound like you still want to work on this. What's the status here?

Didn't have time to fix my own comments yet, but it's open to additional comments.

[RaitoBezarius]

Latest evaluation: https://hydra.nixos.org/eval/1793584?compare=-172800&full=0#tabs-now-fail
From there, it seems like there are only few problematic jobs remaining.

[lilyinstarlight]

I'm not the most knowledgeable about the qemu-vm test stuff, but this change feels sane to me

Since it changes some semantics it could have some fallout for downstream users, but I don't imagine there are many and it should hopefully be easy for them to understand and update

[nixos-discourse]

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/2090

[lilyinstarlight]

It looks like nixosTests.bootspec.grub is failing with this PR with this log snippet:

nixos-disk-image> installing the GRUB 2 boot loader on /dev/vda...
nixos-disk-image> Installing for i386-pc platform.
nixos-disk-image> /nix/store/07bws5lfvgz0a6psxbyhzzz0sbrnqdab-grub-2.06/sbin/grub-install: warning: this GPT partition label contains no BIOS Boot Partition; embedding won't be possible.
nixos-disk-image> /nix/store/07bws5lfvgz0a6psxbyhzzz0sbrnqdab-grub-2.06/sbin/grub-install: warning: Embedding is not possible.  GRUB can only be installed in this setup by using blocklists.  However, blocklists are UNRELIABLE and their use is discouraged..
nixos-disk-image> /nix/store/07bws5lfvgz0a6psxbyhzzz0sbrnqdab-grub-2.06/sbin/grub-install: error: will not proceed with blocklists.
nixos-disk-image> /nix/store/nj03jkzbac12apslfmhhmss8qrlvx7wg-install-grub.pl: installation of GRUB on /dev/vda failed: No such file or directory

[RaitoBezarius]

It looks like nixosTests.bootspec.grub is failing with this PR with this log snippet:

nixos-disk-image> installing the GRUB 2 boot loader on /dev/vda...
nixos-disk-image> Installing for i386-pc platform.
nixos-disk-image> /nix/store/07bws5lfvgz0a6psxbyhzzz0sbrnqdab-grub-2.06/sbin/grub-install: warning: this GPT partition label contains no BIOS Boot Partition; embedding won't be possible.
nixos-disk-image> /nix/store/07bws5lfvgz0a6psxbyhzzz0sbrnqdab-grub-2.06/sbin/grub-install: warning: Embedding is not possible.  GRUB can only be installed in this setup by using blocklists.  However, blocklists are UNRELIABLE and their use is discouraged..
nixos-disk-image> /nix/store/07bws5lfvgz0a6psxbyhzzz0sbrnqdab-grub-2.06/sbin/grub-install: error: will not proceed with blocklists.
nixos-disk-image> /nix/store/nj03jkzbac12apslfmhhmss8qrlvx7wg-install-grub.pl: installation of GRUB on /dev/vda failed: No such file or directory

If I remember well, the failure is alas unrelated. It's because GRUB tries to install legacy for a UEFI only target.
I implemented the fix somewhere and lost it :D.

[lilyinstarlight]

If I remember well, the failure is alas unrelated. It's because GRUB tries to install legacy for a UEFI only target.

Yeah the error message sounded like that was the issue

It looks like the NixOS GRUB module assumes that it needs to install legacy unless boot.loader.grub.device = "nodev" is set (in which case if I'm following this Perl right (ugh) then it will assume it needs to do an EFI-only install)

[RaitoBezarius]

If I remember well, the failure is alas unrelated. It's because GRUB tries to install legacy for a UEFI only target.

Yeah the error message sounded like that was the issue

It looks like the NixOS GRUB module assumes that it needs to install legacy unless boot.loader.grub.device = "nodev" is set (in which case if I'm following this Perl right (ugh) then it will assume it needs to do an EFI-only install)

Thank you for going in the rabbit hole, I rediscover this each time. :)

[lilyinstarlight]

Thank you for going in the rabbit hole, I rediscover this each time. :)

So I don't know if this is "correct" necessarily, but this diff on top of this PR does allow a number of grub tests to pass again (those which currently pass on master and fail on this PR):

diff --git a/nixos/modules/virtualisation/qemu-vm.nix b/nixos/modules/virtualisation/qemu-vm.nix
index f85567c42f2..fa928a1ba2f 100644
--- a/nixos/modules/virtualisation/qemu-vm.nix
+++ b/nixos/modules/virtualisation/qemu-vm.nix
@@ -849,7 +849,7 @@ in
             ${opt.writableStore} = false;
         '';
 
-    boot.loader.grub.device = mkVMOverride cfg.bootLoaderDevice;
+    boot.loader.grub.device = mkVMOverride (if cfg.useEFIBoot then "nodev" else cfg.bootLoaderDevice);
     boot.loader.grub.gfxmodeBios = with cfg.resolution; "${toString x}x${toString y}";
     virtualisation.rootDevice = mkDefault suggestedRootDevice;
 

[RaitoBezarius]

@lilyinstarlight Adressed and latest evaluation shows that it fixes the GRUB tests: https://hydra.nixos.org/eval/1793709

[nikstur]

Very cool change. The comments and descriptions could use some more editing. Most of my comments are either nitpicks or revolve around naming. Although I think naming is very important, I think it is more important to get this PR across the finish line. We can still improve the naming later.

[RaitoBezarius]

Started a new evaluation here: https://hydra.nixos.org/jobset/nixos/python-test-refactoring

[RaitoBezarius]

Finally, this will be merged once ofborg CI passes.

[RaitoBezarius]

Thanks to everyone involved in this journey. :)

[alyssais]

This PR broke nixosTests.kexec. It seems like the final Hydra eval had that test as broken, so maybe it's worth a look to see if any other tests were found to be broken during that eval and overlooked?

[RaitoBezarius]

Hmm, I will investigate this further. I remember that some of the stuff seemed already broken on master but I may have done mistakes. Thank you for the heads-up! Le ven. 19 mai 2023 à 23:45, Alyssa Ross ***@***.***> a écrit :

…

This PR broke nixosTests.kexec. It seems like the final Hydra eval had that test as broken, so maybe it's worth a look to see if any other tests were found to be broken during that eval and overlooked? — Reply to this email directly, view it on GitHub <#207039 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACMZRE4RFCHNMRJXUHLYU3XG7SZBANCNFSM6AAAAAATE4LUVY> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>