diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py index ad7b95fc2b..981e47128f 100644 --- a/ironic/api/controllers/v1/node.py +++ b/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 " diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py index b289754766..a3abe3f80b 100644 --- a/ironic/conductor/manager.py +++ b/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 diff --git a/ironic/tests/api/v1/test_nodes.py b/ironic/tests/api/v1/test_nodes.py index 15a70f2761..b4f1273d58 100644 --- a/ironic/tests/api/v1/test_nodes.py +++ b/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( diff --git a/ironic/tests/conductor/test_manager.py b/ironic/tests/conductor/test_manager.py index f7b978f10e..eb09f66b2e 100644 --- a/ironic/tests/conductor/test_manager.py +++ b/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)