fix: add CELERY_RESULT_BACKEND with default django-cache by navinkarkera · Pull Request #6720 · openedx-unsupported/configuration

navinkarkera · 2022-04-14T06:54:09Z

Configuration Pull Request

Note: This PR has a corresponding edx-platform PR openedx/openedx-platform#30244

Description

At Opencraft, we want to run multiple instances of celery workers in separate servers. The celery workers need to be able to share task results. Currently, the CELERY_RESULT_BACKEND is hard coded to use django-cache.

So this PR allows to setup CELERY_RESULT_BACKEND via configuration.

To use django ORM as celery backend, one can set CELERY_RESULT_BACKEND to django-db as described in docs.

Supporting information

Task: https://tasks.opencraft.com/browse/BB-6077

Deadline

"None"

openedx-webhooks · 2022-04-14T06:54:15Z

Thanks for the pull request, @navinkarkera! I've created OSPR-6621 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

natabene · 2022-04-26T15:26:23Z

@navinkarkera Thank you for your contribution. Is this ready for our review?

kaustavb12 · 2022-04-27T05:32:33Z

Hi @natabene !

This is not ready for your review yet. I'll ping you here once ready.

kaustavb12 · 2022-05-21T05:43:19Z

 CELERY_BROKER_VHOST: ''
 CELERY_EVENT_QUEUE_TTL: null
 CELERY_TIMEZONE: UTC
+CELERY_RESULT_BACKEND: django-cache


Why are we overriding this here for dockers ?

@kaustavb12 As per my understanding, these files contain all available settings for the deployment which user can override if required.

Also it is set to the default value django-cache. Let me know if we need to remove this.

@navinkarkera
From what I understand, these files are meant to override some of the settings from the playbooks, in context of docker. I think this would prevent anyone from changing the value to anything else using the ansible settings.
And we have setup default value to django-cache anyways.

@kaustavb12 We can remove it, but I am trying to get a clear picture of the settings here.

I can see that we have all kinds of settings in this file, for example: CELERY_BROKER_TRANSPORT: amqp. Does this mean we cannot override this in local docker setup as it is fixed here?

Also, we have an additional file specifically for overrides called ansible_overrides.yml in the same folder.

@navinkarkera Sorry for all the back and forth here, but I am trying to make sure all the changes we make here are justified.

As per my understanding, these files contain all available settings for the deployment which user can override if required.

I checked how the env settings work in the context of the devstack and you were right, these values are just initial defaults which can in-fact be overriden by both ansible and by the user as required.

I think this is how it works:

The lms.yml file provides base meaningful defaults for the docker setup which are first copied over to the container.

The ansible playbooks are then run, which overrides these vaules in the lms.yml file. Case in point, the CELERY_BROKER_TRANSPORT variable which you pointed out. The default value in lms.yml is amqp, but the default value in ansible is redis. The vaule for the same in devstack is also redis.

As you rightly pointed out, the values for overriding the defaults of the ansible are provided by ansible_overrides.yml along with other files like devstack_common_ansible_overrides.yml.

During runtime, these values are picked up by production.py and can in-turn be overridden by devstack.py.

In this context, setting this value here, does no harm but does not serve any purpose either, since we anyways have defaults setup in both ansible and in production.py.

Let me know what you think, or if I have missed understood the flow somewhere.

@kaustavb12

Sorry for all the back and forth here, but I am trying to make sure all the changes we make here are justified.

No need to be sorry, in fact it helps us clear our misunderstandings about these files.

In this context, setting this value here, does no harm but does not serve any purpose either, since we anyways have defaults setup in both ansible and in production.py.

I think it would be useful to keep it in these files as a way of documenting possible configuration fields for docker.
Let me know if you feel differently.

@navinkarkera

I think it would be useful to keep it in these files as a way of documenting possible configuration fields for docker.
Let me know if you feel differently.

We don't maintain such defaults list for documentation purposes, for the native install, and I personally don't see too many benefits of maintaining a redundent set of defaults for docker which get overwritten by ansible anyways.

However, since we are maintaining the defaults for other variables, I am approving this for now, and leaving it at your discretion if you want to retain it.

kaustavb12 · 2022-05-21T05:43:38Z

 CELERY_BROKER_VHOST: ''
 CELERY_EVENT_QUEUE_TTL: null
 CELERY_TIMEZONE: UTC
+CELERY_RESULT_BACKEND: django-cache


Same question.

kaustavb12 · 2022-05-22T11:35:50Z

 All notable changes to this project will be documented in this file.
 Add any new changes to the top (right below this line).

+ - 2022-04-14


I think this date needs to be bumped to when the PR will be merged.

@kaustavb12 So we need to update this after the PR is approved?

Ideally yes.

@navinkarkera On second thought could you please bump the dates now, as you might not get a chance to do so after its upstream approved.

Also please do so for the corresponding release branch PRs

kaustavb12 · 2022-05-26T06:50:17Z

👍

Looks good to go from my end.

I tested this: I tested the configuration in the PR sandbox
I read through the code
Includes documentation

navinkarkera · 2022-05-26T07:23:58Z

@natabene This is ready for your review.

natabene · 2022-05-26T17:19:12Z

@navinkarkera I kicked off the tests, let's see how they turn out.

navinkarkera · 2022-05-27T05:51:52Z

Thanks @natabene, Just updated the commit message to fix commit lint error.

navinkarkera · 2022-06-08T04:26:42Z

@natabene I resolved the conflicts and updated the MR. Please kick off the tests and review.

natabene · 2022-06-08T14:05:43Z

@navinkarkera Sure, it has been lined for a review with the appropriate team.

NIXKnight

LGTM 👍

NIXKnight · 2022-06-27T12:03:47Z

@navinkarkera could you resolve merge conflicts with master? I can't merge the PR without it.

NIXKnight · 2022-07-13T09:53:43Z

@kaustavb12 can you get the merge conflicts resolved so that I can merge it?

navinkarkera · 2022-07-13T10:09:07Z

@NIXKnight Sorry for the delay. I'll resolve them and update the MR.

navinkarkera · 2022-07-13T10:14:09Z

@NIXKnight Updated. Thanks!

openedx-webhooks · 2022-07-13T10:17:22Z

@navinkarkera 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Apr 14, 2022

This was referenced Apr 14, 2022

fix: add CELERY_RESULT_BACKEND with default django-cache #6721

Merged

fix: add CELERY_RESULT_BACKEND with default django-cache #6722

Merged

fix: add CELERY_RESULT_BACKEND with default django-cache open-craft/configuration#191

Merged

openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs triage labels Apr 26, 2022

kaustavb12 reviewed May 21, 2022

View reviewed changes

kaustavb12 reviewed May 22, 2022

View reviewed changes

navinkarkera force-pushed the navin/set-celery-result-backend branch from 0340325 to 6c0c175 Compare May 27, 2022 05:49

navinkarkera force-pushed the navin/set-celery-result-backend branch from 6c0c175 to 85c1327 Compare June 8, 2022 04:25

navinkarkera mentioned this pull request Jun 8, 2022

refactor: add CELERY_RESULT_BACKEND with default django-cache open-craft/configuration#197

Merged

navinkarkera force-pushed the navin/set-celery-result-backend branch from 85c1327 to 662469b Compare June 8, 2022 04:34

NIXKnight approved these changes Jun 27, 2022

View reviewed changes

refactor: add CELERY_RESULT_BACKEND with default django-cache

451eb73

navinkarkera force-pushed the navin/set-celery-result-backend branch from 662469b to 451eb73 Compare July 13, 2022 10:13

openedx-webhooks removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Jul 13, 2022

NIXKnight merged commit ffd68cd into openedx-unsupported:master Jul 13, 2022

Uh oh!

Conversation

navinkarkera commented Apr 14, 2022

Configuration Pull Request

Description

Supporting information

Deadline

Uh oh!

openedx-webhooks commented Apr 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

natabene commented Apr 26, 2022

Uh oh!

kaustavb12 commented Apr 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

navinkarkera May 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaustavb12 May 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaustavb12 May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaustavb12 May 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaustavb12 commented May 26, 2022

Uh oh!

navinkarkera commented May 26, 2022

Uh oh!

natabene commented May 26, 2022

Uh oh!

navinkarkera commented May 27, 2022

Uh oh!

navinkarkera commented Jun 8, 2022

Uh oh!

natabene commented Jun 8, 2022

Uh oh!

NIXKnight left a comment

Choose a reason for hiding this comment

Uh oh!

NIXKnight commented Jun 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NIXKnight commented Jul 13, 2022

Uh oh!

navinkarkera commented Jul 13, 2022

Uh oh!

navinkarkera commented Jul 13, 2022

Uh oh!

openedx-webhooks commented Jul 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

openedx-webhooks commented Apr 14, 2022 •

edited

Loading

navinkarkera May 23, 2022 •

edited

Loading

kaustavb12 May 23, 2022 •

edited

Loading

kaustavb12 May 24, 2022 •

edited

Loading

kaustavb12 May 26, 2022 •

edited

Loading

NIXKnight commented Jun 27, 2022 •

edited

Loading