From 2894bcd84960e4133c8bd8552878e7e2c7efb240 Mon Sep 17 00:00:00 2001 From: Lucas Alvares Gomes Date: Thu, 23 Jul 2015 13:15:47 +0100 Subject: [PATCH] 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 | 25 ++++++++++++--------- ironic/conductor/manager.py | 3 ++- ironic/tests/api/v1/test_nodes.py | 31 ++++++++++++++------------ ironic/tests/conductor/test_manager.py | 2 ++ 4 files changed, 35 insertions(+), 26 deletions(-) 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 057cfcc9b2..aa61401007 100644 --- a/ironic/conductor/manager.py +++ b/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 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 2b93042659..32d757acd0 100644 --- a/ironic/tests/conductor/test_manager.py +++ b/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)