diff options
author | Zuul <zuul@review.opendev.org> | 2023-02-14 18:39:57 +0000 |
---|---|---|
committer | Gerrit Code Review <review@openstack.org> | 2023-02-14 18:39:57 +0000 |
commit | e8888aa9aef0380ef25ebf812d258969c5166d04 (patch) | |
tree | df24b942188dd842949e7ebaf26b3ad45623c0ef /ironic | |
parent | eab0f73ff3c590a35d383cc1e5e788128141b9b9 (diff) | |
parent | bc921118b1bc154df04c744612fafadb1081941a (diff) | |
download | ironic-e8888aa9aef0380ef25ebf812d258969c5166d04.tar.gz |
Merge "Erase swift inventory entry on node deletion"
Diffstat (limited to 'ironic')
-rw-r--r-- | ironic/api/controllers/v1/node.py | 3 | ||||
-rw-r--r-- | ironic/common/exception.py | 7 | ||||
-rw-r--r-- | ironic/conductor/manager.py | 21 | ||||
-rw-r--r-- | ironic/drivers/modules/inspect_utils.py | 103 | ||||
-rw-r--r-- | ironic/tests/unit/api/controllers/v1/test_node.py | 12 | ||||
-rw-r--r-- | ironic/tests/unit/conductor/test_manager.py | 34 | ||||
-rw-r--r-- | ironic/tests/unit/drivers/modules/test_inspect_utils.py | 110 |
7 files changed, 275 insertions, 15 deletions
diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py index 9a117a95f..022b5f66a 100644 --- a/ironic/api/controllers/v1/node.py +++ b/ironic/api/controllers/v1/node.py @@ -1964,7 +1964,8 @@ class NodeInventoryController(rest.RestController): """ node = api_utils.check_node_policy_and_retrieve( 'baremetal:node:inventory:get', self.node_ident) - return inspect_utils.get_introspection_data(node, api.request.context) + return inspect_utils.get_introspection_data(node, + api.request.context) class NodesController(rest.RestController): diff --git a/ironic/common/exception.py b/ironic/common/exception.py index 6714b7d4a..731d31486 100644 --- a/ironic/common/exception.py +++ b/ironic/common/exception.py @@ -829,7 +829,7 @@ class NodeHistoryNotFound(NotFound): class NodeInventoryNotFound(NotFound): - _msg_fmt = _("Node inventory record %(inventory)s could not be found.") + _msg_fmt = _("Node inventory record for node %(node)s could not be found.") class IncorrectConfiguration(IronicException): @@ -865,3 +865,8 @@ class ConcurrentActionLimit(IronicException): "The concurrent action limit for %(task_type)s " "has been reached. Please contact your administrator " "and try again later.") + + +class SwiftObjectStillExists(IronicException): + _msg_fmt = _("Clean up failed for swift object %(obj)s during deletion" + " of node %(node)s.") diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py index cf4988958..a5817cf2e 100644 --- a/ironic/conductor/manager.py +++ b/ironic/conductor/manager.py @@ -73,6 +73,7 @@ from ironic.conf import CONF from ironic.drivers import base as drivers_base from ironic.drivers.modules import deploy_utils from ironic.drivers.modules import image_cache +from ironic.drivers.modules import inspect_utils from ironic import objects from ironic.objects import base as objects_base from ironic.objects import fields @@ -2023,6 +2024,26 @@ class ConductorManager(base_manager.BaseConductorManager): node.console_enabled = False notify_utils.emit_console_notification( task, 'console_set', fields.NotificationStatus.END) + # Destroy Swift Inventory entries for this node + try: + inspect_utils.clean_up_swift_entries(task) + except exception.SwiftObjectStillExists as e: + if node.maintenance: + # Maintenance -> Allow orphaning + LOG.warning('Swift object orphaned during destruction of ' + 'node %(node)s: %(e)s', + {'node': node.uuid, 'e': e}) + else: + LOG.error('Swift object cannot be orphaned during ' + 'destruction of node %(node)s: %(e)s', + {'node': node.uuid, 'e': e}) + raise + except Exception as err: + LOG.error('Failed to delete Swift entries related ' + 'to the node %(node)s: %(err)s.', + {'node': node.uuid, 'err': err}) + raise + node.destroy() LOG.info('Successfully deleted node %(node)s.', {'node': node.uuid}) diff --git a/ironic/drivers/modules/inspect_utils.py b/ironic/drivers/modules/inspect_utils.py index 1340d0d7d..0089302c1 100644 --- a/ironic/drivers/modules/inspect_utils.py +++ b/ironic/drivers/modules/inspect_utils.py @@ -15,9 +15,9 @@ from oslo_log import log as logging from oslo_utils import netutils +import swiftclient.exceptions from ironic.common import exception -from ironic.common.i18n import _ from ironic.common import swift from ironic.conf import CONF from ironic import objects @@ -58,7 +58,61 @@ def create_ports_if_not_exist(task, macs): "for node %(node)s", {'address': mac, 'node': node.uuid}) +def clean_up_swift_entries(task): + """Delete swift entries containing introspection data. + + Delete swift entries related to the node in task.node containing + introspection data. The entries are + ``inspector_data-<task.node.uuid>-inventory`` for hardware inventory and + similar for ``-plugin`` containing the rest of the introspection data. + + :param task: A TaskManager instance. + """ + if CONF.inventory.data_backend != 'swift': + return + swift_api = swift.SwiftAPI() + swift_object_name = '%s-%s' % (_OBJECT_NAME_PREFIX, task.node.uuid) + container = CONF.inventory.swift_data_container + inventory_obj_name = swift_object_name + '-inventory' + plugin_obj_name = swift_object_name + '-plugin' + try: + swift_api.delete_object(inventory_obj_name, container) + except swiftclient.exceptions.ClientException as e: + if e.http_status == 404: + # 404 -> entry did not exist - acceptable. + pass + else: + LOG.error("Object %(obj)s related to node %(node)s " + "failed to be deleted with expection: %(e)s", + {'obj': inventory_obj_name, 'node': task.node.uuid, + 'e': e}) + raise exception.SwiftObjectStillExists(obj=inventory_obj_name, + node=task.node.uuid) + try: + swift_api.delete_object(plugin_obj_name, container) + except swiftclient.exceptions.ClientException as e: + if e.http_status == 404: + # 404 -> entry did not exist - acceptable. + pass + else: + LOG.error("Object %(obj)s related to node %(node)s " + "failed to be deleted with exception: %(e)s", + {'obj': plugin_obj_name, 'node': task.node.uuid, + 'e': e}) + raise exception.SwiftObjectStillExists(obj=plugin_obj_name, + node=task.node.uuid) + + def store_introspection_data(node, introspection_data, context): + """Store introspection data. + + Store the introspection data for a node. Either to database + or swift as configured. + + :param node: the Ironic node that the introspection data is about + :param introspection_data: the data to store + :param context: an admin context + """ # If store_data == 'none', do not store the data store_data = CONF.inventory.data_backend if store_data == 'none': @@ -92,16 +146,28 @@ def _node_inventory_convert(node_inventory): def get_introspection_data(node, context): + """Get introspection data. + + Retrieve the introspection data for a node. Either from database + or swift as configured. + + :param node_id: the Ironic node that the required data is about + :param context: an admin context + :returns: dictionary with ``inventory`` and ``plugin_data`` fields + """ store_data = CONF.inventory.data_backend if store_data == 'none': - raise exception.NotFound( - (_("Cannot obtain node inventory because it was not stored"))) + raise exception.NodeInventoryNotFound(node=node.uuid) if store_data == 'database': node_inventory = objects.NodeInventory.get_by_node_id( context, node.id) return _node_inventory_convert(node_inventory) if store_data == 'swift': - return _get_introspection_data_from_swift(node.uuid) + try: + node_inventory = _get_introspection_data_from_swift(node.uuid) + except exception.SwiftObjectNotFoundError: + raise exception.NodeInventoryNotFound(node=node.uuid) + return node_inventory def _store_introspection_data_in_swift(node_uuid, inventory_data, plugin_data): @@ -124,17 +190,30 @@ def _store_introspection_data_in_swift(node_uuid, inventory_data, plugin_data): def _get_introspection_data_from_swift(node_uuid): - """Uploads introspection data to Swift. + """Get introspection data from Swift. - :param data: data to store in Swift - :param node_id: ID of the Ironic node that the data came from - :returns: name of the Swift object that the data is stored in + :param node_uuid: UUID of the Ironic node that the data came from + :returns: dictionary with ``inventory`` and ``plugin_data`` fields """ swift_api = swift.SwiftAPI() swift_object_name = '%s-%s' % (_OBJECT_NAME_PREFIX, node_uuid) container = CONF.inventory.swift_data_container - inventory_data = swift_api.get_object(swift_object_name + '-inventory', - container) - plugin_data = swift_api.get_object(swift_object_name + '-plugin', - container) + inv_obj = swift_object_name + '-inventory' + plug_obj = swift_object_name + '-plugin' + try: + inventory_data = swift_api.get_object(inv_obj, container) + except exception.SwiftOperationError: + LOG.error("Failed to retrieve object %(obj)s from swift", + {'obj': inv_obj}) + raise exception.SwiftObjectNotFoundError(obj=inv_obj, + container=container, + operation='get') + try: + plugin_data = swift_api.get_object(plug_obj, container) + except exception.SwiftOperationError: + LOG.error("Failed to retrieve object %(obj)s from swift", + {'obj': plug_obj}) + raise exception.SwiftObjectNotFoundError(obj=plug_obj, + container=container, + operation='get') return {"inventory": inventory_data, "plugin_data": plugin_data} diff --git a/ironic/tests/unit/api/controllers/v1/test_node.py b/ironic/tests/unit/api/controllers/v1/test_node.py index e1ef916b9..19ef1235c 100644 --- a/ironic/tests/unit/api/controllers/v1/test_node.py +++ b/ironic/tests/unit/api/controllers/v1/test_node.py @@ -7958,6 +7958,18 @@ class TestNodeInventory(test_api_base.BaseApiTest): self.assertEqual({'inventory': self.fake_inventory_data, 'plugin_data': self.fake_plugin_data}, ret) + @mock.patch.object(inspect_utils, 'get_introspection_data', + autospec=True) + def test_get_inventory_exception(self, mock_get_data): + CONF.set_override('data_backend', 'database', + group='inventory') + mock_get_data.side_effect = [ + exception.NodeInventoryNotFound] + ret = self.get_json('/nodes/%s/inventory' % self.node.uuid, + headers={api_base.Version.string: self.version}, + expect_errors=True) + self.assertEqual(http_client.NOT_FOUND, ret.status_int) + @mock.patch.object(inspect_utils, '_get_introspection_data_from_swift', autospec=True) def test_get_inventory_swift(self, mock_get_data): diff --git a/ironic/tests/unit/conductor/test_manager.py b/ironic/tests/unit/conductor/test_manager.py index fd206e36d..b63907e53 100644 --- a/ironic/tests/unit/conductor/test_manager.py +++ b/ironic/tests/unit/conductor/test_manager.py @@ -54,6 +54,7 @@ from ironic.conductor import verify from ironic.db import api as dbapi from ironic.drivers import base as drivers_base from ironic.drivers.modules import fake +from ironic.drivers.modules import inspect_utils from ironic.drivers.modules.network import flat as n_flat from ironic import objects from ironic.objects import base as obj_base @@ -3901,6 +3902,39 @@ class DestroyNodeTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): self.dbapi.get_node_by_uuid, node.uuid) + @mock.patch.object(inspect_utils, 'clean_up_swift_entries', autospec=True) + def test_inventory_in_swift_get_destroyed_after_destroying_a_node_by_uuid( + self, mock_clean_up): + node = obj_utils.create_test_node(self.context, driver='fake-hardware') + CONF.set_override('data_backend', 'swift', group='inventory') + self._start_service() + self.service.destroy_node(self.context, node.uuid) + mock_clean_up.assert_called_once_with(mock.ANY) + + @mock.patch.object(inspect_utils, 'clean_up_swift_entries', autospec=True) + def test_inventory_in_swift_not_destroyed_SwiftOSE_maintenance( + self, mock_clean_up): + node = obj_utils.create_test_node(self.context, driver='fake-hardware', + maintenance=True) + CONF.set_override('data_backend', 'swift', group='inventory') + mock_clean_up.side_effect = exception.SwiftObjectStillExists( + obj="inventory-123", node=node.uuid) + self._start_service() + self.service.destroy_node(self.context, node.uuid) + + @mock.patch.object(inspect_utils, 'clean_up_swift_entries', autospec=True) + def test_inventory_in_swift_not_destroyed_SwiftOSE_not_maintenance( + self, mock_clean_up): + node = obj_utils.create_test_node(self.context, driver='fake-hardware', + maintenance=False) + CONF.set_override('data_backend', 'swift', group='inventory') + mock_clean_up.side_effect = exception.SwiftObjectStillExists( + obj="inventory-123", node=node.uuid) + self._start_service() + self.assertRaises(exception.SwiftObjectStillExists, + self.service.destroy_node, self.context, + node.uuid) + @mgr_utils.mock_record_keepalive class CreatePortTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): diff --git a/ironic/tests/unit/drivers/modules/test_inspect_utils.py b/ironic/tests/unit/drivers/modules/test_inspect_utils.py index e12bbb743..7cb451473 100644 --- a/ironic/tests/unit/drivers/modules/test_inspect_utils.py +++ b/ironic/tests/unit/drivers/modules/test_inspect_utils.py @@ -17,6 +17,7 @@ from unittest import mock from oslo_utils import importutils +import swiftclient.exceptions from ironic.common import context as ironic_context from ironic.common import exception @@ -94,6 +95,75 @@ class InspectFunctionTestCase(db_base.DbTestCase): self.assertEqual(2, port_mock.return_value.create.call_count) +class SwiftCleanUp(db_base.DbTestCase): + + def setUp(self): + super(SwiftCleanUp, self).setUp() + self.node = obj_utils.create_test_node(self.context) + + @mock.patch.object(swift, 'SwiftAPI', autospec=True) + def test_clean_up_swift_entries(self, swift_api_mock): + CONF.set_override('data_backend', 'swift', group='inventory') + container = 'introspection_data' + CONF.set_override('swift_data_container', container, group='inventory') + swift_obj_mock = swift_api_mock.return_value + with task_manager.acquire(self.context, self.node.uuid, + shared=False) as task: + utils.clean_up_swift_entries(task) + object_name = 'inspector_data-' + str(self.node.uuid) + swift_obj_mock.delete_object.assert_has_calls([ + mock.call(object_name + '-inventory', container), + mock.call(object_name + '-plugin', container)]) + + @mock.patch.object(swift, 'SwiftAPI', autospec=True) + def test_clean_up_swift_entries_with_404_exception(self, swift_api_mock): + CONF.set_override('data_backend', 'swift', group='inventory') + container = 'introspection_data' + CONF.set_override('swift_data_container', container, group='inventory') + swift_obj_mock = swift_api_mock.return_value + with task_manager.acquire(self.context, self.node.uuid, + shared=False) as task: + swift_obj_mock.delete_object.side_effect = [ + swiftclient.exceptions.ClientException("not found", + http_status=404), + swiftclient.exceptions.ClientException("not found", + http_status=404)] + utils.clean_up_swift_entries(task) + + @mock.patch.object(swift, 'SwiftAPI', autospec=True) + def test_clean_up_swift_entries_with_fail_exception(self, swift_api_mock): + CONF.set_override('data_backend', 'swift', group='inventory') + container = 'introspection_data' + CONF.set_override('swift_data_container', container, group='inventory') + swift_obj_mock = swift_api_mock.return_value + with task_manager.acquire(self.context, self.node.uuid, + shared=False) as task: + swift_obj_mock.delete_object.side_effect = [ + swiftclient.exceptions.ClientException("failed", + http_status=417), + swiftclient.exceptions.ClientException("not found", + http_status=404)] + self.assertRaises(exception.SwiftObjectStillExists, + utils.clean_up_swift_entries, task) + + @mock.patch.object(swift, 'SwiftAPI', autospec=True) + def test_clean_up_swift_entries_with_fail_exceptions(self, swift_api_mock): + CONF.set_override('data_backend', 'swift', group='inventory') + container = 'introspection_data' + CONF.set_override('swift_data_container', container, group='inventory') + swift_obj_mock = swift_api_mock.return_value + with task_manager.acquire(self.context, self.node.uuid, + shared=False) as task: + swift_obj_mock.delete_object.side_effect = [ + swiftclient.exceptions.ClientException("failed", + http_status=417), + swiftclient.exceptions.ClientException("failed", + http_status=417)] + self.assertRaises((exception.SwiftObjectStillExists, + exception.SwiftObjectStillExists), + utils.clean_up_swift_entries, task) + + class IntrospectionDataStorageFunctionsTestCase(db_base.DbTestCase): fake_inventory_data = {"cpu": "amd"} fake_plugin_data = {"disks": [{"name": "/dev/vda"}]} @@ -163,6 +233,17 @@ class IntrospectionDataStorageFunctionsTestCase(db_base.DbTestCase): utils.get_introspection_data(self.node, fake_context) mock_convert.assert_called_once_with(fake_introspection_data) + @mock.patch.object(objects, 'NodeInventory', spec_set=True, autospec=True) + def test_get_introspection_data_db_exception(self, mock_inventory): + CONF.set_override('data_backend', 'database', + group='inventory') + fake_context = ironic_context.RequestContext() + mock_inventory.get_by_node_id.side_effect = [ + exception.NodeInventoryNotFound(self.node.uuid)] + self.assertRaises( + exception.NodeInventoryNotFound, utils.get_introspection_data, + self.node, fake_context) + @mock.patch.object(utils, '_get_introspection_data_from_swift', autospec=True) def test_get_introspection_data_swift(self, mock_get_data): @@ -175,11 +256,24 @@ class IntrospectionDataStorageFunctionsTestCase(db_base.DbTestCase): mock_get_data.assert_called_once_with( self.node.uuid) + @mock.patch.object(utils, '_get_introspection_data_from_swift', + autospec=True) + def test_get_introspection_data_swift_exception(self, mock_get_data): + CONF.set_override('data_backend', 'swift', group='inventory') + CONF.set_override( + 'swift_data_container', 'introspection_data', + group='inventory') + fake_context = ironic_context.RequestContext() + mock_get_data.side_effect = exception.SwiftObjectNotFoundError() + self.assertRaises( + exception.NodeInventoryNotFound, utils.get_introspection_data, + self.node, fake_context) + def test_get_introspection_data_nostore(self): CONF.set_override('data_backend', 'none', group='inventory') fake_context = ironic_context.RequestContext() self.assertRaises( - exception.NotFound, utils.get_introspection_data, + exception.NodeInventoryNotFound, utils.get_introspection_data, self.node, fake_context) @mock.patch.object(swift, 'SwiftAPI', autospec=True) @@ -209,3 +303,17 @@ class IntrospectionDataStorageFunctionsTestCase(db_base.DbTestCase): req_ret = {"inventory": self.fake_inventory_data, "plugin_data": self.fake_plugin_data} self.assertEqual(req_ret, ret) + + @mock.patch.object(swift, 'SwiftAPI', autospec=True) + def test__get_introspection_data_from_swift_exception(self, + swift_api_mock): + container = 'introspection_data' + CONF.set_override('swift_data_container', container, group='inventory') + swift_obj_mock = swift_api_mock.return_value + swift_obj_mock.get_object.side_effect = [ + exception.SwiftOperationError, + self.fake_plugin_data + ] + self.assertRaises(exception.SwiftObjectNotFoundError, + utils._get_introspection_data_from_swift, + self.node.uuid) |