Handle file delete races in image cache

In at least one case (see bug report) files have been deleted between
when we check that they exist and when we actually attempt the deletion.
Since we ultimately just want the file gone, we shouldn't care if we
get an error that the file does not exist and just move on.

Change-Id: Iaf71109fc6659650075ab4b4b48aec57819fb0b5
Closes-bug: #1229823

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.1,>=1.5.1 # BSD