Skip to content
This repository was archived by the owner on May 6, 2024. It is now read-only.

fix: add CELERY_RESULT_BACKEND with default django-cache #6720

Merged
NIXKnight merged 1 commit into from
Jul 13, 2022
Merged

fix: add CELERY_RESULT_BACKEND with default django-cache #6720

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,11 @@
All notable changes to this project will be documented in this file.
Add any new changes to the top (right below this line).

- 2022-06-08
- Role: edxapp
- Added a new `CELERY_RESULT_BACKEND` setting to allow operators to
override the default celery result backend.

- 2022-06-06
- Role: common
- Remove PPA for `watchman` as we have shifted to Ubuntu 20.04,
Expand Down
1 change: 1 addition & 0 deletions docker/build/edxapp/lms.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ CELERY_BROKER_USE_SSL: false
CELERY_BROKER_VHOST: ''
CELERY_EVENT_QUEUE_TTL: null
CELERY_TIMEZONE: UTC
CELERY_RESULT_BACKEND: django-cache

@kaustavb12 kaustavb12 May 21, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why are we overriding this here for dockers ?

@navinkarkera navinkarkera May 23, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@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.

@kaustavb12 kaustavb12 May 23, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@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.

@navinkarkera navinkarkera May 23, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@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.

@kaustavb12 kaustavb12 May 24, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@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:

  1. The lms.yml file provides base meaningful defaults for the docker setup which are first copied over to the container.
  2. 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.
  3. 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.
  4. 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.

@navinkarkera navinkarkera May 24, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@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.

@kaustavb12 kaustavb12 May 26, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@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.

CERTIFICATE_TEMPLATE_LANGUAGES:
en: English
es: Español
Expand Down
1 change: 1 addition & 0 deletions docker/build/edxapp/studio.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ CELERY_BROKER_USE_SSL: false
CELERY_BROKER_VHOST: ''
CELERY_EVENT_QUEUE_TTL: null
CELERY_TIMEZONE: UTC
CELERY_RESULT_BACKEND: django-cache

@kaustavb12 kaustavb12 May 21, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same question.

CERTIFICATE_TEMPLATE_LANGUAGES:
en: English
es: Español
Expand Down
2 changes: 2 additions & 0 deletions playbooks/roles/edxapp/defaults/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ EDXAPP_CELERY_BROKER_USE_SSL: false
EDXAPP_CELERY_EVENT_QUEUE_TTL: !!null
EDXAPP_CELERY_TIMEZONE: "UTC"
EDXAPP_CELERYBEAT_SCHEDULER: "celery.beat:PersistentScheduler"
EDXAPP_CELERY_RESULT_BACKEND: "django-cache"
EDXAPP_ENABLE_CELERY_BEAT: false
EDXAPP_SINGLE_BEAT_LOCK_TIME: 60
# EDXAPP_SINGLE_BEAT_HEARTBEAT_INTERVAL must be smaller than EDXAPP_SINGLE_BEAT_LOCK_TIME / 2
Expand Down Expand Up @@ -1345,6 +1346,7 @@ generic_env_config: &edxapp_generic_env
CELERY_BROKER_VHOST: "{{ EDXAPP_CELERY_BROKER_VHOST }}"
CELERY_BROKER_USE_SSL: "{{ EDXAPP_CELERY_BROKER_USE_SSL }}"
CELERY_EVENT_QUEUE_TTL: "{{ EDXAPP_CELERY_EVENT_QUEUE_TTL }}"
CELERY_RESULT_BACKEND: "{{ EDXAPP_CELERY_RESULT_BACKEND }}"
PAYMENT_SUPPORT_EMAIL: "{{ EDXAPP_PAYMENT_SUPPORT_EMAIL }}"
ZENDESK_URL: "{{ EDXAPP_ZENDESK_URL }}"
ZENDESK_CUSTOM_FIELDS: "{{ EDXAPP_ZENDESK_CUSTOM_FIELDS }}"
Expand Down