diff --git a/swift/cli/relinker.py b/swift/cli/relinker.py index 19a59f8fe5..f35a7e16b8 100644 --- a/swift/cli/relinker.py +++ b/swift/cli/relinker.py @@ -300,7 +300,8 @@ def relink(conf, logger, device): for fname, _, _ in locations: newfname = replace_partition_in_path(fname, next_part_power) try: - diskfile.relink_paths(fname, newfname, check_existing=True) + logger.debug('Relinking %s to %s', fname, newfname) + diskfile.relink_paths(fname, newfname) relinked += 1 suffix_dir = os.path.dirname(os.path.dirname(newfname)) diskfile.invalidate_hash(suffix_dir) diff --git a/swift/obj/diskfile.py b/swift/obj/diskfile.py index aa3bb84c62..6423d02530 100644 --- a/swift/obj/diskfile.py +++ b/swift/obj/diskfile.py @@ -443,20 +443,20 @@ def invalidate_hash(suffix_dir): inv_fh.write(suffix + b"\n") -def relink_paths(target_path, new_target_path, check_existing=False): +def relink_paths(target_path, new_target_path): """ 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 check_existing: if True, check whether the link is already present - before attempting to create a new one + :raises: OSError if the hard link could not be created, unless the intended + hard link already exists or the target_path does not exist. + :returns: True if the link was created by the call to this method, False + otherwise. """ - + link_created = False if target_path != new_target_path: - logging.debug('Relinking %s to %s due to next_part_power set', - target_path, new_target_path) new_target_dir = os.path.dirname(new_target_path) try: os.makedirs(new_target_dir) @@ -464,17 +464,33 @@ def relink_paths(target_path, new_target_path, check_existing=False): if err.errno != errno.EEXIST: raise - link_exists = False - if check_existing: - try: - new_stat = os.stat(new_target_path) - orig_stat = os.stat(target_path) - link_exists = (new_stat.st_ino == orig_stat.st_ino) - except OSError: - pass # if anything goes wrong, try anyway - - if not link_exists: + try: os.link(target_path, new_target_path) + link_created = True + except OSError as err: + # there are some circumstances in which it may be ok that the + # attempted link failed + 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) + 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 + else: + try: + new_stat = os.stat(new_target_path) + ok = new_stat.st_ino == orig_stat.st_ino + except OSError: + # squash this exception; the original will be raised + pass + if not ok: + raise err + return link_created def get_part_path(dev_path, policy, partition): @@ -1845,6 +1861,9 @@ class BaseDiskFileWriter(object): if target_path != new_target_path: try: fsync_dir(os.path.dirname(target_path)) + self.manager.logger.debug( + 'Relinking %s to %s due to next_part_power set', + target_path, new_target_path) relink_paths(target_path, new_target_path) except OSError as exc: self.manager.logger.exception( diff --git a/test/unit/cli/test_relinker.py b/test/unit/cli/test_relinker.py index ee5c7183c5..eab781c3cf 100644 --- a/test/unit/cli/test_relinker.py +++ b/test/unit/cli/test_relinker.py @@ -37,7 +37,8 @@ from swift.common.exceptions import PathNotDir from swift.common.storage_policy import ( StoragePolicy, StoragePolicyCollection, POLICIES, ECStoragePolicy) -from swift.obj.diskfile import write_metadata, DiskFileRouter, DiskFileManager +from swift.obj.diskfile import write_metadata, DiskFileRouter, \ + DiskFileManager, relink_paths from test.unit import FakeLogger, skip_if_no_xattrs, DEFAULT_TEST_EC_TYPE, \ patch_policies @@ -424,6 +425,58 @@ class TestRelinker(unittest.TestCase): self.assertFalse(os.path.exists( os.path.join(self.part_dir, 'hashes.invalid'))) + def test_relink_link_already_exists(self): + self.rb.prepare_increase_partition_power() + self._save_ring() + orig_relink_paths = relink_paths + + def mock_relink_paths(target_path, new_target_path): + # pretend another process has created the link before this one + os.makedirs(self.expected_dir) + os.link(target_path, new_target_path) + orig_relink_paths(target_path, new_target_path) + + with mock.patch('swift.cli.relinker.diskfile.relink_paths', + mock_relink_paths): + self.assertEqual(0, relinker.main([ + 'relink', + '--swift-dir', self.testdir, + '--devices', self.devices, + '--skip-mount', + ])) + + self.assertTrue(os.path.isdir(self.expected_dir)) + self.assertTrue(os.path.isfile(self.expected_file)) + stat_old = os.stat(os.path.join(self.objdir, self.object_fname)) + stat_new = os.stat(self.expected_file) + self.assertEqual(stat_old.st_ino, stat_new.st_ino) + + def test_relink_link_target_disappears(self): + # we need object name in lower half of current part so that there is no + # rehash of the new partition which wold erase the empty new partition + # - we want to assert it was created + self._setup_object(lambda part: part < 2 ** (PART_POWER - 1)) + self.rb.prepare_increase_partition_power() + self._save_ring() + orig_relink_paths = relink_paths + + def mock_relink_paths(target_path, new_target_path): + # pretend another process has cleaned up the target path + os.unlink(target_path) + orig_relink_paths(target_path, new_target_path) + + with mock.patch('swift.cli.relinker.diskfile.relink_paths', + mock_relink_paths): + self.assertEqual(0, relinker.main([ + 'relink', + '--swift-dir', self.testdir, + '--devices', self.devices, + '--skip-mount', + ])) + + self.assertTrue(os.path.isdir(self.expected_dir)) + self.assertFalse(os.path.isfile(self.expected_file)) + def test_relink_no_applicable_policy(self): # NB do not prepare part power increase self._save_ring() diff --git a/test/unit/obj/test_diskfile.py b/test/unit/obj/test_diskfile.py index b80ce3f892..3727191d6d 100644 --- a/test/unit/obj/test_diskfile.py +++ b/test/unit/obj/test_diskfile.py @@ -207,7 +207,8 @@ class TestDiskFileModuleMethods(unittest.TestCase): with open(target_path, 'w') as fd: fd.write('junk') new_target_path = os.path.join(self.testdir, 'd2', 'f1') - diskfile.relink_paths(target_path, new_target_path) + created = diskfile.relink_paths(target_path, new_target_path) + self.assertTrue(created) self.assertTrue(os.path.isfile(new_target_path)) with open(new_target_path, 'r') as fd: self.assertEqual('junk', fd.read()) @@ -225,8 +226,9 @@ class TestDiskFileModuleMethods(unittest.TestCase): diskfile.relink_paths(target_path, new_target_path) self.assertEqual('oops', str(cm.exception)) - def test_relink_paths_race(self): - # test two concurrent relinks of the same object hash dir + def test_relink_paths_makedirs_race(self): + # test two concurrent relinks of the same object hash dir with race + # around makedirs target_dir = os.path.join(self.testdir, 'd1') # target dir exists os.mkdir(target_dir) @@ -236,31 +238,34 @@ class TestDiskFileModuleMethods(unittest.TestCase): new_target_dir = os.path.join(self.testdir, 'd2') new_target_path_1 = os.path.join(new_target_dir, 't1.data') new_target_path_2 = os.path.join(new_target_dir, 't2.data') + created = [] def write_and_relink(target_path, new_target_path): with open(target_path, 'w') as fd: fd.write(target_path) - diskfile.relink_paths(target_path, new_target_path) + created.append(diskfile.relink_paths(target_path, new_target_path)) calls = [] orig_makedirs = os.makedirs - def mock_makedirs(path): + def mock_makedirs(path, *args): calls.append(path) if len(calls) == 1: # pretend another process jumps in here and relinks same dirs write_and_relink(target_path_2, new_target_path_2) - return orig_makedirs(path) + return orig_makedirs(path, *args) with mock.patch('swift.obj.diskfile.os.makedirs', mock_makedirs): write_and_relink(target_path_1, new_target_path_1) + self.assertEqual([new_target_dir, new_target_dir], calls) self.assertTrue(os.path.isfile(new_target_path_1)) with open(new_target_path_1, 'r') as fd: self.assertEqual(target_path_1, fd.read()) self.assertTrue(os.path.isfile(new_target_path_2)) with open(new_target_path_2, 'r') as fd: self.assertEqual(target_path_2, fd.read()) + self.assertEqual([True, True], created) def test_relink_paths_object_dir_exists_but_not_dir(self): target_dir = os.path.join(self.testdir, 'd1') @@ -285,6 +290,149 @@ class TestDiskFileModuleMethods(unittest.TestCase): diskfile.relink_paths(target_path, new_target_path) self.assertEqual(errno.ENOTDIR, cm.exception.errno) + def test_relink_paths_os_link_error(self): + # check relink_paths raises exception from os.link + target_dir = os.path.join(self.testdir, 'd1') + os.mkdir(target_dir) + target_path = os.path.join(target_dir, 'f1') + with open(target_path, 'w') as fd: + fd.write('junk') + new_target_path = os.path.join(self.testdir, 'd2', 'f1') + with mock.patch('swift.obj.diskfile.os.link', + side_effect=OSError(errno.EPERM, 'nope')): + with self.assertRaises(Exception) as cm: + diskfile.relink_paths(target_path, new_target_path) + self.assertEqual(errno.EPERM, cm.exception.errno) + + def test_relink_paths_target_path_does_not_exist(self): + # check relink_paths does not raise exception + target_dir = os.path.join(self.testdir, 'd1') + os.mkdir(target_dir) + target_path = os.path.join(target_dir, 'f1') + new_target_path = os.path.join(self.testdir, 'd2', 'f1') + created = diskfile.relink_paths(target_path, new_target_path) + self.assertFalse(os.path.exists(target_path)) + self.assertFalse(os.path.exists(new_target_path)) + self.assertFalse(created) + + def test_relink_paths_os_link_race(self): + # test two concurrent relinks of the same object hash dir with race + # around os.link + target_dir = os.path.join(self.testdir, 'd1') + # target dir exists + os.mkdir(target_dir) + target_path = os.path.join(target_dir, 't1.data') + # new target dir and file do not exist + new_target_dir = os.path.join(self.testdir, 'd2') + new_target_path = os.path.join(new_target_dir, 't1.data') + created = [] + + def write_and_relink(target_path, new_target_path): + with open(target_path, 'w') as fd: + fd.write(target_path) + created.append(diskfile.relink_paths(target_path, new_target_path)) + + calls = [] + orig_link = os.link + + def mock_link(path, new_path): + calls.append((path, new_path)) + if len(calls) == 1: + # pretend another process jumps in here and links same files + write_and_relink(target_path, new_target_path) + return orig_link(path, new_path) + + with mock.patch('swift.obj.diskfile.os.link', mock_link): + write_and_relink(target_path, new_target_path) + + self.assertEqual([(target_path, new_target_path)] * 2, calls) + self.assertTrue(os.path.isfile(new_target_path)) + with open(new_target_path, 'r') as fd: + self.assertEqual(target_path, fd.read()) + with open(target_path, 'r') as fd: + self.assertEqual(target_path, fd.read()) + self.assertEqual([True, False], created) + + def test_relink_paths_different_file_exists(self): + # check for an exception if a hard link cannot be made because a + # different file already exists at new_target_path + target_dir = os.path.join(self.testdir, 'd1') + # target dir and file exists + os.mkdir(target_dir) + target_path = os.path.join(target_dir, 't1.data') + with open(target_path, 'w') as fd: + fd.write(target_path) + # new target dir and different file exist + new_target_dir = os.path.join(self.testdir, 'd2') + os.mkdir(new_target_dir) + new_target_path = os.path.join(new_target_dir, 't1.data') + with open(new_target_path, 'w') as fd: + fd.write(new_target_path) + + with self.assertRaises(OSError) as cm: + diskfile.relink_paths(target_path, new_target_path) + + self.assertEqual(errno.EEXIST, cm.exception.errno) + # check nothing got deleted... + self.assertTrue(os.path.isfile(target_path)) + with open(target_path, 'r') as fd: + self.assertEqual(target_path, fd.read()) + self.assertTrue(os.path.isfile(new_target_path)) + with open(new_target_path, 'r') as fd: + self.assertEqual(new_target_path, fd.read()) + + def test_relink_paths_same_file_exists(self): + # check for no exception if a hard link cannot be made because a link + # to the same file already exists at the path + target_dir = os.path.join(self.testdir, 'd1') + # target dir and file exists + os.mkdir(target_dir) + target_path = os.path.join(target_dir, 't1.data') + with open(target_path, 'w') as fd: + fd.write(target_path) + # new target dir and link to same file exist + new_target_dir = os.path.join(self.testdir, 'd2') + os.mkdir(new_target_dir) + new_target_path = os.path.join(new_target_dir, 't1.data') + os.link(target_path, new_target_path) + with open(new_target_path, 'r') as fd: + self.assertEqual(target_path, fd.read()) # sanity check + + # existing link checks ok + created = diskfile.relink_paths(target_path, new_target_path) + with open(new_target_path, 'r') as fd: + self.assertEqual(target_path, fd.read()) # sanity check + self.assertFalse(created) + + # now pretend there is an error when checking that the link already + # exists - expect the EEXIST exception to be raised + orig_stat = os.stat + + def mocked_stat(path): + if path == new_target_path: + raise OSError(errno.EPERM, 'cannot be sure link exists :(') + return orig_stat(path) + + with mock.patch('swift.obj.diskfile.os.stat', mocked_stat): + with self.assertRaises(OSError) as cm: + diskfile.relink_paths(target_path, new_target_path) + self.assertEqual(errno.EEXIST, cm.exception.errno, str(cm.exception)) + with open(new_target_path, 'r') as fd: + self.assertEqual(target_path, fd.read()) # sanity check + + # ...unless while checking for an existing link the target file is + # found to no longer exists, which is ok + def mocked_stat(path): + if path == target_path: + raise OSError(errno.ENOENT, 'target longer here :)') + return orig_stat(path) + + with mock.patch('swift.obj.diskfile.os.stat', mocked_stat): + created = diskfile.relink_paths(target_path, new_target_path) + with open(new_target_path, 'r') as fd: + self.assertEqual(target_path, fd.read()) # sanity check + self.assertFalse(created) + def test_extract_policy(self): # good path names pn = 'objects/0/606/1984527ed7ef6247c78606/1401379842.14643.data' diff --git a/test/unit/obj/test_server.py b/test/unit/obj/test_server.py index a041d4f5fe..21e1a4a2ed 100644 --- a/test/unit/obj/test_server.py +++ b/test/unit/obj/test_server.py @@ -2921,12 +2921,61 @@ class TestObjectController(unittest.TestCase): self.assertFalse(os.path.isfile(data_file(new_part, ts_1))) self.assertFalse(os.path.isfile(data_file(old_part, ts_1))) error_lines = self.logger.get_lines_for_level('error') - # the older request's data file in the old partition will have been - # cleaned up by the newer request, so it's attempt to relink will fail - self.assertEqual(1, len(error_lines)) - self.assertIn(ts_1.internal + '.data failed: ' - '[Errno 2] No such file or directory', - error_lines[0]) + self.assertEqual([], error_lines) + + def test_PUT_next_part_power_races_around_makedirs_enoent(self): + hash_path_ = hash_path('a', 'c', 'o') + part_power = 10 + old_part = utils.get_partition_for_hash(hash_path_, part_power) + new_part = utils.get_partition_for_hash(hash_path_, part_power + 1) + policy = POLICIES.default + + def make_request(timestamp): + headers = {'X-Timestamp': timestamp.internal, + 'Content-Length': '6', + 'Content-Type': 'application/octet-stream', + 'X-Backend-Storage-Policy-Index': int(policy), + 'X-Backend-Next-Part-Power': part_power + 1} + req = Request.blank( + '/sda1/%s/a/c/o' % old_part, method='PUT', + headers=headers, body=b'VERIFY') + resp = req.get_response(self.object_controller) + self.assertEqual(resp.status_int, 201) + + def data_file(part, timestamp): + return os.path.join( + self.testdir, 'sda1', + storage_directory(diskfile.get_data_dir(int(policy)), + part, hash_path_), + timestamp.internal + '.data') + + ts_1 = next(self.ts) + ts_2 = next(self.ts) + calls = [] + orig_makedirs = os.makedirs + + def mock_makedirs(path, *args, **kwargs): + # let another request race ahead just as the first is about to + # create the next part power object dir + if path == os.path.dirname(data_file(new_part, ts_1)): + calls.append(path) + if len(calls) == 1: + # pretend 'yield' to other request process + make_request(ts_2) + return orig_makedirs(path, *args, **kwargs) + + with mock.patch('swift.obj.diskfile.os.makedirs', mock_makedirs): + make_request(ts_1) + + self.assertEqual( + [os.path.dirname(data_file(new_part, ts_1)), + os.path.dirname(data_file(new_part, ts_1))], calls) + self.assertTrue(os.path.isfile(data_file(old_part, ts_2))) + self.assertTrue(os.path.isfile(data_file(new_part, ts_2))) + self.assertFalse(os.path.isfile(data_file(new_part, ts_1))) + self.assertFalse(os.path.isfile(data_file(old_part, ts_1))) + error_lines = self.logger.get_lines_for_level('error') + self.assertEqual([], error_lines) def test_HEAD(self): # Test swift.obj.server.ObjectController.HEAD