Superset worker has no lifecycle preStop command

[otunyk-smtc]

Bug description

I noticed that Superset worker is not gracefully shuts down when it is deployed to k8s, as a result a celery signal worker_shutdown is never triggered and tasks can be aborted without let them to complete. Also due to this issue workers status keeps as online while should be marked as offline on pod termination.
I tried both celery signals and SIGINT for bash and python and it is not catching the process termination.
Also I noticed that worker deployment (helm/superset/templates/deployment-worker.yaml) has no lifecycle preStop command. As soon as I added lifecycle preStop as this celery --app=superset.tasks.celery_app:app control shutdown --destination=celery@$HOSTNAME it solves the issue with gracefull shutdown.

Please add lifecycle preStop to the worker deployment-worker.yaml and/or values.yaml

Here is my patch I used with kubectl:

spec:
  template:
    spec:
      containers:
        - name: {{ .Chart.Name }}
          lifecycle:
            preStop:
              exec:
                command:
                  - /bin/sh
                  - '-c'
                  - celery --app=superset.tasks.celery_app:app control shutdown --destination=celery@$HOSTNAME

Screenshots/recordings

No response

Superset version

master / latest-dev

Python version

3.9

Node version

16

Browser

Chrome

Additional context

No response

Checklist

• I have searched Superset docs and Slack and didn't find a solution to my problem.
• I have searched the GitHub issue tracker and didn't find a similar bug report.
• I have checked Superset's logs for errors and if I found a relevant Python stacktrace, I included it here as text in the "additional context" section.

[rusackas]

@mistercrunch @eschutho not sure if you've run into this one. CC @villebro as the helm guru.

[mistercrunch]

I thought celery starts a graceful shutdown on the first SIGTERM/SIGINT it gets, but remember this being an issue in Airflow as long running jobs would prefer workers from shutting down in a decent amount of time. Clearly requires some orchestration.

It's beyond me as we have fine tuned our own k8s deployment at Preset and I'm unclear on how exactly with do it, but it would be great to contribute this fix to the community helm chart though.

[otunyk-smtc]

@mistercrunch You are correct, Celery does have a graceful shutdown on SIGTERM/SIGINT, and shutdown signal is triggered when I run it locally, but for some reason it is not invoked on pod termination in k8s and looks like it just kills it on pod termination without waiting for graceful shutdown. I also tried bash trap on SIGINT and it has the same behaviour, so it does not look like an issue with Celery itself. However if the lifecycle preStop command is set to celery --app=superset.tasks.celery_app:app control shutdown --destination=celery@$HOSTNAME then it takes terminationGracePeriodSeconds and shuts down celery worker correctly as it should be. It would be nice to have lifecycle preStop in superset helm/superset/templates/deployment-worker.yaml, it could be optional and configured/enabled in values.yaml, in this case I could avoid patching a deployment with lifecycle preStop command.

[mistercrunch]

Wondering if that's for everyone or could be something specific to your environment, curious if other users of the helm chart can confirm they see the same behavior. Curious if the issue is the same in say Minikube or EKS - but won't have time to test it personally.

[otunyk-smtc]

@mistercrunch

It does not look like something specific to my k8s because I found another opened issue mher/flower#1128 which is exactly what I faced with a superset helm deployment (superset+worker+flower).

In a few words the issue is next: After celery worker pod termination, the flower --purge_offline_workers metrics still shows terminated workers as online while on flower UI it shows as offline (most likely due to missing heartbeat from terminated celery worker, not due to proper worker shutdown).

After investigation I found that celery worker shutdown signal is never triggered and realized that graceful shutdown is not working properly on k8s. However when running a standalone celery worker locally the graceful shutdown works as expected.

So I added lifecycle.preStop command celery --app=superset.tasks.celery_app:app control shutdown --destination=celery@$HOSTNAME which does correct celery worker shutdown and both flower metrics and flower UI shows correct worker state as offline for terminated worker pods.

So I believe it is not a bug of celery or flower or even superset, it looks more like how k8s manages it when pod is terminated without lifecycle.preStop command.

It is also possible that Kubernetes sends SIGTERM to PID 1 in the container when terminating a pod, and SIGTERM is not propagated from entrypoint to celery worker master PID 7, if that is the case then it explains why celery worker graceful shutdown is not working as expected.

Does it make sense?

[rusackas]

@otunyk-smtc — would you be willing to open a PR for this? It sounds like you already have a working patch. Making it configurable via values.yaml (e.g., worker.lifecycle.preStop) with your command as the default would be the most community-friendly approach.

[mjlshen]

Just chiming in from essentially the same issue I found in the Superset webserver not gracefully shutting down - tl;dr this guess earlier from @otunyk-smtc was/is true:

It is also possible that Kubernetes sends SIGTERM to PID 1 in the container when terminating a pod, and SIGTERM is not propagated from entrypoint to celery worker master PID 7, if that is the case then it explains why celery worker graceful shutdown is not working as expected.

I have a fix for the webserver #41173 and worker #41175 to correctly use exec to elevate gunicorn/celery to PID 1 so that they receive SIGTERM correctly. I also included the ability to override lifecycle and gracefulTerminationPeriodSeconds moving forward so that larger setups can more fully customize how long SIGTERMs should take (gracefulTerminationPeriodSeconds defaults to 30s on Kubernetes)