Sending e-mail async with celery

[wes-otf]

This does a full implementation of celery for sending emails. The functionality was already partially implemented but wasn't properly configured to be used app-wide nor was it ever enabled.

Running locally

1. Get a Redis instance setup locally (I used docker, ie. docker run -d -p 6379:6379 redis)
2. Install requirements
3. Configuration
   • SEND_MESSAGES=True so emails will appear in terminal
   • CELERY_TASK_ALWAYS_EAGER=False so celery will add tasks to the broker rather than run it immediately
   • CELERY_BROKER_URL & CELERY_RESULT_BACKEND are properly set to your broker
4. In a separate terminal from the running make serve (but with same environment vars), run celery -A hypha.celery worker -E --loglevel=info to start celery

Test Steps

• Ensure that when Celery is configured emails are sending async
  • When running locally. they appear in the celery worker terminal

[wes-otf]

The tests were the most difficult part of this but I think the mocking makes sense. Seems like lots of these unit tests were acting more like integration tests & encompassing a lot more logic than it should.

I was also wondering if the auth/account creation email logic should be using the same send_mail function, as they needed to be tested separately in a few cases. Not sure if either of you have thoughts @frjo @theskumar

[frjo]

This looks clean and neat.

[frjo]

Deploying to test now.

I added "Heroku Key-Value Store" and made the following settings.

CELERY_BROKER_URL = "rediss://…"
CELERY_RESULT_BACKEND = "rediss://…"
CELERY_TASK_ALWAYS_EAGER = False

[frjo]

When adding "Heroku Key-Value Store" a setting REDIS_URL is automatically added and updated when needed. Since OTF deploy on Heroku I think it would be good to set CELERY_BROKER_URL and CELERY_RESULT_BACKEND to REDIS_URL if it exist. Maybe an extra setting CELERY_USE_REDIS_URL to control it.

[frjo]

Removed from test now. Need to figure some things out since messages are not getting sent.

The URLs need to look something like this:

CELERY_BROKER_URL = rediss://:longpasswordgoeshere@host.example.org:4321/0?ssl_cert_reqs=CERT_NONE
CELERY_RESULT_BACKEND = rediss://:longpasswordgoeshere@host.example.org:4321/1?ssl_cert_reqs=CERT_NONE

Without ssl_cert_reqs you get error A rediss:// URL must have parameter ssl_cert_reqs and this must be set to CERT_REQUIRED, CERT_OPTIONAL, or CERT_NONE.

I think we could set ssl_cert_reqs in app = Celery("hypha") as well. I think we should set it in code somehow and not add it directly in the settings since Herokus automatically generated REDIS_URL does not have it.

The /0 and /1 set the db table and pops up here and there in examples, needs a look into. IF needed these should also be added in code.

[frjo]

Added the "redis" packages in a separate commit to this PR, got error messages without it.

With CELERY_TASK_ALWAYS_EAGER = False Slack ("django_slack") will use Celery out of the box so we need to make sure that part works as well.

[frjo]

This is something we should have added to Hypha a long time ago. Nice work getting this off the ground @wes!

[wes-otf]

The URLs need to look something like this:

CELERY_BROKER_URL = rediss://:longpasswordgoeshere@host.example.org:4321/0?ssl_cert_reqs=CERT_NONE
CELERY_RESULT_BACKEND = rediss://:longpasswordgoeshere@host.example.org:4321/1?ssl_cert_reqs=CERT_NONE

Without ssl_cert_reqs you get error A rediss:// URL must have parameter ssl_cert_reqs and this must be set to CERT_REQUIRED, CERT_OPTIONAL, or CERT_NONE.

If we're not using TLS for the broker I think we use redis://... instead of rediss://..., but should we be using TLS? I'm not entirely familiar with heroku so not sure if any of those broker messages are going over the internet somewhere they could be intercepted

Added the "redis" packages in a separate commit to this PR, got error messages without it.

I must've left this out of my commit, thanks for adding it!

With CELERY_TASK_ALWAYS_EAGER = False Slack ("django_slack") will use Celery out of the box so we need to make sure that part works as well.

This is also good to know, I didn't realize that!

[frjo]

For Heroku it's all inside AWS but other implementors we have no control over. I think it is safer to always go with TLS.

[wes-otf]

@frjo I got TLS working with ssl_cert_reqs=CERT_NONE locally with this compose.yml & the following env variables:

• CELERY_BROKER_URL='rediss://localhost:6379/0?ssl_cert_reqs=CERT_NONE'
• CELERY_RESULT_BACKEND='rediss://localhost:6379?ssl_cert_reqs=CERT_NONE'
• CELERY_TASK_ALWAYS_EAGER=0

that docker setup requires Redis TLS certs to be generated in a directory located at tests/tls/ (relative to where docker compose up is being ran), directory & certs can be generated by running this script from Redis. Everything worked fine for me in this setup though so maybe we could redeploy on the test instance at some point so I could troubleshoot the other mentioned issues live on heroku

if we do add the cert config to the app = Celery("hypha") I think it'd be a good idea to require another env variable to specify cert req type so we don't usher any adopters into being vulnerable to a TLS MITM if they use the defaults with a different stack

[wes-otf]

I've been digging in this all week and can't figure out what is happening here - with the new base.py and REDIS_URL=rediss://localhost:6379 & REDIS_SSL_CERT_REQS=CERT_NONE set, the CELERY_BROKER_URL & CELERY_RESULT_BACKEND settings are identical to the previously mentioned env variables:

• CELERY_BROKER_URL='rediss://localhost:6379/0?ssl_cert_reqs=CERT_NONE'
• CELERY_RESULT_BACKEND='rediss://localhost:6379?ssl_cert_reqs=CERT_NONE'

and the celery worker connects to redis without error, but the tasks are just silently not passed to the queue. No errors and send_mail is called as expected but nothing happens after apply_async. Meanwhile, running these settings and manually setting env variables mentioned above (overriding the REDIS_URL setting of vars) causes everything to work perfect. Feeling close to a solution

[wes-otf]

not in love with this solution in general but it works - seems like celery is kinda weird and under-documented when it comes to configuration options, some folks in celery/celery#4284 had similar issues to what I was seeing.

open to any optimizations for this! but should fix our heroku config issues

[frjo]

No, not that pretty. But that it works is a clear benefit 👍 .

Perhaps add a comment about why it looks like it does.

[wes-otf]

done!

[frjo]

Deploying to test now.

[wes-otf]

@frjo I updated the Procfile to have a worker, along with the django_slack module in the celery app autodiscover because that was throwing an error. Seemed like the issue was there wasn't a worker to consume the tasks off the queue. Do we need to do the working scaling mentioned here?

[theskumar]

Considering the existing Sentry compatibility within this project, consider add Sentry for Celery ? @wes-otf @frjo

Ref: https://docs.sentry.io/platforms/python/integrations/celery/

Edit: Looks like Sentry integration should work out of the box!

If you're using Celery with Django in a typical setup, have initialized the SDK in your settings.py file (as described in the Django integration documentation), and have your Celery configured to use the same settings as config_from_object, there's no need to initialize the Celery SDK separately.

[wes-otf]

@theskumar I read that earlier, pretty sweet! Do you think we still need to add sentry[celery] to the requirements file?

[theskumar]

@theskumar I read that earlier, pretty sweet! Do you think we still need to add sentry[celery] to the requirements file?

No. Initially I was a little confused as the official docs say nothing is needed, but we are using DjangoIntegration and there are similar integrations for RedisIntegration and CeleryIntegration that can be added to sentry init. Digging deeper, it looks like explicit addition is needed only if more configuration options need to be passed.

[theskumar]

@wes-otf just a small thing, I believe it's a good time to update docs/getting-started/architecture.md as well.

[wes-otf]

@theskumar should be all set, I also redid the existing diagram with mermaid which is pretty cool! never used it before

[theskumar]

@theskumar should be all set, I also redid the existing diagram with mermaid which is pretty cool! never used it before

Looking great! I was thinking of doing the same.

Mermaid is really great for maintainable diagrams, but if you need something more complicated, concise, or fancy, it's better done custom.

[wes-otf]

@frjo anything left on this or are we ready to merge?