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

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

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

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

@c0bw3b c0bw3b added the 6.topic: testing Tooling for automated testing of packages and modules label Apr 28, 2019
@stale
Copy link

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 https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 1, 2020
@flokli
Copy link
Contributor

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 https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 1, 2020
@flokli flokli requested a review from tfc June 1, 2020 21:35
@tfc
Copy link
Contributor

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

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

@andir
Copy link
Member Author

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
@rnhmjoj
Copy link
Contributor

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.

@andir
Copy link
Member Author

andir commented Oct 21, 2020 via email

rnhmjoj added a commit to rnhmjoj/nixpkgs that referenced this pull request Oct 21, 2020
@KamilaBorowska
Copy link
Member

@ofborg test installer.simple

marcus7070 pushed a commit to marcus7070/nixpkgs that referenced this pull request Nov 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants