launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

The proposal to merge lp:~jtv/maas/celery-shutdown into lp:maas has been updated.

Description changed to:

Actually, getting the celery shutdown unstuck was a one-liner from Gavin in services/celery/run.  In broad terms the work was discussed with him, but the details have expanded a bit.  The rest of the branch is all about ensuring that the multiprocessing.Manager used for the cache gets started up (1) in celery's parent process, (2) exactly once, (3) before it forks off workers, (4) explicitly, and (5) if at all possible, nowhere else.

Previously, initialization seemed to rely on celery importing the provisioningserver.cache module before forking, which as far as I can see probably happend as a consequence of provisioningserver.tasks importing items from provisioningserver.auth, which in turn imported from provisioningserver.cache, which as a side effect initialized the multiprocessing.Manager object as a side effect of the import.  Not too reassuring.

Here's how I changed the situation:
 * A function in provisioningserver.cache lets you initialize the cache, but only explicitly.
 * There's a guard against double-initialization.
 * Initialization-as-side-effect is now isolated in a dedicated module, initialize_cache.
 * Celery config is told to import this, from the parent daemon, before forking.
 * A fixture replaces the manual cache setup/teardown.
 * The fixture fakes out the cache's shared dict with a plain old dict.
 * Use imports the cache module, not the object, to keep patching simple.

I verified with some debug output that manager initialization happens exactly once, even though every worker process re-imports the settings file.  Modules from CELERY_IMPORTS are documented as imports that happen in the daemon before forking.

Perhaps the “initialized” variable in the initializer function is not strictly needed.  But it seemed slightly cleaner than deriving state from the current values in _manager/cache.

Jeroen

For more details, see:
https://code.launchpad.net/~jtv/maas/celery-shutdown/+merge/121041
-- 
https://code.launchpad.net/~jtv/maas/celery-shutdown/+merge/121041
Your team MAAS Maintainers is requested to review the proposed merge of lp:~jtv/maas/celery-shutdown into lp:maas.