launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #14773
[Merge] lp:~jtv/maas/bug-1086239 into lp:maas
Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/bug-1086239 into lp:maas.
Commit message:
Take cluster-uuid setting from environment, not celery config.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1086239 in MAAS: "CLUSTER_UUID is in cluster celery config, but tftpd needs it"
https://bugs.launchpad.net/maas/+bug/1086239
For more details, see:
https://code.launchpad.net/~jtv/maas/bug-1086239/+merge/139172
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
--
https://code.launchpad.net/~jtv/maas/bug-1086239/+merge/139172
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/bug-1086239 into lp:maas.
=== modified file 'src/provisioningserver/auth.py'
--- src/provisioningserver/auth.py 2012-11-23 01:42:03 +0000
+++ src/provisioningserver/auth.py 2012-12-11 11:46:21 +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 11:46:21 +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 11:46:21 +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 11:46:21 +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 11:46:21 +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 11:46:21 +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 11:46:21 +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 11:46:21 +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 11:46:21 +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 11:46:21 +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 11:46:21 +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)
