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

[Blake Rouse]

Fix storage constraints to map the root disk to the disk with '/' on it. Non root constraints map to block devices that are unused, but can be a virtual block device.

[Andres Rodriguez (andreserl)]

lgtm!

review: Approve

=== modified file 'src/maasserver/api/tests/test_machines.py'
--- src/maasserver/api/tests/test_machines.py	2017-04-05 21:06:53 +0000
+++ src/maasserver/api/tests/test_machines.py	2017-04-11 16:17:53 +0000
@@ -686,7 +686,7 @@
             status=available_status, owner=None, with_boot_disk=False)
         disk_1 = factory.make_PhysicalBlockDevice(
             node=machine, size=(random.randint(8, 16) * (1000 ** 3)),
-            tags=['local'])
+            tags=['local'], formatted_root=True)
         disk_2 = factory.make_ISCSIBlockDevice(
             node=machine, size=(random.randint(8, 16) * (1000 ** 3)),
             tags=['iscsi'])
@@ -1146,11 +1146,9 @@
         """Storage label is returned alongside machine data"""
         machine = factory.make_Node(
             status=NODE_STATUS.READY, with_boot_disk=False)
-        # The ID may always be '1', which won't be interesting for testing.
-        for _ in range(1, random.choice([1, 3, 5])):
-            factory.make_PhysicalBlockDevice()
         factory.make_PhysicalBlockDevice(
-            node=machine, size=11 * (1000 ** 3), tags=['ssd'])
+            node=machine, size=11 * (1000 ** 3), tags=['ssd'],
+            formatted_root=True)
         response = self.client.post(reverse('machines_handler'), {
             'op': 'allocate',
             'storage': 'needed:10(ssd)',
@@ -1169,11 +1167,9 @@
         """Storage label is returned alongside machine data"""
         machine = factory.make_Node(
             status=NODE_STATUS.READY, with_boot_disk=False)
-        # The ID may always be '1', which won't be interesting for testing.
-        for _ in range(1, random.choice([1, 3, 5])):
-            factory.make_PhysicalBlockDevice()
         factory.make_PhysicalBlockDevice(
-            node=machine, size=11 * (1000 ** 3), tags=['ssd'])
+            node=machine, size=11 * (1000 ** 3), tags=['ssd'],
+            formatted_root=True)
         response = self.client.post(reverse('machines_handler'), {
             'op': 'allocate',
             'storage': 'needed:10(ssd)',
 
=== modified file 'src/maasserver/node_constraint_filter_forms.py'
--- src/maasserver/node_constraint_filter_forms.py	2017-04-05 13:41:28 +0000
+++ src/maasserver/node_constraint_filter_forms.py	2017-04-11 16:17:53 +0000
@@ -27,12 +27,11 @@
 import maasserver.forms as maasserver_forms
 from maasserver.models import (
     BlockDevice,
+    Filesystem,
     Interface,
-    PhysicalBlockDevice,
     Pod,
     Subnet,
     Tag,
-    VirtualBlockDevice,
     VLAN,
     Zone,
 )
@@ -319,71 +318,70 @@
     root_device = True  # The 1st constraint refers to the node's 1st device
     for constraint_name, size, tags in constraints:
         if root_device:
-            # Sort the `PhysicalBlockDevice`s by id because we consider the
-            # first device as the root device.
+            # This branch of the if is only used on first iteration.
             root_device = False
-            matched_devices = PhysicalBlockDevice.objects.all()
+
+            # Use only block devices that are mounted as '/'. Either the
+            # block device has root sitting on it or its on a partition on
+            # that block device.
+            filesystems = Filesystem.objects.filter(
+                mount_point='/', acquired=False)
+            filesystems = filesystems.filter(
+                Q(block_device__size__gte=size) |
+                Q(partition__partition_table__block_device__size__gte=size))
+            if tags is not None:
+                filesystems = filesystems.filter(
+                    Q(block_device__tags__contains=tags) |
+                    Q(**{
+                        'partition__partition_table__block_device'
+                        '__tags__contains': tags
+                    }))
             if node_ids is not None:
-                matched_devices = matched_devices.filter(node_id__in=node_ids)
-            matched_devices = matched_devices.order_by('id').values(
-                'id', 'node_id', 'size', 'tags')
+                filesystems = filesystems.filter(
+                    Q(block_device__node_id__in=node_ids) |
+                    Q(**{
+                        'partition__partition_table__block_device'
+                        '__node_id__in': node_ids
+                    }))
+            filesystems = filesystems.prefetch_related(
+                'block_device', 'partition__partition_table__block_device')
 
             # Only keep the first device for every node. This is done to make
             # sure filtering out the size and tags is not done to all the
             # block devices. This should only be done to the first block
             # device.
             found_nodes = set()
-            devices = []
-            for device in matched_devices:
-                if device['node_id'] in found_nodes:
+            matched_devices = []
+            for filesystem in filesystems:
+                if filesystem.block_device is not None:
+                    device = filesystem.block_device
+                else:
+                    device = (
+                        filesystem.partition.partition_table.block_device)
+                if device.node_id in found_nodes:
                     continue
-                devices.append(device)
-                found_nodes.add(device['node_id'])
-
-            # Remove the devices that are not of correct size and the devices
-            # that are missing the correct tags or label.
-            devices = [
-                device
-                for device in devices
-                if device['size'] >= size
-                ]
+                matched_devices.append(device)
+                found_nodes.add(device.node_id)
+        else:
+            # Query for any block device the closest size and, if specified,
+            # the given tags. # The block device must also be unused in the
+            # storage model.
+            matched_devices = BlockDevice.objects.filter(size__gte=size)
+            matched_devices = matched_devices.filter(
+                filesystem__isnull=True, partitiontable__isnull=True)
             if tags is not None:
-                tags = set(tags)
-                devices = [
-                    device
-                    for device in devices
-                    if tags.issubset(set(device['tags']))
-                    ]
-            matched_devices = devices
-        else:
-            # Query for the `PhysicalBlockDevice`s and `ISCSIBlockDevice`'s
-            # that have the closest size and, if specified, the given tags.
-            if tags is None:
-                matched_devices = BlockDevice.objects.filter(
-                    size__gte=size).order_by('size')
-            else:
-                matched_devices = BlockDevice.objects.filter_by_tags(
-                    tags).filter(size__gte=size)
+                matched_devices = matched_devices.filter(tags__contains=tags)
             if node_ids is not None:
                 matched_devices = matched_devices.filter(
                     node_id__in=node_ids)
-            matched_devices = [
-                {
-                    'id': device.id,
-                    'node_id': device.node_id,
-                    'size': device.size,
-                    'tags': device.tags,
-                }
-                for device in matched_devices.order_by('size')
-                if not isinstance(device.actual_instance, VirtualBlockDevice)
-            ]
+            matched_devices = list(matched_devices.order_by('size'))
 
         # Loop through all the returned devices. Insert only the first
         # device from each node into `matches`.
         matched_in_loop = []
         for device in matched_devices:
-            device_id = device['id']
-            device_node_id = device['node_id']
+            device_id = device.id
+            device_node_id = device.node_id
 
             if device_node_id in matched_in_loop:
                 continue
 
=== modified file 'src/maasserver/testing/factory.py'
--- src/maasserver/testing/factory.py	2017-04-10 21:49:47 +0000
+++ src/maasserver/testing/factory.py	2017-04-11 16:17:53 +0000
@@ -1793,7 +1793,8 @@
 
     def make_PhysicalBlockDevice(
             self, node=None, name=None, size=None, block_size=None,
-            tags=None, model=None, serial=None, id_path=None):
+            tags=None, model=None, serial=None, id_path=None,
+            formatted_root=False):
         if node is None:
             node = self.make_Node()
         if name is None:
@@ -1819,9 +1820,15 @@
         else:
             model = ""
             serial = ""
-        return PhysicalBlockDevice.objects.create(
+        block_device = PhysicalBlockDevice.objects.create(
             node=node, name=name, size=size, block_size=block_size,
             tags=tags, model=model, serial=serial, id_path=id_path)
+        if formatted_root:
+            partition = self.make_Partition(
+                partition_table=(
+                    self.make_PartitionTable(block_device=block_device)))
+            self.make_Filesystem(mount_point='/', partition=partition)
+        return block_device
 
     def make_PartitionTable(
             self, table_type=None, block_device=None, node=None,
 
=== modified file 'src/maasserver/tests/test_node_constraint_filter_forms.py'
--- src/maasserver/tests/test_node_constraint_filter_forms.py	2017-03-15 20:40:52 +0000
+++ src/maasserver/tests/test_node_constraint_filter_forms.py	2017-04-11 16:17:53 +0000
@@ -6,11 +6,12 @@
 __all__ = []
 
 from random import randint
-from unittest import skip
 
 from django import forms
 from django.core.exceptions import ValidationError
 from maasserver.enum import (
+    FILESYSTEM_GROUP_TYPE,
+    FILESYSTEM_TYPE,
     INTERFACE_TYPE,
     IPADDRESS_TYPE,
     NODE_STATUS,
@@ -835,34 +836,50 @@
                 ['Malformed storage constraint, "abc".']}),
             (form.is_valid(), form.errors))
 
-    def test_storage_single_contraint_only_matches_physical_devices(self):
+    def test_storage_matches_disk_with_root_mount_on_disk(self):
         node1 = factory.make_Node(with_boot_disk=False)
         factory.make_PhysicalBlockDevice(node=node1)
-        node2 = factory.make_Node(with_boot_disk=False)
-        factory.make_BlockDevice(node=node2)
+        block_device = factory.make_PhysicalBlockDevice(node=node1)
+        factory.make_Filesystem(mount_point='/', block_device=block_device)
+        node2 = factory.make_Node(with_boot_disk=False)
+        factory.make_PhysicalBlockDevice(node=node2)
+        self.assertConstrainedNodes([node1], {'storage': '0'})
+
+    def test_storage_matches_disk_with_root_mount_on_partition(self):
+        node1 = factory.make_Node(with_boot_disk=False)
+        factory.make_PhysicalBlockDevice(
+            node=node1, formatted_root=True)
+        node2 = factory.make_Node(with_boot_disk=False)
+        block_device = factory.make_PhysicalBlockDevice(node=node2)
+        partition_table = factory.make_PartitionTable(
+            block_device=block_device)
+        partition = factory.make_Partition(partition_table=partition_table)
+        factory.make_Filesystem(mount_point='/srv', partition=partition)
         self.assertConstrainedNodes([node1], {'storage': '0'})
 
     def test_storage_single_contraint_matches_all_sizes_larger(self):
         node1 = factory.make_Node(with_boot_disk=False)
         # 1gb block device
         factory.make_PhysicalBlockDevice(
-            node=node1, size=1 * (1000 ** 3))
+            node=node1, size=1 * (1000 ** 3), formatted_root=True)
         node2 = factory.make_Node(with_boot_disk=False)
         # 4gb block device
         factory.make_PhysicalBlockDevice(
-            node=node2, size=4 * (1000 ** 3))
+            node=node2, size=4 * (1000 ** 3), formatted_root=True)
         node3 = factory.make_Node(with_boot_disk=False)
         # 8gb block device
         factory.make_PhysicalBlockDevice(
-            node=node3, size=8 * (1000 ** 3))
+            node=node3, size=8 * (1000 ** 3), formatted_root=True)
         # all nodes with physical devices larger than 2gb
         self.assertConstrainedNodes([node2, node3], {'storage': '2'})
 
     def test_storage_single_contraint_matches_on_tags(self):
         node1 = factory.make_Node(with_boot_disk=False)
-        factory.make_PhysicalBlockDevice(node=node1, tags=['ssd'])
+        factory.make_PhysicalBlockDevice(
+            node=node1, tags=['ssd'], formatted_root=True)
         node2 = factory.make_Node(with_boot_disk=False)
-        factory.make_PhysicalBlockDevice(node=node2, tags=['rotary'])
+        factory.make_PhysicalBlockDevice(
+            node=node2, tags=['rotary'], formatted_root=True)
         self.assertConstrainedNodes([node1], {'storage': '0(ssd)'})
 
     def test_storage_single_contraint_matches_decimal_size(self):
@@ -871,56 +888,153 @@
         factory.make_PhysicalBlockDevice(
             node=node1, size=2 * (1000 ** 3))
         factory.make_PhysicalBlockDevice(
-            node=node1, size=4 * (1000 ** 3))
+            node=node1, size=4 * (1000 ** 3), formatted_root=True)
         node2 = factory.make_Node(with_boot_disk=False)
         # 1gb block device
         factory.make_PhysicalBlockDevice(
-            node=node2, size=1 * (1000 ** 3))
+            node=node2, size=1 * (1000 ** 3), formatted_root=True)
         self.assertConstrainedNodes([node1], {'storage': '1.5'})
 
-    def test_storage_multi_contraint_only_matches_physical_devices(self):
-        node1 = factory.make_Node(with_boot_disk=False)
-        factory.make_PhysicalBlockDevice(node=node1)
-        factory.make_PhysicalBlockDevice(node=node1)
-        node2 = factory.make_Node(with_boot_disk=False)
-        factory.make_BlockDevice(node=node2)
-        factory.make_BlockDevice(node=node2)
-        self.assertConstrainedNodes([node1], {'storage': '0,0'})
-
-    def test_storage_multi_contraint_matches_all_sizes_larger(self):
-        node1 = factory.make_Node(with_boot_disk=False)
-        # 1gb, 2gb, 3gb block device
-        factory.make_PhysicalBlockDevice(
-            node=node1, size=1 * (1000 ** 3))
-        factory.make_PhysicalBlockDevice(
-            node=node1, size=2 * (1000 ** 3))
-        factory.make_PhysicalBlockDevice(
-            node=node1, size=3 * (1000 ** 3))
-        node2 = factory.make_Node(with_boot_disk=False)
-        # 5gb, 6gb, 7gb block device
-        factory.make_PhysicalBlockDevice(
-            node=node2, size=5 * (1000 ** 3))
-        factory.make_PhysicalBlockDevice(
-            node=node2, size=6 * (1000 ** 3))
-        factory.make_PhysicalBlockDevice(
-            node=node2, size=7 * (1000 ** 3))
-        node3 = factory.make_Node(with_boot_disk=False)
-        # 8gb, 9gb, 10gb block device
-        factory.make_PhysicalBlockDevice(
-            node=node3, size=8 * (1000 ** 3))
-        factory.make_PhysicalBlockDevice(
-            node=node3, size=9 * (1000 ** 3))
-        factory.make_PhysicalBlockDevice(
-            node=node3, size=10 * (1000 ** 3))
-        # all nodes with physical devices larger than 2gb
-        self.assertConstrainedNodes([node2, node3], {'storage': '4,4,4'})
+    def test_storage_single_contraint_allows_root_on_virtual(self):
+        node1 = factory.make_Node(with_boot_disk=False)
+        physical = factory.make_PhysicalBlockDevice(node=node1)
+        partition_table = factory.make_PartitionTable(block_device=physical)
+        partition = factory.make_Partition(partition_table=partition_table)
+        pv = factory.make_Filesystem(
+            fstype=FILESYSTEM_TYPE.LVM_PV, partition=partition)
+        vg = factory.make_FilesystemGroup(
+            filesystems=[pv], group_type=FILESYSTEM_GROUP_TYPE.LVM_VG)
+        virtual = factory.make_VirtualBlockDevice(
+            filesystem_group=vg, node=node1)
+        factory.make_Filesystem(mount_point='/', block_device=virtual)
+        self.assertConstrainedNodes([node1], {'storage': '0'})
+
+    def test_storage_single_contraint_size_on_virtual(self):
+        node1 = factory.make_Node(with_boot_disk=False)
+        physical = factory.make_PhysicalBlockDevice(
+            node=node1, size=(6 * (1000 ** 3)))
+        partition_table = factory.make_PartitionTable(block_device=physical)
+        partition = factory.make_Partition(
+            partition_table=partition_table, size=(5.5 * (1000 ** 3)))
+        pv = factory.make_Filesystem(
+            fstype=FILESYSTEM_TYPE.LVM_PV, partition=partition)
+        vg = factory.make_FilesystemGroup(
+            filesystems=[pv], group_type=FILESYSTEM_GROUP_TYPE.LVM_VG)
+        virtual = factory.make_VirtualBlockDevice(
+            filesystem_group=vg, node=node1, size=(5 * (1000 ** 3)))
+        factory.make_Filesystem(mount_point='/', block_device=virtual)
+        self.assertConstrainedNodes([node1], {'storage': '4'})
+
+    def test_storage_multi_contraint_matches_physical_and_unused(self):
+        node1 = factory.make_Node(with_boot_disk=False)
+        factory.make_PhysicalBlockDevice(
+            node=node1, formatted_root=True)
+        # 1gb, 2gb, 3gb block device
+        factory.make_PhysicalBlockDevice(
+            node=node1, size=1 * (1000 ** 3))
+        factory.make_PhysicalBlockDevice(
+            node=node1, size=2 * (1000 ** 3))
+        factory.make_PhysicalBlockDevice(
+            node=node1, size=3 * (1000 ** 3))
+        node2 = factory.make_Node(with_boot_disk=False)
+        factory.make_PhysicalBlockDevice(
+            node=node2, formatted_root=True)
+        # 5gb, 6gb, 7gb block device
+        factory.make_PhysicalBlockDevice(
+            node=node2, size=5 * (1000 ** 3))
+        factory.make_PhysicalBlockDevice(
+            node=node2, size=6 * (1000 ** 3))
+        factory.make_PhysicalBlockDevice(
+            node=node2, size=7 * (1000 ** 3))
+        node3 = factory.make_Node(with_boot_disk=False)
+        factory.make_PhysicalBlockDevice(
+            node=node3, formatted_root=True)
+        # 8gb, 9gb, 10gb block device
+        factory.make_PhysicalBlockDevice(
+            node=node3, size=8 * (1000 ** 3))
+        factory.make_PhysicalBlockDevice(
+            node=node3, size=9 * (1000 ** 3))
+        factory.make_PhysicalBlockDevice(
+            node=node3, size=10 * (1000 ** 3))
+        # all nodes with physical devices larger than 2gb
+        self.assertConstrainedNodes([node2, node3], {'storage': '0,4,4,4'})
+
+    def test_storage_multi_contraint_matches_virtual_and_unused(self):
+        node1 = factory.make_Node(with_boot_disk=False)
+        factory.make_PhysicalBlockDevice(
+            node=node1, formatted_root=True)
+        # 1gb, 2gb, 3gb block device
+        factory.make_VirtualBlockDevice(
+            node=node1, size=1 * (1000 ** 3))
+        factory.make_VirtualBlockDevice(
+            node=node1, size=2 * (1000 ** 3))
+        factory.make_VirtualBlockDevice(
+            node=node1, size=3 * (1000 ** 3))
+        node2 = factory.make_Node(with_boot_disk=False)
+        factory.make_PhysicalBlockDevice(
+            node=node2, formatted_root=True)
+        # 5gb, 6gb, 7gb block device
+        factory.make_VirtualBlockDevice(
+            node=node2, size=5 * (1000 ** 3))
+        factory.make_VirtualBlockDevice(
+            node=node2, size=6 * (1000 ** 3))
+        factory.make_VirtualBlockDevice(
+            node=node2, size=7 * (1000 ** 3))
+        node3 = factory.make_Node(with_boot_disk=False)
+        factory.make_PhysicalBlockDevice(
+            node=node3, formatted_root=True)
+        # 8gb, 9gb, 10gb block device
+        factory.make_VirtualBlockDevice(
+            node=node3, size=8 * (1000 ** 3))
+        factory.make_VirtualBlockDevice(
+            node=node3, size=9 * (1000 ** 3))
+        factory.make_VirtualBlockDevice(
+            node=node3, size=10 * (1000 ** 3))
+        # all nodes with physical devices larger than 2gb
+        self.assertConstrainedNodes([node2, node3], {'storage': '0,4,4,4'})
+
+    def test_storage_multi_contraint_matches_iscsi_and_unused(self):
+        node1 = factory.make_Node(with_boot_disk=False)
+        factory.make_PhysicalBlockDevice(
+            node=node1, formatted_root=True)
+        # 1gb, 2gb, 3gb block device
+        factory.make_ISCSIBlockDevice(
+            node=node1, size=1 * (1000 ** 3))
+        factory.make_ISCSIBlockDevice(
+            node=node1, size=2 * (1000 ** 3))
+        factory.make_ISCSIBlockDevice(
+            node=node1, size=3 * (1000 ** 3))
+        node2 = factory.make_Node(with_boot_disk=False)
+        factory.make_PhysicalBlockDevice(
+            node=node2, formatted_root=True)
+        # 5gb, 6gb, 7gb block device
+        factory.make_ISCSIBlockDevice(
+            node=node2, size=5 * (1000 ** 3))
+        factory.make_ISCSIBlockDevice(
+            node=node2, size=6 * (1000 ** 3))
+        factory.make_ISCSIBlockDevice(
+            node=node2, size=7 * (1000 ** 3))
+        node3 = factory.make_Node(with_boot_disk=False)
+        factory.make_PhysicalBlockDevice(
+            node=node3, formatted_root=True)
+        # 8gb, 9gb, 10gb block device
+        factory.make_ISCSIBlockDevice(
+            node=node3, size=8 * (1000 ** 3))
+        factory.make_ISCSIBlockDevice(
+            node=node3, size=9 * (1000 ** 3))
+        factory.make_ISCSIBlockDevice(
+            node=node3, size=10 * (1000 ** 3))
+        # all nodes with physical devices larger than 2gb
+        self.assertConstrainedNodes([node2, node3], {'storage': '0,4,4,4'})
 
     def test_storage_multi_contraint_matches_on_tags(self):
         node1 = factory.make_Node(with_boot_disk=False)
-        factory.make_PhysicalBlockDevice(node=node1, tags=['ssd'])
+        factory.make_PhysicalBlockDevice(
+            node=node1, tags=['ssd'], formatted_root=True)
         factory.make_PhysicalBlockDevice(node=node1, tags=['ssd', 'removable'])
         node2 = factory.make_Node(with_boot_disk=False)
-        factory.make_PhysicalBlockDevice(node=node2, tags=['ssd'])
+        factory.make_PhysicalBlockDevice(
+            node=node2, tags=['ssd'], formatted_root=True)
         factory.make_PhysicalBlockDevice(node=node2, tags=['ssd', 'sata'])
         self.assertConstrainedNodes(
             [node1], {'storage': '0(ssd),0(ssd,removable)'})
@@ -930,7 +1044,7 @@
         # 1gb, 2gb block device
         factory.make_PhysicalBlockDevice(
             node=node1, size=1 * (1000 ** 3),
-            tags=['ssd'])
+            tags=['ssd'], formatted_root=True)
         factory.make_PhysicalBlockDevice(
             node=node1, size=2 * (1000 ** 3),
             tags=['ssd'])
@@ -938,65 +1052,81 @@
         # 4gb, 5gb block device
         factory.make_PhysicalBlockDevice(
             node=node2, size=4 * (1000 ** 3),
-            tags=['ssd'])
+            tags=['ssd'], formatted_root=True)
         factory.make_PhysicalBlockDevice(
             node=node2, size=5 * (1000 ** 3),
             tags=['ssd'])
         self.assertConstrainedNodes(
             [node2], {'storage': '3(ssd),3(ssd)'})
 
-    def test_storage_first_constraint_matches_first_blockdevice(self):
+    def test_storage_first_constraint_matches_blockdevice_with_root(self):
         """
         Make sure a constraint like 10(ssd),5,20 will match a node with a
         11(ssd) first device, a 21 second device and a 10 third device,
         but not a 5/20/10(ssd) node
         """
         node1 = factory.make_Node(with_boot_disk=False)
-        factory.make_PhysicalBlockDevice(node=node1, size=6 * (1000 ** 3))
         factory.make_PhysicalBlockDevice(node=node1, size=21 * (1000 ** 3))
-        factory.make_PhysicalBlockDevice(node=node1, size=11 * (1000 ** 3),
-                                         tags=['ssd'])
+        factory.make_PhysicalBlockDevice(
+            node=node1, size=11 * (1000 ** 3), tags=['ssd'])
+        factory.make_PhysicalBlockDevice(
+            node=node1, size=6 * (1000 ** 3), formatted_root=True)
         node2 = factory.make_Node(with_boot_disk=False)
-        factory.make_PhysicalBlockDevice(node=node2, size=11 * (1000 ** 3),
-                                         tags=['ssd'])
         factory.make_PhysicalBlockDevice(node=node2, size=6 * (1000 ** 3))
         factory.make_PhysicalBlockDevice(node=node2, size=21 * (1000 ** 3))
+        factory.make_PhysicalBlockDevice(
+            node=node2, size=11 * (1000 ** 3),
+            tags=['ssd'], formatted_root=True)
         self.assertConstrainedNodes(
             [node2], {'storage': '10(ssd),5,20'})
 
     def test_storage_multi_contraint_matches_large_disk_count(self):
         node1 = factory.make_Node(with_boot_disk=False)
+        factory.make_PhysicalBlockDevice(node=node1, formatted_root=True)
         for _ in range(10):
             factory.make_PhysicalBlockDevice(node=node1)
         node2 = factory.make_Node(with_boot_disk=False)
+        factory.make_PhysicalBlockDevice(node=node2, formatted_root=True)
         for _ in range(5):
             factory.make_PhysicalBlockDevice(node=node2)
         self.assertConstrainedNodes(
             [node1], {'storage': '0,0,0,0,0,0,0,0,0,0'})
 
-    @skip(
-        "XXX: allenap 2015-03-17 bug=1433012: This test keeps failing when "
-        "landing unrelated branches, so has been disabled.")
     def test_storage_with_named_constraints(self):
         node1 = factory.make_Node(with_boot_disk=False)
-        factory.make_PhysicalBlockDevice(node=node1, size=11 * (1000 ** 3),
-                                         tags=['ssd'])
-        factory.make_PhysicalBlockDevice(node=node1, size=6 * (1000 ** 3),
-                                         tags=['rotary', '5400rpm'])
-        factory.make_PhysicalBlockDevice(node=node1, size=21 * (1000 ** 3))
-        form = AcquireNodeForm({'storage':
-                                'root:10(ssd),data:5(rotary,5400rpm),20'})
+        physical = factory.make_PhysicalBlockDevice(
+            node=node1, size=11 * (1000 ** 3))
+        partition_table = factory.make_PartitionTable(block_device=physical)
+        partition = factory.make_Partition(
+            partition_table=partition_table, size=10 * (1000 ** 3))
+        pv = factory.make_Filesystem(
+            fstype=FILESYSTEM_TYPE.LVM_PV, partition=partition)
+        vg = factory.make_FilesystemGroup(
+            filesystems=[pv], group_type=FILESYSTEM_GROUP_TYPE.LVM_VG)
+        virtual = factory.make_VirtualBlockDevice(
+            filesystem_group=vg, node=node1, size=9 * (1000 ** 3),
+            tags=['lvm'])
+        factory.make_Filesystem(mount_point='/', block_device=virtual)
+        physical = factory.make_PhysicalBlockDevice(
+            node=node1, size=6 * (1000 ** 3), tags=['rotary', '5400rpm'])
+        iscsi = factory.make_ISCSIBlockDevice(
+            node=node1, size=21 * (1000 ** 3))
+        form = AcquireNodeForm({
+            'storage': 'root:8(lvm),physical:5(rotary,5400rpm),iscsi:20'})
         self.assertTrue(form.is_valid(), form.errors)
-        filtered_nodes, constraint_map = form.filter_nodes(
+        filtered_nodes, constraint_map, _ = form.filter_nodes(
             Machine.objects.all())
         node = filtered_nodes[0]
-        constraints = list(constraint_map[node.id])
-        disk0 = node.physicalblockdevice_set.get(
-            id=constraints[0])  # 1st constraint with name
-        self.assertGreaterEqual(disk0.size, 10 * 1000 ** 3)
-        disk1 = node.physicalblockdevice_set.get(
-            id=constraints[1])  # 2nd constraint with name
-        self.assertGreaterEqual(disk1.size, 5 * 1000 ** 3)
+        constraints = {
+            value: key
+            for key, value in constraint_map[node.id].items()
+        }
+        disk0 = node.blockdevice_set.get(id=constraints['root'])
+        self.assertEquals(virtual.id, disk0.id)
+        disk1 = node.blockdevice_set.get(id=constraints['physical'])
+        self.assertEquals(physical.id, disk1.id)
+        disk2 = node.blockdevice_set.get(id=constraints['iscsi'])
+        self.assertEquals(iscsi.id, disk2.id)
 
     def test_fabrics_constraint(self):
         fabric1 = factory.make_Fabric(name="fabric1")