-
Notifications
You must be signed in to change notification settings - Fork 10
WIP: Feat: Add celery beat for scheduling new cronjobs #2813
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
Draft
susilnem
wants to merge
1
commit into
project/spark-integration
Choose a base branch
from
feature/setup-celery-beat
base: project/spark-integration
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -143,7 +143,19 @@ services: | |
| # For development only | ||
| celery: | ||
| <<: *base_server_setup | ||
| command: python manage.py run_celery_dev | ||
| restart: unless-stopped | ||
| command: ./misc/dev/run_worker.sh | ||
| healthcheck: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's add TODO here for healthcheck to upate after banjo-utils integration |
||
| test: ["CMD-SHELL", "celery -A main inspect ping -d celery@$$HOSTNAME || exit 1"] | ||
| interval: 30s | ||
| timeout: 10s | ||
| retries: 3 | ||
| start_period: 40s | ||
|
|
||
| celery-beat: | ||
| <<: *base_server_setup | ||
| restart: unless-stopped | ||
| command: ./misc/dev/run_worker_beat.sh | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's add TODO here for healthcheck to upate after banjo-utils integration |
||
|
|
||
| # ------------------ Helper CLI Commands | ||
| # Usage: `docker compose run --rm <service-name>` | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,97 @@ | ||
| # Cronjobs | ||
|
|
||
| There are **two** cronjob mechanisms. Use celery beat for new cronjobs. | ||
|
|
||
| A job belongs to one mechanism or the other, **never both** — | ||
| `SentryMonitor.validate_config()` asserts that the enum matches `values.yaml`, | ||
| so mixing them breaks it. | ||
|
|
||
| ## 1. Celery beat — use this for new cronjobs | ||
|
|
||
| Schedules are declared in [`main/cronjobs.py`](../main/cronjobs.py) and synced | ||
| into `django_celery_beat` `PeriodicTask` rows when beat starts. `SCHEDULES` is | ||
| the source of truth: remove an entry and its row is deleted on the next start. | ||
| Rows named `manual:*` are left alone, as an escape hatch for one-off tasks | ||
| created through the admin. | ||
|
|
||
| Adding one takes two files, with no helm change and no `cron_job_monitor` run: | ||
|
|
||
| **1. Write the task in `<app>/tasks.py`** | ||
|
|
||
| ```python | ||
| @shared_task(soft_time_limit=..., time_limit=...) | ||
| def my_new_job(): | ||
| with redis_lock(RedisLockKey.MY_NEW_JOB) as acquired: | ||
| if not acquired: | ||
| return | ||
| ... | ||
| ``` | ||
|
|
||
| - The lock matters: `CELERY_ACKS_LATE` is on, so a task can be redelivered to | ||
| another worker if the one running it dies. | ||
| - Time limits go **on the decorator**. `DatabaseScheduler` silently discards | ||
| `time_limit` / `soft_time_limit` from a schedule entry's options. | ||
| - Don't set `queue` here — it belongs in the schedule entry below. | ||
|
|
||
| **2. Add a `CronJob` entry to `SCHEDULES` in `main/cronjobs.py`** | ||
|
|
||
| ```python | ||
| "my_new_job": CronJob( | ||
| task="myapp.tasks.my_new_job", | ||
| schedule=TimeConstants.EVERY_DAY, | ||
| options=CronJobOption( | ||
| expire_seconds=TimeConstants.SECONDS_IN_A_DAY, | ||
| queue=CeleryQueue.cronjob.name, | ||
| ), | ||
| sentry_config=CronJobSentryConfig(max_runtime=10), | ||
| ), | ||
| ``` | ||
|
|
||
| `options` only supports the keys `ModelEntry._unpack_options` keeps — `queue`, | ||
| `exchange`, `routing_key`, `priority`, `headers`, `expire_seconds`. Anything | ||
| else is dropped without warning. `expire_seconds` stops a backlog accumulating | ||
| while workers are down. | ||
|
|
||
| Sentry cron monitoring is automatic, controlled by | ||
| `SENTRY_MONITOR_CELERY_BEAT_TASKS` (default on). `CronJobSentryConfig` sets each | ||
| job's grace period, max runtime and thresholds next to its schedule. | ||
|
|
||
| ### Queues | ||
|
|
||
| `CeleryQueue` in `main/cronjobs.py` declares which queues exist (`default`, | ||
| `heavy`, `cronjob`) and feeds `app.conf.task_queues`. A worker started without | ||
| `-Q` consumes all of them, which is the dev setup. | ||
|
|
||
| A queue that is routed to but not declared here is a black hole: the task is | ||
| accepted and then never consumed by anything. | ||
|
|
||
| ### Running locally | ||
|
|
||
| ```bash | ||
| docker-compose up celery celery-beat | ||
| ``` | ||
|
|
||
| Worker and beat entrypoints live in `misc/dev/`. | ||
|
|
||
| ### Not deployed yet | ||
|
|
||
| **Beat currently runs in local development only.** There is no beat Deployment | ||
| in `deploy/helm/`, so nothing in `SCHEDULES` fires in alpha/staging/prod until | ||
| one is added. Still to do: | ||
|
|
||
| - A beat Deployment with **`replicas: 1`** and `strategy: Recreate` — two beat | ||
| processes fire every cronjob twice — plus a `celeryBeat` block in | ||
| `values.yaml`. It needs the same `envFrom` secret + configmap as the celery | ||
| worker. | ||
| - Beat needs the `django_celery_beat` tables, which `manage.py migrate` creates | ||
| on the API pod. If beat starts first it crashloops until migrations have run. | ||
|
|
||
| ## 2. Kubernetes CronJobs — the legacy set | ||
|
|
||
| The pre-existing cronjobs run as k8s CronJob resources listed under `cronjobs:` | ||
| in `deploy/helm/ifrcgo-helm/values.yaml`, one pod per run, monitored via | ||
| `SentryMonitor` in `main/sentry.py`. Their Sentry monitors are registered with: | ||
|
|
||
| ```bash | ||
| docker-compose exec serve bash ./manage.py cron_job_monitor | ||
| ``` |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, https://docs.djangoproject.com/en/6.0/topics/http/sessions/#clearing-the-session-store
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it would be effective only for the admin panel users. right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes sir