Skip to content

nixos/tests/netboot: repair testing#228346

Open
RaitoBezarius wants to merge 3 commits intomasterfrom
netboot-tests
Open

nixos/tests/netboot: repair testing#228346
RaitoBezarius wants to merge 3 commits intomasterfrom
netboot-tests

Conversation

@RaitoBezarius
Copy link
Member

Description of changes

This repairs netboot testing, performs some cleanup to move to modern QEMU test infrastructure.

Depends on #228047.

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.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
  • 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 Apr 26, 2023
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Apr 26, 2023
@RaitoBezarius RaitoBezarius force-pushed the netboot-tests branch 2 times, most recently from 81e7333 to 102bc20 Compare April 26, 2023 19:26
@nikstur
Copy link
Contributor

nikstur commented Apr 26, 2023

Very cool. Misleading title though. Repairing netboot testing is just a side effect of the change :D

@RaitoBezarius
Copy link
Member Author

Very cool. Misleading title though. Repairing netboot testing is just a side effect of the change :D

heh, I take any suggestions :)

It's still WIP because I have some semantic issues with the solutions.

@alyssais
Copy link
Member

Testing with the second to last commit as instructed, my VM is still being created with a block device where AIUI there shouldn't be one:

nixosTest {
  name = "test";
  nodes.machine = {
    imports = [ nixos/modules/installer/netboot/netboot-minimal.nix ];
    virtualisation.diskImage = null;
    virtualisation.useNixStoreImage = true;
    virtualisation.mountHostNixStore = false;
  };
  testScript = "machine.fail('stat /dev/vda')"; 
}

It looks like fileSystems still ends up with /nix/store being mounted from /dev/vda.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. and removed 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. labels May 5, 2023
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label May 5, 2023
@RaitoBezarius RaitoBezarius force-pushed the netboot-tests branch 2 times, most recently from 9e37f60 to c71fbaa Compare May 5, 2023 16:14
@nikstur
Copy link
Contributor

nikstur commented Jun 21, 2023

Can you rebase this?

@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jun 22, 2023
@nikstur
Copy link
Contributor

nikstur commented Jun 22, 2023

It seems like neither nixosTests.biosNetboot nor nixosTests.uefiNetboot is actually broken. Neither in 23.05 nor on master. Is this just a refactor then? Or am I missing something?

@RaitoBezarius
Copy link
Member Author

You're right, this deserves more investigation.

@nikstur
Copy link
Contributor

nikstur commented Jul 3, 2023

@alyssais do you have some more insight on what is currently broken for netbooting?

@alyssais alyssais added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 6, 2023
@alyssais
Copy link
Member

alyssais commented Nov 6, 2023

It's not that netboot tests are broken, it's that directBoot is broken, as you'll see if you try to run the directBoot test from this PR without the first commit. The NixOS netboto tests don't use directBoot, so they're still working.

@RaitoBezarius RaitoBezarius force-pushed the netboot-tests branch 2 times, most recently from ff45b56 to e971b6b Compare January 25, 2024 01:36
@RaitoBezarius
Copy link
Member Author

So really: 8321fc6 (#228346) is a commit that should be three:

  • modernize the boot test
  • force link the channel or fail at boot
  • enable fine-grained override of each filesystem if they are not completely swapped <- this one, @nikstur, is the one I guess we need to discuss.

Next commits, AFAIK, are pretty much uncontroversial, right?

@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 25, 2024
@nikstur
Copy link
Contributor

nikstur commented Jan 26, 2024

  • enable fine-grained override of each filesystem if they are not completely swapped

This one shouldn't be required anymore since you can set virtualisation.fileSystems = lib.mkForce { } now.

@RaitoBezarius
Copy link
Member Author

  • enable fine-grained override of each filesystem if they are not completely swapped

This one shouldn't be required anymore since you can set virtualisation.fileSystems = lib.mkForce { } now.

How does that work when you want to keep the virtualization filesystems, and you want to override yours? (remember: my usecase is a bit different from yours :P — I need the intelligence of qemu-vm.nix and the netboot mechanism)

Sometimes, you would like to inherit from the VM framework and your user
code because you know better than the VM framework rather than replacing
wholesale the fileSystems module.

This came up for netboot testing needs.
This verifies that we can direct boot a netboot image as expected.
…hardcoding `initrd`, disable compression for initrd
@RaitoBezarius
Copy link
Member Author

PTAL @nikstur. I moved the irrelevant stuff and fixed the direct boot test again.

@RaitoBezarius RaitoBezarius marked this pull request as ready for review February 11, 2024 16:58
Copy link
Contributor

@nikstur nikstur left a comment

Choose a reason for hiding this comment

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

The description of the PR doesn't really fit anymore. You're not really reparing anything and also not cleaning up the old infrastructure. Instead you're adding a new direct boot test (which I still think is worthwhile).


# We only want to set those options in the context of
# the QEMU infrastructure.
virtualisation = lib.optionalAttrs (options ? virtualisation.directBoot) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really understand why we need to set this here. If this needs to be re-usable between tests it should be a fixure in tests/common IMO

# filesystems don't necessarily exist in the VM). You can disable this
# override by setting `virtualisation.fileSystems = lib.mkForce { };`.
fileSystems = lib.mkIf (cfg.fileSystems != { }) (mkVMOverride cfg.fileSystems);
fileSystems = lib.mkIf (cfg.fileSystems != { }) (lib.mapAttrs (n: v: mkVMOverride v) cfg.fileSystems);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we really need this. You could just set virtualisation.fileSystems to { } to use just the ones defined in netboot.nix. Is there actually a good reason to mix the ones defined by qemu-vm.nix and netboot.nix?

imports = [ ../modules/installer/netboot/netboot-minimal.nix ];
virtualisation.memorySize = 4096;
};
testScript = "machine.fail('stat /dev/vda')";
Copy link
Contributor

Choose a reason for hiding this comment

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

We can probably do better than this. What about checking whether the nix store is mounted via a loop device? That's more of a key characteristic of netboot for me.

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 4, 2024
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 2, 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 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 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/` 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.

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

4 participants