Merge lp:~rvb/maas/nonce-bug-1190986-tasks into lp:~maas-committers/maas/trunk

[Raphaël Badin]

Enable the master tasks. Add the cleanup_old_nonces task.

This requires the following changes in the packaging to work https://code.launchpad.net/~rvb/maas/packaging-master-django/+merge/190614.

(I've built a package from this branch and the packaging branch mentioned above and QAed this in the lab.)

[Gavin Panella (allenap)]

Looks good.

[1]

+    'cleanup-old-nonces': {
+        'task': 'maasserver.tasks.cleanup_old_nonces',
+        'schedule': timedelta(minutes=5),
+        'options': {'queue': WORKER_QUEUE_REGION},
+    },

Does Celery ensure that jobs don't pile up? If a run of this job takes
more than 5 minutes (I'm thinking of the first run, maybe), will
Celery enqueue the next job, or will it be automatically cancelled?
Or... does schedule mean the time *between* runs, rather than a rigid
from-the-clock schedule?

review: Approve

[Julian Edwards (julian-edwards)]

AFAIK they will pile up (we have seen this happen with all other jobs when something went wrong with Rabbit. I think 5 minutes is too often to run this anyway - how about once a day?

[Raphaël Badin (rvb)]

> Or... does schedule mean the time *between* runs, rather than a rigid
> from-the-clock schedule?

Like Julian said, celerybeat works like cron in this respect, it only fires off tasks at regular interval, nothing more intelligent than that.

[Raphaël Badin (rvb)]

> AFAIK they will pile up (we have seen this happen with all other jobs when something went wrong with Rabbit.
> I think 5 minutes is too often to run this anyway - how about once a day?

fwiw, the 5 minutes interval comes from the fact that nonces are only valid for 5 minutes (oauth enforces that) and the cleanup_old_nonces utility uses that… so running this utility every 5 minutes is the best we can do (now, your might want to argue doing something different but I'm explaining why I picked up 5 minutes in the first place). Considering that a nonce is created *every time* an authenticated request is made to that API, in the context of hyperscale, I think it's worth running this as often as possible.

Also:
- running cleanup_old_nonces when there is no nonces to clean up is instantaneous (well, obviously).
- running cleanup_old_nonces when there is 6M nonces to clean up takes ~10s on a modest canonistack instance.

[Gavin Panella (allenap)]

Julian's point about jobs piling up is concerning. If we can ensure that jobs don't pile up (is that a per-message, per-queue or per-exchange setting?) then I think that every 5 minutes is fine. If they're going to accumulate, then once a day seems okay. We've not had *anything* for over a year, so once a day seems more than adequate.

[Raphaël Badin (rvb)]

All right, https://code.launchpad.net/~rvb/maas/change-freq/+merge/190894

[Julian Edwards (julian-edwards)]

On 14/10/13 18:51, Gavin Panella wrote:
> We've not had *anything* for over a year, so once a day seems more than adequate.

That was precisely my reasoning for once a day.

=== modified file 'etc/celeryconfig.py'
--- etc/celeryconfig.py	2013-10-07 09:12:40 +0000
+++ etc/celeryconfig.py	2013-10-11 11:16:45 +0000
@@ -17,11 +17,23 @@
 
 __metaclass__ = type
 
+from datetime import timedelta
+
 import celeryconfig_common
 from maas import import_settings
 
+# Region worker queue.  Will be overridden by the customized setting in the
+# local MAAS Celery config.
+WORKER_QUEUE_REGION = None
+CELERY_IMPORTS = None
+
 import_settings(celeryconfig_common)
 
+CELERY_IMPORTS = CELERY_IMPORTS + (
+    # Master tasks.
+    "maasserver.tasks",
+)
+
 try:
     import maas_local_celeryconfig
 except ImportError:
@@ -29,6 +41,10 @@
 else:
     import_settings(maas_local_celeryconfig)
 
-
 CELERYBEAT_SCHEDULE = {
+    'cleanup-old-nonces': {
+        'task': 'maasserver.tasks.cleanup_old_nonces',
+        'schedule': timedelta(minutes=5),
+        'options': {'queue': WORKER_QUEUE_REGION},
+    },
 }

=== modified file 'etc/celeryconfig_common.py'
--- etc/celeryconfig_common.py	2013-10-07 09:12:40 +0000
+++ etc/celeryconfig_common.py	2013-10-11 11:16:45 +0000
@@ -56,6 +56,7 @@
 CELERYBEAT_SCHEDULE_FILENAME = '/var/lib/maas/celerybeat-cluster-schedule'
 
 WORKER_QUEUE_DNS = 'celery'
+WORKER_QUEUE_REGION = 'celery'
 
 # Each cluster should have its own queue created automatically by Celery.
 CELERY_CREATE_MISSING_QUEUES = True

=== modified file 'services/region-worker/run'
--- services/region-worker/run	2013-01-02 16:57:53 +0000
+++ services/region-worker/run	2013-10-11 11:16:45 +0000
@@ -15,6 +15,7 @@
 [ -z "${logdir:-}" ] || exec &>> "${logdir}/current"
 
 export PYTHONPATH=etc/:src/
+export DJANGO_SETTINGS_MODULE=maas.demo
 script="$(readlink -f bin/celeryd)"
 # XXX GavinPanella 2013-01-02, bug=1040529: celeryd does not shutdown
 # correctly when signalled: processes are often left behind. However,

=== added file 'src/maasserver/tasks.py'
--- src/maasserver/tasks.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/tasks.py	2013-10-11 11:16:45 +0000
@@ -0,0 +1,30 @@
+# Copyright 2013 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Maasserver tasks that are run in Celery workers."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+str = None
+
+__metaclass__ = type
+__all__ = [
+    'cleanup_old_nonces',
+    ]
+
+
+from celery.task import task
+from maasserver import (
+    logger,
+    nonces_cleanup,
+    )
+
+
+@task
+def cleanup_old_nonces(**kwargs):
+    nb_nonces_deleted = nonces_cleanup.cleanup_old_nonces()
+    logger.info("%d expired nonce(s) cleaned up." % nb_nonces_deleted)

=== added file 'src/maasserver/tests/test_tasks.py'
--- src/maasserver/tests/test_tasks.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/tests/test_tasks.py	2013-10-11 11:16:45 +0000
@@ -0,0 +1,39 @@
+# Copyright 2013 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for Celery tasks."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+str = None
+
+__metaclass__ = type
+__all__ = []
+
+from fixtures import FakeLogger
+from maasserver import tasks
+from maastesting.celery import CeleryFixture
+from maastesting.testcase import MAASTestCase
+from testresources import FixtureResource
+from testtools.matchers import Contains
+
+
+class TestCleanupOldNonces(MAASTestCase):
+
+    resources = (
+        ("celery", FixtureResource(CeleryFixture())),
+        )
+
+    def test_cleanup_old_nonces_calls_cleanup_old_nonces(self):
+        logger = self.useFixture(FakeLogger('maasserver'))
+        nb_cleanups = 3
+        mock = self.patch(tasks, 'nonces_cleanup')
+        mock.cleanup_old_nonces.return_value = nb_cleanups
+        tasks.cleanup_old_nonces()
+        mock.cleanup_old_nonces.assert_called_once_with()
+        message = "%d expired nonce(s) cleaned up." % nb_cleanups
+        self.assertThat(logger.output, Contains(message))