Skip to content
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_test: disable features that are not needed for tests (closure 641 -> 335.3M) #49403

Merged
merged 9 commits into from Oct 20, 2020

Conversation

@andir
Copy link
Member

@andir andir commented Oct 29, 2018

Motivation for this change

This is an attempt to change the NixOS test runner in a way that allows faster iteration on lower-level parts of the system by reducing the number of packages that contribute to the test closure size.

While qemu_test has always been a bit slimmer then one of its siblings it wasn't really "slim" in comparison. With this PR we reduce the closure size of qemu_test to about 335MB from previously > 640MB.

One of the changes this introduces (based on the discussion when this PR was first opened) is that test drivers will now come without any graphical support by default. There is a new attribute to the tests (driverInteractive) that comes with the full feature set of QEMU including the graphical output and sound.

Also, while running VM tests, we will use the QEMU Guest Agent from the corresponding QEMU package as otherwise we'd not have gained much.

I removed systemd from iputils as that was only used to toggle the systemd unit generation (that we aren't using on systemd anyway). There are no references to systemd in the runtime closure. The required patch has been submitted upstream and is awaiting feedback.

Things done
  • I executed a few tests nothing more fancy. What are good candidates to verify it doesn't break more fancy tests?
    • tests.installer.simple.x86_64-linux
    • tests.installer.zfsroot.x86_64-linux
    • tests.vault.x86_64-linux
    • tests.mpd.x86_64-linux
    • tests.containers-ipv4.x86_64-linux
@GrahamcOfBorg
Copy link

@GrahamcOfBorg GrahamcOfBorg commented Oct 29, 2018

Success on x86_64-darwin (full log)

Attempted: qemu_test

Partial log (click to expand)

find: '/nix/store/zri6x2b20rjg6avdddsa8k8vrwsna802-qemu-host-cpu-only-for-vm-tests-3.0.0-ga': No such file or directory
find: '/nix/store/zri6x2b20rjg6avdddsa8k8vrwsna802-qemu-host-cpu-only-for-vm-tests-3.0.0-ga': No such file or directory
find: '/nix/store/zri6x2b20rjg6avdddsa8k8vrwsna802-qemu-host-cpu-only-for-vm-tests-3.0.0-ga': No such file or directory
find: '/nix/store/zri6x2b20rjg6avdddsa8k8vrwsna802-qemu-host-cpu-only-for-vm-tests-3.0.0-ga': No such file or directory
find: '/nix/store/zri6x2b20rjg6avdddsa8k8vrwsna802-qemu-host-cpu-only-for-vm-tests-3.0.0-ga': No such file or directory
find: '/nix/store/zri6x2b20rjg6avdddsa8k8vrwsna802-qemu-host-cpu-only-for-vm-tests-3.0.0-ga': No such file or directory
find: '/nix/store/zri6x2b20rjg6avdddsa8k8vrwsna802-qemu-host-cpu-only-for-vm-tests-3.0.0-ga': No such file or directory
find: '/nix/store/zri6x2b20rjg6avdddsa8k8vrwsna802-qemu-host-cpu-only-for-vm-tests-3.0.0-ga': No such file or directory
find: '/nix/store/zri6x2b20rjg6avdddsa8k8vrwsna802-qemu-host-cpu-only-for-vm-tests-3.0.0-ga': No such file or directory
/nix/store/f53dznvx2k7sy9r4a2q83yibhsdy49ar-qemu-host-cpu-only-for-vm-tests-3.0.0

@GrahamcOfBorg
Copy link

@GrahamcOfBorg GrahamcOfBorg commented Oct 29, 2018

Success on x86_64-linux (full log)

Attempted: qemu_test

Partial log (click to expand)

patching script interpreter paths in /nix/store/ibz4sprmrmvs6r2plwgalsdnslapymvn-qemu-host-cpu-only-for-vm-tests-3.0.0
checking for references to /build in /nix/store/ibz4sprmrmvs6r2plwgalsdnslapymvn-qemu-host-cpu-only-for-vm-tests-3.0.0...
cannot find section .dynamic
cannot find section .dynamic
cannot find section .dynamic
cannot find section .dynamic
cannot find section .dynamic
cannot find section .dynamic
strip is /nix/store/vcc4svb8gy29g4pam2zja6llkbcwsyiq-binutils-2.30/bin/strip
/nix/store/ibz4sprmrmvs6r2plwgalsdnslapymvn-qemu-host-cpu-only-for-vm-tests-3.0.0

@xeji
Copy link
Contributor

@xeji xeji commented Oct 29, 2018

This removes the possibility to log in to testing machines interactively nix-build some-test.nix -A driver and then running result/bin/nixos-test-driver or result/bin/nixos-run-vms, which I think is a crucial feature for developing and debugging nixos tests.

It also breaks nixos-build-vms which uses qemu_test by default because the resulting VMs are headless and non-interactive.

@GrahamcOfBorg
Copy link

@GrahamcOfBorg GrahamcOfBorg commented Oct 29, 2018

Success on aarch64-linux (full log)

Attempted: qemu_test

Partial log (click to expand)

patching script interpreter paths in /nix/store/bj6xlpiqcsf1ds5225fczhxyrriyqi47-qemu-host-cpu-only-for-vm-tests-3.0.0
checking for references to /build in /nix/store/bj6xlpiqcsf1ds5225fczhxyrriyqi47-qemu-host-cpu-only-for-vm-tests-3.0.0...
cannot find section .dynamic
cannot find section .dynamic
cannot find section .dynamic
cannot find section .dynamic
cannot find section .dynamic
cannot find section .dynamic
strip is /nix/store/p9akxn2sfy4wkhqdqa3li97pc6jaz3r1-binutils-2.30/bin/strip
/nix/store/bj6xlpiqcsf1ds5225fczhxyrriyqi47-qemu-host-cpu-only-for-vm-tests-3.0.0

@andir
Copy link
Member Author

@andir andir commented Oct 29, 2018

@xeji true, I haven't thought about them. Do we have some intermediate path that we can go without pulling in all those dependencies for all the tests? When iterating on things like libc, systemd, … it is a bit annoying to rebuild gtk, pulseaudio, and a few other packages.

@xeji
Copy link
Contributor

@xeji xeji commented Oct 29, 2018

Do we have some intermediate path that we can go without pulling in all those dependencies for all the tests?

I'm not sure there is. Maybe it's better to have two variants qemu_test_minimal (the minimum needed to run the automated tests on Hydra) and qemu_test (for nixos-build-vms and developing/building nixos tests. But that would require some refactoring.

@andir
Copy link
Member Author

@andir andir commented Oct 29, 2018

The difference is that there will not be a g GTK window popping up. You can still connect to localhost via a vnclient to get the same output.

@andir
Copy link
Member Author

@andir andir commented Oct 29, 2018

After a bit of discussion with @xeji on IRC we think this might be fine for working on nixpkgs but we should implement something better for nixos-build-vms since in those cases the users most likely wants to see the VMs

@andir andir changed the title qemu_test: disable features that are not needed for tests (closure 567MB -> 174MB) WIP: qemu_test: disable features that are not needed for tests (closure 567MB -> 174MB) Oct 29, 2018
@andir
Copy link
Member Author

@andir andir commented Dec 20, 2018

Just a quick hack on a potential solution for the mentioned nixos-rebuild-vms issue (not tested):

diff --git a/nixos/lib/testing.nix b/nixos/lib/testing.nix
index 42a0c60c7e1..758b45699e5 100644
--- a/nixos/lib/testing.nix
+++ b/nixos/lib/testing.nix
@@ -1,4 +1,4 @@
-{ system, minimal ? false, config ? {} }:
+{ system, minimal ? false, config ? {}, useQemuTest ? true }:

 with import ./build-vms.nix { inherit system minimal config; };
 with pkgs;
@@ -22,6 +22,9 @@ in rec {
     preferLocalBuild = true;

     installPhase =
+      let
+        qemu = if useQemuTest then pkgs.qemu_test else pkgs.qemu;
+      in
       ''
         mkdir -p $out/bin
         cp ${./test-driver/test-driver.pl} $out/bin/nixos-test-driver
@@ -33,7 +36,7 @@ in rec {
         cp ${./test-driver/Logger.pm} $libDir/Logger.pm

         wrapProgram $out/bin/nixos-test-driver \
-          --prefix PATH : "${lib.makeBinPath [ qemu_test vde2 netpbm coreutils ]}" \
+          --prefix PATH : "${lib.makeBinPath [ qemu vde2 netpbm coreutils ]}" \
           --prefix PERL5LIB : "${with perlPackages; lib.makePerlPath [ TermReadLineGnu XMLWriter IOTty FileSlurp ]}:$out/lib/perl5/site_perl"
       '';
   };

diff --git a/nixos/modules/installer/tools/nixos-build-vms/build-vms.nix b/nixos/modules/installer/tools/nixos-build-vms/build-vms.nix
index 4372d196261..c8547844d3d 100644
--- a/nixos/modules/installer/tools/nixos-build-vms/build-vms.nix
+++ b/nixos/modules/installer/tools/nixos-build-vms/build-vms.nix
@@ -6,4 +6,4 @@ let nodes = import networkExpr; in

 with import ../../../../lib/testing.nix { inherit system; };

-(makeTest { inherit nodes; testScript = ""; }).driver
+(makeTest { inherit nodes; testScript = ""; useQemuTest = false; }).driver

@stale
Copy link

@stale stale bot commented Jun 1, 2020

Thank you for your contributions.
This has been automatically marked as stale because it has had no activity for 180 days.
If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.
Here are suggestions that might help resolve this more quickly:

  1. Search for maintainers and people that previously touched the
    related code and @ mention them in a comment.
  2. Ask on the NixOS Discourse. 3. Ask on the #nixos channel on
    irc.freenode.net.

@stale stale bot added the 2.status: stale label Jun 1, 2020
@flokli
Copy link
Contributor

@flokli flokli commented Jun 1, 2020

I'd really like to see this. Not having to rebuild all of GTK and its dependencies, to run a VM test after a low-level change (like systemd or glibc) is a huge timesaver.

@stale stale bot removed the 2.status: stale label Jun 1, 2020
@flokli flokli requested a review from tfc Jun 1, 2020
@tfc
Copy link
Contributor

@tfc tfc commented Jun 2, 2020

Oh, awesome. I see this PR for the first time right now. Why wasn't it merged earlier? Was the difficulty of testing if the change breaks any existing tests a/the problem?

@flokli
Copy link
Contributor

@flokli flokli commented Jun 2, 2020

Oh, awesome. I see this PR for the first time right now. Why wasn't it merged earlier? Was the difficulty of testing if the change breaks any existing tests a/the problem?

No, the problem was that people want to see the GTK window when interactively debugging (via result/bin/nixos-run-vms, and maybe also via the test driver), but there's currently no way to distinguish.

@andir andir force-pushed the qemu_test_reduce_closure branch from d213364 to e61cde7 Oct 19, 2020
@andir andir changed the title WIP: qemu_test: disable features that are not needed for tests (closure 567MB -> 174MB) qemu_test: disable features that are not needed for tests (closure 567MB -> 174MB) Oct 19, 2020
@andir
Copy link
Member Author

@andir andir commented Oct 19, 2020

I've updated this PR to my latest work in this area.

The current qemu_test size is now 340.9M 338.7M down from 641.M before this PR. I'll look into reducing the size of qemu closure further now.

andir added 2 commits Oct 19, 2020
This allows much faster VM-test based systemd testing as the closure of
qemu suddenly shrinks to reasonable sizes again.
Since we previously stripped down the features of `qemu_test` some of
the features users are used to while running tests through the (impure)
driver didn't work anymore. Most notably we lost support for graphical
output and audio. With this change the `driver` attribute uses are more
feature complete version of QEmu compared to the one used in the pure
Nix builds.

This gives us the best of both worlds. Users are able to see the
graphical windows of VMs while CI and regular nix builds do not have to
download all the (unnecessary) dependencies.
andir added 3 commits Oct 19, 2020
In our case systemd is only used to figure out if the unit files should
be generated.
This allows us to get rid of the hack and the systemd dependency and
thus reduces the rebuild closure whenever systemd changes.
While (currently) it is the same package it carries more information if
we explicitly state that we want udev.
@andir andir changed the title qemu_test: disable features that are not needed for tests (closure 567MB -> 174MB) qemu_test: disable features that are not needed for tests (closure 641 -> 335.3M) Oct 19, 2020
@andir
Copy link
Member Author

@andir andir commented Oct 19, 2020

I updated the initial description:

Motivation for this change

This is an attempt to change the NixOS test runner in a way that allows faster iteration on lower-level parts of the system by reducing the number of packages that contribute to the test closure size.

While qemu_test has always been a bit slimmer then one of its siblings it wasn't really "slim" in comparison. With this PR we reduce the closure size of qemu_test to about 335MB from previously > 640MB.

One of the changes this introduces (based on the discussion when this PR was first opened) is that test drivers will now come without any graphical support by default. There is a new attribute to the tests (driverInteractive) that comes with the full feature set of QEMU including the graphical output and sound.

Also, while running VM tests, we will use the QEMU Guest Agent from the corresponding QEMU package as otherwise we'd not have gained much.

I removed systemd from iputils as that was only used to toggle the systemd unit generation (that we aren't using on systemd anyway). There are no references to systemd in the runtime closure. The required patch has been submitted upstream and is awaiting feedback.

Things done
  • I executed a few tests nothing more fancy. What are good candidates to verify it doesn't break more fancy tests?
    • tests.installer.simple.x86_64-linux
    • tests.installer.zfsroot.x86_64-linux
    • tests.vault.x86_64-linux
    • tests.mpd.x86_64-linux
    • tests.containers-ipv4.x86_64-linux

@flokli
Copy link
Contributor

@flokli flokli commented Oct 20, 2020

@andir I also did successfully run nixosTests.cage (which does screenshots and OCR), nixosTests.bittorrent (which does some more fancy networking stuff with multiple switches), and nixosTests.signal-desktop (GUI, but X).

IMHO, this should be good enough (tm) to merge. Do you want to squash 54e6cfc and 0a55c5d together?

flokli
flokli approved these changes Oct 20, 2020
@andir
Copy link
Member Author

@andir andir commented Oct 20, 2020

I would like to keep those two commits on iputils as is. It gives a bit more history on how this came to be. It might also be useful to revert the commit that applies the commit if upstream doesn't accept it (then we already have an alternative implementation).

I did rebuild all the release tests and only had a +/-1 deviation from the regular amount of test failures.

@andir andir merged commit f6cd172 into NixOS:master Oct 20, 2020
19 of 20 checks passed
19 of 20 checks passed
@github-actions[bot]
tests
Details
@github-actions[bot]
action
Details
@ofborg[bot]
iputils, iputils.passthru.tests, libndctl, libndctl.passthru.tests, qemu, qemu.passthru.tests on x86_64-darwin
Details
@ofborg[bot]
Evaluation Performance Report Evaluator Performance Report
Details
@github-actions[bot]
Wait for ofborg
Details
@ofborg[bot]
grahamcofborg-eval ^.^!
Details
@ofborg[bot]
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
@ofborg[bot]
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
@ofborg[bot]
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="16a7ff5"; rev="16a7ff5b867f6e9cb615eac7a136421a197921c0"; } ./pkgs/t
Details
@ofborg[bot]
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
@ofborg[bot]
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="16a7ff5"; rev="16a7ff5b867f6e9cb615eac7a136421a197921c0"; } ./nixos/
Details
@ofborg[bot]
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="16a7ff5"; rev="16a7ff5b867f6e9cb615eac7a136421a197921c0"; } ./nixos/
Details
@ofborg[bot]
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="16a7ff5"; rev="16a7ff5b867f6e9cb615eac7a136421a197921c0"; } ./nixos/
Details
@ofborg[bot]
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="16a7ff5"; rev="16a7ff5b867f6e9cb615eac7a136421a197921c0"; } ./pkgs/t
Details
@ofborg[bot]
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="16a7ff5"; rev="16a7ff5b867f6e9cb615eac7a136421a197921c0"; } ./pkgs/t
Details
@ofborg[bot]
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="16a7ff5"; rev="16a7ff5b867f6e9cb615eac7a136421a197921c0"; } ./pkgs/t
Details
@ofborg[bot]
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
@ofborg[bot]
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@ofborg[bot]
iputils, iputils.passthru.tests, libndctl, libndctl.passthru.tests, qemu, qemu.passthru.tests on aarch64-linux Success
Details
@ofborg[bot]
iputils, iputils.passthru.tests, libndctl, libndctl.passthru.tests, qemu, qemu.passthru.tests on x86_64-linux Success
Details
@rnhmjoj
Copy link
Contributor

@rnhmjoj rnhmjoj commented Oct 21, 2020

It seems this change has been applied to nixos-rebuild build-vm too. I hope it's not intended because I find invaluable to simply start the SDL screen without using a remote viewer.

rnhmjoj added a commit to rnhmjoj/nixpkgs that referenced this issue Oct 21, 2020
@andir
Copy link
Member Author

@andir andir commented Oct 21, 2020

rnhmjoj added a commit to rnhmjoj/nixpkgs that referenced this issue Oct 21, 2020
sdier added a commit to sdier/nixpkgs that referenced this issue Oct 23, 2020
@xfix
Copy link
Contributor

@xfix xfix commented Oct 25, 2020

@ofborg test installer.simple

marcus7070 added a commit to marcus7070/nixpkgs that referenced this issue Nov 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment