runInLinuxVM: fix for structured attrs #354535

Ma27 · 2024-11-08T16:38:12Z

Closes #334705
Addresses #205690

The main issue was that the output variable (i.e. $out and friends) didn't exist. I figured the easiest way to add those is to source stdenv here. Given that we build another derivation in this builder, it's pretty likely that stdenv gets pulled already, so I don't expect a real overhead here.

Also, this mounts /build into the VM: this is required to make sure .attrs.json & .attrs.sh are available. Dropped the mount of xchg into /tmp now since it's also part of /build.

cc @ShamrockLee

Things done

Add a 👍 reaction to pull requests you find important.

ShamrockLee

Many thanks for working on this problem!

pkgs/build-support/vm/default.nix

Ma27 · 2024-11-08T18:03:43Z

Thanks for the review, squashed the changes into the previous commit.

ShamrockLee

I tested with hello made structured built with runInLinuxVM. LGTM!

nix-build --no-out-link --expr "with (import ./. { }); vmTools.runInLinuxVM (hello.overrideAttrs { __structuredAttrs = true; })"

Adding a test under pkgs.tests would be great, but that's not a blocker.

I'm unfamiliar with the implementation of vm.nix-related build helpers. Having another pair of eyes from someone familiar with this topic would be safer.

pkgs/build-support/vm/default.nix

wolfgangwalther · 2024-11-16T17:17:48Z

Because my comments are naturally not on the right lines, when they didn't change, here's what I tested with:

diff --git a/pkgs/build-support/vm/default.nix b/pkgs/build-support/vm/default.nix
index dc5c29fcb733..147a030ac139 100644
--- a/pkgs/build-support/vm/default.nix
+++ b/pkgs/build-support/vm/default.nix
@@ -169,6 +169,7 @@ rec {
 
   stage2Init = writeScript "vm-run-stage2" ''
     #! ${bash}/bin/sh
+    set -euo pipefail
     source /build/xchg/saved-env
     if [ -f "''${NIX_ATTRS_SH_FILE-}" ]; then
       source "$NIX_ATTRS_SH_FILE"
@@ -183,7 +184,6 @@ rec {
     export NIX_BUILD_TOP=/tmp
     export TMPDIR=/tmp
     export PATH=/empty
-    out="$1"
     cd "$NIX_BUILD_TOP"
 
     if ! test -e /bin/sh; then
@@ -240,10 +240,6 @@ rec {
 
 
   vmRunCommand = qemuCommand: writeText "vm-run" ''
-    if [ -f "''${NIX_ATTRS_SH_FILE-}" ]; then
-      source "$NIX_ATTRS_SH_FILE"
-    fi
-    source $stdenv/setup
     export > saved-env
 
     PATH=${coreutils}/bin
@@ -271,8 +267,6 @@ rec {
     ${qemuCommand}
     EOF
 
-    mkdir -p -m 0700 $out
-
     chmod +x ./run-vm
     source ./run-vm

Removing the mkdir line is, what I was not sure about. Not sure about why it is needed in the first place, that is.

wolfgangwalther · 2024-11-16T17:20:20Z

The mkdir seems to have been added in aa5646f.

wolfgangwalther · 2024-11-16T17:34:35Z

The mkdir seems to have been added in aa5646f.

I tried to build runInLinuxVM (samba), but it doesn't even build for me.

Having a fix for a very specific package in that place without a comment explaining why... seems entirely wrong. I think we should just remove the mkdir.

Closes NixOS#334705 Addresses NixOS#205690 The main issue was that the output variable (i.e. `$out` and friends) didn't exist. I figured the easiest way to add those is to source `stdenv` here. Given that we build another derivation in this builder, it's pretty likely that `stdenv` gets pulled already, so I don't expect a real overhead here. Also, this mounts `/build` into the VM: this is required to make sure `.attrs.json` & `.attrs.sh` are available. Dropped the mount of `xchg` into `/tmp` now since it's also part of `/build`.

wolfgangwalther

I can't say anything about the temporary directory -> build directory part - I don't know enough about this code to understand the implications of that beyond the fact that it seems to fix things here ;)

Everything else LGTM. I didn't build again, just read through it.

ShamrockLee · 2024-11-24T09:52:59Z

appimage.tests.image-hello-cowsay breaks after this commit, as $out is no longer occupied by runInLinuxVM during preVM, and the rm "$out" line fails.

It's a good change since $out is no longer arbitrarily occupied. Still, it needs a release note entry because of its backward incompatibility.

I should have tested apptainer.tests.image-hello-cowsay before approving this PR. That's my fault.

ShamrockLee · 2024-11-24T10:14:02Z

In addition, the disk creation functionality with preVM seems to break when __structuredAttrs = true.

wolfgangwalther · 2024-11-24T10:14:38Z

appimage.tests.image-hello-cowsay breaks after this commit, as $out is no longer occupied by runInLinuxVM during preVM, and the rm "$out" line fails.

I am seeing this failure when building apptainer.tests.image-hello-cowsay:

mounting Nix store...
mounting host's build directory...
starting stage 2 (/nix/store/wmp357hsbipw1hv71q39l24aj50llvnj-vm-run-stage2)
rmdir: failed to remove '/nix/store/7nwhbfr8kvc6rvhbq0xzx2xnz4d9przd-apptainer-image-hello-cowsay.sif': No such file or directory

This doesn't look like "$out is empty".

ShamrockLee · 2024-11-24T10:16:34Z

This doesn't look like "$out is empty".

It's not "$out is empty", but "$out does not exist". (Here, "$out" is /nix/store/7nwhbfr8kvc6rvhbq0xzx2xnz4d9przd-apptainer-image-hello-cowsay.sif)

wolfgangwalther · 2024-11-24T10:17:45Z

Ah ok, misunderstood. So it's this removed line that broke it?

mkdir -p -m 0700 $out

ShamrockLee · 2024-11-24T10:19:34Z

Ah ok, misunderstood. So it's this removed line that broke it?
mkdir -p -m 0700 $out

I think so.

It's a good change, as most people would rm $out when using runInLinuxVM, but we must address the backward incompatibility.

wolfgangwalther · 2024-11-24T10:28:40Z

and the rm "$out" line fails.

Grepping for rm "$out" didn't lead me anywhere until I realized that I needed to be looking for rmdir "$out" instead.

It's a good change, as most people would rm $out when using runInLinuxVM, but we must address the backward incompatibility.

I can only find two uses of runInLinuxVM in nixpkgs. Your's is easily fixed by removing the rmdir. Still have to understand how it's used for docker.

Is it used more often outside of nixpkgs?

ShamrockLee · 2024-11-24T10:32:20Z

Is it used more often outside of nixpkgs?

A search across GitHub could inform us about that.

Do you know how to exclude forked repositories from the GitHub code search result?

wolfgangwalther · 2024-11-24T10:34:09Z

In addition, the disk creation functionality with preVM seems to break when __structuredAttrs = true.

I think that's because of this:

  createEmptyImage = {
    # Disk image size in MiB
    size,
    # Name that will be written to ${destination}/nix-support/full-name
    fullName,
    # Where to write the image files, defaulting to $out
    destination ? "$out"
  }: ''
    mkdir -p ${destination}
    diskImage=${destination}/disk-image.qcow2
    ${qemu}/bin/qemu-img create -f qcow2 $diskImage "${toString size}M"

    mkdir ${destination}/nix-support
    echo "${fullName}" > ${destination}/nix-support/full-name
  '';

This needs $out to be defined.

With my proposal in #354535 (comment) we removed the mkdir line, but also the loading of attrs at the beginning of vmRunCommand, assuming that $out was not needed anymore. We failed to see that eval "$preVM" could do anything, including depending on $out.

So I guess we should add those lines back:

    vmRunCommand = qemuCommand: writeText "vm-run" ''
+    if [ -f "''${NIX_ATTRS_SH_FILE-}" ]; then
+      source "$NIX_ATTRS_SH_FILE"
+    fi
+    source $stdenv/setup

WDYT?

wolfgangwalther · 2024-11-24T10:44:14Z

Is it used more often outside of nixpkgs?

A search across GitHub could inform us about that.

Do you know how to exclude forked repositories from the GitHub code search result?

I don't know how to exclude forks, but I browsed through the results and randomly picked uses of runInLinuxVM. From what I can see nobody assumes that $out will be created for them - many have mkdir -p $out in their preVM script. Of course, in the preVM script $out was not created previously, yet. So anyone doing magic there needs to create it anyway.

I didn't see anyone removing the existing $out.

wolfgangwalther · 2024-11-24T10:57:19Z

Still have to understand how it's used for docker.

It's used in runWithOverlay, which does a preVM = vmTools.createEmptyImage. This will create $out as a directory anyway. So this should continue to work as is.

I think the only broken case is indeed apptainer.tests.image-hello-cowsay.

ShamrockLee · 2024-11-24T11:31:30Z

I don't know how to exclude forks, but I browsed through the results and randomly picked uses of runInLinuxVM. From what I can see nobody assumes that $out will be created for them - many have mkdir -p $out in their preVM script. Of course, in the preVM script $out was not created previously, yet. So anyone doing magic there needs to create it anyway.

I didn't see anyone removing the existing $out.

I see. Thank you for checking it out.

ShamrockLee · 2024-11-24T11:48:49Z

In addition, the disk creation functionality with preVM seems to break when __structuredAttrs = true.

I think that's because of this:
  createEmptyImage = {
    # Disk image size in MiB
    size,
    # Name that will be written to ${destination}/nix-support/full-name
    fullName,
    # Where to write the image files, defaulting to $out
    destination ? "$out"
  }: ''
    mkdir -p ${destination}
    diskImage=${destination}/disk-image.qcow2
    ${qemu}/bin/qemu-img create -f qcow2 $diskImage "${toString size}M"

    mkdir ${destination}/nix-support
    echo "${fullName}" > ${destination}/nix-support/full-name
  '';
This needs $out to be defined.

With my proposal in #354535 (comment) we removed the mkdir line, but also the loading of attrs at the beginning of vmRunCommand, assuming that $out was not needed anymore. We failed to see that eval "$preVM" could do anything, including depending on $out.

So I guess we should add those lines back:
    vmRunCommand = qemuCommand: writeText "vm-run" ''
+    if [ -f "''${NIX_ATTRS_SH_FILE-}" ]; then
+      source "$NIX_ATTRS_SH_FILE"
+    fi
+    source $stdenv/setup
WDYT?

If it is needed to make preVM work, adding them back is sensible.

ElvishJerricco · 2024-11-26T07:13:42Z

This broke systemd-boot tests, and probably all tests built with disk images.

wolfgangwalther · 2024-11-26T08:04:45Z

This broke systemd-boot tests, and probably all tests built with disk images.

Just to double-check, this is with #358705 included, yes?

Can you give me a quick reproducer line what I need to build to see the failing test?

wolfgangwalther · 2024-11-26T08:11:50Z

nixosTests.systemd-boot.basic fails for me on current master.

wolfgangwalther · 2024-11-26T08:21:31Z

It does build fine right before this PR.

Relevant log when it succeeds:

[...]
EFI stub: Loaded initrd from LINUX_EFI_INITRD_MEDIA_GUID device path
loading kernel modules...
mounting Nix store...
mounting host's temporary directory...
starting stage 2 (/nix/store/iv0cw96ybnfncihnamdzhjlni6zr72xy-vm-run-stage2)
hwclock: select() to /dev/rtc0 to wait for clock tick timed out
tune2fs 1.47.1 (20-May-2024)
Setting maximal mount count to -1
Setting interval between checks to 0 seconds
Setting time filesystem last checked to Tue Nov 26 08:13:14 2024
[...]

The same part when it fails:

[...]
EFI stub: Loaded initrd from LINUX_EFI_INITRD_MEDIA_GUID device path
loading kernel modules...
mounting Nix store...
mounting host's build directory...
starting stage 2 (/nix/store/wmp357hsbipw1hv71q39l24aj50llvnj-vm-run-stage2)
hwclock: select() to /dev/rtc0 to wait for clock tick timed out
[   11.152361] Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000100
[...]

This is happening in stage 2.

When I revert the set -euo pipefail we have added there, then it passes again.

In fact when I only remove the set -e here, then it passes again.

This means that the current tests throw an error somewhere - but silently so. Imho, this PR just revealed a different problem. I am investigating further.

wolfgangwalther · 2024-11-26T08:33:44Z

Ah, it's in the logs already:

hwclock: select() to /dev/rtc0 to wait for clock tick timed out

So, this means that this command is actually returning an error:

    ${util-linux}/bin/hwclock -s

wolfgangwalther · 2024-11-26T08:39:31Z

It would be great to understand when the hwclock command started to fail.

Ma27 · 2024-11-26T11:05:51Z

Thanks for tracking it down, was busy investigating a production issue at work.
Can try to find out what went wrong with the hwclock invocation this ~afternoon.

I think this might be a channel-blocker, so I guess if I don't get to a solution quickly, I'd temporarily remove the set -euo pipefail.

wolfgangwalther · 2024-11-26T11:16:04Z

I think this might be a channel-blocker, so I guess if I don't get to a solution quickly, I'd temporarily remove the set -euo pipefail.

Alternatively, just swallow the error for hwclock manually with || true temporarily.

ElvishJerricco · 2024-11-26T12:42:37Z

I'm not 100% that hwclock command is even needed anymore. The commit that added it is over 10 years old and doesn't provide an explanation for why it's there other than a bug in an ancient version of qemu.

wolfgangwalther · 2024-11-26T13:22:14Z

I'm not 100% that hwclock command is even needed anymore. The commit that added it is over 10 years old and doesn't provide an explanation for why it's there other than a bug in an ancient version of qemu.

+1 for just removing those hacks.

Commit 97ed6b4 broke the systemd-boot tests (among others) because of the `hwclock -s` invocation. This was broken for a while, but not noticed because we didn't have a `set -e` before. The error hwclock: select() to /dev/rtc0 to wait for clock tick timed out MAY be related to an open QEMU bug[1]: I can't reproduce the error on aarch64-linux and x86_64-linux with `partitionTableType = "legacy";`. Also, the issue disappears on x86_64-linux when adding `--directisa`. However, the invocation was added in f73ff05 10 years ago which didn't give any reasoning or pointer to what KVM bug this may be. Given that this must have happened on an ancient version, we agreed on removing it altogether[2]. [1] https://gitlab.com/qemu-project/qemu/-/issues/1762 [2] NixOS#354535 (comment)

iFreilicht · 2024-11-29T19:45:41Z

pkgs/build-support/vm/default.nix

@@ -223,11 +229,12 @@ rec {
      -nographic -no-reboot \
      -device virtio-rng-pci \
      -virtfs local,path=${storeDir},security_model=none,mount_tag=store \
+      -virtfs local,path=/build,security_model=none,mount_tag=sa \


Hmm, this isn't backwards compatible and creates quite a few annoyances. Right now, it breaks disko, see nix-community/disko#900

Formatting 'main.raw', fmt=raw size=10737418240 qemu-kvm: -virtfs local,path=/build,security_model=none,mount_tag=sa: cannot initialize fsdev 'sa': failed to open '/build': No such file or directory

The other paths in this invocation are configurable, but /build requires me to do a switch_root before. I would like to allow adding a custom prefix in front of this path. I'll submit a PR for this.

I'm currently looking into this.

Thank you! I don't have the bandwidth for it this weekend, can take a look at this on ~Monday if needed (please ping me here if that's the case).

Fixes #900 This was caused by NixOS/nixpkgs#354535 originally. The breaking changes introduced there have been resolved by NixOS/nixpkgs#360413, but one addition survived, which was the line `source $stdenv/setup`. Because we used `>` instead of `>>`, `saved-env` was overwritten, so even with the second PR, the script failed with the following error: /nix/store/pw...ykc-vm-run-stage2: line 16: stdenv: unbound variable Once this and the second PR mentioned above are merged, #903 will be unblocked.

Ma27 requested review from philiptaron, wolfgangwalther and Artturin November 8, 2024 16:38

ShamrockLee reviewed Nov 8, 2024

View reviewed changes

pkgs/build-support/vm/default.nix Outdated Show resolved Hide resolved

pkgs/build-support/vm/default.nix Outdated Show resolved Hide resolved

Ma27 force-pushed the runInLinuxVM-structured-attrs branch from d633c90 to 487dd26 Compare November 8, 2024 18:03

ShamrockLee approved these changes Nov 8, 2024

View reviewed changes

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Nov 8, 2024

wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 8, 2024

wolfgangwalther reviewed Nov 16, 2024

View reviewed changes

pkgs/build-support/vm/default.nix Outdated Show resolved Hide resolved

pkgs/build-support/vm/default.nix Show resolved Hide resolved

pkgs/build-support/vm/default.nix Outdated Show resolved Hide resolved

wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 17, 2024

Ma27 force-pushed the runInLinuxVM-structured-attrs branch from 487dd26 to 97ed6b4 Compare November 22, 2024 19:03

Ma27 requested a review from wolfgangwalther November 22, 2024 19:06

wolfgangwalther approved these changes Nov 22, 2024

View reviewed changes

wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 23, 2024

Ma27 merged commit 6deea16 into NixOS:master Nov 23, 2024
17 of 18 checks passed

Ma27 deleted the runInLinuxVM-structured-attrs branch November 23, 2024 08:35

Ma27 mentioned this pull request Nov 24, 2024

runInLinuxVM: fix createEmptyImage/preVM case #358705

Merged

13 tasks

Ma27 mentioned this pull request Nov 26, 2024

runInLinuxVM: remove hwclock -s invocation #359309

Merged

13 tasks

iFreilicht reviewed Nov 29, 2024

View reviewed changes

iFreilicht mentioned this pull request Nov 29, 2024

Example in docs/disko-images.md generates broken result script nix-community/disko#900

Closed

wolfgangwalther mentioned this pull request Nov 30, 2024

runInLinuxVM: refactor structuredAttrs support, fix disko #360413

Merged

13 tasks

iFreilicht mentioned this pull request Nov 30, 2024

disk-image: Fix compatibility with nixpkgs unstable nix-community/disko#907

Merged

wolfgangwalther mentioned this pull request Dec 2, 2024

[Tracking Issue] __structuredAttrs TODOs and FIXMEs #205690

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

runInLinuxVM: fix for structured attrs #354535

runInLinuxVM: fix for structured attrs #354535

Ma27 commented Nov 8, 2024

ShamrockLee left a comment

Ma27 commented Nov 8, 2024

ShamrockLee left a comment •

edited

Loading

wolfgangwalther commented Nov 16, 2024

wolfgangwalther commented Nov 16, 2024

wolfgangwalther commented Nov 16, 2024

wolfgangwalther left a comment

ShamrockLee commented Nov 24, 2024

ShamrockLee commented Nov 24, 2024

wolfgangwalther commented Nov 24, 2024

ShamrockLee commented Nov 24, 2024 •

edited

Loading

wolfgangwalther commented Nov 24, 2024

ShamrockLee commented Nov 24, 2024 •

edited

Loading

wolfgangwalther commented Nov 24, 2024

ShamrockLee commented Nov 24, 2024 •

edited

Loading

wolfgangwalther commented Nov 24, 2024

wolfgangwalther commented Nov 24, 2024

wolfgangwalther commented Nov 24, 2024

ShamrockLee commented Nov 24, 2024

ShamrockLee commented Nov 24, 2024

ElvishJerricco commented Nov 26, 2024

wolfgangwalther commented Nov 26, 2024

wolfgangwalther commented Nov 26, 2024

wolfgangwalther commented Nov 26, 2024

wolfgangwalther commented Nov 26, 2024

wolfgangwalther commented Nov 26, 2024

Ma27 commented Nov 26, 2024

wolfgangwalther commented Nov 26, 2024

ElvishJerricco commented Nov 26, 2024

wolfgangwalther commented Nov 26, 2024

iFreilicht Nov 29, 2024

wolfgangwalther Nov 30, 2024

Ma27 Nov 30, 2024

runInLinuxVM: fix for structured attrs #354535

runInLinuxVM: fix for structured attrs #354535

Conversation

Ma27 commented Nov 8, 2024

Things done

ShamrockLee left a comment

Choose a reason for hiding this comment

Ma27 commented Nov 8, 2024

ShamrockLee left a comment • edited Loading

Choose a reason for hiding this comment

wolfgangwalther commented Nov 16, 2024

wolfgangwalther commented Nov 16, 2024

wolfgangwalther commented Nov 16, 2024

wolfgangwalther left a comment

Choose a reason for hiding this comment

ShamrockLee commented Nov 24, 2024

ShamrockLee commented Nov 24, 2024

wolfgangwalther commented Nov 24, 2024

ShamrockLee commented Nov 24, 2024 • edited Loading

wolfgangwalther commented Nov 24, 2024

ShamrockLee commented Nov 24, 2024 • edited Loading

wolfgangwalther commented Nov 24, 2024

ShamrockLee commented Nov 24, 2024 • edited Loading

wolfgangwalther commented Nov 24, 2024

wolfgangwalther commented Nov 24, 2024

wolfgangwalther commented Nov 24, 2024

ShamrockLee commented Nov 24, 2024

ShamrockLee commented Nov 24, 2024

ElvishJerricco commented Nov 26, 2024

wolfgangwalther commented Nov 26, 2024

wolfgangwalther commented Nov 26, 2024

wolfgangwalther commented Nov 26, 2024

wolfgangwalther commented Nov 26, 2024

wolfgangwalther commented Nov 26, 2024

Ma27 commented Nov 26, 2024

wolfgangwalther commented Nov 26, 2024

ElvishJerricco commented Nov 26, 2024

wolfgangwalther commented Nov 26, 2024

iFreilicht Nov 29, 2024

Choose a reason for hiding this comment

wolfgangwalther Nov 30, 2024

Choose a reason for hiding this comment

Ma27 Nov 30, 2024

Choose a reason for hiding this comment

ShamrockLee left a comment •

edited

Loading

ShamrockLee commented Nov 24, 2024 •

edited

Loading

ShamrockLee commented Nov 24, 2024 •

edited

Loading

ShamrockLee commented Nov 24, 2024 •

edited

Loading