Clean up instance_uuid as part of the node's tear down

Prior to this patch Ironic would expect Nova to clean up the instance
association, but this assumption does not play well with the standalone
version of Ironic or cleaning.

This patch keeps a backward compatibility layer with old versions of Nova
that will try to clean up the instance_uuid field even if it's already
cleaned.

Depends-On: I0b1b710056d48c8b7bb2b46fdaba192922926420
Closes-Bug: #1436568
Change-Id: I58bc848ab3a60d3e9ab16d00ce96a603df1c244a

ironic/api/controllers/v1/node.py

@@ -1036,20 +1036,23 @@ class NodesController(rest.RestController):
 
         rpc_node = api_utils.get_rpc_node(node_ident)
 
-        # Check if node is transitioning state, although nodes in some states
-        # can be updated.
-        if ((rpc_node.maintenance or
-                rpc_node.provision_state == ir_states.CLEANING) and
-                patch == [{'op': 'remove', 'path': '/instance_uuid'}]):
-            # Allow node.instance_uuid removal during cleaning, but not other
-            # operations. Also allow it during maintenance, to break
-            # association with Nova in case of serious problems.
-            # TODO(JoshNang) remove node.instance_uuid when removing
-            # instance_info and stop removing node.instance_uuid in the Nova
-            # Ironic driver. Bug: 1436568
+        # TODO(lucasagomes): This code is here for backward compatibility
+        # with old nova Ironic drivers that will attempt to remove the
+        # instance even if it's already deleted in Ironic. This conditional
+        # should be removed in the next cycle (Mitaka).
+        remove_inst_uuid_patch = [{'op': 'remove', 'path': '/instance_uuid'}]
+        if (rpc_node.provision_state in (ir_states.CLEANING,
+                                         ir_states.CLEANWAIT)
+            and patch == remove_inst_uuid_patch):
+            # The instance_uuid is already removed as part of the node's
+            # tear down, skip this update.
+            return Node.convert_with_links(rpc_node)
+        elif rpc_node.maintenance and patch == remove_inst_uuid_patch:
             LOG.debug('Removing instance uuid %(instance)s from node %(node)s',
                       {'instance': rpc_node.instance_uuid,
                        'node': rpc_node.uuid})
+        # Check if node is transitioning state, although nodes in some states
+        # can be updated.
         elif (rpc_node.target_provision_state and rpc_node.provision_state
               not in ir_states.UPDATE_ALLOWED_STATES):
             msg = _("Node %s can not be updated while a state transition "

ironic/conductor/manager.py

@@ -805,8 +805,9 @@ class ConductorManager(periodic_task.PeriodicTasks):
             # NOTE(deva): there is no need to unset conductor_affinity
             # because it is a reference to the most recent conductor which
             # deployed a node, and does not limit any future actions.
-            # But we do need to clear the instance_info
+            # But we do need to clear the instance-related fields.
             node.instance_info = {}
+            node.instance_uuid = None
             driver_internal_info = node.driver_internal_info
             driver_internal_info.pop('instance', None)
             node.driver_internal_info = driver_internal_info

ironic/tests/api/v1/test_nodes.py

@@ -1010,20 +1010,23 @@ class TestPatch(test_api_base.FunctionalTest):
         self.assertEqual(400, response.status_code)
         self.assertTrue(response.json['error_message'])
 
-    def test_remove_instance_uuid_cleaning(self):
-        node = obj_utils.create_test_node(
-            self.context,
-            uuid=uuidutils.generate_uuid(),
-            provision_state=states.CLEANING,
-            target_provision_state=states.AVAILABLE)
-        self.mock_update_node.return_value = node
-        response = self.patch_json('/nodes/%s' % node.uuid,
-                                   [{'op': 'remove',
-                                     'path': '/instance_uuid'}])
-        self.assertEqual('application/json', response.content_type)
-        self.assertEqual(200, response.status_code)
-        self.mock_update_node.assert_called_once_with(
-            mock.ANY, mock.ANY, 'test-topic')
+    def test_remove_instance_uuid_clean_backward_compat(self):
+        for state in (states.CLEANING, states.CLEANWAIT):
+            node = obj_utils.create_test_node(
+                self.context,
+                uuid=uuidutils.generate_uuid(),
+                provision_state=state,
+                target_provision_state=states.AVAILABLE)
+            self.mock_update_node.return_value = node
+            response = self.patch_json('/nodes/%s' % node.uuid,
+                                       [{'op': 'remove',
+                                         'path': '/instance_uuid'}])
+            self.assertEqual('application/json', response.content_type)
+            self.assertEqual(200, response.status_code)
+            # NOTE(lucasagomes): instance_uuid is already removed as part of
+            # node's tear down, assert update has not been called. This test
+            # should be removed in the next cycle (Mitaka).
+            self.assertFalse(self.mock_update_node.called)
 
     def test_add_state_in_cleaning(self):
         node = obj_utils.create_test_node(

ironic/tests/conductor/test_manager.py

@@ -1374,6 +1374,7 @@ class DoNodeDeployTearDownTestCase(_ServiceSetUpMixin,
         node = obj_utils.create_test_node(
             self.context, driver='fake', provision_state=states.DELETING,
             target_provision_state=states.AVAILABLE,
+            instance_uuid=uuidutils.generate_uuid(),
             instance_info={'foo': 'bar'},
             driver_internal_info={'is_whole_disk_image': False,
                                   'instance': {'ephemeral_gb': 10}})
@@ -1386,6 +1387,7 @@ class DoNodeDeployTearDownTestCase(_ServiceSetUpMixin,
         self.assertEqual(states.CLEANING, node.provision_state)
         self.assertEqual(states.AVAILABLE, node.target_provision_state)
         self.assertIsNone(node.last_error)
+        self.assertIsNone(node.instance_uuid)
         self.assertEqual({}, node.instance_info)
         self.assertNotIn('instance', node.driver_internal_info)
         mock_tear_down.assert_called_once_with(mock.ANY)