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/docker-containers: daemonize docker run and handle restarts by docker #77479

Closed
wants to merge 3 commits into from
Closed

nixos/docker-containers: daemonize docker run and handle restarts by docker #77479

wants to merge 3 commits into from

Conversation

misuzu
Copy link
Contributor

@misuzu misuzu commented Jan 11, 2020 •
edited

Motivation for this change

I've had several issues with existing implementation and made some changes that was working for me for over two weeks without any problems. So this is what i've done:
Issue with logs - was: either proper logging to journald or working docker logs, now: both working properly.
Issue with redundant docker run in process list - fixed.
Issue with containers names - was: docker-${name}.service, now: docker-${name} this is fixed by #78366
Issue with docker tools like watchtower - was: updating container would leave service in inconsistent state, now: service would stay up as it should be.

Also ported test to python because why not (#72828).

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.

cc @Atemu @benley

@misuzu
Copy link
Contributor Author

misuzu commented Jan 11, 2020

@GrahamcOfBorg test docker-containers

@misuzu
Copy link
Contributor Author

misuzu commented Jan 28, 2020

@GrahamcOfBorg test docker-containers

benley
benley reviewed Jan 29, 2020
View reviewed changes
nixos/modules/virtualisation/docker-containers.nix Outdated
@@ -204,8 +209,8 @@ let

serviceConfig = {
ExecStart = concatStringsSep " \\\n " ([
"${pkgs.docker}/bin/docker run"
"--rm"
"${pkgs.docker}/bin/docker run -d"
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about daemonizing the container. One of the nice things about keeping it in the foreground is that systemd can know when the container crashes and report it in systemctl status output, making these services behave somewhat more like normal systemd units. What do you think?

Copy link
Contributor

@mkaito mkaito Jan 29, 2020 •
edited

Choose a reason for hiding this comment

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

Seems like this switches process oversight from Systemd to Docker itself. Is this because of Watchtower relying on docker being the process supervisor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about daemonizing the container. One of the nice things about keeping it in the foreground is that systemd can know when the container crashes and report it in systemctl status output, making these services behave somewhat more like normal systemd units. What do you think?

docker run process consumes memory without doing much and breaks logging. This also makes declared containers "special" because they behave in a way that is not obvious from docker container management perspective.

Seems like this switches process oversight from Systemd to Docker itself. Is this because of Watchtower relying on docker being the process supervisor?

Yes

@danbst danbst mentioned this pull request Feb 3, 2020
Closed
10 tasks
@CRTified CRTified mentioned this pull request Feb 10, 2020
Closed
@danbst
Copy link
Contributor

danbst commented Feb 15, 2020

I've tested this approach and I think it is a no go. When container is stopped with docker stop, corresponding systemd service is not stopped. Service is up from NixOS perspective. This breaks NixOS guarantees on nixos-rebuild switch, where it starts everything that should be started.

Google led me to https://github.com/DonTseTse/systemd-docker. Maybe it can solve problems you mentioned (extra process and logs).

What about adding an option

docker-containers.*.supervisor = mkOption { 
   type = lib.types.enum [ "systemd" "systemd+docker" ];
   default = "systemd";
   description = ''
     When set to "systemd", NixOS is aware of status of container.
     When set to "systemd+docker", container can be stopped/restarted/modified, and NixOS may not notice that. But NixOS still can stop/restart container on it's own.
   '';
}

? Though it is also more of a hack.

@CRTified CRTified mentioned this pull request Feb 15, 2020
Merged
10 tasks
@misuzu misuzu closed this Apr 5, 2020
@misuzu misuzu deleted the docker-containers branch April 5, 2020 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkaito mkaito mkaito left review comments

@benley benley benley left review comments

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@misuzu @danbst @mkaito @benley