Merge lp:~rvb/maas/prevent-deletion into lp:~maas-committers/maas/trunk

[Raphaël Badin]

Do not allow the deletion of a node if it's owned by a user.

This is something we missed when refactoring the node lifecycle: now we have more than one state that means that a node is "in use". In this branch I changed the check that MAAS performs before allowing a node to be deleted: the check is now that it's not owned by someone. This is meant to force an admin (only an admin can delete a node) to consciously release an "in use" node before he's allowed to remove it.

[Graham Binns (gmb)]

review: Approve

[Jeroen T. Vermeulen (jtv)]

Two questions though:

1. Should an admin perhaps be allowed to delete a node that they own, so a user can't grab it at just the wrong moment?

2. Where the error message interpolates the owner, does it get the owner's name, or some other representation? AFAICT the test just uses the same interpolation, so it doesn't actually prove that the message is sensible.

[Raphaël Badin (rvb)]

> Two questions though:
>
> 1. Should an admin perhaps be allowed to delete a node that they own, so a
> user can't grab it at just the wrong moment?

I don't think that's worth adding: an admin can always mark a node as 'broken' to make sure nobody will grab it.

> 2. Where the error message interpolates the owner, does it get the owner's
> name, or some other representation? AFAICT the test just uses the same
> interpolation, so it doesn't actually prove that the message is sensible.

Good point, I've fixed this by using the username explicitly.

[MAAS Lander (maas-lander)]

The attempt to merge lp:~rvb/maas/prevent-deletion into lp:maas failed. Below is the output from the failed tests.

Ign http://security.ubuntu.com trusty-security InRelease
Get:1 http://security.ubuntu.com trusty-security Release.gpg [933 B]
Get:2 http://security.ubuntu.com trusty-security Release [62.0 kB]
Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Hit http://nova.clouds.archive.ubuntu.com trusty Release.gpg
Get:3 http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg [933 B]
Hit http://nova.clouds.archive.ubuntu.com trusty Release
Get:4 http://nova.clouds.archive.ubuntu.com trusty-updates Release [62.0 kB]
Get:5 http://security.ubuntu.com trusty-security/main Sources [49.5 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty/main Sources
Get:6 http://security.ubuntu.com trusty-security/universe Sources [11.2 kB]
Get:7 http://security.ubuntu.com trusty-security/main amd64 Packages [153 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Sources
Get:8 http://security.ubuntu.com trusty-security/universe amd64 Packages [51.2 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en
Get:9 http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources [136 kB]
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://security.ubuntu.com trusty-security/universe Translation-en
Get:10 http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources [89.3 kB]
Get:11 http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages [356 kB]
Get:12 http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages [217 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com trusty/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en_US
Fetched 1,188 kB in 2s (420 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 authbind bind9 bind9utils build-essential bzr-builddeb curl daemontools debhelper dh-apport distro-info dnsutils firefox freeipmi-tools gjs ipython isc-dhcp-common libjs-raphael libjs-yui3-full libjs-yui3-min libpq-dev make pep8 postgresql pyflakes python-amqplib python-bzrlib python-celery python-convoy python-crochet python-cssselect python-curtin python-dev python-distro-info python-django 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-lockfile python-lxml python-mimeparse python-mock python-netaddr python-netifaces python-nose python-oauth python-oops python-oops-amqp python-oops-datedir-repo python-oops-twisted python-oops-wsgi python-openssl py...

[Jason Hobbs (jason-hobbs)]

The ability to deleted an owned node is used by at least one customer - I don't think we should land this.

Their workflow is

1) Deploy a node with Juju
2) Delete the node from MAAS
3) Run the node offline

If they have to release the node first, it will shut the node down when that's not what they want.

review: Disapprove

[lp:~rvb/maas/prevent-deletion]

3336. By Raphaël Badin on 2014-11-05

Fix test failures.

[Graham Binns (gmb)]

On 5 November 2014 13:39, Jason Hobbs <email address hidden> wrote:
> The ability to deleted an owned node is used by at least one customer - I don't think we should land this.

!!!

I'd like to understand the use-case for this — other than "this is
what they do," because ISTM this isn't a workflow that we really want
to support.

[Raphaël Badin (rvb)]

> I'd like to understand the use-case for this — other than "this is
> what they do," because ISTM this isn't a workflow that we really want
> to support.

I see two valid workflows here:
- you might want to deploy nodes with a MAAS server and then use these nodes outside of MAAS (for instance because these nodes need to belong to a different service that is not comfortable letting others keep the credentials to the BMC of their nodes).
- if something goes wrong with a node (let say the BMC dies), it's awkward to force the admins to 'release' a node (because releasing will fail) before deleting the node.

Ultimately, the main thing that I was trying to fix here is the inconsistency; as long as deleting nodes is reserved to the admins, I'm fine with the new solution.

Unmerged revisions

3336. By Raphaël Badin on 2014-11-05

Fix test failures.

3335. By Raphaël Badin on 2014-11-05

Use username instead of str(user).

3334. By Raphaël Badin on 2014-11-05

Do not allow the deletion of a node if it's owned by a user.

=== modified file 'src/maasserver/api/tests/test_node.py'
--- src/maasserver/api/tests/test_node.py	2014-11-05 10:41:37 +0000
+++ src/maasserver/api/tests/test_node.py	2014-11-05 13:41:36 +0000
@@ -27,7 +27,6 @@
     IPADDRESS_TYPE,
     NODE_STATUS,
     NODE_STATUS_CHOICES,
-    NODE_STATUS_CHOICES_DICT,
     )
 from maasserver.fields import (
     MAC,
@@ -925,24 +924,25 @@
     def test_DELETE_deletes_node(self):
         # The api allows to delete a Node.
         self.become_admin()
-        node = factory.make_Node(owner=self.logged_in_user)
+        node = factory.make_Node(owner=None)
         system_id = node.system_id
         response = self.client.delete(self.get_node_uri(node))
 
         self.assertEqual(204, response.status_code)
         self.assertItemsEqual([], Node.objects.filter(system_id=system_id))
 
-    def test_DELETE_cannot_delete_allocated_node(self):
+    def test_DELETE_cannot_delete_owned_node(self):
         # The api allows to delete a Node.
         self.become_admin()
-        node = factory.make_Node(status=NODE_STATUS.ALLOCATED)
+        user = factory.make_User()
+        node = factory.make_Node(owner=user)
         response = self.client.delete(self.get_node_uri(node))
 
         self.assertEqual(
             (httplib.CONFLICT,
-                "Cannot delete node %s: node is in state %s." % (
+                "Cannot delete node %s: node is owned by user %s." % (
                     node.system_id,
-                    NODE_STATUS_CHOICES_DICT[NODE_STATUS.ALLOCATED])),
+                    user.username)),
             (response.status_code, response.content))
 
     def test_DELETE_deletes_node_fails_if_not_admin(self):

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2014-11-05 13:27:50 +0000
+++ src/maasserver/models/node.py	2014-11-05 13:41:36 +0000
@@ -881,10 +881,10 @@
     def delete(self):
         """Delete this node."""
         # Allocated nodes can't be deleted.
-        if self.status == NODE_STATUS.ALLOCATED:
+        if self.owner is not None:
             raise NodeStateViolation(
-                "Cannot delete node %s: node is in state %s."
-                % (self.system_id, NODE_STATUS_CHOICES_DICT[self.status]))
+                "Cannot delete node %s: node is owned by user %s."
+                % (self.system_id, self.owner.username))
 
         maaslog.info("%s: Deleting node", self.hostname)
 

=== modified file 'src/maasserver/models/tests/test_node.py'
--- src/maasserver/models/tests/test_node.py	2014-11-05 13:27:50 +0000
+++ src/maasserver/models/tests/test_node.py	2014-11-05 13:41:36 +0000
@@ -383,8 +383,8 @@
         self.assertRaises(
             MACAddress.DoesNotExist, MACAddress.objects.get, id=mac.id)
 
-    def test_cannot_delete_allocated_node(self):
-        node = factory.make_Node(status=NODE_STATUS.ALLOCATED)
+    def test_cannot_delete_owned_node(self):
+        node = factory.make_Node(owner=factory.make_User())
         self.assertRaises(NodeStateViolation, node.delete)
 
     def test_delete_node_also_deletes_related_static_IPs(self):

=== modified file 'src/maasserver/node_action.py'
--- src/maasserver/node_action.py	2014-11-03 17:18:09 +0000
+++ src/maasserver/node_action.py	2014-11-05 13:41:36 +0000
@@ -170,8 +170,11 @@
     permission = NODE_PERMISSION.ADMIN
 
     def inhibit(self):
-        if self.node.status == NODE_STATUS.ALLOCATED:
-            return "You cannot delete this node because it's in use."
+        if self.node.owner is not None:
+            return (
+                "You cannot delete this node because it's being used "
+                "by user %s." % self.node.owner.username
+            )
         return None
 
     def execute(self, allow_redirect=True):

=== modified file 'src/maasserver/support/pertenant/tests/test_migration.py'
--- src/maasserver/support/pertenant/tests/test_migration.py	2014-09-10 16:20:31 +0000
+++ src/maasserver/support/pertenant/tests/test_migration.py	2014-11-05 13:41:36 +0000
@@ -167,6 +167,8 @@
         factory.make_User()
         node = factory.make_Node(owner=user)
         make_provider_state_file(node)
+        node.owner = None
+        node.save()
         node.delete()  # Orphan the state.
         self.assertEqual(get_legacy_user(), get_destination_user())
 

=== modified file 'src/maasserver/support/pertenant/tests/test_utils.py'
--- src/maasserver/support/pertenant/tests/test_utils.py	2014-09-10 16:20:31 +0000
+++ src/maasserver/support/pertenant/tests/test_utils.py	2014-11-05 13:41:36 +0000
@@ -72,6 +72,8 @@
     def test_returns_None_if_node_does_not_exist(self):
         node = factory.make_Node(owner=factory.make_User())
         make_provider_state_file(node=node)
+        node.owner = None
+        node.save()
         node.delete()
         self.assertIsNone(get_bootstrap_node_owner())
 

=== modified file 'src/maasserver/tests/test_node_action.py'
--- src/maasserver/tests/test_node_action.py	2014-11-05 04:40:21 +0000
+++ src/maasserver/tests/test_node_action.py	2014-11-05 13:41:36 +0000
@@ -195,12 +195,15 @@
 
 class TestDeleteNodeAction(MAASServerTestCase):
 
-    def test_Delete_inhibit_when_node_is_allocated(self):
-        node = factory.make_Node(status=NODE_STATUS.ALLOCATED)
+    def test_Delete_inhibit_when_node_is_owned(self):
+        user = factory.make_User()
+        node = factory.make_Node(owner=user)
         action = Delete(node, factory.make_admin())
         inhibition = action.inhibit()
         self.assertEqual(
-            "You cannot delete this node because it's in use.", inhibition)
+            "You cannot delete this node because it's being used "
+            "by user %s." % user.username,
+            inhibition)
 
     def test_Delete_does_not_inhibit_otherwise(self):
         node = factory.make_Node(status=NODE_STATUS.FAILED_COMMISSIONING)