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

nixos/tests: follow-up to the closure reduction PR #101598

Merged
merged 5 commits into from Oct 26, 2020

Conversation

@andir
Copy link
Member

@andir andir commented Oct 24, 2020

Motivation for this change

This is a follow-up on #49403 where I made a big oversight regarding the QEMU executable that is being described within the NixOS module system. That package actually drives the VMs that are being started. I previously only changed the QEMU package that was used within the test driver script.

I've tried to detail the changes in the commits. Please let me know if there are any open questions and/or remarks.

While working on this I noticed that in 5500dc8 we introduced the --keep-vm-state flag and defaulted to that flag not being set. This lead to the runInMachine tests not longer
working and that going unnoticed for quite some time now.

cc @Ma27 @rnhmjoj #101473

@rnhmjoj
Copy link
Contributor

@rnhmjoj rnhmjoj commented Oct 24, 2020

Can you change nixos-build-vms to use the driverInterative? It should work, now.

@andir andir force-pushed the nixos-build-vms-qemu branch from 1a4350f to adc0964 Oct 24, 2020
@andir
Copy link
Member Author

@andir andir commented Oct 24, 2020

@rnhmjoj that is actually already the case. Due to the changes that @Ma27 merges the nixos-build-vms output defaults to the fullfeatured QEMU even thought the driver Attribute is used.

@xaverdh
Copy link
Contributor

@xaverdh xaverdh commented Oct 25, 2020

so does / should this resolve the eval error currently present on unstable-small ?

@rnhmjoj
Copy link
Contributor

@rnhmjoj rnhmjoj commented Oct 25, 2020

@andir yes, but do you think that driverInteractive is more explicit that virtualisation.qemu.package = pkgs.qemu;? From this line alone it's not clear what's going on.

@andir
Copy link
Member Author

@andir andir commented Oct 25, 2020

@xaverdh
Copy link
Contributor

@xaverdh xaverdh commented Oct 25, 2020

error: "error: --- ThrownError --- hydra-eval-jobs\nThe option virtualisation.qemu' does not exist. Definition values:\n- In /nix/store/zp8k91zbq2rpj5ky8r629zw2z85s3rw4-source/nixos/modules/testing/test-instrumentation.nix':\n {\n consoles = [ ];\n package = {\n _type = "override";\n content = <derivation /nix/store/q72h2cdcb9zjgiay5gdgzwddjkbjr7xq-qemu-host-cpu-only-for-vm-tests-5.1.0.drv>;\n ..."

(from https://hydra.nixos.org/jobset/nixos/unstable-small#tabs-errors)

@xaverdh
Copy link
Contributor

@xaverdh xaverdh commented Oct 25, 2020

Not sure if that is indeed what is preventing eval though. There is also the mysterious

error: [json.exception.type_error.302] type must be string, but is null

at the tail of that log..

@andir
Copy link
Member Author

@andir andir commented Oct 25, 2020

I belive the fromer could be fixed (will verify) the latter is (hopefully) unrelated as master has been stuck for a while now.

Funnily enough my hydra instance was able to eval the release jobset of this PR just fine. I'll also let it build the small release set to see if that also succeeds.

@andir
Copy link
Member Author

@andir andir commented Oct 25, 2020

@andir yes, but do you think that driverInteractive is more explicit that virtualisation.qemu.package = pkgs.qemu;? From this line alone it's not clear what's going on.

You might have a good point there. So would it be clearer if we switched the nixos-build-vms expression to driverInteractive and just remove the explicit override in the nix-build-vms module?

@andir
Copy link
Member Author

@andir andir commented Oct 25, 2020

Oh yeah indeed. The small jobset is failing with the same eval error here:

hydra-eval-jobs returned exit code 1:
error: "error: --- ThrownError --- hydra-eval-jobs\nThe option `virtualisation.qemu' does not exist. Definition values:\n- In `/nix/store/c9mv62dd7r442yiigamw4q1xwx73pfi0-source/nixos/modules/testing/test-instrumentation.nix':\n    {\n      consoles = [ ];\n      package = {\n        _type = \"override\";\n        content = <derivation /nix/store/q72h2cdcb9zjgiay5gdgzwddjkbjr7xq-qemu-host-cpu-only-for-vm-tests-5.1.0.drv>;\n    ..."
error: [json.exception.type_error.302] type must be string, but is null

I managed to reproduce it locally with this command: nix-instantiate nixos/release-small.nix -A nixos.tests.boot.biosCdrom.x86_64-linux.

I'll have a look why this test is complaining while others are just fine.

@xaverdh
Copy link
Contributor

@xaverdh xaverdh commented Oct 25, 2020

I think it is due to #101246 actually..

@andir
Copy link
Member Author

@andir andir commented Oct 25, 2020

I think it is due to #101246 actually..

Yeah, I just started fixing that. The problem is because the virtualisation.qemu options do not always exist and thus we must (continue) to check if they exist before we set them.

@andir
Copy link
Member Author

@andir andir commented Oct 25, 2020

@xaverdh I opened a PR for that fix as it can go in on it's own: #101645

@andir
Copy link
Member Author

@andir andir commented Oct 25, 2020

Once that lands I'll rebase this PR ontop of it and give it another spin.

Ma27
Ma27 approved these changes Oct 25, 2020
Copy link
Member

@Ma27 Ma27 left a comment

LGTM.
As @rnhmjoj stated, nixos-build-vms should use driverInteractive now rather than my workaround btw.

andir added 5 commits Oct 25, 2020
Previously you would be able to override only the QEMU package to be
used in the test runner. Frankly that doesn't help a lot if you are
trying to get a graphical session. The graphical session requires the
option in the NixOS module system to bet set to the correct QEMU
package.

In this commit I moved most of the test node configuration and
transformations into the `mkDriver` function (previously called
`driver`). The motivation was to be able to create a `driver` instance
with a given QEMU package that will be used consistently througout the
test expression.
In a previous commit I broke this as there is no longer one testDriver
but only a function to generate one based on some QEMU inputs.
In 5500dc8 we introduced the --keep-vm-state flag and defaulted to that
flag not being set. This lead to the `runInMachine` tests not longer
working and that going unnoticed for quite some time now.
This reverts commit aab534b & uses the
driverInteractive attribute for the test driver instead.

This has the same effect but removes the extra module in the
nixos-build-vms code.
@andir andir force-pushed the nixos-build-vms-qemu branch from adc0964 to d4fb7da Oct 25, 2020
Copy link
Contributor

@rnhmjoj rnhmjoj left a comment

Looks good to me. Thank you for change I mentioned.

@andir andir merged commit 1088f05 into NixOS:master Oct 26, 2020
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants