Merge "relinker: allow clobber-hardlink-collision"

etc/object-server.conf-sample

@@ -793,3 +793,12 @@ use = egg:swift#xprofile
 #
 # stats_interval = 300.0
 # recon_cache_path = /var/cache/swift
+#
+# Because of the way old versions of swift on old kernels worked you may end up
+# with a file in the new part dir path that had the exact timestamp of a
+# "newer" file in the current part dir.  With this option enabled during the
+# relink phase we'll quarantine the colliding file in the new target part dir
+# and retry the relink.  During the cleanup phase we ignore the un-matched
+# inode "collision" and allow the cleanup of the old file in the old part dir
+# same as tombstones.
+# clobber_hardlink_collisions = false

swift/cli/relinker.py

@@ -395,11 +395,128 @@ class Relinker(object):
                 hashes.remove(hsh)
         return hashes
 
-    def process_location(self, hash_path, new_hash_path):
+    def do_relink(self, device, hash_path, new_hash_path, filename,
-        # Compare the contents of each hash dir with contents of same hash
+                  already_quarantined=False):
-        # dir in its new partition to verify that the new location has the
+        """
-        # most up to date set of files. The new location may have newer
+        Attempt to relink a file from old location to new location.
-        # files if it has been updated since relinked.
+
+        :param device: device name
+        :param hash_path: source hash directory path
+        :param new_hash_path: destination hash directory path
+        :param filename: filename to relink
+        :param already_quarantined: whether quarantine has already been
+                                    attempted
+        :returns: tuple of (success, created) where success is True if the link
+                  is successfully verified, and created is True if a new link
+                  needed to be created for successful verification (if created
+                  is True for any new link in any hash_path some caller above
+                  us should ideally invalidate the whole suffix)
+        """
+        old_file = os.path.join(hash_path, filename)
+        new_file = os.path.join(new_hash_path, filename)
+        success = created = False
+        try:
+            created = diskfile.relink_paths(old_file, new_file)
+            success = True
+        except FileExistsError:
+            # we've detected a hardlink collision, so we need to handle it
+            # depending on what kind of file it is and our mode and
+            # configuration
+            if filename.endswith('.ts'):
+                # special case for duplicate tombstones, see:
+                # https://bugs.launchpad.net/swift/+bug/1921718
+                # https://bugs.launchpad.net/swift/+bug/1934142
+                self.logger.debug(
+                    "Relinking%s: tolerating different inodes for "
+                    "tombstone with same timestamp: %s to %s",
+                    ' (cleanup)' if self.do_cleanup else '',
+                    old_file, new_file)
+                success = True
+            elif self.conf['clobber_hardlink_collisions']:
+                if self.do_cleanup:
+                    # At this point your clients are already *in* the new part
+                    # dir, if the "better" data was in the old part dir you're
+                    # already hurting and maybe flipped back to retry the
+                    # relink phase again?  If you're moving forward with the
+                    # cleanup presumably you're ready for this circus to be
+                    # over and doing extra io to quarantine the data you're
+                    # currently using and replace it with old data seems less
+                    # attractive than letting the un-referenced data get
+                    # cleaned up.  But there might be a case to argue that
+                    # clobber_hardlink_collision should quarantine old_file
+                    # here before returning success.
+                    self.logger.debug(
+                        "Relinking%s: tolerating hardlink collision: "
+                        "%s to %s",
+                        ' (cleanup)' if self.do_cleanup else '',
+                        old_file, new_file)
+                    success = True
+                elif already_quarantined:
+                    # Already attempted quarantine, this is a failure, but user
+                    # can retry (or already_quarantined becomes a counter?)
+                    # N.B. this can exit non-zero w/o logging at "error"
+                    self.logger.warning(
+                        "Relinking%s: hardlink collision persists after "
+                        "quarantine: %s to %s",
+                        ' (cleanup)' if self.do_cleanup else '',
+                        old_file, new_file)
+                else:
+                    # During relink phase, quarantine and retry once
+                    dev_path = os.path.join(self.diskfile_mgr.devices, device)
+                    to_dir = diskfile.quarantine_renamer(dev_path, new_file)
+                    self.logger.info(
+                        "Relinking%s: clobbering hardlink collision: "
+                        "%s moved to %s",
+                        ' (cleanup)' if self.do_cleanup else '',
+                        new_file, to_dir)
+                    # retry with quarantine flag set
+                    return self.do_relink(
+                        device, hash_path, new_hash_path, filename,
+                        already_quarantined=True)
+            else:
+                self.logger.error(
+                    "Error relinking%s: hardlink collision: "
+                    "%s to %s (consider enabling clobber_hardlink_collisions)",
+                    ' (cleanup)' if self.do_cleanup else '',
+                    old_file, new_file)
+        except Exception as exc:
+            # Depending on what kind of errors these are, it might be
+            # reasonable to consider them "warnings" if we expect re-running
+            # the relinker would be able to fix them (like if it's just a
+            # general file-system corruption error and your auditor is still
+            # running maybe it will quarantine bad paths to clear the way).
+            # But AFAIK all currently known/observed error conditions are
+            # enumerated above and any unknown error conditions may not be
+            # fixable by simply re-running the relinker: so we log them as
+            # error to match the expected non-zero return code.
+            self.logger.error(
+                "Error relinking%s: failed to relink %s to %s: %s",
+                ' (cleanup)' if self.do_cleanup else '',
+                old_file, new_file, exc)
+        if created:
+            self.logger.debug(
+                "Relinking%s created link: %s to %s",
+                ' (cleanup)' if self.do_cleanup else '',
+                old_file, new_file)
+        return success, created
+
+    def process_location(self, device, hash_path, new_hash_path):
+        """
+        Handle relink of all files in a hash_dir 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
+        most up to date set of files. The new location may have newer
+        files if it has been updated since relinked.
+
+        If any new links are created the suffix will be invalidated.
+        In cleanup mode, the unwanted files in the old hash_path will be
+        removed as long as there are no errors.
+
+        :param device: device name
+        :param hash_path: old hash directory path
+        :param new_hash_path: new hash directory path
+        """
         self.stats['hash_dirs'] += 1
 
         # Get on disk data for new and old locations, cleaning up any
@@ -449,33 +566,15 @@ class Relinker(object):
             #    no longer required. The new file will eventually be
             #    cleaned up again.
             self.stats['files'] += 1
-            old_file = os.path.join(hash_path, filename)
+            success, created = self.do_relink(
-            new_file = os.path.join(new_hash_path, filename)
+                device, hash_path, new_hash_path, filename)
-            try:
+            if success:
-                if diskfile.relink_paths(old_file, new_file):
+                if created:
-                    self.logger.debug(
-                        "Relinking%s created link: %s to %s",
-                        ' (cleanup)' if self.do_cleanup else '',
-                        old_file, new_file)
                     created_links += 1
                     self.stats['linked'] += 1
-            except OSError as exc:
+            else:
-                if exc.errno == errno.EEXIST and filename.endswith('.ts'):
+                self.stats['errors'] += 1
-                    # special case for duplicate tombstones, see:
+                missing_links += 1
-                    # https://bugs.launchpad.net/swift/+bug/1921718
-                    # https://bugs.launchpad.net/swift/+bug/1934142
-                    self.logger.debug(
-                        "Relinking%s: tolerating different inodes for "
-                        "tombstone with same timestamp: %s to %s",
-                        ' (cleanup)' if self.do_cleanup else '',
-                        old_file, new_file)
-                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:
             self.linked_into_partitions.add(get_partition_from_path(
                 self.conf['devices'], new_hash_path))
@@ -503,6 +602,8 @@ class Relinker(object):
             try:
                 os.remove(old_file)
             except OSError as exc:
+                # N.B. if we want to allow old_file to get quarantined this
+                # should probably be robust to ENOENT
                 self.logger.warning('Error cleaning up %s: %r', old_file, exc)
                 self.stats['errors'] += 1
             else:
@@ -560,13 +661,13 @@ class Relinker(object):
         if self.conf['files_per_second'] > 0:
             locations = RateLimitedIterator(
                 locations, self.conf['files_per_second'])
-        for hash_path, device, partition in locations:
+        for hash_path, device, _part_num in locations:
             # note, in cleanup step next_part_power == part_power
             new_hash_path = replace_partition_in_path(
                 self.conf['devices'], hash_path, self.next_part_power)
             if new_hash_path == hash_path:
                 continue
-            self.process_location(hash_path, new_hash_path)
+            self.process_location(device, hash_path, new_hash_path)
 
         # any unmounted devices don't trigger the pre_device trigger.
         # so we'll deal with them here.
@@ -793,6 +894,14 @@ def main(args=None):
                         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('--clobber-hardlink-collisions', action='store_true',
+                        help='Tolerate hard link collisions when relinking'
+                             'object files. If the action is relink then the '
+                             'file in the new target part dir is quarantined '
+                             'and the relink is retried. If the action is '
+                             'cleanup then the file in the new target dir is '
+                             'retained and the file in the old target dir is '
+                             'removed. (default: false)')
 
     args = parser.parse_args(args)
     hubs.use_hub(get_hub())
@@ -835,6 +944,10 @@ def main(args=None):
         'stats_interval': non_negative_float(
             args.stats_interval or conf.get('stats_interval',
                                             DEFAULT_STATS_INTERVAL)),
+        'clobber_hardlink_collisions': (
+            args.clobber_hardlink_collisions or
+            config_true_value(conf.get('clobber_hardlink_collisions',
+                                       'false'))),
     })
     return parallel_process(
         args.action == 'cleanup', conf, logger, args.device_list)

test/unit/cli/test_relinker.py

@@ -520,6 +520,7 @@ class TestRelinker(unittest.TestCase):
             'partitions': set(),
             'recon_cache_path': '/var/cache/swift',
             'stats_interval': 300.0,
+            'clobber_hardlink_collisions': False,
         }
         mock_relinker.assert_called_once_with(
             exp_conf, mock.ANY, ['sdx'], do_cleanup=False)
@@ -566,6 +567,7 @@ class TestRelinker(unittest.TestCase):
             'workers': 'auto',
             'recon_cache_path': '/var/cache/swift-foo',
             'stats_interval': 111.0,
+            'clobber_hardlink_collisions': False,
         }, mock.ANY, ['sdx'], do_cleanup=False)
         logger = mock_relinker.call_args[0][1]
         self.assertEqual(logging.WARNING, logger.getEffectiveLevel())
@@ -598,6 +600,7 @@ class TestRelinker(unittest.TestCase):
             'workers': 2,
             'recon_cache_path': '/var/cache/swift-foo',
             'stats_interval': 222.0,
+            'clobber_hardlink_collisions': False,
         }, mock.ANY, ['sdx'], do_cleanup=False)
 
         with mock.patch('swift.cli.relinker.Relinker') as mock_relinker, \
@@ -616,6 +619,7 @@ class TestRelinker(unittest.TestCase):
             'workers': 'auto',
             'recon_cache_path': '/var/cache/swift',
             'stats_interval': 300.0,
+            'clobber_hardlink_collisions': False,
         }, mock.ANY, ['sdx'], do_cleanup=False)
         mock_logging_config.assert_called_once_with(
             format='%(message)s', level=logging.INFO, filename=None)
@@ -643,6 +647,7 @@ class TestRelinker(unittest.TestCase):
             'workers': 'auto',
             'recon_cache_path': '/var/cache/swift',
             'stats_interval': 300.0,
+            'clobber_hardlink_collisions': False,
         }, mock.ANY, ['sdx'], do_cleanup=False)
         # --debug is now effective
         mock_logging_config.assert_called_once_with(
@@ -682,6 +687,7 @@ class TestRelinker(unittest.TestCase):
             'workers': 'auto',
             'recon_cache_path': '/var/cache/swift',
             'stats_interval': 300.0,
+            'clobber_hardlink_collisions': False,
         }, mock.ANY, ['sdx'], do_cleanup=False)
         logger = mock_relinker.call_args[0][1]
         self.assertEqual(logging.WARNING, logger.getEffectiveLevel())
@@ -717,11 +723,102 @@ class TestRelinker(unittest.TestCase):
             'workers': 'auto',
             'recon_cache_path': '/var/cache/swift',
             'stats_interval': 300.0,
+            'clobber_hardlink_collisions': False,
         }, mock.ANY, ['sdx'], do_cleanup=False)
         logger = mock_relinker.call_args[0][1]
         self.assertEqual(logging.WARNING, logger.getEffectiveLevel())
         self.assertEqual('test-relinker', logger.logger.name)
 
+    def test_relinker_clobber_hardlink_collisions_default(self):
+        config = """
+        [DEFAULT]
+        swift_dir = %(swift_dir)s
+        devices = /test/node
+        mount_check = false
+
+        [object-relinker]
+        log_name = test-relinker
+        """ % {
+            'swift_dir': self.testdir,
+        }
+
+        conf_file = os.path.join(self.testdir, 'relinker.conf')
+        with open(conf_file, 'w') as f:
+            f.write(dedent(config))
+
+        captured_relinker_instance = None
+
+        def capture_relinker(instance_self):
+            nonlocal captured_relinker_instance
+            captured_relinker_instance = instance_self
+
+        with mock.patch('swift.cli.relinker.Relinker.run', capture_relinker):
+            relinker.main(['relink', conf_file, '--device', 'sdx'])
+
+        self.assertFalse(
+            captured_relinker_instance.conf['clobber_hardlink_collisions'])
+
+    def test_relinker_clobber_hardlink_collisions_config(self):
+        config = """
+        [DEFAULT]
+        swift_dir = %(swift_dir)s
+        devices = /test/node
+        mount_check = false
+
+        [object-relinker]
+        log_name = test-relinker
+        clobber_hardlink_collisions = true
+        """ % {
+            'swift_dir': self.testdir,
+        }
+
+        conf_file = os.path.join(self.testdir, 'relinker.conf')
+        with open(conf_file, 'w') as f:
+            f.write(dedent(config))
+
+        captured_relinker_instance = None
+
+        def capture_relinker(instance_self):
+            nonlocal captured_relinker_instance
+            captured_relinker_instance = instance_self
+
+        with mock.patch('swift.cli.relinker.Relinker.run', capture_relinker):
+            relinker.main(['relink', conf_file, '--device', 'sdx'])
+
+        self.assertTrue(
+            captured_relinker_instance.conf['clobber_hardlink_collisions'])
+
+    def test_relinker_clobber_hardlink_collisions_arg(self):
+        config = """
+        [DEFAULT]
+        swift_dir = %(swift_dir)s
+        devices = /test/node
+        mount_check = false
+
+        [object-relinker]
+        log_name = test-relinker
+        clobber_hardlink_collisions = false
+        """ % {
+            'swift_dir': self.testdir,
+        }
+
+        conf_file = os.path.join(self.testdir, 'relinker.conf')
+        with open(conf_file, 'w') as f:
+            f.write(dedent(config))
+
+        captured_relinker_instance = None
+
+        def capture_relinker(instance_self):
+            nonlocal captured_relinker_instance
+            captured_relinker_instance = instance_self
+
+        with mock.patch('swift.cli.relinker.Relinker.run', capture_relinker):
+            relinker.main(['relink', conf_file, '--device', 'sdx',
+                           '--clobber-hardlink-collisions'])
+
+        self.assertTrue(
+            captured_relinker_instance.conf['clobber_hardlink_collisions'])
+
     def test_relinker_utils_get_hub(self):
         cli_cmd = ['relink', '--device', 'sdx', '--workers', 'auto',
                    '--device', '/some/device']
@@ -880,7 +977,18 @@ class TestRelinker(unittest.TestCase):
             self.assertEqual(sorted(exp_filenames), sorted(actual_old))
         else:
             self.assertFalse(os.path.exists(self.objdir))
-        self.assertEqual([], self.logger.get_lines_for_level('error'))
+        if exp_ret_code == 0:
+            # a successful relink should not have logged errors
+            self.assertEqual([], self.logger.get_lines_for_level('error'))
+        elif conflict_file_specs:
+            # if a conflict was an error, we should have logged it
+            error_lines = self.logger.get_lines_for_level('error')
+            self.assertEqual(len(conflict_file_specs), len(error_lines))
+            for err_msg in error_lines:
+                self.assertTrue(err_msg.startswith("Error relinking"))
+                self.assertIn("hardlink collision", err_msg)
+                self.assertTrue(err_msg.endswith(
+                    "(consider enabling clobber_hardlink_collisions)"))
 
     def _relink_test(self, old_file_specs, new_file_specs,
                      exp_old_specs, exp_new_specs):
@@ -1081,15 +1189,138 @@ class TestRelinker(unittest.TestCase):
                 '--skip-mount',
             ]))
 
+        error_lines = self.logger.get_lines_for_level('error')
+        self.assertEqual([
+            'Error relinking: hardlink collision: %s to %s '
+            '(consider enabling clobber_hardlink_collisions)' % (
+                self.objname,
+                self.expected_file,
+            ),
+        ], error_lines)
+        self.assertEqual([
+            '1 hash dirs processed (cleanup=False) '
+            '(1 files, 0 linked, 0 removed, 1 errors)',
+        ], self.logger.get_lines_for_level('warning'))
+
+    def test_relink_link_already_exists_clobber_hardlink_collisions(self):
+        self.rb.prepare_increase_partition_power()
+        self._save_ring()
+
+        # make a file where we'd expect the link to be created
+        os.makedirs(self.expected_dir)
+        with open(self.expected_file, 'w'):
+            pass
+
+        # expect no error w/ --clobber-hardlink-collisions
+        with self._mock_relinker():
+            self.assertEqual(0, relinker.main([
+                'relink',
+                '--swift-dir', self.testdir,
+                '--devices', self.devices,
+                '--skip-mount',
+                '--clobber-hardlink-collisions',
+            ]))
+
+        for level in ['error', 'warning']:
+            self.assertEqual([], self.logger.get_lines_for_level(level))
+        # ... just info about clobbering your hardlink collisions
+        info_lines = self.logger.get_lines_for_level('info')
+        clobbering_lines = [
+            line for line in info_lines
+            if line.startswith('Relinking: clobbering hardlink collision')]
+        self.assertEqual(1, len(clobbering_lines), info_lines)
+        quarantine_dir = os.path.join(
+            self.devices, 'sda1', 'quarantined', 'objects')
+        self.assertIn(quarantine_dir, clobbering_lines[0])
+        self.assertEqual(
+            '1 hash dirs processed (cleanup=False) '
+            '(1 files, 1 linked, 0 removed, 0 errors)',
+            info_lines[-2])
+
+    def test_relink_clobber_hardlink_collisions_after_quarantine(self):
+        self.rb.prepare_increase_partition_power()
+        self._save_ring()
+
+        # make a file where we'd expect the link to be created
+        os.makedirs(self.expected_dir)
+        with open(self.expected_file, 'w'):
+            pass
+
+        # expect error
+        with mock.patch(
+                'swift.obj.diskfile.quarantine_renamer') as mock_quarantine, \
+                self._mock_relinker():
+            mock_quarantine.return_value = '/the/quarantine/dir'
+            self.assertEqual(1, relinker.main([
+                'relink',
+                '--swift-dir', self.testdir,
+                '--devices', self.devices,
+                '--skip-mount',
+                '--clobber-hardlink-collisions',
+            ]))
+
+        dev_path = os.path.join(self.devices, 'sda1')
+        self.assertEqual([
+            mock.call(dev_path, self.expected_file)
+        ], mock_quarantine.call_args_list)
+
+        # we still log info about clobbering hardlink collisions
+        info_lines = self.logger.get_lines_for_level('info')
+        clobbering_lines = [
+            line for line in info_lines
+            if line.startswith('Relinking: clobbering hardlink collision')]
+        self.assertEqual(1, len(clobbering_lines), info_lines)
+        self.assertIn('/the/quarantine/dir', clobbering_lines[0])
+
+        # but there was a *another* hardlink collision (!?)
         warning_lines = self.logger.get_lines_for_level('warning')
-        self.assertIn('Error relinking: failed to relink %s to %s: '
+        collision_lines = [
-                      '[Errno 17] File exists'
+            line for line in warning_lines
-                      % (self.objname, self.expected_file),
+            if line.startswith('Relinking: hardlink collision')]
-                      warning_lines[0])
+        self.assertEqual(1, len(collision_lines), warning_lines)
-        self.assertIn('1 hash dirs processed (cleanup=False) '
+        self.assertIn('persists after quarantine', collision_lines[0])
-                      '(1 files, 0 linked, 0 removed, 1 errors)',
+
-                      warning_lines)
+        # XXX this is kind of sketch: we got exit code 1 but no *error* lines;
+        # does relinker just treat errors as warnings?
         self.assertEqual([], self.logger.get_lines_for_level('error'))
+        self.assertEqual(
+            '1 hash dirs processed (cleanup=False) '
+            '(1 files, 0 linked, 0 removed, 1 errors)',
+            warning_lines[-1])
+
+    def test_cleanup_link_already_exists_clobber_hardlink_collisions(self):
+        self.rb.prepare_increase_partition_power()
+        self.rb.increase_partition_power()
+        self._save_ring()
+
+        # make a file where we'd expect the link to be created
+        os.makedirs(self.expected_dir)
+        with open(self.expected_file, 'w'):
+            pass
+
+        with self._mock_relinker():
+            self.assertEqual(0, relinker.main([
+                'cleanup',
+                '--swift-dir', self.testdir,
+                '--devices', self.devices,
+                '--skip-mount',
+                '--debug',
+                '--clobber-hardlink-collisions',
+            ]))
+
+        for level in ['error', 'warning']:
+            self.assertEqual([], self.logger.get_lines_for_level(level))
+        debug_lines = self.logger.get_lines_for_level('debug')
+        relinking_lines = [
+            line for line in debug_lines
+            if line.startswith('Relinking (cleanup):')]
+        self.assertEqual(1, len(relinking_lines), debug_lines)
+        self.assertIn('tolerating hardlink collision', relinking_lines[0])
+        info_lines = self.logger.get_lines_for_level('info')
+        self.assertEqual(
+            '1 hash dirs processed (cleanup=True) '
+            '(1 files, 0 linked, 1 removed, 0 errors)',
+            info_lines[-2])
 
     def test_relink_link_already_exists(self):
         self.rb.prepare_increase_partition_power()
@@ -2026,6 +2257,11 @@ class TestRelinker(unittest.TestCase):
                            relink_errors={'data': OSError(errno.EPERM, 'oops'),
                                           'meta': OSError(errno.EPERM, 'oops')}
                            )
+        error_lines = self.logger.get_lines_for_level('error')
+        self.assertEqual(2, len(error_lines))
+        for err_msg in error_lines:
+            self.assertTrue(err_msg.startswith("Error relinking"))
+            self.assertIn("failed to relink", err_msg)
         warning_lines = self.logger.get_lines_for_level('warning')
         self.assertIn('1 hash dirs processed (cleanup=True) '
                       '(2 files, 0 linked, 0 removed, 2 errors)',
@@ -3095,17 +3331,20 @@ class TestRelinker(unittest.TestCase):
         self.assertTrue(os.path.isfile(self.expected_file))
         with open(self.expected_file, 'r') as fd:
             self.assertEqual('same but different', fd.read())
-        warning_lines = self.logger.get_lines_for_level('warning')
+        error_lines = self.logger.get_lines_for_level('error')
-        self.assertEqual(2, len(warning_lines), warning_lines)
+        self.assertEqual([
-        self.assertIn('Error relinking (cleanup): failed to relink %s to %s'
+            'Error relinking (cleanup): hardlink collision: %s to %s'
-                      % (self.objname, self.expected_file), warning_lines[0])
+            ' (consider enabling clobber_hardlink_collisions)'
+            % (self.objname, self.expected_file),
+        ], error_lines)
         # suffix should not be invalidated in new partition
         hashes_invalid = os.path.join(self.next_part_dir, 'hashes.invalid')
         self.assertFalse(os.path.exists(hashes_invalid))
-        self.assertEqual('1 hash dirs processed (cleanup=True) '
+        warning_lines = self.logger.get_lines_for_level('warning')
-                         '(1 files, 0 linked, 0 removed, 1 errors)',
+        self.assertEqual([
-                         warning_lines[1])
+            '1 hash dirs processed (cleanup=True) '
-        self.assertEqual([], self.logger.get_lines_for_level('error'))
+            '(1 files, 0 linked, 0 removed, 1 errors)',
+        ], warning_lines)
 
     def test_cleanup_older_object_in_new_partition(self):
         # relink of the current object failed, but there is an older version of
@@ -3411,7 +3650,7 @@ class TestRelinker(unittest.TestCase):
                       '(1 files, 1 linked, 1 removed, 0 errors)', info_lines)
         self.assertEqual([], self.logger.get_lines_for_level('error'))
 
-    def test_cleanup_new_does_not_exist_and_relink_fails(self):
+    def test_cleanup_new_does_not_exist_and_relink_raises_os_error(self):
         # force rehash of new partition to not happen during cleanup
         self._setup_object(lambda part: part >= 2 ** (PART_POWER - 1))
         self._common_test_cleanup()
@@ -3422,7 +3661,8 @@ class TestRelinker(unittest.TestCase):
         # cleanup: cleanup attempts to re-create the link but fails
         os.remove(self.expected_file)
 
-        with mock.patch('swift.obj.diskfile.os.link', side_effect=OSError):
+        with mock.patch('swift.obj.diskfile.os.link',
+                        side_effect=OSError('os-error!')):
             with self._mock_relinker():
                 self.assertEqual(1, relinker.main([
                     'cleanup',
@@ -3432,9 +3672,49 @@ class TestRelinker(unittest.TestCase):
                 ]))
         self.assertFalse(os.path.isfile(self.expected_file))
         self.assertTrue(os.path.isfile(self.objname))  # old file intact
-        self.assertEqual(self.logger.get_lines_for_level('warning'), [
+        self.assertEqual(self.logger.get_lines_for_level('error'), [
-            'Error relinking (cleanup): failed to relink %s to %s: '
+            'Error relinking (cleanup): failed to relink %s to %s: os-error!'
             % (self.objname, self.expected_file),
+        ])
+        self.assertEqual(self.logger.get_lines_for_level('warning'), [
+            '1 hash dirs processed (cleanup=True) '
+            '(1 files, 0 linked, 0 removed, 1 errors)',
+        ])
+        # suffix should not be invalidated in new partition
+        self.assertTrue(os.path.exists(hashes_invalid))
+        with open(hashes_invalid, 'r') as fd:
+            self.assertEqual('', fd.read().strip())
+        # nor in the old partition
+        old_hashes_invalid = os.path.join(self.part_dir, 'hashes.invalid')
+        self.assertFalse(os.path.exists(old_hashes_invalid))
+
+    def test_cleanup_new_does_not_exist_and_relink_raises_other_error(self):
+        # force rehash of new partition to not happen during cleanup
+        self._setup_object(lambda part: part >= 2 ** (PART_POWER - 1))
+        self._common_test_cleanup()
+        # rehash during relink creates hashes.invalid...
+        hashes_invalid = os.path.join(self.next_part_dir, 'hashes.invalid')
+        self.assertTrue(os.path.exists(hashes_invalid))
+        # Pretend the file in the new place got deleted in between relink and
+        # cleanup: cleanup attempts to re-create the link but fails
+        os.remove(self.expected_file)
+
+        with mock.patch('swift.obj.diskfile.os.link',
+                        side_effect=ValueError('kaboom!')):
+            with self._mock_relinker():
+                self.assertEqual(1, relinker.main([
+                    'cleanup',
+                    '--swift-dir', self.testdir,
+                    '--devices', self.devices,
+                    '--skip-mount',
+                ]))
+        self.assertFalse(os.path.isfile(self.expected_file))
+        self.assertTrue(os.path.isfile(self.objname))  # old file intact
+        self.assertEqual(self.logger.get_lines_for_level('error'), [
+            'Error relinking (cleanup): failed to relink %s to %s: kaboom!'
+            % (self.objname, self.expected_file),
+        ])
+        self.assertEqual(self.logger.get_lines_for_level('warning'), [
             '1 hash dirs processed (cleanup=True) '
             '(1 files, 0 linked, 0 removed, 1 errors)',
         ])
@@ -3445,7 +3725,6 @@ class TestRelinker(unittest.TestCase):
         # nor in the old partition
         old_hashes_invalid = os.path.join(self.part_dir, 'hashes.invalid')
         self.assertFalse(os.path.exists(old_hashes_invalid))
-        self.assertEqual([], self.logger.get_lines_for_level('error'))
 
     def test_cleanup_remove_fails(self):
         meta_file = utils.Timestamp(int(self.obj_ts) + 1).internal + '.meta'