From d306345ddd22c38a18de8f8e5fb9b34a1f91fd93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexandre=20L=C3=A9cuyer?= Date: Wed, 17 Oct 2018 12:11:22 +0200 Subject: [PATCH] Remove empty directories after a revert job Currently, the reconstructor will not remove empty object and suffixes directories after processing a revert job. This will only happen during its next run. This patch will attempt to remove these empty directories immediately, while we have the inodes cached. Change-Id: I5dfc145b919b70ab7dae34fb124c8a25ba77222f --- swift/common/utils.py | 12 ++++++++++++ swift/obj/diskfile.py | 7 ++++--- swift/obj/reconstructor.py | 7 ++++++- test/unit/common/test_utils.py | 24 ++++++++++++++++++++++++ test/unit/obj/test_diskfile.py | 8 ++++---- test/unit/obj/test_reconstructor.py | 10 ++-------- 6 files changed, 52 insertions(+), 16 deletions(-) diff --git a/swift/common/utils.py b/swift/common/utils.py index 57b219c36a..926f7c01c1 100644 --- a/swift/common/utils.py +++ b/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): """ diff --git a/swift/obj/diskfile.py b/swift/obj/diskfile.py index 987ec1d8d6..ff0b71d001 100644 --- a/swift/obj/diskfile.py +++ b/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 - hsh_path will be removed on next cleanup_ondisk_files. + The hash will be invalidated, and if empty the hsh_path will + 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)) diff --git a/swift/obj/reconstructor.py b/swift/obj/reconstructor.py index 2a2de8ef22..dc6793b55e 100644 --- a/swift/obj/reconstructor.py +++ b/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 @@ -741,6 +741,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( @@ -752,6 +753,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): """ diff --git a/test/unit/common/test_utils.py b/test/unit/common/test_utils.py index 6e94b20878..e9b098f7d8 100644 --- a/test/unit/common/test_utils.py +++ b/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') diff --git a/test/unit/obj/test_diskfile.py b/test/unit/obj/test_diskfile.py index 12840bce04..e4ad4c0e7a 100644 --- a/test/unit/obj/test_diskfile.py +++ b/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 - self.assertEqual(sorted(os.listdir(df._datadir)), []) + # the directory was empty and has been removed + self.assertFalse(os.path.exists(df._datadir)) def _do_test_open_most_recent_durable(self, legacy_durable): policy = POLICIES.default diff --git a/test/unit/obj/test_reconstructor.py b/test/unit/obj/test_reconstructor.py index 33d75ed85e..cc932d9be9 100644 --- a/test/unit/obj/test_reconstructor.py +++ b/test/unit/obj/test_reconstructor.py @@ -4007,15 +4007,9 @@ class TestObjectReconstructor(BaseTestObjectReconstructor): ], [ (r['ip'], r['path']) for r in request_log.requests ]) - # hashpath is still there, but all files have been purged - files = os.listdir(df._datadir) - self.assertFalse(files) + # hashpath has been removed + self.assertFalse(os.path.exists(df._datadir)) - # 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):