Raise exception if both cms.celery and lms.celery are imported #25825
Conversation
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`.
| from openedx.core.lib.celery.routers import AlternateEnvironmentRouter | ||
|
|
||
| # set the default Django settings module for the 'celery' program. | ||
| os.environ.setdefault('DJANGO_SETTINGS_MODULE', 'cms.envs.production') | ||
|
|
||
| # Prevent instantiation of both CMS and LMS celery. | ||
| # See assert_celery_uninstantiated for more details. | ||
| assert_celery_uninstantiated('cms') |
There was a problem hiding this comment.
In which process would this code be run? In the CMS process or the CMS worker process? I'm trying to determine:
- which process would get killed.
- does the mutex need to be updated at a later point.
There was a problem hiding this comment.
This would break if LMS or LMS Worker processes tried to import cms.celery directly.
It will not break if CMS or CMS Worker processes imported cms.celery multiple times.
There was a problem hiding this comment.
I believe it would be run in both. cms/__init__.py imports cms.celery, so if any module under cms is loaded, this code runs. However, Feanil's change means that at least if the LMS process loads CMS code, this doesn't get loaded automatically. There's still the possibility that something imports it more directly, either now or in the future, and only in that case should this kill the process.
The assertion function should only get called once per process under normal conditions -- the cms.celery module is loaded for the first time, and the SINGLETON_VARIANT changes from None to 'cms'. If the module is imported again, there's no additional call, since the module is already loaded.
The only time the function would be called multiple times is if both lms.celery and cms.celery are imported, which we very much want to avoid (and don't believe currently happens, after #25822).
|
Studio imports stuff from LMS. I'm not sure if that stuff includes things that import celery. I'm sure you have, but to ask the obvious question: does studio start w/ this assertion? |
|
Your PR has finished running tests. The following contexts failed:
|
|
At least one test doesn't like this, but that was expected. :-/ |
|
@cpennington I haven't verified with devstack yet, but the results of a code grep were promising. |
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
|
Yes, LMS and Studio run fine locally. |
| from openedx.core.lib.celery.routers import AlternateEnvironmentRouter | ||
|
|
||
| # set the default Django settings module for the 'celery' program. | ||
| os.environ.setdefault('DJANGO_SETTINGS_MODULE', 'lms.envs.production') | ||
|
|
||
| # Prevent instantiation of both CMS and LMS celery. | ||
| # See assert_celery_uninstantiated for more details. | ||
| assert_celery_uninstantiated('lms') |
There was a problem hiding this comment.
This ends up breaking tests, which load both cms and lms files and also don't define a SERVICE_VARIANT environment variable, thus loading both cms.celery and lms.celery. There are several possible options, but none totally straightforward, I think.
|
This is superseded by https://github.com/edx/edx-platform/pull/25840 |
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
In #25822 we protect against the case where any
cms.**module isimported 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.celeryorlms.celerybeing imported explicitly, rather than implicitly by theirrespective root
__init__.py.