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

Stop builders gracefully on SIGTERM #6960

Merged
merged 2 commits into from Apr 27, 2020
Merged

Conversation

humitos
Copy link
Member

@humitos humitos commented Apr 24, 2020

When SIGTERM is received by the process that is building a build, we only log a
message instead of passing it to the child processes (pip running inside the
Docker container, for example).

Otherwise, the pip process also receives the SIGTERM and it kills itself,
producing a exit_status != 0, registering this as a BuildCommand and making
the whole build to fail. After that, celery "stops gracefully" with a failed build.

This is useful combined with supervisor to make our builders to stop gracefully
in this scenario:

  1. supervisorctl stop build
  2. celery receives the SIGTERM and logs the warning message
  3. supervisor waits for stopwaitsecs before sending SIGKILL

@humitos humitos requested a review from Apr 24, 2020
Copy link
Member

@ericholscher ericholscher left a comment

Seems worth trying 👍


# Do not send the SIGTERM signal to childs
# (pip is automatically killed when receives SIGTERM and make the build to fail one command and stop build)
signal.signal(signal.SIGTERM, sigterm_received)
Copy link
Member

@ericholscher ericholscher Apr 24, 2020

Choose a reason for hiding this comment

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

I wonder where this code should live really, in this task seems resonable, but I wonder if there's a celery-level function that would make more sense.

Copy link
Member Author

@humitos humitos Apr 24, 2020

Choose a reason for hiding this comment

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

Yes. I had the same concern. I was thinking on readthedocs.worker but that's executed on normal runs as well.

Copy link
Member Author

@humitos humitos Apr 26, 2020

Choose a reason for hiding this comment

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

It seems there is a "correct" way of doing this via a Celery plugin. I just found https://github.com/MnogoByte/celery-graceful-stop that replace the SIGTERM handler for the workers as https://github.com/MnogoByte/celery-graceful-stop/blob/master/celery_graceful_stop/bootsteps.py#L35

I will keep reading to see if we can do something similar in a nicer way.

When SIGTERM is received by the process that is building a build, we only log a
message instead of passing it to the child processes (`pip` running inside the
Docker container, for example).

Otherwise, the `pip` process also receives the SIGTERM and it kills itself,
producing a `exit_status != 0`, registering this as a BuildCommand and making
the whole build to fail. After that, celery "stops gracefully" with a failed build.

This is useful combined with supervisor to make our builders to stop gracefully
in this scenario:

1. supervisorctl stop build
2. celery receives the SIGTERM and logs the warning message
3. supervisor waits for `stopwaitsecs` before sending SIGKILL
@humitos humitos force-pushed the humitos/stop-builders-gracefully branch from 6e49051 to 72db6f4 Compare Apr 24, 2020
Copy link
Member

@ericholscher ericholscher left a comment

I'm 👍 on giving this a try. It should only run on autoscaling and deploys, so seems same to experiment with.

readthedocs/projects/tasks.py Outdated Show resolved Hide resolved
@humitos humitos merged commit 7c036d3 into master Apr 27, 2020
2 checks passed
@humitos humitos deleted the humitos/stop-builders-gracefully branch Apr 27, 2020
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.

None yet

2 participants