fix: Handle terminate signals in init script #1390
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mikhailv
commented
Dec 26, 2024
docker/init_scripts/init
Outdated
Comment on lines
193
to
194
| # check for died processes every 5 seconds | ||
| sleep 5 |
There was a problem hiding this comment.
I moved sleep to the end of the loop because it does not make sense to wait for 5 seconds when script is started instead of starting of all child processes.
adamantike
reviewed
Dec 26, 2024
docker/init_scripts/init
Outdated
Comment on lines
119
to
126
| PROCESS=$1 | ||
| if [[ -f "/tmp/${PROCESS}.pid" ]]; then | ||
| PID=$(cat "/tmp/${PROCESS}.pid") || true | ||
| if [[ -d "/proc/${PID}" ]]; then | ||
| info_log "stopping ${PROCESS}" | ||
| kill "${PID}" || true | ||
| # wait for process exit | ||
| while [ -e "/proc/${PID}" ]; do sleep 0.1; done |
There was a problem hiding this comment.
nit: Seems indentation is broken within this function.
docker/init_scripts/init
Outdated
|
|
||
| shutdown() { | ||
| # shutdown in reverse order | ||
| stop_process_pid sheduler |
There was a problem hiding this comment.
typo: sheduler instead of scheduler
There was a problem hiding this comment.
nice catch, thanks, this is why last one python3 process been killed, now termination exit codes looks like
python3 163192 162602 163192 15.15 0
python3 163184 162602 163184 15.39 0
python3 163176 162602 163176 15.45 signal 15 (TERM)
nginx 163193 163163 163193 15.58 0
nginx 163190 163163 163190 15.58 0
nginx 163173 163163 163173 15.58 0
nginx 163189 163163 163189 15.58 0
nginx 163169 163163 163169 15.58 0
nginx 163178 163163 163178 15.58 0
nginx 163180 163163 163180 15.58 0
nginx 163172 163163 163172 15.58 0
nginx 163165 163163 163165 15.59 0
nginx 163191 163163 163191 15.58 0
nginx 163168 163163 163168 15.59 0
nginx 163186 163163 163186 15.59 0
nginx 163174 163163 163174 15.59 0
nginx 163177 163163 163177 15.59 0
nginx 163182 163163 163182 15.59 0
nginx 163166 163163 163166 15.59 0
nginx 163164 163163 163164 15.59 0
nginx 163183 163163 163183 15.59 0
nginx 163167 163163 163167 15.59 0
nginx 163170 163163 163170 15.59 0
nginx 163181 163163 163181 15.59 0
nginx 163187 163163 163187 15.59 0
nginx 163175 163163 163175 15.59 0
nginx 163185 163163 163185 15.59 0
nginx 163163 162602 163163 15.60 0
gunicorn 163154 163151 163154 16.65 signal 15 (TERM)
gunicorn 163155 163151 163155 16.63 signal 15 (TERM)
gunicorn 163151 162602 163151 16.90 0
valkey-server 162649 162602 162649 21.35 0
python3 163862 163735 163862 0.08 0
docker/init_scripts/init
Outdated
Comment on lines
193
to
194
| # check for died processes every 5 seconds | ||
| sleep 5 |
adamantike
approved these changes
Dec 26, 2024
|
Great improvement, and it works as expected based on my local testing. RomM requires signing all commits before we can merge this PR. |
`tini` does not wait for child processes to close, so all processes will be killed immediately. This is why the container stops so fast. This fix makes the `init` script listen and handle terminate signals. It also ensures that child processes are shut down in reverse order with proper waiting for completion.
mikhailv
force-pushed
the
fix/graceful-shutdown
branch
from
448071c to
f5bd75c
Compare
|
Done, I squashed two commits into one and signed it |
zurdi15
approved these changes
Dec 26, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
tini(added in PR) does not wait for child processes to exit gracefully, so all processes will be killed immediately. This is why the container stops so fast.Possible
tiniissue: krallin/tini#180This fix makes the
initscript listen and handle terminate signals. It also ensures that child processes are shut down in reverse order with proper waiting for completion.How to reproduce:
To see exit codes of processes I used
exitsnoop-bpfcctool from this toolset. Not sure maybe it is possible in easier way.This command started in terminal:
sudo exitsnoop-bpfcc | egrep '(gunicorn|valkey|nginx|python3)'Before the fix
As you may see, in the logs there are only
exited with code 0andStarting up, no any word about shutting downOutput of
exitsnoop-bpfcc:Almost all processes killed and this is bad because some of these processes may write some data to the disk and operation will be interrupted, data can be written incomplete.
After the fix
You may notice shutting down logs appeared in logs and shutdown handlers executed.
Output of
exitsnoop-bpfcc:You may see almost all processes exited with
0code orTERMwhich is also good I think. Only only process is finishing withKILLevery time, not sure why.