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

[pomegranited]

Description

This change fixes the default celery queue used for heartbeat checks in production, setting the default to the high-priority queue for LMS/Studio, respectively.

Currently, the HEARTBEAT_CELERY_ROUTING_KEY defaults to the HIGH_PRIORITY_QUEUE set in lms.env.common, which is a generic 'edx.core.high' queue name.

Unfortunately, since both the LMS and Studio env.production settings files override HIGH_PRIORITY_QUEUE to be 'edx.lms.core.high' (and edx.cms.core.high respectively) before defining the list of CELERY_QUEUES, the 'edx.core.high' queue never gets created, and so the celery heartbeat check fails.

This change marks HEARTBEAT_CELERY_ROUTING_KEY as a derived setting, so that it can be lazily defaulted to the service variant's HIGH_PRIORITY_QUEUE.

Native default master settings (without this change)

# LMS
$ /edx/bin/edxapp-shell-lms 
>>> from django.conf import settings
>>> settings.HEARTBEAT_CELERY_ROUTING_KEY
'edx.core.high'
>>> settings.HIGH_PRIORITY_QUEUE
'edx.lms.core.high'
>>> settings.CELERY_QUEUES
{'edx.lms.core.default': {}, 'edx.lms.core.high': {}, 'edx.lms.core.high_mem': {}, 'edx.cms.core.default': {}}
>>> assert settings.HEARTBEAT_CELERY_ROUTING_KEY in settings.CELERY_QUEUES
Traceback (most recent call last):
  File "<console>", line 1, in <module>
AssertionError

# Studio
$ /edx/bin/edxapp-shell-cms 
>>> from django.conf import settings
>>> settings.HEARTBEAT_CELERY_ROUTING_KEY
'edx.core.high'
>>> settings.HIGH_PRIORITY_QUEUE
'edx.cms.core.high'
>>> settings.CELERY_QUEUES
{'edx.cms.core.default': {}, 'edx.cms.core.high': {}, 'edx.lms.core.default': {}}
>>> assert settings.HEARTBEAT_CELERY_ROUTING_KEY in settings.CELERY_QUEUES
Traceback (most recent call last):
  File "<console>", line 1, in <module>
AssertionError

Native default master settings (with this change)

# LMS
$ /edx/bin/edxapp-shell-lms 
>>> from django.conf import settings
>>> settings.HEARTBEAT_CELERY_ROUTING_KEY
'edx.lms.core.high'
>>> settings.HIGH_PRIORITY_QUEUE
'edx.lms.core.high'
>>> settings.CELERY_QUEUES
{'edx.lms.core.default': {}, 'edx.lms.core.high': {}, 'edx.lms.core.high_mem': {}, 'edx.cms.core.default': {}}
>>> assert settings.HEARTBEAT_CELERY_ROUTING_KEY in settings.CELERY_QUEUES
>>>

# Studio
$ /edx/bin/edxapp-shell-cms 
>>> from django.conf import settings
>>> settings.HEARTBEAT_CELERY_ROUTING_KEY
'edx.cms.core.high'
>>> settings.HIGH_PRIORITY_QUEUE
'edx.cms.core.high'
>>> settings.CELERY_QUEUES
{'edx.cms.core.default': {}, 'edx.cms.core.high': {}, 'edx.lms.core.default': {}}
>>> assert settings.HEARTBEAT_CELERY_ROUTING_KEY in settings.CELERY_QUEUES
>>>

Supporting information

This issue was introduced by https://github.com/edx/edx-platform/pull/23731 (edx@73fd2ed) and is present in koa.master, lilac.master, and master.

Testing instructions

Sandbox URL

• LMS: https://pr28034.sandbox.opencraft.hosting/
• Studio: https://studio.pr28034.sandbox.opencraft.hosting/

Extra settings:

HEARTBEAT_EXTENDED_CHECKS:
  - openedx.core.djangoapps.heartbeat.default_checks.check_celery
  - openedx.core.djangoapps.django_comment_common.comment_client.utils.check_forum_heartbeat

1. Ensure that the celery extended heartbeat check is passing for the sandbox.

Deadline

No deadline on our side, but it could be affecting many people, so we'd like to get it merged ASAP.

Other information

1. We would expect edx.org's worker servers to exhibit this issue too. Are edx.org's workers not using the extended heartbeat check? Or perhaps edx.org overrides HEARTBEAT_CELERY_ROUTING_KEY? courses.edx.org fails the celery extended heartbeat check.

[openedx-webhooks]

Thanks for the pull request, @pomegranited! I've created OSPR-5883 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.

[gabor-boros]

👍 🎉

• I tested what's in the test instructions
• I read through the code
• I checked for accessibility issues
• Includes documentation

[natabene]

@pomegranited Thank you for your contribution. Is this good for our review?

[pomegranited]

@natabene Yep, ready for edx/community review. Thanks @gabor-boros !

Pinging @felipemontoya @ziafazal @bradenmacdonald for a core committer review, and @arbrandes FYI since we'll need this in lilac too.

[felipemontoya]

Hey @pomegranited this is great. I did not know about the ?extended heartbeat.

In this case would it help to use derived to make the setting defined in common be up to date with the overrides given in production?

[pomegranited]

@felipemontoya Oo I don't know about derived settings either!

I've done that with 0940b02, but since it adds a function, also had to add a test (and update the test settings). Will see whether that affects anything else?

FYI, we generally override HEARTBEAT_EXTENDED_CHECKS to also run a forum heartbeat check, to make sure the discussion forum is working as expected.

HEARTBEAT_EXTENDED_CHECKS:
  - openedx.core.djangoapps.heartbeat.default_checks.check_celery
  - openedx.core.djangoapps.django_comment_common.comment_client.utils.check_forum_heartbeat

[edx-status-bot]

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

[arbrandes]

@pomegranited, ACK! Scheduling a task to look into it once its merged.

[pomegranited]

@felipemontoya I've applied your suggestions and added the necessary tests, and my sandbox is running with the latest code and working as expected. Are you able to review and merge this fix as core committer?

[felipemontoya]

Hey @pomegranited, I was queuing this until I could load it in a environment running master. However I have not had the time to do it.

In terms of the approach and a static code analysis it looks correct to me and as a Core Commiter I would approve it.

[pomegranited]

Thanks @felipemontoya ! For our scheduling, do you have an idea of when you'll be able to test this?

[felipemontoya]

I'm very sorry @pomegranited, I don't know when I will be able to review. As I said, from a static read I am good with the approach.

Perhaps a different CC can take this PR from here.

[pomegranited]

No worries @felipemontoya , thanks for letting me know!
Pinging the other CCs on edx-platform to see if any of you have time to review this? @ziafazal @bradenmacdonald @pdpinch

[bradenmacdonald]

@pomegranited I think this may already be fixed by @feanil in #28066 - can you please check? If not, I can take this review.

[pomegranited]

@bradenmacdonald Yep, the master periodic build heartbeat is working fine now, so https://github.com/edx/edx-platform/pull/28066 addressed this. I should have checked for open PRs before doing the rework here, and saved some duplicated effort!

[openedx-webhooks]

@pomegranited Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.