Move Celery check task to the high priority queue by kaizoku · Pull Request #23731 · openedx/openedx-platform

kaizoku · 2020-04-15T23:52:44Z

Currently, LMS uses 3 Celery workers: lms_default_1, lms_high_1, and lms_high_mem_1. Each Celery worker sends messages to a single queue: edx.core.default, edx.core.high and edx.core.high_mem, respectively.
The number of child processes per Celery worker is set to 1. Due to this configuration, any task in a queue blocks all other tasks.

Currently, the Celery check task submitted by the /heartbeat?extended LMS HTTP API endpoint runs in the default queue. When some slow task (eg course grades creation) is sent to the default queue, it will block the Celery check task, which will expire and the heartbeat endpointwill fail. This patch moves the task to another queue which has only shorter tasks and in which this problem will not occur.

Use a Django setting for the Celery check task routing key so that it can be overriden by individual OpenEDX instances via JSON env files.

JIRA tickets: Implements OSPR-4325

Sandbox URL: TBD - sandbox is being provisioned.

Testing instructions:

Load the heartbeat endpoint.
See that it runs in the high priority queue.

Reviewers

(@giovannicimolin)
edX reviewer[s] TBD

openedx-webhooks · 2020-04-15T23:52:53Z

Thanks for the pull request, @kaizoku! I've created OSPR-4388 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:

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 · 2020-04-16T01:43:24Z

@kaizoku Thank you for your contribution. Please let me know once it is ready for our review.

kaizoku · 2020-04-16T03:25:44Z

Thanks @natabene, this is ready for review.

natabene · 2020-04-16T14:08:55Z

@kaizoku Thanks for letting me know, I will queue it for our review.

giovannicimolin

👍

I tested this:

Checked that the task is registered in the high priority queue:

-> celery@edx.lms.core.high.edxapp-pr23731sandbo-appserver-1: OK
    * openedx.core.djangoapps.heartbeat.tasks.sample_task [routing_key=edx.lms.core.high]

I read through the code
~~I checked for accessibility issues~~ NA
~~Includes documentation~~ NA

@kaizoku Good to go!

@natabene I'll add this to the prioritization list.

kaizoku · 2020-05-04T09:09:53Z

@natabene has this PR come up for review yet?

natabene · 2020-05-05T01:09:55Z

@kaizoku No, not yet.

cf https://github.com/edx/edx-platform/pull/23731 Move Celery check task to the high priority queue Currently, LMS uses 3 Celery workers: lms_default_1, lms_high_1 and lms_high_mem_1. Each Celery worker sends messages to a single queue: edx.core.default, edx.core.high and edx.core.high_mem, respectively. The number of child processes per Celery worker is set to 1. Due to this configuration, any task in a queue blocks all other tasks. Currently, the Celery check task submitted by the /heartbeat?extended LMS HTTP API endpoint runs in the default queue. When some slow task (eg course grades creation) is sent to the default queue, it will block the Celery check task, which will expire and the heartbeat endpoint will fail. This patch moves the task to another queue which has only shorter tasks and in which this problem will not occur. Use a Django setting for the Celery check task routing key so that it can be overriden by individual OpenEDX instances via JSON env files. (cherry picked from commit eaf229556365889d66ac7c9baeb866df0d659e11)

natabene · 2020-09-08T18:51:11Z

jenkins run all

giovannicimolin · 2020-09-14T17:48:52Z

@kaizoku Can you rebase this one? It's been already approved.

natabene · 2020-09-14T18:01:16Z

@giovannicimolin Just FYI we will need one more approver, someone with edx-platform code ownership.

giovannicimolin · 2020-09-14T18:05:27Z

@natabene I though that @fredsmith had ownership here. Since he approved I assumed this was good to go.
We can wait another review, no problem.

natabene · 2020-09-14T18:20:10Z

@giovannicimolin Yeah, it was my bad. He confirmed he would like someone with edx-platform ownership to review, too.

cf https://github.com/edx/edx-platform/pull/23731 Move Celery check task to the high priority queue Currently, LMS uses 3 Celery workers: lms_default_1, lms_high_1 and lms_high_mem_1. Each Celery worker sends messages to a single queue: edx.core.default, edx.core.high and edx.core.high_mem, respectively. The number of child processes per Celery worker is set to 1. Due to this configuration, any task in a queue blocks all other tasks. Currently, the Celery check task submitted by the /heartbeat?extended LMS HTTP API endpoint runs in the default queue. When some slow task (eg course grades creation) is sent to the default queue, it will block the Celery check task, which will expire and the heartbeat endpoint will fail. This patch moves the task to another queue which has only shorter tasks and in which this problem will not occur. Use a Django setting for the Celery check task routing key so that it can be overriden by individual OpenEDX instances via JSON env files. (cherry picked from commit eaf229556365889d66ac7c9baeb866df0d659e11)

nasthagiri

Once the rename is complete and merge conflicts are corrected, we can go ahead and merge.

nasthagiri · 2020-09-21T02:17:44Z

Can you rename this setting to be consistent with the other related settings?
Suggestion: CELERY_CHECK_ROUTING_KEY -> HEARTBEAT_CELERY_ROUTING_KEY

Yes, I've updated this variable name to be more consistent with the others.

Currently, LMS uses 3 Celery workers: lms_default_1, lms_high_1 and lms_high_mem_1. Each Celery worker sends messages to a single queue: edx.core.default, edx.core.high and edx.core.high_mem, respectively. The number of child processes per Celery worker is set to 1. Due to this configuration, any task in a queue blocks all other tasks. Currently, the Celery check task submitted by the /heartbeat?extended LMS HTTP API endpoint runs in the default queue. When some slow task (eg course grades creation) is sent to the default queue, it will block the Celery check task, which will expire and the heartbeat endpoint will fail. This patch moves the task to another queue which has only shorter tasks and in which this problem will not occur. Use a Django setting for the Celery check task routing key so that it can be overriden by individual OpenEDX instances via JSON env files.

kaizoku

Thanks @nasthagiri, I've rebased against master and updated the variable name.
Is this otherwise good for merge now?

kaizoku · 2020-09-24T06:42:28Z

Yes, I've updated this variable name to be more consistent with the others.

nasthagiri · 2020-09-25T15:28:34Z

 TRACKING_SEGMENTIO_SOURCE_MAP = ENV_TOKENS.get("TRACKING_SEGMENTIO_SOURCE_MAP", TRACKING_SEGMENTIO_SOURCE_MAP)

+# Heartbeat
+HEARTBEAT_CELERY_ROUTING_KEY = ENV_TOKENS.get('HEARTBEAT_CELERY_ROUTING_KEY', HIGH_PRIORITY_QUEUE)


Can you update the default value here to be HEARTBEAT_CELERY_ROUTING_KEY? This way, (1) the value of HIGH_PRIORITY_QUEUE can be set only in a single place in common.py and (2) it supports other overrides along the way, like in private.py.

This will hopefully be the last change before merge.

Sure @nasthagiri, that's a good application of DRY and I've updated this for the lms and cms production.py.

edx-status-bot · 2020-09-26T05:32:38Z

Your PR has finished running tests. There were no failures.

nasthagiri · 2020-09-26T15:09:51Z

+)
+
+HEARTBEAT_CELERY_TIMEOUT = 5
+HEARTBEAT_CELERY_ROUTING_KEY = HIGH_PRIORITY_QUEUE


One more last thing (sorry), is there a reason you change the pre-existing default value? By default, the code had been using CELERY_DEFAULT_ROUTING_KEY for this (which defaults to the CELERY_DEFAULT_QUEUE). To be on the safe side (unless it was intentional), let's change the value of this to CELERY_DEFAULT_ROUTING_KEY.

No worries @nasthagiri, this change was intentional. The original intent behind this PR was to change the heartbeat queue to the high priority. We ran into problems when running grade reports or other long running tasks since all default priority workers were busy, so the celery heartbeat check would time out and trigger false positives in our monitoring infrastructure. Moving this heartbeat check to the high priority queue prevents that since there's generally a high priority worker available.

Got it. Thank you.

openedx-webhooks · 2020-09-29T10:37:10Z

@kaizoku 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

edx-pipeline-bot · 2020-09-29T11:29:06Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

edx-pipeline-bot · 2020-09-29T15:35:33Z

EdX Release Notice: This PR has been deployed to the production environment.

feanil · 2021-06-30T06:20:44Z

@kaizoku this PR introduced a bug because the default queue naming in common.py did not match what is set in production.py. I believe https://github.com/edx/edx-platform/pull/28066 should resolve the issue, please take a look.

common.py has queue names that always get overridden by production.py and lead to confusion. Set a default SERVICE_VANIANT in common.py and then set the queue names based on that in common.py so that production.py doesn't make it more complicated. This should prevent the issue where if you copy a queue name in common.py it ends up being incorrect in the production system. This is what happened with the sample_task change. https://github.com/edx/edx-platform/pull/23731 made it so that the queue name for that queue is independently configurable but the default was set to the value of HIGH_PRIORITY_QUEUE in common.py which is not the same as the value set in production.py leading to stale tasks that never get picked up in production. BREAKING_CHANGE: If anyone was building a different settings file on top of common, the default names in common.py are now change to be service variant specific. eg 'edx.cms.core.high' instead of 'edx.core.high'

kaizoku requested a review from a team April 15, 2020 23:52

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

kaizoku mentioned this pull request Apr 16, 2020

Move Celery check task to the high priority queue #23523

Closed

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

openedx-webhooks added awaiting prioritization and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Apr 16, 2020

giovannicimolin approved these changes Apr 16, 2020

View reviewed changes

nizarmah mentioned this pull request Jul 18, 2020

SE-2170 Port changes not yet merged to upstream into our juniper release branch open-craft/openedx-platform#225

Merged

1 task

fredsmith approved these changes Sep 8, 2020

View reviewed changes

nasthagiri reviewed Sep 21, 2020

View reviewed changes

openedx-webhooks added changes requested and removed awaiting prioritization labels Sep 21, 2020

kaizoku force-pushed the alanoe/move_celery_check_to_high_priority_queue_upstream branch from eaf2295 to 831da76 Compare September 24, 2020 06:42

kaizoku force-pushed the alanoe/move_celery_check_to_high_priority_queue_upstream branch from 831da76 to 6b9cba5 Compare September 24, 2020 07:04

kaizoku force-pushed the alanoe/move_celery_check_to_high_priority_queue_upstream branch from 6b9cba5 to b9559fc Compare September 24, 2020 08:27

kaizoku commented Sep 24, 2020

View reviewed changes

openedx-webhooks added engineering review and removed changes requested labels Sep 25, 2020

nasthagiri reviewed Sep 25, 2020

View reviewed changes

openedx-webhooks added changes requested and removed engineering review labels Sep 25, 2020

Update default for override to avoid setting duplication

73fd2ed

nasthagiri reviewed Sep 26, 2020

View reviewed changes

nasthagiri merged commit bc1d1bc into openedx:master Sep 29, 2020

openedx-webhooks added merged and removed changes requested labels Sep 29, 2020

pomegranited mentioned this pull request Jun 25, 2021

[FAL-2076] use high priority queue for celery heartbeat check #28034

Closed

feanil mentioned this pull request Jun 30, 2021

fix!: Make default queue names service variant specific. #28066

Merged

bradenmacdonald deleted the alanoe/move_celery_check_to_high_priority_queue_upstream branch January 12, 2022 02:11

Uh oh!

Conversation

kaizoku commented Apr 15, 2020 • edited by nasthagiri Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

natabene commented Apr 16, 2020

Uh oh!

kaizoku commented Apr 16, 2020

Uh oh!

natabene commented Apr 16, 2020

Uh oh!

giovannicimolin left a comment • 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!

kaizoku commented May 4, 2020

Uh oh!

natabene commented May 5, 2020

Uh oh!

natabene commented Sep 8, 2020

Uh oh!

giovannicimolin commented Sep 14, 2020

Uh oh!

natabene commented Sep 14, 2020

Uh oh!

giovannicimolin commented Sep 14, 2020

Uh oh!

natabene commented Sep 14, 2020

Uh oh!

nasthagiri left a comment

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!

kaizoku left a comment

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!

edx-status-bot commented Sep 26, 2020

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!

openedx-webhooks commented Sep 29, 2020

Uh oh!

edx-pipeline-bot commented Sep 29, 2020

Uh oh!

edx-pipeline-bot commented Sep 29, 2020

Uh oh!

feanil commented Jun 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

kaizoku commented Apr 15, 2020 •

edited by nasthagiri

Loading

openedx-webhooks commented Apr 15, 2020 •

edited

Loading

giovannicimolin left a comment •

edited

Loading