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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define DB config earlier for django-celery-results, fix INSTALLED_APPS #291

Closed
wants to merge 1 commit into from

Conversation

juspence
Copy link
Collaborator

@juspence juspence commented Jul 31, 2023

@Elkasitu I found two possible issues, but I'm not sure if fixing them will actually make the monitoring email work:

  1. INSTALLED_APPS ordering was different compared to Corgi, and django_celery_beat was not present so all the django_celery_beat.models couldn't be imported.
  2. CELERY_RESULT_BACKEND was set to "django-db", but Django's DATABASES setting hadn't been defined yet, so maybe django-celery-results didn't know where to store each TaskResult. I'd expect this to raise an error instead of silently failing, but 馃し

@juspence juspence requested a review from Elkasitu July 31, 2023 15:09
@juspence juspence self-assigned this Jul 31, 2023
@juspence juspence force-pushed the add-monitoring-email branch 3 times, most recently from 01992fa to 0012250 Compare July 31, 2023 15:33
Copy link
Contributor

@Elkasitu Elkasitu left a comment

Choose a reason for hiding this comment

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

I am not convinced that either of these will change anything, as the task results seem to be working locally without django-celery-beat, with the current INSTALLED_APPS order and without the DATABASES setting being defined in the base settings.pyfile.

This seems to be a recurring error [1][2] and there doesn't seem to be a definitive solution, I don't have time for this right now either so let's leave it for Friday.

[1] celery/django-celery-results#19
[2] celery/django-celery-results#102

@juspence
Copy link
Collaborator Author

juspence commented Aug 4, 2023

The docs above mention this is usually a config error. I checked in stage after I manually ran the first email task (which should create its own TaskResult), but TaskResult.objects.count() was still 0.

I don't have a working local OSIDB env set up (and don't have time to fix this unfortunately), so in this PR I just tried to minimize config differences between OSIDB and Corgi (which works with this config).

I do see that the Celery script you run in Kubernetes is almost identical to the Corgi one, and I don't think the differences would explain this bug. However, it looks like in your script you use "-P gevent", but the Celery app config's worker_pool setting is still set to "prefork".

So it looks like either the settings in your run script aren't being applied, or they're being overridden somewhere else. I also see the result_backend is still set to a redis:// URL and not to "django-db" like we have in Corgi. Below shows current OSIDB settings in the Celery worker pod in stage:

>>> from config.celery import app
>>> app.conf.get("worker_pool")
'prefork'
>>> app.conf.get("broker_url")
'rediss://secret@redis:6379/'
>>> app.conf.get("result_backend")
'rediss://secret@redis:6379/'
>>> app.conf.get("result_backend_max_retries")
2
>>> app.conf.get("result_backend_always_retry")
True

Compare with Corgi:

>>> app.conf.get("broker_url")
'redis://redis:6379'
>>> app.conf.get("result_backend")
'django-db'

I can't guarantee this change will fix the issue, but in Corgi we do define the Django database settings before we tell Celery to use Django's DB for storing TaskResults. I'm not familiar enough with OSIDB to know how you apply Celery settings or what else might override the stuff in config/settings.py though.

@Elkasitu
Copy link
Contributor

Elkasitu commented Aug 7, 2023

Closed by #296

@Elkasitu Elkasitu closed this Aug 7, 2023
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.

None yet

2 participants