Skip to content

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

Merged
feanil merged 1 commit into
masterfrom
feanil/arch-1821-alternative
Jul 1, 2021
Merged

fix!: Make default queue names service variant specific.#28066
feanil merged 1 commit into
masterfrom
feanil/arch-1821-alternative

Conversation

@feanil

@feanil feanil commented Jun 30, 2021

Copy link
Copy Markdown
Contributor

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.

Supporting information

Jira: https://openedx.atlassian.net/browse/ARCHBOM-1821
Ticket Summary: Please investigate why sample_task tasks are stuck in a edx.core.high queue.

Deadline

None

Testing Instructions

  1. Go to: https://courses.edx.org/heartbeat?extended
  2. See that the Celery check fails.(Comes back expired)
  3. Go to: https://feanil.sandbox.edx.org/heartbeat?extended
  4. See that the celery check succeeds.

@feanil

feanil commented Jun 30, 2021

Copy link
Copy Markdown
Contributor Author

jenkins run js

@feanil

feanil commented Jun 30, 2021

Copy link
Copy Markdown
Contributor Author

This should supersede https://github.com/edx/edx-platform/pull/28065

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'
@feanil
feanil force-pushed the feanil/arch-1821-alternative branch from 6dba902 to c2d8488 Compare June 30, 2021 14:06
@feanil

feanil commented Jun 30, 2021

Copy link
Copy Markdown
Contributor Author

jenkins run python

@edx-status-bot

Copy link
Copy Markdown

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

@adzuci adzuci left a comment

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.

:shipit:

@feanil
feanil merged commit 4b29872 into master Jul 1, 2021
@feanil
feanil deleted the feanil/arch-1821-alternative branch July 1, 2021 13:56
@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

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

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

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

@pomegranited

Copy link
Copy Markdown
Contributor

@arbrandes Could we get this cherry-picked into lilac.master? Do you need a PR for that?

Thank you!

@arbrandes

Copy link
Copy Markdown
Contributor

@pomegranited, if I can get a PR, yes, I'll add it to my list of PR cherry-picks to approve. Is this something useful to us, internally? (I.e., can we get somebody in the team to whip up the PR?)

@pomegranited

Copy link
Copy Markdown
Contributor

Thanks @arbrandes , and yep, it's useful internally (it'll make our extended heartbeat checks succeed for lilac instances).
@xitij2000 maybe you'd like to create a task to do this PR? It's quick, but would count towards your core committer contribution.

pomegranited pushed a commit to open-craft/openedx-platform that referenced this pull request Jul 22, 2021
fix!: Make default queue names service variant specific.

(cherry picked from commit 4b29872)
@pomegranited

Copy link
Copy Markdown
Contributor

NVM @xitij2000 -- I'll do it as part of FAL-2076.

@xitij2000

Copy link
Copy Markdown
Contributor

@pomegranited Sure. I'm still in process so not sure if I can start just yet.

@pomegranited

Copy link
Copy Markdown
Contributor

No worries @xitij2000 -- https://github.com/edx/edx-platform/pull/28250 covers the issue for lilac, so no need for any action from you here.

ghost pushed a commit that referenced this pull request Aug 4, 2021
## Description

Cherry-picks https://github.com/edx/edx-platform/pull/28066 into `open-release/lilac.master`.

## Testing instructions

* LMS: https://periodic-build-lilac.opencraft.hosting/
* Studio: https://studio.periodic-build-lilac.opencraft.hosting/

(Ocim Admins only)

1. Locate the Active running appserver for the [Periodic build Lilac instance](https://manage.opencraft.com/instance/28490/) via Ocim
2. Visit the Authenticated Link , and append `/heartbeat?extended`
3. Ensure that the `celery` check is passing.

## Deadline

None

## Other information

1. Ordinarily, we'd be able to view the https://periodic-build-lilac.opencraft.hosting/heartbeat?extended to verify that the heartbeat checks are succeeding.

   However, the `forum` extended check is also failing for lilac.master (and master), and so Ocim redirects to the standard error page instead of showing the healthcheck json. The `forum` failure will be addressed by https://github.com/edx/configuration/pull/6482.

## Reviewer

- [ ] @gabor-boros  
- [ ] @arbrandes
pomegranited pushed a commit to open-craft/openedx-platform that referenced this pull request Sep 29, 2021
fix!: Make default queue names service variant specific.

(cherry picked from commit 4b29872)
ghost pushed a commit that referenced this pull request Oct 13, 2021
## Description

Cherry-picks https://github.com/edx/edx-platform/pull/28066 into `open-release/koa.master`.

## Testing instructions

* LMS: https://periodic-build-koa.opencraft.hosting/
* Studio: https://studio.periodic-build-koa.opencraft.hosting/

(Ocim Admins only)

1. Visit https://periodic-build-koa.opencraft.hosting/heartbeat?extended
2. Ensure that the `celery` check is passing. 
3. Shell into the latest deployed instance and run:

   ```
   # 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
   >>>
   ```

## Deadline

None

## Review
- [ ] @gabor-boros
- [x] @arbrandes or @Agrendalath ?
CC @bradenmacdonald
fsologureng pushed a commit to eol-uchile/openedx-platform that referenced this pull request Dec 13, 2023
fix!: Make default queue names service variant specific.

(cherry picked from commit 4b29872)
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.

8 participants