Merge container configurations correctly. #17365

Merged
merged 1 commit into from Aug 29, 2016

Projects

None yet

8 participants

@layus
Contributor
layus commented Jul 29, 2016

An issue arises with container configuration when one defines multiples times the same sub-option, or tries to merge two configuration sets.

For example, only the second extraHost is taken into account in the following setup:

{ lib, ... }:
{
    containers.test-1.config = let
        cfg1 = {
            networking.extraHosts = ''
                10.10.10.10 bla1
            '';
        };
        cfg2 = {
            networking.extraHosts = ''
                11.11.11.11 bla2
            '';
        };
    in lib.mkMerge [
        cfg1
        cfg2
    ];
}
$ export NIXOS_CONFIG=/above/file.nix
$ nix-instantiate "<nixpkgs/nixos>" -A config.containers.test-1.config.networking.extraHosts --eval
"11.11.11.11 bla2\n"
$ # whoops ;-)

This arises because config.containers.<name>.config has no type, and the merge defaults to a dumb // of the two attributes sets.

This pull request moves eval-config.nix earlier in the evaluation process.
This way, eval-config.nix is used to merge configuration settings according to their expected (top-level) definition.

I would love input from @shlevy and @nbp as it seems related to comments they made in the module architecture.

@danbst, This is both an explanation of why your config did not work and a potential fix for your issue described in http://lists.science.uu.nl/pipermail/nix-dev/2016-July/thread.html#20989

@mention-bot

@layus, thanks for your PR! By analyzing the annotation information on this pull request, we identified @edolstra, @kampfschlaefer and @wavewave to be potential reviewers

@kampfschlaefer
Contributor

looks fine to me

but I have no commit permissions (yet)

@joachifm joachifm added this to the 16.09 milestone Jul 30, 2016
@fpletz
Member
fpletz commented Jul 31, 2016

Also looks fine to me, but I'd also like to wait for comments by @shlevy and @nbp.

@nbp
Member
nbp commented Aug 10, 2016

Thanks for looking into this :)

This arises because config.containers.<name>.config has no type, and the merge defaults to a dumb // of the two attributes sets.

I understand the current code and the code present before that, still the config attribute should not be an option within the submodule option definitions, as config is already part of the module system.

Identically, using eval-config.nix should not be necessary as this is already part of the types.submodule option type.

Thus, I think the proper solution would be to remove the config option from the submodule.

All, it seems that the .config is used as a short-cut for the .system.build.toplevel, in which case a new option might be needed.

@layus
Contributor
layus commented Aug 10, 2016

@nbp Well, config is used by the module system, and also as an option name. I see no conflict as you can access the config option with config.config, right ?
If we remove the config option from the submodule, how would we configure the container ?

eval-config.nix should not be necessary, but is helpful here because it pulls all NixOS modules in the definition of the submodule. How would you do that with types.submodule ?
That file also takes into acount "NIXOS_EXTRA_MODULE_PATH". Should we duplicate its content ?

All in all, we need your help to understand @shlevy's comment in https://github.com/NixOS/nixpkgs/blob/master/nixos/lib/eval-config.nix#L5-L10, outlining that containers could use types.submodule, but also implying that it is not possible right now.

@nbp
Member
nbp commented Aug 14, 2016

@nbp Well, config is used by the module system, and also as an option name. I see no conflict as you can access the config option with config.config, right ?

Yes, but there is no value at all in calling the module system twice, here. And I think having a config option is more likely to confuse the module system in how the content should be interpreted, as well as all the users of the containers, because writing something like:

{
    containers.test-1.config = {
            networking.extraHosts = ''
                10.10.10.10 bla1
            '';
        };
}

should be equivalent to:

{
    containers.test-1 = {
            networking.extraHosts = ''
                10.10.10.10 bla1
            '';
        };
}

for submodules.

eval-config.nix should not be necessary, but is helpful here because it pulls all NixOS modules in the definition of the submodule. How would you do that with types.submodule ?

All NixOS modules, can be provided to the module system as an option definition in eval-config.nix, in which case extra modules can be loaded by simply using this list as the inherits attribute for the container submodule. There is no need for manually calling eval-config.nix.

That file also takes into acount "NIXOS_EXTRA_MODULE_PATH". Should we duplicate its content ?

If eval-config.nix set the content of extraModules as an option definition, then this can be re-used in any submodule, identically. More over, these can be extended with custom configuration, such as making a container with a custom module, which is used no where but in this one specific container.

All in all, we need your help to understand @shlevy's comment in https://github.com/NixOS/nixpkgs/blob/master/nixos/lib/eval-config.nix#L5-L10, outlining that containers could use types.submodule, but also implying that it is not possible right now.

I do understand this comment, and this comment is mostly out of date by the fact that we now have the ability to extend the module system arguments from the _module.args option within the module system.

@domenkozar
Member

Just a heads up: to get this into 16.09 it needs to be reviewed, addressed and merged in a few days.

@danbst
Contributor
danbst commented Aug 28, 2016

I'm in favor to include this hotfix as it is, and proper solution in next release

@layus
Contributor
layus commented Aug 29, 2016

@domenkozar I agree with @danbst that we should merge it as-is. I have tried to implement a better solution, and it seems to me that the module system needs patching to support recursive use.

@fpletz fpletz merged commit 3aef93e into NixOS:master Aug 29, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@domenkozar
Member

@layus can I ask you to write a changelog entry for this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment