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

Jump to bottom

nixos-container: minor fixes #79736

Merged
merged 2 commits into from Feb 22, 2020
Merged

nixos-container: minor fixes #79736

merged 2 commits into from Feb 22, 2020

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:

  • 3909f50990ef2a33828c2b43b89846f718d72ea2 - 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).

  • 7d51c490b58977f725710d839c8f0aefa39947ba - 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 Ma27 added 0.kind: bug 6.topic: nixos-container Imperative and declarative systemd-nspawn containers labels Feb 10, 2020
@Ma27 Ma27 added this to the 20.03 milestone Feb 10, 2020
@Ma27 Ma27 force-pushed the minor-imperative-container-fixes branch from 7d51c49 to 31bbcc2 Compare February 11, 2020 13:50
@Ma27
Copy link
Member Author

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 February 11, 2020 13:51
@danbst
Copy link
Contributor

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 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 February 20, 2020 02:04
@danbst danbst merged commit 9336b08 into NixOS:master Feb 22, 2020
@Ma27 Ma27 deleted the minor-imperative-container-fixes branch February 22, 2020 20:48
@Ma27
Copy link
Member Author

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 commented Feb 23, 2020

@Ma27 yeah, no objections, please you do

@Ma27 Ma27 added the 9.needs: port to stable A PR needs a backport to the stable release. label Feb 23, 2020
@Ma27
Copy link
Member Author

Ma27 commented Feb 23, 2020

Backported as 3d9983b, 73d246f.

@Ma27 Ma27 added 8.has: port to stable A PR already has a backport to the stable release. and removed 9.needs: port to stable A PR needs a backport to the stable release. labels Feb 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edolstra edolstra Awaiting requested review from edolstra

@danbst danbst Awaiting requested review from danbst

Assignees
No one assigned
Labels
0.kind: bug 6.topic: nixos 6.topic: nixos-container Imperative and declarative systemd-nspawn containers 8.has: port to stable A PR already has a backport to the stable release. 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
20.03
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Ma27 @danbst