Make `worker` node optional #770

seanpreston · 2022-06-30T22:25:34Z

Purpose

We've recently added Celery as a project dependency, and it's currently compulsory to run a worker in order to execute privacy requests. This PR makes that optional, by providing an option to run the worker on the same node as the webserver as a subprocess.

Changes

Moves worker definition into docker-compose.worker.yml
Adds USE_DEDICATED_WORKER config var
If USE_DEDICATED_WORKER is False -> run fidesops worker as a subprocess on webserver startup

Effect

Running docker compose up will bring up the webserver, and run fidesops worker as a subprocess on that container
Running docker compose -f docker-compose.yml -f docker-compose.worker.yml will force USE_DEDICATED_WORKER to True and bring up the worker container separately

Checklist

Ticket

Fixes #769

seanpreston · 2022-06-30T22:28:06Z

CHANGELOG.md

@@ -27,6 +27,8 @@ The types of changes are:
 * Parallelize CI safe checks to reduce run time [#717](https://github.com/ethyca/fidesops/pull/717)
 * Add dependabot to keep dependencies up to date [#718](https://github.com/ethyca/fidesops/pull/718)



Have left a blank space here in anticipation of a conflict with this PR

seanpreston · 2022-06-30T22:32:18Z

src/fidesops/main.py

@@ -172,6 +174,10 @@ def start_webserver() -> None:
        )
    )

+    if not config.execution.USE_DEDICATED_WORKER:
+        logger.info("Starting worker...")
+        subprocess.Popen(["fidesops", "worker"])


There may well be a smarter way to run this on the same container. For instance this wouldn't be any more performant than Fidesops prior to Celery if this was run outside of a high availability environment, as the same server is doing the work albeit in a slightly different structure. The main benefits of this are:

giving users the option to run everything with as small container footprint as possible

saving developers time and memory building + operating the worker container to test the privacy request flow locally

I'd be interested to test whether running the privacy request blocks the uvicorn server receiving requests in a deployed setting.

src/fidesops/core/config.py

seanpreston · 2022-06-30T22:45:10Z

docs/fidesops/docs/guides/configuration_reference.md

@@ -98,6 +101,10 @@ TASK_RETRY_DELAY=20
 TASK_RETRY_BACKOFF=2
 REQUIRE_MANUAL_REQUEST_APPROVAL=true
 MASKING_STRICT=true
+CELERY_BROKER_URL="redis://:testpassword@redis:6379/1"
+CELERY_RESULT_BACKEND="redis://:testpassword@redis:6379/1"


I've a separate task out to make these defaults more sane

seanpreston · 2022-06-30T22:45:19Z

docs/fidesops/docs/guides/configuration_reference.md

@@ -58,6 +58,9 @@ The `fidesops.toml` file should specify the following variables:
 |`TASK_RETRY_BACKOFF` | `FIDESOPS__EXECUTION__TASK_RETRY_BACKOFF` | int | 2 | 1 | The backoff factor for retries, to space out repeated retries.
 |`REQUIRE_MANUAL_REQUEST_APPROVAL` | `FIDESOPS__EXECUTION__REQUIRE_MANUAL_REQUEST_APPROVAL` | bool | False | False | Whether privacy requests require explicit approval to execute
 |`MASKING_STRICT` | `FIDESOPS__EXECUTION__MASKING_STRICT` | bool | True | True | If MASKING_STRICT is True, we only use "update" requests to mask data. (For third-party integrations, you should define an `update` endpoint to use.)  If MASKING_STRICT is False, you are allowing fidesops to use any defined DELETE or GDPR DELETE endpoints to remove PII. In this case, you should define `delete` or `data_protection_request` endpoints for your third-party integrations.  Note that setting MASKING_STRICT to False means that data may be deleted beyond the specific data categories that you've configured in your Policy.
+|`CELERY_BROKER_URL` | `FIDESOPS__EXECUTION__CELERY_BROKER_URL` | str | redis://:testpassword@redis:6379/1 | N/A | The datastore to maintain ordered queues of tasks.
+|`CELERY_RESULT_BACKEND` | `FIDESOPS__EXECUTION__RESULT_BACKEND` | str | redis://:testpassword@redis:6379/1 | N/A | The datastore to put results from asynchronously processed tasks.
+|`WORKER_ENABLED` | `FIDESOPS__EXECUTION__WORKER_ENABLED` | bool | True | True | Whether Fidesops is running with a dedicated worker to process privacy requests asynchronously.


These descriptions will undoubtedly need work

This should be fine for right now - they can be expanded on with #755

eastandwestwind · 2022-07-06T15:28:33Z

src/fidesops/core/config.py

@@ -41,6 +41,7 @@ class ExecutionSettings(FidesSettings):
    MASKING_STRICT: bool = True
    CELERY_BROKER_URL: str = "redis://:testpassword@redis:6379/1"
    CELERY_RESULT_BACKEND: str = "redis://:testpassword@redis:6379/1"
+    WORKER_ENABLED: bool = True


Is there a particular reason why we default our users to True but use false for ourselves? - https://github.com/ethyca/fidesops/pull/770/files#diff-b9a882b5033bf73236083b237701d90db30d7405950cbba9cfee37df20bdb191R37

They're generally deploying Fidesops in production, so should be encouraged to use a robust setup. We're using Fidesops in a development context and might not need to focus on the execution of privacy requests always, and it's mainly important when execution is happening.

eastandwestwind · 2022-07-06T16:12:04Z

Doing some thinking about workflow here, a user would need to both set FIDESOPS__EXECUTION__WORKER_ENABLED=True and use the make server-and-worker command, if they wished to run Celery on a separate worker.

This is fine for now, but I'd like to be able to detect the FIDESOPS__EXECUTION__WORKER_ENABLED from make server, so that the user experience is simpler.

We talked briefly about passing the env var around in Makefile, but I'm unsure if we specifically addressed the following: We could do something similar to how we call run_infrastructure.py for other Make commands, and pass in our WORKER_ENABLED env variable. If enabled, we can then bring up docker-compose and docker-compose.worker. What do you think? Should we create a ticket for this?

seanpreston · 2022-07-06T16:17:23Z

We talked briefly about passing the env var around in Makefile, but I'm unsure if we specifically addressed the following: We could do something similar to how we call run_infrastructure.py for other Make commands, and pass in our WORKER_ENABLED env variable. If enabled, we can then bring up docker-compose and docker-compose.worker. What do you think? Should we create a ticket for this?

I think they're two separate concerns. It's good for us to be able to spin up workers as and when we need to work with them locally, but it doesn't need to be part of the default setup as long as we can guarantee the same code is being run and it's only the paradigm that changes. Ideally I'd like to see if this does get in our way first and then address via the current change management avenues (retros etc).

eastandwestwind · 2022-07-06T16:22:18Z

@seanpreston sounds good to me, let's make sure we at least explain steps needed in the docs issue- #755

* default fidesops to running the worker and webserver on same container * default to using a worker, add docker config for worker * update changelog * USE_DEDICATED_WORKER -> WORKER_ENABLED * add basic descriptions for celery vars to docs * remove unused import * add Make command for a server + worker

Sean Preston added 3 commits June 30, 2022 18:22

default fidesops to running the worker and webserver on same container

57df62e

default to using a worker, add docker config for worker

eac642b

update changelog

2784bb4

seanpreston commented Jun 30, 2022

View reviewed changes

NevilleS reviewed Jun 30, 2022

View reviewed changes

src/fidesops/core/config.py Outdated Show resolved Hide resolved

Sean Preston added 2 commits June 30, 2022 18:44

USE_DEDICATED_WORKER -> WORKER_ENABLED

6735954

add basic descriptions for celery vars to docs

503ce33

seanpreston commented Jun 30, 2022

View reviewed changes

remove unused import

66ce348

seanpreston assigned TheAndrewJackson Jul 1, 2022

seanpreston requested a review from conceptualshark July 1, 2022 15:07

seanpreston assigned eastandwestwind and unassigned TheAndrewJackson Jul 6, 2022

eastandwestwind reviewed Jul 6, 2022

View reviewed changes

add Make command for a server + worker

3fdc6b6

eastandwestwind approved these changes Jul 6, 2022

View reviewed changes

Merge branch 'main' into optional-worker-config

483d535

seanpreston merged commit f493e55 into main Jul 6, 2022

seanpreston deleted the optional-worker-config branch July 6, 2022 17:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `worker` node optional #770

Make `worker` node optional #770

seanpreston commented Jun 30, 2022 •

edited

seanpreston Jun 30, 2022

seanpreston Jun 30, 2022 •

edited

seanpreston Jun 30, 2022 •

edited

seanpreston Jun 30, 2022

conceptualshark Jul 5, 2022

eastandwestwind Jul 6, 2022 •

edited

seanpreston Jul 6, 2022

eastandwestwind commented Jul 6, 2022 •

edited

seanpreston commented Jul 6, 2022

eastandwestwind commented Jul 6, 2022

		@@ -27,6 +27,8 @@ The types of changes are:
		* Parallelize CI safe checks to reduce run time [#717](https://github.com/ethyca/fidesops/pull/717)
		* Add dependabot to keep dependencies up to date [#718](https://github.com/ethyca/fidesops/pull/718)

Make worker node optional #770

Make worker node optional #770

Conversation

seanpreston commented Jun 30, 2022 • edited

Purpose

Changes

Effect

Checklist

Ticket

seanpreston Jun 30, 2022

Choose a reason for hiding this comment

seanpreston Jun 30, 2022 • edited

Choose a reason for hiding this comment

seanpreston Jun 30, 2022 • edited

Choose a reason for hiding this comment

seanpreston Jun 30, 2022

Choose a reason for hiding this comment

conceptualshark Jul 5, 2022

Choose a reason for hiding this comment

eastandwestwind Jul 6, 2022 • edited

Choose a reason for hiding this comment

seanpreston Jul 6, 2022

Choose a reason for hiding this comment

eastandwestwind commented Jul 6, 2022 • edited

seanpreston commented Jul 6, 2022

eastandwestwind commented Jul 6, 2022

Make `worker` node optional #770

Make `worker` node optional #770

seanpreston commented Jun 30, 2022 •

edited

seanpreston Jun 30, 2022 •

edited

seanpreston Jun 30, 2022 •

edited

eastandwestwind Jul 6, 2022 •

edited

eastandwestwind commented Jul 6, 2022 •

edited