launchpad-reviewers team mailing list archive

[Colin Watson <mp+452286@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/lp-mailman:kombu-amqp into lp-mailman:master.

Commit message:
Support multiple RabbitMQ broker URLs

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/lp-mailman/+git/lp-mailman/+merge/452286

`kombu` is a higher-level messaging library than `amqp`.  For our purposes, we can treat it mostly as a wrapper for `amqp` with some slightly more convenient interfaces, but it has one key feature that's of interest to us: it supports multiple RabbitMQ broker URLs with round-robin failover between them.  This makes it possible to configure lp-mailman to use RabbitMQ with high availability: if we have a RabbitMQ cluster, then we can configure lp-mailman with broker URLs for all the nodes in the cluster, and if one fails then `kombu` will automatically fail over to the next.

To make it practical to configure this, I had to add a `rabbitmq.broker_urls` configuration option, which supersedes the existing broken-out configuration options (`rabbitmq.host`, `rabbitmq.userid`, `rabbitmq.password`, and `rabbitmq.virtual_host`).  For backward compatibility, the old options continue to work as long as `rabbitmq.broker_urls` is unset.

This also includes upgrading to oops-amqp 0.2.0, since that includes a change to accept connection factories that return `kombu` connections.

This is a reduced version of https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/432065 in Launchpad, picked up when I realized that it was going to be needed by the lp-mailman charm.  Testing is difficult since we stripped out much of the supporting code when extracting lp-mailman from Launchpad, but at least similar code is already tested as part of Launchpad.

Dependencies MP: https://code.launchpad.net/~cjwatson/lp-mailman/+git/dependencies/+merge/452285
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/lp-mailman:kombu-amqp into lp-mailman:master.

diff --git a/configs/development/mailman-lazr.conf b/configs/development/mailman-lazr.conf
index 563b52a..b20036b 100644
--- a/configs/development/mailman-lazr.conf
+++ b/configs/development/mailman-lazr.conf
@@ -30,7 +30,4 @@ hard_max_size: 1000000
 
 [rabbitmq]
 launch: True
-host: localhost:56720
-userid: guest
-password: guest
-virtual_host: /
+broker_urls: amqp://guest:guest@localhost:56720//
diff --git a/configs/testrunner/mailman-lazr.conf b/configs/testrunner/mailman-lazr.conf
index 977ba21..37ab96e 100644
--- a/configs/testrunner/mailman-lazr.conf
+++ b/configs/testrunner/mailman-lazr.conf
@@ -14,7 +14,4 @@ shared_secret: topsecret
 
 [rabbitmq]
 launch: False
-host: none
-userid: none
-password: none
-virtual_host: none
+broker_urls: none
diff --git a/constraints.txt b/constraints.txt
index 1680cb0..b3e34a0 100644
--- a/constraints.txt
+++ b/constraints.txt
@@ -165,7 +165,7 @@ zc.recipe.testrunner==2.1
 
 # Alphabetical, case-insensitive, please! :-)
 
-amqp==2.4.2
+amqp==2.6.1
 bson==0.5.9
 contextlib2==0.6.0.post1
 defusedxml==0.6.0
@@ -173,6 +173,7 @@ distro==1.4.0
 httplib2==0.8
 iso8601==0.1.12
 keyring==0.6.2
+kombu==4.6.3
 launchpadlib==1.10.9
 lazr.config==2.2.2
 lazr.delegates==2.0.4
@@ -182,7 +183,7 @@ lazr.uri==1.0.3
 mock==1.0.1
 oauthlib==3.1.0
 oops==0.0.14
-oops-amqp==0.1.0
+oops-amqp==0.2.0
 oops-datedir-repo==0.0.24
 oops-datedir2amqp==0.1.0
 python-dateutil==2.8.1
diff --git a/lib/lp/services/config/schema-lazr.conf b/lib/lp/services/config/schema-lazr.conf
index 85b6330..6df2709 100644
--- a/lib/lp/services/config/schema-lazr.conf
+++ b/lib/lp/services/config/schema-lazr.conf
@@ -168,12 +168,23 @@ build_host_name:
 # Should RabbitMQ be launched by default?
 # datatype: boolean
 launch: False
-# The host:port at which RabbitMQ is listening.
+# The URL at which RabbitMQ is listening (in the form
+# amqp://USERNAME:PASSWORD@HOSTNAME:PORT/VIRTUAL_HOST), or a space-separated
+# list of such URLs to use round-robin failover between them.
+broker_urls: none
+# The host:port at which RabbitMQ is listening (ignored if broker_urls is
+# set).
 # datatype: string
 host: none
+# The username to use when connecting to RabbitMQ (ignored if broker_urls is
+# set).
 # datatype: string
 userid: none
+# The password to use when connecting to RabbitMQ (ignored if broker_urls is
+# set).
 # datatype: string
 password: none
+# The virtual host to use when connecting to RabbitMQ (ignored if
+# broker_urls is set).
 # datatype: string
 virtual_host: none
diff --git a/lib/lp/services/messaging/rabbit.py b/lib/lp/services/messaging/rabbit.py
index d3cbcaa..72373b4 100644
--- a/lib/lp/services/messaging/rabbit.py
+++ b/lib/lp/services/messaging/rabbit.py
@@ -9,7 +9,8 @@ __all__ = [
     "is_configured",
     ]
 
-import amqp
+import kombu
+from lazr.config import as_host_port
 
 from lp.services.config import config
 from lp.services.messaging.interfaces import MessagingUnavailable
@@ -20,6 +21,8 @@ LAUNCHPAD_EXCHANGE = "launchpad-exchange"
 
 def is_configured():
     """Return True if rabbit looks to be configured."""
+    if config.rabbitmq.broker_urls is not None:
+        return True
     return not (
         config.rabbitmq.host is None or
         config.rabbitmq.userid is None or
@@ -34,9 +37,16 @@ def connect():
     """
     if not is_configured():
         raise MessagingUnavailable("Incomplete configuration")
-    connection = amqp.Connection(
-        host=config.rabbitmq.host, userid=config.rabbitmq.userid,
-        password=config.rabbitmq.password,
-        virtual_host=config.rabbitmq.virtual_host)
+    if config.rabbitmq.broker_urls is not None:
+        connection = kombu.Connection(config.rabbitmq.broker_urls.split())
+    else:
+        hostname, port = as_host_port(config.rabbitmq.host, default_port=5672)
+        connection = kombu.Connection(
+            hostname=hostname,
+            userid=config.rabbitmq.userid,
+            password=config.rabbitmq.password,
+            virtual_host=config.rabbitmq.virtual_host,
+            port=port,
+        )
     connection.connect()
     return connection
diff --git a/setup.py b/setup.py
index 4d6dab1..fb3a50b 100644
--- a/setup.py
+++ b/setup.py
@@ -144,6 +144,7 @@ setup(
         'contextlib2; python_version < "3.3"',
         'defusedxml',
         'fixtures',
+        'kombu',
         'lazr.config',
         'lazr.enum',
         'mock',