Only instantiate one celery app per process. #25822
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Ticket: BOM-2086 Currently there are parts of the LMS that import content from the CMS APP and vice-versa. When this happens, we end up with 2 instances of the celery app and some tasks get registered to the wrong one. The tasks that were getting registered to the wrong one are never able to run and result in lots of production errors on celery workers. The timing of the CMS celery app instantiation is non deterministic so different tasks get lost depending on when it's imported by some code in the LMS. As long as SERVICE_VARIANT is set, this code should prevent the instantiation of both celery apps.
timmc-edx
approved these changes
Dec 9, 2020
|
Your PR has finished running tests. There were no failures. |
timmc-edx
added a commit
that referenced
this pull request
Dec 9, 2020
In #25822 we protect against the case where *any* `cms.**` module is imported by the running LMS process, causing cms.celery to be imported and a second celery to be instantiated (leading to tasks being registered to the wrong one and effectively lost). This commit protects against the more restricted case of `cms.celery` or `lms.celery` being imported explicitly, rather than implicitly by their respective root `__init__.py`.
|
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production. |
feanil
referenced
this pull request
Dec 9, 2020
This change was reverted along with the downgrade of super-csv because it was believed that this change was causing issues with celery task registration. It was reverted in https://github.com/edx/edx-platform/pull/25762 The fix turned out to be something different and this and the super-csv library should both be same to upgrade. The library upgrade will come in a follow-on make upgrade with some other constraints that also need to be lifted. Actual fix: https://github.com/edx/edx-platform/pull/25822
feanil
referenced
this pull request
Dec 9, 2020
This should no longer be necessary now that we have found a root cause fix for the issue. This was removed because it was causing the case of celery tasks not getting registered to happen more often. Undoing https://github.com/edx/edx-platform/pull/25787 Actual Fixes: - https://github.com/edx/edx-platform/pull/25822 - https://github.com/edx/edx-platform/pull/25825
|
EdX Release Notice: This PR has been deployed to the production environment. |
|
EdX Release Notice: This PR has been rolled back from the production environment. |
timmc-edx
added a commit
that referenced
this pull request
Dec 10, 2020
This should prevent the issues we've seen recently where cms modules are imported by the running lms process, resulting in two celery instances being created and tasks intermittently being registered to the wrong instance (and therefore effectively lost.) In commit ab6bf34/PR #25822 we tried to ensure that only one or the other of the instances was created by adding a startup check. Unfortunately, there's an external shared library that refers directly to the lms celery, causing a startup failure in cms, so we had to revert it. Rather than waiting to fix that library, this commit collapses the two instances together so that there is only ever one.
|
EdX Release Notice: This PR has been deployed to the production environment. |
timmc-edx
added a commit
that referenced
this pull request
Dec 10, 2020
…25840) This should prevent the issues we've seen recently where cms modules are imported by the running lms process, resulting in two celery instances being created and tasks intermittently being registered to the wrong instance (and therefore effectively lost.) In commit ab6bf34/PR #25822 we tried to ensure that only one or the other of the instances was created by adding a startup check. Unfortunately, there's an external shared library that refers directly to the lms celery, causing a startup failure in cms, so we had to revert it. Rather than waiting to fix that library, this commit collapses the two instances together so that there is only ever one.
feanil
referenced
this pull request
Dec 10, 2020
This change was reverted along with the downgrade of super-csv because it was believed that this change was causing issues with celery task registration. It was reverted in https://github.com/edx/edx-platform/pull/25762 The fix turned out to be something different and this and the super-csv library should both be same to upgrade. The library upgrade will come in a follow-on make upgrade with some other constraints that also need to be lifted. Actual fix: https://github.com/edx/edx-platform/pull/25822
feanil
referenced
this pull request
Dec 10, 2020
This should no longer be necessary now that we have found a root cause fix for the issue. This was removed because it was causing the case of celery tasks not getting registered to happen more often. Undoing https://github.com/edx/edx-platform/pull/25787 Actual Fixes: - https://github.com/edx/edx-platform/pull/25822 - https://github.com/edx/edx-platform/pull/25825
nedbat
pushed a commit
that referenced
this pull request
Dec 11, 2020
…25840) This should prevent the issues we've seen recently where cms modules are imported by the running lms process, resulting in two celery instances being created and tasks intermittently being registered to the wrong instance (and therefore effectively lost.) In commit ab6bf34/PR #25822 we tried to ensure that only one or the other of the instances was created by adding a startup check. Unfortunately, there's an external shared library that refers directly to the lms celery, causing a startup failure in cms, so we had to revert it. Rather than waiting to fix that library, this commit collapses the two instances together so that there is only ever one. (cherry picked from commit 0c57a02)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ticket: BOM-2086
Currently there are parts of the LMS that import content from the CMS
APP and vice-versa. When this happens, we end up with 2 instances of
the celery app and some tasks get registered to the wrong one. The
tasks that were getting registered to the wrong one are never able to
run and result in lots of production errors on celery workers.
The timing of the CMS celery app instantiation is non deterministic
so different tasks get lost depending on when it's imported by some
code in the LMS.
As long as SERVICE_VARIANT is set, this code should prevent the
instantiation of both celery apps.