Merge lp:~julian-edwards/maas/only-ready-when-power-is-off into lp:~maas-committers/maas/trunk

[Julian Edwards]

Only mark nodes READY once power off is complete. It does this by introducing a new state, RELEASING, which is transitioned to READY when the power job calls back successfully. This does not affect the transition to BROKEN if the power job failed.

There was some collateral with this branch, my changed exposed some other potential problems some of which I fixed:

 - release() always tries to power off the node, even though it may have never been powered on. I've left this as is but I welcome your comments as the reviewer here.

 - release() was being unit tested via the API (!) so I removed all of those tests in favour of a more simple enhanced unit test.

 - The start node action could only operate on ALLOCATED or DEPLOYED nodes, however if it got a StaticIPAddressExhaustion error, it was releasing the node! So I stopped it doing that.

[Julian Edwards (julian-edwards)]

I tested this locally and it seems to be ok, you can't start the node again until the poweroff script finishes. In the meantime, the UI shows "Releasing". It'd be great if we had all this on longpoll :)

[Jeroen T. Vermeulen (jtv)]

Thanks for cleaning up the tests. We have far too much logic, and tests, at the API level.

I think it's OK for release() to power off the node always, because our knowledge of its current power state may be out of date. I doubt we control all state transitions thoroughly at the moment, so there may be obscure paths like: node is switched off; power poller registers that it's off; node is marked as broken; power poller stops looking at the node because it's broken; admin powers node on and works their magic; admin marks node as fixed; user allocates node; MAAS issues power-on command; node misses the command because it's already on.

So as far as I'm concerned, the XXX comment can become just a regular comment note. In fact I probably wouldn't even bother with a special code path for the case where the node is already off: the power method can figure out that it's not needed, or if it prefers, it can issue a probably redundant power-off command for luck.

And looking at the code under that XXX comment, does it make sense to keep the owner set when releasing a node? The user is done with the node at that point. For debugging it may well be interesting to know who was using it, but that's historical information — maybe it belongs in the node event log. So I think I'd just clear the node's owner during release() either way.

One thing is completely unclear to me: what does the RESERVED state represent? It may have come up in London but if so I don't remember. There's a comment in the enum that talks about the node being ready for “named” deployment. I have no idea what that means. I'm not even sure whether it's talking about a DNS daemon or the normal adjective. I don't see any place in the code where a node might enter this state. Yet your test uses this state for a powered-off node and not for a powered-on node... Why? What's the distinction?

review: Approve

[Raphaël Badin (rvb)]

> I tested this locally and it seems to be ok, you can't start the node again until the poweroff
> script finishes.

What happens if powering the machine off fails? (if the template blows up or if the status of the node is never changed?) I'd say we need to introduce a RELEASING_FAILED state but I'd like to hear your thoughts on this.

[Julian Edwards (julian-edwards)]

On Monday 15 Sep 2014 07:22:08 you wrote:
> > I tested this locally and it seems to be ok, you can't start the node
> > again until the poweroff script finishes.
>
> What happens if powering the machine off fails? (if the template blows up or
> if the status of the node is never changed?) I'd say we need to introduce
> a RELEASING_FAILED state but I'd like to hear your thoughts on this.

It will be marked BROKEN; that's already all been done.

[Raphaël Badin (rvb)]

> On Monday 15 Sep 2014 07:22:08 you wrote:
> > > I tested this locally and it seems to be ok, you can't start the node
> > > again until the poweroff script finishes.
> >
> > What happens if powering the machine off fails? (if the template blows up or
> > if the status of the node is never changed?) I'd say we need to introduce
> > a RELEASING_FAILED state but I'd like to hear your thoughts on this.
>
> It will be marked BROKEN; that's already all been done.

That's not what BROKEN means. BROKEN is a state only a manual action from a user should put a node into.

[Julian Edwards (julian-edwards)]

On Monday 15 Sep 2014 09:34:27 you wrote:
> > On Monday 15 Sep 2014 07:22:08 you wrote:
> > > > I tested this locally and it seems to be ok, you can't start the node
> > > > again until the poweroff script finishes.
> > >
> > > What happens if powering the machine off fails? (if the template blows
> > > up or if the status of the node is never changed?) I'd say we need to
> > > introduce a RELEASING_FAILED state but I'd like to hear your thoughts
> > > on this.>
> > It will be marked BROKEN; that's already all been done.
>
> That's not what BROKEN means. BROKEN is a state only a manual action from a
> user should put a node into.

Then whoever did the work on the power stuff needs to know that.

[Julian Edwards (julian-edwards)]

On Monday 15 Sep 2014 07:07:25 you wrote:
> I think it's OK for release() to power off the node always, because our
> knowledge of its current power state may be out of date. I doubt we
> control all state transitions thoroughly at the moment, so there may be
> obscure paths like: node is switched off; power poller registers that it's
> off; node is marked as broken; power poller stops looking at the node
> because it's broken; admin powers node on and works their magic; admin
> marks node as fixed; user allocates node; MAAS issues power-on command;
> node misses the command because it's already on.
>
> So as far as I'm concerned, the XXX comment can become just a regular
> comment note. In fact I probably wouldn't even bother with a special code
> path for the case where the node is already off: the power method can
> figure out that it's not needed, or if it prefers, it can issue a probably
> redundant power-off command for luck.

I'll try it and see how it works out, but I am concerned it will lead to
concurrent power operations - we must avoid the situation where we need to
serialize them at the power driver level, that's just madness. It's good
practice to catch problems as early as you can!

> And looking at the code under that XXX comment, does it make sense to keep
> the owner set when releasing a node? The user is done with the node at
> that point. For debugging it may well be interesting to know who was using
> it, but that's historical information — maybe it belongs in the node event
> log. So I think I'd just clear the node's owner during release() either
> way.

It's not released until it's off IMO, which is why I kept the owner on. The
state RELEASING implies it's not released yet as well.

> One thing is completely unclear to me: what does the RESERVED state
> represent? It may have come up in London but if so I don't remember.
> There's a comment in the enum that talks about the node being ready for
> “named” deployment. I have no idea what that means. I'm not even sure
> whether it's talking about a DNS daemon or the normal adjective. I don't
> see any place in the code where a node might enter this state. Yet your
> test uses this state for a powered-off node and not for a powered-on
> node... Why? What's the distinction?

It can be deleted as we discussed on the call.

Thanks for the review!

=== modified file 'src/maasserver/api/tests/test_node.py'
--- src/maasserver/api/tests/test_node.py	2014-09-10 16:20:31 +0000
+++ src/maasserver/api/tests/test_node.py	2014-09-16 01:19:01 +0000
@@ -47,6 +47,7 @@
 from maasserver.testing.osystems import make_usable_osystem
 from maasserver.testing.testcase import MAASServerTestCase
 from maastesting.matchers import (
+    Equals,
     MockCalledOnceWith,
     MockNotCalled,
     )
@@ -402,7 +403,7 @@
             [httplib.OK] * len(owned_nodes),
             [response.status_code for response in responses])
         self.assertItemsEqual(
-            [NODE_STATUS.READY] * len(owned_nodes),
+            [NODE_STATUS.RELEASING] * len(owned_nodes),
             [node.status for node in reload_objects(Node, owned_nodes)])
 
     def test_POST_release_releases_failed_node(self):
@@ -414,60 +415,8 @@
         self.assertEqual(
             httplib.OK, response.status_code, response.content)
         owned_node = Node.objects.get(id=owned_node.id)
-        self.assertEqual(
-            (NODE_STATUS.READY, None),
-            (owned_node.status, owned_node.owner))
-
-    def test_POST_release_turns_on_netboot(self):
-        node = factory.make_Node(
-            status=NODE_STATUS.ALLOCATED, owner=self.logged_in_user)
-        node.set_netboot(on=False)
-        self.client.post(self.get_node_uri(node), {'op': 'release'})
-        self.assertTrue(reload_object(node).netboot)
-
-    def test_POST_release_resets_osystem_and_distro_series(self):
-        osystem = factory.pick_OS()
-        release = factory.pick_release(osystem)
-        node = factory.make_Node(
-            status=NODE_STATUS.ALLOCATED, owner=self.logged_in_user,
-            osystem=osystem.name, distro_series=release)
-        self.client.post(self.get_node_uri(node), {'op': 'release'})
-        self.assertEqual('', reload_object(node).osystem)
-        self.assertEqual('', reload_object(node).distro_series)
-
-    def test_POST_release_resets_license_key(self):
-        osystem = factory.pick_OS()
-        release = factory.pick_release(osystem)
-        license_key = factory.make_string()
-        node = factory.make_Node(
-            status=NODE_STATUS.ALLOCATED, owner=self.logged_in_user,
-            osystem=osystem.name, distro_series=release,
-            license_key=license_key)
-        self.client.post(self.get_node_uri(node), {'op': 'release'})
-        self.assertEqual('', reload_object(node).license_key)
-
-    def test_POST_release_resets_agent_name(self):
-        agent_name = factory.make_name('agent-name')
-        osystem = factory.pick_OS()
-        release = factory.pick_release(osystem)
-        node = factory.make_Node(
-            status=NODE_STATUS.ALLOCATED, owner=self.logged_in_user,
-            osystem=osystem.name, distro_series=release,
-            agent_name=agent_name)
-        self.client.post(self.get_node_uri(node), {'op': 'release'})
-        self.assertEqual('', reload_object(node).agent_name)
-
-    def test_POST_release_removes_token_and_user(self):
-        node = factory.make_Node(status=NODE_STATUS.READY)
-        self.client.post(reverse('nodes_handler'), {'op': 'acquire'})
-        node = Node.objects.get(system_id=node.system_id)
-        self.assertEqual(NODE_STATUS.ALLOCATED, node.status)
-        self.assertEqual(self.logged_in_user, node.owner)
-        self.assertEqual(self.client.token.key, node.token.key)
-        self.client.post(self.get_node_uri(node), {'op': 'release'})
-        node = Node.objects.get(system_id=node.system_id)
-        self.assertIs(None, node.owner)
-        self.assertIs(None, node.token)
+        self.expectThat(owned_node.status, Equals(NODE_STATUS.RELEASING))
+        self.expectThat(owned_node.owner, Equals(self.logged_in_user))
 
     def test_POST_release_does_nothing_for_unowned_node(self):
         node = factory.make_Node(
@@ -530,8 +479,8 @@
         self.become_admin()
         response = self.client.post(
             self.get_node_uri(node), {'op': 'release'})
-        self.assertEqual(httplib.OK, response.status_code)
-        self.assertEqual(NODE_STATUS.READY, reload_object(node).status)
+        self.assertEqual(httplib.OK, response.status_code, response.content)
+        self.assertEqual(NODE_STATUS.RELEASING, reload_object(node).status)
 
     def test_POST_release_combines_with_acquire(self):
         node = factory.make_Node(status=NODE_STATUS.READY)
@@ -540,8 +489,8 @@
         self.assertEqual(NODE_STATUS.ALLOCATED, reload_object(node).status)
         node_uri = json.loads(response.content)['resource_uri']
         response = self.client.post(node_uri, {'op': 'release'})
-        self.assertEqual(httplib.OK, response.status_code)
-        self.assertEqual(NODE_STATUS.READY, reload_object(node).status)
+        self.assertEqual(httplib.OK, response.status_code, response.content)
+        self.assertEqual(NODE_STATUS.RELEASING, reload_object(node).status)
 
     def test_POST_commission_commissions_node(self):
         node = factory.make_Node(
 
=== modified file 'src/maasserver/enum.py'
--- src/maasserver/enum.py	2014-09-15 10:35:35 +0000
+++ src/maasserver/enum.py	2014-09-16 01:19:01 +0000
@@ -74,6 +74,8 @@
     ALLOCATED = 10
     #: The deployment of the node failed.
     FAILED_DEPLOYMENT = 11
+    #: The node is powering down after a release request.
+    RELEASING = 12
 
 
 # Django choices for NODE_STATUS: sequence of tuples (key, UI
@@ -91,6 +93,7 @@
     (NODE_STATUS.RETIRED, "Retired"),
     (NODE_STATUS.BROKEN, "Broken"),
     (NODE_STATUS.FAILED_DEPLOYMENT, "Failed deployment"),
+    (NODE_STATUS.RELEASING, "Releasing"),
 )
 
 
 
=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2014-09-15 14:28:28 +0000
+++ src/maasserver/models/node.py	2014-09-16 01:19:01 +0000
@@ -161,6 +161,7 @@
         NODE_STATUS.RETIRED,
         NODE_STATUS.MISSING,
         NODE_STATUS.BROKEN,
+        NODE_STATUS.RELEASING,
         ],
     NODE_STATUS.ALLOCATED: [
         NODE_STATUS.READY,
@@ -168,6 +169,12 @@
         NODE_STATUS.MISSING,
         NODE_STATUS.BROKEN,
         NODE_STATUS.DEPLOYING,
+        NODE_STATUS.RELEASING,
+        ],
+    NODE_STATUS.RELEASING: [
+        NODE_STATUS.READY,
+        NODE_STATUS.BROKEN,
+        NODE_STATUS.MISSING,
         ],
     NODE_STATUS.DEPLOYING: [
         NODE_STATUS.ALLOCATED,
@@ -176,18 +183,21 @@
         NODE_STATUS.FAILED_DEPLOYMENT,
         NODE_STATUS.DEPLOYED,
         NODE_STATUS.READY,
+        NODE_STATUS.RELEASING,
     ],
     NODE_STATUS.FAILED_DEPLOYMENT: [
         NODE_STATUS.ALLOCATED,
         NODE_STATUS.MISSING,
         NODE_STATUS.BROKEN,
         NODE_STATUS.READY,
+        NODE_STATUS.RELEASING,
     ],
     NODE_STATUS.DEPLOYED: [
         NODE_STATUS.ALLOCATED,
         NODE_STATUS.MISSING,
         NODE_STATUS.BROKEN,
         NODE_STATUS.READY,
+        NODE_STATUS.RELEASING,
     ],
     NODE_STATUS.MISSING: [
         NODE_STATUS.NEW,
@@ -205,6 +215,7 @@
     NODE_STATUS.BROKEN: [
         NODE_STATUS.COMMISSIONING,
         NODE_STATUS.READY,
+        NODE_STATUS.RELEASING,
         ],
     }
 
@@ -1238,8 +1249,13 @@
         self.delete_host_maps(deallocated_ips)
         from maasserver.dns.config import change_dns_zones
         change_dns_zones([self.nodegroup])
-        self.status = NODE_STATUS.READY
-        self.owner = None
+        if self.power_state == POWER_STATE.OFF:
+            self.status = NODE_STATUS.READY
+            self.owner = None
+        else:
+            # update_power_state() will take care of making the node READY
+            # and unowned when the power is finally off.
+            self.status = NODE_STATUS.RELEASING
         self.token = None
         self.agent_name = ''
         self.set_netboot()
@@ -1300,7 +1316,10 @@
         """
         if self.status in RELEASABLE_STATUSES:
             self.release()
+        # release() normally sets the status to RELEASING and leaves the
+        # owner in place, override that here as we're broken.
         self.status = NODE_STATUS.BROKEN
+        self.owner = None
         self.error_description = error_description
         self.save()
 
@@ -1317,6 +1336,14 @@
     def update_power_state(self, power_state):
         """Update a node's power state """
         self.power_state = power_state
+        mark_ready = (
+            self.status == NODE_STATUS.RELEASING and
+            power_state == POWER_STATE.OFF)
+        if mark_ready:
+            # Ensure the node is fully released after a successful power
+            # down.
+            self.status = NODE_STATUS.READY
+            self.owner = None
         self.save()
 
     def claim_static_ip_addresses(self):
 
=== modified file 'src/maasserver/models/tests/test_node.py'
--- src/maasserver/models/tests/test_node.py	2014-09-15 14:28:28 +0000
+++ src/maasserver/models/tests/test_node.py	2014-09-16 01:19:01 +0000
@@ -91,6 +91,7 @@
     AfterPreprocessing,
     Equals,
     HasLength,
+    Is,
     MatchesStructure,
     )
 from twisted.internet import defer
@@ -642,15 +643,37 @@
             (user, NODE_STATUS.ALLOCATED, agent_name),
             (node.owner, node.status, node.agent_name))
 
-    def test_release(self):
-        agent_name = factory.make_name('agent-name')
-        node = factory.make_Node(
-            status=NODE_STATUS.ALLOCATED, owner=factory.make_User(),
-            agent_name=agent_name)
-        node.release()
-        self.assertEqual(
-            (NODE_STATUS.READY, None, node.agent_name),
-            (node.status, node.owner, ''))
+    def test_release_node_that_has_power_on(self):
+        agent_name = factory.make_name('agent-name')
+        owner = factory.make_User()
+        node = factory.make_Node(
+            status=NODE_STATUS.ALLOCATED, owner=owner, agent_name=agent_name)
+        node.power_state = POWER_STATE.ON
+        node.release()
+        self.expectThat(node.status, Equals(NODE_STATUS.RELEASING))
+        self.expectThat(node.owner, Equals(owner))
+        self.expectThat(node.agent_name, Equals(''))
+        self.expectThat(node.token, Is(None))
+        self.expectThat(node.netboot, Is(True))
+        self.expectThat(node.osystem, Equals(''))
+        self.expectThat(node.distro_series, Equals(''))
+        self.expectThat(node.license_key, Equals(''))
+
+    def test_release_node_that_has_power_off(self):
+        agent_name = factory.make_name('agent-name')
+        owner = factory.make_User()
+        node = factory.make_Node(
+            status=NODE_STATUS.ALLOCATED, owner=owner, agent_name=agent_name)
+        node.power_state = POWER_STATE.OFF
+        node.release()
+        self.expectThat(node.status, Equals(NODE_STATUS.READY))
+        self.expectThat(node.owner, Is(None))
+        self.expectThat(node.agent_name, Equals(''))
+        self.expectThat(node.token, Is(None))
+        self.expectThat(node.netboot, Is(True))
+        self.expectThat(node.osystem, Equals(''))
+        self.expectThat(node.distro_series, Equals(''))
+        self.expectThat(node.license_key, Equals(''))
 
     def test_release_deletes_static_ip_host_maps(self):
         remove_host_maps = self.patch_autospec(
@@ -1208,10 +1231,11 @@
     def test_mark_broken_releases_allocated_node(self):
         node = factory.make_Node(
             status=NODE_STATUS.ALLOCATED, owner=factory.make_User())
-        node.mark_broken(factory.make_name('error-description'))
-        node = reload_object(node)
-        self.assertEqual(
-            (NODE_STATUS.BROKEN, None), (node.status, node.owner))
+        err_desc = factory.make_name('error-description')
+        release = self.patch(node, 'release')
+        node.mark_broken(err_desc)
+        self.expectThat(node.owner, Is(None))
+        self.assertThat(release, MockCalledOnceWith())
 
     def test_mark_fixed_changes_status(self):
         node = factory.make_Node(status=NODE_STATUS.BROKEN)
@@ -1237,6 +1261,26 @@
         node.update_power_state(state)
         self.assertEqual(state, reload_object(node).power_state)
 
+    def test_update_power_state_readies_node_if_releasing(self):
+        node = factory.make_Node(
+            power_state=POWER_STATE.ON, status=NODE_STATUS.RELEASING,
+            owner=None)
+        node.update_power_state(POWER_STATE.OFF)
+        self.expectThat(node.status, Equals(NODE_STATUS.READY))
+        self.expectThat(node.owner, Is(None))
+
+    def test_update_power_state_does_not_change_status_if_not_releasing(self):
+        node = factory.make_Node(
+            power_state=POWER_STATE.ON, status=NODE_STATUS.ALLOCATED)
+        node.update_power_state(POWER_STATE.OFF)
+        self.assertThat(node.status, Equals(NODE_STATUS.ALLOCATED))
+
+    def test_update_power_state_does_not_change_status_if_not_off(self):
+        node = factory.make_Node(
+            power_state=POWER_STATE.OFF, status=NODE_STATUS.ALLOCATED)
+        node.update_power_state(POWER_STATE.ON)
+        self.expectThat(node.status, Equals(NODE_STATUS.ALLOCATED))
+
     def test_end_deployment_changes_state(self):
         node = factory.make_Node(status=NODE_STATUS.DEPLOYING)
         node.end_deployment()
 
=== modified file 'src/maasserver/node_action.py'
--- src/maasserver/node_action.py	2014-09-12 03:16:19 +0000
+++ src/maasserver/node_action.py	2014-09-16 01:19:01 +0000
@@ -284,7 +284,6 @@
         try:
             Node.objects.start_nodes([self.node.system_id], self.user)
         except StaticIPAddressExhaustion:
-            self.node.release()
             raise NodeActionError(
                 "%s: Failed to start, static IP addresses are exhausted."
                 % self.node.hostname)
 
=== modified file 'src/maasserver/tests/test_node_action.py'
--- src/maasserver/tests/test_node_action.py	2014-09-10 16:20:31 +0000
+++ src/maasserver/tests/test_node_action.py	2014-09-16 01:19:01 +0000
@@ -23,6 +23,7 @@
     NODE_STATUS,
     NODE_STATUS_CHOICES,
     NODE_STATUS_CHOICES_DICT,
+    POWER_STATE,
     )
 from maasserver.exceptions import (
     NodeActionError,
@@ -50,6 +51,7 @@
 from maasserver.testing.testcase import MAASServerTestCase
 from maastesting.matchers import MockCalledOnceWith
 from mock import ANY
+from testtools.matchers import Equals
 
 
 ALL_STATUSES = NODE_STATUS_CHOICES_DICT.keys()
@@ -290,7 +292,8 @@
     def test_StartNode_returns_error_when_no_more_static_IPs(self):
         user = factory.make_User()
         node = factory.make_node_with_mac_attached_to_nodegroupinterface(
-            status=NODE_STATUS.ALLOCATED, power_type='ether_wake', owner=user)
+            status=NODE_STATUS.ALLOCATED, power_type='ether_wake', owner=user,
+            power_state=POWER_STATE.OFF)
         ngi = node.get_primary_mac().cluster_interface
 
         # Narrow the available IP range and pre-claim the only address.
@@ -300,10 +303,11 @@
             ngi.static_ip_range_high, ngi.static_ip_range_low)
 
         e = self.assertRaises(NodeActionError, StartNode(node, user).execute)
-        self.assertEqual(
-            "%s: Failed to start, static IP addresses are exhausted." %
-            node.hostname, e.message)
-        self.assertEqual(NODE_STATUS.READY, node.status)
+        self.expectThat(
+            e.message, Equals(
+                "%s: Failed to start, static IP addresses are exhausted." %
+                node.hostname))
+        self.assertEqual(NODE_STATUS.ALLOCATED, node.status)
 
     def test_StartNode_requires_edit_permission(self):
         user = factory.make_User()
@@ -351,8 +355,7 @@
 
         ReleaseNode(node, user).execute()
 
-        self.assertEqual(NODE_STATUS.READY, node.status)
-        self.assertIsNone(node.owner)
+        self.expectThat(node.status, Equals(NODE_STATUS.RELEASING))
         self.assertThat(
             stop_nodes, MockCalledOnceWith([node.system_id], user))