Merge "Handle file delete races in image cache"

glance/image_cache/drivers/sqlite.py

@@ -30,6 +30,7 @@ from oslo_concurrency import lockutils
 from oslo_config import cfg
 from oslo_log import log as logging
 from oslo_utils import excutils
+from oslo_utils import fileutils
 
 from glance.common import exception
 from glance.i18n import _, _LE, _LI, _LW
@@ -260,7 +261,7 @@ class Driver(base.Driver):
         """
         files = [f for f in self.get_cache_files(self.queue_dir)]
         for file in files:
-            os.unlink(file)
+            fileutils.delete_if_exists(file)
         return len(files)
 
     def delete_queued_image(self, image_id):
@@ -270,8 +271,7 @@ class Driver(base.Driver):
         :param image_id: Image ID
         """
         path = self.get_image_filepath(image_id, 'queue')
-        if os.path.exists(path):
-            os.unlink(path)
+        fileutils.delete_if_exists(path)
 
     def clean(self, stall_time=None):
         """
@@ -331,7 +331,8 @@ class Driver(base.Driver):
 
                 # Make sure that we "pop" the image from the queue...
                 if self.is_queued(image_id):
-                    os.unlink(self.get_image_filepath(image_id, 'queue'))
+                    fileutils.delete_if_exists(
+                        self.get_image_filepath(image_id, 'queue'))
 
                 filesize = os.path.getsize(final_path)
                 now = time.time()
@@ -453,7 +454,7 @@ class Driver(base.Driver):
         Removes any invalid cache entries
         """
         for path in self.get_cache_files(self.invalid_dir):
-            os.unlink(path)
+            fileutils.delete_if_exists(path)
             LOG.info(_LI("Removed invalid cache file %s"), path)
 
     def delete_stalled_files(self, older_than):
@@ -467,7 +468,7 @@ class Driver(base.Driver):
         for path in self.get_cache_files(self.incomplete_dir):
             if os.path.getmtime(path) < older_than:
                 try:
-                    os.unlink(path)
+                    fileutils.delete_if_exists(path)
                     LOG.info(_LI("Removed stalled cache file %s"), path)
                 except Exception as e:
                     msg = (_LW("Failed to delete file %(path)s. "
@@ -503,9 +504,5 @@ class Driver(base.Driver):
 
 
 def delete_cached_file(path):
-    if os.path.exists(path):
-        LOG.debug("Deleting image cache file '%s'", path)
-        os.unlink(path)
-    else:
-        LOG.warn(_LW("Cached image file '%s' doesn't exist, unable to"
-                     " delete") % path)
+    LOG.debug("Deleting image cache file '%s'", path)
+    fileutils.delete_if_exists(path)

glance/image_cache/drivers/xattr.py

@@ -62,11 +62,12 @@ from oslo_config import cfg
 from oslo_log import log as logging
 from oslo_utils import encodeutils
 from oslo_utils import excutils
+from oslo_utils import fileutils
 import six
 import xattr
 
 from glance.common import exception
-from glance.i18n import _, _LI, _LW
+from glance.i18n import _, _LI
 from glance.image_cache.drivers import base
 
 LOG = logging.getLogger(__name__)
@@ -115,8 +116,7 @@ class Driver(base.Driver):
                                                        reason=msg)
         else:
             # Cleanup after ourselves...
-            if os.path.exists(fake_image_filepath):
-                os.unlink(fake_image_filepath)
+            fileutils.delete_if_exists(fake_image_filepath)
 
     def get_cache_size(self):
         """
@@ -221,7 +221,7 @@ class Driver(base.Driver):
         """
         files = [f for f in get_all_regular_files(self.queue_dir)]
         for file in files:
-            os.unlink(file)
+            fileutils.delete_if_exists(file)
         return len(files)
 
     def delete_queued_image(self, image_id):
@@ -231,8 +231,7 @@ class Driver(base.Driver):
         :param image_id: Image ID
         """
         path = self.get_image_filepath(image_id, 'queue')
-        if os.path.exists(path):
-            os.unlink(path)
+        fileutils.delete_if_exists(path)
 
     def get_least_recently_accessed(self):
         """
@@ -279,7 +278,8 @@ class Driver(base.Driver):
             if self.is_queued(image_id):
                 LOG.debug("Removing image '%s' from queue after "
                           "caching it.", image_id)
-                os.unlink(self.get_image_filepath(image_id, 'queue'))
+                fileutils.delete_if_exists(
+                    self.get_image_filepath(image_id, 'queue'))
 
         def rollback(e):
             set_attr('error', encodeutils.exception_to_unicode(e))
@@ -431,12 +431,8 @@ def get_all_regular_files(basepath):
 
 
 def delete_cached_file(path):
-    if os.path.exists(path):
-        LOG.debug("Deleting image cache file '%s'", path)
-        os.unlink(path)
-    else:
-        LOG.warn(_LW("Cached image file '%s' doesn't exist, unable to"
-                     " delete") % path)
+    LOG.debug("Deleting image cache file '%s'", path)
+    fileutils.delete_if_exists(path)
 
 
 def _make_namespaced_xattr_key(key, namespace='user'):

glance/tests/unit/image_cache/drivers/test_sqlite.py

@@ -0,0 +1,40 @@
+# Copyright (c) 2017 Huawei Technologies Co., Ltd.
+# All Rights Reserved.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+"""
+Tests for the sqlite image_cache driver.
+"""
+
+import os
+
+import ddt
+import mock
+
+from glance.image_cache.drivers import sqlite
+from glance.tests import utils
+
+
+@ddt.ddt
+class TestSqlite(utils.BaseTestCase):
+
+    @ddt.data(True, False)
+    def test_delete_cached_file(self, throw_not_exists):
+
+        with mock.patch.object(os, 'unlink') as mock_unlink:
+            if throw_not_exists:
+                mock_unlink.side_effect = OSError((2, 'File not found'))
+
+        # Should not raise an exception in all cases
+        sqlite.delete_cached_file('/tmp/dummy_file')

test-requirements.txt

@@ -11,6 +11,7 @@ Babel!=2.4.0,>=2.3.4 # BSD
 # Needed for testing
 bandit>=1.1.0 # Apache-2.0
 coverage!=4.4,>=4.0 # Apache-2.0
+ddt>=1.0.1 # MIT
 fixtures>=3.0.0 # Apache-2.0/BSD
 mock>=2.0 # BSD
 sphinx>=1.6.2 # BSD