Skip to content
This repository has been archived by the owner on Nov 30, 2022. It is now read-only.

Adds celery.toml for loading custom Celery config [#821] #865

Merged
merged 10 commits into from
Jul 15, 2022
Merged

Adds celery.toml for loading custom Celery config [#821] #865

merged 10 commits into from
Jul 15, 2022

Conversation

seanpreston
Copy link
Contributor

@seanpreston seanpreston commented Jul 12, 2022 •
edited
Loading

Purpose

This PR provides a way for a user to override any element of the Celery config. Originally this ticket was only to investigate overriding the event_queue_prefix var, however I would anticipate customising Celery config to be a relatively common use case given the variety of implementations.

Changes

  • Adds celery.toml to store celery config overrides based on the new lowercase settings.
  • Attempt to load in config from celery.toml when instantiating the celery_app on the worker.

Checklist

  • Update CHANGELOG.md file
    • Merge in main so the most recent CHANGELOG.md file is being appended to
    • Add description within the Unreleased section in an appropriate category. Add a new category from the list at the top of the file if the needed one isn't already there.
    • Add a link to this PR at the end of the description with the PR number as the text. example: #1
  • Applicable documentation updated (guides, quickstart, postman collections, tutorial, fidesdemo, database diagram.
  • If docs updated (select one):
    • 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)
  • Good unit test/integration test coverage
  • This PR contains a DB migration. If checked, the reviewer should confirm with the author that the down_revision correctly references the previous migration before merging
  • The Run Unsafe PR Checks label has been applied, and checks have passed, if this PR touches any external services

Ticket

Fixes #821

@seanpreston seanpreston changed the title [#821] configure EVENT_QUEUE_PREFIX for celery [#821] add celery.toml for loading custom Celery config Jul 13, 2022
@seanpreston seanpreston changed the title [#821] add celery.toml for loading custom Celery config Adds celery.toml for loading custom Celery config [#821] Jul 13, 2022
@seanpreston seanpreston marked this pull request as ready for review July 13, 2022 09:41
seanpreston
seanpreston commented Jul 13, 2022
View reviewed changes
data/config/celery.toml Outdated
@@ -0,0 +1,2 @@
event_queue_prefix = "fidesops_worker"
default_queue_name = "fidesops"
Copy link
Contributor Author

@seanpreston seanpreston Jul 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

A quirk of this implementation is that Celery will add any variable specified here to the config, not throw an error if the variable is not specified in the config spec. Celery then ignores the extra vars.

@seanpreston seanpreston mentioned this pull request Jul 13, 2022
Closed
seanpreston
seanpreston commented Jul 13, 2022
View reviewed changes
src/fidesops/core/config.py
@@ -39,8 +39,6 @@ class ExecutionSettings(FidesSettings):
TASK_RETRY_BACKOFF: int
REQUIRE_MANUAL_REQUEST_APPROVAL: bool = False
MASKING_STRICT: bool = True
CELERY_BROKER_URL: Optional[str] = None
CELERY_RESULT_BACKEND: Optional[str] = None
WORKER_ENABLED: bool = True
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've intentionally left WORKER_ENABLED in here, as whether the implementation is deploying a separate worker, and whether it's overriding the Celery config are two separate concerns.

@ThomasLaPiana
Copy link
Contributor

@seanpreston can you lowercase the new values so it matches with #871 ?

@seanpreston
Copy link
Contributor Author

@seanpreston can you lowercase the new values so it matches with #871 ?

@ThomasLaPiana if it's OK with you I'll do this in a follow-up, since I'd like to get this merged and into a minor release today, and we won't have the changes from #871 in that release.

@sanders41 sanders41 merged commit 8e106c2 into main Jul 15, 2022
@sanders41 sanders41 deleted the 821-event-queue-prefix branch July 15, 2022 09:44
@sanders41 sanders41 mentioned this pull request Jul 15, 2022
Open
sanders41 pushed a commit that referenced this pull request Sep 22, 2022
Adds celery.toml for loading custom Celery config [#821] (#865)
f6a09d8 
* adds option to configure EVENT_QUEUE_PREFIX for celery

* provide the option to specify a default queue name too

* update celery config to load in from its own config toml file

* updates changelog

* update value for event_queue_prefix

* test celery config overrides

* include config_path arg

* add type def

* add config path to execution settings

* correct values
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sanders41 sanders41 sanders41 approved these changes

Assignees

@sanders41 sanders41

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Spike] Support custom event_queue_prefix for Celery
3 participants
@seanpreston @ThomasLaPiana @sanders41