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

Adding railcar container module #56411

Merged
merged 3 commits into from Sep 4, 2019
Merged

Adding railcar container module #56411

merged 3 commits into from Sep 4, 2019

Conversation

spacekookie
Copy link
Member

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.

alyssais
alyssais requested changes Feb 26, 2019
View reviewed changes
Copy link
Member

@alyssais alyssais left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@spacekookie spacekookie force-pushed the railcar-module branch 3 times, most recently from cc132c1 to dee4776 Compare March 2, 2019 19:21
@Lassulus
Copy link
Member

Lassulus commented Mar 5, 2019

@GrahamcOfBorg eval

Mic92
Mic92 requested changes Mar 5, 2019
View reviewed changes
Copy link
Member

@Mic92 Mic92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Compare March 21, 2019 15:57
@spacekookie
Copy link
Member Author

spacekookie commented Mar 21, 2019 •
edited

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.

alyssais
alyssais reviewed Mar 22, 2019
View reviewed changes
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 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 18:43
@spacekookie spacekookie restored the railcar-module branch May 11, 2019 18:43
@spacekookie spacekookie reopened this May 11, 2019
@spacekookie
Copy link
Member Author

Woops

@spacekookie
Copy link
Member Author

Ping @alyssais I changed the ociTools folder name

@edef1c
Copy link
Member

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 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 Compare August 30, 2019 13:47
@spacekookie
Copy link
Member Author

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 August 31, 2019 12:34
edef1c
edef1c requested changes Aug 31, 2019
View reviewed changes
nixos/modules/virtualisation/railcar.nix Show resolved Hide resolved
nixos/modules/virtualisation/railcar.nix Outdated Show resolved Hide resolved
edef1c
edef1c reviewed Aug 31, 2019
View reviewed changes
nixos/modules/virtualisation/railcar.nix Outdated Show resolved Hide resolved
pkgs/build-support/oci-tools/default.nix Outdated Show resolved Hide resolved
alyssais
alyssais requested changes Sep 3, 2019
View reviewed changes
Copy link
Member

@alyssais alyssais left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 September 3, 2019 15:36
@alyssais alyssais merged commit 589c156 into NixOS:master Sep 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mic92 Mic92 Mic92 requested changes

@edef1c edef1c edef1c approved these changes

@alyssais alyssais alyssais approved these changes

@infinisil infinisil Awaiting requested review from infinisil

Assignees
No one assigned
Labels
6.topic: nixos 8.has: documentation 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@spacekookie @Lassulus @alyssais @edef1c @cw789 @Mic92 @GrahamcOfBorg