Fast nixos test eval by roberth · Pull Request #230523 · NixOS/nixpkgs

roberth · 2023-05-07T13:49:34Z

Description of changes

Speed up nixosTests evaluation by reusing nixpkgs.

Add a module that makes nixpkgs.* options read-only (except nixpkgs.pkgs which is set-once)
Add a module for nixpkgs sharing in all-tests.nix. Use the preceding module to make sure we don't test the wrong thing.
Some minor improvements; see commit list.

As for the performance improvement in acme: about 90 seconds down to 40 seconds

Heap stats

Vanilla: 5.2G final heap, 9.5G total allocations

  "gc": {
    "heapSize": 5235208192,
    "totalBytes": 9486783456
  }

Read only 2.3G final heap, 5.3G total allocations

  "gc": {
    "heapSize": 2299195392,
    "totalBytes": 5271224720
  }

Things done

Kranzes · 2023-05-07T14:52:37Z

nixos/tests/all-tests.nix

Is the plan to set this for all tests?

I think that would be best, but I can't speak for everyone.
Most tests aren't in runTest style yet, and I don't know if everyone wants to switch their test to it.
Doing it for handleTest is too complicated for me. Too many callbacks and alternate ways to invoke stuff.

We can default it for runTest for now, right?

Some modules will set something in nixpkgs.config, or add an overlay. Wouldn't recommend, but that means that we can't apply this everywhere. I don't mind if "leaky" things like this are repeated. It's good to be reminded of it.

I've added a node.pkgsReadOnly option to the test framework so that the optimization can be disabled if needed.
We could run it through the python-test-refactoring job on hydra.nixos.org to figure out which tests need an opt out. https://nixos.org/manual/nixos/unstable/index.html#sec-test-the-test-framework
Although that's not necessary for this PR.

tfc · 2023-05-08T06:17:15Z

Looks good to me. It would be great to have it run through all the tests to find out which one needs an opt-out to make the majority of them faster.

Although... doesn't have to happen in this PR. Maybe it would be even cleaner to do that in a follow-up PR.

A nominal type.

mkIf is unnecessary when the condition is statically known - that is knowable before entering the module evaluation. By changing this to a precomputed module, we support changing the defined options to readOnly options.

This speeds up evaluation by a factor 2. Ballpark figures from my machine: ``` $ time nix-build nixos/release.nix -A tests.acme /nix/store/q4fxp55k64clcarsx8xc8f6s10szlfvz-vm-test-run-acme /nix/store/lnfqg051sxx05hclva84bcbnjfc71c8x-vm-test-run-acme real 1m28.142s user 1m7.474s sys 0m7.932s $ time nix-build nixos/release.nix -A tests.acme /nix/store/q4fxp55k64clcarsx8xc8f6s10szlfvz-vm-test-run-acme /nix/store/lnfqg051sxx05hclva84bcbnjfc71c8x-vm-test-run-acme real 0m38.235s user 0m33.814s sys 0m2.283s ```

By factoring out this logic, it's easier for other projects to make use of it this optimization too (and do it correctly).

By adding this option indirection, a test can declare all by itself that it needs a custom nixpkgs. This is a more convenient way of going about this when the caller of the test framework receives a `node.pkgs` unconditionally.

It's just not necessary.

roberth · 2023-05-11T14:30:53Z

I've removed the change that disabled aliases on hydra, because a few tests were not compatible with that.

Other than that, I've added this to all-tests.nix's runTest and tested on hydra that it works.
For now it has a small effect though, because many tests still use the handleTest, as mentioned.

Most tests are not affected by this because they use the `handleTest` function instead.

Kranzes · 2023-05-11T14:48:28Z

btw, are there plans of converting everything to runTest automatically?

roberth · 2023-05-11T15:45:47Z

The tests are too messy to automate it. (maybe there are some patterns; not sure)
For one, some tests have a test matrix. Each of these is custom, so it's worth considering to standardize on something like #176557 or if that's too magical, come up with some convention at least.

Previously I haven't suggested migrating "other people's" tests because there's a little bit of friction involved, but I think now with the performance improvement, the case is strong enough to start migrating them.

If you or anyone feels like converting some of the non-matrix tests, feel free to convert them. To reduce the friction, add something like this.

EDIT: maybe not without making the case to the community in an RFC.

Kranzes · 2023-05-11T16:46:03Z

Does this optimization also works by default on out-of-tree tests that use nixos-lib.runTest?

roberth · 2023-05-11T16:57:31Z

Does this optimization also works by default on out-of-tree tests that use nixos-lib.runTest?

Yeah, just set node.pkgs = pkgs;

That reminds me, #225313 should use it.

RaitoBezarius · 2023-05-11T17:00:51Z

@roberth I think this is a breaking change technically. While I am fine with it, I feel like it should have some:

You may experience that some tests cannot be run anymore with nix-build $test.nix, please use the qualified path in nixosTests, look into all-tests.nix to find it if necessary, some examples includes gitlab, etc.

message in the release notes for 23.05 if that's okay with you :).

(not this PR per se, but the PRs transforming the tests to use this)

Also, I know that manual mention it's not recommended to do this anymore, but we didn't really any warning / deprecation notice, so I think it is helpful to have extra redundancy on that.

roberth · 2023-05-11T17:54:09Z

Individual test are not worth reporting on, but we could announce that this is happening.

warning / deprecation notice

They're not really a public interface, in the sense that nixpkgs is a library of packages and modules and whatnot, but it is a frequently used user interface nonetheless.

Speaking of policy, this should probably be a formal community decision before we migrate other people's tests. Do you think it should be an RFC?

RaitoBezarius · 2023-05-11T18:05:41Z

Individual test are not worth reporting on, but we could announce that this is happening.

Individual no, general notice yes. (with limited examples) :)

warning / deprecation notice

They're not really a public interface, in the sense that nixpkgs is a library of packages and modules and whatnot, but it is a frequently used user interface nonetheless.

That's true, but we are big enough to make it nice for people working with internal APIs to be noticed about changes I believe. People who does this work are the most likely to read the changelog properly, so it's extra appreciated I believe.

Speaking of policy, this should probably be a formal community decision before we migrate other people's tests. Do you think it should be an RFC?

I don't believe it should be a RFC, test ownership is already inferior to module ownership which is itself inferior to package ownership.

But we should definitely ping maintainers on each of their tests when they are listed as maintainers, but I would not block any decision on them.

roberth · 2023-05-11T19:22:13Z

Fair enough. If it's individual / collaborative choice and not policy, it's still worth noting.

Calling `eval-config.nix` without a `system` from a Nix flake fails with `error: attribute 'currentSystem' missing` since #230523. Setting `system = null` removes the use of `currentSystem` and instead uses the value from the `nixpkgs` module.

roberth requested review from edolstra and infinisil as code owners May 7, 2023 13:49

github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` labels May 7, 2023

roberth requested a review from tfc May 7, 2023 13:51

roberth added the 6.topic: testing Tooling for automated testing of packages and modules label May 7, 2023

ofborg bot added 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. labels May 7, 2023

Kranzes reviewed May 7, 2023

View reviewed changes

roberth force-pushed the fast-nixos-test-eval branch 2 times, most recently from c56f2b7 to f879074 Compare May 7, 2023 22:06

roberth added 5 commits May 10, 2023 15:55

lib.types.pkgs: init

16d594a

A nominal type.

nixos/nixpkgs: Use types.pkgs

6e594fe

nixos/eval-config: Remove statically known mkIf

693e2c3

mkIf is unnecessary when the condition is statically known - that is knowable before entering the module evaluation. By changing this to a precomputed module, we support changing the defined options to readOnly options.

nixosModules.pkgsReadOnly: init

e5db80a

nixos/all-tests.nix: Set nixpkgs.system

cd358fe

roberth mentioned this pull request May 11, 2023

NixOS tests: resolve aliases #231287

Closed

12 tasks

roberth added 5 commits May 11, 2023 16:24

nixos/all-tests.nix: Add readOnlyPkgs module

b213791

nixos/testing: Add node.pkgs option

f659db7

By factoring out this logic, it's easier for other projects to make use of it this optimization too (and do it correctly).

nixos/testing: Add node.pkgsReadOnly escape hatch

0f83261

By adding this option indirection, a test can declare all by itself that it needs a custom nixpkgs. This is a more convenient way of going about this when the caller of the test framework receives a `node.pkgs` unconditionally.

nixos/testing/nodes.nix: Do not rely on disabledModules

b0e1789

It's just not necessary.

roberth force-pushed the fast-nixos-test-eval branch from f879074 to e7df22d Compare May 11, 2023 14:24

nixos/all-tests: Enable readOnlyPkgs by default for runTest

16e3647

Most tests are not affected by this because they use the `handleTest` function instead.

roberth force-pushed the fast-nixos-test-eval branch from e7df22d to 16e3647 Compare May 11, 2023 15:34

roberth merged commit 5c3e59b into NixOS:master May 11, 2023

B4dM4n mentioned this pull request May 12, 2023

treewide: pass system argument to eval-config.nix #231473

Merged

12 tasks

nikstur mentioned this pull request May 22, 2023

stub: add fat variant nix-community/lanzaboote#182

Merged

roberth mentioned this pull request Jun 13, 2023

stdenv: fix crossSystem localSystem comparison #237427

Closed

12 tasks

Uh oh!

Conversation

roberth commented May 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes

Things done

Uh oh!

Kranzes May 7, 2023

Choose a reason for hiding this comment

Uh oh!

roberth May 7, 2023

Choose a reason for hiding this comment

Uh oh!

Kranzes May 7, 2023

Choose a reason for hiding this comment

Uh oh!

roberth May 7, 2023

Choose a reason for hiding this comment

Uh oh!

roberth May 7, 2023

Choose a reason for hiding this comment

Uh oh!

tfc commented May 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roberth commented May 11, 2023

Uh oh!

Kranzes commented May 11, 2023

Uh oh!

roberth commented May 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kranzes commented May 11, 2023

Uh oh!

roberth commented May 11, 2023

Uh oh!

RaitoBezarius commented May 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roberth commented May 11, 2023

Uh oh!

RaitoBezarius commented May 11, 2023

Uh oh!

roberth commented May 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

roberth commented May 7, 2023 •

edited

Loading

tfc commented May 8, 2023 •

edited

Loading

roberth commented May 11, 2023 •

edited

Loading

RaitoBezarius commented May 11, 2023 •

edited

Loading