Skip to content

[SE-4860] Upgrade Celery to v5.2.1 - #29046

Merged
iamsobanjaved merged 2 commits into
openedx:masterfrom
open-craft:gabor/celery-bump
Mar 9, 2022
Merged

[SE-4860] Upgrade Celery to v5.2.1#29046
iamsobanjaved merged 2 commits into
openedx:masterfrom
open-craft:gabor/celery-bump

Conversation

@gabor-boros

@gabor-boros gabor-boros commented Oct 18, 2021

Copy link
Copy Markdown
Contributor

Description

Upgrade Celery to the latest version and bump its dependencies.

Supporting information

The release of Redis 6 introduced the ability to share this service among multiple accounts, securing access to specific keys using a username/password and ACLs.

Celery uses kombu to provide Redis brokering for the message queues, but kombu was missing a couple of key enhancements to support multi-tenant Redis:

As edX have identified that the best way to bring our kombu fixes from the original PR into edx-platform is to remove the constraints keeping celery < 5.0 and upgrade celery.

OpenCraft wants to use multi-tenant redis for their shared hosting service, so we can stop maintaining RabbitMQ.

The celery upgrade also requires a newer version of xblock-poll.

Dependencies

Testing instructions

The extended heartbeat check for celery is sufficient to test that these changes don't disrupt the functionality on the sandbox as configured below.

  1. Visit https://pr29046.sandbox.opencraft.hosting/heartbeat?extended
  2. Ensure that the celery check passes.

Deadline

None

Other information

N/A

Reviewers

Settings

DEMO_CREATE_STAFF_USER: true
EDXAPP_FEATURES:
  ENABLE_COMBINED_LOGIN_REGISTRATION: true

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Oct 18, 2021
@openedx-webhooks

Copy link
Copy Markdown

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

@gabor-boros gabor-boros changed the title feat: upgrade to celery 5.2.0 rc version [SE-4860] feat: upgrade to celery 5.2.0 rc version Oct 18, 2021
@natabene

Copy link
Copy Markdown
Contributor

@gabor-boros Thank you for your contribution. Please let me know once this is ready for our review.

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

👍 The multi-tenant redis sandbox is working really well @gabor-boros ! So glad to see these updates working in the platform.

  • I tested this by checking the celery heartbeat task, by running an instructor task grade report, and by ensuring that problems get scored.
  • I read through the code
  • I checked for accessibility issues N/A
  • Includes documentation N/A
  • I made sure any change in configuration variables is reflected in the corresponding
    client's configuration-secure repository.
    N/A

@Agrendalath Agrendalath left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

⚠️ We should first merge open-craft/xblock-poll#96.

👍

  • I tested this: tested that the celery tasks are working correctly on the sandbox instance
  • I read through the code
  • I checked for accessibility issues: n/a
  • Includes documentation: n/a
  • I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository: n/a

Comment thread requirements/edx/github.in Outdated
@gabor-boros

gabor-boros commented Nov 30, 2021

Copy link
Copy Markdown
Contributor Author

@Agrendalath I just rebased onto master and pushed. FYI as the JS unit tests were failing -- which is unrelated to this change.

@Agrendalath

Copy link
Copy Markdown
Member

@sarina, I'll merge this in 2 days, if you have no objections. We will need to coordinate merging this and https://github.com/edx/configuration/pull/6586 (by @pomegranited).

@sarina

sarina commented Dec 6, 2021

Copy link
Copy Markdown
Contributor

Hi @Agrendalath - since I joined tCRIL, I don't really know what's happening with edx.org production :) According to https://openedx.atlassian.net/wiki/spaces/COMM/pages/2800255156/Current+Core+Contributor+Committers+and+Champions you can ping @jmbowman and he can help field queries.

@pomegranited

Copy link
Copy Markdown
Contributor

Thanks @Agrendalath -- I've merged edx/configuration#6586 so you're clear to merge here when ready.

@Agrendalath

Copy link
Copy Markdown
Member

Ah, thanks, @sarina! I didn't notice the update on that page.

@jmbowman, I can merge this tomorrow if you have no objections.

@iamsobanjaved

Copy link
Copy Markdown
Contributor

@gabor-boros We were caught with some other priority work, we are planning to deploy this tomorrow and test this on stage, can you make this ready to be merged?

Bump celery and related dependencies to minimum necessary version.

Signed-off-by: Gabor Boros <gabor.brs@gmail.com>
Signed-off-by: Gabor Boros <gabor.brs@gmail.com>
@gabor-boros

Copy link
Copy Markdown
Contributor Author

can you make this ready to be merged?

@iamsobanjaved Rebased

@iamsobanjaved
iamsobanjaved merged commit 0588c92 into openedx:master Mar 9, 2022
@openedx-webhooks

Copy link
Copy Markdown

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

@iamsobanjaved

Copy link
Copy Markdown
Contributor

Merged the PR, will test this flow on stage (#29046 (comment))

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

@robrap

robrap commented Mar 9, 2022

Copy link
Copy Markdown
Contributor

Thanks for all your work in this @gabor-boros and @iamsobanjaved!

@pomegranited

Copy link
Copy Markdown
Contributor

So fantastic this got merged, thank you @gabor-boros and @iamsobanjaved !!

@kdmccormick

Copy link
Copy Markdown
Member

Congrats on the Celery upgrade 👍🏻

As an FYI: Celery 5's CLI changes caused some breakage in Tutor Nightly. I made a PR to fix it here: overhangio/tutor#614

@robrap

robrap commented Mar 18, 2022

Copy link
Copy Markdown
Contributor

Thanks @kdmccormick. I approved it.

gabor-boros added a commit to open-craft/openedx-platform that referenced this pull request Apr 18, 2022
See more at openedx#29046

Signed-off-by: Gabor Boros <gabor.brs@gmail.com>
gabor-boros added a commit to open-craft/openedx-platform that referenced this pull request Apr 27, 2022
* feat: backport Celery upgrade

See more at openedx#29046

Signed-off-by: Gabor Boros <gabor.brs@gmail.com>
@gabor-boros
gabor-boros deleted the gabor/celery-bump branch May 7, 2022 19:37
@gabor-boros
gabor-boros restored the gabor/celery-bump branch May 7, 2022 19:37
@gabor-boros
gabor-boros deleted the gabor/celery-bump branch May 7, 2022 19:37
@gabor-boros
gabor-boros restored the gabor/celery-bump branch May 7, 2022 19:37
@Agrendalath
Agrendalath deleted the gabor/celery-bump branch May 19, 2022 02:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged open-source-contribution PR author is not from Axim or 2U

Projects

None yet

Development

Successfully merging this pull request may close these issues.