Merge lp:~jtv/maas/bug-1086239 into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

Take cluster-uuid setting from environment, not celery config.

This builds on a multi-step change agreed with Julian. The relevant part is:
 1 Duplicate the CLUSTER_UUID setting, from Celery config to maas_cluster.conf.
 2 Make pserv's upstart job source maas_cluster.conf.
 3 Have get_cluster_uuid read the setting from the environment instead of from celery.
 4 Clean up the duplicated setting.

What you see here is step 3. It also puts get_cluster_uuid in a saner place. That function was "borrowed" from a place where nobody should be importing it from. Not so bad for a provisional fix, but this is the cleanup.

Finally, the cluster uuid is no longer one of the "worker secrets" that the region controller sends to the clusters. So this also removes some of the remnants of that old situation. In particular, "I don't know my cluster uuid yet" is no longer a valid reason for cluster controllers to be unable to perform tasks. Now, if the uuid is not set, either installation failed in some unusual way or we're running something that needs the information without sourcing maas_cluster.conf. There may be some nooks and crannies where this may still need doing, so I tried to make the failure a helpful one.

All of this work is to be backported to the 1.2 branch.

Jeroen

[Gavin Panella (allenap)]

review: Approve

[MAAS Lander (maas-lander)]

The Jenkins job https://jenkins.qa.ubuntu.com/job/maas-merger-trunk/129/console reported an error when processing this lp:~jtv/maas/bug-1086239 branch.
Not merging it.

[Jeroen T. Vermeulen (jtv)]

Weird Jenkins failure. I'll try again. I also snuck in a small change for the part I forgot to cover: running a demo cluster controller in a source branch.

[MAAS Lander (maas-lander)]

The Jenkins job https://jenkins.qa.ubuntu.com/job/maas-merger-trunk/130/console reported an error when processing this lp:~jtv/maas/bug-1086239 branch.
Not merging it.

=== modified file 'etc/democeleryconfig_cluster.py'
--- etc/democeleryconfig_cluster.py	2012-10-23 12:48:31 +0000
+++ etc/democeleryconfig_cluster.py	2012-12-11 13:35:25 +0000
@@ -26,10 +26,6 @@
 
 import_settings(democeleryconfig_common)
 
-# Set a fixed CLUSTER_UUID.  In production, this is taken from
-# maas_local_celeryconfig.
-CLUSTER_UUID = "adfd3977-f251-4f2c-8d61-745dbd690bfc"
-
 CELERYD_LOG_FILE = os.path.join(
     DEV_ROOT_DIRECTORY, 'logs/cluster-worker/current')
 
 
=== modified file 'services/cluster-worker/run'
--- services/cluster-worker/run	2012-10-03 11:39:37 +0000
+++ services/cluster-worker/run	2012-12-11 13:35:25 +0000
@@ -16,6 +16,11 @@
 
 export PYTHONPATH=etc/:src/
 export CELERY_CONFIG_MODULE=democeleryconfig_cluster
+
+# Set a fixed cluster UUID.  This is identical for all cluster services
+# running in the demo setup: they're all the same cluster.
+export CLUSTER_UUID="adfd3977-f251-4f2c-8d61-745dbd690bfc"
+
 script="$(readlink -f bin/maas-provision)"
 exec fghack "${script}" start-cluster-controller \
     http://0.0.0.0:5240/ -u "$(id -un)" -g "$(id -gn)"
 
=== modified file 'services/pserv/run'
--- services/pserv/run	2012-09-26 14:16:33 +0000
+++ services/pserv/run	2012-12-11 13:35:25 +0000
@@ -17,5 +17,10 @@
 # Exec the Provisioning Server.
 script="$(readlink -f bin/twistd.pserv)"
 config="$(readlink -f etc/pserv.yaml)"
+
+# Set a fixed cluster UUID.  This is identical for all cluster services
+# running in the demo setup: they're all the same cluster.
+export CLUSTER_UUID="adfd3977-f251-4f2c-8d61-745dbd690bfc"
+
 exec $(command -v authbind && echo --deep) \
     "${script}" --nodaemon --pidfile="" maas-pserv --config-file "${config}"
 
=== modified file 'src/provisioningserver/auth.py'
--- src/provisioningserver/auth.py	2012-11-23 01:42:03 +0000
+++ src/provisioningserver/auth.py	2012-12-11 13:35:25 +0000
@@ -13,13 +13,10 @@
 __all__ = [
     'get_recorded_api_credentials',
     'get_recorded_nodegroup_uuid',
-    'get_recorded_maas_url',
     'record_api_credentials',
     'record_nodegroup_uuid',
     ]
 
-import os
-
 from apiclient.creds import convert_string_to_tuple
 from provisioningserver import cache
 
@@ -30,11 +27,6 @@
 NODEGROUP_UUID_CACHE_KEY = 'nodegroup_uuid'
 
 
-def get_recorded_maas_url():
-    """Return the base URL for the MAAS server."""
-    return os.environ.get("MAAS_URL")
-
-
 def record_api_credentials(api_credentials):
     """Update the recorded API credentials.
 
 
=== modified file 'src/provisioningserver/boot_images.py'
--- src/provisioningserver/boot_images.py	2012-11-16 11:25:07 +0000
+++ src/provisioningserver/boot_images.py	2012-12-11 13:35:25 +0000
@@ -26,13 +26,13 @@
     MAASDispatcher,
     MAASOAuth,
     )
-from provisioningserver.auth import (
-    get_recorded_api_credentials,
-    get_recorded_maas_url,
+from provisioningserver.auth import get_recorded_api_credentials
+from provisioningserver.cluster_config import (
+    get_cluster_uuid,
+    get_maas_url,
     )
 from provisioningserver.config import Config
 from provisioningserver.pxe import tftppath
-from provisioningserver.start_cluster_controller import get_cluster_uuid
 
 
 logger = getLogger(__name__)
@@ -44,7 +44,7 @@
     :return: Tuple of cached items: (maas_url, api_credentials).  Either may
         be None if the information has not been received from the server yet.
     """
-    maas_url = get_recorded_maas_url()
+    maas_url = get_maas_url()
     if maas_url is None:
         logger.debug("Not reporting boot images: don't have API URL yet.")
     api_credentials = get_recorded_api_credentials()
 
=== added file 'src/provisioningserver/cluster_config.py'
--- src/provisioningserver/cluster_config.py	1970-01-01 00:00:00 +0000
+++ src/provisioningserver/cluster_config.py	2012-12-11 13:35:25 +0000
@@ -0,0 +1,44 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Accessors for cluster configuration as set in `maas_cluster.conf`."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = [
+    'get_cluster_uuid',
+    'get_maas_url',
+    ]
+
+from os import environ
+
+
+def get_cluster_variable(var):
+    """Obtain the given environment variable from maas_cluster.conf.
+
+    If the variable is not set, it probably means that whatever script
+    started the current process neglected to run maas_cluster.conf.
+    In that case, fail helpfully but utterly.
+    """
+    value = environ.get(var)
+    if value is None:
+        raise AssertionError(
+            "%s is not set.  This probably means that the script which "
+            "started this program failed to source maas_cluster.conf."
+            % var)
+    return value
+
+
+def get_cluster_uuid():
+    """Return the `CLUSTER_UUID` setting."""
+    return get_cluster_variable('CLUSTER_UUID')
+
+
+def get_maas_url():
+    """Return the `MAAS_URL` setting."""
+    return get_cluster_variable('MAAS_URL')
 
=== modified file 'src/provisioningserver/dhcp/leases.py'
--- src/provisioningserver/dhcp/leases.py	2012-11-16 11:25:07 +0000
+++ src/provisioningserver/dhcp/leases.py	2012-12-11 13:35:25 +0000
@@ -49,9 +49,9 @@
 from provisioningserver import cache
 from provisioningserver.auth import (
     get_recorded_api_credentials,
-    get_recorded_maas_url,
     get_recorded_nodegroup_uuid,
     )
+from provisioningserver.cluster_config import get_maas_url
 from provisioningserver.dhcp.leases_parser import parse_leases
 
 
@@ -150,7 +150,7 @@
     """Send lease updates to the server API."""
     # Items that the server must have sent us before we can do this.
     knowledge = {
-        'maas_url': get_recorded_maas_url(),
+        'maas_url': get_maas_url(),
         'api_credentials': get_recorded_api_credentials(),
         'nodegroup_uuid': get_recorded_nodegroup_uuid(),
     }
 
=== modified file 'src/provisioningserver/dhcp/tests/test_leases.py'
--- src/provisioningserver/dhcp/tests/test_leases.py	2012-11-26 02:24:46 +0000
+++ src/provisioningserver/dhcp/tests/test_leases.py	2012-12-11 13:35:25 +0000
@@ -328,15 +328,6 @@
             (get_write_time(leases_file), {params['ip']: params['mac']}),
             parse_leases_file())
 
-    def test_send_leases_does_nothing_without_maas_url(self):
-        self.patch(MAASClient, 'post', FakeMethod())
-        self.set_lease_state()
-        self.set_items_needed_for_lease_update()
-        self.clear_maas_url()
-        leases = factory.make_random_leases()
-        send_leases(leases)
-        self.assertEqual([], MAASClient.post.calls)
-
     def test_send_leases_does_nothing_without_credentials(self):
         self.patch(MAASClient, 'post', FakeMethod())
         self.set_items_needed_for_lease_update()
 
=== modified file 'src/provisioningserver/start_cluster_controller.py'
--- src/provisioningserver/start_cluster_controller.py	2012-11-23 01:42:03 +0000
+++ src/provisioningserver/start_cluster_controller.py	2012-12-11 13:35:25 +0000
@@ -32,6 +32,7 @@
     MAASDispatcher,
     NoAuth,
     )
+from provisioningserver.cluster_config import get_cluster_uuid
 from provisioningserver.network import discover_networks
 
 
@@ -65,14 +66,6 @@
     return MAASClient(NoAuth(), MAASDispatcher(), server_url)
 
 
-def get_cluster_uuid():
-    """Read this cluster's UUID from the config."""
-    # Import this lazily.  It reads config as a side effect, which can
-    # produce warnings.
-    from celery.app import app_or_default
-    return app_or_default().conf.CLUSTER_UUID
-
-
 def register(server_url):
     """Request Rabbit connection details from the domain controller.
 
 
=== modified file 'src/provisioningserver/tags.py'
--- src/provisioningserver/tags.py	2012-11-16 11:25:07 +0000
+++ src/provisioningserver/tags.py	2012-12-11 13:35:25 +0000
@@ -30,9 +30,9 @@
 from lxml import etree
 from provisioningserver.auth import (
     get_recorded_api_credentials,
-    get_recorded_maas_url,
     get_recorded_nodegroup_uuid,
     )
+from provisioningserver.cluster_config import get_maas_url
 import simplejson as json
 
 
@@ -51,10 +51,6 @@
 
     :return: (client, nodegroup_uuid)
     """
-    maas_url = get_recorded_maas_url()
-    if maas_url is None:
-        logger.error("Not updating tags: don't have API URL yet.")
-        return None, None
     api_credentials = get_recorded_api_credentials()
     if api_credentials is None:
         logger.error("Not updating tags: don't have API key yet.")
@@ -64,7 +60,7 @@
         logger.error("Not updating tags: don't have UUID yet.")
         return None, None
     client = MAASClient(MAASOAuth(*api_credentials), MAASDispatcher(),
-        maas_url)
+        get_maas_url())
     return client, nodegroup_uuid
 
 
 
=== modified file 'src/provisioningserver/tests/test_auth.py'
--- src/provisioningserver/tests/test_auth.py	2012-11-26 14:41:53 +0000
+++ src/provisioningserver/tests/test_auth.py	2012-12-11 13:35:25 +0000
@@ -14,8 +14,6 @@
 
 from apiclient.creds import convert_tuple_to_string
 from apiclient.testing.credentials import make_api_credentials
-from fixtures import EnvironmentVariableFixture
-from maastesting.factory import factory
 from provisioningserver import (
     auth,
     cache,
@@ -38,13 +36,3 @@
 
     def test_get_recorded_api_credentials_returns_None_without_creds(self):
         self.assertIsNone(auth.get_recorded_api_credentials())
-
-    def test_get_recorded_nodegroup_uuid_vs_record_nodegroup_uuid(self):
-        nodegroup_uuid = factory.make_name('nodegroupuuid')
-        auth.record_nodegroup_uuid(nodegroup_uuid)
-        self.assertEqual(nodegroup_uuid, auth.get_recorded_nodegroup_uuid())
-
-    def test_get_recorded_maas_url_uses_environment_override(self):
-        required_url = factory.make_name("MAAS_URL")
-        self.useFixture(EnvironmentVariableFixture("MAAS_URL", required_url))
-        self.assertEqual(required_url, auth.get_recorded_maas_url())
 
=== modified file 'src/provisioningserver/tests/test_boot_images.py'
--- src/provisioningserver/tests/test_boot_images.py	2012-11-26 02:24:46 +0000
+++ src/provisioningserver/tests/test_boot_images.py	2012-12-11 13:35:25 +0000
@@ -12,7 +12,6 @@
 __metaclass__ = type
 __all__ = []
 
-import os
 import json
 
 from apiclient.maas_client import MAASClient
@@ -46,15 +45,6 @@
         self.assertIs(sentinel.uuid, kwargs["nodegroup"])
         self.assertItemsEqual([image], json.loads(kwargs['images']))
 
-    def test_does_nothing_without_maas_url(self):
-        os.environ.pop("MAAS_URL")  # Ensure nothing is set.
-        self.set_api_credentials()
-        self.patch(
-            tftppath, 'list_boot_images',
-            Mock(return_value=make_boot_image_params()))
-        boot_images.report_to_server()
-        self.assertItemsEqual([], tftppath.list_boot_images.call_args_list)
-
     def test_does_nothing_without_credentials(self):
         self.set_maas_url()
         self.patch(
 
=== added file 'src/provisioningserver/tests/test_cluster_config.py'
--- src/provisioningserver/tests/test_cluster_config.py	1970-01-01 00:00:00 +0000
+++ src/provisioningserver/tests/test_cluster_config.py	2012-12-11 13:35:25 +0000
@@ -0,0 +1,46 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for `provisioningserver.cluster_config`."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = []
+
+from fixtures import EnvironmentVariableFixture
+from maastesting.factory import factory
+from maastesting.testcase import TestCase
+from provisioningserver.cluster_config import (
+    get_cluster_uuid,
+    get_cluster_variable,
+    get_maas_url,
+    )
+
+
+class TestClusterConfig(TestCase):
+
+    def test_get_cluster_variable_reads_env(self):
+        var = factory.make_name('variable')
+        value = factory.make_name('value')
+        self.useFixture(EnvironmentVariableFixture(var, value))
+        self.assertEqual(value, get_cluster_variable(var))
+
+    def test_get_cluster_variable_fails_if_not_set(self):
+        self.assertRaises(
+            AssertionError,
+            get_cluster_variable, factory.make_name('nonexistent-variable'))
+
+    def test_get_cluster_uuid_reads_CLUSTER_UUID(self):
+        uuid = factory.make_name('uuid')
+        self.useFixture(EnvironmentVariableFixture('CLUSTER_UUID', uuid))
+        self.assertEqual(uuid, get_cluster_uuid())
+
+    def test_get_maas_url_reads_MAAS_URL(self):
+        maas_url = factory.make_name('maas_url')
+        self.useFixture(EnvironmentVariableFixture('MAAS_URL', maas_url))
+        self.assertEqual(maas_url, get_maas_url())
 
=== modified file 'src/provisioningserver/tftp.py'
--- src/provisioningserver/tftp.py	2012-12-04 05:02:05 +0000
+++ src/provisioningserver/tftp.py	2012-12-11 13:35:25 +0000
@@ -25,10 +25,10 @@
     urlparse,
     )
 
+from provisioningserver.cluster_config import get_cluster_uuid
 from provisioningserver.enum import ARP_HTYPE
 from provisioningserver.kernel_opts import KernelParameters
 from provisioningserver.pxe.config import render_pxe_config
-from provisioningserver.start_cluster_controller import get_cluster_uuid
 from provisioningserver.utils import deferred
 from tftp.backend import (
     FilesystemSynchronousBackend,
@@ -218,9 +218,6 @@
             params["local"] = local_host
             remote_host, remote_port = get("remote", (None, None))
             params["remote"] = remote_host
-            # XXX 2012-12-04 JeroenVermeulen bug=1086239: move
-            # get_cluster_uuid to a properly reusable location, and
-            # implement it without the celery import (and side effects).
             params["cluster_uuid"] = get_cluster_uuid()
             d = self.get_config_reader(params)
             d.addErrback(self.get_page_errback, file_name)