launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Badin has proposed merging lp:~rvb/maas/use-fqdn into lp:maas with lp:~rvb/maas/add-fqdn as a prerequisite.

Commit message:
Use node.fqdn in the api in lieu of the hostname.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~rvb/maas/use-fqdn/+merge/131860

Use node.fqdn in the api in lieu of the hostname.

= Notes =

You might wonder why I had to do "model = Node" in AnonNodesHandler: this is to make sure that this handler is registered as the node handler used by anon requests so that the hostname replacement trick also works for anon requests. The side effect is that resource_uri is now included in the anon registration response but I think this is an improvement.
-- 
https://code.launchpad.net/~rvb/maas/use-fqdn/+merge/131860
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/use-fqdn into lp:maas.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-10-29 11:46:21 +0000
+++ src/maasserver/api.py	2012-10-29 11:46:21 +0000
@@ -478,6 +478,12 @@
     model = Node
     fields = DISPLAYED_NODE_FIELDS
 
+    # Override the 'hostname' field so that it returns the FQDN instead as
+    # this is used by Juju to reach that node.
+    @classmethod
+    def hostname(handler, node):
+        return node.fqdn
+
     def read(self, request, system_id):
         """Read a specific Node."""
         return Node.objects.get_node_or_404(
@@ -651,8 +657,15 @@
 class AnonNodesHandler(AnonymousOperationsHandler):
     """Anonymous access to Nodes."""
     create = read = update = delete = None
+    model = Node
     fields = DISPLAYED_NODE_FIELDS
 
+    # Override the 'hostname' field so that it returns the FQDN instead as
+    # this is used by Juju to reach that node.
+    @classmethod
+    def hostname(handler, node):
+        return node.fqdn
+
     @operation(idempotent=False)
     def new(self, request):
         """Create a new Node.
@@ -876,9 +889,12 @@
             request.user, NODE_PERMISSION.VIEW, ids=match_ids)
         if match_macs is not None:
             nodes = nodes.filter(macaddress__mac_address__in=match_macs)
-        # Prefetch related macaddresses and tags.
+        # Prefetch related macaddresses, tags and nodegroups (plus
+        # related interfaces).
         nodes = nodes.prefetch_related('macaddress_set__node')
         nodes = nodes.prefetch_related('tags')
+        nodes = nodes.prefetch_related('nodegroup')
+        nodes = nodes.prefetch_related('nodegroup__nodegroupinterface_set')
         return nodes.order_by('id')
 
     @operation(idempotent=True)

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2012-10-29 11:46:21 +0000
+++ src/maasserver/models/node.py	2012-10-29 11:46:21 +0000
@@ -490,7 +490,7 @@
 
     def __unicode__(self):
         if self.hostname:
-            return "%s (%s)" % (self.system_id, self.hostname)
+            return "%s (%s)" % (self.system_id, self.fqdn)
         else:
             return self.system_id
 

=== modified file 'src/maasserver/models/nodegroup.py'
--- src/maasserver/models/nodegroup.py	2012-10-11 10:59:40 +0000
+++ src/maasserver/models/nodegroup.py	2012-10-29 11:46:21 +0000
@@ -31,7 +31,6 @@
 from maasserver.models.nodegroupinterface import NodeGroupInterface
 from maasserver.models.timestampedmodel import TimestampedModel
 from maasserver.refresh_worker import refresh_worker
-from maasserver.utils.orm import get_one
 from piston.models import (
     KEY_SIZE,
     Token,
@@ -196,10 +195,13 @@
         This is a temporary method that should be refactored once we add
         proper support for multiple interfaces on a nodegroup.
         """
-        return get_one(
-            NodeGroupInterface.objects.filter(
-                nodegroup=self).exclude(
-                    management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED))
+        # Iterate over all the interfaces in python instead of doing the
+        # filtering in SQL so that this will use the cached version of
+        # self.nodegroupinterface_set if it is there.
+        for interface in self.nodegroupinterface_set.all():
+            if interface.management != NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED:
+                return interface
+        return None
 
     def ensure_dhcp_key(self):
         """Ensure that this nodegroup has a dhcp key.

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-10-29 11:46:21 +0000
+++ src/maasserver/tests/test_api.py	2012-10-29 11:46:21 +0000
@@ -637,6 +637,70 @@
         self.assertItemsEqual(['architecture'], parsed_result)
 
 
+class NodeHostnameTest(APIv10TestMixin, MultipleUsersScenarios, TestCase):
+
+    scenarios = [
+        ('user', dict(userfactory=factory.make_user)),
+        ('admin', dict(userfactory=factory.make_admin)),
+        ]
+
+    def test_GET_list_returns_fqdn_with_domain_name_from_cluster(self):
+        # If DNS management is enabled, the domain part of a hostname
+        # is replaced by the domain name defined on the cluster.
+        hostname_without_domain = factory.make_name('hostname')
+        hostname_with_domain = '%s.%s' % (
+            hostname_without_domain, factory.getRandomString())
+        domain = factory.make_name('domain')
+        nodegroup = factory.make_node_group(
+            status=NODEGROUP_STATUS.ACCEPTED,
+            name=domain,
+            management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS)
+        node = factory.make_node(
+            hostname=hostname_with_domain, nodegroup=nodegroup)
+        expected_hostname = '%s.%s' % (hostname_without_domain, domain)
+        response = self.client.get(self.get_uri('nodes/'), {'op': 'list'})
+        self.assertEqual(httplib.OK, response.status_code, response.content)
+        parsed_result = json.loads(response.content)
+        self.assertItemsEqual(
+            [expected_hostname],
+            [node.get('hostname') for node in parsed_result])
+
+
+class NodeHostnameEnlistmentTest(APIv10TestMixin, MultipleUsersScenarios,
+                                 TestCase):
+
+    scenarios = [
+        ('anon', dict(userfactory=lambda: AnonymousUser())),
+        ('user', dict(userfactory=factory.make_user)),
+        ('admin', dict(userfactory=factory.make_admin)),
+        ]
+
+    def test_created_node_has_domain_from_cluster(self):
+        hostname_without_domain = factory.make_name('hostname')
+        hostname_with_domain = '%s.%s' % (
+            hostname_without_domain, factory.getRandomString())
+        domain = factory.make_name('domain')
+        factory.make_node_group(
+            status=NODEGROUP_STATUS.ACCEPTED,
+            name=domain,
+            management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS)
+        response = self.client.post(
+            self.get_uri('nodes/'),
+            {
+                'op': 'new',
+                'hostname': hostname_with_domain,
+                'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
+                'after_commissioning_action':
+                    NODE_AFTER_COMMISSIONING_ACTION.DEFAULT,
+                'mac_addresses': [factory.getRandomMACAddress()],
+            })
+        self.assertEqual(httplib.OK, response.status_code, response.content)
+        parsed_result = json.loads(response.content)
+        expected_hostname = '%s.%s' % (hostname_without_domain, domain)
+        self.assertEqual(
+            expected_hostname, parsed_result.get('hostname'))
+
+
 class NonAdminEnlistmentAPITest(APIv10TestMixin, MultipleUsersScenarios,
                                 TestCase):
     # Enlistment tests for non-admin users.
@@ -703,6 +767,7 @@
                 'netboot',
                 'power_type',
                 'tag_names',
+                'resource_uri',
             ],
             list(parsed_result))