Merge lp:~julian-edwards/maas/static-range-in-ui-api into lp:~maas-committers/maas/trunk

[Julian Edwards]

Ensure static_ip_range_[low|high] can be edited in the UI and API.

Mostly mechanical change to allow UI/API editing of the new static IP ranges in cluster interfaces. I added help text to some of the fields since it now seems prudent to differentiate the range types.

[Raphaël Badin (rvb)]

Looks good. See my comments inline; I guess we will add validation for the new fields in a follow-up branch (check that the static range doesn't overlap with other ip ranges [static or dynamic])…?

review: Approve

[Julian Edwards (julian-edwards)]

Thanks for the review!

On 10/06/14 17:50, Raphaël Badin wrote:
> Review: Approve
>
> Looks good. See my comments inline; I guess we will add validation for the new fields in a follow-up branch (check that the static range doesn't overlap with other ip ranges [static or dynamic])…?

Correct, jtv said he'd look at that.

>> class NodeGroupInterfacesHandler(OperationsHandler):
>> @@ -1837,10 +1839,16 @@
>> :type broadcast_ip: unicode (IP Address)
>> :param router_ip: Address of default gateway.
>> :type router_ip: unicode (IP Address)
>> - :param ip_range_low: Lowest IP address to assign to clients.
>> + :param ip_range_low: Lowest dynamic IP address to assign to clients.
>> :type ip_range_low: unicode (IP Address)
>> - :param ip_range_high: Highest IP address to assign to clients.
>> + :param ip_range_high: Highest dynamic IP address to assign to clients.
>> :type ip_range_high: unicode (IP Address)
>> + :param static_ip_range_low: Lowest static IP address to assign to
>> + clients.
>> + :type static_ip_range_low: unicode (IP Address)
>> + :param static_ip_range_high: Highest static IP address to assign to
>> + clients.
>> + :type static_ip_range_high: unicode (IP Address)
>
> This is probably worth testing (see src/maasserver/tests/test_api.py:TestNodeGroupInterfaceAPI and TestNodeGroupInterfacesAPI.test_new_creates_interface).

It's already tested with the existing tests, they iterate over the list
of fields supplied!

>> + if static_ip_range_low is None or static_ip_range_high is None:
>> + static_low, static_high = self.make_ip_range()
>
> AFAIK, we've always worked under the assumption that the static range (same as the dynamic range) was going to be *inside* the nodegroupinterface's network. Here you're creating a completely different network range possibility outside of the nodegroupinterface's network: although this is testing code I think we should change this to better reflect what happens in reality.

Yes. However I am thinking ahead here. If we assume it's not in the
same range and make the code work for that now, it'll also just DTRT
when we redefine the ranges later.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2014-06-04 18:21:48 +0000
+++ src/maasserver/api.py	2014-06-10 06:23:18 +0000
@@ -1799,7 +1799,9 @@
 
 DISPLAYED_NODEGROUPINTERFACE_FIELDS = (
     'ip', 'management', 'interface', 'subnet_mask',
-    'broadcast_ip', 'ip_range_low', 'ip_range_high')
+    'broadcast_ip', 'ip_range_low', 'ip_range_high',
+    'static_ip_range_low', 'static_ip_range_high',
+    )
 
 
 class NodeGroupInterfacesHandler(OperationsHandler):
@@ -1837,10 +1839,16 @@
         :type broadcast_ip: unicode (IP Address)
         :param router_ip: Address of default gateway.
         :type router_ip: unicode (IP Address)
-        :param ip_range_low: Lowest IP address to assign to clients.
+        :param ip_range_low: Lowest dynamic IP address to assign to clients.
         :type ip_range_low: unicode (IP Address)
-        :param ip_range_high: Highest IP address to assign to clients.
+        :param ip_range_high: Highest dynamic IP address to assign to clients.
         :type ip_range_high: unicode (IP Address)
+        :param static_ip_range_low: Lowest static IP address to assign to
+            clients.
+        :type static_ip_range_low: unicode (IP Address)
+        :param static_ip_range_high: Highest static IP address to assign to
+            clients.
+        :type static_ip_range_high: unicode (IP Address)
         """
         nodegroup = get_object_or_404(NodeGroup, uuid=uuid)
         if not request.user.is_superuser:
@@ -1904,10 +1912,16 @@
         :type broadcast_ip: unicode (IP Address)
         :param router_ip: Address of default gateway.
         :type router_ip: unicode (IP Address)
-        :param ip_range_low: Lowest IP address to assign to clients.
+        :param ip_range_low: Lowest dynamic IP address to assign to clients.
         :type ip_range_low: unicode (IP Address)
-        :param ip_range_high: Highest IP address to assign to clients.
+        :param ip_range_high: Highest dynamic IP address to assign to clients.
         :type ip_range_high: unicode (IP Address)
+        :param static_ip_range_low: Lowest static IP address to assign to
+            clients.
+        :type static_ip_range_low: unicode (IP Address)
+        :param static_ip_range_high: Highest static IP address to assign to
+            clients.
+        :type static_ip_range_high: unicode (IP Address)
         """
         nodegroupinterface = self.get_interface(request, uuid, interface)
         form = NodeGroupInterfaceForm(
 
=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2014-06-04 16:46:28 +0000
+++ src/maasserver/forms.py	2014-06-10 06:23:18 +0000
@@ -1074,6 +1074,8 @@
             'router_ip',
             'ip_range_low',
             'ip_range_high',
+            'static_ip_range_low',
+            'static_ip_range_high',
             )
 
 
 
=== modified file 'src/maasserver/models/nodegroup.py'
--- src/maasserver/models/nodegroup.py	2014-05-22 14:15:52 +0000
+++ src/maasserver/models/nodegroup.py	2014-06-10 06:23:18 +0000
@@ -62,7 +62,8 @@
             ip_range_high=None, dhcp_key='', interface='',
             status=NODEGROUP_STATUS.DEFAULT,
             management=NODEGROUPINTERFACE_MANAGEMENT.DEFAULT,
-            cluster_name=None, maas_url=''):
+            cluster_name=None, maas_url='',
+            static_ip_range_low=None, static_ip_range_high=None):
         """Create a :class:`NodeGroup` with the given parameters.
 
         This method will:
@@ -91,7 +92,9 @@
                 nodegroup=nodegroup, ip=ip, subnet_mask=subnet_mask,
                 broadcast_ip=broadcast_ip, router_ip=router_ip,
                 interface=interface, ip_range_low=ip_range_low,
-                ip_range_high=ip_range_high, management=management)
+                ip_range_high=ip_range_high, management=management,
+                static_ip_range_low=static_ip_range_low,
+                static_ip_range_high=static_ip_range_high)
             nginterface.save()
         return nodegroup
 
 
=== modified file 'src/maasserver/models/nodegroupinterface.py'
--- src/maasserver/models/nodegroupinterface.py	2014-05-28 04:53:27 +0000
+++ src/maasserver/models/nodegroupinterface.py	2014-06-10 06:23:18 +0000
@@ -75,13 +75,19 @@
     router_ip = GenericIPAddressField(
         editable=True, unique=False, blank=True, null=True, default=None)
     ip_range_low = GenericIPAddressField(
-        editable=True, unique=False, blank=True, null=True, default=None)
+        editable=True, unique=False, blank=True, null=True, default=None,
+        help_text="Lowest IP number of the range for dynamic IPs.")
     ip_range_high = GenericIPAddressField(
-        editable=True, unique=False, blank=True, null=True, default=None)
+        editable=True, unique=False, blank=True, null=True, default=None,
+        help_text="Highest IP number of the range for dynamic IPs.")
     static_ip_range_low = GenericIPAddressField(
-        editable=True, unique=False, blank=True, null=True, default=None)
+        editable=True, unique=False, blank=True, null=True, default=None,
+        help_text="Lowest IP number of the range for static IPs, must be same"
+                  " network as dynamic range.")
     static_ip_range_high = GenericIPAddressField(
-        editable=True, unique=False, blank=True, null=True, default=None)
+        editable=True, unique=False, blank=True, null=True, default=None,
+        help_text="Highest IP number of the range for static IPs, must be same"
+                  " network as dynamic range.")
 
     # Foreign DHCP server address, if any, that was detected on this
     # interface.
 
=== modified file 'src/maasserver/models/tests/test_nodegroup.py'
--- src/maasserver/models/tests/test_nodegroup.py	2014-05-22 14:15:52 +0000
+++ src/maasserver/models/tests/test_nodegroup.py	2014-06-10 06:23:18 +0000
@@ -63,6 +63,8 @@
         'router_ip': factory.getRandomIPInNetwork(network),
         'ip_range_low': factory.getRandomIPInNetwork(network),
         'ip_range_high': factory.getRandomIPInNetwork(network),
+        'static_ip_range_low': factory.getRandomIPInNetwork(network),
+        'static_ip_range_high': factory.getRandomIPInNetwork(network),
         }
 
 
 
=== modified file 'src/maasserver/testing/factory.py'
--- src/maasserver/testing/factory.py	2014-06-04 17:08:10 +0000
+++ src/maasserver/testing/factory.py	2014-06-10 06:23:18 +0000
@@ -222,13 +222,21 @@
     def get_interface_fields(self, ip=None, router_ip=None, network=None,
                              subnet_mask=None, broadcast_ip=None,
                              ip_range_low=None, ip_range_high=None,
-                             interface=None, management=None, **kwargs):
+                             interface=None, management=None,
+                             static_ip_range_low=None,
+                             static_ip_range_high=None, **kwargs):
         if network is None:
             network = factory.getRandomNetwork()
         if subnet_mask is None:
             subnet_mask = unicode(network.netmask)
         if broadcast_ip is None:
             broadcast_ip = unicode(network.broadcast)
+        if static_ip_range_low is None or static_ip_range_high is None:
+            static_low, static_high = self.make_ip_range()
+            if static_ip_range_low is None:
+                static_ip_range_low = unicode(static_low)
+            if static_ip_range_high is None:
+                static_ip_range_high = unicode(static_high)
         if ip_range_low is None:
             ip_range_low = unicode(IPAddress(network.first))
         if ip_range_high is None:
@@ -246,6 +254,8 @@
             broadcast_ip=broadcast_ip,
             ip_range_low=ip_range_low,
             ip_range_high=ip_range_high,
+            static_ip_range_low=static_ip_range_low,
+            static_ip_range_high=static_ip_range_high,
             router_ip=router_ip,
             ip=ip,
             management=management,
@@ -255,7 +265,8 @@
                         ip=None, router_ip=None, network=None,
                         subnet_mask=None, broadcast_ip=None, ip_range_low=None,
                         ip_range_high=None, interface=None, management=None,
-                        status=None, maas_url='', **kwargs):
+                        status=None, maas_url='', static_ip_range_low=None,
+                        static_ip_range_high=None, **kwargs):
         """Create a :class:`NodeGroup`.
 
         If network (an instance of IPNetwork) is provided, use it to populate
@@ -277,7 +288,9 @@
             ip=ip, router_ip=router_ip, network=network,
             subnet_mask=subnet_mask, broadcast_ip=broadcast_ip,
             ip_range_low=ip_range_low, ip_range_high=ip_range_high,
-            interface=interface, management=management)
+            interface=interface, management=management,
+            static_ip_range_low=static_ip_range_low,
+            static_ip_range_high=static_ip_range_high)
         interface_settings.update(kwargs)
         return NodeGroup.objects.new(
             name=name, uuid=uuid, cluster_name=cluster_name, status=status,
@@ -306,12 +319,16 @@
                                   router_ip=None, network=None,
                                   subnet_mask=None, broadcast_ip=None,
                                   ip_range_low=None, ip_range_high=None,
-                                  interface=None, management=None, **kwargs):
+                                  interface=None, management=None,
+                                  static_ip_range_low=None,
+                                  static_ip_range_high=None, **kwargs):
         interface_settings = self.get_interface_fields(
             ip=ip, router_ip=router_ip, network=network,
             subnet_mask=subnet_mask, broadcast_ip=broadcast_ip,
             ip_range_low=ip_range_low, ip_range_high=ip_range_high,
-            interface=interface, management=management)
+            interface=interface, management=management,
+            static_ip_range_low=static_ip_range_low,
+            static_ip_range_high=static_ip_range_high)
         interface_settings.update(**kwargs)
         interface = NodeGroupInterface(
             nodegroup=nodegroup, **interface_settings)
 
=== modified file 'src/maasserver/tests/test_forms.py'
--- src/maasserver/tests/test_forms.py	2014-06-04 16:46:28 +0000
+++ src/maasserver/tests/test_forms.py	2014-06-10 06:23:18 +0000
@@ -1021,6 +1021,7 @@
         management = factory.getRandomEnum(NODEGROUPINTERFACE_MANAGEMENT)
     # Pick upper and lower boundaries of IP range, with upper > lower.
     ip_range_low, ip_range_high = factory.make_ip_range(network)
+    static_ip_range_low, static_ip_range_high = factory.make_ip_range(network)
     return {
         'ip': factory.getRandomIPInNetwork(network),
         'interface': factory.make_name('interface'),
@@ -1029,13 +1030,16 @@
         'router_ip': factory.getRandomIPInNetwork(network),
         'ip_range_low': unicode(ip_range_low),
         'ip_range_high': unicode(ip_range_high),
+        'static_ip_range_low': unicode(static_ip_range_low),
+        'static_ip_range_high': unicode(static_ip_range_high),
         'management': management,
     }
 
 
 nullable_fields = [
     'subnet_mask', 'broadcast_ip', 'router_ip', 'ip_range_low',
-    'ip_range_high']
+    'ip_range_high', 'static_ip_range_low', 'static_ip_range_high',
+    ]
 
 
 class TestNodeGroupInterfaceForm(MAASServerTestCase):