Merge lp:~allenap/launchpad/add-rabbit-service-bug-806160 into lp:launchpad

[Gavin Panella]

[r=jtv][bug=806160] Start up RabbitMQ on a known port when using make run and make run_all.

This starts up a RabbitMQ server when using make run or make
run_all. It also moves the RabbitServer fixture off into its own
package.

A point of controversy: the hostname and port for the RabbitMQ service
is hard-coded into schema-lazr.conf. Rob has said that he wants this
to be dynamically allocated, and indeed rabbitfixture knows how to do
that already if you ask it, but it makes it much harder for other
processes - outside of the appserver started by make run/run_all - to
know where to connect.

I don't know of an existing way to do this so that it just works -
although I'm sure everyone has ideas - and I think it's important not
to increase development friction while we wait for it. Choosing a
fixed port is no worse than the existing hard-coded ports we have for
things like HTTP, and it gets us up and running and *developing*.

[Robert Collins (lifeless)]

You write out a unique config fragment and export LPCONFIG before
starting services depending on rabbit. We do this in the test runner
and it works well - is there any reason it won't work here?

[Gavin Panella (allenap)]

I was thinking of something like:

$ make run_all &
$ make sync_branches

How do the scripts that sync_branches invokes know where Rabbit is?

[Robert Collins (lifeless)]

$ make run_all &
Assigned LPCONFIG=12345
$ export LPCONFIG=12345
$ make sync_branches

[Jeroen T. Vermeulen (jtv)]

In case you want to address Robert's notes in a separate branch, I'm giving you my approval vote for this one—with comments:

41 === modified file 'lib/canonical/launchpad/scripts/runlaunchpad.py'
42 --- lib/canonical/launchpad/scripts/runlaunchpad.py 2011-03-07 16:32:12 +0000
43 +++ lib/canonical/launchpad/scripts/runlaunchpad.py 2011-09-13 15:01:18 +0000

64 @@ -221,6 +223,20 @@
65 process.stdin.close()
66
67
68 +class RabbitService(Service):
69 + """A RabbitMQ service."""
70 +
71 + @property
72 + def should_launch(self):
73 + return config.rabbitmq.launch
74 +
75 + def launch(self):
76 + hostname, port = config.rabbitmq.host.split(":")
77 + self.server = RabbitServer(
78 + RabbitServerResources(hostname=hostname, port=int(port)))
79 + self.useFixture(self.server)

Anything sensible to be done if no port is specified?

168 === added file 'lib/lp/services/rabbit/tests/test_server.py'
169 --- lib/lp/services/rabbit/tests/test_server.py 1970-01-01 00:00:00 +0000
170 +++ lib/lp/services/rabbit/tests/test_server.py 2011-09-13 15:01:18 +0000
171 @@ -0,0 +1,36 @@
172 +# Copyright 2011 Canonical Ltd. This software is licensed under the
173 +# GNU Affero General Public License version 3 (see the file LICENSE).
174 +
175 +"""Tests for lp.testing.fixture."""
176 +
177 +__metaclass__ = type
178 +
179 +from textwrap import dedent
180 +
181 +from fixtures import EnvironmentVariableFixture
182 +
183 +from canonical.testing.layers import BaseLayer
184 +from lp.services.rabbit.server import RabbitServer
185 +from lp.testing import TestCase
186 +
187 +
188 +class TestRabbitServer(TestCase):
189 +
190 + layer = BaseLayer
191 +
192 + def test_service_config(self):
193 + # Rabbit needs to fully isolate itself: an existing per user
194 + # .erlang.cookie has to be ignored, and ditto bogus HOME if other
195 + # tests fail to cleanup.
196 + self.useFixture(EnvironmentVariableFixture('HOME', '/nonsense/value'))
197 +
198 + # RabbitServer pokes some .ini configuration into its config.
199 + fixture = self.useFixture(RabbitServer())
200 + expected = dedent("""\
201 + [rabbitmq]
202 + host: localhost:%d
203 + userid: guest
204 + password: guest
205 + virtual_host: /
206 + """ % fixture.config.port)
207 + self.assertEqual(expected, fixture.config.service_config)

It seems unnecessarily brittle to test for the exact text. Otherwise you're really just repeating what the code does in the test. Plus, what of item ordering? What of whitespace? It's nice to have some room for cosmetic editing etc. without breaking tests.

To me it'd make more sense to parse the config snippet and assert the presence and value of the keys you care about.

Jeroen

review: Approve

[Gavin Panella (allenap)]

> $ make run_all &
> Assigned LPCONFIG=12345
> $ export LPCONFIG=12345
> $ make sync_branches

To me this would be unacceptable. Development is complicated enough.

Adding an extra step means another thing to remember, or another wiki
page to read, and the "Assigned LPCONFIG=12345" line would be mixed
into other log lines, or disappear off the scrollback.

Perhaps we could do something better:

- config.push() could also write out the fragments its given to
  configs/$LPCONFIG/extra-${sequence}-${name}.conf

- configs.pop() would remove the fragment from the filesystem too.

- canonical.config would automatically know to look for these extra-*
  files and push them in ${sequence} order.

- configs/*/extra-* could be added to .bzrignore

- make clean could remove them

[Robert Collins (lifeless)]

On Thu, Sep 15, 2011 at 5:07 AM, Gavin Panella
<email address hidden> wrote:

> Perhaps we could do something better:
>
> - config.push() could also write out the fragments its given to
>  configs/$LPCONFIG/extra-${sequence}-${name}.conf

We might want a special verb for this to avoid it happening my mistake
on production servers. Make clean probably would want to delete them
all. The outer most test layer may want to zap them all as it comes up
as well.

Other than that this sounds good.

-Rob

[Gavin Panella (allenap)]

> In case you want to address Robert's notes in a separate branch, I'm giving
> you my approval vote for this one—with comments:

Thanks Jeroen :)

[...]
> Anything sensible to be done if no port is specified?

I've changed this to use lazr.config.as_host_port(...,
default_host=None, default_port=None). The effect of this is that the
code in rabbitfixture will revert to its defaults if either or both
are not specified: localhost and a dynamically allocated port.

[...]
> It seems unnecessarily brittle to test for the exact text. Otherwise you're
> really just repeating what the code does in the test. Plus, what of item
> ordering? What of whitespace? It's nice to have some room for cosmetic
> editing etc. without breaking tests.
>
> To me it'd make more sense to parse the config snippet and assert the presence
> and value of the keys you care about.

I've followed your advice; the test now parses the snippet and checks
all the sections and keys.

[Gavin Panella (allenap)]

> On Thu, Sep 15, 2011 at 5:07 AM, Gavin Panella <...> wrote:
>
> > Perhaps we could do something better:
> >
> > - config.push() could also write out the fragments its given to
> >  configs/$LPCONFIG/extra-${sequence}-${name}.conf
>
> We might want a special verb for this to avoid it happening my mistake
> on production servers.

By special verb, do you mean an alternative to .push() and .pop()? If
so, it might instead be worth having the special verb for pushing and
popping outside of dev/test; there are only a few places where these
methods are used outside of test/dev code:

  lib/lp/registry/scripts/distributionmirror_prober.py
  lib/canonical/launchpad/scripts/__init__.py
  lib/canonical/testing/layers.py
  lib/canonical/database/sqlbase.py

(In addition, .push() and .pop() could be disallowed - or warned about
- in non-dev/test environments.)

> Make clean probably would want to delete them all.

Agreed.

> The outer most test layer may want to zap them all as it comes up as
> well.

Agreed, thanks for thinking of that.

> Other than that this sounds good.

Tip top, thank you. Do you *need* me to do that before landing this
branch, or can I land this with a promise to sort out this new idea
asap? Red Squad are going to be working on longpoll as of now so even
a day or two's delay on this is going to affect that.

[Robert Collins (lifeless)]

On Thu, Sep 15, 2011 at 7:39 AM, Gavin Panella
<email address hidden> wrote:

> Tip top, thank you. Do you *need* me to do that before landing this
> branch, or can I land this with a promise to sort out this new idea
> asap? Red Squad are going to be working on longpoll as of now so even
> a day or two's delay on this is going to affect that.

asap is fine.

=== modified file 'Makefile'
--- Makefile	2011-09-10 09:01:08 +0000
+++ Makefile	2011-09-15 19:55:38 +0000
@@ -253,7 +253,7 @@
 	$(PY) cronscripts/merge-proposal-jobs.py -v
 
 run: build inplace stop
-	bin/run -r librarian,google-webservice,memcached -i $(LPCONFIG)
+	bin/run -r librarian,google-webservice,memcached,rabbitmq -i $(LPCONFIG)
 
 run.gdb:
 	echo 'run' > run.gdb
@@ -265,7 +265,7 @@
 
 run_all: build inplace stop
 	bin/run \
-	 -r librarian,sftp,forker,mailman,codebrowse,google-webservice,memcached \
+	 -r librarian,sftp,forker,mailman,codebrowse,google-webservice,memcached,rabbitmq \
 	 -i $(LPCONFIG)
 
 run_codebrowse: build
 
=== modified file 'configs/development/launchpad-lazr.conf'
--- configs/development/launchpad-lazr.conf	2011-08-14 23:11:45 +0000
+++ configs/development/launchpad-lazr.conf	2011-09-15 19:55:38 +0000
@@ -239,6 +239,13 @@
 host_key_private=lib/lp/poppy/tests/poppy-sftp
 host_key_public=lib/lp/poppy/tests/poppy-sftp.pub
 
+[rabbitmq]
+launch: True
+host: localhost:56720
+userid: guest
+password: guest
+virtual_host: /
+
 [reclaimbranchspace]
 error_dir: /var/tmp/codehosting.test
 oops_prefix: RBS
 
=== modified file 'configs/testrunner/launchpad-lazr.conf'
--- configs/testrunner/launchpad-lazr.conf	2011-09-05 15:42:27 +0000
+++ configs/testrunner/launchpad-lazr.conf	2011-09-15 19:55:38 +0000
@@ -201,6 +201,13 @@
 oops_prefix: TAPPORTBLOB
 error_dir: /var/tmp/lperr.test
 
+[rabbitmq]
+launch: False
+host: none
+userid: none
+password: none
+virtual_host: none
+
 [request_daily_builds]
 oops_prefix: TRDB
 error_dir: /var/tmp/lperr.test
 
=== modified file 'lib/canonical/config/schema-lazr.conf'
--- lib/canonical/config/schema-lazr.conf	2011-09-02 16:22:48 +0000
+++ lib/canonical/config/schema-lazr.conf	2011-09-15 19:55:38 +0000
@@ -511,7 +511,8 @@
 forced_hostname: none
 
 # Where to find the code import scheduler service.
-codeimportscheduler_url: http://xmlrpc-private.launchpad.dev:8087/codeimportscheduler
+codeimportscheduler_url:
+    http://xmlrpc-private.launchpad.dev:8087/codeimportscheduler
 
 # The maximum number of jobs to run on a machine at one time.
 max_jobs_per_machine: 3
@@ -585,7 +586,8 @@
 # The database user which will be used by this process.
 # datatype: string
 dbuser: rosettaadmin
-source_interface: lp.translations.interfaces.translationpackagingjob.ITranslationPackagingJobSource
+source_interface:
+    lp.translations.interfaces.translationpackagingjob.ITranslationPackagingJobSource
 
 # See [error_reports].
 error_dir: none
@@ -950,7 +952,8 @@
 
 [initializedistroseries]
 dbuser: initializedistroseries
-source_interface: lp.soyuz.interfaces.distributionjob.IInitializeDistroSeriesJobSource
+source_interface:
+    lp.soyuz.interfaces.distributionjob.IInitializeDistroSeriesJobSource
 
 # See [error_reports].
 error_dir: none
@@ -1025,7 +1028,8 @@
 # domains listed here will be sent to the parent domain,
 # allowing sessions to be shared between vhosts.
 # Domains should not start with a leading '.'.
-cookie_domains: demo.launchpad.net, staging.launchpad.net, launchpad.net, launchpad.dev
+cookie_domains:
+    demo.launchpad.net, staging.launchpad.net, launchpad.net, launchpad.dev
 
 # Maximum size of product release download files in bytes. A value
 # of 0 means no limit.
@@ -1702,15 +1706,18 @@
 memory_profile_log:
 
 [rabbitmq]
+# Should RabbitMQ be launched by default?
+# datatype: boolean
+launch: False
 # The host:port at which RabbitMQ is listening.
 # datatype: string
 host: none
 # datatype: string
-userid: guest
-# datatype: string
-password: guest
-# datatype: string
-virtual_host: /
+userid: none
+# datatype: string
+password: none
+# datatype: string
+virtual_host: none
 
 [reclaimbranchspace]
 # The database user which will be used by this process.
 
=== modified file 'lib/canonical/launchpad/scripts/runlaunchpad.py'
--- lib/canonical/launchpad/scripts/runlaunchpad.py	2011-03-07 16:32:12 +0000
+++ lib/canonical/launchpad/scripts/runlaunchpad.py	2011-09-15 19:55:38 +0000
@@ -14,6 +14,8 @@
 import sys
 
 import fixtures
+from lazr.config import as_host_port
+from rabbitfixture.server import RabbitServerResources
 from zope.app.server.main import main
 
 from canonical.config import config
@@ -22,9 +24,10 @@
     make_pidfile,
     pidfile_path,
     )
+from lp.services.googlesearch import googletestservice
 from lp.services.mailman import runmailman
 from lp.services.osutils import ensure_directory_exists
-from lp.services.googlesearch import googletestservice
+from lp.services.rabbit.server import RabbitServer
 
 
 def make_abspath(path):
@@ -221,6 +224,20 @@
         process.stdin.close()
 
 
+class RabbitService(Service):
+    """A RabbitMQ service."""
+
+    @property
+    def should_launch(self):
+        return config.rabbitmq.launch
+
+    def launch(self):
+        hostname, port = as_host_port(config.rabbitmq.host, None, None)
+        self.server = RabbitServer(
+            RabbitServerResources(hostname=hostname, port=port))
+        self.useFixture(self.server)
+
+
 def stop_process(process):
     """kill process and BLOCK until process dies.
 
@@ -245,6 +262,7 @@
     'codebrowse': CodebrowseService(),
     'google-webservice': GoogleWebService(),
     'memcached': MemcachedService(),
+    'rabbitmq': RabbitService(),
     }
 
 
@@ -287,8 +305,8 @@
     """
     if '-i' in args:
         index = args.index('-i')
-        config.setInstance(args[index+1])
-        del args[index:index+2]
+        config.setInstance(args[index + 1])
+        del args[index:index + 2]
 
     if '-C' not in args:
         zope_config_file = config.zope_config_file
@@ -319,10 +337,10 @@
         if config.launchpad.launch:
             main(argv)
         else:
-            # We just need the foreground process to sit around forever waiting
-            # for the signal to shut everything down.  Normally, Zope itself would
-            # be this master process, but we're not starting that up, so we need
-            # to do something else.
+            # We just need the foreground process to sit around forever
+            # waiting for the signal to shut everything down.  Normally, Zope
+            # itself would be this master process, but we're not starting that
+            # up, so we need to do something else.
             try:
                 signal.pause()
             except KeyboardInterrupt:
 
=== modified file 'lib/canonical/launchpad/scripts/tests/test_runlaunchpad.py'
--- lib/canonical/launchpad/scripts/tests/test_runlaunchpad.py	2010-10-11 02:37:27 +0000
+++ lib/canonical/launchpad/scripts/tests/test_runlaunchpad.py	2011-09-15 19:55:38 +0000
@@ -152,6 +152,10 @@
         if config.google_test_service.launch:
             expected.append(SERVICES['google-webservice'])
 
+        # RabbitMQ may or may not be asked to run.
+        if config.rabbitmq.launch:
+            expected.append(SERVICES['rabbitmq'])
+
         expected = sorted(expected)
         self.assertEqual(expected, services)
 
 
=== modified file 'lib/canonical/testing/layers.py'
--- lib/canonical/testing/layers.py	2011-09-13 08:44:05 +0000
+++ lib/canonical/testing/layers.py	2011-09-15 19:55:38 +0000
@@ -143,13 +143,13 @@
 import lp.services.mail.stub
 from lp.services.memcache.client import memcache_client_factory
 from lp.services.osutils import kill_by_pidfile
+from lp.services.rabbit.server import RabbitServer
 from lp.testing import (
     ANONYMOUS,
     is_logged_in,
     login,
     logout,
     )
-from lp.testing.fixture import RabbitServer
 from lp.testing.pgsql import PgTestSetup
 
 
 
=== added directory 'lib/lp/services/rabbit'
=== added file 'lib/lp/services/rabbit/__init__.py'
=== added file 'lib/lp/services/rabbit/server.py'
--- lib/lp/services/rabbit/server.py	1970-01-01 00:00:00 +0000
+++ lib/lp/services/rabbit/server.py	2011-09-15 19:55:38 +0000
@@ -0,0 +1,31 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""RabbitMQ server fixture."""
+
+__metaclass__ = type
+__all__ = [
+    'RabbitServer',
+    ]
+
+from textwrap import dedent
+
+import rabbitfixture.server
+
+
+class RabbitServer(rabbitfixture.server.RabbitServer):
+    """A RabbitMQ server fixture with Launchpad-specific config.
+
+    :ivar service_config: A snippet of .ini that describes the `rabbitmq`
+        configuration.
+    """
+
+    def setUp(self):
+        super(RabbitServer, self).setUp()
+        self.config.service_config = dedent("""\
+            [rabbitmq]
+            host: localhost:%d
+            userid: guest
+            password: guest
+            virtual_host: /
+            """ % self.config.port)
 
=== added directory 'lib/lp/services/rabbit/tests'
=== added file 'lib/lp/services/rabbit/tests/__init__.py'
=== added file 'lib/lp/services/rabbit/tests/test_server.py'
--- lib/lp/services/rabbit/tests/test_server.py	1970-01-01 00:00:00 +0000
+++ lib/lp/services/rabbit/tests/test_server.py	2011-09-15 19:55:38 +0000
@@ -0,0 +1,40 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for lp.testing.fixture."""
+
+__metaclass__ = type
+
+from ConfigParser import SafeConfigParser
+from StringIO import StringIO
+
+from fixtures import EnvironmentVariableFixture
+
+from canonical.testing.layers import BaseLayer
+from lp.services.rabbit.server import RabbitServer
+from lp.testing import TestCase
+
+
+class TestRabbitServer(TestCase):
+
+    layer = BaseLayer
+
+    def test_service_config(self):
+        # Rabbit needs to fully isolate itself: an existing per user
+        # .erlang.cookie has to be ignored, and ditto bogus HOME if other
+        # tests fail to cleanup.
+        self.useFixture(EnvironmentVariableFixture('HOME', '/nonsense/value'))
+
+        # RabbitServer pokes some .ini configuration into its config.
+        fixture = self.useFixture(RabbitServer())
+        service_config = SafeConfigParser()
+        service_config.readfp(StringIO(fixture.config.service_config))
+        self.assertEqual(["rabbitmq"], service_config.sections())
+        expected = {
+            "host": "localhost:%d" % fixture.config.port,
+            "userid": "guest",
+            "password": "guest",
+            "virtual_host": "/",
+            }
+        observed = dict(service_config.items("rabbitmq"))
+        self.assertEqual(expected, observed)
 
=== modified file 'lib/lp/testing/fixture.py'
--- lib/lp/testing/fixture.py	2011-09-05 16:39:46 +0000
+++ lib/lp/testing/fixture.py	2011-09-15 19:55:38 +0000
@@ -5,7 +5,6 @@
 
 __metaclass__ = type
 __all__ = [
-    'RabbitServer',
     'ZopeAdapterFixture',
     'ZopeEventHandlerFixture',
     'ZopeViewReplacementFixture',
@@ -13,14 +12,12 @@
 
 from ConfigParser import SafeConfigParser
 import os.path
-from textwrap import dedent
 
 from fixtures import (
     EnvironmentVariableFixture,
     Fixture,
     )
 import pgbouncer.fixture
-import rabbitfixture.server
 from zope.component import (
     getGlobalSiteManager,
     provideHandler,
@@ -36,24 +33,6 @@
 from canonical.config import config
 
 
-class RabbitServer(rabbitfixture.server.RabbitServer):
-    """A RabbitMQ server fixture with Launchpad-specific config.
-
-    :ivar service_config: A snippet of .ini that describes the `rabbitmq`
-        configuration.
-    """
-
-    def setUp(self):
-        super(RabbitServer, self).setUp()
-        self.config.service_config = dedent("""\
-            [rabbitmq]
-            host: localhost:%d
-            userid: guest
-            password: guest
-            virtual_host: /
-            """ % self.config.port)
-
-
 class PGBouncerFixture(pgbouncer.fixture.PGBouncerFixture):
     """Inserts a controllable pgbouncer instance in front of PostgreSQL.
 
 
=== modified file 'lib/lp/testing/tests/test_fixture.py'
--- lib/lp/testing/tests/test_fixture.py	2011-09-05 15:42:27 +0000
+++ lib/lp/testing/tests/test_fixture.py	2011-09-15 19:55:38 +0000
@@ -5,9 +5,6 @@
 
 __metaclass__ = type
 
-from textwrap import dedent
-
-from fixtures import EnvironmentVariableFixture
 import psycopg2
 from storm.exceptions import DisconnectionError
 from zope.component import (
@@ -30,33 +27,10 @@
 from lp.testing import TestCase
 from lp.testing.fixture import (
     PGBouncerFixture,
-    RabbitServer,
     ZopeAdapterFixture,
     )
 
 
-class TestRabbitServer(TestCase):
-
-    layer = BaseLayer
-
-    def test_service_config(self):
-        # Rabbit needs to fully isolate itself: an existing per user
-        # .erlange.cookie has to be ignored, and ditto bogus HOME if other
-        # tests fail to cleanup.
-        self.useFixture(EnvironmentVariableFixture('HOME', '/nonsense/value'))
-
-        # RabbitServer pokes some .ini configuration into its config.
-        with RabbitServer() as fixture:
-            expected = dedent("""\
-                [rabbitmq]
-                host: localhost:%d
-                userid: guest
-                password: guest
-                virtual_host: /
-                """ % fixture.config.port)
-            self.assertEqual(expected, fixture.config.service_config)
-
-
 class IFoo(Interface):
     pass