-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SE-4860] Upgrade Celery to v5.2.1 #29046
Conversation
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:
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 Thank you for your contribution. Please let me know once this is ready for our review. |
fde0415
to
a4d4bf5
Compare
a4d4bf5
to
da2085c
Compare
d3be21b
to
8db946d
Compare
cb74872
to
eeeed3d
Compare
There was a problem hiding this comment.
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 issuesN/A -
Includes documentationN/A -
I made sure any change in configuration variables is reflected in the correspondingN/A
client'sconfiguration-secure
repository.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
- 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
eeeed3d
to
73b39b2
Compare
73b39b2
to
a7675a1
Compare
@Agrendalath I just rebased onto master and pushed. FYI as the JS unit tests were failing -- which is unrelated to this change. |
@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). |
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. |
Thanks @Agrendalath -- I've merged edx/configuration#6586 so you're clear to merge here when ready. |
@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 <[email protected]>
Signed-off-by: Gabor Boros <[email protected]>
7c27cdb
to
df50d37
Compare
@iamsobanjaved Rebased |
@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. |
Merged the PR, will test this flow on stage (#29046 (comment)) |
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production. |
EdX Release Notice: This PR has been deployed to the production environment. |
Thanks for all your work in this @gabor-boros and @iamsobanjaved! |
So fantastic this got merged, thank you @gabor-boros and @iamsobanjaved !! |
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 |
Thanks @kdmccormick. I approved it. |
See more at openedx#29046 Signed-off-by: Gabor Boros <[email protected]>
* feat: backport Celery upgrade See more at openedx#29046 Signed-off-by: Gabor Boros <[email protected]>
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, butkombu
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.
celery
check passes.Deadline
None
Other information
N/A
Reviewers
Settings