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

[Blake Rouse]

Prevent bcache creation on devices with partitions. Inform the user in the tooltip why bcache creation is disabled.

[Andres Rodriguez (andreserl)]

The branch overall looks good to me, but I was hoping we can discuss this.

Wouldn't it be better to either:

1. Tell the user that they cannot create a Bcache because there's no partition?
2. Create the partition as part as the bcache creation (I think we did this for LVM, and if we do, we should do the same for Bcache.).

review: Needs Information

[Blake Rouse (blake-rouse)]

I updated the branch to tell the user why bcache creation is disabled.

[Andres Rodriguez (andreserl)]

lgtm!

review: Approve

=== modified file 'src/maasserver/static/js/angular/controllers/node_details_storage.js'
--- src/maasserver/static/js/angular/controllers/node_details_storage.js	2016-05-25 15:18:57 +0000
+++ src/maasserver/static/js/angular/controllers/node_details_storage.js	2016-07-11 13:56:16 +0000
@@ -1415,20 +1415,47 @@
             $scope.available.splice(idx, 1);
         };
 
+        // Return the reason a bcache device cannot be created.
+        $scope.getCannotCreateBcacheMsg = function() {
+            if($scope.cachesets.length === 0) {
+                return "Create at least one cache set to create bcache";
+            } else {
+                var selected = $scope.getSelectedAvailable();
+                if(selected.length === 1) {
+                    if($scope.hasUnmountedFilesystem(selected[0])) {
+                        return (
+                            "Device is formatted; unformat the " +
+                            "device to create bcache");
+                    } else if(selected[0].type === "lvm-vg") {
+                        return (
+                            "Cannot use a logical volume as a backing " +
+                            "device for bcache.");
+                    } else if(selected[0].has_partitions) {
+                        return (
+                            "Device has already been partitioned; create a " +
+                            "new partition to use as the bcache backing " +
+                            "device");
+                    } else {
+                        return null;
+                    }
+                } else {
+                    return "Select only one available device to create bcache";
+                }
+            }
+        };
+
         // Return true if a bcache can be created.
         $scope.canCreateBcache = function() {
-            if($scope.isAvailableDisabled() || ! $scope.isSuperUser()) {
+            if($scope.isAvailableDisabled() || !$scope.isSuperUser()) {
                 return false;
             }
 
-            var selected = $scope.getSelectedAvailable();
-            if(selected.length === 1) {
-                var allowed = (
-                    !$scope.hasUnmountedFilesystem(selected[0]) &&
-                    selected[0].type !== "lvm-vg");
-                return allowed && $scope.cachesets.length > 0;
+            var msg = $scope.getCannotCreateBcacheMsg();
+            if(msg === null) {
+                return true;
+            } else {
+                return false;
             }
-            return false;
         };
 
         // Enter bcache mode.

=== 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	2016-03-28 13:54:47 +0000
+++ src/maasserver/static/js/angular/controllers/tests/test_node_details_storage.js	2016-07-11 13:56:16 +0000
@@ -3202,6 +3202,92 @@
             });
     });
 
+    describe("getCannotCreateBcacheMsg", function() {
+
+        it("returns msg if no cachesets",
+            function() {
+                var controller = makeController();
+                $scope.available = [
+                    {
+                        fstype: null,
+                        $selected: true,
+                        has_partitions: false
+                    }
+                ];
+                $scope.cachesets = [];
+                expect($scope.getCannotCreateBcacheMsg()).toBe(
+                    "Create at least one cache set to create bcache");
+            });
+
+        it("returns msg if two selected", function() {
+            var controller = makeController();
+            $scope.cachesets = [{}];
+            $scope.available = [ { $selected: true }, { $selected: true }];
+            expect($scope.getCannotCreateBcacheMsg()).toBe(
+                "Select only one available device to create bcache");
+        });
+
+        it("returns msg if selected has fstype", function() {
+            var controller = makeController();
+            $scope.available = [
+                {
+                    fstype: "ext4",
+                    $selected: true,
+                    has_partitions: false
+                }
+            ];
+            $scope.cachesets = [{}];
+
+            expect($scope.getCannotCreateBcacheMsg()).toBe(
+                "Device is formatted; unformat the device to create bcache");
+        });
+
+        it("returns msg if selected is volume group", function() {
+            var controller = makeController();
+            $scope.available = [
+                {
+                    type: "lvm-vg",
+                    fstype: null,
+                    $selected: true,
+                    has_partitions: false
+                }
+            ];
+            $scope.cachesets = [{}];
+            expect($scope.getCannotCreateBcacheMsg()).toBe(
+                "Cannot use a logical volume as a backing device for bcache.");
+        });
+
+        it("returns msg if selected has partitions", function() {
+            var controller = makeController();
+            $scope.available = [
+                {
+                    fstype: null,
+                    $selected: true,
+                    has_partitions: true
+                }
+            ];
+            $scope.cachesets = [{}];
+            expect($scope.getCannotCreateBcacheMsg()).toBe(
+                "Device has already been partitioned; create a " +
+                "new partition to use as the bcache backing " +
+                "device");
+        });
+
+        it("returns null if selected is valid",
+            function() {
+                var controller = makeController();
+                $scope.available = [
+                    {
+                        fstype: null,
+                        $selected: true,
+                        has_partitions: false
+                    }
+                ];
+                $scope.cachesets = [{}];
+                expect($scope.getCannotCreateBcacheMsg()).toBeNull();
+            });
+    });
+
     describe("canCreateBcache", function() {
 
         it("returns false when isAvailableDisabled is true", function() {
@@ -3227,7 +3313,8 @@
             $scope.available = [
                 {
                     fstype: "ext4",
-                    $selected: true
+                    $selected: true,
+                    has_partitions: false
                 }
             ];
             $scope.cachesets = [{}];
@@ -3243,7 +3330,24 @@
                 {
                     type: "lvm-vg",
                     fstype: null,
-                    $selected: true
+                    $selected: true,
+                    has_partitions: false
+                }
+            ];
+            $scope.cachesets = [{}];
+            $scope.isSuperUser = function() { return true; };
+
+            expect($scope.canCreateBcache()).toBe(false);
+        });
+
+        it("returns false if selected has partitions", function() {
+            var controller = makeController();
+            spyOn($scope, "isAvailableDisabled").and.returnValue(false);
+            $scope.available = [
+                {
+                    fstype: null,
+                    $selected: true,
+                    has_partitions: true
                 }
             ];
             $scope.cachesets = [{}];
@@ -3259,7 +3363,8 @@
                 $scope.available = [
                     {
                         fstype: null,
-                        $selected: true
+                        $selected: true,
+                        has_partitions: false
                     }
                 ];
                 $scope.cachesets = [];
@@ -3275,7 +3380,8 @@
                 $scope.available = [
                     {
                         fstype: null,
-                        $selected: true
+                        $selected: true,
+                        has_partitions: false
                     }
                 ];
                 $scope.cachesets = [{}];
@@ -3291,7 +3397,8 @@
                 $scope.available = [
                     {
                         fstype: null,
-                        $selected: true
+                        $selected: true,
+                        has_partitions: false
                     }
                 ];
                 $scope.cachesets = [{}];

=== modified file 'src/maasserver/static/partials/node-details.html'
--- src/maasserver/static/partials/node-details.html	2016-06-14 21:30:57 +0000
+++ src/maasserver/static/partials/node-details.html	2016-07-11 13:56:16 +0000
@@ -1746,8 +1746,9 @@
                         data-ng-disabled="!canCreateCacheSet()"
                         data-ng-hide="isAllStorageDisabled() || !isSuperUser()"
                         data-ng-click="createCacheSet()">Create cache Set</a>
-                    <a class="link-cta-ubuntu secondary margin-right tooltip"
-                        data-tooltip="Create at least one cache set, then select your backing device to create bcache"
+                    <a class="link-cta-ubuntu secondary margin-right"
+                        data-tooltip="{$ getCannotCreateBcacheMsg() $}"
+                        data-ng-class="{ tooltip: !canCreateBcache() }"
                         data-ng-disabled="!canCreateBcache()"
                         data-ng-hide="isAllStorageDisabled() || !isSuperUser()"
                         data-ng-click="createBcache()">Create bcache</a>