Merge "Erase swift inventory entry on node deletion"

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):

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.")

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})

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(
+        raise exception.NodeInventoryNotFound(node=node.uuid)
-            (_("Cannot obtain node inventory because it was not stored")))
     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_uuid: UUID of the Ironic node that the data came from
-    :param node_id: ID of the Ironic node that the data came from
+    :returns: dictionary with ``inventory`` and ``plugin_data`` fields
-    :returns: name of the Swift object that the data is stored in
     """
     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',
+    inv_obj = swift_object_name + '-inventory'
-                                          container)
+    plug_obj = swift_object_name + '-plugin'
-    plugin_data = swift_api.get_object(swift_object_name + '-plugin',
+    try:
-                                       container)
+        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}

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):

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):

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)