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

Closed
wants to merge 3 commits into from

Conversation

@misuzu
Copy link
Contributor

@misuzu misuzu commented Jan 11, 2020

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 misuzu commented Jan 11, 2020

@GrahamcOfBorg test docker-containers

@misuzu misuzu force-pushed the misuzu:docker-containers branch from 2e654a7 to fa07148 Jan 28, 2020
@misuzu
Copy link
Contributor Author

@misuzu misuzu commented Jan 28, 2020

@GrahamcOfBorg test docker-containers

@@ -204,8 +209,8 @@ let

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

This comment has been minimized.

@benley

benley Jan 29, 2020
Member

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?

This comment has been minimized.

@mkaito

mkaito Jan 29, 2020
Contributor

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

This comment has been minimized.

@misuzu

misuzu Jan 30, 2020
Author Contributor

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

@misuzu misuzu force-pushed the misuzu:docker-containers branch from fa07148 to 50eff3e Jan 30, 2020
@danbst danbst mentioned this pull request Feb 3, 2020
3 of 10 tasks complete
@misuzu misuzu force-pushed the misuzu:docker-containers branch from 50eff3e to 6a79c61 Feb 5, 2020
@danbst
Copy link
Contributor

@danbst 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.

@misuzu misuzu closed this Apr 5, 2020
@misuzu misuzu deleted the misuzu:docker-containers branch Apr 5, 2020
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

4 participants
You can’t perform that action at this time.