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

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

@@ -806,8 +806,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

@@ -1375,6 +1375,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}})
@@ -1387,6 +1388,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)