From 2b3ce07c32624f9987df87520c5ff8d538c8e5fd Mon Sep 17 00:00:00 2001
From: Erno Kuvaja <jokke@usr.fi>
Date: Thu, 30 Jul 2020 17:20:36 +0100
Subject: [PATCH] Fix active image without data

Fixes image import resulting active image without any data
when the image happens to be deleted from staging before
_DeleteFromFS task is ran which is supposed to cleanup.

Conflict: Correct import order used in test.
Change-Id: I6256a25d1907ec15fbdde560fc8eae37f3067213
Closes-Bug: #1889640
(cherry picked from commit b985e44f795340edfceed3b46abf9b4b28907cbb)
---
 glance/async_/flows/api_image_import.py       |  8 ++-
 .../async_/flows/test_api_image_import.py     | 53 +++++++++++++++++++
 .../notes/fix_1889640-95d543629d7dadce.yaml   |  6 +++
 3 files changed, 66 insertions(+), 1 deletion(-)
 create mode 100644 releasenotes/notes/fix_1889640-95d543629d7dadce.yaml

diff --git a/glance/async_/flows/api_image_import.py b/glance/async_/flows/api_image_import.py
index 2828ecdb15..1458522b06 100644
--- a/glance/async_/flows/api_image_import.py
+++ b/glance/async_/flows/api_image_import.py
@@ -90,7 +90,13 @@ class _DeleteFromFS(task.Task):
         :param file_path: path to the file being deleted
         """
         if CONF.enabled_backends:
-            store_api.delete(file_path, 'os_glance_staging_store')
+            try:
+                store_api.delete(file_path, 'os_glance_staging_store')
+            except store_api.exceptions.NotFound as e:
+                LOG.error(_("After upload to backend, deletion of staged "
+                            "image data from %(fn)s has failed because "
+                            "%(em)s"), {'fn': file_path,
+                                        'em': e.message})
         else:
             # TODO(abhishekk): After removal of backend module from
             # glance_store need to change this to use multi_backend
diff --git a/glance/tests/unit/async_/flows/test_api_image_import.py b/glance/tests/unit/async_/flows/test_api_image_import.py
index 7e76e8fbcb..7dbf1c8c16 100644
--- a/glance/tests/unit/async_/flows/test_api_image_import.py
+++ b/glance/tests/unit/async_/flows/test_api_image_import.py
@@ -15,6 +15,7 @@
 
 from unittest import mock
 
+from glance_store import exceptions as store_exceptions
 from oslo_config import cfg
 
 import glance.async_.flows.api_image_import as import_flow
@@ -166,3 +167,55 @@ class TestImportToStoreTask(test_utils.BaseTestCase):
                              "store1")
         except cursive_exception.SignatureVerificationError:
             self.fail("Exception shouldn't be raised")
+
+
+class TestDeleteFromFS(test_utils.BaseTestCase):
+    def test_delete_with_backends_deletes(self):
+        task = import_flow._DeleteFromFS(TASK_ID1, TASK_TYPE)
+        self.config(enabled_backends='file:foo')
+        with mock.patch.object(import_flow.store_api, 'delete') as mock_del:
+            task.execute(mock.sentinel.path)
+            mock_del.assert_called_once_with(
+                mock.sentinel.path,
+                'os_glance_staging_store')
+
+    def test_delete_with_backends_delete_fails(self):
+        self.config(enabled_backends='file:foo')
+        task = import_flow._DeleteFromFS(TASK_ID1, TASK_TYPE)
+        with mock.patch.object(import_flow.store_api, 'delete') as mock_del:
+            mock_del.side_effect = store_exceptions.NotFound(image=IMAGE_ID1,
+                                                             message='Testing')
+            # If we didn't swallow this we would explode here
+            task.execute(mock.sentinel.path)
+            mock_del.assert_called_once_with(
+                mock.sentinel.path,
+                'os_glance_staging_store')
+
+            # Raise something unexpected and make sure it bubbles up
+            mock_del.side_effect = RuntimeError
+            self.assertRaises(RuntimeError,
+                              task.execute, mock.sentinel.path)
+
+    @mock.patch('os.path.exists')
+    @mock.patch('os.unlink')
+    def test_delete_without_backends_exists(self, mock_unlink, mock_exists):
+        mock_exists.return_value = True
+        task = import_flow._DeleteFromFS(TASK_ID1, TASK_TYPE)
+        task.execute('1234567foo')
+        # FIXME(danms): I have no idea why the code arbitrarily snips
+        # the first seven characters from the path. Need a comment or
+        # *something*.
+        mock_unlink.assert_called_once_with('foo')
+
+        mock_unlink.reset_mock()
+        mock_unlink.side_effect = OSError(123, 'failed')
+        # Make sure we swallow the OSError and don't explode
+        task.execute('1234567foo')
+
+    @mock.patch('os.path.exists')
+    @mock.patch('os.unlink')
+    def test_delete_without_backends_missing(self, mock_unlink, mock_exists):
+        mock_exists.return_value = False
+        task = import_flow._DeleteFromFS(TASK_ID1, TASK_TYPE)
+        task.execute('foo')
+        mock_unlink.assert_not_called()
diff --git a/releasenotes/notes/fix_1889640-95d543629d7dadce.yaml b/releasenotes/notes/fix_1889640-95d543629d7dadce.yaml
new file mode 100644
index 0000000000..03e513d2f1
--- /dev/null
+++ b/releasenotes/notes/fix_1889640-95d543629d7dadce.yaml
@@ -0,0 +1,6 @@
+---
+fixes:
+  - |
+    Bug 1889640_: Image import might result 'active' image with no data.
+
+    .. _1889640: https://bugs.launchpad.net/glance/+bug/1889640