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-container: minor fixes #79736

Merged
merged 2 commits into from Feb 22, 2020
Merged

Conversation

@Ma27
Copy link
Member

@Ma27 Ma27 commented Feb 10, 2020

Motivation for this change

While there are plans to rework the nixos-container infrastructure (#69414) in the long-term, the imperative nixos-container tool is pretty helpful to e.g. test new modules in an isolated environment.

This PR fixes two issues that bothered me for quite some time:

  • 3909f50 - to avoid inconsistent state in case of a failing eval during nixos-container create, the entire state/config is removed in that case (until now you'd still have the /etc/containers/<name>.conf, but no running instance which breaks recreation-attempts).

  • 7d51c49 - add --nixos-path to nixos-container update as well. Helpful when hacking e.g. on new modules in a local checkout.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Ma27 added 2 commits Feb 10, 2020
…uild fails
@Ma27 Ma27 force-pushed the Ma27:minor-imperative-container-fixes branch from 7d51c49 to 31bbcc2 Feb 11, 2020
@Ma27
Copy link
Member Author

@Ma27 Ma27 commented Feb 11, 2020

Resolved the merge conflicts that were caused by the introduction of initial flake support.

@GrahamcOfBorg test containers-imperative

@Ma27 Ma27 requested a review from edolstra Feb 11, 2020
@danbst
Copy link
Contributor

@danbst danbst commented Feb 14, 2020

[danbst@station:~/dev/nixpkgs]$ sudo ./result/bin/nixos-container --nixos-path nixos create test1 --config '{ services.nginx.enable = true; }'
host IP is 10.233.2.1, container IP is 10.233.2.2
error: syntax error, unexpected '{', at /var/lib/containers/test1/etc/nixos/configuration.nix:8:3
(use '--show-trace' to show detailed location information)
./result/bin/nixos-container: failed to build initial container configuration

[danbst@station:~/dev/nixpkgs]$ cat /var/lib/containers/test1/etc/nixos/configuration.nix
cat: /var/lib/containers/test1/etc/nixos/configuration.nix: No such file or directory

Maybe it is possible to leave configuration file, so inspection of a problem is easier?

@Ma27
Copy link
Member Author

@Ma27 Ma27 commented Feb 17, 2020

Maybe it is possible to leave configuration file, so inspection of a problem is easier?

Good idea 👍 . However I wouldn't keep the full data directory. We could either print the configs to stderr (which might be harmful though if the config is rather long) or move it to a temporary file and print the path to it in the error message if the initial setup fails.

Regarding your issue: in case you haven't found it yourself, --config doesn't need the brackets at the beginning and end :)

@Ma27 Ma27 requested a review from danbst Feb 20, 2020
@danbst danbst merged commit 9336b08 into NixOS:master Feb 22, 2020
15 checks passed
15 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
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
tests.containers-imperative on aarch64-linux Success
Details
tests.containers-imperative on x86_64-linux Success
Details
@Ma27 Ma27 deleted the Ma27:minor-imperative-container-fixes branch Feb 22, 2020
@Ma27
Copy link
Member Author

@Ma27 Ma27 commented Feb 23, 2020

@danbst IMHO this should be backported as well. Are there any reasons against this and if not, do you intend to do this or should I take care of this? :)

@danbst
Copy link
Contributor

@danbst danbst commented Feb 23, 2020

@Ma27 yeah, no objections, please you do

@Ma27
Copy link
Member Author

@Ma27 Ma27 commented Feb 23, 2020

Backported as 3d9983b, 73d246f.

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

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