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

[#1515] Remove worker container from local environments #1552

Merged
merged 11 commits into from
Oct 27, 2022

Conversation

seanpreston
Copy link
Contributor

@seanpreston seanpreston commented Oct 24, 2022

Closes #1515

Code Changes

  • Removes FIDES__EXECUTION__WORKER_ENABLED confg. var
  • Sets task_always_eager to True
  • Overrides task_always_eager to False for the unit tests
  • Moves worker out of docker-compose.yml and into docker-compose.worker.yml

Steps to Confirm

  • Run nox -s test_env
  • Login to the dashboard at localhost:3000
  • Approve the privacy request
  • Ensure execution succeeds

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

This change removes the need to run a worker container or subprocess locally. A more detailed description of these changes can be found inside this Confluence doc.

@seanpreston seanpreston requested a review from a team October 24, 2022 21:47
@seanpreston seanpreston changed the title Sp/1515/celery config Remove worker container from local environments Oct 24, 2022
@seanpreston seanpreston changed the title Remove worker container from local environments [#1515] Remove worker container from local environments Oct 24, 2022
@pattisdr
Copy link
Contributor

@seanpreston I can't get this to come up, I'm just trying to get nox -s dev -- postgres to run and it's getting stuck at the configure_db step and exiting out without specific errors - any tips?

 ⠿ Network fides_default               Created                                                                                                                0.0s
 ⠿ Container fides-fides-db-1          Healthy                                                                                                               22.3s
 ⠿ Container fides-postgres_example-1  Healthy                                                                                                               22.4s
 ⠿ Container fides-fides-1             Waiting  
2022-10-25 01:19:31.240 [INFO] (seed:load_default_taxonomy:263): INSERTED 56 data_category resource(s)
2022-10-25 01:19:31.542 [INFO] (seed:load_default_dsr_policies:107): Preparing to create default rules for the following Data Categories: ['user.biometric', 'user.biometric_health', 'user.browsing_history', 'user.childrens', 'user.contact', 'user.date_of_birth', 'user.demographic', 'user.device', 'user.gender', 'user.genetic', 'user.government_id', 'user.health_and_medical', 'user.job_title', 'user.location', 'user.media_consumption', 'user.name', 'user.non_specific_age', 'user.observed', 'user.organization', 'user.political_opinion', 'user.profiling', 'user.race', 'user.religious_belief', 'user.search_history', 'user.sensor', 'user.sexual_orientation', 'user.social', 'user.telemetry', 'user.unique_id', 'user.user_sensor', 'user.workplace']
2022-10-25 01:19:31.542 [INFO] (seed:load_default_dsr_policies:111): Creating: default_access_policy...
2022-10-25 01:19:31.549 [INFO] (seed:load_default_dsr_policies:123): Creating: default_local_storage...
2022-10-25 01:19:31.554 [INFO] (seed:load_default_dsr_policies:137): Creating: default_access_policy_rule...
2022-10-25 01:19:31.560 [INFO] (seed:load_default_dsr_policies:150): Creating: Data Category Access Rules...
2022-10-25 01:19:31.644 [INFO] (seed:load_default_dsr_policies:167): Creating: default_erasure_policy...
2022-10-25 01:19:31.648 [INFO] (seed:load_default_dsr_policies:179): Creating: default_erasure_policy_rule...
2022-10-25 01:19:31.652 [INFO] (seed:load_default_dsr_policies:195): Creating: Data Category Erasure Rules...
2022-10-25 01:19:31.734 [INFO] (seed:load_default_dsr_policies:211): All Policies & Rules Seeded.

@seanpreston
Copy link
Contributor Author

@seanpreston I can't get this to come up, I'm just trying to get nox -s dev -- postgres to run and it's getting stuck at the configure_db step and exiting out without specific errors - any tips?

This is still as issue. I was under the impression when I left it last night that this commit would have fixed the issue. Looks like I've some more diagnosing to do.

src/fides/api/main.py Outdated Show resolved Hide resolved
@ThomasLaPiana
Copy link
Contributor

@seanpreston is there any documentation now on how to use a worker? It seems like this completely tears out the worker, without a way to run it? i.e. the config option is no longer present

Should there be deployment docs updated around this?

@pattisdr
Copy link
Contributor

@seanpreston I think some of our imports need to be adjusted, I was trying to bring up the shell with nox -s dev -- shell postgres and couldn't populate the postgres db:

Attempting to create schema and seed initial data for postgres from tests/ops/integration_tests/setup_scripts/postgres_setup.py...
[+] Running 2/2
 ⠿ Container fides-redis-1     Recreated                                                                                                                                                                                           0.3s
 ⠿ Container fides-fides-db-1  Running                                                                                                                                                                                             0.0s
[+] Running 1/1
 ⠿ Container fides-redis-1  Started                                                                                                                                                                                                0.3s
/root/.local/lib/python3.10/site-packages/snowflake/connector/options.py:96: UserWarning: You have an incompatible version of 'pyarrow' installed (6.0.1), please install a version that adheres to: 'pyarrow<8.1.0,>=8.0.0; extra == "pandas"'
  warn_incompatible_dep(

Traceback (most recent call last):
  File "/fides/tests/ops/integration_tests/setup_scripts/postgres_setup.py", line 14, in <module>
    from tests.ops.test_helpers.db_utils import seed_postgres_data
ModuleNotFoundError: No module named 'tests'
no custom setup logic found for postgres, skipping
Opening bash shell on fides
[+] Running 2/0
 ⠿ Container fides-fides-db-1  Running                                                                                                                                                                                             0.0s
 ⠿ Container fides-redis-1     Running                                                                                                                                                                                             0.0s
root@d15af63c1363:/fides# 

@seanpreston
Copy link
Contributor Author

@seanpreston is there any documentation now on how to use a worker? It seems like this completely tears out the worker, without a way to run it? i.e. the config option is no longer present

Should there be deployment docs updated around this?

We've never had any deployment documentation for it, and I have this issue to cover writing that.

@pattisdr
Copy link
Contributor

@seanpreston I don't understand why we're removing the option to run a worker container locally altogether, it seems like there's a discrepancy between local development and how it might be run on prod. For example, locally I've run both one and two workers to narrow down concurrency issues, it seems like we won't have the flexibility to test this.

@seanpreston
Copy link
Contributor Author

@seanpreston I think some of our imports need to be adjusted, I was trying to bring up the shell with nox -s dev -- shell postgres and couldn't populate the postgres db:

I've spent some time testing this and don't have a fix yet. This is also an issue on the main branch however so I don't think this should block us merging. I've added a follow-up ticket for it.

@seanpreston I don't understand why we're removing the option to run a worker container locally altogether

We aren't, we're switching the default. A worker can always be added back to the setup by setting task_always_eager=False and including -f docker-compose.worker.yml in the docker compose path.

Copy link
Contributor

@pattisdr pattisdr left a comment

Choose a reason for hiding this comment

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

Verified running privacy requests locally both on the fides machine which is now the default or by running it in the worker by setting task_always_eager=False and including -f docker-compose.worker.yml in the docker compose path as @seanpreston pointed out.

As an aside, I do think we shouldn't be going to f string formatting, besides the performance improvements, we also can't mask PII if the string is f-string formatted.

@seanpreston
Copy link
Contributor Author

As an aside, I do think we shouldn't be going to f string formatting, besides the performance improvements, we also can't mask PII if the string is f-string formatted.

FWIW I totally agree here too. We'll need to make a call on the logging standard to move to now the repos are merged.

@ThomasLaPiana
Copy link
Contributor

As an aside, I do think we shouldn't be going to f string formatting, besides the performance improvements, we also can't mask PII if the string is f-string formatted.

FWIW I totally agree here too. We'll need to make a call on the logging standard to move to now the repos are merged.

I fully support not using f-strings for logging as stated earlier, % formatting seems to be objectively better in this case and in general

@ThomasLaPiana
Copy link
Contributor

@seanpreston if we're not running tests both with and without a worker, I won't believe that it works as intended 🙂

We can always only guarantee the configurations in which we're actually testing. Before we only tested with a worker, so could only guarantee that it ran ok with a worker. Now we've swung the other way, and can only guarantee that it works without a worker

Copy link
Contributor

@ThomasLaPiana ThomasLaPiana left a comment

Choose a reason for hiding this comment

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

Given the overall context, this is a good change and is being followed up by further iterations

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.

Improve local Celery config
3 participants