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

[#1587] Adds connected worker info. to the healthcheck #1588

Merged
merged 16 commits into from
Nov 1, 2022

Conversation

seanpreston
Copy link
Contributor

@seanpreston seanpreston commented Oct 28, 2022

Closes #1587

Code Changes

  • Adds "workers": {...} to the healthcheck response to reflect connectivity of any workers connected to the Celery backend
  • move the worker service back into the main compose file so that it's easier to spin things up
  • add a -- worker posarg to the dev command that spins up the worker with the application

Steps to Confirm

Without workers configured

{"webserver":"healthy","version":"1.9.2+84.ge1dc3cb27.dirty","database":"healthy","cache":"healthy","workers_enabled":"False","workers":[]}

With workers configured and healthy

{"webserver":"healthy","version":"1.9.2+84.ge1dc3cb27.dirty","database":"healthy","cache":"healthy","workers_enabled":"True","workers":["celery@0729b9943a81"]}

With workers configured but not healthy (manually kill the worker container)

{"webserver":"healthy","version":"1.9.2+84.ge1dc3cb27.dirty","database":"healthy","cache":"healthy","workers_enabled":"True","workers":[]}

Pre-Merge Checklist

  • All CI Pipelines Succeeded
  • Documentation Updated:
    • documentation complete, or draft/outline provided (tag docs-team to complete/review on this branch)
    • documentation issue created (tag docs-team to complete issue separately)
  • Issue Requirements are Met
  • Relevant Follow-Up Issues Created
  • Update CHANGELOG.md

Description Of Changes

Right now this solution will:

  • return connected workers — unhealthy workers will not be shown in an unhealthy state, but omitted from results entirely.
    • The reason for this is that Fides can't know if a worker exists (let alone its health status) if it is not connected to the backend (Redis).
    • To obtain the health status of an individual worker, Individual workers can be pinged at /health for an accurate healthcheck of that container.
  • return the standard .ping() response of {"ok": "pong"}. Alternatively there's a .stats() inspect method which could be used to show: pid of the worker on the container it's running on, uptime of the process, pool data, backend connection details — this seemed like overkill in the context of the other checks returning a simple "healthy".

@ThomasLaPiana
Copy link
Contributor

@seanpreston please note that I made an additional change here! I updated the description and the steps to confirm so please double check my work too 🙏

@seanpreston seanpreston changed the title adds connected worker info. to the healthcheck [#1587] adds connected worker info. to the healthcheck Oct 28, 2022
@seanpreston seanpreston changed the title [#1587] adds connected worker info. to the healthcheck [#1587] Adds connected worker info. to the healthcheck Oct 28, 2022
@ThomasLaPiana
Copy link
Contributor

@seanpreston woops, I also realized there's no test for this either

@ThomasLaPiana ThomasLaPiana self-requested a review October 30, 2022 18:28
@ThomasLaPiana
Copy link
Contributor

ThomasLaPiana commented Oct 30, 2022

@seanpreston can you update the health endpoint tests for this so its more clear what to expect the responses to look like? (tbf I think there is a single one

Edit: nevermind I'm up, i'll update the test real quick

@ThomasLaPiana ThomasLaPiana self-assigned this Oct 30, 2022
@ThomasLaPiana
Copy link
Contributor

ThomasLaPiana commented Oct 30, 2022

@seanpreston given the logic that unhealthy workers aren't shown, why show "ok"? I'm updating the code here but feel free to revert if you think it isn't more intuitive

.fides/celery.toml Outdated Show resolved Hide resolved
@seanpreston seanpreston merged commit 4055cda into main Nov 1, 2022
@seanpreston seanpreston deleted the sp/1587/worker-healthcheck branch November 1, 2022 18:50
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.

Add healthcheck to Celery
2 participants