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

Forward SIGTERM signal to gunicorn #8156

Merged
merged 1 commit into from
Sep 2, 2019

Conversation

smacker
Copy link
Contributor

@smacker smacker commented Sep 2, 2019

CATEGORY

Choose one

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

Currently docker entrypoint is a bash script.
docker stop command (or restart in kubernetes) sends SIGTERM to the
container but it isn't catched by gunicorn.
So gunicorn can't start graceful shutdown which may lead to killing
user's connection and also adds 10s (default value) delay for stopping
until docker sends SIGKILL.
Using exec replaces the shell with the process being opened and
signals are propagated correctly.

Ref: https://docs.docker.com/engine/reference/commandline/stop/
Ref: https://unix.stackexchange.com/questions/146756/forward-sigterm-to-child-in-bash

TEST PLAN

Run superset in docker container and execute docker stop on master it would result in:

  1. 10s delay before stopping
  2. Exit code 137 (means gunicorn was killed)

Run superset in docker container and execute docker stop from this branch it would result in:

  1. Quick exit (~1s)
  2. Correct exit code 0

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

REVIEWERS

Currently docker entrypoint is a bash script.
`docker stop` command (or restart in kubernetes) sends SIGTERM to the
container but it isn't catched by gunicorn.
So gunicorn can't start graceful shutdown which may lead to killing
user's connection and also adds 10s (default value) delay for stopping
until docker sends SIGKILL.
Using `exec` replaces the shell with the process being opened and
signals are propagated correctly.

Ref: https://docs.docker.com/engine/reference/commandline/stop/
Ref: https://unix.stackexchange.com/questions/146756/forward-sigterm-to-child-in-bash
@codecov-io
Copy link

codecov-io commented Sep 2, 2019

Codecov Report

Merging #8156 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #8156   +/-   ##
======================================
  Coverage    66.2%   66.2%           
======================================
  Files         479     479           
  Lines       22936   22936           
  Branches     2524    2524           
======================================
  Hits        15184   15184           
  Misses       7618    7618           
  Partials      134     134

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2abec19...22e803a. Read the comment docs.

@mistercrunch mistercrunch merged commit 522e801 into apache:master Sep 2, 2019
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.35.0 labels Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XS 🚢 0.35.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants