From aa4d3fa0220450517bebd621b6db0f8ca1fba0ca Mon Sep 17 00:00:00 2001
From: Lucas Alvares Gomes <lucasagomes@gmail.com>
Date: Wed, 16 Mar 2016 16:37:54 +0000
Subject: [PATCH] Make sure target state is cleared on stable states

When a node moves to a stable provisioning state and there's no extra
processing to be done (callback) we have to make sure that the
target_provision_state is cleared (set to None) indicating that the node
is not transitioning to any other state.

Closes-Bug: #1557497
Change-Id: Id4f05ed3c6acbeda07a726049e2857fcb96338db
---
 ironic/common/states.py                       | 10 ++++-----
 ironic/conductor/task_manager.py              |  9 +++++++-
 ironic/tests/unit/conductor/test_manager.py   |  2 +-
 .../tests/unit/conductor/test_task_manager.py | 22 +++++++++++++++++++
 ...target-stable-states-4545602d7aed9898.yaml |  5 +++++
 5 files changed, 41 insertions(+), 7 deletions(-)
 create mode 100644 releasenotes/notes/clear-target-stable-states-4545602d7aed9898.yaml

diff --git a/ironic/common/states.py b/ironic/common/states.py
index 721cbc4e35..09c4aff858 100644
--- a/ironic/common/states.py
+++ b/ironic/common/states.py
@@ -176,6 +176,9 @@ UPDATE_ALLOWED_STATES = (DEPLOYFAIL, INSPECTING, INSPECTFAIL, CLEANFAIL, ERROR,
 DELETE_ALLOWED_STATES = (AVAILABLE, NOSTATE, MANAGEABLE, ENROLL)
 """States in which node deletion is allowed."""
 
+STABLE_STATES = (ENROLL, MANAGEABLE, AVAILABLE, ACTIVE, ERROR)
+"""States that will not transition unless receiving a request."""
+
 
 ##############
 # Power states
@@ -212,11 +215,8 @@ watchers['on_enter'] = on_enter
 machine = fsm.FSM()
 
 # Add stable states
-machine.add_state(ENROLL, stable=True, **watchers)
-machine.add_state(MANAGEABLE, stable=True, **watchers)
-machine.add_state(AVAILABLE, stable=True, **watchers)
-machine.add_state(ACTIVE, stable=True, **watchers)
-machine.add_state(ERROR, stable=True, **watchers)
+for state in STABLE_STATES:
+    machine.add_state(state, stable=True, **watchers)
 
 # Add verifying state
 machine.add_state(VERIFYING, target=MANAGEABLE, **watchers)
diff --git a/ironic/conductor/task_manager.py b/ironic/conductor/task_manager.py
index a520816e39..a136c27e3e 100644
--- a/ironic/conductor/task_manager.py
+++ b/ironic/conductor/task_manager.py
@@ -382,7 +382,14 @@ class TaskManager(object):
                                       self.node.target_provision_state)
 
         self.node.provision_state = self.fsm.current_state
-        self.node.target_provision_state = self.fsm.target_state
+
+        # NOTE(lucasagomes): If there's no extra processing
+        # (callback) and we've moved to a stable state, make sure the
+        # target_provision_state is cleared
+        if not callback and self.fsm.is_stable(self.node.provision_state):
+            self.node.target_provision_state = states.NOSTATE
+        else:
+            self.node.target_provision_state = self.fsm.target_state
 
         # set up the async worker
         if callback:
diff --git a/ironic/tests/unit/conductor/test_manager.py b/ironic/tests/unit/conductor/test_manager.py
index 75eb0f40fb..48cf4c038f 100644
--- a/ironic/tests/unit/conductor/test_manager.py
+++ b/ironic/tests/unit/conductor/test_manager.py
@@ -1120,7 +1120,7 @@ class DoNodeDeployTearDownTestCase(mgr_utils.ServiceSetUpMixin,
                           self.service._do_node_tear_down, task)
         node.refresh()
         self.assertEqual(states.ERROR, node.provision_state)
-        self.assertEqual(states.AVAILABLE, node.target_provision_state)
+        self.assertEqual(states.NOSTATE, node.target_provision_state)
         self.assertIsNotNone(node.last_error)
         # Assert instance_info was erased
         self.assertEqual({}, node.instance_info)
diff --git a/ironic/tests/unit/conductor/test_task_manager.py b/ironic/tests/unit/conductor/test_task_manager.py
index 758edf3f3e..6c6bac1d46 100644
--- a/ironic/tests/unit/conductor/test_task_manager.py
+++ b/ironic/tests/unit/conductor/test_task_manager.py
@@ -625,6 +625,28 @@ class TaskManagerStateModelTestCases(tests_base.TestCase):
         self.assertNotEqual(target_provision_state,
                             self.node.target_provision_state)
 
+    def test_process_event_callback_stable_state(self):
+        callback = mock.Mock()
+        for state in states.STABLE_STATES:
+            self.node.provision_state = state
+            self.node.target_provision_state = 'target'
+            self.task.process_event = task_manager.TaskManager.process_event
+            self.task.process_event(self.task, 'fake', callback=callback)
+            # assert the target state is set when callback is passed
+            self.assertNotEqual(states.NOSTATE,
+                                self.task.node.target_provision_state)
+
+    def test_process_event_no_callback_stable_state(self):
+        for state in states.STABLE_STATES:
+            self.node.provision_state = state
+            self.node.target_provision_state = 'target'
+            self.task.process_event = task_manager.TaskManager.process_event
+            self.task.process_event(self.task, 'fake')
+            # assert the target state was cleared when moving to a
+            # stable state
+            self.assertEqual(states.NOSTATE,
+                             self.task.node.target_provision_state)
+
 
 @task_manager.require_exclusive_lock
 def _req_excl_lock_method(*args, **kwargs):
diff --git a/releasenotes/notes/clear-target-stable-states-4545602d7aed9898.yaml b/releasenotes/notes/clear-target-stable-states-4545602d7aed9898.yaml
new file mode 100644
index 0000000000..e5bac700d4
--- /dev/null
+++ b/releasenotes/notes/clear-target-stable-states-4545602d7aed9898.yaml
@@ -0,0 +1,5 @@
+---
+fixes:
+  - Ensure node's target_provision_state is cleared when the node
+    is moved to a stable state, indicating that the state transition
+    is done.