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

Add a note about PID 1 not terminating automatically on signal #12228

Merged
merged 1 commit into from
Apr 13, 2015

Conversation

bboreham
Copy link
Contributor

@bboreham bboreham commented Apr 9, 2015

As raised in #12022, processes that do not deliberately catch and exit on SIGINT/SIGTERM do not terminate as expected when they are running as PID 1, which is a very common case inside a Docker container. This confused me for a while, so I thought it would be better to mention it in the Docker documentation.

The note I am proposing is rather terse, but hopefully enough to find information via Google. I am open to guidance on how to improve the readability.

I have added the same note in the two places that SIGINT and/or SIGTERM are mentioned in the existing documentation.

I also propose to change the statement that CTRL-c sends a SIGKILL - my tests say this is not the case, and it is certainly unexpected.

closes #12022

@cpuguy83
Copy link
Member

cpuguy83 commented Apr 9, 2015

In all cases SIGTERM, SIGINT, etc must be handled by the process. A process is not required to exit when it receives these signals. This is regardless of it being PID 1 or not.
In most cases when these signals are caught the process will do some cleanup (like finish writing to any files and close them), and then exit... but this is always up to the process.

SIGKILL is handed by the kernel and not the running process. The kernel will forcibly kill the process without giving the process any chance to do any cleanup or react in any way.

You cannot SIGKILL PID 1, hence SIGNAL_UNKILLABLE

So as it is right now, I don't think your notes are quite accurate.
NOTLGTM as is.

@bboreham
Copy link
Contributor Author

bboreham commented Apr 9, 2015

OK, I withdraw the claim that SIGNAL_UNKILLABLE is involved.

I'm not sure about your first paragraph - the default action for SIGINT and SIGTERM if no handler is installed is to terminate. But, empirically, it's different for PID 1.

Much of the discussion is linked from #3793

@moxiegirl
Copy link
Contributor

ping @cpuguy83 revision for your review

@bboreham
Copy link
Contributor Author

@cpuguy83 consider sig_task_ignored() in signal.c - doesn't that say that Linux ignores signals with default action when SIGNAL_UNKILLABLE is set?

@cpuguy83
Copy link
Member

@bboreham I'm wondering if this is a shell-ism (as in something bash/sh/zsh/etc do)... maybe someone more experienced here can weigh in? ping @crosbymichael @LK4D4 @jfrazelle @icecrime

Running a quick ruby loop as pid 1 with no special signal handlers, TERM does kill it.

@bboreham
Copy link
Contributor Author

@cpuguy83 I don't think so; the real reason this came up for me was running curl in a container with no shell.

As an illustration try sudo docker run -i ubuntu /bin/cat - the shell isn't running; control-C doesn't kill it; docker stop takes 10 seconds to kill it.

@cpuguy83
Copy link
Member

Yeah, it's going to depend on the app, I assume ruby has some default signal handler setup for TERM.
So a note saying as such would be good.

When you are attached to a container, and exit its main process, the process's
exit code will be returned to the client.

(Note that a process running as PID 1 inside a container will not
automatically terminate on receipt of SIGINT or SIGTERM)
Copy link
Member

Choose a reason for hiding this comment

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

If this is a "note", then it should probably be formatted as such (see the style guide). But I'd wait with changing until it's confirmed that everything is technically correct. Perhaps the doc maintainers have another way to formulate it so that no "note" is required.

@thaJeztah
Copy link
Member

Thanks, @bboreham! I added closes #12022 to the description so that GitHub automatically closes the issue if this gets merged.

@@ -482,10 +482,13 @@ simultaneously, screen sharing style, or quickly view the progress of your
daemonized process.

You can detach from the container (and leave it running) with `CTRL-p CTRL-q`
(for a quiet exit) or `CTRL-c` which will send a `SIGKILL` to the container.
(for a quiet exit) or `CTRL-c` which will send a `SIGINT` to the container.
Copy link
Contributor

Choose a reason for hiding this comment

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

This paragraph structure is a bit of work. If you could please clean it up while you are in there. Also the note needs reformatting as @thaJeztah notes.

Markdown here: https://gist.github.com/moxiegirl/60a935fccb2e33c8e5ff

---> suggested replace

You can detach from a container and leave it running with CTRL-p CTRL-q. This
is called a quiet exit. You can also exit using CTRL-c which sends a
SIGINT to the container. When you are attached to a container and exit its
main process, Docker returns the process' exit code to the client.

Note: A process running as PID 1 inside a container does not automatically
terminate on receipt of SIGINT or SIGTERM.

@bboreham bboreham force-pushed the 12022-signal-doc branch 2 times, most recently from 7d6c5fb to b6107e2 Compare April 13, 2015 15:10
@bboreham
Copy link
Contributor Author

I have re-worded the note to clarify that the issue applies to programs that don't install a signal handler.
Also applied the 'note' markup, thanks @moxiegirl!

On the 'a bit wonk' part, I have included a third commit that tries to tease apart the things that can happen. However, this is getting a bit further from the original intent, so I'm happy to move it into a separate PR if you prefer.

@@ -481,8 +481,17 @@ interactively. You can attach to the same contained process multiple times
simultaneously, screen sharing style, or quickly view the progress of your
daemonized process.

You can detach from the container (and leave it running) with `CTRL-p CTRL-q`
(for a quiet exit) or `CTRL-c` which will send a `SIGKILL` to the container.
You can detach from the container and leave it running with `CTRL-p
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, thanks for the "wonk." :-D U missed a period in the note. Also, no need for the future tense here.

"Readers use technical documents to perform tasks or gather information. For readers, these activities take place in the present. Therefore, the present tense is appropriate in most cases." -- Read Me First: A Style Guide for the Computer Industry

Finally, in your note, don't jam all the activity into a single sentence. Sentences like these are difficult to translate and to understand. (Ibid)

--->https://gist.github.com/moxiegirl/60a935fccb2e33c8e5ff

You can detach from the container and leave it running with CTRL-p CTRL-q (for a quiet exit) or with CTRL-c if --sig-proxy is false.
If --sig-proxy is true (the default),CTRL-c sends a SIGINT
to the container.

Note: A process running as PID 1 inside a container is treated
specially by Linux: it ignores any signal without a handler installed.
So, the process does not terminate on SIGINT or SIGTERM unless it is
coded to do so.

@thaJeztah
Copy link
Member

Looks like you forgot to sign-off the last commit; not a huge issue yet, but it should be corrected before merging. If you are able to rebase and squash your commits; here are some instructions; Work on your issue - Pull and rebase frequently

@bboreham bboreham force-pushed the 12022-signal-doc branch 2 times, most recently from 810113d to f7bd6d8 Compare April 13, 2015 16:06
@bboreham
Copy link
Contributor Author

I have fixed the missing period and backticks, and the missing sign-off.

The last commit was deliberately left separate to see if I had understood the intent of the 'wonk' comment. If you want all three squashed into one I can do that easily.

And, just noticed the 'don't jam all the activity into a single sentence' addition. Will wait for further comments to avoid crossing on the wire.

@thaJeztah
Copy link
Member

@bboreham no problem on the squashing; just a friendly "notice" so that you aware it will have to be done at some point. It's also to prevent maintainer pressing the "merge" button, not noticint that the commits haven't been squashed yet 😄

Also re-arranged the description of CTRL-c to make it clearer.

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
@bboreham
Copy link
Contributor Author

Rebased, squashed, split the sentence into two.
I also changed the wording from 'with no handler installed' to 'with the default action', to match the wording in the Linux man pages for signal.

@moxiegirl
Copy link
Contributor

LGTM

@moxiegirl
Copy link
Contributor

ping @jamtur01 @SvenDowideit @fredlf

@jamtur01
Copy link
Contributor

LGTM

jamtur01 added a commit that referenced this pull request Apr 13, 2015
Add a note about PID 1 not terminating automatically on signal
@jamtur01 jamtur01 merged commit e29dcd9 into moby:master Apr 13, 2015
@bboreham bboreham deleted the 12022-signal-doc branch April 14, 2015 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document the fact that SIGINT/control-C doesn't usually kill a container
6 participants