From 95905b017495ac822a7e6998ae12e2032481d932 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexandre=20L=C3=A9cuyer?= Date: Fri, 24 Mar 2017 15:41:45 +0100 Subject: [PATCH] Modify _get_hashes() arguments to be more generic Some public functions in the diskfile manager expect or return full file paths. It implies a filesystem diskfile implementation. To make it easier to plug alternate diskfile implementations, patch functions to take more generic arguments. This commit changes DiskFileManager _get_hashes() arguments from: - partition_path, recalculate=None, do_listdir=False to : - device, partition, policy, recalculate=None, do_listdir=False Callers are modified accordingly, in diskfile.py, reconstructor.py, and replicator.py Change-Id: I8e2d7075572e466ae2fa5ebef5e31d87eed90fec --- swift/obj/diskfile.py | 33 +++++++++++++++++++---------- swift/obj/reconstructor.py | 15 +++++++------ swift/obj/replicator.py | 6 ++++-- test/unit/obj/test_diskfile.py | 11 ++++++++++ test/unit/obj/test_reconstructor.py | 2 +- test/unit/obj/test_replicator.py | 7 ++++-- 6 files changed, 52 insertions(+), 22 deletions(-) diff --git a/swift/obj/diskfile.py b/swift/obj/diskfile.py index bf0ae8a0b4..a10263b606 100644 --- a/swift/obj/diskfile.py +++ b/swift/obj/diskfile.py @@ -340,6 +340,14 @@ def invalidate_hash(suffix_dir): inv_fh.write(suffix + "\n") +def get_part_path(dev_path, policy, partition): + """ + Given the device path, policy, and partition, returns the full + path to the partition + """ + return os.path.join(dev_path, get_data_dir(policy), str(partition)) + + class AuditLocation(object): """ Represents an object location to be audited. @@ -1041,13 +1049,16 @@ class BaseDiskFileManager(object): hashes.pop('valid', None) return hashed, hashes - def __get_hashes(self, partition_path, recalculate=None, do_listdir=False): + def __get_hashes(self, device, partition, policy, recalculate=None, + do_listdir=False): """ Get hashes for each suffix dir in a partition. do_listdir causes it to mistrust the hash cache for suffix existence at the (unexpectedly high) cost of a listdir. - :param partition_path: absolute path of partition to get hashes for + :param device: name of target device + :param partition: partition on the device in which the object lives + :param policy: the StoragePolicy instance :param recalculate: list of suffixes which should be recalculated when got :param do_listdir: force existence check for all hashes in the @@ -1056,6 +1067,8 @@ class BaseDiskFileManager(object): :returns: tuple of (number of suffix dirs hashed, dictionary of hashes) """ hashed = 0 + dev_path = self.get_dev_path(device) + partition_path = get_part_path(dev_path, policy, partition) hashes_file = join(partition_path, HASH_FILE) modified = False orig_hashes = {'valid': False} @@ -1115,7 +1128,9 @@ class BaseDiskFileManager(object): if read_hashes(partition_path) == orig_hashes: write_hashes(partition_path, hashes) return hashed, hashes - return self.__get_hashes(partition_path, recalculate, do_listdir) + return self.__get_hashes(device, partition, policy, + recalculate=recalculate, + do_listdir=do_listdir) else: return hashed, hashes @@ -1304,12 +1319,11 @@ class BaseDiskFileManager(object): dev_path = self.get_dev_path(device) if not dev_path: raise DiskFileDeviceUnavailable() - partition_path = os.path.join(dev_path, get_data_dir(policy), - partition) + partition_path = get_part_path(dev_path, policy, partition) if not os.path.exists(partition_path): mkdirs(partition_path) _junk, hashes = tpool_reraise( - self._get_hashes, partition_path, recalculate=suffixes) + self._get_hashes, device, partition, policy, recalculate=suffixes) return hashes def _listdir(self, path): @@ -1337,8 +1351,7 @@ class BaseDiskFileManager(object): dev_path = self.get_dev_path(device) if not dev_path: raise DiskFileDeviceUnavailable() - partition_path = os.path.join(dev_path, get_data_dir(policy), - partition) + partition_path = get_part_path(dev_path, policy, partition) for suffix in self._listdir(partition_path): if len(suffix) != 3: continue @@ -1379,9 +1392,7 @@ class BaseDiskFileManager(object): if suffixes is None: suffixes = self.yield_suffixes(device, partition, policy) else: - partition_path = os.path.join(dev_path, - get_data_dir(policy), - str(partition)) + partition_path = get_part_path(dev_path, policy, partition) suffixes = ( (os.path.join(partition_path, suffix), suffix) for suffix in suffixes) diff --git a/swift/obj/reconstructor.py b/swift/obj/reconstructor.py index d6a6ba9fad..242f461372 100644 --- a/swift/obj/reconstructor.py +++ b/swift/obj/reconstructor.py @@ -505,11 +505,12 @@ class ObjectReconstructor(Daemon): self.kill_coros() self.last_reconstruction_count = self.reconstruction_count - def _get_hashes(self, policy, path, recalculate=None, do_listdir=False): + def _get_hashes(self, device, partition, policy, recalculate=None, + do_listdir=False): df_mgr = self._df_router[policy] hashed, suffix_hashes = tpool_reraise( - df_mgr._get_hashes, path, recalculate=recalculate, - do_listdir=do_listdir) + df_mgr._get_hashes, device, partition, policy, + recalculate=recalculate, do_listdir=do_listdir) self.logger.update_stats('suffix.hashes', hashed) return suffix_hashes @@ -602,8 +603,9 @@ class ObjectReconstructor(Daemon): node['index']) # now recalculate local hashes for suffixes that don't # match so we're comparing the latest - local_suff = self._get_hashes(job['policy'], job['path'], - recalculate=suffixes) + local_suff = self._get_hashes(job['local_dev']['device'], + job['partition'], + job['policy'], recalculate=suffixes) suffixes = self.get_suffix_delta(local_suff, job['frag_index'], @@ -769,7 +771,8 @@ class ObjectReconstructor(Daemon): """ # find all the fi's in the part, and which suffixes have them try: - hashes = self._get_hashes(policy, part_path, do_listdir=True) + hashes = self._get_hashes(local_dev['device'], partition, policy, + do_listdir=True) except OSError as e: if e.errno != errno.ENOTDIR: raise diff --git a/swift/obj/replicator.py b/swift/obj/replicator.py index 4d1dd8a16b..4f1d43c602 100644 --- a/swift/obj/replicator.py +++ b/swift/obj/replicator.py @@ -408,7 +408,8 @@ class ObjectReplicator(Daemon): df_mgr = self._df_router[job['policy']] try: hashed, local_hash = tpool_reraise( - df_mgr._get_hashes, job['path'], + df_mgr._get_hashes, job['device'], + job['partition'], job['policy'], do_listdir=_do_listdir( int(job['partition']), self.replication_cycle)) @@ -462,7 +463,8 @@ class ObjectReplicator(Daemon): continue hashed, recalc_hash = tpool_reraise( df_mgr._get_hashes, - job['path'], recalculate=suffixes) + job['device'], job['partition'], job['policy'], + recalculate=suffixes) self.logger.update_stats('suffix.hashes', hashed) local_hash = recalc_hash suffixes = [suffix for suffix in local_hash if diff --git a/test/unit/obj/test_diskfile.py b/test/unit/obj/test_diskfile.py index e79c3308c3..7148741082 100644 --- a/test/unit/obj/test_diskfile.py +++ b/test/unit/obj/test_diskfile.py @@ -275,6 +275,17 @@ class TestDiskFileModuleMethods(unittest.TestCase): # check tempdir self.assertTrue(os.path.isdir(tmp_path)) + def test_get_part_path(self): + # partition passed as 'str' + part_dir = diskfile.get_part_path('/srv/node/sda1', POLICIES[0], '123') + exp_dir = '/srv/node/sda1/objects/123' + self.assertEqual(part_dir, exp_dir) + + # partition passed as 'int' + part_dir = diskfile.get_part_path('/srv/node/sdb5', POLICIES[1], 123) + exp_dir = '/srv/node/sdb5/objects-1/123' + self.assertEqual(part_dir, exp_dir) + @patch_policies class TestObjectAuditLocationGenerator(unittest.TestCase): diff --git a/test/unit/obj/test_reconstructor.py b/test/unit/obj/test_reconstructor.py index d2814b52fb..e980ab12f7 100644 --- a/test/unit/obj/test_reconstructor.py +++ b/test/unit/obj/test_reconstructor.py @@ -1790,7 +1790,7 @@ class TestObjectReconstructor(unittest.TestCase): diskfile.HASH_FILE) self.assertTrue(os.path.exists(hashes_file)) suffixes = self.reconstructor._get_hashes( - self.policy, part_path, do_listdir=True) + self.local_dev['device'], 0, self.policy, do_listdir=True) self.assertEqual(suffixes, {}) def test_build_jobs_no_hashes(self): diff --git a/test/unit/obj/test_replicator.py b/test/unit/obj/test_replicator.py index d3116fb427..29c1d142ea 100644 --- a/test/unit/obj/test_replicator.py +++ b/test/unit/obj/test_replicator.py @@ -1627,8 +1627,11 @@ class TestObjectReplicator(unittest.TestCase): # if a timeout occurs while replicating one partition to one node. timeouts = [Timeout()] - def fake_get_hashes(df_mgr, part_path, **kwargs): + def fake_get_hashes(df_mgr, device, partition, policy, **kwargs): self.get_hash_count += 1 + dev_path = df_mgr.get_dev_path(device) + part_path = os.path.join(dev_path, diskfile.get_data_dir(policy), + str(partition)) # Simulate a REPLICATE timeout by raising Timeout for second call # to get_hashes (with recalculate suffixes) for a specific # partition @@ -1750,7 +1753,7 @@ class TestObjectReplicator(unittest.TestCase): mock_do_listdir.side_effect = do_listdir_results expected_tpool_calls = [ mock.call(self.replicator._df_router[job['policy']]._get_hashes, - job['path'], + job['device'], job['partition'], job['policy'], do_listdir=do_listdir) for job, do_listdir in zip(jobs, do_listdir_results) ]