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

other: Improve health check for docker-compose. #17320

Merged
merged 2 commits into from
Oct 4, 2022

Conversation

PMExtra
Copy link
Contributor

@PMExtra PMExtra commented Nov 2, 2021

For now, all services in docker-compose that using superset image will inherit the health check from the image. But the image is built for web app default.

So I made this change that using celery inspect ping for worker health check. And it also disabled health check for init and worker-beat services because they are always failed (unhealthy).

@PMExtra PMExtra changed the title Improve health check for docker-compose. other: Improve health check for docker-compose. Nov 4, 2021
@villebro villebro requested a review from dpgaspar November 25, 2021 09:20
Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM and change makes sense, but pinging Daniel for second opinion.

@kakoni
Copy link
Contributor

kakoni commented Mar 10, 2022

@PMExtra
For worker-beat one could do

    healthcheck:
      test: ["CMD-SHELL", "[ -f /proc/$$(cat /tmp/celerybeat.pid)/status ]"]

@PMExtra
Copy link
Contributor Author

PMExtra commented Mar 14, 2022

@kakoni I don't think so. It only checks if the process exists. But a health-check should check if the function effective.
And I think it make an unnecessary move. if the process is quit, the celery command should return and the container should be stopped.
What do you think?

@kakoni
Copy link
Contributor

kakoni commented Mar 14, 2022

@PMExtra Ah right, thats correct.

@dpgaspar
Copy link
Member

dpgaspar commented Oct 3, 2022

Sorry for the delay here, LGTM, can you plz solve current conflits @PMExtra ?

@rusackas rusackas merged commit f3f9f3b into apache:master Oct 4, 2022
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 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/S 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants