Merge lp:~jtv/maas/retire-get_dns_managed_interface into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

Retire NodeGroup.get_dns_managed_interface, in favour of the more general NodeGroup.manages_dns().

This also subsumes the helper function is_dns_managed. The change eliminates the last currently known roadblock to permitting multiple DNS-managed nodegroupinterfaces per nodegroup.

Jeroen

[Raphaël Badin (rvb)]

review: Approve

=== modified file 'src/maasserver/dns.py'
--- src/maasserver/dns.py	2014-01-28 09:00:22 +0000
+++ src/maasserver/dns.py	2014-01-29 10:12:13 +0000
@@ -16,7 +16,6 @@
     'add_zone',
     'change_dns_zones',
     'is_dns_enabled',
-    'is_dns_managed',
     'next_zone_serial',
     'write_full_dns_config',
     ]
@@ -31,10 +30,7 @@
 
 from django.conf import settings
 from maasserver import logger
-from maasserver.enum import (
-    NODEGROUP_STATUS,
-    NODEGROUPINTERFACE_MANAGEMENT,
-    )
+from maasserver.enum import NODEGROUPINTERFACE_MANAGEMENT
 from maasserver.exceptions import MAASException
 from maasserver.models import (
     Config,
@@ -85,13 +81,6 @@
     """An error occured when setting up MAAS's DNS server."""
 
 
-def is_dns_managed(nodegroup):
-    """Does MAAS manage a DNS zone for this Nodegroup?"""
-    return (
-        nodegroup.status == NODEGROUP_STATUS.ACCEPTED and
-        nodegroup.get_dns_managed_interface() is not None)
-
-
 WARNING_MESSAGE = (
     "The DNS server will use the address '%s',  which is inside the "
     "loopback network.  This may not be a problem if you're not using "
@@ -171,7 +160,10 @@
     @staticmethod
     def _filter_dns_managed(nodegroups):
         """Return the subset of `nodegroups` for which we manage DNS."""
-        return set(filter(is_dns_managed, nodegroups))
+        return set(
+            nodegroup
+            for nodegroup in nodegroups
+            if nodegroup.manages_dns())
 
     @staticmethod
     def _get_forward_nodegroups(domains):

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2014-01-23 12:00:13 +0000
+++ src/maasserver/forms.py	2014-01-29 10:12:13 +0000
@@ -1,4 +1,4 @@
-# Copyright 2012, 2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Forms."""
@@ -65,7 +65,6 @@
     NODE_AFTER_COMMISSIONING_ACTION,
     NODE_AFTER_COMMISSIONING_ACTION_CHOICES,
     NODE_STATUS,
-    NODEGROUP_STATUS,
     NODEGROUPINTERFACE_MANAGEMENT,
     NODEGROUPINTERFACE_MANAGEMENT_CHOICES,
     )
@@ -101,9 +100,7 @@
 from maasserver.utils import strip_domain
 from metadataserver.fields import Bin
 from metadataserver.models import CommissioningScript
-from provisioningserver.enum import (
-    POWER_TYPE_CHOICES,
-    )
+from provisioningserver.enum import POWER_TYPE_CHOICES
 
 
 INVALID_ARCHITECTURE_MESSAGE = compose_invalid_choice_text(
@@ -867,11 +864,7 @@
             # No change to the name.  Return old name.
             return old_name
 
-        if self.instance.status != NODEGROUP_STATUS.ACCEPTED:
-            # This nodegroup is not in use.  Change it at will.
-            return new_name
-
-        if self.instance.get_dns_managed_interface() is None:
+        if not self.instance.manages_dns():
             # Not managing DNS.  There won't be any DNS problems with this
             # name change then.
             return new_name

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2014-01-23 08:50:05 +0000
+++ src/maasserver/models/node.py	2014-01-29 10:12:13 +0000
@@ -1,4 +1,4 @@
-# Copyright 2012, 2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Node objects."""
@@ -38,8 +38,8 @@
     IntegerField,
     Manager,
     ManyToManyField,
+    Q,
     SET_DEFAULT,
-    Q,
     )
 from django.shortcuts import get_object_or_404
 import djorm_pgarray.fields
@@ -72,9 +72,7 @@
     strip_domain,
     )
 from piston.models import Token
-from provisioningserver.enum import (
-    POWER_TYPE_CHOICES,
-    )
+from provisioningserver.enum import POWER_TYPE_CHOICES
 from provisioningserver.tasks import (
     power_off,
     power_on,
@@ -501,9 +499,7 @@
         on the cluster controller.
         If not, simply return the node's hostname.
         """
-        # Avoid circular imports.
-        from maasserver.dns import is_dns_managed
-        if is_dns_managed(self.nodegroup):
+        if self.nodegroup.manages_dns():
             # If the hostname field contains a domain, strip it.
             hostname = strip_domain(self.hostname)
             # Build the FQDN by using the hostname and nodegroup.name

=== modified file 'src/maasserver/models/nodegroup.py'
--- src/maasserver/models/nodegroup.py	2014-01-28 04:58:37 +0000
+++ src/maasserver/models/nodegroup.py	2014-01-29 10:12:13 +0000
@@ -222,14 +222,20 @@
             if itf.management != NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED
             ]
 
-    def get_dns_managed_interface(self):
-        """Return the `NodeGroupInterface`, if any, whose DNS we manage."""
+    def manages_dns(self):
+        """Does this `NodeGroup` manage DNS on any interfaces?
+
+        This returns `True` when the `NodeGroup` is accepted, and has a
+        `NodeGroupInterface` that's set to manage both DHCP and DNS.
+        """
+        if self.status != NODEGROUP_STATUS.ACCEPTED:
+            return False
         # Filter in python instead of in SQL.  This will use the cached
         # version of self.nodegroupinterface_set if present.
         for itf in self.nodegroupinterface_set.all():
             if itf.management == NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS:
-                return itf
-        return None
+                return True
+        return False
 
     def ensure_dhcp_key(self):
         """Ensure that this nodegroup has a dhcp key.

=== modified file 'src/maasserver/models/tests/test_nodegroup.py'
--- src/maasserver/models/tests/test_nodegroup.py	2014-01-28 04:58:37 +0000
+++ src/maasserver/models/tests/test_nodegroup.py	2014-01-29 10:12:13 +0000
@@ -333,27 +333,51 @@
         args, kwargs = task.apply_async.call_args
         self.assertEqual(nodegroup.work_queue, kwargs['queue'])
 
-    def test_get_dns_managed_interface_returns_dns_managed_interface(self):
+    def test_manages_dns_returns_True_if_managing_DNS(self):
         nodegroup = factory.make_node_group(
+            status=NODEGROUP_STATUS.ACCEPTED,
             management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS)
-        self.assertEqual(
-            nodegroup.nodegroupinterface_set.all()[0],
-            nodegroup.get_dns_managed_interface())
-
-    def test_get_dns_managed_interface_returns_None_if_dhcp_only(self):
-        nodegroup = factory.make_node_group(
-            management=NODEGROUPINTERFACE_MANAGEMENT.DHCP)
-        self.assertIsNone(nodegroup.get_dns_managed_interface())
-
-    def test_get_dns_managed_interface_returns_None_if_unmanaged(self):
-        nodegroup = factory.make_node_group(
-            management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED)
-        self.assertIsNone(nodegroup.get_dns_managed_interface())
-
-    def test_get_dns_managed_interface_returns_None_if_no_interface(self):
-        nodegroup = factory.make_node_group()
+        self.assertTrue(nodegroup.manages_dns())
+
+    def test_manages_dns_returns_False_if_not_accepted(self):
+        nodegroups = [
+            factory.make_node_group(
+                status=status,
+                management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS)
+            for status in map_enum(NODEGROUP_STATUS).values()
+            ]
+        self.assertEqual(
+            {
+                NODEGROUP_STATUS.PENDING: False,
+                NODEGROUP_STATUS.ACCEPTED: True,
+                NODEGROUP_STATUS.REJECTED: False,
+            },
+            {
+                nodegroup.status: nodegroup.manages_dns()
+                for nodegroup in nodegroups
+            })
+
+    def test_manages_dns_returns_False_if_no_interface_manages_DNS(self):
+        nodegroups = {
+            management: factory.make_node_group(
+                status=NODEGROUP_STATUS.ACCEPTED, management=management)
+            for management in map_enum(NODEGROUPINTERFACE_MANAGEMENT).values()
+            }
+        self.assertEqual(
+            {
+                NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED: False,
+                NODEGROUPINTERFACE_MANAGEMENT.DHCP: False,
+                NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS: True,
+            },
+            {
+                management: nodegroup.manages_dns()
+                for management, nodegroup in nodegroups.items()
+            })
+
+    def test_manages_dns_returns_False_if_nodegroup_has_no_interfaces(self):
+        nodegroup = factory.make_node_group(status=NODEGROUP_STATUS.ACCEPTED)
         nodegroup.nodegroupinterface_set.all().delete()
-        self.assertIsNone(nodegroup.get_dns_managed_interface())
+        self.assertFalse(nodegroup.manages_dns())
 
     def test_get_managed_interfaces_returns_list(self):
         nodegroup = factory.make_node_group()

=== modified file 'src/maasserver/tests/test_dns.py'
--- src/maasserver/tests/test_dns.py	2014-01-28 10:38:25 +0000
+++ src/maasserver/tests/test_dns.py	2014-01-29 10:12:13 +0000
@@ -132,30 +132,6 @@
             (ip, [(hostname, )]),
             (dns.get_dns_server_address(nodegroup), resolver.extract_args()))
 
-    def test_is_dns_managed(self):
-        nodegroups_with_expected_results = {
-            factory.make_node_group(
-                status=NODEGROUP_STATUS.PENDING,
-                management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS): False,
-            factory.make_node_group(
-                status=NODEGROUP_STATUS.REJECTED,
-                management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS): False,
-            factory.make_node_group(
-                status=NODEGROUP_STATUS.ACCEPTED,
-                management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS): True,
-            factory.make_node_group(
-                status=NODEGROUP_STATUS.ACCEPTED,
-                management=NODEGROUPINTERFACE_MANAGEMENT.DHCP): False,
-            factory.make_node_group(
-                status=NODEGROUP_STATUS.ACCEPTED,
-                management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED): False,
-        }
-        results = {
-            nodegroup: dns.is_dns_managed(nodegroup)
-            for nodegroup, _ in nodegroups_with_expected_results.items()
-        }
-        self.assertEqual(nodegroups_with_expected_results, results)
-
 
 class TestLazyDict(TestCase):
     """Tests for `lazydict`."""