launchpad-reviewers team mailing list archive

[Julian Edwards <julian.edwards@xxxxxxxxxxxxx>]

Julian Edwards has proposed merging lp:~julian-edwards/maas/cluster-wrong-address-bug-1081688 into lp:maas.

Commit message:
Start_cluster_controller now sets the environment variable MAAS_URL to be the configured URL to the region controller, which is picked up inside refresh secrets and used to override the value passed from the region.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1081688 in MAAS: "The cluster uses the wrong address to contact the region controller"
  https://bugs.launchpad.net/maas/+bug/1081688

For more details, see:
https://code.launchpad.net/~julian-edwards/maas/cluster-wrong-address-bug-1081688/+merge/135596

I could not think of a better way of doing this.  Passing values into celery isn't really possible.
-- 
https://code.launchpad.net/~julian-edwards/maas/cluster-wrong-address-bug-1081688/+merge/135596
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/maas/cluster-wrong-address-bug-1081688 into lp:maas.

=== modified file 'src/provisioningserver/auth.py'
--- src/provisioningserver/auth.py	2012-09-10 14:27:00 +0000
+++ src/provisioningserver/auth.py	2012-11-22 07:15:24 +0000
@@ -18,6 +18,8 @@
     'record_nodegroup_uuid',
     ]
 
+import os
+
 from apiclient.creds import convert_string_to_tuple
 from provisioningserver import cache
 
@@ -33,6 +35,10 @@
 
 def record_maas_url(maas_url):
     """Record the MAAS server URL as sent by the server."""
+    # If MAAS_URL exists in the environment, use that instead.
+    env_value =  os.environ.get("MAAS_URL")
+    if env_value is not None:
+        maas_url = env_value
     cache.cache.set(MAAS_URL_CACHE_KEY, maas_url)
 
 

=== modified file 'src/provisioningserver/start_cluster_controller.py'
--- src/provisioningserver/start_cluster_controller.py	2012-11-16 11:27:48 +0000
+++ src/provisioningserver/start_cluster_controller.py	2012-11-22 07:15:24 +0000
@@ -122,13 +122,14 @@
         raise AssertionError("Unexpected return code: %r" % status_code)
 
 
-def start_celery(connection_details, user, group):
+def start_celery(server_url, connection_details, user, group):
     broker_url = connection_details['BROKER_URL']
     uid = getpwnam(user).pw_uid
     gid = getgrnam(group).gr_gid
 
     # Copy environment, but also tell celeryd what broker to listen to.
-    env = dict(os.environ, CELERY_BROKER_URL=broker_url)
+    env = dict(
+        os.environ, CELERY_BROKER_URL=broker_url, MAAS_URL=server_url)
     command = 'celeryd', '--beat', '--queues', get_cluster_uuid()
 
     # Change gid first, just in case changing the uid might deprive
@@ -160,7 +161,7 @@
     # in our queue.  Even if we're new and the queue did not exist yet,
     # the arriving task will create the queue.
     request_refresh(server_url)
-    start_celery(connection_details, user=user, group=group)
+    start_celery(server_url, connection_details, user=user, group=group)
 
 
 def set_up_logging():

=== modified file 'src/provisioningserver/tests/test_auth.py'
--- src/provisioningserver/tests/test_auth.py	2012-09-14 11:04:33 +0000
+++ src/provisioningserver/tests/test_auth.py	2012-11-22 07:15:24 +0000
@@ -12,6 +12,8 @@
 __metaclass__ = type
 __all__ = []
 
+import os
+
 from apiclient.creds import convert_tuple_to_string
 from apiclient.testing.credentials import make_api_credentials
 from maastesting.factory import factory
@@ -42,3 +44,16 @@
         nodegroup_uuid = factory.make_name('nodegroupuuid')
         auth.record_nodegroup_uuid(nodegroup_uuid)
         self.assertEqual(nodegroup_uuid, auth.get_recorded_nodegroup_uuid())
+
+    def test_record_maas_url_uses_environment_override(self):
+        self.addCleanup(lambda: os.environ.pop("MAAS_URL"))
+        required_url = factory.make_name("MAAS_URL")
+        unwanted_url = factory.make_name("unwanted")
+        os.environ['MAAS_URL'] = required_url
+        auth.record_maas_url(unwanted_url)
+        self.assertEqual(required_url, auth.get_recorded_maas_url())
+
+    def test_record_maas_url_uses_passed_value_if_environ_not_set(self):
+        required_url = factory.make_name("passed-value")
+        auth.record_maas_url(required_url)
+        self.assertEqual(required_url, auth.get_recorded_maas_url())

=== modified file 'src/provisioningserver/tests/test_start_cluster_controller.py'
--- src/provisioningserver/tests/test_start_cluster_controller.py	2012-11-16 14:42:50 +0000
+++ src/provisioningserver/tests/test_start_cluster_controller.py	2012-11-22 07:15:24 +0000
@@ -31,6 +31,7 @@
     )
 from maastesting.factory import factory
 from mock import (
+    ANY,
     call,
     sentinel,
     )
@@ -263,7 +264,8 @@
         self.patch(os, "setgid", setuidgid)
         self.assertRaises(
             Executing, start_cluster_controller.start_celery,
-            self.make_connection_details(), sentinel.user, sentinel.group)
+            make_url(), self.make_connection_details(), sentinel.user,
+            sentinel.group)
         # getpwname and getgrnam are used to query the passwd and group
         # databases respectively.
         self.assertEqual(
@@ -277,3 +279,19 @@
         self.assertEqual(
             [call(sentinel.gid), call(sentinel.uid)],
             setuidgid.call_args_list)
+
+    def test_start_celery_passes_environment(self):
+        server_url = make_url()
+        connection_details = self.make_connection_details()
+        self.assertRaises(
+            Executing,
+            start_cluster_controller.start_celery,
+            server_url, connection_details, factory.make_name('user'),
+            factory.make_name('group'))
+
+        env = dict(
+            os.environ,
+            CELERY_BROKER_URL=connection_details['BROKER_URL'],
+            MAAS_URL=server_url)
+        os.execvpe.assert_called_once_with(ANY, ANY, env=env)
+