From 3bdd01cf4a1c8ee9ac953850106da505d381aa44 Mon Sep 17 00:00:00 2001 From: Alistair Coles Date: Fri, 26 Mar 2021 13:41:36 +0000 Subject: [PATCH] relinker: retry links from older part powers If a previous partition power increase failed to cleanup all files in their old partition locations, then during the next partition power increase the relinker may find the same file to relink in more than one source partition. This currently leads to an error log due to the second relink attempt getting an EEXIST error. With this patch, when an EEXIST is raised, the relinker will attempt to create/verify a link from older partition power locations to the next part power location, and if such a link is found then suppress the error log. During the relink step, if an alternative link is verified and if a file is found that is neither linked to the next partition power location nor in the current part power location, then the file is removed during the relink step. That prevents the same EEXIST occuring again during the cleanup step when it may no longer be possible to verify that an alternative link exists. For example, consider identical filenames in the N+1th, Nth and N-1th partition power locations, with the N+1th being linked to the Nth: - During relink, the Nth location is visited and its link is verified. Then the N-1th location is visited and an EEXIST error is encountered, but the new check verifies that a link exists to the Nth location, which is OK. - During cleanup the locations are visited in the same order, but files are removed so that the Nth location file no longer exists when the N-1th location is visited. If the N-1th location still has a conflicting file then existence of an alternative link to the Nth location can no longer be verified, so an error would be raised. Therefore, the N-1th location file must be removed during relink. The error is only suppressed for tombstones. The number of partition power location that the relinker will look back over may be configured using the link_check_limit option in a conf file or --link-check-limit on the command line, and defaults to 2. Closes-Bug: 1921718 Change-Id: If9beb9efabdad64e81d92708f862146d5fafb16c --- etc/object-server.conf-sample | 19 ++ swift/cli/relinker.py | 84 +++++- swift/obj/diskfile.py | 16 +- test/unit/cli/test_relinker.py | 520 ++++++++++++++++++++++++++++++++- test/unit/common/test_utils.py | 2 + test/unit/obj/test_diskfile.py | 12 +- 6 files changed, 620 insertions(+), 33 deletions(-) diff --git a/etc/object-server.conf-sample b/etc/object-server.conf-sample index b51d35c2d7..cc6a54b43f 100644 --- a/etc/object-server.conf-sample +++ b/etc/object-server.conf-sample @@ -614,3 +614,22 @@ use = egg:swift#xprofile # likelihood that the added I/O from a partition-power increase impacts # client traffic. Use zero for unlimited. # files_per_second = 0.0 +# +# Maximum number of partition power locations to check for a valid link target +# if the relinker encounters an existing tombstone, but with different inode, +# in the next partition power location. If the relinker fails to make a link +# because a different tombstone already exists in the next partition power +# location then it will try to validate that the existing tombstone links to a +# valid target in the current partition power location, or previous partition +# power locations, in descending order. This option limits the number of +# partition power locations searched, including the current partition power, +# and should be a whole number. A value of 0 implies that no validation is +# attempted, and an error is logged, when an existing tombstone prevents link +# creation. A value of 1 implies that an existing link is accepted if it links +# to a tombstone in the current partition power location. The default value of +# 2 implies that an existing link is acceptable if it links to a tombstone in +# the current or previous partition power locations. Increased values may be +# useful if previous partition power increases have failed to cleanup +# tombstones from their old locations, causing duplicate tombstones with +# different inodes to be relinked to the next partition power location. +# link_check_limit = 2 \ No newline at end of file diff --git a/swift/cli/relinker.py b/swift/cli/relinker.py index c564e88827..d5856c037f 100644 --- a/swift/cli/relinker.py +++ b/swift/cli/relinker.py @@ -257,6 +257,29 @@ class Relinker(object): hashes.remove(hsh) return hashes + def check_existing(self, new_file): + existing_link = None + link_created = False + start = self.next_part_power - 1 + stop = max(start - self.conf['link_check_limit'], -1) + for check_part_power in range(start, stop, -1): + # Try to create the link from each of 3 previous part power + # locations. If an attempt succeeds then either a link was made or + # an existing link with the same inode as the next part power + # location was found: either is acceptable. The part power location + # that previously failed with EEXIST is included in the further + # attempts here for simplicity. + target_file = replace_partition_in_path( + self.conf['devices'], new_file, check_part_power) + try: + link_created = diskfile.relink_paths(target_file, new_file, + ignore_missing=False) + existing_link = target_file + break + except OSError: + pass + return existing_link, link_created + def process_location(self, hash_path, new_hash_path): # Compare the contents of each hash dir with contents of same hash # dir in its new partition to verify that the new location has the @@ -288,6 +311,7 @@ class Relinker(object): missing_links = 0 created_links = 0 + unwanted_files = [] for filename in required_links: # Before removing old files, be sure that the corresponding # required new files exist by calling relink_paths again. There @@ -321,22 +345,52 @@ class Relinker(object): created_links += 1 self.stats['linked'] += 1 except OSError as exc: - self.logger.warning( - "Error relinking%s: failed to relink %s to " - "%s: %s", ' (cleanup)' if self.do_cleanup else '', - old_file, new_file, exc) - self.stats['errors'] += 1 - missing_links += 1 + existing_link = None + link_created = False + if exc.errno == errno.EEXIST and filename.endswith('.ts'): + # special case for duplicate tombstones in older partition + # power locations + # (https://bugs.launchpad.net/swift/+bug/1921718) + existing_link, link_created = self.check_existing(new_file) + if existing_link: + self.logger.debug( + "Relinking%s: link not needed: %s to %s due to " + "existing %s", ' (cleanup)' if self.do_cleanup else '', + old_file, new_file, existing_link) + if link_created: + # uncommon case: the retry succeeded in creating a link + created_links += 1 + self.stats['linked'] += 1 + wanted_file = replace_partition_in_path( + self.conf['devices'], old_file, self.part_power) + if old_file not in (existing_link, wanted_file): + # A link exists to a different file and this file + # is not the current target for client requests. If + # this file is visited again it is possible that + # the existing_link will have been cleaned up and + # the check will fail, so clean it up now. + self.logger.info( + "Relinking%s: cleaning up unwanted file: %s", + ' (cleanup)' if self.do_cleanup else '', old_file) + unwanted_files.append(filename) + else: + self.logger.warning( + "Error relinking%s: failed to relink %s to %s: %s", + ' (cleanup)' if self.do_cleanup else '', + old_file, new_file, exc) + self.stats['errors'] += 1 + missing_links += 1 if created_links: diskfile.invalidate_hash(os.path.dirname(new_hash_path)) - if missing_links or not self.do_cleanup: - return + + if self.do_cleanup and not missing_links: + # use the sorted list to help unit testing + unwanted_files = old_df_data['files'] # the new partition hash dir has the most up to date set of on # disk files so it is safe to delete the old location... rehash = False - # use the sorted list to help unit testing - for filename in old_df_data['files']: + for filename in unwanted_files: old_file = os.path.join(hash_path, filename) try: os.remove(old_file) @@ -487,6 +541,13 @@ def main(args): help='Set log file name. Ignored if using conf_file.') parser.add_argument('--debug', default=False, action='store_true', help='Enable debug mode') + parser.add_argument('--link-check-limit', type=non_negative_int, + default=None, dest='link_check_limit', + help='Maximum number of partition power locations to ' + 'check for a valid link target if relink ' + 'encounters an existing tombstone with different ' + 'inode in the next partition power location ' + '(default: 2).') args = parser.parse_args(args) if args.conf_file: @@ -519,6 +580,9 @@ def main(args): else non_negative_float(conf.get('files_per_second', '0'))), 'policies': set(args.policies) or POLICIES, 'partitions': set(args.partitions), + 'link_check_limit': ( + args.link_check_limit if args.link_check_limit is not None + else non_negative_int(conf.get('link_check_limit', 2))), }) if args.action == 'relink': diff --git a/swift/obj/diskfile.py b/swift/obj/diskfile.py index b119831521..0a486299c5 100644 --- a/swift/obj/diskfile.py +++ b/swift/obj/diskfile.py @@ -443,15 +443,19 @@ def invalidate_hash(suffix_dir): inv_fh.write(suffix + b"\n") -def relink_paths(target_path, new_target_path): +def relink_paths(target_path, new_target_path, ignore_missing=True): """ - Hard-links a file located in target_path using the second path - new_target_path. Creates intermediate directories if required. + Hard-links a file located in ``target_path`` using the second path + ``new_target_path``. Creates intermediate directories if required. :param target_path: current absolute filename :param new_target_path: new absolute filename for the hardlink + :param ignore_missing: if True then no exception is raised if the link + could not be made because ``target_path`` did not exist, otherwise an + OSError will be raised. :raises: OSError if the hard link could not be created, unless the intended - hard link already exists or the target_path does not exist. + hard link already exists or the ``target_path`` does not exist and + ``must_exist`` if False. :returns: True if the link was created by the call to this method, False otherwise. """ @@ -473,14 +477,14 @@ def relink_paths(target_path, new_target_path): ok = False if err.errno == errno.ENOENT: # this is ok if the *target* path doesn't exist anymore - ok = not os.path.exists(target_path) + ok = not os.path.exists(target_path) and ignore_missing if err.errno == errno.EEXIST: # this is ok *if* the intended link has already been made try: orig_stat = os.stat(target_path) except OSError as sub_err: # this is ok: the *target* path doesn't exist anymore - ok = sub_err.errno == errno.ENOENT + ok = sub_err.errno == errno.ENOENT and ignore_missing else: try: new_stat = os.stat(new_target_path) diff --git a/test/unit/cli/test_relinker.py b/test/unit/cli/test_relinker.py index b7ce68c64b..616aa6e66f 100644 --- a/test/unit/cli/test_relinker.py +++ b/test/unit/cli/test_relinker.py @@ -78,8 +78,8 @@ class TestRelinker(unittest.TestCase): container = 'c' obj = 'o-' + str(uuid.uuid4()) _hash = utils.hash_path(account, container, obj) - part = utils.get_partition_for_hash(_hash, 8) - next_part = utils.get_partition_for_hash(_hash, 9) + part = utils.get_partition_for_hash(_hash, PART_POWER) + next_part = utils.get_partition_for_hash(_hash, PART_POWER + 1) obj_path = os.path.join(os.path.sep, account, container, obj) # There's 1/512 chance that both old and new parts will be 0; # that's not a terribly interesting case, as there's nothing to do @@ -121,6 +121,21 @@ class TestRelinker(unittest.TestCase): self.expected_dir = os.path.join(self.next_suffix_dir, self._hash) self.expected_file = os.path.join(self.expected_dir, self.object_fname) + def _make_link(self, filename, part_power): + # make a file in the older part_power location and link it to a file in + # the next part power location + new_filepath = os.path.join(self.expected_dir, filename) + older_filepath = utils.replace_partition_in_path( + self.devices, new_filepath, part_power) + os.makedirs(os.path.dirname(older_filepath)) + with open(older_filepath, 'w') as fd: + fd.write(older_filepath) + os.makedirs(self.expected_dir) + os.link(older_filepath, new_filepath) + with open(new_filepath, 'r') as fd: + self.assertEqual(older_filepath, fd.read()) # sanity check + return older_filepath, new_filepath + def _save_ring(self, policies=POLICIES): self.rb._ring = None rd = self.rb.get_ring() @@ -288,6 +303,7 @@ class TestRelinker(unittest.TestCase): 'log_level': 'DEBUG', 'policies': POLICIES, 'partitions': set(), + 'link_check_limit': 2, } mock_relink.assert_called_once_with(exp_conf, mock.ANY, device='sdx') logger = mock_relink.call_args[0][1] @@ -313,6 +329,7 @@ class TestRelinker(unittest.TestCase): log_level = WARNING log_name = test-relinker files_per_second = 11.1 + link_check_limit = 1 """ with open(conf_file, 'w') as f: f.write(dedent(config)) @@ -328,6 +345,7 @@ class TestRelinker(unittest.TestCase): 'log_level': 'WARNING', 'policies': POLICIES, 'partitions': set(), + 'link_check_limit': 1, }, mock.ANY, device='sdx') logger = mock_relink.call_args[0][1] self.assertEqual(logging.WARNING, logger.getEffectiveLevel()) @@ -340,7 +358,9 @@ class TestRelinker(unittest.TestCase): '--swift-dir', 'cli-dir', '--devices', 'cli-devs', '--skip-mount-check', '--files-per-second', '2.2', '--policy', '1', '--partition', '123', - '--partition', '123', '--partition', '456']) + '--partition', '123', '--partition', '456', + '--link-check-limit', '3', + ]) mock_relink.assert_called_once_with({ '__file__': mock.ANY, 'swift_dir': 'cli-dir', @@ -351,6 +371,7 @@ class TestRelinker(unittest.TestCase): 'log_name': 'test-relinker', 'policies': {POLICIES[1]}, 'partitions': {123, 456}, + 'link_check_limit': 3, }, mock.ANY, device='sdx') with mock.patch('swift.cli.relinker.relink') as mock_relink, \ @@ -366,6 +387,7 @@ class TestRelinker(unittest.TestCase): 'log_level': 'INFO', 'policies': POLICIES, 'partitions': set(), + 'link_check_limit': 2, }, mock.ANY, device='sdx') mock_logging_config.assert_called_once_with( format='%(message)s', level=logging.INFO, filename=None) @@ -390,6 +412,7 @@ class TestRelinker(unittest.TestCase): 'log_level': 'DEBUG', 'policies': set(POLICIES), 'partitions': set(), + 'link_check_limit': 2 }, mock.ANY, device='sdx') # --debug is now effective mock_logging_config.assert_called_once_with( @@ -456,7 +479,8 @@ class TestRelinker(unittest.TestCase): def _do_link_test(self, command, old_file_specs, new_file_specs, conflict_file_specs, exp_old_specs, exp_new_specs, - exp_ret_code=0, relink_errors=None): + exp_ret_code=0, relink_errors=None, + mock_relink_paths=None, extra_options=None): # Each 'spec' is a tuple (file extension, timestamp offset); files are # created for each old_file_specs and links are created for each in # new_file_specs, then cleanup is run and checks made that @@ -466,12 +490,13 @@ class TestRelinker(unittest.TestCase): # - relink_errors is a dict ext->exception; the exception will be # raised each time relink_paths is called with a target_path ending # with 'ext' + self.assertFalse(relink_errors and mock_relink_paths) # sanity check new_file_specs = [] if new_file_specs is None else new_file_specs conflict_file_specs = ([] if conflict_file_specs is None else conflict_file_specs) exp_old_specs = [] if exp_old_specs is None else exp_old_specs relink_errors = {} if relink_errors is None else relink_errors - + extra_options = extra_options if extra_options else [] # remove the file created by setUp - we'll create it again if wanted os.unlink(self.objname) @@ -491,7 +516,7 @@ class TestRelinker(unittest.TestCase): for filename in old_filenames: filepath = os.path.join(self.objdir, filename) with open(filepath, 'w') as fd: - fd.write(filename) + fd.write(filepath) for filename in new_filenames: new_filepath = os.path.join(self.expected_dir, filename) if filename in old_filenames: @@ -499,22 +524,24 @@ class TestRelinker(unittest.TestCase): os.link(filepath, new_filepath) else: with open(new_filepath, 'w') as fd: - fd.write(filename) + fd.write(new_filepath) for filename in conflict_filenames: new_filepath = os.path.join(self.expected_dir, filename) with open(new_filepath, 'w') as fd: - fd.write(filename) + fd.write(new_filepath) orig_relink_paths = relink_paths - def mock_relink_paths(target_path, new_target_path): + def default_mock_relink_paths(target_path, new_target_path, **kwargs): for ext, error in relink_errors.items(): if target_path.endswith(ext): raise error - return orig_relink_paths(target_path, new_target_path) + return orig_relink_paths(target_path, new_target_path, + **kwargs) with mock.patch('swift.cli.relinker.diskfile.relink_paths', - mock_relink_paths): + mock_relink_paths if mock_relink_paths + else default_mock_relink_paths): with mock.patch.object(relinker.logging, 'getLogger', return_value=self.logger): self.assertEqual(exp_ret_code, relinker.main([ @@ -522,7 +549,7 @@ class TestRelinker(unittest.TestCase): '--swift-dir', self.testdir, '--devices', self.devices, '--skip-mount', - ]), [self.logger.all_log_lines()]) + ] + extra_options), [self.logger.all_log_lines()]) if exp_new_specs: self.assertTrue(os.path.isdir(self.expected_dir)) @@ -701,6 +728,18 @@ class TestRelinker(unittest.TestCase): self.assertIn('1 hash dirs processed (cleanup=False) ' '(0 files, 0 linked, 0 removed, 0 errors)', info_lines) + def test_relink_conflicting_ts_file(self): + self._cleanup_test((('ts', 0),), + None, + (('ts', 0),), # different inode + (('ts', 0),), + (('ts', 0),), + exp_ret_code=1) + info_lines = self.logger.get_lines_for_level('info') + self.assertIn('1 hash dirs processed (cleanup=True) ' + '(1 files, 0 linked, 0 removed, 1 errors)', + info_lines) + def test_relink_link_already_exists_but_different_inode(self): self.rb.prepare_increase_partition_power() self._save_ring() @@ -734,11 +773,12 @@ class TestRelinker(unittest.TestCase): self._save_ring() orig_relink_paths = relink_paths - def mock_relink_paths(target_path, new_target_path): + def mock_relink_paths(target_path, new_target_path, **kwargs): # pretend another process has created the link before this one os.makedirs(self.expected_dir) os.link(target_path, new_target_path) - return orig_relink_paths(target_path, new_target_path) + return orig_relink_paths(target_path, new_target_path, + **kwargs) with mock.patch.object(relinker.logging, 'getLogger', return_value=self.logger): @@ -769,10 +809,11 @@ class TestRelinker(unittest.TestCase): self._save_ring() orig_relink_paths = relink_paths - def mock_relink_paths(target_path, new_target_path): + def mock_relink_paths(target_path, new_target_path, **kwargs): # pretend another process has cleaned up the target path os.unlink(target_path) - return orig_relink_paths(target_path, new_target_path) + return orig_relink_paths(target_path, new_target_path, + **kwargs) with mock.patch.object(relinker.logging, 'getLogger', return_value=self.logger): @@ -1159,6 +1200,359 @@ class TestRelinker(unittest.TestCase): self.assertEqual([], mocked.call_args_list) self.assertEqual(2, res) + def test_relink_conflicting_ts_is_linked_to_part_power(self): + # link from next partition to current partition; + # different file in current-1 partition + self._setup_object(lambda part: part >= 2 ** (PART_POWER - 1)) + self.rb.prepare_increase_partition_power() + self._save_ring() + filename = '.'.join([self.obj_ts.internal, 'ts']) + new_filepath = os.path.join(self.expected_dir, filename) + old_filepath = os.path.join(self.objdir, filename) + # setup a file in the current-1 part power (PART_POWER - 1) location + # that is *not* linked to the file in the next part power location; + # this file should be removed during relink. + older_filepath = utils.replace_partition_in_path( + self.devices, new_filepath, PART_POWER - 1) + os.makedirs(os.path.dirname(older_filepath)) + with open(older_filepath, 'w') as fd: + fd.write(older_filepath) + self._do_link_test('relink', + (('ts', 0),), + (('ts', 0),), + None, + (('ts', 0),), + (('ts', 0),), + exp_ret_code=0) + info_lines = self.logger.get_lines_for_level('info') + self.assertIn( + 'Relinking: cleaning up unwanted file: %s' % older_filepath, + info_lines) + # both the PART_POWER and PART_POWER - N partitions are visited, no new + # links are created, and both the older files are removed + self.assertIn('2 hash dirs processed (cleanup=False) ' + '(2 files, 0 linked, 1 removed, 0 errors)', + info_lines) + print(info_lines) + with open(new_filepath, 'r') as fd: + self.assertEqual(old_filepath, fd.read()) + self.assertFalse(os.path.exists(older_filepath)) + + def test_relink_conflicting_ts_is_linked_to_part_power_minus_1(self): + # link from next partition to current-1 partition; + # different file in current partition + self._setup_object(lambda part: part >= 2 ** (PART_POWER - 1)) + self.rb.prepare_increase_partition_power() + self._save_ring() + # setup a file in the next part power (PART_POWER + 1) location that is + # linked to a file in an older (PART_POWER - 1) location + filename = '.'.join([self.obj_ts.internal, 'ts']) + older_filepath, new_filepath = self._make_link(filename, + PART_POWER - 1) + self._do_link_test('relink', + (('ts', 0),), + None, + None, # we already made file linked to older part + (('ts', 0),), # retained + (('ts', 0),), + exp_ret_code=0) + info_lines = self.logger.get_lines_for_level('info') + # both the PART_POWER and PART_POWER - N partitions are visited, no new + # links are created, and both the older files are retained + self.assertIn('2 hash dirs processed (cleanup=False) ' + '(2 files, 0 linked, 0 removed, 0 errors)', + info_lines) + with open(new_filepath, 'r') as fd: + self.assertEqual(older_filepath, fd.read()) + # prev part power file is retained because it is link target + self.assertTrue(os.path.exists(older_filepath)) + + def test_relink_conflicting_ts_link_to_part_power_minus_1_disappears(self): + # uncommon case: existing link from next partition to current-1 + # partition disappears between an EEXIST while attempting to link + # different file in current partition and checking for an ok link + self._setup_object(lambda part: part >= 2 ** (PART_POWER - 1)) + self.rb.prepare_increase_partition_power() + self._save_ring() + # setup a file in the next part power (PART_POWER + 1) location that is + # linked to a file in an older (PART_POWER - 1) location + filename = '.'.join([self.obj_ts.internal, 'ts']) + older_filepath, new_filepath = self._make_link(filename, + PART_POWER - 1) + old_filepath = os.path.join(self.objdir, filename) + orig_relink_paths = relink_paths + calls = [] + + def mock_relink_paths(target_path, new_target_path, **kwargs): + # while retrying link from current part power to next part power + # location, unlink the conflicting file in next part power location + calls.append((target_path, kwargs)) + if len(calls) == 2: + os.unlink(os.path.join(self.expected_dir, filename)) + return orig_relink_paths(target_path, new_target_path, + **kwargs) + + with mock.patch('swift.cli.relinker.diskfile.relink_paths', + mock_relink_paths): + self._do_link_test('relink', + (('ts', 0),), + None, + None, + (('ts', 0),), # retained + (('ts', 0),), + exp_ret_code=0, + mock_relink_paths=mock_relink_paths) + self.assertEqual( + [(old_filepath, {}), # link attempted - EEXIST raised + (old_filepath, {'ignore_missing': False}), # check makes link! + (older_filepath, {}), # link attempted - EEXIST raised + (old_filepath, {'ignore_missing': False})], # check finds ok link + calls) + info_lines = self.logger.get_lines_for_level('info') + self.assertIn( + 'Relinking: cleaning up unwanted file: %s' % older_filepath, + info_lines) + self.assertIn('2 hash dirs processed (cleanup=False) ' + '(2 files, 1 linked, 1 removed, 0 errors)', + info_lines) + self.assertTrue(os.path.exists(old_filepath)) + with open(new_filepath, 'r') as fd: + self.assertEqual(old_filepath, fd.read()) + # older file is not needed + self.assertFalse(os.path.exists(older_filepath)) + + def test_relink_conflicting_ts_link_to_part_power_disappears(self): + # uncommon case: existing link from next partition to current + # partition disappears between an EEXIST while attempting to link + # different file in current-1 partition and checking for an ok link + self._setup_object(lambda part: part >= 2 ** (PART_POWER - 1)) + self.rb.prepare_increase_partition_power() + self._save_ring() + filename = '.'.join([self.obj_ts.internal, 'ts']) + new_filepath = os.path.join(self.expected_dir, filename) + old_filepath = utils.replace_partition_in_path( + self.devices, new_filepath, PART_POWER) + older_filepath = utils.replace_partition_in_path( + self.devices, new_filepath, PART_POWER - 1) + os.makedirs(os.path.dirname(older_filepath)) + with open(older_filepath, 'w') as fd: + fd.write(older_filepath) + orig_relink_paths = relink_paths + calls = [] + + def mock_relink_paths(target_path, new_target_path, **kwargs): + # while retrying link from current-1 part power to next part power + # location, unlink the conflicting file in next part power location + calls.append((target_path, kwargs)) + if len(calls) == 3: + os.unlink(os.path.join(self.expected_dir, filename)) + return orig_relink_paths(target_path, new_target_path, + **kwargs) + + with mock.patch('swift.cli.relinker.diskfile.relink_paths', + mock_relink_paths): + self._do_link_test('relink', + (('ts', 0),), + (('ts', 0),), + None, + (('ts', 0),), # retained + (('ts', 0),), + exp_ret_code=0, + mock_relink_paths=mock_relink_paths) + self.assertEqual( + [(old_filepath, {}), # link attempted but exists + (older_filepath, {}), # link attempted - EEXIST raised + (old_filepath, {'ignore_missing': False})], # check makes link! + calls) + info_lines = self.logger.get_lines_for_level('info') + self.assertIn('2 hash dirs processed (cleanup=False) ' + '(2 files, 1 linked, 1 removed, 0 errors)', + info_lines) + self.assertTrue(os.path.exists(old_filepath)) + with open(new_filepath, 'r') as fd: + self.assertEqual(old_filepath, fd.read()) + self.assertFalse(os.path.exists(older_filepath)) + + def test_relink_conflicting_ts_is_linked_to_part_power_minus_2_err(self): + # link from next partition to current-2 partition; + # different file in current partition + # by default the relinker will NOT validate the current-2 location + self._setup_object(lambda part: part >= 2 ** (PART_POWER - 1)) + self.rb.prepare_increase_partition_power() + self._save_ring() + # setup a file in the next part power (PART_POWER + 1) location that is + # linked to a file in an older (PART_POWER - 2) location + filename = '.'.join([self.obj_ts.internal, 'ts']) + older_filepath, new_filepath = self._make_link(filename, + PART_POWER - 2) + + self._do_link_test('relink', + (('ts', 0),), + None, + None, # we already made file linked to older part + (('ts', 0),), # retained + (('ts', 0),), + exp_ret_code=1) + info_lines = self.logger.get_lines_for_level('info') + self.assertIn('2 hash dirs processed (cleanup=False) ' + '(2 files, 0 linked, 0 removed, 1 errors)', + info_lines) + warning_lines = self.logger.get_lines_for_level('warning') + self.assertIn('Error relinking: failed to relink', warning_lines[0]) + with open(new_filepath, 'r') as fd: + self.assertEqual(older_filepath, fd.read()) + # prev-1 part power file is always retained because it is link target + self.assertTrue(os.path.exists(older_filepath)) + + def test_relink_conflicting_ts_is_linked_to_part_power_minus_2_ok(self): + # link from next partition to current-2 partition; + # different file in current partition + # increase link_check_limit so that the relinker WILL validate the + # current-2 location + self._setup_object(lambda part: part >= 2 ** (PART_POWER - 1)) + self.rb.prepare_increase_partition_power() + self._save_ring() + # setup a file in the next part power (PART_POWER + 1) location that is + # linked to a file in an older (PART_POWER - 2) location + filename = '.'.join([self.obj_ts.internal, 'ts']) + older_filepath, new_filepath = self._make_link(filename, + PART_POWER - 2) + self._do_link_test('relink', + (('ts', 0),), + None, + None, # we already made file linked to older part + (('ts', 0),), # retained + (('ts', 0),), + exp_ret_code=0, + extra_options=['--link-check-limit', '3']) + info_lines = self.logger.get_lines_for_level('info') + self.assertIn('2 hash dirs processed (cleanup=False) ' + '(2 files, 0 linked, 0 removed, 0 errors)', + info_lines) + warning_lines = self.logger.get_lines_for_level('warning') + self.assertEqual([], warning_lines) + with open(new_filepath, 'r') as fd: + self.assertEqual(older_filepath, fd.read()) + # prev-1 part power file is retained because it is link target + self.assertTrue(os.path.exists(older_filepath)) + + def test_relink_conflicting_ts_both_in_older_part_powers(self): + # link from next partition to current-1 partition; + # different file in current partition + # different file in current-2 location + self._setup_object(lambda part: part >= 2 ** (PART_POWER - 2)) + self.rb.prepare_increase_partition_power() + self._save_ring() + # setup a file in the next part power (PART_POWER + 1) location that is + # linked to a file in an older (PART_POWER - 1) location + filename = '.'.join([self.obj_ts.internal, 'ts']) + older_filepath, new_filepath = self._make_link(filename, + PART_POWER - 1) + # setup a file in an even older part power (PART_POWER - 2) location + # that is *not* linked to the file in the next part power location; + # this file should be removed during relink. + oldest_filepath = utils.replace_partition_in_path( + self.devices, new_filepath, PART_POWER - 2) + os.makedirs(os.path.dirname(oldest_filepath)) + with open(oldest_filepath, 'w') as fd: + fd.write(oldest_filepath) + + self._do_link_test('relink', + (('ts', 0),), + None, + None, # we already made file linked to older part + (('ts', 0),), # retained + (('ts', 0),), + exp_ret_code=0) + info_lines = self.logger.get_lines_for_level('info') + # both the PART_POWER and PART_POWER - N partitions are visited, no new + # links are created, and both the older files are retained + self.assertIn('3 hash dirs processed (cleanup=False) ' + '(3 files, 0 linked, 1 removed, 0 errors)', + info_lines) + with open(new_filepath, 'r') as fd: + self.assertEqual(older_filepath, fd.read()) + self.assertTrue(os.path.exists(older_filepath)) # linked so retained + self.assertFalse(os.path.exists(oldest_filepath)) + + def test_relink_conflicting_ts_is_not_linked_link_check_limit_zero(self): + # different file in next part power partition, so all attempts to link + # fail, check no rechecks made with zero link-check-limit + self._setup_object(lambda part: part >= 2 ** (PART_POWER - 1)) + self.rb.prepare_increase_partition_power() + self._save_ring() + filename = '.'.join([self.obj_ts.internal, 'ts']) + current_filepath = os.path.join(self.objdir, filename) + orig_relink_paths = relink_paths + calls = [] + + def mock_relink_paths(target_path, new_target_path, **kwargs): + calls.append((target_path, kwargs)) + return orig_relink_paths(target_path, new_target_path, + **kwargs) + + self._do_link_test( + 'relink', + (('ts', 0),), + None, + (('ts', 0),), + (('ts', 0),), + (('ts', 0),), + exp_ret_code=1, + extra_options=['--link-check-limit', '0'], + mock_relink_paths=mock_relink_paths, + ) + info_lines = self.logger.get_lines_for_level('info') + self.assertIn('1 hash dirs processed (cleanup=False) ' + '(1 files, 0 linked, 0 removed, 1 errors)', + info_lines) + + # one attempt to link from current plus a check/retry from each + # possible partition power, stopping at part power 1 + expected = [(current_filepath, {})] + self.assertEqual(expected, calls) + + def test_relink_conflicting_ts_is_not_linked_link_check_limit_absurd(self): + # different file in next part power partition, so all attempts to link + # fail, check that absurd link-check-limit gets capped + self._setup_object(lambda part: part >= 2 ** (PART_POWER - 1)) + self.rb.prepare_increase_partition_power() + self._save_ring() + filename = '.'.join([self.obj_ts.internal, 'ts']) + current_filepath = os.path.join(self.objdir, filename) + orig_relink_paths = relink_paths + calls = [] + + def mock_relink_paths(target_path, new_target_path, **kwargs): + calls.append((target_path, kwargs)) + return orig_relink_paths(target_path, new_target_path, + **kwargs) + + self._do_link_test( + 'relink', + (('ts', 0),), + None, + (('ts', 0),), + (('ts', 0),), + (('ts', 0),), + exp_ret_code=1, + extra_options=['--link-check-limit', str(PART_POWER + 99)], + mock_relink_paths=mock_relink_paths, + ) + info_lines = self.logger.get_lines_for_level('info') + self.assertIn('1 hash dirs processed (cleanup=False) ' + '(1 files, 0 linked, 0 removed, 1 errors)', + info_lines) + + # one attempt to link from current plus a check/retry from each + # possible partition power, stopping at part power 1 + expected = [(current_filepath, {})] + [ + (utils.replace_partition_in_path( + self.devices, current_filepath, part_power), + {'ignore_missing': False}) + for part_power in range(PART_POWER, -1, -1)] + self.assertEqual(expected, calls) + @patch_policies( [StoragePolicy(0, name='gold', is_default=True), ECStoragePolicy(1, name='platinum', ec_type=DEFAULT_TEST_EC_TYPE, @@ -1518,6 +1912,100 @@ class TestRelinker(unittest.TestCase): '(1 files, 0 linked, 0 removed, 1 errors)', info_lines) + def test_cleanup_conflicting_ts_is_linked_to_part_power_minus_1(self): + self._setup_object(lambda part: part >= 2 ** (PART_POWER - 1)) + self.rb.prepare_increase_partition_power() + self.rb.increase_partition_power() + self._save_ring() + # setup a file in the next part power (PART_POWER + 1) location that is + # linked to a file in an older PART_POWER - 1 location + filename = '.'.join([self.obj_ts.internal, 'ts']) + older_filepath, new_filepath = self._make_link(filename, + PART_POWER - 1) + self._do_link_test('cleanup', + (('ts', 0),), + None, + None, # we already made file linked to older part + None, + (('ts', 0),), + exp_ret_code=0) + info_lines = self.logger.get_lines_for_level('info') + # both the PART_POWER and PART_POWER - N partitions are visited, no new + # links are created, and both the older files are removed + self.assertIn('2 hash dirs processed (cleanup=True) ' + '(2 files, 0 linked, 2 removed, 0 errors)', + info_lines) + with open(new_filepath, 'r') as fd: + self.assertEqual(older_filepath, fd.read()) + self.assertFalse(os.path.exists(older_filepath)) + + def test_cleanup_conflicting_ts_is_linked_to_part_power_minus_2_err(self): + # link from next partition to current-2 partition; + # different file in current partition + # by default the relinker will NOT validate the current-2 location + self._setup_object(lambda part: part >= 2 ** (PART_POWER - 1)) + self.rb.prepare_increase_partition_power() + self.rb.increase_partition_power() + self._save_ring() + # setup a file in the next part power (PART_POWER + 1) location that is + # linked to a file in an older (PART_POWER - 2) location + filename = '.'.join([self.obj_ts.internal, 'ts']) + older_filepath, new_filepath = self._make_link(filename, + PART_POWER - 2) + + self._do_link_test('cleanup', + (('ts', 0),), + None, + None, # we already made file linked to older part + (('ts', 0),), # retained + (('ts', 0),), + exp_ret_code=1) + info_lines = self.logger.get_lines_for_level('info') + self.assertIn('2 hash dirs processed (cleanup=True) ' + '(2 files, 0 linked, 1 removed, 1 errors)', + info_lines) + warning_lines = self.logger.get_lines_for_level('warning') + self.assertIn('Error relinking (cleanup): failed to relink', + warning_lines[0]) + with open(new_filepath, 'r') as fd: + self.assertEqual(older_filepath, fd.read()) + # current-2 is linked so can be removed in cleanup + self.assertFalse(os.path.exists(older_filepath)) + + def test_cleanup_conflicting_ts_is_linked_to_part_power_minus_2_ok(self): + # link from next partition to current-2 partition; + # different file in current partition + # increase link_check_limit so that the relinker WILL validate the + # current-2 location + self._setup_object(lambda part: part >= 2 ** (PART_POWER - 1)) + self.rb.prepare_increase_partition_power() + self.rb.increase_partition_power() + self._save_ring() + # setup a file in the next part power (PART_POWER + 1) location that is + # linked to a file in an older (PART_POWER - 2) location + filename = '.'.join([self.obj_ts.internal, 'ts']) + older_filepath, new_filepath = self._make_link(filename, + PART_POWER - 2) + self._do_link_test('cleanup', + (('ts', 0),), + None, + None, # we already made file linked to older part + None, + (('ts', 0),), + exp_ret_code=0, + extra_options=['--link-check-limit', '3']) + info_lines = self.logger.get_lines_for_level('info') + # both the PART_POWER and PART_POWER - N partitions are visited, no new + # links are created, and both the older files are removed + self.assertIn('2 hash dirs processed (cleanup=True) ' + '(2 files, 0 linked, 2 removed, 0 errors)', + info_lines) + warning_lines = self.logger.get_lines_for_level('warning') + self.assertEqual([], warning_lines) + with open(new_filepath, 'r') as fd: + self.assertEqual(older_filepath, fd.read()) + self.assertFalse(os.path.exists(older_filepath)) + def test_cleanup_conflicting_older_data_file(self): # older conflicting file isn't relevant so cleanup succeeds self._cleanup_test((('data', 0),), diff --git a/test/unit/common/test_utils.py b/test/unit/common/test_utils.py index cf01ce90b3..9a870d33cf 100644 --- a/test/unit/common/test_utils.py +++ b/test/unit/common/test_utils.py @@ -4439,6 +4439,8 @@ cluster_dfw1 = http://dfw1.host/v1/ self.assertEqual(350, utils.get_partition_for_hash(hex_hash, 9)) self.assertEqual(700, utils.get_partition_for_hash(hex_hash, 10)) self.assertEqual(1400, utils.get_partition_for_hash(hex_hash, 11)) + self.assertEqual(0, utils.get_partition_for_hash(hex_hash, 0)) + self.assertEqual(0, utils.get_partition_for_hash(hex_hash, -1)) def test_replace_partition_in_path(self): # Check for new part = part * 2 diff --git a/test/unit/obj/test_diskfile.py b/test/unit/obj/test_diskfile.py index d9a8788a3c..f1e4c935c0 100644 --- a/test/unit/obj/test_diskfile.py +++ b/test/unit/obj/test_diskfile.py @@ -223,7 +223,11 @@ class TestDiskFileModuleMethods(unittest.TestCase): side_effect=Exception('oops')): with self.assertRaises(Exception) as cm: diskfile.relink_paths(target_path, new_target_path) - self.assertEqual('oops', str(cm.exception)) + self.assertEqual('oops', str(cm.exception)) + with self.assertRaises(Exception) as cm: + diskfile.relink_paths(target_path, new_target_path, + ignore_missing=False) + self.assertEqual('oops', str(cm.exception)) def test_relink_paths_makedirs_race(self): # test two concurrent relinks of the same object hash dir with race @@ -313,6 +317,12 @@ class TestDiskFileModuleMethods(unittest.TestCase): self.assertFalse(os.path.exists(target_path)) self.assertFalse(os.path.exists(new_target_path)) self.assertFalse(created) + with self.assertRaises(OSError) as cm: + diskfile.relink_paths(target_path, new_target_path, + ignore_missing=False) + self.assertEqual(errno.ENOENT, cm.exception.errno) + self.assertFalse(os.path.exists(target_path)) + self.assertFalse(os.path.exists(new_target_path)) def test_relink_paths_os_link_race(self): # test two concurrent relinks of the same object hash dir with race