-
Notifications
You must be signed in to change notification settings - Fork 963
Ensure BookKeeper process receives sigterm in docker container #2857
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
Conversation
eolivelli
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
nicoloboschi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
docker/scripts/entrypoint.sh
Outdated
| chmod -R +x ${SCRIPTS_DIR} | ||
| echo "This is root, will use user $BK_USER to run command '$@'" | ||
| sudo -s -E -u "$BK_USER" /bin/bash "$@" | ||
| exec sudo -s -E -u "$BK_USER" /bin/bash "$@" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there's a similar problem here that the process gets wrapped by bash and doesn't receive the sigterm?
I would change this to something like this
exec sudo -s -E -u "$BK_USER" /bin/bash -c 'exec "$@"' -- "$@"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested it as is and it works. Sudo passes on the SIGTERM to the bin/bookkeeper script which itself uses exec when kicking off java.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true that it "works" from the SIGTERM signalling point of view. My comments are more about the existing solution that was in place.
It just seems odd that there isn't a symmetry to the other execution path where sudo isn't used. sudo -s -E -u "$BK_USER" /bin/bash "$@" expects that the command is always a bash script.
the equivalent of exec "$@" with sudo would be something like exec sudo -s -E -u "$BK_USER" /bin/bash -c 'exec "$@"' -- "$@" . It probably doesn't make difference in this context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I see your point. I'l update it.
lhotari
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
### Motivation Current official docker images do not handle the SIGTERM sent by the docker runtime and so get killed after the timeout. No graceful shutdown occurs. The reason is that the entrypoint does not use `exec` when executing the `bin/bookkeeper` shell script and so the BookKeeper process cannot receive signals from the docker runtime. ### Changes Use `exec` when calling the `bin/bookkeeper` shell script. Reviewers: Nicolò Boschi <boschi1997@gmail.com>, Enrico Olivelli <eolivelli@gmail.com>, Lari Hotari <None>, Matteo Merli <mmerli@apache.org> This closes apache#2857 from Vanlightly/docker-image-handle-sigterm
### Motivation Current official docker images do not handle the SIGTERM sent by the docker runtime and so get killed after the timeout. No graceful shutdown occurs. The reason is that the entrypoint does not use `exec` when executing the `bin/bookkeeper` shell script and so the BookKeeper process cannot receive signals from the docker runtime. ### Changes Use `exec` when calling the `bin/bookkeeper` shell script. Reviewers: Nicolò Boschi <boschi1997@gmail.com>, Enrico Olivelli <eolivelli@gmail.com>, Lari Hotari <None>, Matteo Merli <mmerli@apache.org> This closes apache#2857 from Vanlightly/docker-image-handle-sigterm
Motivation
Current official docker images do not handle the SIGTERM sent by the docker runtime and so get killed after the timeout. No graceful shutdown occurs.
The reason is that the entrypoint does not use
execwhen executing thebin/bookkeepershell script and so the BookKeeper process cannot receive signals from the docker runtime.Changes
Use
execwhen calling thebin/bookkeepershell script.