Skip to content

Conversation

bryanlatten
Copy link
Contributor

@bryanlatten bryanlatten commented May 12, 2019

First suppress initial TERM/HUP, then use cont-finish.d as shutdown script

  • Used according to newly-clarified + documented shutdown behavior: https://github.com/behance/docker-base#shutdown-behavior
  • Set drain timeout to 55s, deliberately <60s to account for lack of timing precision
  • Also, added inline startup check for nginx syntax. Prevents bad env variable inputs from producing invalid config

@bryanlatten bryanlatten requested review from mannytoledo and ko-be May 15, 2019 03:40
@bryanlatten bryanlatten force-pushed the cont-init-finish branch 2 times, most recently from 88d3ca4 to e1d5cab Compare May 15, 2019 04:02
@bryanlatten bryanlatten requested a review from bossjones May 22, 2019 04:11
@bryanlatten bryanlatten force-pushed the cont-init-finish branch 2 times, most recently from 4a8e856 to 0895d6a Compare May 22, 2019 21:58
First suppress initial TERM/HUP, then use 
cont-finish.d as shutdown script
@bryanlatten bryanlatten changed the title Nginx: using cont-init.d for more graceful shutdown Nginx: using cont-finish.d for more graceful shutdown May 23, 2019
Copy link
Collaborator

@adobejmong adobejmong left a comment

Choose a reason for hiding this comment

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

Minor comments. Everything else LGTM

CONF_NGINX_SERVER="/etc/nginx/nginx.conf" \
NOT_ROOT_USER=www-data
NOT_ROOT_USER=www-data \
S6_KILL_FINISH_MAXTIME=55000
Copy link
Collaborator

Choose a reason for hiding this comment

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

@bryanlatten : One thing we found out is that this value might be limited to less than 65.5 seconds (64k effectively) on Alpine: https://jira.corp.adobe.com/browse/EON-4242. Would you think its worthwhile to link to the JIRA and our the resources I link off it?

I guess if not here, maybe in the README. Also maybe just a note as to why 55 seconds was chosen (even if it was arbitrary) so that we'll remember why we did it. (We have a similar case where the "magic number" for image scans is CVE > 8 but nobody has real clarity how they arrived at this number :-))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the image scan number is/was set by ASSET - maybe log a separate ticket to update the scan display to say that ?

README.md Outdated
SERVER_LARGE_CLIENT_HEADER_BUFFERS | SERVER_LARGE_CLIENT_HEADER_BUFFERS=8 16k | [docs](http://nginx.org/en/docs/http/ngx_http_core_module.html#large_client_header_buffers)
SERVER_CLIENT_BODY_BUFFER_SIZE | SERVER_CLIENT_BODY_BUFFER_SIZE=128k | [docs](http://nginx.org/en/docs/http/ngx_http_core_module.html#client_body_buffer_size)
SERVER_LOG_MINIMAL | SERVER_LOG_MINIMAL=1 | Minimize the logging format, appropriate for development environments
S6_KILL_FINISH_MAXTIME | S6_KILL_FINISH_MAXTIME=1000 | The maximum time (in ms) a script in /etc/cont-finish.d could take before sending a KILL signal to it. Take into account that this parameter will be used per each script execution, it's not a max time for the whole set of scripts.
Copy link
Collaborator

Choose a reason for hiding this comment

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

@bryanlatten : Since this value is being set to 55000 should we change it from 1000 to that value?

@@ -0,0 +1,4 @@
#!/usr/bin/execlineb -P

foreground { nginx -s quit }
Copy link
Collaborator

Choose a reason for hiding this comment

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

@bryanlatten : Does it make sense to trap the exit code here somehow to see that it actually ended gracefully or do we feel that if the process nginx -s quit returns with a non-zero exit code, it'll just abort at that point and return? If so, then if that log message is missing the assumption is that it didn't die gracefully. I guess it'll also print something like

[cont-finish.d] executing container finish scripts...
[cont-finish.d] failed or something.

Does that sound right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adobejmong the default behavior of foreground is that the exit code is returned as the exit code of the script: https://www.skarnet.org/software/execline/foreground.html

you would see the message:
[cont-finish.d] script 00-nginx.sh exit code 1

@bryanlatten bryanlatten merged commit cddac88 into behance:master May 23, 2019
@bryanlatten bryanlatten deleted the cont-init-finish branch May 23, 2019 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants