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

Adding railcar container module #56411

Merged
merged 3 commits into from Sep 4, 2019
Merged

Conversation

@spacekookie
Copy link
Member

@spacekookie spacekookie commented Feb 26, 2019

Motivation for this change

The OCI container spec describes a simple way of creating a container with a config.json and a rootfs. These containers can be run by a container runner (such as runc or railcar) without having to rely on a daemon to manage containers outside of a NixOS config.

This PR adds two things

  • A simple builder function that can create an OCI spec container in the nix-store
  • A module to configure one particular container runner (railcar)

I also added documentation to the manual to explain how to use the underlying container-building function, so that other container runner modules can be added down the line.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Copy link
Member

@alyssais alyssais left a comment

First, a disclosure for other reviewers: I helped a bit with the development of this, especially in the initial exploration. With that in mind, I'm giving feedback on the code, but I won't be merging this, nor making any judgement about whether this should be in nixpkgs (though I'd hope it won't be controversial). I'll leave that to a fresh pair of eyes.

As for this review, it's mostly little niggly style stuff. There are a lot of comments but they're pretty trivial and nothing to worry about. :)

pkgs/build-support/oci-wrapper/default.nix Outdated Show resolved Hide resolved
pkgs/build-support/oci-wrapper/default.nix Outdated Show resolved Hide resolved
pkgs/build-support/oci-wrapper/examples.nix Outdated Show resolved Hide resolved
nixos/modules/virtualisation/railcar.nix Show resolved Hide resolved
pkgs/build-support/oci-wrapper/default.nix Outdated Show resolved Hide resolved
pkgs/build-support/oci-wrapper/default.nix Outdated Show resolved Hide resolved
pkgs/build-support/oci-wrapper/default.nix Outdated Show resolved Hide resolved
nixos/modules/virtualisation/railcar.nix Outdated Show resolved Hide resolved
nixos/modules/virtualisation/railcar.nix Outdated Show resolved Hide resolved
@Lassulus
Copy link
Member

@Lassulus Lassulus commented Mar 5, 2019

Copy link
Member

@Mic92 Mic92 left a comment

Some updates regarding documentation.

doc/functions/ocitools.xml Outdated Show resolved Hide resolved
doc/functions/ocitools.xml Outdated Show resolved Hide resolved
doc/functions/ocitools.xml Outdated Show resolved Hide resolved
doc/functions/ocitools.xml Outdated Show resolved Hide resolved
doc/functions/ocitools.xml Outdated Show resolved Hide resolved
doc/functions/ocitools.xml Outdated Show resolved Hide resolved
doc/functions/ocitools.xml Outdated Show resolved Hide resolved
@spacekookie spacekookie force-pushed the railcar-module branch 2 times, most recently from b23640a to bea458f Mar 21, 2019
@spacekookie
Copy link
Member Author

@spacekookie spacekookie commented Mar 21, 2019

I updated the documentation according to the given advice.

I also ended up removing os and arch from the docs. The options still exist in the module as I think it's important to expose them as they're part of the spec, but more work needs to be put into making them useful before they should be mentioned in the manual.

pkgs/build-support/oci-wrapper/default.nix Outdated Show resolved Hide resolved
pkgs/build-support/oci-wrapper/default.nix Outdated Show resolved Hide resolved
pkgs/build-support/oci-wrapper/default.nix Outdated Show resolved Hide resolved
@alyssais
Copy link
Member

@alyssais alyssais commented May 8, 2019

This has been open for ages, and nobody has had any concerns, so I'm going to merge this in a couple of days if nothing comes up before then.

@spacekookie spacekookie deleted the railcar-module branch May 11, 2019
@spacekookie spacekookie restored the railcar-module branch May 11, 2019
@spacekookie spacekookie reopened this May 11, 2019
@spacekookie
Copy link
Member Author

@spacekookie spacekookie commented May 11, 2019

Woops

@spacekookie
Copy link
Member Author

@spacekookie spacekookie commented May 12, 2019

Ping @alyssais I changed the ociTools folder name

@edef1c
Copy link
Member

@edef1c edef1c commented May 15, 2019

I'm slightly unsure if having mounts be a list of attrsets is the right move… it's tempting to mirror the NixOS fileSystems option.

@cw789
Copy link
Contributor

@cw789 cw789 commented Jul 9, 2019

I'm looking forward to see ociTools within Nix.

@spacekookie spacekookie force-pushed the railcar-module branch 3 times, most recently from e5ccb32 to db8b81e Aug 30, 2019
@spacekookie
Copy link
Member Author

@spacekookie spacekookie commented Aug 30, 2019

Alright, it's been a while but I finally made some progress on this

Mounts into an OCI container now use a similar declarative structure as filesystems, making it all a bit less boilerplate-y. I adjusted the railcar module and docs accordingly.

I kinda wanted to add a test for all this, but couldn't figure out how to make that work. I'd love to add one later, but also kinda just wanna see this merged 😬 So I'll do another PR at some point with tests (famous last words, I know)

@alyssais alyssais requested a review from edef1c Aug 31, 2019
nixos/modules/virtualisation/railcar.nix Show resolved Hide resolved
nixos/modules/virtualisation/railcar.nix Outdated Show resolved Hide resolved
nixos/modules/virtualisation/railcar.nix Outdated Show resolved Hide resolved
pkgs/build-support/oci-tools/default.nix Outdated Show resolved Hide resolved
Copy link
Member

@alyssais alyssais left a comment

Tiny style things. We’re so close!!

nixos/modules/virtualisation/railcar.nix Outdated Show resolved Hide resolved
nixos/modules/virtualisation/railcar.nix Outdated Show resolved Hide resolved
@alyssais alyssais requested a review from edef1c Sep 3, 2019
edef1c
edef1c approved these changes Sep 4, 2019
@alyssais alyssais merged commit 589c156 into NixOS:master Sep 4, 2019
12 checks passed
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

7 participants