Merge lp:~ltrager/maas/validate_storage into lp:~maas-committers/maas/trunk

[Lee Trager]

Validate a node has a boot device and / is mounted before deploying

This patch validates that the node has a boot device and has / mounted somewhere before deploying. This also adds an error message to the UI when this is not the case. I modified the make_Node method to also create a partition and mount it at '/' since we now require it to be a valid bootable node.

I also made two small fixes for the previous validate node on network patch
* I was raising a validation error with just the error message. If we don't return a dictionary the error doesn't display in the UI.
* Rename the on_network tests to not include 'managed'

[Blake Rouse (blake-rouse)]

Haven't reviewed the code but please add one more check.

Make sure that a /boot partition exists not on a bcache device if the / partition is on a bcache devices.

[Lee Trager (ltrager)]

We now check if / is on a bcache block device, if we are we check if /boot is mounted not on a bcache device. I also made the error message more specific to why the storage layout isn't valid.

[Gavin Panella (allenap)]

Lots of comments. I think there are a few things that need fixing before it can be landed. I'm worried about how the new methods will perform. I won't block landing based on that but it would be good if you could do something about it.

review: Needs Fixing

[Lee Trager (ltrager)]

Thanks for the review. I've gone over and responded to your comments below. The performance issues you are seeing are all due to validating that / isn't on bcache. In order to check if a partition/block device is on bcache I need to check the parent devices. There isn't currently a relation between child and parent so I have to iterate over all the blockdevices the node has until I find the partition/block device in question.

There are only two ways I can think of that will improve performance
1. Modify the schema so we have a relation between child(virtual block devices, and partitions), and parent(block devices, partitions)
2. Don't check if / is on bcache.

[Lee Trager (ltrager)]

I stand corrected. A partition/block device can have two file systems. One is the filesystem the administrator sets another is the one a user sets. I now use the get_effective_filesystem() method which returns the correct filesystem depending on what the node status is.

The only place I use the first() method is getting the filesystem_group for a cache_set.

[Gavin Panella (allenap)]

Replies to diff comments.

[Gavin Panella (allenap)]

This seems sane to me. Just change storage_layout_status's "Valid" return value to something that doesn't make me think you're a troll from the PHP camp and it's good to go.

Actually, hold up, there's no test for the change to node-details.html. Better get your Angular boots on. If you really want to land this now, undo the change to node-details.html and propose it separately with a test.

review: Approve

[Lee Trager (ltrager)]

I took your suggestion and renamed storage_layout_status to storage_layout_issues which returns a list of possible errors. With that change I also added a function to node_details_storage.js which detects if there is something in $scope.node.storage_layout_issues so we can test the change to node-details.html.

[MAAS Lander (maas-lander)]

The attempt to merge lp:~ltrager/maas/validate_storage into lp:maas failed. Below is the output from the failed tests.

Hit http://security.ubuntu.com xenial-security InRelease
Get:1 http://nova.clouds.archive.ubuntu.com xenial InRelease [218 kB]
Hit http://nova.clouds.archive.ubuntu.com xenial-updates InRelease
Hit http://security.ubuntu.com xenial-security/main Sources
Hit http://security.ubuntu.com xenial-security/universe Sources
Hit http://security.ubuntu.com xenial-security/main amd64 Packages
Hit http://security.ubuntu.com xenial-security/universe amd64 Packages
Hit http://security.ubuntu.com xenial-security/main Translation-en
Hit http://security.ubuntu.com xenial-security/universe Translation-en
Get:2 http://nova.clouds.archive.ubuntu.com xenial/main Sources [1,117 kB]
Get:3 http://nova.clouds.archive.ubuntu.com xenial/universe Sources [7,461 kB]
Get:4 http://nova.clouds.archive.ubuntu.com xenial/main amd64 Packages [1,441 kB]
Get:5 http://nova.clouds.archive.ubuntu.com xenial/universe amd64 Packages [6,950 kB]
Get:6 http://nova.clouds.archive.ubuntu.com xenial/main Translation-en [841 kB]
Get:7 http://nova.clouds.archive.ubuntu.com xenial/universe Translation-en [4,705 kB]
Hit http://nova.clouds.archive.ubuntu.com xenial-updates/main Sources
Hit http://nova.clouds.archive.ubuntu.com xenial-updates/universe Sources
Hit http://nova.clouds.archive.ubuntu.com xenial-updates/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com xenial-updates/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com xenial-updates/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com xenial-updates/universe Translation-en
Fetched 22.7 MB in 13s (1,725 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 pxelinux pyflakes python-apt python-bson python-convoy python-coverage python-crochet python-cssselect python-curtin python-dev python-distro-info python-django python-django16 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-psycopg2 python-pyinotify python-pyparsing python-seamicroclient python-simplejson python-simplestreams python-sphinx python-subunit python-tempita python-testresources python-testscenarios python-testtools python-twisted python-txtftp python-tz python-unittest2 python-virtualenv python-yaml python-zope.interface socat syslinux-common tgt ubuntu-cloudimage-keyring virtualenv wget xvfb
Reading package lists...
Building dependency tree...
Reading state information...
apache2 is...

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2015-11-13 20:01:04 +0000
+++ src/maasserver/models/node.py	2015-11-17 23:04:09 +0000
@@ -63,6 +63,7 @@
 from maasserver.enum import (
     ALLOCATED_NODE_STATUSES,
     FILESYSTEM_FORMAT_TYPE_CHOICES_DICT,
+    FILESYSTEM_TYPE,
     INTERFACE_LINK_TYPE,
     INTERFACE_TYPE,
     IPADDRESS_FAMILY,
@@ -760,6 +761,73 @@
                 event_action=action,
                 event_description=description)
 
+    def storage_layout_issues(self):
+        """Return any errors with the storage layout.
+
+        Checks that the node has / mounted. If / is mounted on bcache check
+        that /boot is mounted and is not on bcache."""
+        def on_bcache(obj):
+            if obj.type == "physical":
+                return False
+            elif obj.type == "partition":
+                return on_bcache(obj.partition_table.block_device)
+            for parent in obj.virtualblockdevice.get_parents():
+                if((parent.type != "physical" and on_bcache(parent)) or
+                    (parent.get_effective_filesystem().fstype in
+                     [FILESYSTEM_TYPE.BCACHE_CACHE,
+                      FILESYSTEM_TYPE.BCACHE_BACKING])):
+                    return True
+            return False
+
+        has_boot = False
+        root_mounted = False
+        root_on_bcache = False
+        boot_mounted = False
+
+        for block_device in self.blockdevice_set.all():
+            if block_device.is_boot_disk():
+                has_boot = True
+            pt = block_device.get_partitiontable()
+            if pt is not None:
+                for partition in pt.partitions.all():
+                    fs = partition.get_effective_filesystem()
+                    if fs is None:
+                        continue
+                    if fs.mount_point == '/':
+                        root_mounted = True
+                        if on_bcache(block_device):
+                            root_on_bcache = True
+                        else:
+                            # If / is mounted and its not on bcache the system
+                            # is bootable
+                            return []
+                    elif (fs.mount_point == '/boot' and
+                          not on_bcache(block_device)):
+                        boot_mounted = True
+            else:
+                fs = block_device.get_effective_filesystem()
+                if fs is None:
+                    continue
+                if fs.mount_point == '/':
+                    root_mounted = True
+                    if on_bcache(block_device):
+                        root_on_bcache = True
+                    else:
+                        # If / is mounted and its not on bcache the system
+                        # is bootable.
+                        return []
+                elif fs.mount_point == '/boot' and not on_bcache(block_device):
+                    boot_mounted = True
+        issues = []
+        if not has_boot:
+            issues.append("Node must have boot disk.")
+        if not root_mounted:
+            issues.append("Node must have / mounted.")
+        if root_mounted and root_on_bcache and not boot_mounted:
+            issues.append("Because / is on a bcache volume you must create "
+                          "/boot on a non-bcache volume")
+        return issues
+
     def on_network(self):
         """Return true if the node is connected to a managed network."""
         for interface in self.interface_set.all():
@@ -772,7 +840,12 @@
     def _start_deployment(self):
         """Mark a node as being deployed."""
         if self.on_network() is False:
-            raise ValidationError("Node must be configured to use a network")
+            raise ValidationError(
+                {"network":
+                 ["Node must be configured to use a network"]})
+        storage_layout_issues = self.storage_layout_issues()
+        if len(storage_layout_issues) > 0:
+            raise ValidationError({"storage": storage_layout_issues})
         self.status = NODE_STATUS.DEPLOYING
         self.save()
 
 
=== modified file 'src/maasserver/models/tests/test_filesystem.py'
--- src/maasserver/models/tests/test_filesystem.py	2015-10-27 20:53:16 +0000
+++ src/maasserver/models/tests/test_filesystem.py	2015-11-17 23:04:09 +0000
@@ -29,6 +29,9 @@
 
     def test_filter_by_node(self):
         node = factory.make_Node()
+        boot_bd = node.blockdevice_set.first()
+        root_partition = boot_bd.partitiontable_set.first().partitions.first()
+        root_fs = root_partition.filesystem_set.first()
         block_device = factory.make_PhysicalBlockDevice(node=node)
         bd_for_partitions = factory.make_PhysicalBlockDevice(node=node)
         partition_table = factory.make_PartitionTable(
@@ -37,7 +40,7 @@
         filesystem_on_bd = factory.make_Filesystem(block_device=block_device)
         filesystem_on_partition = factory.make_Filesystem(partition=partition)
         self.assertItemsEqual(
-            [filesystem_on_bd, filesystem_on_partition],
+            [root_fs, filesystem_on_bd, filesystem_on_partition],
             Filesystem.objects.filter_by_node(node))
 
 
 
=== modified file 'src/maasserver/models/tests/test_node.py'
--- src/maasserver/models/tests/test_node.py	2015-11-10 18:12:57 +0000
+++ src/maasserver/models/tests/test_node.py	2015-11-17 23:04:09 +0000
@@ -3774,15 +3774,46 @@
 
         self.assertThat(release_auto_ips, MockCalledOnceWith())
 
-    def test_connected_to_managed_network_true_when_connected(self):
+    def test_on_network_returns_true_when_connected(self):
         node = factory.make_Node_with_Interface_on_Subnet(
             status=NODE_STATUS.ALLOCATED)
         self.assertTrue(node.on_network())
 
-    def test_connected_to_managed_network_false_when_not_connected(self):
+    def test_on_network_returns_false_when_not_connected(self):
         node = factory.make_Node(status=NODE_STATUS.ALLOCATED)
         self.assertFalse(node.on_network())
 
+    def test_storage_layout_issues_is_valid_when_flat(self):
+        node = factory.make_Node()
+        self.assertEquals([], node.storage_layout_issues())
+
+    def test_storage_layout_issues_returns_valid_with_boot_and_bcache(self):
+        node = factory.make_Node(with_boot_disk=False)
+        boot_partition = factory.make_Partition(node=node)
+        factory.make_Filesystem(partition=boot_partition, mount_point='/boot')
+        fs_group = factory.make_FilesystemGroup(
+            node=node, group_type=FILESYSTEM_GROUP_TYPE.BCACHE)
+        bcache = fs_group.virtual_device
+        factory.make_Filesystem(block_device=bcache, mount_point="/")
+        self.assertEquals([], node.storage_layout_issues())
+
+    def test_storage_layout_issues_returns_invalid_when_no_disk(self):
+        node = factory.make_Node(with_boot_disk=False)
+        self.assertEquals(["Node must have boot disk.",
+                           "Node must have / mounted."],
+                          node.storage_layout_issues())
+
+    def test_storage_layout_issues_returns_invalid_when_root_on_bcache(self):
+        node = factory.make_Node(with_boot_disk=False)
+        factory.make_Partition(node=node)
+        fs_group = factory.make_FilesystemGroup(
+            node=node, group_type=FILESYSTEM_GROUP_TYPE.BCACHE)
+        bcache = fs_group.virtual_device
+        factory.make_Filesystem(block_device=bcache, mount_point="/")
+        self.assertEquals(["Because / is on a bcache volume you must create "
+                          "/boot on a non-bcache volume"],
+                          node.storage_layout_issues())
+
 
 class TestNode_Stop(MAASServerTestCase):
     """Tests for Node.stop()."""
 
=== modified file 'src/maasserver/models/tests/test_virtualblockdevice.py'
--- src/maasserver/models/tests/test_virtualblockdevice.py	2015-08-14 13:48:59 +0000
+++ src/maasserver/models/tests/test_virtualblockdevice.py	2015-11-17 23:04:09 +0000
@@ -159,3 +159,17 @@
         uuid = uuid4()
         block_device = factory.make_VirtualBlockDevice(uuid=uuid)
         self.assertEquals('%s' % uuid, block_device.uuid)
+
+    def test_get_parents_finds_devices(self):
+        node = factory.make_Node()
+        factory.make_FilesystemGroup(
+            node=node,
+            group_type=factory.pick_enum(
+                FILESYSTEM_GROUP_TYPE, but_not=FILESYSTEM_GROUP_TYPE.LVM_VG))
+        fs_group_disks = [block_device.blockdevice_ptr
+                          for block_device in
+                          node.physicalblockdevice_set.all()
+                          if not block_device.is_boot_disk()]
+        virtualblockdevice = node.virtualblockdevice_set.first()
+        self.assertEqual(
+            len(fs_group_disks), len(virtualblockdevice.get_parents()))
 
=== modified file 'src/maasserver/models/virtualblockdevice.py'
--- src/maasserver/models/virtualblockdevice.py	2015-11-16 06:26:17 +0000
+++ src/maasserver/models/virtualblockdevice.py	2015-11-17 23:04:09 +0000
@@ -137,3 +137,37 @@
         if not self.uuid:
             self.uuid = uuid4()
         return super(VirtualBlockDevice, self).save(*args, **kwargs)
+
+    def get_parents(self):
+        """Return the blockdevices and partition which make up this device."""
+        def check_fs_group(obj):
+            fs = obj.get_effective_filesystem()
+            if fs is None:
+                return False
+            if fs.filesystem_group is not None:
+                fs_group = fs.filesystem_group
+            elif fs.cache_set is not None:
+                # A block device/partition can only have one cache_set
+                fs_group = fs.cache_set.filesystemgroup_set.first()
+            else:
+                return False
+            for virtual_device in fs_group.virtual_devices.all():
+                if virtual_device.id == self.id:
+                    return True
+            return False
+
+        parents = []
+        # We need to check all of the nodes block devices incase
+        # we have nested virtual block devices.
+        for block_device in self.node.blockdevice_set.all():
+            if block_device.id == self.id:
+                continue
+            if check_fs_group(block_device):
+                parents.append(block_device)
+            pt = block_device.get_partitiontable()
+            if pt is None:
+                continue
+            for partition in pt.partitions.all():
+                if check_fs_group(partition):
+                    parents.append(partition)
+        return parents
 
=== modified file 'src/maasserver/static/js/angular/controllers/node_details_storage.js'
--- src/maasserver/static/js/angular/controllers/node_details_storage.js	2015-11-10 17:36:38 +0000
+++ src/maasserver/static/js/angular/controllers/node_details_storage.js	2015-11-17 23:04:09 +0000
@@ -1913,4 +1913,12 @@
                 return false;
             }
         };
+
+        // Returns true if there are storage layout errors
+        $scope.hasStorageLayoutIssues = function() {
+            if(angular.isObject($scope.node)) {
+                return $scope.node.storage_layout_issues.length > 0;
+            }
+            return false;
+        };
     }]);
 
=== modified file 'src/maasserver/static/js/angular/controllers/tests/test_node_details_storage.js'
--- src/maasserver/static/js/angular/controllers/tests/test_node_details_storage.js	2015-11-10 17:36:38 +0000
+++ src/maasserver/static/js/angular/controllers/tests/test_node_details_storage.js	2015-11-17 23:04:09 +0000
@@ -4744,4 +4744,18 @@
             expect($scope.isAllStorageDisabled()).toBe(true);
         });
     });
+
+    describe("hasStorageLayoutIssues", function() {
+        it("true when node.storage_layout_issues has issues", function() {
+            var controller = makeController();
+            $scope.node.storage_layout_issues = [makeName("issue")];
+            expect($scope.hasStorageLayoutIssues()).toBe(true);
+        });
+
+        it("false when node.storage_layout_issues has no issues", function() {
+            var controller = makeController();
+            $scope.node.storage_layout_issues = [];
+            expect($scope.hasStorageLayoutIssues()).toBe(false);
+        });
+    });
 });
 
=== modified file 'src/maasserver/static/partials/node-details.html'
--- src/maasserver/static/partials/node-details.html	2015-11-14 00:32:02 +0000
+++ src/maasserver/static/partials/node-details.html	2015-11-17 23:04:09 +0000
@@ -772,6 +772,15 @@
                         </li>
                     </ul>
                 </div>
+                <div class="twelve-col error ng-hide" data-ng-show="hasStorageLayoutIssues()">
+                    <div data-ng-repeat="issue in node.storage_layout_issues">
+                        <ul class="flash-messages">
+                            <li class="flash-messages__item error">
+                                {$ issue $}
+                            </li>
+                        </ul>
+                    </div>
+                </div>
                 <div class="twelve-col padding-bottom no-margin">
                     <h3>File systems</h3>
                     <section class="table">
 
=== modified file 'src/maasserver/testing/factory.py'
--- src/maasserver/testing/factory.py	2015-11-12 19:57:04 +0000
+++ src/maasserver/testing/factory.py	2015-11-17 23:04:09 +0000
@@ -32,6 +32,7 @@
 from django.utils import timezone
 from maasserver.clusterrpc.power_parameters import get_power_types
 from maasserver.enum import (
+    ALLOCATED_NODE_STATUSES,
     BOOT_RESOURCE_FILE_TYPE,
     BOOT_RESOURCE_TYPE,
     CACHE_MODE_TYPE,
@@ -330,7 +331,10 @@
             self.make_Interface(
                 INTERFACE_TYPE.PHYSICAL, node=node, vlan=vlan, fabric=fabric)
         if installable and with_boot_disk:
-            self.make_PhysicalBlockDevice(node=node)
+            root_partition = self.make_Partition(node=node)
+            acquired = node.status in ALLOCATED_NODE_STATUSES
+            self.make_Filesystem(
+                partition=root_partition, mount_point='/', acquired=acquired)
 
         # Update the 'updated'/'created' fields with a call to 'update'
         # preventing a call to save() from overriding the values.
 
=== modified file 'src/maasserver/websockets/handlers/node.py'
--- src/maasserver/websockets/handlers/node.py	2015-11-10 23:41:49 +0000
+++ src/maasserver/websockets/handlers/node.py	2015-11-17 23:04:09 +0000
@@ -313,6 +313,7 @@
                 {'key': key, 'ui': ui}
                 for key, ui in FILESYSTEM_FORMAT_TYPE_CHOICES
             ]
+            data["storage_layout_issues"] = obj.storage_layout_issues()
 
             # Events
             data["events"] = self.dehydrate_events(obj)
 
=== modified file 'src/maasserver/websockets/handlers/tests/test_node.py'
--- src/maasserver/websockets/handlers/tests/test_node.py	2015-11-12 23:20:54 +0000
+++ src/maasserver/websockets/handlers/tests/test_node.py	2015-11-17 23:04:09 +0000
@@ -165,6 +165,7 @@
             "disable_ipv4": node.disable_ipv4,
             "physical_disk_count": node.physicalblockdevice_set.count(),
             "disks": disks,
+            "storage_layout_issues": node.storage_layout_issues(),
             "supported_filesystems": [
                 {'key': key, 'ui': ui}
                 for key, ui in FILESYSTEM_FORMAT_TYPE_CHOICES],
@@ -1471,20 +1472,23 @@
             interface=True,
             architecture=architecture,
             status=NODE_STATUS.ALLOCATED)
-        partition_table = factory.make_PartitionTable(node=node)
+        block_device = factory.make_BlockDevice(node=node)
+        partition_table = factory.make_PartitionTable(
+            block_device=block_device, node=node)
         size = partition_table.block_device.size / 2
         handler.create_partition({
             'system_id': node.system_id,
             'block_id': partition_table.block_device_id,
             'partition_size': size
             })
+        partition = partition_table.partitions.first()
         self.assertEquals(
-            1, Partition.objects.count())
+            2, Partition.objects.count())
         self.assertEquals(
             human_readable_bytes(
                 round_size_to_nearest_block(
                     size, PARTITION_ALIGNMENT_SIZE, False)),
-            human_readable_bytes(Partition.objects.first().size))
+            human_readable_bytes(partition.size))
 
     def test_create_partition_with_filesystem(self):
         user = factory.make_admin()
@@ -1494,7 +1498,10 @@
             interface=True,
             architecture=architecture,
             status=NODE_STATUS.ALLOCATED)
-        partition_table = factory.make_PartitionTable(node=node)
+        block_device = factory.make_BlockDevice(node=node)
+        partition_table = factory.make_PartitionTable(
+            block_device=block_device, node=node)
+        partition = partition_table.partitions.first()
         size = partition_table.block_device.size / 2
         fstype = factory.pick_choice(FILESYSTEM_FORMAT_TYPE_CHOICES)
         mount_point = factory.make_absolute_path()
@@ -1505,19 +1512,20 @@
             'fstype': fstype,
             'mount_point': mount_point,
             })
+        partition = partition_table.partitions.first()
         self.assertEquals(
-            1, Partition.objects.count())
+            2, Partition.objects.count())
         self.assertEquals(
             human_readable_bytes(
                 round_size_to_nearest_block(
                     size, PARTITION_ALIGNMENT_SIZE, False)),
-            human_readable_bytes(Partition.objects.first().size))
+            human_readable_bytes(partition.size))
         self.assertEquals(
             fstype,
-            Partition.objects.first().get_effective_filesystem().fstype)
+            partition.get_effective_filesystem().fstype)
         self.assertEquals(
             mount_point,
-            Partition.objects.first().get_effective_filesystem().mount_point)
+            partition.get_effective_filesystem().mount_point)
 
     def test_create_cache_set_for_partition(self):
         user = factory.make_admin()