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

[Jeroen T. Vermeulen]

Pass cluster uuid to pxeconfig API call, so it knows which cluster a request comes from.

The pxeconfig call used to figure out which cluster it should write a config for by inspecting the requesting IP address. That won't work in the general case because the request is (now) made by the cluster controller, from an IP address that isn't on any cluster-managed network interface.

Discussed this with Julian. We really want to avoid dragging celery config into the tftpd process, but that's where we keep a cluster's uuid at the moment. The plan is to move that into maas_cluster.conf, but in a separate branch. It's nontrivial yak-shaving. This does mean that this branch by itself won't solve the problem for cluster controllers that aren't running on the region controller: by default, the process will still read the default celery config rather than the one we manage for cluster controllers. Setting the right environment variable in the pserv startup job might fix that, but we're planning to go with the "nicer" fix.

Jeroen

[Julian Edwards (julian-edwards)]

This wasn't *quite* what I had in mind in our pre-imp, but it's a reasonable start. I was hoping that you'd fix the pserv so it gets the UUID value from a new (duplicate) location in the /etc/maas/maas_cluster.conf file, and then clean up the *duplication* later.

8 +def find_cluster_for_pxeconfig_request(request):
...
15 + return find_nodegroup(request)

We really need to be more consistent with naming; "cluster" externally and "nodegroup" internally. In fact, cluster really refers to the whole set of processes running on that edifice, whereas nodegroup is the name of a model.

Therefore I'd change that function to:
def find_nodegroup_for_pxeconfig_request(request)

review: Approve

[Jeroen T. Vermeulen (jtv)]

I changed the name of find_cluster_for_pxeconfig_request. I'm sorry about the misunderstanding about when to duplicate the CLUSTER_UUID setting: the work to make it happen is not entirely trivial because an installation may already have a setting in the celeryconfig but not in the cluster config, or it may have both, or neither. All in untestable shell scripts, of course. I really wanted to keep that to a separate branch.

[Raphaël Badin (rvb)]

'find_nodegroup_for_pxeconfig_request_uses_cluster_uuid' is not a test because it does not start with 'test' and the method it tests, 'find_nodegroup_for_pxeconfig_request' does not seem to be looking for the param 'cluster_uuid' inside the request… What am I missing?

review: Needs Information

[Raphaël Badin (rvb)]

2 more things about 'RequestFactory.get(reverse('pxeconfig'), **params)':

- RequestFactory is, well, a factory and is supposed to be instantiated before being used (see https://docs.djangoproject.com/en/dev/topics/testing/#django.test.client.RequestFactory)

- RequestFactory().get('path', **params) will put the params in request.META; I don't think that's what you want here.

[Raphaël Badin (rvb)]

Happy to be told otherwise, but I think we need this: http://paste.ubuntu.com/1427119/

[Jeroen T. Vermeulen (jtv)]

Thanks Raphaël. Reported (and fixed) that as bug 1089464.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-11-29 11:30:37 +0000
+++ src/maasserver/api.py	2012-12-04 07:37:22 +0000
@@ -1598,6 +1598,16 @@
     return macaddress.node if macaddress else None
 
 
+def find_nodegroup_for_pxeconfig_request(request):
+    """Find the nodegroup responsible for a `pxeconfig` request.
+
+    Looks for the `cluster_uuid` parameter in the request.  If there is
+    none, figures it out based on the requesting IP as a compatibility
+    measure.  In that case, the result may be incorrect.
+    """
+    return find_nodegroup(request)
+
+
 def pxeconfig(request):
     """Get the PXE configuration given a node's details.
 
@@ -1621,6 +1631,10 @@
     :param subarch: Subarchitecture name (in the pxelinux namespace).
     :param local: The IP address of the cluster controller.
     :param remote: The IP address of the booting node.
+    :param cluster_uuid: UUID of the cluster responsible for this node.
+        If omitted, the call will attempt to figure it out based on the
+        requesting IP address, for compatibility.  Passing `cluster_uuid`
+        is preferred.
     """
     node = get_node_from_mac_string(request.GET.get('mac', None))
 
@@ -1665,7 +1679,7 @@
             # 1-1 mapping.
             subarch = pxelinux_subarch
 
-        nodegroup = find_nodegroup(request)
+        nodegroup = find_nodegroup_for_pxeconfig_request(request)
         preseed_url = compose_enlistment_preseed_url(nodegroup=nodegroup)
         hostname = 'maas-enlist'
         domain = Config.objects.get_config('enlistment_domain')

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-11-28 15:37:34 +0000
+++ src/maasserver/tests/test_api.py	2012-12-04 07:37:22 +0000
@@ -56,6 +56,7 @@
     describe,
     DISPLAYED_NODEGROUP_FIELDS,
     extract_constraints,
+    find_nodegroup_for_pxeconfig_request,
     store_node_power_parameters,
     )
 from maasserver.enum import (
@@ -3361,7 +3362,7 @@
             self.get_pxeconfig(),
             ContainsAll(KernelParameters._fields))
 
-    def test_pxeconfig_returns_data_for_known_node(self):
+    def test_pxeconfig_returns_success_for_known_node(self):
         params = self.get_mac_params()
         response = self.client.get(reverse('pxeconfig'), params)
         self.assertEqual(httplib.OK, response.status_code)
@@ -3371,7 +3372,7 @@
         response = self.client.get(reverse('pxeconfig'), params)
         self.assertEqual(httplib.NO_CONTENT, response.status_code)
 
-    def test_pxeconfig_returns_data_for_detailed_but_unknown_node(self):
+    def test_pxeconfig_returns_success_for_detailed_but_unknown_node(self):
         architecture = factory.getRandomEnum(ARCHITECTURE)
         arch, subarch = architecture.split('/')
         params = dict(
@@ -3484,6 +3485,19 @@
             compose_preseed_url(node),
             json.loads(response.content)["preseed_url"])
 
+    def find_nodegroup_for_pxeconfig_request_uses_cluster_uuid(self):
+        # find_nodegroup_for_pxeconfig_request returns the nodegroup
+        # identified by the cluster_uuid parameter, if given.  It
+        # completely ignores the other node or request details, as shown
+        # here by passing a uuid for a different cluster.
+        params = self.get_mac_params()
+        nodegroup = factory.make_node_group()
+        params['cluster_uuid'] = nodegroup.uuid
+        request = RequestFactory.get(reverse('pxeconfig'), **params)
+        self.assertEqual(
+            nodegroup,
+            find_nodegroup_for_pxeconfig_request(request))
+
     def test_preseed_url_for_known_node_uses_nodegroup_maas_url(self):
         ng_url = 'http://%s' % factory.make_name('host')
         network = IPNetwork("10.1.1/24")

=== modified file 'src/provisioningserver/tests/test_tftp.py'
--- src/provisioningserver/tests/test_tftp.py	2012-11-08 06:34:48 +0000
+++ src/provisioningserver/tests/test_tftp.py	2012-12-04 07:37:22 +0000
@@ -23,6 +23,7 @@
 
 from maastesting.factory import factory
 from maastesting.testcase import TestCase
+from provisioningserver import tftp as tftp_module
 from provisioningserver.pxe.tftppath import compose_config_path
 from provisioningserver.tests.test_kernel_opts import make_kernel_parameters
 from provisioningserver.tftp import (
@@ -211,6 +212,9 @@
     def test_get_reader_config_file(self):
         # For paths matching re_config_file, TFTPBackend.get_reader() returns
         # a Deferred that will yield a BytesReader.
+        cluster_uuid = factory.getRandomUUID()
+        self.patch(tftp_module, 'get_cluster_uuid').return_value = (
+            cluster_uuid)
         mac = factory.getRandomMACAddress(b"-")
         config_path = compose_config_path(mac)
         backend = TFTPBackend(self.make_dir(), b"http://example.com/")
@@ -240,6 +244,7 @@
             "mac": mac,
             "local": call_context["local"][0],  # address only.
             "remote": call_context["remote"][0],  # address only.
+            "cluster_uuid": cluster_uuid,
             }
         observed_params = json.loads(output)
         self.assertEqual(expected_params, observed_params)

=== modified file 'src/provisioningserver/tftp.py'
--- src/provisioningserver/tftp.py	2012-11-08 06:34:48 +0000
+++ src/provisioningserver/tftp.py	2012-12-04 07:37:22 +0000
@@ -28,6 +28,7 @@
 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,
@@ -217,6 +218,10 @@
             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)
             return d