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

Docker-containers: Consider the exit status of docker run in ExecStop #76444

Merged
merged 1 commit into from Jan 1, 2020

Conversation

@Atemu
Copy link
Member

@Atemu Atemu commented Dec 24, 2019

Motivation for this change

If the docker run command exits with a positive exit status we don't need to stop it.

In fact, if we try to stop it, the docker stop command will fail which then makes the service fail and that obviously shouldn't happen (see #73114).

I am aware that checking things with a shell script like that seems hacky but the systemd manual states that ExecStop needs to account for exit status by itself and there is no simpler way to make it do that afaik.

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) ~5K
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

I have no idea how I would go about using NixOS tests and a link to the source of a bunch of tests doesn't help. If someone could give me an example for this, that'd teach me a lot. (I did test it manually though.)
I tried the nixpkgs-review command but I don't think I used it properly, how do I do that? Is this even relevant for modules?

This is my first PR, I am pretty sure I missed something else too. Please tell me if I did and what.

Notify maintainers

cc @

We don't need to stop the container if it already exited sucessfully
@danbst
Copy link
Contributor

@danbst danbst commented Dec 25, 2019

I have no idea how I would go about using NixOS tests

clone nixpkgs repo && cd there
then
nix-build nixos/tests/nginx.nix (or nix-build nixos/tests/docker.nix)

That's it.

@danbst
Copy link
Contributor

@danbst danbst commented Dec 25, 2019

See also https://www.freedesktop.org/software/systemd/man/systemd.service.html#Type=

It is unset currently for declarative docker containers, which means it has type simple. It also seems like you want behavior of type oneshot.

You may set it with:

systemd.services.docker-mycontainer.serviceConfig.Type = "oneshot";

Also, you may use preStop option (https://nixos.org/nixos/options.html#systemd.services.%3Cname%3E.prestop) instead of ExecStop, which already is bash script.

@Atemu
Copy link
Member Author

@Atemu Atemu commented Dec 25, 2019

nix-build nixos/tests/nginx.nix (or nix-build nixos/tests/docker.nix)

That's exactly what I was looking for, thank you! I'm still learning how to properly interface with the Nix language, so basic things like these are still not obvious to me.

It is unset currently for declarative docker containers, which means it has type simple. It also seems like you want behavior of type oneshot.

The oneshot type is not is not what I'm looking for (I may have used the wrong terminology in my issue). I do want the service to stop when the main process exits, I just want the service to stop successfully if the main process succeeded and currently it doesn't.

You may set it with:

systemd.services.docker-mycontainer.serviceConfig.Type = "oneshot";

*mkForce and you'd also have to mkForce serviceConfig.Restart because oneshot doesn't work with Restart=always iirc.

You could to it that way but having to fiddle with systemd services to get your docker containers to behave the way you want is a bit unclean IMO and actually not at all obvious to someone who doesn't know how docker-containers are implemented in NixOS.
They'd also have to know about mkForce and that it's the solution to Nix complaining about unique values being declared in multiple places (which I didn't back then).

Also, you may use preStop option (https://nixos.org/nixos/options.html#systemd.services.%3Cname%3E.prestop) instead of ExecStop, which already is bash script.

That sounds cleaner, I'll take a look. Thanks!

@Atemu
Copy link
Member Author

@Atemu Atemu commented Dec 25, 2019

Also, you may use preStop option (https://nixos.org/nixos/options.html#systemd.services.%3Cname%3E.prestop) instead of ExecStop, which already is bash script.

That sounds cleaner, I'll take a look. Thanks!

I tried it out and it does indeed look a bit cleaner but I have a few issues with it:

  • It's not that much cleaner
  • It's not immediately obvious that it fills out the ExecStop field anymore, you have to know or look up what the option does
  • It now has to be written outside the serviceConfig which rips it out of the context of the other Exec* options and makes it even less clear
  • It puts a script in the form of a nix store path as an argument to ExecStop= in the unit file which adds a layer of abstraction for users looking at docker-containers' services through systemctl
@danbst
Copy link
Contributor

@danbst danbst commented Dec 26, 2019

okay, this sounds reasonable.

What about removing ExecStop entirely? Looking at https://docs.docker.com/engine/reference/commandline/stop/:

The main process inside the container will receive SIGTERM, and after a grace period, SIGKILL.

But this is almost systemd behavior when no ExecStop present (https://www.freedesktop.org/software/systemd/man/systemd.service.html):

Since no ExecStop= was specified, systemd will send SIGTERM to all processes started from this service, and after a timeout also SIGKILL.

The last bit (kill only main process instead of all subprocesses) can be achieved with KillMode=mixed (https://www.freedesktop.org/software/systemd/man/systemd.kill.html#KillMode=)

@Atemu
Copy link
Member Author

@Atemu Atemu commented Dec 26, 2019

Yeah, I also thought about that and assumed they were added for good reason.
The docker rm commands also seem kind of redundant because it uses docker run --rm.

I the author didn't include a comment on these parameters in their commit message or PR unfortunately but they do mention that reload isn't implemented because docker containers might not respond well to some kill signals (https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/virtualisation/docker-containers.nix#L192), so maybe docker stop is a safer way of stopping them?

@benley

@danbst
Copy link
Contributor

@danbst danbst commented Dec 26, 2019

The docker rm commands also seem kind of redundant because it uses docker run --rm.

+1, agree

they do mention that reload isn't implemented because docker containers might not respond

we can try =)

@benley
Copy link
Member

@benley benley commented Dec 27, 2019

I didn't know about $SERVICE_RESULT; this looks like a good thing.

If I recall correctly, the redundant docker rm is there to account for unusual situations like what happens if the system crashes hard while a container is running: when it boots up again, the old container still exists (I think) but is stopped, so docker run can't create another one with the same name.

@Atemu
Copy link
Member Author

@Atemu Atemu commented Dec 30, 2019

That makes sense, thanks a lot!

Maybe we should investigate that in a separate PR/Issue though, this one works for now.

@benley benley merged commit a461f3f into NixOS:master Jan 1, 2020
12 checks passed
12 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
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
dtzWill added a commit to dtzWill/nixpkgs that referenced this pull request Jan 23, 2020
We don't need to stop the container if it already exited sucessfully

(cherry picked from commit a461f3f)
@Atemu Atemu deleted the Atemu:docker-containers-execstop branch Jul 30, 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

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