Merge "Remove empty directories after a revert job"

swift/common/utils.py

@@ -2935,6 +2935,18 @@ def remove_file(path):
         pass
 
 
+def remove_directory(path):
+    """Wrapper for os.rmdir, ENOENT and ENOTEMPTY are ignored
+
+    :param path: first and only argument passed to os.rmdir
+    """
+    try:
+        os.rmdir(path)
+    except OSError as e:
+        if e.errno not in (errno.ENOENT, errno.ENOTEMPTY):
+            raise
+
+
 def audit_location_generator(devices, datadir, suffix='',
                              mount_check=True, logger=None):
     """

swift/obj/diskfile.py

@@ -65,7 +65,7 @@ from swift.common.utils import mkdirs, Timestamp, \
     config_true_value, listdir, split_path, remove_file, \
     get_md5_socket, F_SETPIPE_SZ, decode_timestamps, encode_timestamps, \
     MD5_OF_EMPTY_STRING, link_fd_to_path, o_tmpfile_supported, \
-    O_TMPFILE, makedirs_count, replace_partition_in_path
+    O_TMPFILE, makedirs_count, replace_partition_in_path, remove_directory
 from swift.common.splice import splice, tee
 from swift.common.exceptions import DiskFileQuarantined, DiskFileNotExist, \
     DiskFileCollision, DiskFileNoSpace, DiskFileDeviceUnavailable, \
@@ -3169,8 +3169,8 @@ class ECDiskFile(BaseDiskFile):
         remove a tombstone or fragment from a handoff node after
         reverting it to its primary node.
 
-        The hash will be invalidated, and if empty or invalid the
+        The hash will be invalidated, and if empty the hsh_path will
-        hsh_path will be removed on next cleanup_ondisk_files.
+        be removed immediately.
 
         :param timestamp: the object timestamp, an instance of
                           :class:`~swift.common.utils.Timestamp`
@@ -3189,6 +3189,7 @@ class ECDiskFile(BaseDiskFile):
             purge_file = self.manager.make_on_disk_filename(
                 timestamp, ext='.data', frag_index=frag_index, durable=True)
             remove_file(os.path.join(self._datadir, purge_file))
+            remove_directory(self._datadir)
         self.manager.invalidate_hash(dirname(self._datadir))
 
 

swift/obj/reconstructor.py

@@ -34,7 +34,7 @@ from swift.common.utils import (
     dump_recon_cache, mkdirs, config_true_value,
     GreenAsyncPile, Timestamp, remove_file,
     load_recon_cache, parse_override_options, distribute_evenly,
-    PrefixLoggerAdapter)
+    PrefixLoggerAdapter, remove_directory)
 from swift.common.header_key_dict import HeaderKeyDict
 from swift.common.bufferedhttp import http_connect
 from swift.common.daemon import Daemon
@@ -745,6 +745,7 @@ class ObjectReconstructor(Daemon):
         :param frag_index: (int) the fragment index of data files to be deleted
         """
         df_mgr = self._df_router[job['policy']]
+        suffixes_to_delete = set()
         for object_hash, timestamps in objects.items():
             try:
                 df = df_mgr.get_diskfile_from_hash(
@@ -756,6 +757,10 @@ class ObjectReconstructor(Daemon):
                 self.logger.exception(
                     'Unable to purge DiskFile (%r %r %r)',
                     object_hash, timestamps['ts_data'], frag_index)
+            suffixes_to_delete.add(object_hash[-3:])
+
+        for suffix in suffixes_to_delete:
+            remove_directory(os.path.join(job['path'], suffix))
 
     def process_job(self, job):
         """

test/unit/common/test_utils.py

@@ -2592,6 +2592,30 @@ log_name = %(yarr)s'''
             self.assertIsNone(utils.remove_file(file_name))
             self.assertFalse(os.path.exists(file_name))
 
+    def test_remove_directory(self):
+        with temptree([]) as t:
+            dir_name = os.path.join(t, 'subdir')
+
+            os.mkdir(dir_name)
+            self.assertTrue(os.path.isdir(dir_name))
+            self.assertIsNone(utils.remove_directory(dir_name))
+            self.assertFalse(os.path.exists(dir_name))
+
+            # assert no raise only if it does not exist, or is not empty
+            self.assertEqual(os.path.exists(dir_name), False)
+            self.assertIsNone(utils.remove_directory(dir_name))
+
+            _m_rmdir = mock.Mock(
+                side_effect=OSError(errno.ENOTEMPTY,
+                                    os.strerror(errno.ENOTEMPTY)))
+            with mock.patch('swift.common.utils.os.rmdir', _m_rmdir):
+                self.assertIsNone(utils.remove_directory(dir_name))
+
+            _m_rmdir = mock.Mock(
+                side_effect=OSError(errno.EPERM, os.strerror(errno.EPERM)))
+            with mock.patch('swift.common.utils.os.rmdir', _m_rmdir):
+                self.assertRaises(OSError, utils.remove_directory, dir_name)
+
     def test_human_readable(self):
         self.assertEqual(utils.human_readable(0), '0')
         self.assertEqual(utils.human_readable(1), '1')

test/unit/obj/test_diskfile.py

@@ -5456,7 +5456,7 @@ class TestECDiskFile(DiskFileMixin, unittest.TestCase):
             ts.internal + '#0#d.data',
         ])
         df.purge(ts, frag_index)
-        self.assertFalse(os.listdir(df._datadir))
+        self.assertFalse(os.path.exists(df._datadir))
 
     def test_purge_last_fragment_index_legacy_durable(self):
         # a legacy durable file doesn't get purged in case another fragment is
@@ -5518,7 +5518,7 @@ class TestECDiskFile(DiskFileMixin, unittest.TestCase):
             ts.internal + '.ts',
         ])
         df.purge(ts, 3)
-        self.assertEqual(sorted(os.listdir(df._datadir)), [])
+        self.assertFalse(os.path.exists(df._datadir))
 
     def test_purge_without_frag(self):
         ts = self.ts()
@@ -5557,8 +5557,8 @@ class TestECDiskFile(DiskFileMixin, unittest.TestCase):
         os.makedirs(df._datadir)
         self.assertEqual(sorted(os.listdir(df._datadir)), [])
         df.purge(self.ts(), 6)
-        # no effect
+        # the directory was empty and has been removed
-        self.assertEqual(sorted(os.listdir(df._datadir)), [])
+        self.assertFalse(os.path.exists(df._datadir))
 
     def _do_test_open_most_recent_durable(self, legacy_durable):
         policy = POLICIES.default

test/unit/obj/test_reconstructor.py

@@ -4061,15 +4061,9 @@ class TestObjectReconstructor(BaseTestObjectReconstructor):
         ], [
             (r['ip'], r['path']) for r in request_log.requests
         ])
-        # hashpath is still there, but all files have been purged
+        # hashpath has been removed
-        files = os.listdir(df._datadir)
+        self.assertFalse(os.path.exists(df._datadir))
-        self.assertFalse(files)
 
-        # and more to the point, the next suffix recalc will clean it up
-        df_mgr = self.reconstructor._df_router[self.policy]
-        df_mgr.get_hashes(self.local_dev['device'], str(partition), [],
-                          self.policy)
-        self.assertFalse(os.access(df._datadir, os.F_OK))
         self.assertEqual(self.reconstructor.handoffs_remaining, 0)
 
     def test_process_job_revert_cleanup_tombstone(self):