-
-
Notifications
You must be signed in to change notification settings - Fork 202
fix(s6): make web services wait for their config oneshots (fixes root-mode startup race) #689
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -77,4 +77,37 @@ for file in "$ENTRYPOINT_DIR"/*.sh; do | |||||||||||||||
| echo "Skipping ${script_name} because it already exists at ${S6_HOME}/scripts/${script_name}" | ||||||||||||||||
| fi | ||||||||||||||||
|
|
||||||||||||||||
| done | ||||||||||||||||
| done | ||||||||||||||||
|
|
||||||||||||||||
| # Make the long-running services wait for the entrypoint oneshots that configure | ||||||||||||||||
| # them. When the container runs as root, php-fpm and the web server otherwise | ||||||||||||||||
| # start in parallel with these oneshots and can lose the race: php-fpm reads the | ||||||||||||||||
| # pool before "5-fpm-pool-user" adds "user = www-data" (ALERT: [pool www] user | ||||||||||||||||
| # has not been defined -> FPM initialization failed), and the web server starts | ||||||||||||||||
| # before "10-init-webserver-config" renders its config. s6 restarts the crashed | ||||||||||||||||
| # services so the container recovers, but it produces alarming errors, a slower | ||||||||||||||||
| # start, and a brief window with no service. The entrypoint oneshots are chained | ||||||||||||||||
| # in alphabetical order, so depending on one transitively waits for all earlier | ||||||||||||||||
| # ones. Each dependency is only added when both the service and the oneshot exist. | ||||||||||||||||
| add_startup_dependency() { | ||||||||||||||||
| # $1 = long-running service that must wait, $2 = entrypoint oneshot it needs | ||||||||||||||||
| service_dir="${S6_HOME}/s6-rc.d/$1" | ||||||||||||||||
| [ -d "$service_dir" ] && [ -d "${S6_HOME}/s6-rc.d/$2" ] || return 0 | ||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe add an explanatory comment for this condition. |
||||||||||||||||
|
|
||||||||||||||||
| dependencies_file="${service_dir}/dependencies" | ||||||||||||||||
| [ -e "$dependencies_file" ] || : > "$dependencies_file" | ||||||||||||||||
|
Comment on lines
+97
to
+98
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
No need to check for file existence, |
||||||||||||||||
|
|
||||||||||||||||
| # Skip if the dependency is already declared | ||||||||||||||||
| grep -qxF "$2" "$dependencies_file" 2>/dev/null && return 0 | ||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
IMO CLI arguments should always be long form unless you are typing in a terminal. This makes it a lot easier to understand the command. |
||||||||||||||||
|
|
||||||||||||||||
| # Ensure existing content ends with a newline before appending (some shipped | ||||||||||||||||
| # dependency files, e.g. nginx's, have no trailing newline) | ||||||||||||||||
| if [ -s "$dependencies_file" ] && [ -n "$(tail -c 1 "$dependencies_file")" ]; then | ||||||||||||||||
| printf '\n' >> "$dependencies_file" | ||||||||||||||||
| fi | ||||||||||||||||
| printf '%s\n' "$2" >> "$dependencies_file" | ||||||||||||||||
|
Comment on lines
+103
to
+108
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
The file doesn't need to be pretty. If you need a newline before, just always output it. :-) |
||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| add_startup_dependency php-fpm 5-fpm-pool-user | ||||||||||||||||
| add_startup_dependency nginx 10-init-webserver-config | ||||||||||||||||
| add_startup_dependency apache2 10-init-webserver-config | ||||||||||||||||
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.
IMO this should be in Git/PR description only to keep the code compact.