nixos/qemu-vm: refactor bootDisk using make-disk-image by RaitoBezarius · Pull Request #207039 · NixOS/nixpkgs

RaitoBezarius · 2022-12-20T20:18:35Z

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

nixos/lib/make-disk-image.nix

nixos/maintainers/scripts/ec2/amazon-image.nix

nixos/modules/virtualisation/qemu-vm.nix

blitz · 2023-01-10T08:31:58Z

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

RaitoBezarius · 2023-01-12T11:53:13Z

@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 · 2023-04-15T12:55:02Z

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/modules/virtualisation/qemu-vm.nix

nixos-discourse · 2023-04-16T20:06:58Z

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 · 2023-04-16T21:38:57Z

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 · 2023-04-16T21:42:26Z

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 · 2023-04-16T22:08:01Z

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 · 2023-04-16T22:08:51Z

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 · 2023-04-16T22:21:18Z

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 · 2023-04-19T06:40:33Z

@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.

nixos/tests/bootspec.nix

nixos/modules/virtualisation/qemu-vm.nix

nixos/tests/bootspec.nix

nixos/modules/virtualisation/qemu-vm.nix

RaitoBezarius · 2023-04-19T23:50:17Z

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

…e → rootDevice

…json

This option has been introduced in 678eed3 without realizing there was this PR inflight, unfortunately, it collide with what this PR does and make it irrelevant. Therefore, I remove it here.

I do a lot of work on QEMU VM and make-disk-image and I was bitten by an unnotified change recently, so I want to chime in the future changes of this file.

RaitoBezarius · 2023-04-21T11:01:04Z

Finally, this will be merged once ofborg CI passes.

RaitoBezarius · 2023-04-21T11:47:55Z

Thanks to everyone involved in this journey. :)

alyssais · 2023-05-19T21:45:42Z

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 · 2023-05-20T00:48:06Z

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: ***@***.***>

github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` labels Dec 20, 2022

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 8.has: clean-up This PR removes packages or removes other cruft labels Dec 20, 2022

RaitoBezarius mentioned this pull request Dec 20, 2022

nixos/qemu: Support System Management Mode #207043

Closed

13 tasks

RaitoBezarius force-pushed the qemu-boot-disk-using-make-disk-image branch from 0102cdc to 486bf1e Compare December 20, 2022 21:11

RaitoBezarius mentioned this pull request Dec 21, 2022

qemu-vm: remove bootDisk, refactor using make-disk-image with better EFI support #203641

Closed

20 tasks

RaitoBezarius force-pushed the qemu-boot-disk-using-make-disk-image branch from 486bf1e to b2ac559 Compare December 26, 2022 11:08

RaitoBezarius marked this pull request as ready for review December 26, 2022 11:08

github-actions bot removed the 8.has: documentation This PR adds or changes documentation label Dec 26, 2022

RaitoBezarius requested review from Mic92, blitz, dasJ, nikstur and oxalica December 26, 2022 11:08

RaitoBezarius commented Dec 26, 2022

View reviewed changes

nixos/lib/make-disk-image.nix Outdated Show resolved Hide resolved

RaitoBezarius commented Dec 26, 2022

View reviewed changes

nixos/maintainers/scripts/ec2/amazon-image.nix Outdated Show resolved Hide resolved

RaitoBezarius commented Dec 26, 2022

View reviewed changes

nixos/modules/virtualisation/qemu-vm.nix Show resolved Hide resolved

Mic92 requested a review from Lassulus December 26, 2022 11:21

RaitoBezarius mentioned this pull request Mar 15, 2023

Properly handle fwupd update capsules, take 2 nix-community/lanzaboote#131

Merged

RaitoBezarius force-pushed the qemu-boot-disk-using-make-disk-image branch from b2ac559 to b82c8f9 Compare March 16, 2023 20:05

github-actions bot added the 8.has: documentation This PR adds or changes documentation label Mar 16, 2023

RaitoBezarius force-pushed the qemu-boot-disk-using-make-disk-image branch from b82c8f9 to 5b082f5 Compare March 16, 2023 20:10

github-actions bot removed the 8.has: documentation This PR adds or changes documentation label Mar 16, 2023

RaitoBezarius force-pushed the qemu-boot-disk-using-make-disk-image branch 2 times, most recently from 3fde81c to 371d377 Compare March 16, 2023 23:32

RaitoBezarius force-pushed the qemu-boot-disk-using-make-disk-image branch 2 times, most recently from fd8d369 to 5724827 Compare April 15, 2023 17:46

lilyinstarlight reviewed Apr 16, 2023

View reviewed changes

nixos/modules/virtualisation/qemu-vm.nix Show resolved Hide resolved

nixos/modules/virtualisation/qemu-vm.nix Outdated Show resolved Hide resolved

nixos/modules/virtualisation/qemu-vm.nix Outdated Show resolved Hide resolved

RaitoBezarius force-pushed the qemu-boot-disk-using-make-disk-image branch from 5724827 to 792fd0a Compare April 16, 2023 20:47

RaitoBezarius force-pushed the qemu-boot-disk-using-make-disk-image branch from 792fd0a to 07f5a70 Compare April 17, 2023 09:52

nikstur approved these changes Apr 19, 2023

View reviewed changes

RaitoBezarius force-pushed the qemu-boot-disk-using-make-disk-image branch 2 times, most recently from 318c68b to 58f7c4e Compare April 19, 2023 23:49

RaitoBezarius force-pushed the qemu-boot-disk-using-make-disk-image branch from 58f7c4e to e81f8d2 Compare April 20, 2023 00:02

RaitoBezarius added 5 commits April 21, 2023 13:00

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

76c7b65

nixos/tests(treewide): vdb → vda, vdc → vdb, vdd → vdc, … / bootDevic…

58f4c39

…e → rootDevice

nixos/tests/bootspec: assert for boot.json rather than bootspec/boot.…

e3a41f3

…json

nixos/virtualisation/qemu-vm: remove persistBootDevice option

614b83a

This option has been introduced in 678eed3 without realizing there was this PR inflight, unfortunately, it collide with what this PR does and make it irrelevant. Therefore, I remove it here.

CODEOWNERS: add raitobezarius to qemu-vm.nix

76f1b63

I do a lot of work on QEMU VM and make-disk-image and I was bitten by an unnotified change recently, so I want to chime in the future changes of this file.

RaitoBezarius force-pushed the qemu-boot-disk-using-make-disk-image branch from e81f8d2 to 76f1b63 Compare April 21, 2023 11:00

RaitoBezarius merged commit 4b4e4c3 into master Apr 21, 2023

alyssais deleted the qemu-boot-disk-using-make-disk-image branch April 28, 2023 13:27

Uh oh!

Conversation

RaitoBezarius commented Dec 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes

Things done

Uh oh!

Uh oh!

Uh oh!

Uh oh!

blitz commented Jan 10, 2023

Uh oh!

RaitoBezarius commented Jan 12, 2023

Uh oh!

RaitoBezarius commented Apr 15, 2023

Uh oh!

lilyinstarlight left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nixos-discourse commented Apr 16, 2023

Uh oh!

lilyinstarlight commented Apr 16, 2023

Uh oh!

RaitoBezarius commented Apr 16, 2023

Uh oh!

lilyinstarlight commented Apr 16, 2023

Uh oh!

RaitoBezarius commented Apr 16, 2023

Uh oh!

lilyinstarlight commented Apr 16, 2023

Uh oh!

RaitoBezarius commented Apr 19, 2023

Uh oh!

nikstur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RaitoBezarius commented Apr 19, 2023

Uh oh!

RaitoBezarius commented Apr 21, 2023

Uh oh!

RaitoBezarius commented Apr 21, 2023

Uh oh!

alyssais commented May 19, 2023

Uh oh!

RaitoBezarius commented May 20, 2023 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

RaitoBezarius commented Dec 20, 2022 •

edited

Loading

lilyinstarlight left a comment •

edited

Loading