launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/bug-1084507 into lp:maas.

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1084507 in MAAS: "When pserv requests the pxeconfig from the region, the context nodegroup is not detected correctly."
  https://bugs.launchpad.net/maas/+bug/1084507

For more details, see:
https://code.launchpad.net/~jtv/maas/bug-1084507/+merge/137755

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
-- 
https://code.launchpad.net/~jtv/maas/bug-1084507/+merge/137755
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/bug-1084507 into lp:maas.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-11-29 11:30:37 +0000
+++ src/maasserver/api.py	2012-12-04 05:10:04 +0000
@@ -1598,6 +1598,16 @@
     return macaddress.node if macaddress else None
 
 
+def find_cluster_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_cluster_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 05:10:04 +0000
@@ -56,6 +56,7 @@
     describe,
     DISPLAYED_NODEGROUP_FIELDS,
     extract_constraints,
+    find_cluster_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_cluster_for_pxeconfig_request_uses_cluster_uuid(self):
+        # find_cluster_for_pxeconfig_request returns the cluster
+        # 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_cluster_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 05:10:04 +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 05:10:04 +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