Merge "Cleanup import status information after busting a lock" into stable/ussuri

3 files changed, 96 insertions, 11 deletions

diff --git a/glance/api/v2/images.py b/glance/api/v2/images.py
index c73c7ee17..3b4ea9a50 100644
--- a/glance/api/v2/images.py
+++ b/glance/api/v2/images.py
@@ -169,7 +169,7 @@ class ImagesController(object):
         if not task or (task.status in bustable_states and age >= expiry):
             self._bust_import_lock(admin_image_repo, admin_task_repo,
                                    image, task, other_task)
-            return
+            return task
 
         if task.status in bustable_states:
             LOG.warning('Image %(image)s has active import task %(task)s in '
@@ -184,6 +184,30 @@ class ImagesController(object):
                       '%(status)s and does not qualify for expiry.')
         raise exception.Conflict('Image has active task')
 
+    def _cleanup_stale_task_progress(self, image_repo, image, task):
+        """Cleanup stale in-progress information from a previous task.
+
+        If we stole the lock from another task, we should try to clean up
+        the in-progress status information from that task while we have
+        the lock.
+        """
+        stores = task.task_input.get('backend', [])
+        keys = ['os_glance_importing_to_stores', 'os_glance_failed_import']
+        changed = set()
+        for store in stores:
+            for key in keys:
+                values = image.extra_properties.get(key, '').split(',')
+                if store in values:
+                    values.remove(store)
+                    changed.add(key)
+                image.extra_properties[key] = ','.join(values)
+        if changed:
+            image_repo.save(image)
+            LOG.debug('Image %(image)s had stale import progress info '
+                      '%(keys)s from task %(task)s which was cleaned up',
+                      {'image': image.image_id, 'task': task.task_id,
+                       'keys': ','.join(changed)})
+
     @utils.mutating
     def import_image(self, req, image_id, body):
         image_repo = self.gateway.get_repo(req.context)
@@ -193,6 +217,7 @@ class ImagesController(object):
         import_method = body.get('method').get('name')
         uri = body.get('method').get('uri')
         all_stores_must_succeed = body.get('all_stores_must_succeed', True)
+        stole_lock_from_task = None
 
         try:
             image = image_repo.get(image_id)
@@ -225,7 +250,7 @@ class ImagesController(object):
             if 'os_glance_import_task' in image.extra_properties:
                 # NOTE(danms): This will raise exception.Conflict if the
                 # lock is present and valid, or return if absent or invalid.
-                self._enforce_import_lock(req, image)
+                stole_lock_from_task = self._enforce_import_lock(req, image)
 
             stores = [None]
             if CONF.enabled_backends:
@@ -297,6 +322,14 @@ class ImagesController(object):
                          "prior operation is still in progress") % image_id)
                 raise exception.Conflict(msg)
 
+            # NOTE(danms): We now have the import lock on this image. If we
+            # busted the lock above and have a reference to that task, try
+            # to clean up the import status information left over from that
+            # execution.
+            if stole_lock_from_task:
+                self._cleanup_stale_task_progress(image_repo, image,
+                                                  stole_lock_from_task)
+
             task_repo.add(import_task)
             task_executor = executor_factory.new_task_executor(req.context)
             pool = common.get_thread_pool("tasks_eventlet_pool")
diff --git a/glance/tests/functional/v2/test_images_import_locking.py b/glance/tests/functional/v2/test_images_import_locking.py
index 95db792c9..11848022d 100644
--- a/glance/tests/functional/v2/test_images_import_locking.py
+++ b/glance/tests/functional/v2/test_images_import_locking.py
@@ -210,10 +210,8 @@ class TestImageImportLocking(functional.SynchronousAPIBase):
 
         # After completion, we expect store1 (original) and store3 (new)
         # and that the other task is still stuck importing
-        # FIXME(danms): The stuck importing state needs fixing
         image = self.api_get('/v2/images/%s' % image_id).json
         self.assertEqual('store1,store3', image['stores'])
-        self.assertEqual('store2', image['os_glance_importing_to_stores'])
         self.assertEqual('', image['os_glance_failed_import'])
 
         # Free up the stalled task and give eventlet time to let it
@@ -227,12 +225,7 @@ class TestImageImportLocking(functional.SynchronousAPIBase):
         # terminal state that we expect.
         image = self.api_get('/v2/images/%s' % image_id).json
         self.assertEqual('', image.get('os_glance_import_task', ''))
-        # FIXME(danms): With the strict import lock checking in
-        # ImportActionWrapper, we lose the ability to update
-        # importing_to_stores after our lock has been stolen. We
-        # should probably do something about that in the lock-busting
-        # code. We would expect this in that case:
-        # self.assertEqual('', image['os_glance_importing_to_stores'])
+        self.assertEqual('', image['os_glance_importing_to_stores'])
         self.assertEqual('', image['os_glance_failed_import'])
         self.assertEqual('store1,store3', image['stores'])
 
diff --git a/glance/tests/unit/v2/test_images_resource.py b/glance/tests/unit/v2/test_images_resource.py
index 1d6d125a0..6cf6c66a7 100644
--- a/glance/tests/unit/v2/test_images_resource.py
+++ b/glance/tests/unit/v2/test_images_resource.py
@@ -3018,20 +3018,31 @@ class TestImagesController(base.IsolatedUnitTest):
         mock_spi.assert_called_once_with(image.id, 'os_glance_import_task',
                                          'mytask')
 
+    @mock.patch.object(glance.api.authorization.ImageRepoProxy, 'save')
     @mock.patch('glance.db.simple.api.image_set_property_atomic')
     @mock.patch('glance.db.simple.api.image_delete_property_atomic')
     @mock.patch.object(glance.api.authorization.TaskFactoryProxy, 'new_task')
     @mock.patch.object(glance.api.authorization.ImageRepoProxy, 'get')
     def test_image_import_locked_by_bustable_task(self, mock_get, mock_nt,
                                                   mock_dpi, mock_spi,
+                                                  mock_save,
                                                   task_status='processing'):
+        if task_status == 'processing':
+            # NOTE(danms): Only set task_input on one of the tested
+            # states to make sure we don't choke on a task without
+            # some of the data set yet.
+            task_input = {'backend': ['store2']}
+        else:
+            task_input = {}
         task = test_tasks_resource._db_fixture(
             test_tasks_resource.UUID1,
-            status=task_status)
+            status=task_status,
+            input=task_input)
         self.db.task_create(None, task)
         image = FakeImage(status='uploading')
         # Image is locked by a task in 'processing' state
         image.extra_properties['os_glance_import_task'] = task['id']
+        image.extra_properties['os_glance_importing_to_stores'] = 'store2'
         mock_get.return_value = image
 
         request = unit_test_utils.get_fake_request(tenant=TENANT1)
@@ -3062,6 +3073,14 @@ class TestImagesController(base.IsolatedUnitTest):
         mock_spi.assert_called_once_with(image.id, 'os_glance_import_task',
                                          'mytask')
 
+        # If we stored task_input with information about the stores
+        # and thus triggered the cleanup code, make sure that cleanup
+        # happened here.
+        if task_status == 'processing':
+            self.assertNotIn('store2',
+                             image.extra_properties[
+                                 'os_glance_importing_to_stores'])
+
     def test_image_import_locked_by_bustable_terminal_task_failure(self):
         # Make sure we don't fail with a task status transition error
         self.test_image_import_locked_by_bustable_task(task_status='failure')
@@ -3070,6 +3089,46 @@ class TestImagesController(base.IsolatedUnitTest):
         # Make sure we don't fail with a task status transition error
         self.test_image_import_locked_by_bustable_task(task_status='success')
 
+    def test_cleanup_stale_task_progress(self):
+        img_repo = mock.MagicMock()
+        image = mock.MagicMock()
+        task = mock.MagicMock()
+
+        # No backend info from the old task, means no action
+        task.task_input = {}
+        image.extra_properties = {}
+        self.controller._cleanup_stale_task_progress(img_repo, image, task)
+        img_repo.save.assert_not_called()
+
+        # If we have info but no stores, no action
+        task.task_input = {'backend': []}
+        self.controller._cleanup_stale_task_progress(img_repo, image, task)
+        img_repo.save.assert_not_called()
+
+        # If task had stores, but image does not have those stores in
+        # the lists, no action
+        task.task_input = {'backend': ['store1', 'store2']}
+        self.controller._cleanup_stale_task_progress(img_repo, image, task)
+        img_repo.save.assert_not_called()
+
+        # If the image has stores in the lists, but not the ones we care
+        # about, make sure they are not disturbed
+        image.extra_properties = {'os_glance_failed_import': 'store3'}
+        self.controller._cleanup_stale_task_progress(img_repo, image, task)
+        img_repo.save.assert_not_called()
+
+        # Only if the image has stores that relate to our old task should
+        # take action, and only on those stores.
+        image.extra_properties = {
+            'os_glance_importing_to_stores': 'foo,store1,bar',
+            'os_glance_failed_import': 'foo,store2,bar',
+        }
+        self.controller._cleanup_stale_task_progress(img_repo, image, task)
+        img_repo.save.assert_called_once_with(image)
+        self.assertEqual({'os_glance_importing_to_stores': 'foo,bar',
+                          'os_glance_failed_import': 'foo,bar'},
+                         image.extra_properties)
+
     def test_bust_import_lock_race_to_delete(self):
         image_repo = mock.MagicMock()
         task_repo = mock.MagicMock()