launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/turnip:fix-rabbitmq-hooks into turnip:master.

Commit message:
charm: Fix handling of rabbitmq relations

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/turnip/+git/turnip/+merge/389630

Avoid trying to start turnip-celery when its code and storage prerequisites are potentially unavailable.

Rearrange how we request access to rabbitmq.  Using the 'available' state provided by the rabbitmq interface means we can guarantee that the broker URL will always be available at that point, simplifying configuration.  To ensure that things work if the rabbitmq relation is removed and re-added, we must explicitly clear any previous access request so that the rabbitmq-server charm notices that the relation data has changed.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/turnip:fix-rabbitmq-hooks into turnip:master.

diff --git a/charm/layer/turnip-base/lib/charms/turnip/base.py b/charm/layer/turnip-base/lib/charms/turnip/base.py
index dbeada4..35ebf76 100644
--- a/charm/layer/turnip-base/lib/charms/turnip/base.py
+++ b/charm/layer/turnip-base/lib/charms/turnip/base.py
@@ -368,15 +368,10 @@ def publish_website(website, name, port):
 def get_rabbitmq_url():
     rabbitmq = endpoint_from_name('amqp')
 
-    vhost = rabbitmq.vhost() if rabbitmq.vhost() else "/"
-    if not rabbitmq.username():
-        try:
-            rabbitmq.request_access(username="turnip", vhost=vhost)
-        except Exception:
-            pass
-
     if not rabbitmq.username() or not rabbitmq.password():
-        return None
+        raise AssertionError(
+            'get_rabbitmq_url called when amqp relation data incomplete')
 
     user = "%s:%s" % (rabbitmq.username(), rabbitmq.password())
+    vhost = rabbitmq.vhost() if rabbitmq.vhost() else "/"
     return "pyamqp://%s@%s/%s" % (user, rabbitmq.private_address(), vhost)
diff --git a/charm/turnip-api/lib/charms/turnip/api.py b/charm/turnip-api/lib/charms/turnip/api.py
index 77b7976..7635b10 100644
--- a/charm/turnip-api/lib/charms/turnip/api.py
+++ b/charm/turnip-api/lib/charms/turnip/api.py
@@ -24,9 +24,6 @@ from charms.turnip.base import (
 
 
 def configure_wsgi():
-    celery_broker = get_rabbitmq_url()
-    if celery_broker is None:
-        return host.service_running('turnip-api')
     config = hookenv.config()
     context = dict(config)
     context.update({
@@ -36,7 +33,7 @@ def configure_wsgi():
         'data_mount_unit': data_mount_unit(),
         'logs_dir': logs_dir(),
         'venv_dir': venv_dir(),
-        'celery_broker': celery_broker,
+        'celery_broker': get_rabbitmq_url(),
         })
     if context['wsgi_workers'] == 0:
         context['wsgi_workers'] = cpu_count() * 2 + 1
@@ -51,4 +48,3 @@ def configure_wsgi():
         host.service_stop('turnip-api')
     if not host.service_resume('turnip-api'):
         raise RuntimeError('Failed to start turnip-api')
-    return True
diff --git a/charm/turnip-api/reactive/turnip-api.py b/charm/turnip-api/reactive/turnip-api.py
index 223112d..dc304cf 100644
--- a/charm/turnip-api/reactive/turnip-api.py
+++ b/charm/turnip-api/reactive/turnip-api.py
@@ -22,8 +22,7 @@ from charms.turnip.base import (
     )
 
 
-@when('turnip.installed', 'turnip.storage.available',
-      'turnip.rabbitmq.available')
+@when('turnip.installed', 'turnip.storage.available', 'amqp.available')
 @when_not('turnip.configured')
 def configure_turnip():
     configure_wsgi()
@@ -36,7 +35,7 @@ def configure_turnip():
 
 
 @when('turnip.configured')
-@when_not_all('turnip.storage.available', 'turnip.rabbitmq.available')
+@when_not_all('turnip.storage.available', 'amqp.available')
 def deconfigure_turnip():
     deconfigure_service('turnip-api')
     clear_flag('turnip.configured')
@@ -44,11 +43,19 @@ def deconfigure_turnip():
 
 
 @when('amqp.connected')
-def rabbitmq_available():
-    if configure_wsgi():
-        set_flag('turnip.rabbitmq.available')
-    else:
-        clear_flag('turnip.rabbitmq.available')
+@when_not('turnip.amqp.requested-access')
+def request_amqp_access(rabbitmq):
+    # Clear any previous request so that the rabbitmq-server charm notices
+    # the change.
+    rabbitmq.request_access(username='', vhost='')
+    rabbitmq.request_access(username='turnip', vhost='/')
+    set_flag('turnip.amqp.requested-access')
+
+
+@when('turnip.amqp.requested-access')
+@when_not('amqp.connected')
+def unrequest_amqp_access():
+    clear_flag('turnip.amqp.requested-access')
 
 
 @when('nrpe-external-master.available', 'turnip.configured')
diff --git a/charm/turnip-celery/lib/charms/turnip/celery.py b/charm/turnip-celery/lib/charms/turnip/celery.py
index 60ed019..8120e77 100644
--- a/charm/turnip-celery/lib/charms/turnip/celery.py
+++ b/charm/turnip-celery/lib/charms/turnip/celery.py
@@ -22,12 +22,7 @@ from charms.turnip.base import (
 
 
 def configure_celery():
-    """Configure celery service, connecting it to rabbitmq.
-
-    :return: True if service is running, False otherwise."""
-    celery_broker = get_rabbitmq_url()
-    if celery_broker is None:
-        return host.service_running('turnip-celery')
+    """Configure celery service, connecting it to rabbitmq."""
     config = hookenv.config()
     context = dict(config)
     context.update({
@@ -36,7 +31,7 @@ def configure_celery():
         'data_mount_unit': data_mount_unit(),
         'logs_dir': logs_dir(),
         'venv_dir': venv_dir(),
-        'celery_broker': celery_broker,
+        'celery_broker': get_rabbitmq_url(),
         })
     templating.render(
         'turnip-celery.service.j2',
@@ -47,4 +42,3 @@ def configure_celery():
     reload_systemd()
     if not host.service_resume('turnip-celery'):
         raise RuntimeError('Failed to start turnip-celery')
-    return True
diff --git a/charm/turnip-celery/reactive/turnip-celery.py b/charm/turnip-celery/reactive/turnip-celery.py
index 357e96b..84e3513 100644
--- a/charm/turnip-celery/reactive/turnip-celery.py
+++ b/charm/turnip-celery/reactive/turnip-celery.py
@@ -21,10 +21,10 @@ from charms.turnip.base import (
     )
 
 
-@when('turnip.installed', 'turnip.storage.available',
-      'turnip.rabbitmq.available')
+@when('turnip.installed', 'turnip.storage.available', 'amqp.available')
 @when_not('turnip.configured')
 def configure_turnip():
+    configure_celery()
     configure_service()
     set_flag('turnip.configured')
     clear_flag('turnip.storage.nrpe-external-master.published')
@@ -34,7 +34,7 @@ def configure_turnip():
 
 
 @when('turnip.configured')
-@when_not_all('turnip.storage.available', 'turnip.rabbitmq.available')
+@when_not_all('turnip.storage.available', 'amqp.available')
 def deconfigure_turnip():
     deconfigure_service('turnip-celery')
     clear_flag('turnip.configured')
@@ -42,11 +42,19 @@ def deconfigure_turnip():
 
 
 @when('amqp.connected')
-def rabbitmq_available():
-    if configure_celery():
-        set_flag('turnip.rabbitmq.available')
-    else:
-        clear_flag('turnip.rabbitmq.available')
+@when_not('turnip.amqp.requested-access')
+def request_amqp_access(rabbitmq):
+    # Clear any previous request so that the rabbitmq-server charm notices
+    # the change.
+    rabbitmq.request_access(username='', vhost='')
+    rabbitmq.request_access(username='turnip', vhost='/')
+    set_flag('turnip.amqp.requested-access')
+
+
+@when('turnip.amqp.requested-access')
+@when_not('amqp.connected')
+def unrequest_amqp_access():
+    clear_flag('turnip.amqp.requested-access')
 
 
 @when('nrpe-external-master.available', 'turnip.configured')