Merge "Cleanup tests from empty suffix quarantined db fix"

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:

test/unit/common/test_db_replicator.py

@@ -1053,147 +1053,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
+        datadir = os.path.join(tempdir, 'containers')
-        listdir_calls = []
+        db_path = ('450/afd/7089ab48d955ab0851fc51cc17a34afd/'
-        isdir_calls = []
+                   '7089ab48d955ab0851fc51cc17a34afd.db')
-        exists_calls = []
+        random_file = ('1060/xyz/1234ab48d955ab0851fc51cc17a34xyz/'
-        shuffle_calls = []
+                       '1234ab48d955ab0851fc51cc17a34xyz.abc')
-        rmdir_calls = []
 
-        existing_file = os.path.join(tempdir, 'a_file_exists')
+        # trailing "/" indicates empty dir
-        open(existing_file, 'a').close()
+        paths = [
-        dir_with_no_obj_file = '/srv/node/sdb/containers/9876/xyz/' \
+            # empty part dir
-                               '11111111111111111111111111111xyz'
+            '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):
+        node_id = 1
-            listdir_calls.append(path)
+        results = list(db_replicator.roundrobin_datadirs([(datadir, node_id)]))
-            if not path.startswith('/srv/node/sda/containers') and \
+        expected = [
-                    not path.startswith('/srv/node/sdb/containers'):
+            ('450', os.path.join(datadir, db_path), node_id),
-                return []
+        ]
-            path = path[len('/srv/node/sdx/containers'):]
+        self.assertEqual(results, expected)
-            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):
+        # all the empty leaf dirs are cleaned up
-            isdir_calls.append(path)
+        for path in paths:
-            if not path.startswith('/srv/node/sda/containers') and \
+            if os.path.basename(path):
-                    not path.startswith('/srv/node/sdb/containers'):
+                check = self.assertTrue
-                return False
+            else:
-            path = path[len('/srv/node/sdx/containers'):]
+                check = self.assertFalse
-            if path in ('/9876', '/9876/xyz',
+            dirpath = os.path.join(datadir, os.path.dirname(path))
-                        '/9876/xyz/11111111111111111111111111111xyz',
+            isdir = os.path.isdir(dirpath)
-                        '/6789', '/6789/jkl'):
+            check(isdir, '%r is%s a directory!' % (
-                return True
+                dirpath, '' if isdir else ' not'))
-            return False
 
-        def _exists(arg):
+        # despite the leaves cleaned up it takes a few loops to finish it off
-            exists_calls.append(arg)
+        self.assertEqual({'18', '1054', '1060', '450'},
-            if arg in ('/srv/node/sda/containers/9876/xyz/'
+                         set(os.listdir(datadir)))
-                       '11111111111111111111111111111xyz/'
-                       '11111111111111111111111111111xyz.db',
-                       '/srv/node/sdb/containers/9876/xyz/'
-                       '11111111111111111111111111111xyz/'
-                       '11111111111111111111111111111xyz.db'):
-                return False
-            return True
 
-        def _shuffle(arg):
+        results = list(db_replicator.roundrobin_datadirs([(datadir, node_id)]))
-            shuffle_calls.append(arg)
+        self.assertEqual(results, expected)
+        self.assertEqual({'1054', '1060', '450'},
+                         set(os.listdir(datadir)))
 
-        def _rmdir(arg):
+        results = list(db_replicator.roundrobin_datadirs([(datadir, node_id)]))
-            rmdir_calls.append(arg)
+        self.assertEqual(results, expected)
-            if arg == dir_with_no_obj_file:
+        # non db file in '1060' dir is not deleted and exception is handled
-                # use db_replicator.os.rmdir to delete a directory with an
+        self.assertEqual({'1060', '450'},
-                # existing file; fail if it doesn't handle OSError
+                         set(os.listdir(datadir)))
-                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 = []
@@ -1263,18 +1187,12 @@ class TestDBReplicator(unittest.TestCase):
         def _rmdir(arg):
             rmdir_calls.append(arg)
 
-        orig_listdir = db_replicator.os.listdir
+        base = 'swift.common.db_replicator.'
-        orig_isdir = db_replicator.os.path.isdir
+        with mock.patch(base + 'os.listdir', _listdir), \
-        orig_exists = db_replicator.os.path.exists
+                mock.patch(base + 'os.path.isdir', _isdir), \
-        orig_shuffle = db_replicator.random.shuffle
+                mock.patch(base + 'os.path.exists', _exists), \
-        orig_rmdir = db_replicator.os.rmdir
+                mock.patch(base + 'random.shuffle', _shuffle), \
-
+                mock.patch(base + 'os.rmdir', _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)]
@@ -1367,12 +1285,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):