diff --git a/swift/common/db_replicator.py b/swift/common/db_replicator.py index 7d1bb7afac..4c0082a206 100644 --- a/swift/common/db_replicator.py +++ b/swift/common/db_replicator.py @@ -111,9 +111,6 @@ def roundrobin_datadirs(datadirs): 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 d93da620e6..47c36c1017 100644 --- a/test/unit/common/test_db_replicator.py +++ b/test/unit/common/test_db_replicator.py @@ -1051,147 +1051,71 @@ class TestDBReplicator(unittest.TestCase): @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 = [] + datadir = os.path.join(tempdir, 'containers') + db_path = ('450/afd/7089ab48d955ab0851fc51cc17a34afd/' + '7089ab48d955ab0851fc51cc17a34afd.db') + random_file = ('1060/xyz/1234ab48d955ab0851fc51cc17a34xyz/' + '1234ab48d955ab0851fc51cc17a34xyz.abc') - 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' + # trailing "/" indicates empty dir + paths = [ + # empty part dir + '240/', + # empty suffix dir + '18/aba/', + # empty hashdir + '1054/27e/d41d8cd98f00b204e9800998ecf8427e/', + # database + db_path, + # non database file + random_file, + ] + for path in paths: + path = os.path.join(datadir, path) + os.makedirs(os.path.dirname(path)) + if os.path.basename(path): + # our setup requires "directories" to end in "/" (i.e. basename + # is ''); otherwise, create an empty file + open(path, 'w') + # sanity + self.assertEqual({'240', '18', '1054', '1060', '450'}, + set(os.listdir(datadir))) + for path in paths: + dirpath = os.path.join(datadir, os.path.dirname(path)) + self.assertTrue(os.path.isdir(dirpath)) - 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 [] + node_id = 1 + results = list(db_replicator.roundrobin_datadirs([(datadir, node_id)])) + expected = [ + ('450', os.path.join(datadir, db_path), node_id), + ] + self.assertEqual(results, expected) - 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 + # all the empty leaf dirs are cleaned up + for path in paths: + if os.path.basename(path): + check = self.assertTrue + else: + check = self.assertFalse + dirpath = os.path.join(datadir, os.path.dirname(path)) + isdir = os.path.isdir(dirpath) + check(isdir, '%r is%s a directory!' % ( + dirpath, '' if isdir else ' not')) - 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 + # despite the leaves cleaned up it takes a few loops to finish it off + self.assertEqual({'18', '1054', '1060', '450'}, + set(os.listdir(datadir))) - def _shuffle(arg): - shuffle_calls.append(arg) + results = list(db_replicator.roundrobin_datadirs([(datadir, node_id)])) + self.assertEqual(results, expected) + self.assertEqual({'1054', '1060', '450'}, + set(os.listdir(datadir))) - 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 + results = list(db_replicator.roundrobin_datadirs([(datadir, node_id)])) + self.assertEqual(results, expected) + # non db file in '1060' dir is not deleted and exception is handled + self.assertEqual({'1060', '450'}, + set(os.listdir(datadir))) def test_roundrobin_datadirs(self): listdir_calls = [] @@ -1261,18 +1185,12 @@ class TestDBReplicator(unittest.TestCase): def _rmdir(arg): rmdir_calls.append(arg) - 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 + base = 'swift.common.db_replicator.' + with mock.patch(base + 'os.listdir', _listdir), \ + mock.patch(base + 'os.path.isdir', _isdir), \ + mock.patch(base + 'os.path.exists', _exists), \ + mock.patch(base + 'random.shuffle', _shuffle), \ + mock.patch(base + 'os.rmdir', _rmdir): datadirs = [('/srv/node/sda/containers', 1), ('/srv/node/sdb/containers', 2)] @@ -1365,12 +1283,6 @@ class TestDBReplicator(unittest.TestCase): self.assertEqual( rmdir_calls, ['/srv/node/sda/containers/9999', '/srv/node/sdb/containers/9999']) - 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 @mock.patch("swift.common.db_replicator.ReplConnection", mock.Mock()) def test_http_connect(self):