Merge lp:~mpontillo/maas/l2-spaces--juju-bug-workaround into lp:~maas-committers/maas/trunk

[Mike Pontillo]

Spaces API cleanup.

 * Make spaces API more backward compatible with older Juju versions.
 * Always return a string for the space name. The string "undefined" will be used for null spaces.
 * "undefined" is now a reserved space name.
 * A specifier (in practice, the name or the ID) can now be used to specify a VLAN's space.
 * Remove dead code.

[Blake Rouse (blake-rouse)]

Looks good. Only have a question about the dead code you removed.

review: Needs Information

[Blake Rouse (blake-rouse)]

Okay thanks fore verifying.

review: Approve

=== modified file 'src/maasserver/api/subnets.py'
--- src/maasserver/api/subnets.py	2016-12-20 20:16:37 +0000
+++ src/maasserver/api/subnets.py	2017-01-22 22:55:49 +0000
@@ -13,7 +13,10 @@
 from maasserver.enum import NODE_PERMISSION
 from maasserver.exceptions import MAASAPIValidationError
 from maasserver.forms_subnet import SubnetForm
-from maasserver.models import Subnet
+from maasserver.models import (
+    Space,
+    Subnet,
+)
 from piston3.utils import rc
 from provisioningserver.utils.network import IPRangeStatistics
 
@@ -147,7 +150,7 @@
     def space(cls, subnet):
         """Return the name of the space, or None if the space is undefined."""
         if subnet.space is None:
-            return None
+            return Space.UNDEFINED
         return subnet.space.get_name()
 
     def read(self, request, id):

=== modified file 'src/maasserver/api/tests/test_vlans.py'
--- src/maasserver/api/tests/test_vlans.py	2016-12-08 23:02:30 +0000
+++ src/maasserver/api/tests/test_vlans.py	2017-01-22 22:55:49 +0000
@@ -11,12 +11,18 @@
 
 from django.conf import settings
 from django.core.urlresolvers import reverse
+from maasserver.models import Space
 from maasserver.testing.api import APITestCase
-from maasserver.testing.factory import factory
+from maasserver.testing.factory import (
+    factory,
+    RANDOM,
+)
 from maasserver.utils.orm import reload_object
 from testtools.matchers import (
     ContainsDict,
     Equals,
+    Is,
+    Not,
 )
 
 
@@ -126,7 +132,7 @@
         self.assertEqual(vlan_name, response_data['name'])
         self.assertEqual(vid, response_data['vid'])
         self.assertEqual(mtu, response_data['mtu'])
-        self.assertEqual(None, response_data['space'])
+        self.assertEqual(Space.UNDEFINED, response_data['space'])
 
     def test_create_with_space(self):
         self.become_admin()
@@ -239,6 +245,23 @@
             "resource_uri": Equals(get_vlan_uri(vlan)),
         }))
 
+    def test_read_without_space_returns_undefined_space(self):
+        vlan = factory.make_VLAN(space=None)
+        uri = get_vlan_uri(vlan, vlan.fabric)
+        response = self.client.get(uri)
+
+        self.assertEqual(
+            http.client.OK, response.status_code, response.content)
+        parsed_vlan = json.loads(
+            response.content.decode(settings.DEFAULT_CHARSET))
+        self.assertThat(parsed_vlan, ContainsDict({
+            "id": Equals(vlan.id),
+            "name": Equals(vlan.get_name()),
+            "vid": Equals(vlan.vid),
+            "space": Equals(Space.UNDEFINED),
+            "resource_uri": Equals(get_vlan_uri(vlan)),
+        }))
+
     def test_read_404_when_bad_id(self):
         fabric = factory.make_Fabric()
         uri = reverse(
@@ -306,6 +329,40 @@
         self.assertEqual(new_vid, parsed_vlan['vid'])
         self.assertEqual(new_vid, vlan.vid)
 
+    def test_update_with_empty_space_clears_space(self):
+        self.become_admin()
+        fabric = factory.make_Fabric()
+        vlan = factory.make_VLAN(fabric=fabric, space=RANDOM)
+        self.assertThat(vlan.space, Not(Is(None)))
+        uri = get_vlan_uri(vlan, fabric)
+        response = self.client.put(uri, {
+            "space": '',
+        })
+        self.assertEqual(
+            http.client.OK, response.status_code, response.content)
+        parsed_vlan = json.loads(
+            response.content.decode(settings.DEFAULT_CHARSET))
+        vlan = reload_object(vlan)
+        self.assertThat(vlan.space, Is(None))
+        self.assertThat(parsed_vlan['space'], Equals(Space.UNDEFINED))
+
+    def test_update_with_undefined_space_clears_space(self):
+        self.become_admin()
+        fabric = factory.make_Fabric()
+        vlan = factory.make_VLAN(fabric=fabric, space=RANDOM)
+        self.assertThat(vlan.space, Not(Is(None)))
+        uri = get_vlan_uri(vlan, fabric)
+        response = self.client.put(uri, {
+            "space": Space.UNDEFINED,
+        })
+        self.assertEqual(
+            http.client.OK, response.status_code, response.content)
+        parsed_vlan = json.loads(
+            response.content.decode(settings.DEFAULT_CHARSET))
+        vlan = reload_object(vlan)
+        self.assertThat(vlan.space, Is(None))
+        self.assertThat(parsed_vlan['space'], Equals(Space.UNDEFINED))
+
     def test_update_admin_only(self):
         fabric = factory.make_Fabric()
         vlan = factory.make_VLAN(fabric=fabric)

=== modified file 'src/maasserver/api/vlans.py'
--- src/maasserver/api/vlans.py	2016-12-08 23:02:30 +0000
+++ src/maasserver/api/vlans.py	2017-01-22 22:55:49 +0000
@@ -11,6 +11,7 @@
 from maasserver.forms_vlan import VLANForm
 from maasserver.models import (
     Fabric,
+    Space,
     VLAN,
 )
 from piston3.utils import rc
@@ -43,27 +44,6 @@
         # See the comment in NodeHandler.resource_uri.
         return ('vlans_handler', ["fabric_id"])
 
-    @classmethod
-    def primary_rack(handler, vlan):
-        if vlan.primary_rack:
-            return vlan.primary_rack.system_id
-        else:
-            return None
-
-    @classmethod
-    def secondary_rack(handler, vlan):
-        if vlan.secondary_rack:
-            return vlan.secondary_rack.system_id
-        else:
-            return None
-
-    @classmethod
-    def space(handler, vlan):
-        if vlan.space:
-            return vlan.space.get_name()
-        else:
-            return None
-
     def read(self, request, fabric_id):
         """List all VLANs belonging to fabric.
 
@@ -126,7 +106,7 @@
         if vlan.space:
             return vlan.space.get_name()
         else:
-            return None
+            return Space.UNDEFINED
 
     @classmethod
     def fabric(cls, vlan):
@@ -194,11 +174,23 @@
             be configured to proxy reqests to the primary and/or secondary
             rack controller interfaces for the VLAN specified in this field.
         :type relay_vlan: ID of VLAN
+        :param space: The space this VLAN should be placed in. Passing in an
+            empty string (or the string 'undefined') will cause the VLAN to be
+            placed in the 'undefined' space.
+        :type space: unicode
 
         Returns 404 if the fabric or VLAN is not found.
         """
         vlan = self._get_vlan(request.user, NODE_PERMISSION.ADMIN, **kwargs)
-        form = VLANForm(instance=vlan, data=request.data)
+        data = {}
+        # If the user passed in a space, make the undefined space name a
+        # synonym for the empty space. But the Django request data object is
+        # immutable, so we must first copy its contents into our own dict.
+        for k, v in request.data.items():
+            data[k] = v
+        if 'space' in data and data['space'] == Space.UNDEFINED:
+            data['space'] = ''
+        form = VLANForm(instance=vlan, data=data)
         if form.is_valid():
             return form.save()
         else:

=== modified file 'src/maasserver/forms_subnet.py'
--- src/maasserver/forms_subnet.py	2016-12-17 03:08:28 +0000
+++ src/maasserver/forms_subnet.py	2017-01-22 22:55:49 +0000
@@ -52,7 +52,6 @@
             'name',
             'description',
             'vlan',
-            'space',
             'cidr',
             'gateway_ip',
             'dns_servers',
@@ -68,6 +67,11 @@
 
     def clean(self):
         cleaned_data = super(SubnetForm, self).clean()
+        if 'space' in self.data:
+            set_form_error(
+                self, "space",
+                "Spaces may no longer be set on subnets. Set the space on the "
+                "underlying VLAN.")
         # The default value for 'allow_proxy' is True.
         if 'allow_proxy' not in self.data:
             cleaned_data['allow_proxy'] = True
@@ -84,7 +88,6 @@
             # the VLAN specifically. This is because we cannot make a correct
             # determination on what should be done in this case.
             cleaned_data = self._clean_vlan(cleaned_data)
-            cleaned_data = self._clean_space(cleaned_data)
         return cleaned_data
 
     def _clean_name(self, cleaned_data):
@@ -139,15 +142,6 @@
                     "VLAN %s is not in fabric %s." % (vlan, fabric))
         return cleaned_data
 
-    def _clean_space(self, cleaned_data):
-        space = cleaned_data.get("space", None)
-        if space is not None:
-            set_form_error(
-                self, "space",
-                "Spaces may no longer be set on subnets. Set the space on the "
-                "underlying VLAN.")
-        return cleaned_data
-
     def _clean_dns_servers(self, cleaned_data):
         dns_servers = cleaned_data.get("dns_servers", None)
         if dns_servers is None:

=== modified file 'src/maasserver/forms_vlan.py'
--- src/maasserver/forms_vlan.py	2016-12-07 16:40:35 +0000
+++ src/maasserver/forms_vlan.py	2017-01-22 22:55:49 +0000
@@ -9,7 +9,10 @@
 
 from django import forms
 from django.core.exceptions import ValidationError
-from maasserver.fields import NodeChoiceField
+from maasserver.fields import (
+    NodeChoiceField,
+    SpecifierOrModelChoiceField,
+)
 from maasserver.forms import MAASModelForm
 from maasserver.models import (
     RackController,
@@ -24,8 +27,8 @@
     # Linux doesn't allow lower than 552 for the MTU.
     mtu = forms.IntegerField(min_value=552, required=False)
 
-    space = forms.ModelChoiceField(
-        queryset=Space.objects.all(), required=False)
+    space = SpecifierOrModelChoiceField(
+        queryset=Space.objects.all(), required=False, empty_label="")
 
     class Meta:
         model = VLAN
@@ -128,6 +131,9 @@
         if (cleaned_data.get('secondary_rack') is None and
                 self.instance.secondary_rack is not None):
             self.instance.secondary_rack = None
+        if (cleaned_data.get('space') is "" and
+                self.instance.space is not None):
+            self.instance.space = None
         return cleaned_data
 
     def clean_dhcp_on(self):

=== modified file 'src/maasserver/models/space.py'
--- src/maasserver/models/space.py	2017-01-04 17:45:19 +0000
+++ src/maasserver/models/space.py	2017-01-22 22:55:49 +0000
@@ -119,6 +119,9 @@
     :ivar objects: An instance of the class :class:`SpaceManager`.
     """
 
+    # Name of the undefined space.
+    UNDEFINED = "undefined"
+
     class Meta(DefaultMeta):
         """Needed for South to recognize this model."""
         verbose_name = "Space"
@@ -151,8 +154,11 @@
     def clean_name(self):
         reserved = re.compile('^space-\d+$')
         if self.name is not None and self.name != '':
+            if self.name == Space.UNDEFINED:
+                raise ValidationError(
+                    {'name': ["Reserved space name."]})
             if reserved.search(self.name):
-                if (self.id is None or self.name != 'space-%d' % self.id):
+                if self.id is None or self.name != 'space-%d' % self.id:
                     raise ValidationError(
                         {'name': ["Reserved space name."]})
         elif self.id is not None:

=== modified file 'src/maasserver/models/tests/test_space.py'
--- src/maasserver/models/tests/test_space.py	2016-12-20 06:52:13 +0000
+++ src/maasserver/models/tests/test_space.py	2017-01-22 22:55:49 +0000
@@ -155,6 +155,12 @@
             factory.make_Space,
             name='space-1999')
 
+    def test_undefined_name_raises_exception(self):
+        self.assertRaises(
+            ValidationError,
+            factory.make_Space,
+            name=Space.UNDEFINED)
+
     def test_create_sets_name(self):
         space = Space.objects.create(name=None)
         self.assertEqual("space-%d" % space.id, space.name)

=== modified file 'src/maasserver/tests/test_forms_subnet.py'
--- src/maasserver/tests/test_forms_subnet.py	2016-12-17 03:08:28 +0000
+++ src/maasserver/tests/test_forms_subnet.py	2017-01-22 22:55:49 +0000
@@ -24,7 +24,20 @@
             "cidr": ["This field is required."],
             }, form.errors)
 
-    def test__rejects_provided_space(self):
+    def test__rejects_provided_space_on_update(self):
+        space = factory.make_Space()
+        subnet = factory.make_Subnet()
+        form = SubnetForm(instance=subnet, data={
+            "space": space.id
+        })
+        self.assertFalse(form.is_valid(), form.errors)
+        self.assertEqual({
+            "space": [
+                "Spaces may no longer be set on subnets. Set the space on the "
+                "underlying VLAN."
+            ]}, form.errors)
+
+    def test__rejects_space_on_create(self):
         space = factory.make_Space()
         form = SubnetForm({
             "space": space.id, "cidr": factory._make_random_network()

=== modified file 'src/maasserver/tests/test_forms_vlan.py'
--- src/maasserver/tests/test_forms_vlan.py	2016-12-07 16:40:35 +0000
+++ src/maasserver/tests/test_forms_vlan.py	2017-01-22 22:55:49 +0000
@@ -277,6 +277,17 @@
         vlan = reload_object(vlan)
         self.assertEquals(space.id, vlan.space.id)
 
+    def test_update_sets_space_by_specifier(self):
+        vlan = factory.make_VLAN()
+        space = factory.make_Space()
+        form = VLANForm(instance=vlan, data={
+            "space": "name:" + space.name,
+        })
+        self.assertTrue(form.is_valid(), form.errors)
+        form.save()
+        vlan = reload_object(vlan)
+        self.assertEquals(space.id, vlan.space.id)
+
     def test_update_clears_space_when_None(self):
         space = factory.make_Space()
         vlan = factory.make_VLAN(space=space)
@@ -288,6 +299,17 @@
         vlan = reload_object(vlan)
         self.assertIsNone(vlan.space)
 
+    def test_update_clears_space_when_empty(self):
+        space = factory.make_Space()
+        vlan = factory.make_VLAN(space=space)
+        form = VLANForm(instance=vlan, data={
+            "space": '',
+        })
+        self.assertTrue(form.is_valid(), form.errors)
+        form.save()
+        vlan = reload_object(vlan)
+        self.assertIsNone(vlan.space)
+
     def test_update_clears_space_vlan_when_empty(self):
         space = factory.make_Space()
         vlan = factory.make_VLAN(space=space)