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

[Lee Trager]

Allow deleting a rack controller from the UI and provide more descriptive error messages when unable to delete.

This add the ability to delete rack controllers in the UI. I added more descriptive error messages which are shown in both the UI and API when unable to delete a rack controller due to being attached to a VLAN. If a rack controller is unable to be deleted due to it being set as a secondary rack controller the UI will ask the user if they want to unlink the rack controller from all VLANs and try to delete again.

Deleting a rack controller still deletes the node object. Let me know if we want to have it convert back to a machine.

[Andres Rodriguez (andreserl)]

Not a full review, just on the messaging.

review: Needs Information

[Lee Trager (ltrager)]

I've updated the error messages to display the <fabric name>.<vlan name> when displaying which VLAN's a rack is still connected to. I noticed that VLAN.get_name didn't handle unnamed VLAN's so it now returns the VID if the VLAN has no name.

These messages appear over the API and UI when trying to delete a rack controller which are still set to one or more VLAN's primary or secondary rack controller. If the rack controller is unable to be deleted due to being a secondary rack controller on one or more VLAN's the UI will ask the user if they'd like to remove that association and then delete the rack.

[Jeffrey C Jones (trapnine)]

Looks awesome, just a test needed for the error message string matching (and some comments about it), and some simple typos.

review: Needs Fixing

[Lee Trager (ltrager)]

Thanks for the review. I've fixed the typos and added the missing tests.

[Jeffrey C Jones (trapnine)]

> Thanks for the review. I've fixed the typos and added the missing tests.

Lookin' good.

review: Approve

[Mike Pontillo (mpontillo)]

Sorry this took so long for me to review.

Some comments below, mostly non-blocking.

But I was a bit confused about the "force" behavior, so I cloned your branch locally to test it out. I configured DHCP on the VLAN and got an error that the rack was still connected. So I stopped the rack, then got the error that it was a primary DHCP provider on a VLAN. So I disabled DHCP and tried it again; then I got an error as follows:

The delete action for 1 controller failed with error: 'RackController' object has no attribute '_fields_last_seen_values__node_type'

Full traceback here:

https://paste.ubuntu.com/16089729/

So that needs to be fixed before this can land.

Just curious why the "force" parameter was added? It looks like there is no way for a user to pass in force=True; at least I didn't see it in the UI?

review: Needs Fixing

[Andres Rodriguez (andreserl)]

Ok, Mike's review highlights something. If I understand correctly:

"got an error that the rack was still connected."

That means that if a rack controller is running we cannot delete the rack? That is a serious issue that we need to fix (a usability one). We should not, at all cost, have to stop maas-rackd manually before we can delete a rack.

In fact, I'm thinking that we have to automatically tell the rack to stop, however, the problem is that if we are to restart the machine, it will auto-register again. As such, we need to discuss this specific problem.

review: Needs Fixing

[Lee Trager (ltrager)]

Deleting a rack now always automatically removes any secondary_rack links. The only change to the UI is that the operations drop down is displayed on the node-list page. As per our discussion this morning the rack still has to be manually shutoff. I'll post a review later which disables the maas-rackd service.

Mike, I took a look at str(VLAN), its outputting "%s.%s" % (self.fabric.get_name(), self.get_name()). Is that close enough to what you wanted displayed or did you want it to be "%s on %s" % (vlan.get_name(), vlan.fabric.get_name())? Should I change the __str__ method?

[Andres Rodriguez (andreserl)]

So I just want to confirm. When you delete a rack controller that's a secondary rack controlller, it will show aconfirmation message to proceed, otherwise cancel right?

[Lee Trager (ltrager)]

The Cancel/Go message is displayed in the UI when you click delete the same way its displayed when deleting a machine or device. If the rack controller is a secondary rack controller the association will be automatically removed once you click Go.

[Andres Rodriguez (andreserl)]

I'm ok with this branch now, although, I'd like mike to review it before landing.

review: Approve

[Mike Pontillo (mpontillo)]

Looks good to me. Thanks.

review: Approve

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2016-05-09 17:19:23 +0000
+++ src/maasserver/models/node.py	2016-05-09 19:08:38 +0000
@@ -3644,11 +3644,23 @@
         from maasserver.models import RegionRackRPCConnection
         connections = RegionRackRPCConnection.objects.filter(
             rack_controller=self)
-
         if len(connections) != 0:
             raise ValidationError(
                 "Unable to delete %s as it's currently connected to one or "
                 "more regions." % self.hostname)
+
+        primary_vlans = VLAN.objects.filter(primary_rack=self)
+        if len(primary_vlans) != 0:
+            raise ValidationError(
+                "Unable to delete '%s'; it is currently set as a primary rack"
+                " controller on VLANs %s" %
+                (self.hostname,
+                    ', '.join([str(vlan) for vlan in primary_vlans])))
+
+        for vlan in VLAN.objects.filter(secondary_rack=self):
+            vlan.secondary_rack = None
+            vlan.save()
+
         if self.node_type == NODE_TYPE.REGION_AND_RACK_CONTROLLER:
             self.node_type = NODE_TYPE.REGION_CONTROLLER
             self.save()

=== modified file 'src/maasserver/models/tests/test_node.py'
--- src/maasserver/models/tests/test_node.py	2016-05-04 16:23:11 +0000
+++ src/maasserver/models/tests/test_node.py	2016-05-09 19:08:38 +0000
@@ -6951,6 +6951,20 @@
         mock_filter.return_value = [rackcontroller]
         self.assertRaises(ValidationError, rackcontroller.delete)
 
+    def test_prevents_delete_when_primary_rack(self):
+        rackcontroller = factory.make_RackController()
+        factory.make_VLAN(primary_rack=rackcontroller)
+        self.assertRaises(ValidationError, rackcontroller.delete)
+
+    def test_delete_removes_secondary_link(self):
+        rackcontroller = factory.make_RackController()
+        vlan = factory.make_VLAN(secondary_rack=rackcontroller)
+        rackcontroller.delete()
+        self.assertIsNone(reload_object(vlan).secondary_rack)
+        self.assertRaises(
+            RackController.DoesNotExist,
+            RackController.objects.get, system_id=rackcontroller.system_id)
+
     def test_delete_converts_region_and_rack_to_region(self):
         region_and_rack = factory.make_Node(
             node_type=NODE_TYPE.REGION_AND_RACK_CONTROLLER)

=== modified file 'src/maasserver/models/tests/test_vlan.py'
--- src/maasserver/models/tests/test_vlan.py	2016-03-28 13:54:47 +0000
+++ src/maasserver/models/tests/test_vlan.py	2016-05-09 19:08:38 +0000
@@ -102,6 +102,10 @@
         vlan = factory.make_VLAN(name=name)
         self.assertEqual(name, vlan.get_name())
 
+    def test_get_name_for_unnamed_vlan(self):
+        vlan = factory.make_VLAN()
+        self.assertEqual(str(vlan.vid), vlan.get_name())
+
     def test_creates_vlan(self):
         name = factory.make_name('name')
         vid = random.randint(3, 55)

=== modified file 'src/maasserver/models/vlan.py'
--- src/maasserver/models/vlan.py	2016-04-27 20:38:06 +0000
+++ src/maasserver/models/vlan.py	2016-05-09 19:08:38 +0000
@@ -246,8 +246,10 @@
         """Return the name of the VLAN."""
         if self.is_fabric_default():
             return "untagged"
-        else:
+        elif self.name is not None:
             return self.name
+        else:
+            return str(self.vid)
 
     def manage_connected_interfaces(self):
         """Deal with connected interfaces:

=== modified file 'src/maasserver/static/partials/node-details.html'
--- src/maasserver/static/partials/node-details.html	2016-04-26 19:52:34 +0000
+++ src/maasserver/static/partials/node-details.html	2016-05-09 19:08:38 +0000
@@ -39,16 +39,8 @@
                     data-ng-disabled="editHeaderInvalid()"
                     data-ng-click="saveEditHeader()">Save</a>
             </h1>
-            <!-- XXX trapnine 2016-02-21 - Sample HTML disabled until implemented. -->
-            <div class="page-header__actions four-col last-col" data-ng-show="false && isController">
-                 <div class="page-header__cta two-col no-margin-bottom last-col">
-                    <div class="two-col no-margin-bottom last-col">
-                        <a href="#" class="link-cta-ubuntu right">Remove</a>
-                    </div>
-                </div>
-            </div>
             <!-- XXX blake_r 2015-02-19 - Need to add e2e test. -->
-            <div data-ng-show="!isController && !header.editing && availableActionOptions.length">
+            <div data-ng-show="!header.editing && availableActionOptions.length">
                 <div class="page-header__actions four-col last-col">
                      <div class="page-header__cta two-col no-margin-bottom last-col">
                         <div class="two-col no-margin-bottom last-col">

=== modified file 'src/maasserver/websockets/handlers/node.py'
--- src/maasserver/websockets/handlers/node.py	2016-03-28 13:54:47 +0000
+++ src/maasserver/websockets/handlers/node.py	2016-05-09 19:08:38 +0000
@@ -21,6 +21,7 @@
 from maasserver.models.config import Config
 from maasserver.models.event import Event
 from maasserver.models.filesystemgroup import VolumeGroup
+from maasserver.models.node import typecast_to_node_type
 from maasserver.models.nodeprobeddetails import get_single_probed_details
 from maasserver.models.physicalblockdevice import PhysicalBlockDevice
 from maasserver.models.virtualblockdevice import VirtualBlockDevice
@@ -476,9 +477,9 @@
         """Get object by using the `pk` in `params`."""
         obj = super(NodeHandler, self).get_object(params)
         if self.user.is_superuser:
-            return obj
+            return typecast_to_node_type(obj)
         if obj.owner is None or obj.owner == self.user:
-            return obj
+            return typecast_to_node_type(obj)
         raise HandlerDoesNotExistError(params[self._meta.pk])
 
     def get_mac_addresses(self, data):