Merge lp:~blake-rouse/maas/fix-1495849 into lp:~maas-committers/maas/trunk

[Blake Rouse]

Improve the subnet API to give better parameters to make creating subnets easier.

[Andres Rodriguez (andreserl)]

lgtm!

review: Approve

[MAAS Lander (maas-lander)]

The attempt to merge lp:~blake-rouse/maas/fix-1495849 into lp:maas failed. Below is the output from the failed tests.

Ign http://security.ubuntu.com trusty-security InRelease
Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Hit http://security.ubuntu.com trusty-security Release.gpg
Hit http://security.ubuntu.com trusty-security Release
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Hit http://nova.clouds.archive.ubuntu.com trusty Release.gpg
Get:1 http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg [933 B]
Hit http://nova.clouds.archive.ubuntu.com trusty Release
Hit http://security.ubuntu.com trusty-security/main Sources
Get:2 http://nova.clouds.archive.ubuntu.com trusty-updates Release [63.5 kB]
Hit http://security.ubuntu.com trusty-security/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/main Sources
Hit http://security.ubuntu.com trusty-security/main amd64 Packages
Hit http://security.ubuntu.com trusty-security/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Sources
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages
Hit http://security.ubuntu.com trusty-security/universe Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en
Get:3 http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources [234 kB]
Get:4 http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources [135 kB]
Get:5 http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages [619 kB]
Get:6 http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages [312 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com trusty/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en_US
Fetched 1,365 kB in 3s (438 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 authbind bind9 bind9utils build-essential bzr-builddeb chromium-browser chromium-chromedriver curl daemontools debhelper dh-apport dh-systemd distro-info dnsutils firefox freeipmi-tools git gjs ipython isc-dhcp-common libjs-angularjs libjs-jquery libjs-jquery-hotkeys libjs-yui3-full libjs-yui3-min libpq-dev make nodejs-legacy npm pep8 phantomjs postgresql pyflakes python-apt python-bson python-bzrlib python-convoy python-coverage python-crochet python-cssselect python-curtin python-dev python-distro-info python-django python-django-piston python-django-south python-djorm-ext-pgarray python-docutils python-extras python-fixtures python-flake8 python-formencode python-hivex python-httplib2 python-jinja2 python-jsonschema python-lxml python-mock python-netaddr python-netifaces python-nose python-oauth python-openssl python-paramiko python-pexpect python-pip python-pocket-lint python-...

[MAAS Lander (maas-lander)]

The attempt to merge lp:~blake-rouse/maas/fix-1495849 into lp:maas failed. Below is the output from the failed tests.

Ign http://security.ubuntu.com trusty-security InRelease
Hit http://security.ubuntu.com trusty-security Release.gpg
Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Hit http://security.ubuntu.com trusty-security Release
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Hit http://nova.clouds.archive.ubuntu.com trusty Release.gpg
Get:1 http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg [933 B]
Hit http://nova.clouds.archive.ubuntu.com trusty Release
Get:2 http://nova.clouds.archive.ubuntu.com trusty-updates Release [63.5 kB]
Hit http://security.ubuntu.com trusty-security/main Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/main Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages
Hit http://security.ubuntu.com trusty-security/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/universe amd64 Packages
Hit http://security.ubuntu.com trusty-security/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/main Translation-en
Hit http://security.ubuntu.com trusty-security/universe amd64 Packages
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en
Hit http://security.ubuntu.com trusty-security/universe Translation-en
Get:3 http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources [234 kB]
Get:4 http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources [135 kB]
Get:5 http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages [619 kB]
Get:6 http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages [312 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com trusty/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en_US
Fetched 1,365 kB in 3s (404 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 authbind bind9 bind9utils build-essential bzr-builddeb chromium-browser chromium-chromedriver curl daemontools debhelper dh-apport dh-systemd distro-info dnsutils firefox freeipmi-tools git gjs ipython isc-dhcp-common libjs-angularjs libjs-jquery libjs-jquery-hotkeys libjs-yui3-full libjs-yui3-min libpq-dev make nodejs-legacy npm pep8 phantomjs postgresql pyflakes python-apt python-bson python-bzrlib python-convoy python-coverage python-crochet python-cssselect python-curtin python-dev python-distro-info python-django python-django-piston python-django-south python-djorm-ext-pgarray python-docutils python-extras python-fixtures python-flake8 python-formencode python-hivex python-httplib2 python-jinja2 python-jsonschema python-lxml python-mock python-netaddr python-netifaces python-nose python-oauth python-openssl python-paramiko python-pexpect python-pip python-pocket-lint python-...

=== modified file 'src/maasserver/api/subnets.py'
--- src/maasserver/api/subnets.py	2015-09-01 19:21:50 +0000
+++ src/maasserver/api/subnets.py	2015-09-15 16:27:58 +0000
@@ -55,8 +55,15 @@
         """Create a subnet.
 
         :param name: Name of the subnet.
-        :param vlan: VLAN this subnet belongs to.
-        :param space: Space this subnet is in.
+        :param fabric: Fabric for the subnet. Defaults to the fabric the
+            provided VLAN belongs to or defaults to the default fabric.
+        :param vlan: VLAN this subnet belongs to. Defaults to the default
+            VLAN for the provided fabric or defaults to the default VLAN in
+            the default fabric.
+        :param vid: VID of the VLAN this subnet belongs to. Only used when
+            vlan is not provided. Picks the VLAN with this VID in the provided
+            fabric or the default fabric if one is not given.
+        :param space: Space this subnet is in. Defaults to the default space.
         :param cidr: The network CIDR for this subnet.
         :param gateway_ip: The gateway IP address for this subnet.
         :param dns_servers: Comma-seperated list of DNS servers for this \

=== modified file 'src/maasserver/api/tests/test_subnets.py'
--- src/maasserver/api/tests/test_subnets.py	2015-09-02 02:56:11 +0000
+++ src/maasserver/api/tests/test_subnets.py	2015-09-15 16:27:58 +0000
@@ -111,8 +111,6 @@
         self.assertEqual(
             httplib.BAD_REQUEST, response.status_code, response.content)
         self.assertEqual({
-            "vlan": ["This field is required."],
-            "space": ["This field is required."],
             "cidr": ["This field is required."],
             }, json.loads(response.content))
 

=== modified file 'src/maasserver/forms_subnet.py'
--- src/maasserver/forms_subnet.py	2015-09-02 02:56:11 +0000
+++ src/maasserver/forms_subnet.py	2015-09-15 16:27:58 +0000
@@ -18,19 +18,28 @@
 
 from django import forms
 from maasserver.forms import MAASModelForm
+from maasserver.models.fabric import Fabric
 from maasserver.models.space import Space
 from maasserver.models.subnet import Subnet
 from maasserver.models.vlan import VLAN
+from maasserver.utils.forms import set_form_error
+from maasserver.utils.orm import get_one
 
 
 class SubnetForm(MAASModelForm):
     """Subnet creation/edition form."""
 
+    fabric = forms.ModelChoiceField(
+        queryset=Fabric.objects.all(), required=False)
+
     vlan = forms.ModelChoiceField(
-        queryset=VLAN.objects.all(), required=True)
+        queryset=VLAN.objects.all(), required=False)
+
+    vid = forms.IntegerField(
+        min_value=0, max_value=4095, required=False)
 
     space = forms.ModelChoiceField(
-        queryset=Space.objects.all(), required=True)
+        queryset=Space.objects.all(), required=False)
 
     class Meta:
         model = Subnet
@@ -49,6 +58,16 @@
 
     def clean(self):
         cleaned_data = super(SubnetForm, self).clean()
+        cleaned_data = self._clean_name(cleaned_data)
+        if self.instance.id is None:
+            # We only allow the helpers when creating. When updating we require
+            # 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):
         name = cleaned_data.get("name", None)
         instance_name_and_cidr_match = (
             self.instance.id is not None and
@@ -62,3 +81,46 @@
             # Update the subnet to have the same name as its cidr.
             cleaned_data["name"] = cleaned_data["cidr"]
         return cleaned_data
+
+    def _clean_vlan(self, cleaned_data):
+        fabric = cleaned_data.get("fabric", None)
+        vlan = cleaned_data.get("vlan", None)
+        vid = cleaned_data.get("vid", None)
+        if fabric is None and vlan is None:
+            if not vid:
+                cleaned_data["vlan"] = (
+                    Fabric.objects.get_default_fabric().get_default_vlan())
+            else:
+                vlan = get_one(
+                    VLAN.objects.filter(
+                        fabric=Fabric.objects.get_default_fabric(), vid=vid))
+                if vlan is None:
+                    set_form_error(
+                        self, "vid",
+                        "No VLAN with vid %s in default fabric." % vid)
+                else:
+                    cleaned_data["vlan"] = vlan
+        elif fabric is not None:
+            if vlan is None:
+                if not vid:
+                    cleaned_data["vlan"] = fabric.get_default_vlan()
+                else:
+                    vlan = get_one(VLAN.objects.filter(fabric=fabric, vid=vid))
+                    if vlan is None:
+                        set_form_error(
+                            self, "vid",
+                            "No VLAN with vid %s in fabric %s." % (
+                                vid, fabric))
+                    else:
+                        cleaned_data["vlan"] = vlan
+            elif vlan.fabric_id != fabric.id:
+                set_form_error(
+                    self, "vlan",
+                    "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 None:
+            cleaned_data["space"] = Space.objects.get_default_space()
+        return cleaned_data

=== modified file 'src/maasserver/tests/test_forms_subnet.py'
--- src/maasserver/tests/test_forms_subnet.py	2015-09-02 02:56:11 +0000
+++ src/maasserver/tests/test_forms_subnet.py	2015-09-15 16:27:58 +0000
@@ -15,6 +15,8 @@
 __all__ = []
 
 from maasserver.forms_subnet import SubnetForm
+from maasserver.models.fabric import Fabric
+from maasserver.models.space import Space
 from maasserver.testing.factory import factory
 from maasserver.testing.orm import reload_object
 from maasserver.testing.testcase import MAASServerTestCase
@@ -23,12 +25,10 @@
 
 class TestSubnetForm(MAASServerTestCase):
 
-    def test__requires_vlan_space_cidr(self):
+    def test__requires_cidr(self):
         form = SubnetForm({})
         self.assertFalse(form.is_valid(), form.errors)
         self.assertEquals({
-            "vlan": ["This field is required."],
-            "space": ["This field is required."],
             "cidr": ["This field is required."],
             }, form.errors)
 
@@ -75,6 +75,129 @@
             subnet, MatchesStructure.byEquality(
                 name=cidr, vlan=vlan, space=space, cidr=cidr))
 
+    def test__creates_subnet_in_default_space(self):
+        vlan = factory.make_VLAN()
+        network = factory.make_ip4_or_6_network()
+        cidr = unicode(network.cidr)
+        form = SubnetForm({
+            "vlan": vlan.id,
+            "cidr": cidr,
+        })
+        self.assertTrue(form.is_valid(), form.errors)
+        subnet = form.save()
+        self.assertThat(
+            subnet, MatchesStructure.byEquality(
+                name=cidr, vlan=vlan, cidr=cidr,
+                space=Space.objects.get_default_space()))
+
+    def test__creates_subnet_in_default_fabric_and_vlan(self):
+        network = factory.make_ip4_or_6_network()
+        cidr = unicode(network.cidr)
+        form = SubnetForm({
+            "cidr": cidr,
+        })
+        self.assertTrue(form.is_valid(), form.errors)
+        subnet = form.save()
+        self.assertThat(
+            subnet, MatchesStructure.byEquality(
+                name=cidr, cidr=cidr,
+                vlan=Fabric.objects.get_default_fabric().get_default_vlan(),
+                space=Space.objects.get_default_space()))
+
+    def test__creates_subnet_in_default_vlan_in_fabric(self):
+        fabric = factory.make_Fabric()
+        network = factory.make_ip4_or_6_network()
+        cidr = unicode(network.cidr)
+        form = SubnetForm({
+            "cidr": cidr,
+            "fabric": fabric.id,
+        })
+        self.assertTrue(form.is_valid(), form.errors)
+        subnet = form.save()
+        self.assertThat(
+            subnet, MatchesStructure.byEquality(
+                name=cidr, cidr=cidr,
+                vlan=fabric.get_default_vlan(),
+                space=Space.objects.get_default_space()))
+
+    def test__creates_subnet_in_default_fabric_with_vid(self):
+        vlan = factory.make_VLAN(fabric=Fabric.objects.get_default_fabric())
+        network = factory.make_ip4_or_6_network()
+        cidr = unicode(network.cidr)
+        form = SubnetForm({
+            "cidr": cidr,
+            "vid": vlan.vid,
+        })
+        self.assertTrue(form.is_valid(), form.errors)
+        subnet = form.save()
+        self.assertThat(
+            subnet, MatchesStructure.byEquality(
+                name=cidr, cidr=cidr,
+                vlan=vlan,
+                space=Space.objects.get_default_space()))
+
+    def test__creates_subnet_in_fabric_with_vid(self):
+        fabric = factory.make_Fabric()
+        vlan = factory.make_VLAN(fabric=fabric)
+        network = factory.make_ip4_or_6_network()
+        cidr = unicode(network.cidr)
+        form = SubnetForm({
+            "cidr": cidr,
+            "fabric": fabric.id,
+            "vid": vlan.vid,
+        })
+        self.assertTrue(form.is_valid(), form.errors)
+        subnet = form.save()
+        self.assertThat(
+            subnet, MatchesStructure.byEquality(
+                name=cidr, cidr=cidr,
+                vlan=vlan,
+                space=Space.objects.get_default_space()))
+
+    def test__error_for_unknown_vid_in_default_fabric(self):
+        fabric = factory.make_Fabric()
+        vlan = factory.make_VLAN(fabric=fabric)
+        network = factory.make_ip4_or_6_network()
+        cidr = unicode(network.cidr)
+        form = SubnetForm({
+            "cidr": cidr,
+            "vid": vlan.vid,
+        })
+        self.assertFalse(form.is_valid(), form.errors)
+        self.assertEquals({
+            "vid": ["No VLAN with vid %s in default fabric." % vlan.vid]
+            }, form.errors)
+
+    def test__error_for_unknown_vid_in_fabric(self):
+        fabric = factory.make_Fabric()
+        vlan = factory.make_VLAN(fabric=Fabric.objects.get_default_fabric())
+        network = factory.make_ip4_or_6_network()
+        cidr = unicode(network.cidr)
+        form = SubnetForm({
+            "cidr": cidr,
+            "fabric": fabric.id,
+            "vid": vlan.vid,
+        })
+        self.assertFalse(form.is_valid(), form.errors)
+        self.assertEquals({
+            "vid": ["No VLAN with vid %s in fabric %s." % (vlan.vid, fabric)]
+            }, form.errors)
+
+    def test__error_for_vlan_not_in_fabric(self):
+        fabric = factory.make_Fabric()
+        vlan = factory.make_VLAN(fabric=Fabric.objects.get_default_fabric())
+        network = factory.make_ip4_or_6_network()
+        cidr = unicode(network.cidr)
+        form = SubnetForm({
+            "cidr": cidr,
+            "fabric": fabric.id,
+            "vlan": vlan.id,
+        })
+        self.assertFalse(form.is_valid(), form.errors)
+        self.assertEquals({
+            "vlan": ["VLAN %s is not in fabric %s." % (vlan, fabric)]
+            }, form.errors)
+
     def test__doest_require_vlan_space_or_cidr_on_update(self):
         subnet = factory.make_Subnet()
         form = SubnetForm(instance=subnet, data={})