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

Fix ungraceful shutdown of NGINX #1501

Merged
merged 1 commit into from Jun 15, 2021
Merged

Fix ungraceful shutdown of NGINX #1501

merged 1 commit into from Jun 15, 2021

Conversation

pleshakov
Copy link
Contributor

To shutdown the IC container, the container runtime sends a signal
to it (SIGTERM by default). The IC handles the SIGTERM signal - after
receiving it, it will gracefully shutdown NGINX and exit.

The termination signal can be customized in the Dockerfile via
STOPSIGNAL. In nginxinc/docker-nginx@3fb70dd
the STOPSIGNAL in the NGINX base images was changed from SIGTERM to
SIGQUIT. Because the IC didn't register a handler for SIGQUIT, the IC
would terminate immediately with a stack dump. As a result, the NGINX
wasn't gracefully shutdown, meaning any inflight requests were aborted.

This commits explicitly configures STOPSIGNAL to SIGTERM in the
Dockerfiles, which overrides any value set in the base images. This
restores graceful shutdown of NGINX.

The bug only affected NGINX images, because NGINX Plus images use
different base images without configured STOPSIGNAL.

while not requred, I added STOPSIGNAL to build/DockerfileWithAppProtectForPlusForOpenShift for consistency

@pleshakov pleshakov added the bug An issue reporting a potential bug label Apr 2, 2021
@lucacome
Copy link
Member

lucacome commented Apr 2, 2021

Wouldn't it be better to handle the SIGQUIT too in the code, so we don't differ too much from the base image?

@pleshakov
Copy link
Contributor Author

@lucacome since we have control (in the Dockerfile) over what signal is sent to IC, I don't see a point of handling multiple signals, when handling SIGTERM is enough

@lucacome
Copy link
Member

lucacome commented Apr 5, 2021

You linked a document saying that not every runtime will respect the STOPSIGNAL, I don't see the harm of handling multiple signals directly in the code where we have control over things.

@pleshakov
Copy link
Contributor Author

@lucacome could you give an example when a runtime will send a SIGQUIT?

build/Dockerfile Outdated Show resolved Hide resolved
@pleshakov pleshakov requested a review from lucacome April 5, 2021 20:58
@soneillf5 soneillf5 self-requested a review April 6, 2021 17:13
@github-actions
Copy link

github-actions bot commented Jun 6, 2021

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the stale Pull requests/issues with no activity label Jun 6, 2021
@pleshakov pleshakov removed the stale Pull requests/issues with no activity label Jun 7, 2021
@soneillf5 soneillf5 removed their request for review June 14, 2021 08:28
@lucacome lucacome removed their request for review June 14, 2021 15:21
To shutdown the IC container, the container runtime sends a signal
to it (SIGTERM by default). The IC handles the SIGTERM signal - after
receiving it, it will gracefully shutdown NGINX and exit.

The termination signal can be customized in the Dockerfile via
STOPSIGNAL. In nginxinc/docker-nginx@3fb70dd
the STOPSIGNAL in the NGINX base images was changed from SIGTERM to
SIGQUIT. Because the IC didn't register a handler for SIGQUIT, the IC
would terminate immediately with a stack dump. As a result, the NGINX
wasn't gracefully shutdown, meaning any inflight requests were aborted.

This commits explicitly configures STOPSIGNAL to SIGTERM in the
Dockerfiles, which overrides any value set in the base images. This
restores graceful shutdown of NGINX.

The bug only affected NGINX images, because NGINX Plus images use
different base images without configured STOPSIGNAL.
@pleshakov
Copy link
Contributor Author

rebased against the master branch

@pleshakov pleshakov merged commit 030935e into master Jun 15, 2021
@pleshakov pleshakov deleted the fix-oss-panic branch June 15, 2021 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue reporting a potential bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants