From 321bb9145f9e2e184994e5d0cf78c62b6043112c Mon Sep 17 00:00:00 2001 From: Mahati Chamarthy Date: Mon, 20 Jun 2016 15:56:15 +0530 Subject: [PATCH] remove empty db hash and suffix directories If a db gets quarantined we may fail to cleanup an empty suffix dir or a hash dir. Change-Id: I721fa5fe9a7ae22eead8d5141f93e116847ca058 Closes-Bug: #1583719 --- swift/common/db_replicator.py | 13 +++ test/unit/common/test_db_replicator.py | 145 +++++++++++++++++++++++++ 2 files changed, 158 insertions(+) diff --git a/swift/common/db_replicator.py b/swift/common/db_replicator.py index 7115a8afe4..7d1bb7afac 100644 --- a/swift/common/db_replicator.py +++ b/swift/common/db_replicator.py @@ -89,11 +89,15 @@ def roundrobin_datadirs(datadirs): suffixes = os.listdir(part_dir) if not suffixes: os.rmdir(part_dir) + continue for suffix in suffixes: suff_dir = os.path.join(part_dir, suffix) if not os.path.isdir(suff_dir): continue hashes = os.listdir(suff_dir) + if not hashes: + os.rmdir(suff_dir) + continue for hsh in hashes: hash_dir = os.path.join(suff_dir, hsh) if not os.path.isdir(hash_dir): @@ -101,6 +105,15 @@ def roundrobin_datadirs(datadirs): object_file = os.path.join(hash_dir, hsh + '.db') if os.path.exists(object_file): yield (partition, object_file, node_id) + else: + try: + os.rmdir(hash_dir) + except OSError as e: + if e.errno is not errno.ENOTEMPTY: + raise + # remove empty partitions after the above directory walk + if not suffixes: + os.rmdir(part_dir) its = [walk_datadir(datadir, node_id) for datadir, node_id in datadirs] while its: diff --git a/test/unit/common/test_db_replicator.py b/test/unit/common/test_db_replicator.py index 29d66df99d..d93da620e6 100644 --- a/test/unit/common/test_db_replicator.py +++ b/test/unit/common/test_db_replicator.py @@ -36,6 +36,7 @@ from swift.common.swob import HTTPException from test import unit from test.unit.common.test_db import ExampleBroker +from test.unit import with_tempdir TEST_ACCOUNT_NAME = 'a c t' @@ -1048,6 +1049,150 @@ class TestDBReplicator(unittest.TestCase): finally: rmtree(drive) + @with_tempdir + def test_empty_suffix_and_hash_dirs_get_cleanedup(self, tempdir): + # tests empty suffix and hash dirs of a quarantined db are cleaned up + listdir_calls = [] + isdir_calls = [] + exists_calls = [] + shuffle_calls = [] + rmdir_calls = [] + + existing_file = os.path.join(tempdir, 'a_file_exists') + open(existing_file, 'a').close() + dir_with_no_obj_file = '/srv/node/sdb/containers/9876/xyz/' \ + '11111111111111111111111111111xyz' + + def _listdir(path): + listdir_calls.append(path) + if not path.startswith('/srv/node/sda/containers') and \ + not path.startswith('/srv/node/sdb/containers'): + return [] + path = path[len('/srv/node/sdx/containers'):] + if path == '': + return ['6789', '9876'] + elif path == '/9876': + return ['xyz'] + elif path == '/9876/xyz': + return ['11111111111111111111111111111xyz'] + elif path == '/9876/xyz/11111111111111111111111111111xyz': + return [] + elif path == '/6789': + return ['jkl'] + elif path == '/6789/jkl': + return [] + return [] + + def _isdir(path): + isdir_calls.append(path) + if not path.startswith('/srv/node/sda/containers') and \ + not path.startswith('/srv/node/sdb/containers'): + return False + path = path[len('/srv/node/sdx/containers'):] + if path in ('/9876', '/9876/xyz', + '/9876/xyz/11111111111111111111111111111xyz', + '/6789', '/6789/jkl'): + return True + return False + + def _exists(arg): + exists_calls.append(arg) + if arg in ('/srv/node/sda/containers/9876/xyz/' + '11111111111111111111111111111xyz/' + '11111111111111111111111111111xyz.db', + '/srv/node/sdb/containers/9876/xyz/' + '11111111111111111111111111111xyz/' + '11111111111111111111111111111xyz.db'): + return False + return True + + def _shuffle(arg): + shuffle_calls.append(arg) + + def _rmdir(arg): + rmdir_calls.append(arg) + if arg == dir_with_no_obj_file: + # use db_replicator.os.rmdir to delete a directory with an + # existing file; fail if it doesn't handle OSError + orig_rmdir(tempdir) + self.fail('The rmdir did not handle the exception as expected') + + orig_listdir = db_replicator.os.listdir + orig_isdir = db_replicator.os.path.isdir + orig_exists = db_replicator.os.path.exists + orig_shuffle = db_replicator.random.shuffle + orig_rmdir = db_replicator.os.rmdir + + try: + db_replicator.os.listdir = _listdir + db_replicator.os.path.isdir = _isdir + db_replicator.os.path.exists = _exists + db_replicator.random.shuffle = _shuffle + db_replicator.os.rmdir = _rmdir + + datadirs = [('/srv/node/sda/containers', 1), + ('/srv/node/sdb/containers', 2)] + results = list(db_replicator.roundrobin_datadirs(datadirs)) + # The results show that there are no .db files to be returned + # in this case + self.assertEqual(results, []) + # The listdir calls show that we only listdir the dirs + self.assertEqual(listdir_calls, [ + '/srv/node/sda/containers', + '/srv/node/sda/containers/6789', + '/srv/node/sda/containers/6789/jkl', + '/srv/node/sda/containers/9876', + '/srv/node/sda/containers/9876/xyz', + '/srv/node/sdb/containers', + '/srv/node/sdb/containers/6789', + '/srv/node/sdb/containers/6789/jkl', + '/srv/node/sdb/containers/9876', + '/srv/node/sdb/containers/9876/xyz']) + # The isdir calls show that we did ask about the things pretending + # to be files at various levels. + self.assertEqual(isdir_calls, [ + '/srv/node/sda/containers/6789', + '/srv/node/sda/containers/6789/jkl', + '/srv/node/sda/containers/9876', + '/srv/node/sda/containers/9876/xyz', + ('/srv/node/sda/containers/9876/xyz/' + '11111111111111111111111111111xyz'), + '/srv/node/sdb/containers/6789', + '/srv/node/sdb/containers/6789/jkl', + '/srv/node/sdb/containers/9876', + '/srv/node/sdb/containers/9876/xyz', + ('/srv/node/sdb/containers/9876/xyz/' + '11111111111111111111111111111xyz')]) + # The exists calls are the .db files we looked for as we walked the + # structure. + self.assertEqual(exists_calls, [ + ('/srv/node/sda/containers/9876/xyz/' + '11111111111111111111111111111xyz/' + '11111111111111111111111111111xyz.db'), + ('/srv/node/sdb/containers/9876/xyz/' + '11111111111111111111111111111xyz/' + '11111111111111111111111111111xyz.db')]) + # Shows that we called shuffle twice, once for each device. + self.assertEqual( + shuffle_calls, [['6789', '9876'], + ['6789', '9876']]) + + # Shows that we called rmdir and removed directories with no db + # files and directories with no hashes + self.assertEqual( + rmdir_calls, ['/srv/node/sda/containers/6789/jkl', + '/srv/node/sda/containers/9876/xyz/' + '11111111111111111111111111111xyz', + '/srv/node/sdb/containers/6789/jkl', + '/srv/node/sdb/containers/9876/xyz/' + '11111111111111111111111111111xyz']) + finally: + db_replicator.os.listdir = orig_listdir + db_replicator.os.path.isdir = orig_isdir + db_replicator.os.path.exists = orig_exists + db_replicator.random.shuffle = orig_shuffle + db_replicator.os.rmdir = orig_rmdir + def test_roundrobin_datadirs(self): listdir_calls = [] isdir_calls = []