nixos-container: fix `nixpkgs` container options being ignored #106767

erikarvstedt · 2020-12-12T17:36:59Z

Since the introduction of option containers.<name>.pkgs, the nixpkgs.* options (including nixpkgs.pkgs, nixpkgs.config, ...) were always ignored in container configs, which broke existing containers.

This was due to containers.<name>.pkgs having two separate effects:

It sets the source for the modules that are used to evaluate the container.
It sets the pkgs arg (_module.args.pkgs) that is used inside the container modules.
This happens even when the default value of containers.<name>.pkgs is unchanged, in which
case the container pkgs arg is set to the pkgs of the host system.
Previously, the pkgs arg was determined by the containers.<name>.config.nixpkgs.* options.

This commit reverts the breaking change (2.) while adding a backwards-compatible way to achieve (1.).
It removes option pkgs and adds option nixpkgs which implements (1.).
Existing users of pkgs are informed by an error message to use option nixpkgs for (1.) or to achieve only (2.) by setting option containers.<name>.config.nixpkgs.pkgs.

The first three commits
tests/containers-custom-pkgs: improve test
nixos-container: simplify 'pkgs' option type
nixos-containers: remove redundant eval-config args
are refactorings without any functional changes.

Reproduce

The following config was broken before and is fixed by this PR:

nix-instantiate --eval - <<'EOF'
let
  configuration = {
    containers.mycontainer = {
      config = { pkgs, ... }: {
        nixpkgs.overlays = [ (self: super: {
          myval = "hello";
        }) ];
        networking.hostName = pkgs.myval;
      };
    };
  };
in
(import <nixpkgs/nixos> { inherit configuration; }).vm.outPath
EOF

# error: attribute 'myval' missing, at (string):8:31

Fixes #88621.
cc @adisbladis

This PR is an improved version of #98655

rnhmjoj · 2020-12-12T20:48:18Z

(1) It sets the source for the modules that are used to evaluate the container.

Ok, now I understand the need for a contains.<name>.pkgs

This PR reverts the breaking change (2) while keeping the backwards-compatible change (1).

Seems like the way to go. I'll try to review this in the next couple of days, for the moment I would suggest adding a subtest for overlays in containers to avoid more regressions in the future.

erikarvstedt · 2020-12-14T17:27:46Z

I've updated the test and added a helpful error message for current users of pkgs to avoid unexplained breaking changes.

To test the error message, run

nix-instantiate --eval - <<'EOF'
(import /path/to/nixpkgs/nixos {
  configuration = { pkgs, ... }: {
    containers.test = {
      inherit pkgs;
      config = {};
    };
  };
}).vm.outPath
EOF

erikarvstedt · 2020-12-14T17:28:15Z

@adisbladis, why did you add option containers.<name>.pkgs instead of using the existing option containers.<name>.config.nixpkgs.pkgs if your intention seemingly was to set the pkgs argument used inside the container modules?
Is your use case still supported with this PR?

rnhmjoj · 2020-12-14T18:33:45Z

Is your use case still supported with this PR?

From the test they added, it looks like the use case was modifying the package set. This can indeed be done using config.nixpkgs.overlays or similar, without additional changes. containers.<name>.pkgs could still be useful to change NixOS modules, though.

However, I fear this option (even if it add very little complexity to Nixpkgs) will make things more confusing for new users. Maybe we should rename it to something more descriptive.

erikarvstedt · 2020-12-14T18:54:47Z

I've also considered renaming the option. Do you think modulesPkgs would be a good fit?

rnhmjoj · 2020-12-14T20:25:50Z

Uhm, I'm not entirely sure because it will also affect other things besides the evaluation of modules.

erikarvstedt · 2020-12-14T20:39:11Z

Can you (very quickly) elaborate on what else it does affect? I don't see any other effects besides choosing the NixOS modules used for container evaluation.

What do you think about calling it nixpkgs, with description A path to the nixpkgs that are used for evaluating the container?

rnhmjoj · 2020-12-14T22:26:06Z

Can you (very quickly) elaborate on what else it does affect? I don't see any other effects besides choosing the NixOS modules used for container evaluation.

Besides modules and packages, I had lib in mind: it might affect the result of the evaluation in different ways.

What do you think about calling it nixpkgs, with description A path to the nixpkgs that are used for evaluating the container?

Good idea, it think it's more appropriate since the option is a path, pkgs has the connotation of attrset.

boxofrox · 2020-12-15T00:28:09Z

I must confess, I use NixOS recreationally since 17.09, and I'm lost as to what there difference between containers.<name>.nixpkgs.* and containers.<name>.pkgs is supposed to be. It sounds to me from this conversation, that if, for example, I want to run a container that uses 19.09 packages, exclusively, on a 20.09 host, I would originally use containers.<name>.nixpkgs.pkgs to import "path/to/19.09/nixpkgs" {}, and the new containers.<name>.pkgs (or .nixpkgs) is a replacement that would use a path instead of an import?

I'm certainly eager to see this (any) PR merged to address the issue preventing overlays from working inside containers so I can upgrade my last server. But I'm happy to wait for the confusion to be sorted.

rnhmjoj · 2020-12-15T07:35:02Z

I'm lost as to what there difference between containers..nixpkgs.* and containers..pkgs is supposed to be

The difference is a bit subtle and may have been unintended when containers.<name>.pkgs was first added.
The nub is nixpkgs/nixos/lib/eval-config.nix, which is a function that takes your system configuration and outputs the final system. It does not only affect how the system is built, but also the content of the packages and modules because eval-config.nix does imports relative to its path (ie ../modules/module-list.nix).

I want to run a container that uses 19.09 packages, exclusively, on a 20.09 host, I would originally use containers..nixpkgs.pkgs to import "path/to/19.09/nixpkgs" {}, and the new containers..pkgs (or .nixpkgs) is a replacement that would use a path instead of an import?

Yes, but the second option is not really a replacement: in the first case you would get a system with 19.09 packages but 20.09 NixOS modules and standard library (lib), from the host machine. In the second case, everything would be from 19.09.

- Fix name - Remove unneeded leftovers that were copied from containers-hosts.nix - Remove redundant 'start_all()'

The values of these args are identical to the default values defined in `eval-config.nix`. Note especially that `lib` is not reevaluated.

Set the default value directly instead of using a `null` proxy value.

erikarvstedt · 2020-12-15T20:11:19Z

@rnhmjoj, yeah, sorry, with modules I was also implying pkgs and lib because they're set by the modules.

I've now removed option pkgs and added option nixpkgs.
Thanks for all your thoughtful feedback so far!

rnhmjoj · 2020-12-18T19:05:39Z

nixos/modules/virtualisation/nixos-containers.nix

-            ];
+          config = let
+            # Throw an error when removed option `pkgs` is used.
+            # Because this is a submodule we cannot use `mkRemovedOptionModule` or option `assertions`.


Just hours ago Infinisil merged PR #97023 that should remove this limitation.
It had to be reverted because an issue came up but I'd keep it in mind.

boxofrox · 2021-01-13T22:46:24Z

Is there anything blocking progress on this that I could assist with?

rnhmjoj · 2021-01-13T23:01:34Z

I have been busy, I'll try to finish my review in the next couple of days.

erikarvstedt · 2021-01-13T23:11:33Z

@rnhmjoj, I don't think it's worth to wait for Infinisil's fixed PR.
Even when submodule assertions are supported, mkRemovedOptionModule will only show a confusing module-relative option path, instead of the full path.
Concretely, it will show

error: The option definition `pkgs' in `myconfig.nix' no longer has any effect; please remove it.

instead of

error: The option definition `containers.test.pkgs' in `myconfig.nix' no longer has any effect; please remove it.

The above is the error msg output by the current code.

rnhmjoj · 2021-01-14T07:56:51Z

@rnhmjoj, I don't think it's worth to wait for Infinisil's fixed PR.

Agreed, I was mostly making a note rather than a request.

rnhmjoj

I've looked at this more carefully: the option and assetion are working as intended and diff looks good to me. I don't have any more suggestions but I'm not an expert in this part of NixOS (containers + module system) so I'm not confident with merging the PR right away.

erikarvstedt · 2021-01-15T11:48:54Z

@rnhmjoj, I've just done some a minor editing to clarify the description of commit nixos-container: fix nixpkgs container options being ignored. This should make reviewing easier.
Please reapprove.

Since the introduction of option `containers.<name>.pkgs`, the `nixpkgs.*` options (including `nixpkgs.pkgs`, `nixpkgs.config`, ...) were always ignored in container configs, which broke existing containers. This was due to `containers.<name>.pkgs` having two separate effects: (1) It sets the source for the modules that are used to evaluate the container. (2) It sets the `pkgs` arg (`_module.args.pkgs`) that is used inside the container modules. This happens even when the default value of `containers.<name>.pkgs` is unchanged, in which case the container `pkgs` arg is set to the pkgs of the host system. Previously, the `pkgs` arg was determined by the `containers.<name>.config.nixpkgs.*` options. This commit reverts the breaking change (2) while adding a backwards-compatible way to achieve (1). It removes option `pkgs` and adds option `nixpkgs` which implements (1). Existing users of `pkgs` are informed by an error message to use option `nixpkgs` or to achieve only (2) by setting option `containers.<name>.config.nixpkgs.pkgs`.

nixos-discourse · 2021-02-06T10:21:59Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/342

boxofrox · 2021-02-08T01:21:14Z

Is it possible backport this fix to 20.09, or shall I just wait until 21.05 to update my servers?

Mic92 · 2021-02-08T07:19:51Z

You can open a backport pull request if you want.

erikarvstedt mentioned this pull request Dec 12, 2020

nixos-container: re-enable nixpkgs option #98655

Closed

erikarvstedt requested review from rnhmjoj, Ma27, zimbatm and adisbladis December 12, 2020 17:39

ofborg bot added 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Dec 12, 2020

erikarvstedt force-pushed the fix-container-pkgs-2 branch from 5d60630 to ed3cb44 Compare December 12, 2020 18:15

erikarvstedt force-pushed the fix-container-pkgs-2 branch 3 times, most recently from b13bcb7 to c3b91ea Compare December 14, 2020 17:52

erikarvstedt added 3 commits December 15, 2020 20:25

tests/containers-custom-pkgs: improve test

34fcfc8

- Fix name - Remove unneeded leftovers that were copied from containers-hosts.nix - Remove redundant 'start_all()'

nixos-containers: remove redundant eval-config args

29385f0

The values of these args are identical to the default values defined in `eval-config.nix`. Note especially that `lib` is not reevaluated.

nixos-container: simplify 'pkgs' option type

77c4fc2

Set the default value directly instead of using a `null` proxy value.

erikarvstedt force-pushed the fix-container-pkgs-2 branch from c3b91ea to 8c265f4 Compare December 15, 2020 20:07

rnhmjoj reviewed Dec 18, 2020

View reviewed changes

rnhmjoj approved these changes Jan 15, 2021

View reviewed changes

SuperSandro2000 requested a review from Mic92 January 15, 2021 11:34

erikarvstedt force-pushed the fix-container-pkgs-2 branch from 8c265f4 to c139941 Compare January 15, 2021 11:44

erikarvstedt force-pushed the fix-container-pkgs-2 branch from c139941 to 9a283a0 Compare January 15, 2021 11:50

infinisil self-requested a review January 15, 2021 12:34

rnhmjoj added this to the 21.05 milestone Jan 29, 2021

adisbladis merged commit 3c6035c into NixOS:master Feb 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos-container: fix `nixpkgs` container options being ignored #106767

nixos-container: fix `nixpkgs` container options being ignored #106767

erikarvstedt commented Dec 12, 2020 •

edited

rnhmjoj commented Dec 12, 2020

erikarvstedt commented Dec 14, 2020 •

edited

erikarvstedt commented Dec 14, 2020

rnhmjoj commented Dec 14, 2020

erikarvstedt commented Dec 14, 2020

rnhmjoj commented Dec 14, 2020

erikarvstedt commented Dec 14, 2020

rnhmjoj commented Dec 14, 2020

boxofrox commented Dec 15, 2020

rnhmjoj commented Dec 15, 2020 •

edited

erikarvstedt commented Dec 15, 2020

rnhmjoj Dec 18, 2020

boxofrox commented Jan 13, 2021

rnhmjoj commented Jan 13, 2021

erikarvstedt commented Jan 13, 2021

rnhmjoj commented Jan 14, 2021

rnhmjoj left a comment

erikarvstedt commented Jan 15, 2021

nixos-discourse commented Feb 6, 2021

boxofrox commented Feb 8, 2021

Mic92 commented Feb 8, 2021

nixos-container: fix nixpkgs container options being ignored #106767

nixos-container: fix nixpkgs container options being ignored #106767

Conversation

erikarvstedt commented Dec 12, 2020 • edited

Reproduce

rnhmjoj commented Dec 12, 2020

erikarvstedt commented Dec 14, 2020 • edited

erikarvstedt commented Dec 14, 2020

rnhmjoj commented Dec 14, 2020

erikarvstedt commented Dec 14, 2020

rnhmjoj commented Dec 14, 2020

erikarvstedt commented Dec 14, 2020

rnhmjoj commented Dec 14, 2020

boxofrox commented Dec 15, 2020

rnhmjoj commented Dec 15, 2020 • edited

erikarvstedt commented Dec 15, 2020

rnhmjoj Dec 18, 2020

Choose a reason for hiding this comment

boxofrox commented Jan 13, 2021

rnhmjoj commented Jan 13, 2021

erikarvstedt commented Jan 13, 2021

rnhmjoj commented Jan 14, 2021

rnhmjoj left a comment

Choose a reason for hiding this comment

erikarvstedt commented Jan 15, 2021

nixos-discourse commented Feb 6, 2021

boxofrox commented Feb 8, 2021

Mic92 commented Feb 8, 2021

nixos-container: fix `nixpkgs` container options being ignored #106767

nixos-container: fix `nixpkgs` container options being ignored #106767

erikarvstedt commented Dec 12, 2020 •

edited

erikarvstedt commented Dec 14, 2020 •

edited

rnhmjoj commented Dec 15, 2020 •

edited