From e72aaf0c57eb3b7ed4ef1561fb544c1e6c6e8725 Mon Sep 17 00:00:00 2001 From: Tim Burke Date: Mon, 1 Feb 2021 16:40:21 -0800 Subject: [PATCH] relinker: Pull arg parsing into module This allows us to do testing that's more end-to-end. Change-Id: Ifc47b00c597217efb4d705bd84dc8f7df117ae9d --- bin/swift-object-relinker | 22 +--- swift/cli/relinker.py | 24 +++- test/probe/test_object_partpower_increase.py | 10 +- test/unit/cli/test_relinker.py | 129 +++++++++++++++---- 4 files changed, 131 insertions(+), 54 deletions(-) diff --git a/bin/swift-object-relinker b/bin/swift-object-relinker index 7afd7b8735..ff7e227a64 100755 --- a/bin/swift-object-relinker +++ b/bin/swift-object-relinker @@ -14,30 +14,10 @@ # limitations under the License. -import argparse import sys from swift.cli.relinker import main if __name__ == '__main__': - parser = argparse.ArgumentParser( - description='Relink and cleanup objects to increase partition power') - parser.add_argument('action', choices=['relink', 'cleanup']) - parser.add_argument('--swift-dir', default='/etc/swift', - dest='swift_dir', help='Path to swift directory') - parser.add_argument('--devices', default='/srv/node', - dest='devices', help='Path to swift device directory') - parser.add_argument('--device', default=None, dest='device', - help='Device name to relink (default: all)') - parser.add_argument('--skip-mount-check', default=False, - help='Don\'t test if disk is mounted', - action="store_true", dest='skip_mount_check') - parser.add_argument('--logfile', default=None, - dest='logfile', help='Set log file name') - parser.add_argument('--debug', default=False, action='store_true', - help='Enable debug mode') - - args = parser.parse_args() - - sys.exit(main(args)) + sys.exit(main(sys.argv[1:])) diff --git a/swift/cli/relinker.py b/swift/cli/relinker.py index c7567c9a8a..6c74cc5312 100644 --- a/swift/cli/relinker.py +++ b/swift/cli/relinker.py @@ -14,6 +14,7 @@ # limitations under the License. +import argparse import errno import fcntl import json @@ -24,7 +25,7 @@ from swift.common.storage_policy import POLICIES from swift.common.exceptions import DiskFileDeleted, DiskFileNotExist, \ DiskFileQuarantined from swift.common.utils import replace_partition_in_path, \ - audit_location_generator, get_logger + audit_location_generator from swift.obj import diskfile @@ -225,7 +226,7 @@ def cleanup(swift_dir='/etc/swift', device=None): mount_check = not skip_mount_check conf = {'devices': devices, 'mount_check': mount_check} - diskfile_router = diskfile.DiskFileRouter(conf, get_logger(conf)) + diskfile_router = diskfile.DiskFileRouter(conf, logger) errors = cleaned_up = 0 run = False for policy in POLICIES: @@ -323,6 +324,25 @@ def cleanup(swift_dir='/etc/swift', def main(args): + parser = argparse.ArgumentParser( + description='Relink and cleanup objects to increase partition power') + parser.add_argument('action', choices=['relink', 'cleanup']) + parser.add_argument('--swift-dir', default='/etc/swift', + dest='swift_dir', help='Path to swift directory') + parser.add_argument('--devices', default='/srv/node', + dest='devices', help='Path to swift device directory') + parser.add_argument('--device', default=None, dest='device', + help='Device name to relink (default: all)') + parser.add_argument('--skip-mount-check', default=False, + help='Don\'t test if disk is mounted', + action="store_true", dest='skip_mount_check') + parser.add_argument('--logfile', default=None, + dest='logfile', help='Set log file name') + parser.add_argument('--debug', default=False, action='store_true', + help='Enable debug mode') + + args = parser.parse_args(args) + logging.basicConfig( format='%(message)s', level=logging.DEBUG if args.debug else logging.INFO, diff --git a/test/probe/test_object_partpower_increase.py b/test/probe/test_object_partpower_increase.py index 3639104214..60425c0846 100755 --- a/test/probe/test_object_partpower_increase.py +++ b/test/probe/test_object_partpower_increase.py @@ -24,7 +24,7 @@ from uuid import uuid4 from swiftclient import client -from swift.cli.relinker import relink, cleanup +from swift.cli.relinker import main as relinker_main from swift.common.manager import Manager from swift.common.ring import RingBuilder from swift.common.utils import replace_partition_in_path @@ -117,7 +117,9 @@ class TestPartPowerIncrease(ProbeTest): # Relink existing objects for device in self.devices: - self.assertEqual(0, relink(skip_mount_check=True, devices=device)) + self.assertEqual(0, relinker_main([ + 'relink', '--skip-mount-check', + '--devices', device])) # Create second object after relinking and ensure it is accessible client.put_object(self.url, self.token, container, obj2, self.data) @@ -164,7 +166,9 @@ class TestPartPowerIncrease(ProbeTest): # Cleanup old objects in the wrong location for device in self.devices: - self.assertEqual(0, cleanup(skip_mount_check=True, devices=device)) + self.assertEqual(0, relinker_main([ + 'cleanup', '--skip-mount-check', + '--devices', device])) # Ensure objects are still accessible client.head_object(self.url, self.token, container, obj) diff --git a/test/unit/cli/test_relinker.py b/test/unit/cli/test_relinker.py index 8f31c33597..3d9b8e909b 100644 --- a/test/unit/cli/test_relinker.py +++ b/test/unit/cli/test_relinker.py @@ -15,6 +15,7 @@ import binascii import errno import fcntl import json +import mock import os import shutil import struct @@ -94,7 +95,12 @@ class TestRelinker(unittest.TestCase): def test_relink(self): self.rb.prepare_increase_partition_power() self._save_ring() - relinker.relink(self.testdir, self.devices, True) + 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)) @@ -106,8 +112,13 @@ class TestRelinker(unittest.TestCase): def test_relink_device_filter(self): self.rb.prepare_increase_partition_power() self._save_ring() - relinker.relink(self.testdir, self.devices, True, - device=self.existing_device) + self.assertEqual(0, relinker.main([ + 'relink', + '--swift-dir', self.testdir, + '--devices', self.devices, + '--skip-mount', + '--device', self.existing_device, + ])) self.assertTrue(os.path.isdir(self.expected_dir)) self.assertTrue(os.path.isfile(self.expected_file)) @@ -119,7 +130,13 @@ class TestRelinker(unittest.TestCase): def test_relink_device_filter_invalid(self): self.rb.prepare_increase_partition_power() self._save_ring() - relinker.relink(self.testdir, self.devices, True, device='none') + self.assertEqual(0, relinker.main([ + 'relink', + '--swift-dir', self.testdir, + '--devices', self.devices, + '--skip-mount', + '--device', 'none', + ])) self.assertFalse(os.path.isdir(self.expected_dir)) self.assertFalse(os.path.isfile(self.expected_file)) @@ -140,7 +157,12 @@ class TestRelinker(unittest.TestCase): def test_cleanup(self): self._common_test_cleanup() - self.assertEqual(0, relinker.cleanup(self.testdir, self.devices, True)) + self.assertEqual(0, relinker.main([ + 'cleanup', + '--swift-dir', self.testdir, + '--devices', self.devices, + '--skip-mount', + ])) # Old objectname should be removed, new should still exist self.assertTrue(os.path.isdir(self.expected_dir)) @@ -150,8 +172,13 @@ class TestRelinker(unittest.TestCase): def test_cleanup_device_filter(self): self._common_test_cleanup() - self.assertEqual(0, relinker.cleanup(self.testdir, self.devices, True, - device=self.existing_device)) + self.assertEqual(0, relinker.main([ + 'cleanup', + '--swift-dir', self.testdir, + '--devices', self.devices, + '--skip-mount', + '--device', self.existing_device, + ])) # Old objectname should be removed, new should still exist self.assertTrue(os.path.isdir(self.expected_dir)) @@ -161,8 +188,13 @@ class TestRelinker(unittest.TestCase): def test_cleanup_device_filter_invalid(self): self._common_test_cleanup() - self.assertEqual(0, relinker.cleanup(self.testdir, self.devices, True, - device='none')) + self.assertEqual(0, relinker.main([ + 'cleanup', + '--swift-dir', self.testdir, + '--devices', self.devices, + '--skip-mount', + '--device', 'none', + ])) # Old objectname should still exist, new should still exist self.assertTrue(os.path.isdir(self.expected_dir)) @@ -176,7 +208,12 @@ class TestRelinker(unittest.TestCase): self.rb.prepare_increase_partition_power() self._save_ring() - relinker.relink(self.testdir, self.devices, True) + self.assertEqual(0, relinker.main([ + 'relink', + '--swift-dir', self.testdir, + '--devices', self.devices, + '--skip-mount', + ])) with open(state_file, 'rt') as f: orig_inode = os.stat(state_file).st_ino self.assertEqual(json.load(f), { @@ -191,7 +228,12 @@ class TestRelinker(unittest.TestCase): # Keep the state file open during cleanup so the inode can't be # released/re-used when it gets unlinked self.assertEqual(orig_inode, os.stat(state_file).st_ino) - relinker.cleanup(self.testdir, self.devices, True) + self.assertEqual(0, relinker.main([ + 'cleanup', + '--swift-dir', self.testdir, + '--devices', self.devices, + '--skip-mount', + ])) self.assertNotEqual(orig_inode, os.stat(state_file).st_ino) with open(state_file, 'rt') as f: # NB: part_power/next_part_power tuple changed, so state was reset @@ -401,7 +443,12 @@ class TestRelinker(unittest.TestCase): def test_cleanup_not_yet_relinked(self): self._common_test_cleanup(relink=False) - self.assertEqual(1, relinker.cleanup(self.testdir, self.devices, True)) + self.assertEqual(1, relinker.main([ + 'cleanup', + '--swift-dir', self.testdir, + '--devices', self.devices, + '--skip-mount', + ])) self.assertTrue(os.path.isfile( os.path.join(self.objdir, self.object_fname))) @@ -413,7 +460,12 @@ class TestRelinker(unittest.TestCase): fname_ts = self.expected_file[:-4] + "ts" os.rename(self.expected_file, fname_ts) - self.assertEqual(0, relinker.cleanup(self.testdir, self.devices, True)) + self.assertEqual(0, relinker.main([ + 'cleanup', + '--swift-dir', self.testdir, + '--devices', self.devices, + '--skip-mount', + ])) def test_cleanup_doesnotexist(self): self._common_test_cleanup() @@ -421,8 +473,14 @@ class TestRelinker(unittest.TestCase): # Pretend the file in the new place got deleted inbetween os.remove(self.expected_file) - self.assertEqual( - 1, relinker.cleanup(self.testdir, self.devices, True, self.logger)) + with mock.patch.object(relinker.logging, 'getLogger', + return_value=self.logger): + self.assertEqual(1, relinker.main([ + 'cleanup', + '--swift-dir', self.testdir, + '--devices', self.devices, + '--skip-mount', + ])) self.assertEqual(self.logger.get_lines_for_level('warning'), ['Error cleaning up %s: %s' % (self.objname, repr(exceptions.DiskFileNotExist()))]) @@ -431,17 +489,22 @@ class TestRelinker(unittest.TestCase): [ECStoragePolicy( 0, name='platin', is_default=True, ec_type=DEFAULT_TEST_EC_TYPE, ec_ndata=4, ec_nparity=2)]) - def test_cleanup_non_durable_fragment(self): + def test_cleanup_diskfile_error(self): self._common_test_cleanup() - # Switch the policy type so that actually all fragments are non-durable - # and raise a DiskFileNotExist in EC in this test. However, if the - # counterpart exists in the new location, this is ok - it will be fixed - # by the reconstructor later on - self.assertEqual( - 0, relinker.cleanup(self.testdir, self.devices, True, - self.logger)) - self.assertEqual(self.logger.get_lines_for_level('warning'), []) + # Switch the policy type so all fragments raise DiskFileError. + with mock.patch.object(relinker.logging, 'getLogger', + return_value=self.logger): + self.assertEqual(0, relinker.main([ + 'cleanup', + '--swift-dir', self.testdir, + '--devices', self.devices, + '--skip-mount', + ])) + log_lines = self.logger.get_lines_for_level('warning') + self.assertEqual(1, len(log_lines), + 'Expected 1 log line, got %r' % log_lines) + self.assertIn('Bad fragment index: None', log_lines[0]) def test_cleanup_quarantined(self): self._common_test_cleanup() @@ -449,11 +512,21 @@ class TestRelinker(unittest.TestCase): with open(self.expected_file, "wb") as obj: obj.write(b'trash') - self.assertEqual( - 1, relinker.cleanup(self.testdir, self.devices, True, self.logger)) + with mock.patch.object(relinker.logging, 'getLogger', + return_value=self.logger): + self.assertEqual(1, relinker.main([ + 'cleanup', + '--swift-dir', self.testdir, + '--devices', self.devices, + '--skip-mount', + ])) - self.assertIn('failed audit and was quarantined', - self.logger.get_lines_for_level('warning')[0]) + log_lines = self.logger.get_lines_for_level('warning') + self.assertEqual(2, len(log_lines), + 'Expected 2 log lines, got %r' % log_lines) + self.assertIn('metadata content-length 12 does not match ' + 'actual object size 5', log_lines[0]) + self.assertIn('failed audit and was quarantined', log_lines[1]) if __name__ == '__main__':