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

Module system improvements for NixOS as a submodule #75031

Merged
merged 8 commits into from Jan 2, 2020

Conversation

@Infinisil
Copy link
Member

@Infinisil Infinisil commented Dec 5, 2019

Motivation for this change

All these changes are there to allow NixOS to be used as submodule, which I'm doing tests with right now. This will allow things like:

  • Not having O(n²) evaluation time for n interdependent machines like in nixops (Edit: I'm actually unsure now if that would be fixed with this)
  • Writing modules that span multiple machines without problems

Ping @Profpatsch @rycee @kreisys @nbp @edolstra @oxij @danbst @roberth

Best reviewed commit by commit

Things done
  • Write documentation on submoduleWith
  • Find a better name than fullSubmodule -> Now named submoduleWith
  • Find a better argument name for configDefault -> Now named shorthandOnlyDefinesConfig
  • write a comment for what shorthandOnlyDefinesConfig does -> It's in the docs
  • Run lib tests successfully
  • Maybe add some new lib tests
    • For idempotent unifyModuleSyntax Nothing needs this behavior directly, so no tests necessary
    • For submoduleWith
    • For submodules with paths as modules
  • Made sure evaluation perf doesn't get worse (if at all it should get better hopefully) -> Indeed, evaluating the tested set gives smaller numbers for all values
  • Maybe deprecate packSubmodule and unpackSubmodule instead of removing, though they very very likely aren't used in the wild, so probably not worth keeping them around Removing them seems fine, it's really just an artifact from the module system implementation, there never should've been a need to expose this anyways.
  • Made sure the tested set evaluates still
  • Made sure that this change doesn't introduce weird derivation changes (specifically the commit about submodule packing) -> It does, but nothing significant, only value ordering in lists for the same priority, which is arbitrary anyways
Infinisil added 2 commits Dec 4, 2019
This will be useful for doing more complicated module evaluations
Because why not
@domenkozar
Copy link
Member

@domenkozar domenkozar commented Dec 5, 2019

I wonder if this changes #70638

@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Dec 5, 2019

@domenkozar Very relevant issue indeed! While this PR doesn't change that behavior, it does allow you to turn it off for specific submodules by replacing types.submodule with types.fullSubmodule {} (it uses configDefault = false by default).

This configDefault behavior transforms all submodule = value assignments (with a non-function value) to submodule.config = value. This is necessary (unfortunately) because there exist many options named config: If this wasn't done you'd have to do submodule.config.config = value to set a config option in the submodule.

@roberth
Copy link
Contributor

@roberth roberth commented Dec 5, 2019

Some more documentation will be helpful.

@danbst
Copy link
Contributor

@danbst danbst commented Dec 9, 2019

This is nice. Finally a coment left by @nbp makes sense.

Here I've tried to apply this fullSubmodule to containers configs (an excerpt):

diff --git a/nixos/modules/virtualisation/container-config.nix b/nixos/modules/virtualisation/container-config.nix
index f7a37d8c9f3..98a2091aee1 100644
--- a/nixos/modules/virtualisation/container-config.nix
+++ b/nixos/modules/virtualisation/container-config.nix
@@ -3,6 +3,14 @@
 with lib;
 
 {
+  options.boot.isContainer = mkOption {
+    type = types.bool;
+    default = false;
+    description = ''
+      Whether this NixOS machine is a lightweight container running
+      in another NixOS system.
+    '';
+  };
 
   config = mkIf config.boot.isContainer {
 
diff --git a/nixos/modules/virtualisation/containers.nix b/nixos/modules/virtualisation/containers.nix
index 09678ce9ea7..a07fb71acc7 100644
--- a/nixos/modules/virtualisation/containers.nix
+++ b/nixos/modules/virtualisation/containers.nix
@@ -1,4 +1,4 @@
-{ config, lib, pkgs, ... }:
+{ config, lib, pkgs, system, baseModules, ... }:
 
 with lib;
 
@@ -441,15 +441,6 @@ in
 {
   options = {
 
-    boot.isContainer = mkOption {
-      type = types.bool;
-      default = false;
-      description = ''
-        Whether this NixOS machine is a lightweight container running
-        in another NixOS system.
-      '';
-    };
-
     boot.enableContainers = mkOption {
       type = types.bool;
       default = !config.boot.isContainer;
@@ -458,6 +449,31 @@ in
       '';
     };
 
+    containers2 = let
+      m1 = { config, name, lib, ... }: {
+        disabledModules = [ "virtualisation/containers.nix" ];
+        options.autoStart = mkOption {
+          type = types.bool;
+          default = false;
+          description = ''
+            Whether the container is automatically started at boot-time.
+          '';
+        };
+        config = {
+          _module.args.baseModules = baseModules;
+          boot.isContainer = true;
+          networking.hostName = mkDefault name;
+          networking.useDHCP = false;
+          nixpkgs.system = system;
+        };
+      };
+    in mkOption {
+      type = types.attrsOf (types.fullSubmodule {
+        modules = [ m1 ] ++ baseModules;
+        specialArgs.modulesPath = builtins.toString ../.;
+      });
+    };
+
     containers = mkOption {
       type = types.attrsOf (types.submodule (
         { config, options, name, ... }:

I've also made the needed adjustments to containers API, see https://github.com/danbst/nixpkgs/tree/containers-full-submodules for full result. It is indeed much cleaner now.

As for performance, everything is not better than before. I've collected stats, and found out that instantiation is worse than before:

{ lib, ... }: {
    boot.loader.grub.devices = ["/dev/sdv"];
    fileSystems."/".device = "nodev";

    # generating identical containers with names c1, c2, c3...

    # for new containers API
    containers = lib.genAttrs (map (num: "c${toString num}") (lib.range 1 0)) (x: { });
    # for old containers API
    #containers = lib.genAttrs (map (num: "c${toString num}") (lib.range 1 0)) (x: { config = {}; });

    # STATS
    # old approach
    # containers  | time (sec)
    #      0      | 2.9
    #      1      | 6.5
    #      2      | 10.0
    #      5      | 19.7
    #     10      | 39.6
    #     20      | 73.7
    # Estimated formula: 2.9+3.6*n

    # new approach
    # containers  | time (sec)
    #      0      | 7.7
    #      1      | 15.8
    #      2      | 23.5
    #      5      | 47.7
    #     10      | 91.1
    # Estimated formula: 7.7+8.1*n
}

Maybe I made a mistake somewhere?

@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Dec 12, 2019

@danbst After investigating, it seems to have to do with the manual. Before your change with defaulting documentation.enable to false and a single container:

$ NIX_SHOW_STATS=1 nix-instantiate nixos --arg configuration ./config.nix -A config.system.build.toplevel --show-trace
{
  "cpuTime": 1.46572,
  [..]
  "gc": {
    "heapSize": 402976768,
    "totalBytes": 516986768
  }
}

After your change also with documentation.enable defaulting to false:

{
  "cpuTime": 1.45586,
  [..]
  "gc": {
    "heapSize": 402976768,
    "totalBytes": 520466960
  }
}

Patch to disable documentation:

diff --git a/nixos/modules/misc/documentation.nix b/nixos/modules/misc/documentation.nix
index deecb005270..28f686e2ad8 100644
--- a/nixos/modules/misc/documentation.nix
+++ b/nixos/modules/misc/documentation.nix
@@ -74,7 +74,7 @@ in
 
       enable = mkOption {
         type = types.bool;
-        default = true;
+        default = false;
         description = ''
           Whether to install documentation of packages from
           <option>environment.systemPackages</option> into the generated system path.

Also, trying to actually build the manual with the changes, I get

output '/nix/store/a2i90migwip9qia6wwz8sc9knxz43s3y-nixos-manual-html' is not allowed to refer to the following paths:
  /nix/store/0ds9dxm8gprzhfd8dnffh04iky8d03qy-openresolv-3.9.2
  /nix/store/0m71zgh745giq1awy8p442kvkjyhrqs5-xlockmore-5.59
  /nix/store/2xdfs6ia4jxf7kpx79hzplmm7v35aisk-iputils-20190709
  /nix/store/3agbpqiwbxdpi4apmf7pb976cvy3paz7-libnotify-0.7.8
  [..]

This probably comes from the fact that the pkgs scrubbing in

specialArgs = { pkgs = scrubDerivations "pkgs" pkgs; };
doesn't seem to work (which is needed for things like mkOption { default = pkgs.foo; } to not refer to pkgs.foo), and that module seems to cause the slowdown in general too. I have a feeling that potentially also all NixOS options are duplicated in the manual in containers.<name?>.*.

@danbst
Copy link
Contributor

@danbst danbst commented Dec 13, 2019

@Infinisil
yeah, with documentation disabled (both for system and containers) I get now instantiate time formula 1.2+1.2*n.

However it is still of same performance as previous approach. It uses same amount of memory. I doubt now if it had previously used O(n²) time to instantiate n machines. From my experiments it seems to be linear, with both approaches.

Though it is much cleaner API (for containers example), which is nice!

@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Dec 13, 2019

@danbst Ah yeah, the O(n^2) would only be with n interdependent machines in nixops

nixos/doc/manual/development/option-types.xml Outdated Show resolved Hide resolved
nixos/doc/manual/development/option-types.xml Outdated Show resolved Hide resolved
lib/types.nix Outdated Show resolved Hide resolved
lib/types.nix Outdated Show resolved Hide resolved
nixos/doc/manual/development/option-types.xml Outdated Show resolved Hide resolved
@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Dec 15, 2019

@danbst Looking it into your container rewrite a bit more, I figured out some problems:

  • All NixOS options would get duplicated under containers.*, which is no good. So there should be a way to turn off submodule options if desired, which the new commit 571c7da allows for
  • But there are some container specific options such as enableTun, which should not be hidden. So your rewrite of the container module should still keep containers.*.config as the NixOS submodule (for which we can disable sub options completely), but keep the options containers.*.enableTun just like before.

You can then do something like this to get it to work with the same performance as before (the first xml docs patch shouldn't be necessary anymore once the second point above is done):

diff --git a/nixos/doc/manual/administration/declarative-containers.xml b/nixos/doc/manual/administration/declarative-containers.xml
index d03dbc4d705..8cff2c97c3a 100644
--- a/nixos/doc/manual/administration/declarative-containers.xml
+++ b/nixos/doc/manual/administration/declarative-containers.xml
@@ -33,9 +33,9 @@ containers.database =
   follows:
 <programlisting>
 containers.database = {
-  <link linkend="opt-containers._name_.privateNetwork">privateNetwork</link> = true;
-  <link linkend="opt-containers._name_.hostAddress">hostAddress</link> = "192.168.100.10";
-  <link linkend="opt-containers._name_.localAddress">localAddress</link> = "192.168.100.11";
+  privateNetwork = true;
+  hostAddress = "192.168.100.10";
+  localAddress = "192.168.100.11";
 };
 </programlisting>
   This gives the container a private virtual Ethernet interface with IP address
diff --git a/nixos/modules/misc/documentation.nix b/nixos/modules/misc/documentation.nix
index deecb005270..8445b75f6bb 100644
--- a/nixos/modules/misc/documentation.nix
+++ b/nixos/modules/misc/documentation.nix
@@ -20,7 +20,10 @@ let
     options =
       let
         scrubbedEval = evalModules {
-          modules = [ { nixpkgs.localSystem = config.nixpkgs.localSystem; } ] ++ manualModules;
+          modules = [ {
+            nixpkgs.localSystem = config.nixpkgs.localSystem;
+            documentation.isDocumentationEval = true;
+          } ] ++ manualModules;
           args = (config._module.args) // { modules = [ ]; };
           specialArgs = { pkgs = scrubDerivations "pkgs" pkgs; };
         };
@@ -84,6 +87,15 @@ in
         # which is at ../../../doc/multiple-output.xml
       };
 
+      isDocumentationEval = mkOption {
+        type = types.bool;
+        default = false;
+        internal = true;
+        description = ''
+          Whether this module system evaluation is purely to get an options listing
+        '';
+      };
+
       man.enable = mkOption {
         type = types.bool;
         default = true;
diff --git a/nixos/modules/virtualisation/containers.nix b/nixos/modules/virtualisation/containers.nix
index bbeebdb9db8..8a755e872ff 100644
--- a/nixos/modules/virtualisation/containers.nix
+++ b/nixos/modules/virtualisation/containers.nix
@@ -635,6 +635,7 @@ in
       type = types.attrsOf (types.fullSubmodule {
         modules = [ containerDefaults containerParamsModule ] ++ baseModules;
         specialArgs.modulesPath = builtins.toString ../.;
+        hideSubOptions = config.documentation.isDocumentationEval;
       });
       default = {};
       example = literalExample

Edit: Instead of 571c7da, it would also be possible to inline this type modification into the container module, but that gets ugly because of substSubModules. So I think an attribute like hideSubOptions to do that in fullSubmodule is reasonable

@danbst
Copy link
Contributor

@danbst danbst commented Dec 15, 2019

@Infinisil what about this danbst@cd74159 ? This solves the containers options problems. I have just verified that generated manual includes only extra options.

BTW, I've just recalled that submodule types are mergeable. #24653 (comment) for insights. I wonder if fullSubmodule has same properties (specifically talking about those extra params for fullSubmodule args).

@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Dec 16, 2019

@danbst I think separating the container options from NixOS options is cleaner, because they're like on a higher level than just a standard NixOS configuration. Similarly I'd like for nixops to not use deployment.* NixOS options for what really are nixops options. Also, doing it this way makes backwards compatibility easier.

lib/modules.nix Outdated Show resolved Hide resolved
lib/modules.nix Outdated Show resolved Hide resolved
lib/types.nix Outdated Show resolved Hide resolved
lib/types.nix Outdated Show resolved Hide resolved
@danbst
danbst approved these changes Dec 17, 2019
@Infinisil Infinisil force-pushed the Infinisil:module-improvements branch from 093768c to 6e521fa Dec 17, 2019
@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Dec 17, 2019

Pushed code with @roberth's suggestions and rebased for a clean history. I'll do some final tests with this and if it looks good to both of you too, this should be ready to merge.

lib/types.nix Outdated Show resolved Hide resolved
@roberth
Copy link
Contributor

@roberth roberth commented Dec 31, 2019

Great work!
Also don't forget to document lib/types: Allow paths as submodule values

Infinisil added 2 commits Dec 4, 2019
…bmodule
This has the beneficial side effect of allowing paths to be used as modules in
types.{submodule,submoduleWith}
@Infinisil Infinisil force-pushed the Infinisil:module-improvements branch 2 times, most recently from ea7ef12 to a2e42df Jan 1, 2020
@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Jan 1, 2020

Changes:

  • Added a bunch of tests for submoduleWith
  • Added a small mention in the docs that paths can be used as submodule values
  • Added a small note to the coerce paths explaining why they're necessary

From my side this is ready to be merged now!

Copy link
Contributor

@roberth roberth left a comment

Found a documentation improvement (see review comment) and a question/idea.
I was thinking about the difference between options.x = submoduleWith { modules } and config.x = modules.
Did you or @danbst try hiding option docs by adding them through a config definition instead of the modules parameter?
I haven't checked this, but assuming you can use option priority (mkForce etc) to override the set of config modules, that may be a reason to pick the one or the other.

Perhaps these questions can also be addressed in the documentation.

nixos/doc/manual/development/option-types.xml Outdated Show resolved Hide resolved
@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Jan 2, 2020

Ah yes that does work, confirmed with this:

{ lib, ... }: {

  options.test = lib.mkOption {
    type = lib.types.submoduleWith {
      modules = [{
        options.foo = lib.mkOption {};
      }];
    };
  };

  config = {
    test.options.bar = lib.mkOption {};

    documentation.nixos.includeAllModules = true;

    boot.loader.grub.device = "nodev";
    fileSystems."/".device = "test";
  };
}
$ cat $(nix-build nixos --arg configuration ./config.nix -A config.system.build.manual.optionsJSON)/share/doc/nixos/options.json | grep test.bar

But yeah the fact that modules defined in the config section can be overridden with priorities doesn't seem very good for this, so I think what I suggested earlier to hide the docs is still preferable, feels cleaner too. I can add a small mention in the docs saying "Note that only the options defined in the modules argument will be included in rendered documentation" though.

Infinisil added 4 commits Dec 13, 2019
Module arguments should be taken from the arguments directly. This
allows evalModule's specialArgs to override them if necessary
@Infinisil Infinisil force-pushed the Infinisil:module-improvements branch from a2e42df to cc81320 Jan 2, 2020
@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Jan 2, 2020

@roberth Done did that

@roberth
roberth approved these changes Jan 2, 2020
Copy link
Contributor

@roberth roberth left a comment

👍

@danbst
danbst approved these changes Jan 2, 2020
@Infinisil Infinisil merged commit cdf79db into NixOS:master Jan 2, 2020
12 checks passed
12 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@Infinisil Infinisil deleted the Infinisil:module-improvements branch Jan 2, 2020
@Infinisil Infinisil mentioned this pull request Jan 3, 2020
2 of 2 tasks complete
@grahamc grahamc mentioned this pull request Jan 3, 2020
0 of 10 tasks complete
Infinisil added a commit to Infinisil/nixpkgs that referenced this pull request Jan 6, 2020
This fixes the dhcpcd issue in NixOS#76969,
which was exposed by NixOS#75031
introducing changes in the module ordering and therefore option ordering
too.

The dhcpcd issue would also be fixable by explicitly putting
dhcpcd's paths before others, however it makes more sense for systemd's
default paths to be after all others by default, since they should only
be a fallback, which is how binary finding will work if they come after.
dtzWill added a commit to dtzWill/nixpkgs that referenced this pull request Jan 6, 2020
This fixes the dhcpcd issue in NixOS#76969,
which was exposed by NixOS#75031
introducing changes in the module ordering and therefore option ordering
too.

The dhcpcd issue would also be fixable by explicitly putting
dhcpcd's paths before others, however it makes more sense for systemd's
default paths to be after all others by default, since they should only
be a fallback, which is how binary finding will work if they come after.

(cherry picked from commit 9327e1c)
colemickens added a commit to colemickens/nixpkgs that referenced this pull request Jan 7, 2020
This fixes the dhcpcd issue in NixOS#76969,
which was exposed by NixOS#75031
introducing changes in the module ordering and therefore option ordering
too.

The dhcpcd issue would also be fixable by explicitly putting
dhcpcd's paths before others, however it makes more sense for systemd's
default paths to be after all others by default, since they should only
be a fallback, which is how binary finding will work if they come after.
@Infinisil Infinisil mentioned this pull request Sep 27, 2020
2 of 2 tasks complete
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

4 participants
You can’t perform that action at this time.