Merge lp:~mpontillo/maas/l2-spaces--constraints into lp:~maas-committers/maas/trunk

[Mike Pontillo]

Add machine acquisition constraint support for L2 spaces.

 * Change specifier matching to allow a space match based on VLAN (in addition to subnet).
 * Reduce complexity of `filter_nodes()` (in order to pass new lint checks).
 * Fix clean_* methods on machine acquisition form to handle 'vlans' and 'not_vlans'.
 * Add tests for 'vlans' and 'not_vlans' machine acquisition constraints.
 * Add 'space' specifier support to VLAN model.
 * Add tests for 'space' specified handling in VLAN model.
 * Add tests for machine acquisition form errors for specifier-based queries.

[Mike Pontillo (mpontillo)]

Needs fixing: I forgot to add the 'space' key to the VLAN specifiers, which is needed to do `vlan` and `not_vlan` constraints on machine acquisition (plus, need testing for that).

I'm not going to formally review this "Needs Fixing" as such, because I don't want others to assume it was already reviewed and not find other things. ;-)

[Mike Pontillo (mpontillo)]

Fixed the above. This is ready for a final review.

[Blake Rouse (blake-rouse)]

Looks good. Thanks for the cleanup of the allocation, will be modifying that soon for composition!

review: Approve

=== modified file 'src/maasserver/models/interface.py'
--- src/maasserver/models/interface.py	2016-12-07 12:46:14 +0000
+++ src/maasserver/models/interface.py	2016-12-08 16:07:44 +0000
@@ -106,7 +106,7 @@
             'name': '__name',
             'hostname': 'node__hostname',
             'subnet': (Subnet.objects, 'staticipaddress__interface'),
-            'space': 'subnet{s}space'.format(s=separator),
+            'space': self._add_space_query,
             'subnet_cidr': 'subnet{s}cidr'.format(s=separator),
             'type': '__type',
             'vlan': (VLAN.objects, 'interface'),
@@ -125,6 +125,15 @@
         else:
             return op(current_q, Q(id=item))
 
+    def _add_space_query(self, current_q, op, space):
+        """Query for a related VLAN or subnet with the specified space."""
+        # Circular imports.
+        from maasserver.models import Space
+        space = Space.objects.get_object_by_specifiers_or_raise(space)
+        current_q = op(
+            current_q, Q(vlan__subnet__space=space) | Q(vlan__space=space))
+        return current_q
+
     def _add_default_query(self, current_q, op, item):
         # First, just try treating this as an interface ID.
         try:

=== modified file 'src/maasserver/models/tests/test_interface.py'
--- src/maasserver/models/tests/test_interface.py	2016-12-07 12:46:14 +0000
+++ src/maasserver/models/tests/test_interface.py	2016-12-08 16:07:44 +0000
@@ -533,7 +533,7 @@
                 ["subnet_cidr:%s" % subnet1.cidr,
                  "subnet_cidr:%s" % subnet2.cidr], ), [iface1, iface2])
 
-    def test__filter_by_specifiers_matches_space(self):
+    def test__filter_by_specifiers_matches_space_by_subnet(self):
         space1 = factory.make_Space()
         space2 = factory.make_Space()
         subnet1 = factory.make_Subnet(space=space1)
@@ -555,6 +555,30 @@
                 ["space:%s" % space1.name,
                  "space:%s" % space2.name], ), [iface1, iface2])
 
+    def test__filter_by_specifiers_matches_space_by_vlan(self):
+        space1 = factory.make_Space()
+        space2 = factory.make_Space()
+        vlan1 = factory.make_VLAN(space=space1)
+        vlan2 = factory.make_VLAN(space=space2)
+        subnet1 = factory.make_Subnet(vlan=vlan1)
+        subnet2 = factory.make_Subnet(vlan=vlan2)
+        node1 = factory.make_Node_with_Interface_on_Subnet(
+            subnet=subnet1, with_dhcp_rack_primary=False)
+        node2 = factory.make_Node_with_Interface_on_Subnet(
+            subnet=subnet2, with_dhcp_rack_primary=False)
+        iface1 = node1.get_boot_interface()
+        iface2 = node2.get_boot_interface()
+        self.assertItemsEqual(
+            Interface.objects.filter_by_specifiers(
+                "space:%s" % space1.name), [iface1])
+        self.assertItemsEqual(
+            Interface.objects.filter_by_specifiers(
+                "space:%s" % space2.name), [iface2])
+        self.assertItemsEqual(
+            Interface.objects.filter_by_specifiers(
+                ["space:%s" % space1.name,
+                 "space:%s" % space2.name], ), [iface1, iface2])
+
     def test__filter_by_specifiers_matches_type(self):
         physical = factory.make_Interface()
         bond = factory.make_Interface(

=== modified file 'src/maasserver/models/tests/test_vlan.py'
--- src/maasserver/models/tests/test_vlan.py	2016-12-07 12:46:14 +0000
+++ src/maasserver/models/tests/test_vlan.py	2016-12-08 16:07:44 +0000
@@ -65,6 +65,26 @@
             [vlan]
         )
 
+    def test__space_specifier_matches_space_by_name(self):
+        factory.make_VLAN()
+        space = factory.make_Space()
+        vlan = factory.make_VLAN(space=space)
+        factory.make_VLAN()
+        self.assertItemsEqual(
+            VLAN.objects.filter_by_specifiers('space:%s' % space.name),
+            [vlan]
+        )
+
+    def test__space_specifier_matches_space_by_id(self):
+        factory.make_VLAN()
+        space = factory.make_Space()
+        vlan = factory.make_VLAN(space=space)
+        factory.make_VLAN()
+        self.assertItemsEqual(
+            VLAN.objects.filter_by_specifiers('space:%s' % space.id),
+            [vlan]
+        )
+
     def test__class_specifier_matches_attached_subnet(self):
         factory.make_VLAN()
         vlan = factory.make_VLAN()

=== modified file 'src/maasserver/models/vlan.py'
--- src/maasserver/models/vlan.py	2016-12-07 16:40:35 +0000
+++ src/maasserver/models/vlan.py	2016-12-08 16:07:44 +0000
@@ -29,6 +29,7 @@
 from maasserver.models.interface import VLANInterface
 from maasserver.models.timestampedmodel import TimestampedModel
 from maasserver.utils.orm import MAASQueriesMixin
+from netaddr import AddrFormatError
 from provisioningserver.utils.network import parse_integer
 
 
@@ -53,6 +54,7 @@
         # Circular imports.
         from maasserver.models import (
             Fabric,
+            Space,
             Subnet,
         )
 
@@ -65,6 +67,7 @@
             'id': "__id",
             'name': "__name",
             'subnet': (Subnet.objects, 'vlan'),
+            'space': (Space.objects, 'vlan'),
             'vid': self._add_vid_query,
         }
         return super(VLANQueriesMixin, self).get_specifiers_q(
@@ -105,6 +108,13 @@
         current_q = op(current_q, Q(vid=vid))
         return current_q
 
+    def validate_filter_specifiers(self, specifiers):
+        """Validate the given filter string."""
+        try:
+            self.filter_by_specifiers(specifiers)
+        except (ValueError, AddrFormatError) as e:
+            raise ValidationError(e.message)
+
 
 class VLANQuerySet(QuerySet, VLANQueriesMixin):
     """Custom QuerySet which mixes in some additional queries specific to

=== modified file 'src/maasserver/node_constraint_filter_forms.py'
--- src/maasserver/node_constraint_filter_forms.py	2016-11-02 23:46:43 +0000
+++ src/maasserver/node_constraint_filter_forms.py	2016-12-08 16:07:44 +0000
@@ -30,6 +30,7 @@
     PhysicalBlockDevice,
     Subnet,
     Tag,
+    VLAN,
     Zone,
 )
 from maasserver.models.zone import ZONE_NAME_VALIDATOR
@@ -516,6 +517,22 @@
             "Invalid parameter: list of subnet specifiers required.",
             })
 
+    vlans = ValidatorMultipleChoiceField(
+        validator=VLAN.objects.validate_filter_specifiers,
+        label="Attached to VLANs",
+        required=False, error_messages={
+            'invalid_list':
+            "Invalid parameter: list of VLAN specifiers required.",
+            })
+
+    not_vlans = ValidatorMultipleChoiceField(
+        validator=VLAN.objects.validate_filter_specifiers,
+        label="Not attached to VLANs",
+        required=False, error_messages={
+            'invalid_list':
+            "Invalid parameter: list of VLAN specifiers required.",
+            })
+
     connected_to = ValidatorMultipleChoiceField(
         validator=mac_validator, label="Connected to", required=False,
         error_messages={
@@ -616,21 +633,30 @@
             return None
         return value
 
-    def _clean_subnet_specifiers(self, specifiers):
+    def _clean_specifiers(self, model, specifiers):
         if not specifiers:
             return None
-        subnets = set(Subnet.objects.filter_by_specifiers(specifiers))
-        if len(subnets) == 0:
-            raise ValidationError("No matching subnets found.")
-        return subnets
+        objects = set(model.objects.filter_by_specifiers(specifiers))
+        if len(objects) == 0:
+            raise ValidationError(
+                "No matching %s found." % model._meta.verbose_name_plural)
+        return objects
 
     def clean_subnets(self):
         value = self.cleaned_data[self.get_field_name('subnets')]
-        return self._clean_subnet_specifiers(value)
+        return self._clean_specifiers(Subnet, value)
 
     def clean_not_subnets(self):
         value = self.cleaned_data[self.get_field_name('not_subnets')]
-        return self._clean_subnet_specifiers(value)
+        return self._clean_specifiers(Subnet, value)
+
+    def clean_vlans(self):
+        value = self.cleaned_data[self.get_field_name('vlans')]
+        return self._clean_specifiers(VLAN, value)
+
+    def clean_not_vlans(self):
+        value = self.cleaned_data[self.get_field_name('not_vlans')]
+        return self._clean_specifiers(VLAN, value)
 
     def __init__(self, *args, **kwargs):
         super(AcquireNodeForm, self).__init__(*args, **kwargs)
@@ -694,7 +720,161 @@
         :rtype: `django.db.models.query.QuerySet`
         """
         filtered_nodes = nodes
-
+        filtered_nodes = self.filter_by_hostname(filtered_nodes)
+        filtered_nodes = self.filter_by_system_id(filtered_nodes)
+        filtered_nodes = self.filter_by_arch(filtered_nodes)
+        filtered_nodes = self.filter_by_cpu_count(filtered_nodes)
+        filtered_nodes = self.filter_by_mem(filtered_nodes)
+        filtered_nodes = self.filter_by_tags(filtered_nodes)
+        filtered_nodes = self.filter_by_zone(filtered_nodes)
+        filtered_nodes = self.filter_by_subnets(filtered_nodes)
+        filtered_nodes = self.filter_by_vlans(filtered_nodes)
+        filtered_nodes = self.filter_by_fabrics(filtered_nodes)
+        filtered_nodes = self.filter_by_fabric_classes(filtered_nodes)
+        compatible_nodes, filtered_nodes = self.filter_by_storage(
+            filtered_nodes)
+        compatible_interfaces, filtered_nodes = self.filter_by_interfaces(
+            filtered_nodes)
+        filtered_nodes = self.reorder_nodes_by_cost(filtered_nodes)
+        return filtered_nodes, compatible_nodes, compatible_interfaces
+
+    def reorder_nodes_by_cost(self, filtered_nodes):
+        # This uses a very simple procedure to compute a machine's
+        # cost. This procedure is loosely based on how ec2 computes
+        # the costs of machines. This is here to give a hint to let
+        # the call to acquire() decide which machine to return based
+        # on the machine's cost when multiple machines match the
+        # constraints.
+        filtered_nodes = filtered_nodes.distinct().extra(
+            select={'cost': "cpu_count + memory / 1024."})
+        return filtered_nodes.order_by("cost")
+
+    def filter_by_interfaces(self, filtered_nodes):
+        compatible_interfaces = {}
+        interfaces_label_map = self.cleaned_data.get(
+            self.get_field_name('interfaces'))
+        if interfaces_label_map is not None:
+            node_ids, compatible_interfaces = nodes_by_interface(
+                interfaces_label_map)
+            if node_ids is not None:
+                filtered_nodes = filtered_nodes.filter(id__in=node_ids)
+
+        return compatible_interfaces, filtered_nodes
+
+    def filter_by_storage(self, filtered_nodes):
+        compatible_nodes = {}  # Maps node/storage to named storage constraints
+        storage = self.cleaned_data.get(
+            self.get_field_name('storage'))
+        if storage:
+            compatible_nodes = nodes_by_storage(storage)
+            node_ids = list(compatible_nodes)
+            if node_ids is not None:
+                filtered_nodes = filtered_nodes.filter(id__in=node_ids)
+        return compatible_nodes, filtered_nodes
+
+    def filter_by_fabric_classes(self, filtered_nodes):
+        fabric_classes = self.cleaned_data.get(self.get_field_name(
+            'fabric_classes'))
+        if fabric_classes is not None and len(fabric_classes) > 0:
+            filtered_nodes = filtered_nodes.filter(
+                interface__vlan__fabric__class_type__in=fabric_classes)
+        not_fabric_classes = self.cleaned_data.get(self.get_field_name(
+            'not_fabric_classes'))
+        if not_fabric_classes is not None and len(not_fabric_classes) > 0:
+            filtered_nodes = filtered_nodes.exclude(
+                interface__vlan__fabric__class_type__in=not_fabric_classes)
+        return filtered_nodes
+
+    def filter_by_fabrics(self, filtered_nodes):
+        fabrics = self.cleaned_data.get(self.get_field_name('fabrics'))
+        if fabrics is not None and len(fabrics) > 0:
+            # XXX mpontillo 2015-10-30 need to also handle fabrics whose name
+            # is null (fabric-<id>).
+            filtered_nodes = filtered_nodes.filter(
+                interface__vlan__fabric__name__in=fabrics)
+        not_fabrics = self.cleaned_data.get(self.get_field_name('not_fabrics'))
+        if not_fabrics is not None and len(not_fabrics) > 0:
+            # XXX mpontillo 2015-10-30 need to also handle fabrics whose name
+            # is null (fabric-<id>).
+            filtered_nodes = filtered_nodes.exclude(
+                interface__vlan__fabric__name__in=not_fabrics)
+        return filtered_nodes
+
+    def filter_by_vlans(self, filtered_nodes):
+        vlans = self.cleaned_data.get(self.get_field_name('vlans'))
+        if vlans is not None and len(vlans) > 0:
+            for vlan in set(vlans):
+                filtered_nodes = filtered_nodes.filter(
+                    interface__vlan=vlan)
+        not_vlans = self.cleaned_data.get(self.get_field_name('not_vlans'))
+        if not_vlans is not None and len(not_vlans) > 0:
+            for not_vlan in set(not_vlans):
+                filtered_nodes = filtered_nodes.exclude(
+                    interface__vlan=not_vlan)
+        return filtered_nodes
+
+    def filter_by_subnets(self, filtered_nodes):
+        subnets = self.cleaned_data.get(self.get_field_name('subnets'))
+        if subnets is not None and len(subnets) > 0:
+            for subnet in set(subnets):
+                filtered_nodes = filtered_nodes.filter(
+                    interface__ip_addresses__subnet=subnet)
+        not_subnets = self.cleaned_data.get(
+            self.get_field_name('not_subnets'))
+        if not_subnets is not None and len(not_subnets) > 0:
+            for not_subnet in set(not_subnets):
+                filtered_nodes = filtered_nodes.exclude(
+                    interface__ip_addresses__subnet=not_subnet)
+        return filtered_nodes
+
+    def filter_by_zone(self, filtered_nodes):
+        zone = self.cleaned_data.get(self.get_field_name('zone'))
+        if zone:
+            zone_obj = Zone.objects.get(name=zone)
+            filtered_nodes = filtered_nodes.filter(zone=zone_obj)
+        not_in_zone = self.cleaned_data.get(self.get_field_name('not_in_zone'))
+        if not_in_zone is not None and len(not_in_zone) > 0:
+            not_in_zones = Zone.objects.filter(name__in=not_in_zone)
+            filtered_nodes = filtered_nodes.exclude(zone__in=not_in_zones)
+        return filtered_nodes
+
+    def filter_by_tags(self, filtered_nodes):
+        tags = self.cleaned_data.get(self.get_field_name('tags'))
+        if tags:
+            for tag in tags:
+                filtered_nodes = filtered_nodes.filter(tags__name=tag)
+        not_tags = self.cleaned_data.get(self.get_field_name('not_tags'))
+        if len(not_tags) > 0:
+            for not_tag in not_tags:
+                filtered_nodes = filtered_nodes.exclude(tags__name=not_tag)
+        return filtered_nodes
+
+    def filter_by_mem(self, filtered_nodes):
+        mem = self.cleaned_data.get(self.get_field_name('mem'))
+        if mem:
+            filtered_nodes = filtered_nodes.filter(memory__gte=mem)
+        return filtered_nodes
+
+    def filter_by_cpu_count(self, filtered_nodes):
+        cpu_count = self.cleaned_data.get(self.get_field_name('cpu_count'))
+        if cpu_count:
+            filtered_nodes = filtered_nodes.filter(cpu_count__gte=cpu_count)
+        return filtered_nodes
+
+    def filter_by_arch(self, filtered_nodes):
+        arch = self.cleaned_data.get(self.get_field_name('arch'))
+        if arch:
+            filtered_nodes = filtered_nodes.filter(architecture__in=arch)
+        return filtered_nodes
+
+    def filter_by_system_id(self, filtered_nodes):
+        # Filter by system_id.
+        system_id = self.cleaned_data.get(self.get_field_name('system_id'))
+        if system_id:
+            filtered_nodes = filtered_nodes.filter(system_id=system_id)
+        return filtered_nodes
+
+    def filter_by_hostname(self, filtered_nodes):
         # Filter by hostname.
         hostname = self.cleaned_data.get(self.get_field_name('name'))
         if hostname:
@@ -708,125 +888,4 @@
             else:
                 clause = Q(hostname=hostname)
             filtered_nodes = filtered_nodes.filter(clause)
-
-        # Filter by system_id.
-        system_id = self.cleaned_data.get(self.get_field_name('system_id'))
-        if system_id:
-            filtered_nodes = filtered_nodes.filter(system_id=system_id)
-
-        # Filter by architecture.
-        arch = self.cleaned_data.get(self.get_field_name('arch'))
-        if arch:
-            filtered_nodes = filtered_nodes.filter(architecture__in=arch)
-
-        # Filter by cpu_count.
-        cpu_count = self.cleaned_data.get(self.get_field_name('cpu_count'))
-        if cpu_count:
-            filtered_nodes = filtered_nodes.filter(cpu_count__gte=cpu_count)
-
-        # Filter by memory.
-        mem = self.cleaned_data.get(self.get_field_name('mem'))
-        if mem:
-            filtered_nodes = filtered_nodes.filter(memory__gte=mem)
-
-        # Filter by tags.
-        tags = self.cleaned_data.get(self.get_field_name('tags'))
-        if tags:
-            for tag in tags:
-                filtered_nodes = filtered_nodes.filter(tags__name=tag)
-
-        # Filter by not_tags.
-        not_tags = self.cleaned_data.get(self.get_field_name('not_tags'))
-        if len(not_tags) > 0:
-            for not_tag in not_tags:
-                filtered_nodes = filtered_nodes.exclude(tags__name=not_tag)
-
-        # Filter by zone.
-        zone = self.cleaned_data.get(self.get_field_name('zone'))
-        if zone:
-            zone_obj = Zone.objects.get(name=zone)
-            filtered_nodes = filtered_nodes.filter(zone=zone_obj)
-
-        # Filter by not_in_zone.
-        not_in_zone = self.cleaned_data.get(self.get_field_name('not_in_zone'))
-        if not_in_zone is not None and len(not_in_zone) > 0:
-            not_in_zones = Zone.objects.filter(name__in=not_in_zone)
-            filtered_nodes = filtered_nodes.exclude(zone__in=not_in_zones)
-
-        # Filter by subnet.
-        subnets = self.cleaned_data.get(self.get_field_name('subnets'))
-        if subnets is not None and len(subnets) > 0:
-            for subnet in set(subnets):
-                filtered_nodes = filtered_nodes.filter(
-                    interface__ip_addresses__subnet=subnet)
-
-        # Filter by not_subnets.
-        not_subnets = self.cleaned_data.get(
-            self.get_field_name('not_subnets'))
-        if not_subnets is not None and len(not_subnets) > 0:
-            for not_subnet in set(not_subnets):
-                filtered_nodes = filtered_nodes.exclude(
-                    interface__ip_addresses__subnet=not_subnet)
-
-        # Filter by fabrics.
-        fabrics = self.cleaned_data.get(self.get_field_name('fabrics'))
-        if fabrics is not None and len(fabrics) > 0:
-            # XXX mpontillo 2015-10-30 need to also handle fabrics whose name
-            # is null (fabric-<id>).
-            filtered_nodes = filtered_nodes.filter(
-                interface__vlan__fabric__name__in=fabrics)
-
-        # Filter by not_fabrics.
-        not_fabrics = self.cleaned_data.get(self.get_field_name('not_fabrics'))
-        if not_fabrics is not None and len(not_fabrics) > 0:
-            # XXX mpontillo 2015-10-30 need to also handle fabrics whose name
-            # is null (fabric-<id>).
-            filtered_nodes = filtered_nodes.exclude(
-                interface__vlan__fabric__name__in=not_fabrics)
-
-        # Filter by fabric classes.
-        fabric_classes = self.cleaned_data.get(self.get_field_name(
-            'fabric_classes'))
-        if fabric_classes is not None and len(fabric_classes) > 0:
-            filtered_nodes = filtered_nodes.filter(
-                interface__vlan__fabric__class_type__in=fabric_classes)
-
-        # Filter by not_fabric_classes.
-        not_fabric_classes = self.cleaned_data.get(self.get_field_name(
-            'not_fabric_classes'))
-        if not_fabric_classes is not None and len(not_fabric_classes) > 0:
-            filtered_nodes = filtered_nodes.exclude(
-                interface__vlan__fabric__class_type__in=not_fabric_classes)
-
-        # Filter by storage.
-        compatible_nodes = {}  # Maps node/storage to named storage constraints
-        storage = self.cleaned_data.get(
-            self.get_field_name('storage'))
-        if storage:
-            compatible_nodes = nodes_by_storage(storage)
-            node_ids = list(compatible_nodes)
-            if node_ids is not None:
-                filtered_nodes = filtered_nodes.filter(id__in=node_ids)
-
-        # Filter by interfaces (networking)
-        compatible_interfaces = {}
-        interfaces_label_map = self.cleaned_data.get(
-            self.get_field_name('interfaces'))
-        if interfaces_label_map is not None:
-            node_ids, compatible_interfaces = nodes_by_interface(
-                interfaces_label_map)
-            if node_ids is not None:
-                filtered_nodes = filtered_nodes.filter(id__in=node_ids)
-
-        # This uses a very simple procedure to compute a machine's
-        # cost. This procedure is loosely based on how ec2 computes
-        # the costs of machines. This is here to give a hint to let
-        # the call to acquire() decide which machine to return based
-        # on the machine's cost when multiple machines match the
-        # constraints.
-        filtered_nodes = filtered_nodes.distinct().extra(
-            select={'cost': "cpu_count + memory / 1024."})
-        return (
-            filtered_nodes.order_by("cost"), compatible_nodes,
-            compatible_interfaces
-        )
+        return filtered_nodes

=== modified file 'src/maasserver/testing/factory.py'
--- src/maasserver/testing/factory.py	2016-12-07 16:40:35 +0000
+++ src/maasserver/testing/factory.py	2016-12-08 16:07:44 +0000
@@ -1040,11 +1040,15 @@
             self, name=None, vid=None, fabric=None, dhcp_on=False, space=None,
             primary_rack=None, secondary_rack=None, relay_vlan=None):
         assert vid != 0, "VID=0 VLANs are auto-created"
+        if name is RANDOM:
+            name = factory.make_name()
         if fabric is None:
             fabric = Fabric.objects.get_default_fabric()
         if vid is None:
             # Don't create the vid=0 VLAN, it's auto-created.
             vid = self._get_available_vid(fabric)
+        if space is RANDOM:
+            space = factory.make_Space()
         vlan = VLAN(
             name=name, vid=vid, fabric=fabric, dhcp_on=dhcp_on, space=space,
             primary_rack=primary_rack, secondary_rack=secondary_rack,

=== modified file 'src/maasserver/tests/test_node_constraint_filter_forms.py'
--- src/maasserver/tests/test_node_constraint_filter_forms.py	2016-10-27 23:07:18 +0000
+++ src/maasserver/tests/test_node_constraint_filter_forms.py	2016-12-08 16:07:44 +0000
@@ -31,12 +31,16 @@
     RenamableFieldsForm,
 )
 from maasserver.testing.architecture import patch_usable_architectures
-from maasserver.testing.factory import factory
+from maasserver.testing.factory import (
+    factory,
+    RANDOM,
+)
 from maasserver.testing.testcase import MAASServerTestCase
 from maasserver.utils import ignore_unused
 from testtools.matchers import (
     Contains,
     ContainsAll,
+    Equals,
     MatchesDict,
     MatchesListwise,
     StartsWith,
@@ -356,6 +360,89 @@
                 ]
             })
 
+    def test_vlans_filters_by_space(self):
+        vlans = [
+            factory.make_VLAN(space=RANDOM)
+            for _ in range(3)
+        ]
+        subnets = [
+            factory.make_Subnet(vlan=vlan)
+            for vlan in vlans
+        ]
+        nodes = [
+            factory.make_Node_with_Interface_on_Subnet(subnet=subnet)
+            for subnet in subnets
+        ]
+        # Filter for this subnet.  Take one in the middle to avoid
+        # coincidental success based on ordering.
+        pick = 1
+        self.assertConstrainedNodes(
+            {nodes[pick]},
+            {'vlans': ["space:%s" % vlans[pick].space.name]})
+
+    def test_vlans_filters_by_multiple_not_space_arguments(self):
+        # Create 3 different VLANs (will be on 3 random spaces)
+        vlans = [
+            factory.make_VLAN(space=RANDOM)
+            for _ in range(3)
+        ]
+        subnets = [
+            factory.make_Subnet(vlan=vlan)
+            for vlan in vlans
+            ]
+        nodes = [
+            factory.make_Node_with_Interface_on_Subnet(subnet=subnet)
+            for subnet in subnets
+        ]
+        expected_selection = randint(0, len(vlans) - 1)
+        expected_node = nodes[expected_selection]
+        # Remove the expected subnet from the list of subnets; we'll use the
+        # remaining subnets to filter the list.
+        del vlans[expected_selection]
+        self.assertConstrainedNodes(
+            {expected_node},
+            {
+                'not_vlans': [
+                    "space:%s" % vlan.space.name
+                    for vlan in vlans
+                ]
+            })
+
+    def test_fails_validation_for_no_matching_vlans(self):
+        form = AcquireNodeForm(data={
+            'vlans': ["space:foo"]
+        })
+        self.assertThat(form.is_valid(), Equals(False))
+        self.assertThat(
+            dict(form.errors)['vlans'], Equals(["No matching VLANs found."]))
+
+    def test_fails_validation_for_no_matching_not_vlans(self):
+        form = AcquireNodeForm(data={
+            'not_vlans': ["space:foo"]
+        })
+        self.assertThat(form.is_valid(), Equals(False))
+        self.assertThat(
+            dict(form.errors)['not_vlans'],
+            Equals(["No matching VLANs found."]))
+
+    def test_fails_validation_for_no_matching_subnets(self):
+        form = AcquireNodeForm(data={
+            'subnets': ["foo"]
+        })
+        self.assertThat(form.is_valid(), Equals(False))
+        self.assertThat(
+            dict(form.errors)['subnets'],
+            Equals(["No matching subnets found."]))
+
+    def test_fails_validation_for_no_matching_not_subnets(self):
+        form = AcquireNodeForm(data={
+            'not_subnets': ["foo"]
+        })
+        self.assertThat(form.is_valid(), Equals(False))
+        self.assertThat(
+            dict(form.errors)['not_subnets'],
+            Equals(["No matching subnets found."]))
+
     def test_subnets_filters_by_ip(self):
         subnets = [
             factory.make_Subnet()
@@ -1129,6 +1216,8 @@
             'not_tags': [factory.make_Tag().name],
             'subnets': [factory.make_Subnet().name],
             'not_subnets': [factory.make_Subnet().name],
+            'vlans': ['name:' + factory.make_VLAN(name=RANDOM).name],
+            'not_vlans': ['name:' + factory.make_VLAN(name=RANDOM).name],
             'connected_to': [factory.make_mac_address()],
             'not_connected_to': [factory.make_mac_address()],
             'zone': factory.make_Zone(),
@@ -1158,7 +1247,7 @@
 
 class TestAcquireNodeFormOrdersResults(MAASServerTestCase):
 
-    def test_describe_constraints_shows_all_constraints(self):
+    def test_describe_constraints_orders_based_on_cost(self):
         nodes = [
             factory.make_Node(
                 cpu_count=randint(5, 32),