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/docker-containers: Rename to virtualisation.oci-containers.containers #85933

Merged
merged 1 commit into from May 4, 2020

Conversation

@adisbladis
Copy link
Member

adisbladis commented Apr 24, 2020

Motivation for this change

Using the docker daemon for the docker-containers never fit entirely in the NixOS model.

  • There was no way to apply systemd limits to each container systemd service.
  • It did not play nicely with systemd sandboxing features as the actual execution happens in the docker daemon context, not the systemd service.
  • Setting users on the systemd service would have no real effect.

This PR implements a minimal set of changes to change the runtime. Any module improvements (like rootless declarative containers) we can make with podman should be left for a follow-up.

The potential downside is that users may be confused by the change when they run docker ps and the container is not showing up.

Discussion

After the discussion in this PR I have decided to change the scope of the PR: to only make the backend for this module configurable while not changing the default quite yet.
Both backends (docker & podman) have NixOS tests.

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.
Copy link
Member

saschagrunert left a comment

Nice! LGTM

Copy link
Member

vdemeester left a comment

This is nice 🎉
LGTM 🐯

@adisbladis adisbladis force-pushed the adisbladis:podman-declarative-containers branch from f568f44 to 31a954c Apr 24, 2020
@adisbladis
Copy link
Member Author

adisbladis commented Apr 24, 2020

@GrahamcOfBorg test docker-containers

@misuzu
Copy link
Contributor

misuzu commented Apr 24, 2020

Maybe this should be renamed docker-containers -> podman-containers since this is not really docker containers anymore? And maybe put it under virtualisation namespace if we are at it.

@ofborg ofborg bot added the 8.has: changelog label Apr 24, 2020
@adisbladis
Copy link
Member Author

adisbladis commented Apr 24, 2020

Maybe this should be renamed docker-containers -> podman-containers since this is not really docker containers anymore? And maybe put it under virtualisation namespace if we are at it.

I would rather not switch the name to podman-containers as I consider the runtime implementation to be unimportant.
I think docker-containers is a fine name even if we're not using docker as the underlying implementation any longer.

@saschagrunert
Copy link
Member

saschagrunert commented Apr 24, 2020

Maybe this should be renamed docker-containers -> podman-containers since this is not really docker containers anymore? And maybe put it under virtualisation namespace if we are at it.

How about OCI containers?

@adisbladis adisbladis force-pushed the adisbladis:podman-declarative-containers branch from 31a954c to 9a07505 Apr 24, 2020
@adisbladis
Copy link
Member Author

adisbladis commented Apr 24, 2020

How about OCI containers?

SGTM! I've renamed docker-containers to virtualisation.oci-containers.

@adisbladis adisbladis force-pushed the adisbladis:podman-declarative-containers branch from 9a07505 to 8b350d6 Apr 24, 2020
@Profpatsch
Copy link
Member

Profpatsch commented Apr 24, 2020

Now that’s what I call drop-in.

@danielfullmer
Copy link
Contributor

danielfullmer commented Apr 24, 2020

Would this allow us to switch to using cgroupsv2 in systemd by default? My understanding was that docker was holding us back for that change. Some previous discussion here: #73800 #68096

@saschagrunert
Copy link
Member

saschagrunert commented Apr 24, 2020

For cgroupsv2 we have to wait for a new runc release or switch to crun.

@denibertovic
Copy link

denibertovic commented Apr 25, 2020

As a NixOS user I'm super -1 on this. It's OK to add another module but removing docker-containers is going to suck for users like me who don't want to use podman (and the rest of that ecosystem). What's wrong with adding this as a separate module and leave the docker modules exist? I'm -1 on the name as well. Podman is not the only OCI compliant tool out there. It seems disingenuous to imply that.

@arianvp
Copy link
Member

arianvp commented Apr 25, 2020

We should fix outstanding issues with podman before merging this.

this one is pretty serious for example: #77925

@arianvp
Copy link
Member

arianvp commented Apr 25, 2020

I also agree with @denibertovic that naming it OCI is not fair. How about just naming it podman-containers ? I also see no reason to drop the docker-containers module for now; though we might want to throw an assertion error when enabling docker and cgroupsv2 at the same time in the future

Having it have a different distinct name; but still be drop-in is valuable and gaves the same benefits without stepping on toes of existing docker users.

Disabling the docker module conditionally when cgroupsv2 is enabled and giving a assertion message to switch to podman-containers sounds like the best way to solve that issue.

We could at a later stage maybe drop support for docker but I think it's still very early for that. -1 for changing the module name +1 for keeping both modules and documenting (through an assertion) that the one is a drop-in replacement for the other.

parameterized NixOS VM tests can make sure the two are in sync

@adisbladis
Copy link
Member Author

adisbladis commented Apr 25, 2020

As a NixOS user I'm super -1 on this. It's OK to add another module but removing docker-containers is going to suck for users like me who don't want to use podman (and the rest of that ecosystem).

Do you have any actual arguments for not using Podman & associated tooling or is this just FUD?

What's wrong with adding this as a separate module and leave the docker modules exist?

Fragmentation and the fact that running Docker containers inside systemd units has surprising limitations like being unable to constrain a unit in various ways like ReadWriteDirectories, CPUAccounting, etc, etc.
This means the current Docker container module never really worked in the composable way you'd expect from NixOS.

I'm strongly against duplicating the current module under a new podman-containers module as this would be pretty much a carbon-copy of the current module.

What we could do quite easily is to provide a parameter which docker runtime you'd like to use, then anyone actually wanting to use the Docker daemon for whatever reason could do so.
This parameter should default to podman for above mentioned reasons.

We should fix outstanding issues with podman before merging this.
this one is pretty serious for example: #77925

I completely agree.
Maybe if we implement the runtime parameter that should be set to docker by default until #77925 is fixed.

@denibertovic
Copy link

denibertovic commented Apr 25, 2020

Do you have any actual arguments for not using Podman & associated tooling or is this just FUD?

@adisbladis What kind of a response is that? Why would you imply malice straight away??!
I don't think I need to explain or list, potentially subjective, arguments why I want to use one tech over the other.
That would not be productive.
If this was a discussion about using apache instead of nginx (or the other way around I don't care) would I still need to present valid arguments why I want to be able to keep using my choice of program? I'm just saying
leave it to the user to choose whatever they want to use.

Fragmentation and the fact that running Docker containers inside systemd units has surprising limitations like being unable to constrain a unit in various ways like ReadWriteDirectories, CPUAccounting, etc, etc.

Not being able to use ReadWriteDirectories and CPUAccouting is very far from This means the current Docker container module never really worked in the composable way you'd expect from NixOS.

I don't need those features for instance. I'm sure there are many users that use various tech in different ways than you and I. That should be encouraged and they should be able to choose that.

I'm strongly against duplicating the current module under a new podman-containers module as this would be pretty much a carbon-copy of the current module.
What we could do quite easily is to provide a parameter which docker runtime you'd like to use, then anyone actually wanting to use the Docker daemon for whatever reason could do so.
This parameter should default to podman for above mentioned reasons.

I don't have any particular opinion on how the implementation should go. I'm sure there are various abstractions that could be used to make this not be a carbon copy. My point was merely that we should allow users to be able run one or the other.

I don't think there should be a default though. It should be clear to the user what it is they're "activating". It would be very wrong, as someone pointed out above, if the user ran a container and then did "docker ps" and got zilch.
Nix already has enough bad UX to keep adding to it. Just my 2c.

@apeyroux
Copy link
Member

apeyroux commented Apr 25, 2020

Do you have any actual arguments for not using Podman & associated tooling or is this just FUD?

My company uses docker and I want to be isofunctional with her for the dev.

@arianvp
Copy link
Member

arianvp commented Apr 26, 2020

I'm strongly against duplicating the current module under a new podman-containers module as this would be pretty much a carbon-copy of the current module

Luckily nix is a language and we can abstract these things no?

I really would like to avoid breaking compatibility for the sake of breaking it. If this PR is merged I would imagine something like this:

Oci-container module has indeed some kind of backend parameter that's either podman, docker or others.

There is a mkRenameModuleOption or equivalent that translates the old docker-containers module option to the new one and emits an optional warning about deprecation whilst setting the backend parameter to docker

Renaming a module without any form of depreciation or mkRenameModule sounds like a bad idea to me as it just breaks people's existing configs without any good reason. And also doesn't guide the user to do the right thing when nixos-rebuild switch'ing

Even if we do not want to have docker support Al all anymore (I don't see why we would make that choice given podman and docker have identical CLIs) we should give people a way to figure out from the evaluation error or warning how to migrate (or not)

edit: I see you already had a rename in place, however it would be nice to have a makeChangeOptionModule instead. I'll clarify below

.

@arianvp
Copy link
Member

arianvp commented Apr 26, 2020

Concretely:

I would suggest adding this to the module:

  1. add an option called virtualisation.oci-containers.backend which can either be docker or podman

  2. add the following mkChangedOptionModule (as opposed to the current mkRenameOptionModule) so that existing docker-containers modules get automatically translated to virtualisaiton.oci-containers whilst issueing a warning to the user, but still use docker:

    (lib.mkChangedOptionModule
      [ "docker-containers"  ]
      [ "virtualisation" "oci-containers" ]
      (oldcfg: lib.mkMerge [
        oldcfg 
        {backend = "docker";}
      ]))
  1. in the test suite; we can test both backends to make sure there weren't any regressions between the two.

  2. In the future we could decide to deprecate the docker backend and remove it. Add assertion that docker backend doesn't work with cgroupv2 flag in systemd etc..

This seems to both make @adisbladis happy as we dont have to maintain two modules and @denibertovic happy as they can keep using docker and get a hint on how to migrate.

@adisbladis adisbladis force-pushed the adisbladis:podman-declarative-containers branch 2 times, most recently from 737b737 to b98c691 Apr 26, 2020
@adisbladis
Copy link
Member Author

adisbladis commented Apr 26, 2020

According to the suggestion by @arianvp I've gone ahead and change the purpose of this PR.
Until #77925 is resolved we cannot merge the change of default runtime from docker to podman.

It's still my intent to do so but for now we're mainly changing the module name and making the backend configurable.

@adisbladis adisbladis force-pushed the adisbladis:podman-declarative-containers branch from b98c691 to 141cb72 Apr 26, 2020
@adisbladis adisbladis changed the title nixos/docker-containers: Use podman for declarative "docker" containers nixos/docker-containers: Rename to virtualisation.oci-containers.containers Apr 26, 2020
@adisbladis adisbladis force-pushed the adisbladis:podman-declarative-containers branch 3 times, most recently from e72380c to 3e68fed Apr 26, 2020
@adisbladis
Copy link
Member Author

adisbladis commented Apr 27, 2020

@GrahamcOfBorg test oci-containers

@arianvp
Copy link
Member

arianvp commented Apr 27, 2020

Looks good to me now

@grahamc
Copy link
Member

grahamc commented Apr 28, 2020

I just wrote up an issue about some issues with podman's overall packaging: #86245

@adisbladis
Copy link
Member Author

adisbladis commented Apr 29, 2020

Unless I hear objections I plan to merge this PR within the next couple of days.

Note that the PR scope has changed, this is now only a module rename + it makes the backend configurable.
We are not changing any defaults (yet).

…ainers.

And allow the runtime to be configurable via the
`virtualisation.oci-containers.backend` option.

Valid choices are "podman" and "docker".
@adisbladis adisbladis force-pushed the adisbladis:podman-declarative-containers branch from 3e68fed to 2f77475 May 4, 2020
@adisbladis adisbladis merged commit 4cbadd3 into NixOS:master May 4, 2020
14 checks passed
14 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="2f77475"; rev="2f7747526cc80844a506c4aa14706429324be157"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="2f77475"; rev="2f7747526cc80844a506c4aa14706429324be157"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="2f77475"; rev="2f7747526cc80844a506c4aa14706429324be157"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="2f77475"; rev="2f7747526cc80844a506c4aa14706429324be157"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="2f77475"; rev="2f7747526cc80844a506c4aa14706429324be157"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="2f77475"; rev="2f7747526cc80844a506c4aa14706429324be157"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="2f77475"; rev="2f7747526cc80844a506c4aa14706429324be157"; } ./pkgs/t
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
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

You can’t perform that action at this time.