From 397f94c73bbfac121960d551fdb0a6877a6b6171 Mon Sep 17 00:00:00 2001 From: Tim Burke Date: Wed, 10 Sep 2025 09:36:24 -0700 Subject: [PATCH] diskfile: Fix UnboundLocalError during part power increase Closes-Bug: #2122543 Co-Authored-By: Clay Gerrard Signed-off-by: Tim Burke Change-Id: I8a2a96394734899ee48e1d9264bf3908968c51a8 --- swift/obj/diskfile.py | 13 ++++-- test/unit/obj/test_diskfile.py | 83 ++++++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+), 5 deletions(-) diff --git a/swift/obj/diskfile.py b/swift/obj/diskfile.py index 0fad687ce8..09106aa7c8 100644 --- a/swift/obj/diskfile.py +++ b/swift/obj/diskfile.py @@ -3293,6 +3293,7 @@ class ECDiskFileWriter(BaseDiskFileWriter): new_durable_data_file_path = replace_partition_in_path( self.manager.devices, durable_data_file_path, self.next_part_power) + error_in_ppi_rename = False try: try: os.rename(data_file_path, durable_data_file_path) @@ -3302,11 +3303,9 @@ class ECDiskFileWriter(BaseDiskFileWriter): try: os.rename(new_data_file_path, new_durable_data_file_path) - except OSError as exc: - self.manager.logger.exception( - 'Renaming new path %s to %s failed: %s', - new_data_file_path, new_durable_data_file_path, - exc) + except OSError: + error_in_ppi_rename = True + raise # Still translate to a DiskFileError below except (OSError, IOError) as err: if err.errno == errno.ENOENT: @@ -3324,6 +3323,10 @@ class ECDiskFileWriter(BaseDiskFileWriter): for frag in durables): return + if error_in_ppi_rename: + self.manager.logger.error( + 'Renaming new path %s to %s failed', + new_data_file_path, new_durable_data_file_path) if err.errno not in (errno.ENOSPC, errno.EDQUOT): # re-raise to catch all handler raise diff --git a/test/unit/obj/test_diskfile.py b/test/unit/obj/test_diskfile.py index d04e431739..a9dba0d6be 100644 --- a/test/unit/obj/test_diskfile.py +++ b/test/unit/obj/test_diskfile.py @@ -6360,6 +6360,89 @@ class TestECDiskFile(DiskFileMixin, unittest.TestCase): self._test_commit_raises_DiskFileError_for_fsync_dir_errors( OSError(100, 'Some Error')) + def test_commit_raises_DiskFileError_for_error_in_ppi_rename(self): + df = self._simple_get_diskfile(account='a', container='c', + obj='o_fsync_dir_err', + policy=POLICIES.default, + next_part_power=11) + + orig_rename = os.rename + captured_renames = [] + + def exploding_rename(*args): + captured_renames.append(args) + if len(captured_renames) >= 2: + raise IOError(21, 'Some IO Error') + else: + return orig_rename(*args) + timestamp = Timestamp.now() + with df.create() as writer: + metadata = { + 'ETag': 'bogus_etag', + 'X-Timestamp': timestamp.internal, + 'Content-Length': '0', + } + writer.put(metadata) + with mock.patch('swift.obj.diskfile.os.rename', + side_effect=exploding_rename): + with self.assertRaises(DiskFileError) as cm: + writer.commit(timestamp) + self.assertEqual(2, len(captured_renames)) + dl = os.listdir(df._datadir) + datafile = _make_datafilename( + timestamp, POLICIES.default, frag_index=2, durable=True) + self.assertEqual([datafile], dl) + self.assertIn('Problem making data file durable', str(cm.exception)) + error_lines = df.manager.logger.get_lines_for_level('error') + self.assertEqual(2, len(error_lines), error_lines) + self.assertTrue(error_lines[0].startswith('Renaming new path')) + self.assertTrue(error_lines[1].startswith( + 'Problem making data file durable')) + + def test_commit_overwritten_before_ppi_rename(self): + df = self._simple_get_diskfile(account='a', container='c', + obj='o_fsync_dir_err', + policy=POLICIES.default, + next_part_power=11) + + orig_rename = os.rename + captured_renames = [] + + def overwriting_rename(*args): + captured_renames.append(args) + if len(captured_renames) == 2: + # Overwrite! + with df.create() as writer: + metadata = { + 'ETag': 'also_bogus_etag', + 'X-Timestamp': ts2.internal, + 'Content-Length': '0', + } + writer.put(metadata) + writer.commit(ts2) + os.unlink(args[0]) + return orig_rename(*args) + + timestamp = Timestamp.now() + ts2 = Timestamp(timestamp, delta=1) + with df.create() as writer: + metadata = { + 'ETag': 'bogus_etag', + 'X-Timestamp': timestamp.internal, + 'Content-Length': '0', + } + writer.put(metadata) + with mock.patch('swift.obj.diskfile.os.rename', + side_effect=overwriting_rename): + writer.commit(timestamp) + self.assertEqual(4, len(captured_renames)) + dl = os.listdir(df._datadir) + datafile = _make_datafilename( + ts2, POLICIES.default, frag_index=2, durable=True) + self.assertEqual([datafile], dl) + error_lines = df.manager.logger.get_lines_for_level('error') + self.assertEqual(0, len(error_lines), error_lines) + def test_data_file_has_frag_index(self): policy = POLICIES.default for good_value in (0, '0', 2, '2', 13, '13'):