From 2b4ec5a45b31fad6884b75a23a229e8b1dfb5776 Mon Sep 17 00:00:00 2001 From: Alistair Coles Date: Wed, 10 Mar 2021 15:19:15 +0000 Subject: [PATCH] relinker: add --policy option Add --policy option to restrict relinker to processing one policy. Change-Id: I9455738f4677d9c3d72e8e18f5bde37f6a7f623e --- swift/cli/relinker.py | 15 +++-- swift/common/utils.py | 29 ++++++++ test/unit/cli/test_relinker.py | 118 +++++++++++++++++++++++++++++++-- test/unit/common/test_utils.py | 37 +++++++++++ 4 files changed, 188 insertions(+), 11 deletions(-) diff --git a/swift/cli/relinker.py b/swift/cli/relinker.py index 4b581714e3..78a3ad98fd 100644 --- a/swift/cli/relinker.py +++ b/swift/cli/relinker.py @@ -24,7 +24,7 @@ from functools import partial from swift.common.storage_policy import POLICIES from swift.common.utils import replace_partition_in_path, config_true_value, \ audit_location_generator, get_logger, readconf, drop_privileges, \ - RateLimitedIterator, lock_path + RateLimitedIterator, lock_path, non_negative_float, non_negative_int from swift.obj import diskfile @@ -38,9 +38,10 @@ EXIT_NO_APPLICABLE_POLICY = 2 EXIT_ERROR = 1 -def non_negative_float(value): - value = float(value) - if value < 0: +def policy(policy_index): + value = non_negative_int(policy_index) + value = POLICIES.get_by_index(value) + if value is None: raise ValueError return value @@ -407,7 +408,7 @@ class Relinker(object): def run(self): self._zero_stats() - for policy in POLICIES: + for policy in self.conf['policies']: policy.object_ring = None # Ensure it will be reloaded policy.load_ring(self.conf['swift_dir']) ring = policy.object_ring @@ -467,6 +468,9 @@ def main(args): 'Path to config file with [object-relinker] section')) parser.add_argument('--swift-dir', default=None, dest='swift_dir', help='Path to swift directory') + parser.add_argument('--policy', default=None, dest='policy', + type=policy, + help='Policy to relink (default: all)') parser.add_argument('--devices', default=None, dest='devices', help='Path to swift device directory') parser.add_argument('--user', default=None, dest='user', @@ -514,6 +518,7 @@ def main(args): 'files_per_second': ( args.files_per_second if args.files_per_second is not None else non_negative_float(conf.get('files_per_second', '0'))), + 'policies': POLICIES if args.policy is None else [args.policy], }) if args.action == 'relink': diff --git a/swift/common/utils.py b/swift/common/utils.py index 4ec71d4ce7..8174532c86 100644 --- a/swift/common/utils.py +++ b/swift/common/utils.py @@ -424,6 +424,35 @@ def backward(f, blocksize=4096): TRUE_VALUES = set(('true', '1', 'yes', 'on', 't', 'y')) +def non_negative_float(value): + """ + Check that the value casts to a float and is non-negative. + + :param value: value to check + :raises ValueError: if the value cannot be cast to a float or is negative. + :return: a float + """ + value = float(value) + if value < 0: + raise ValueError + return value + + +def non_negative_int(value): + """ + Check that the value casts to an int and is a whole number. + + :param value: value to check + :raises ValueError: if the value cannot be cast to an int or does not + represent a whole number. + :return: an int + """ + int_value = int(value) + if int_value != non_negative_float(value): + raise ValueError + return int_value + + def config_true_value(value): """ Returns True if the value is either True or a string in TRUE_VALUES. diff --git a/test/unit/cli/test_relinker.py b/test/unit/cli/test_relinker.py index fc8f79c708..5b18d50603 100644 --- a/test/unit/cli/test_relinker.py +++ b/test/unit/cli/test_relinker.py @@ -246,6 +246,10 @@ class TestRelinker(unittest.TestCase): self._common_test_cleanup() self._do_test_relinker_files_per_second('cleanup') + @patch_policies( + [StoragePolicy(0, name='gold', is_default=True), + ECStoragePolicy(1, name='platinum', ec_type=DEFAULT_TEST_EC_TYPE, + ec_ndata=4, ec_nparity=2)]) def test_conf_file(self): config = """ [DEFAULT] @@ -274,6 +278,7 @@ class TestRelinker(unittest.TestCase): 'files_per_second': 0.0, 'log_name': 'test-relinker', 'log_level': 'DEBUG', + 'policies': POLICIES } mock_relink.assert_called_once_with(exp_conf, mock.ANY, device='sdx') logger = mock_relink.call_args[0][1] @@ -312,6 +317,7 @@ class TestRelinker(unittest.TestCase): 'files_per_second': 11.1, 'log_name': 'test-relinker', 'log_level': 'WARNING', + 'policies': POLICIES }, mock.ANY, device='sdx') logger = mock_relink.call_args[0][1] self.assertEqual(logging.WARNING, logger.getEffectiveLevel()) @@ -322,7 +328,8 @@ class TestRelinker(unittest.TestCase): relinker.main([ 'relink', conf_file, '--device', 'sdx', '--debug', '--swift-dir', 'cli-dir', '--devices', 'cli-devs', - '--skip-mount-check', '--files-per-second', '2.2']) + '--skip-mount-check', '--files-per-second', '2.2', + '--policy', '1']) mock_relink.assert_called_once_with({ '__file__': mock.ANY, 'swift_dir': 'cli-dir', @@ -331,6 +338,7 @@ class TestRelinker(unittest.TestCase): 'files_per_second': 2.2, 'log_level': 'DEBUG', 'log_name': 'test-relinker', + 'policies': [POLICIES[1]] }, mock.ANY, device='sdx') with mock.patch('swift.cli.relinker.relink') as mock_relink, \ @@ -344,6 +352,7 @@ class TestRelinker(unittest.TestCase): 'mount_check': False, 'files_per_second': 0.0, 'log_level': 'INFO', + 'policies': POLICIES }, mock.ANY, device='sdx') mock_logging_config.assert_called_once_with( format='%(message)s', level=logging.INFO, filename=None) @@ -359,6 +368,7 @@ class TestRelinker(unittest.TestCase): 'mount_check': False, 'files_per_second': 0.0, 'log_level': 'DEBUG', + 'policies': POLICIES }, mock.ANY, device='sdx') # --debug is now effective mock_logging_config.assert_called_once_with( @@ -550,13 +560,91 @@ class TestRelinker(unittest.TestCase): self.assertFalse(os.path.isdir(self.expected_dir)) self.assertFalse(os.path.isfile(self.expected_file)) + @patch_policies( + [StoragePolicy(0, name='gold', is_default=True), + ECStoragePolicy(1, name='platinum', ec_type=DEFAULT_TEST_EC_TYPE, + ec_ndata=4, ec_nparity=2)]) + def test_relink_policy_option(self): + self._setup_object() + self.rb.prepare_increase_partition_power() + self._save_ring() + + # invalid policy + with mock.patch('sys.stdout'), mock.patch('sys.stderr'): + with self.assertRaises(SystemExit) as cm: + self.assertEqual(0, relinker.main([ + 'relink', + '--swift-dir', self.testdir, + '--policy', '9', + '--skip-mount', + '--devices', self.devices, + '--device', self.existing_device, + ])) + self.assertEqual(2, cm.exception.code) + + # policy must be index not name + with mock.patch('sys.stdout'), mock.patch('sys.stderr'): + with self.assertRaises(SystemExit) as cm: + self.assertEqual(0, relinker.main([ + 'relink', + '--swift-dir', self.testdir, + '--policy', 'gold', + '--skip-mount', + '--devices', self.devices, + '--device', self.existing_device, + ])) + self.assertEqual(2, cm.exception.code) + + # policy with no object + with mock.patch.object(relinker.logging, 'getLogger', + return_value=self.logger): + self.assertEqual(0, relinker.main([ + 'relink', + '--swift-dir', self.testdir, + '--policy', '1', + '--skip-mount', + '--devices', self.devices, + '--device', self.existing_device, + ])) + self.assertFalse(os.path.isdir(self.expected_dir)) + self.assertEqual( + ['Processing files for policy platinum under %s/%s (cleanup=False)' + % (self.devices, self.existing_device), + '0 diskfiles processed (cleanup=False) (0 errors)'], + self.logger.get_lines_for_level('info')) + + # policy with object + self.logger.clear() + with mock.patch.object(relinker.logging, 'getLogger', + return_value=self.logger): + self.assertEqual(0, relinker.main([ + 'relink', + '--swift-dir', self.testdir, + '--policy', '0', + '--skip-mount', + '--devices', self.devices, + '--device', self.existing_device, + ])) + 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) + self.assertEqual( + ['Processing files for policy gold under %s/%s (cleanup=False)' + % (self.devices, self.existing_device), + 'Device: sda1 Step: relink Partitions: 1/1', + '1 diskfiles processed (cleanup=False) (0 errors)'], + self.logger.get_lines_for_level('info')) + @patch_policies( [StoragePolicy(0, name='gold', is_default=True), ECStoragePolicy(1, name='platinum', ec_type=DEFAULT_TEST_EC_TYPE, ec_ndata=4, ec_nparity=2)]) def test_relink_all_policies(self): # verify that only policies in appropriate state are processed - def do_relink(): + def do_relink(options=None): + options = [] if options is None else options with mock.patch( 'swift.cli.relinker.Relinker.process_policy') as mocked: res = relinker.main([ @@ -565,7 +653,7 @@ class TestRelinker(unittest.TestCase): '--skip-mount', '--devices', self.devices, '--device', self.existing_device, - ]) + ] + options) return res, mocked self._save_ring(POLICIES) # no ring prepared for increase @@ -580,6 +668,10 @@ class TestRelinker(unittest.TestCase): self.assertEqual([mock.call(POLICIES[1])], mocked.call_args_list) self.assertEqual(0, res) + res, mocked = do_relink(['--policy', 0]) + self.assertEqual([], mocked.call_args_list) + self.assertEqual(2, res) + self._save_ring([POLICIES[0]]) # prepared for increase res, mocked = do_relink() self.assertEqual([mock.call(POLICIES[0]), mock.call(POLICIES[1])], @@ -597,6 +689,10 @@ class TestRelinker(unittest.TestCase): self.assertEqual([], mocked.call_args_list) self.assertEqual(2, res) + res, mocked = do_relink(['--policy', '0']) + self.assertEqual([], mocked.call_args_list) + self.assertEqual(2, res) + self.rb.finish_increase_partition_power() self._save_ring(POLICIES) # all rings finished res, mocked = do_relink() @@ -609,7 +705,8 @@ class TestRelinker(unittest.TestCase): ec_ndata=4, ec_nparity=2)]) def test_cleanup_all_policies(self): # verify that only policies in appropriate state are processed - def do_cleanup(): + def do_cleanup(options=None): + options = [] if options is None else options with mock.patch( 'swift.cli.relinker.Relinker.process_policy') as mocked: res = relinker.main([ @@ -618,7 +715,7 @@ class TestRelinker(unittest.TestCase): '--skip-mount', '--devices', self.devices, '--device', self.existing_device, - ]) + ] + options) return res, mocked self._save_ring(POLICIES) # no ring prepared for increase @@ -638,6 +735,10 @@ class TestRelinker(unittest.TestCase): self.assertEqual([mock.call(POLICIES[0])], mocked.call_args_list) self.assertEqual(0, res) + res, mocked = do_cleanup(['--policy', '1']) + self.assertEqual([], mocked.call_args_list) + self.assertEqual(2, res) + self._save_ring([POLICIES[1]]) # increased res, mocked = do_cleanup() self.assertEqual([mock.call(POLICIES[0]), mock.call(POLICIES[1])], @@ -655,6 +756,10 @@ class TestRelinker(unittest.TestCase): self.assertEqual([], mocked.call_args_list) self.assertEqual(2, res) + res, mocked = do_cleanup(['--policy', '1']) + self.assertEqual([], mocked.call_args_list) + self.assertEqual(2, res) + def _common_test_cleanup(self, relink=True): # Create a ring that has prev_part_power set self.rb.prepare_increase_partition_power() @@ -664,7 +769,8 @@ class TestRelinker(unittest.TestCase): conf = {'swift_dir': self.testdir, 'devices': self.devices, 'mount_check': False, - 'files_per_second': 0} + 'files_per_second': 0, + 'policies': POLICIES} self.assertEqual(0, relinker.relink( conf, logger=self.logger, device=self.existing_device)) self.rb.increase_partition_power() diff --git a/test/unit/common/test_utils.py b/test/unit/common/test_utils.py index 1ee9313e85..180a21fc38 100644 --- a/test/unit/common/test_utils.py +++ b/test/unit/common/test_utils.py @@ -3007,6 +3007,43 @@ cluster_dfw1 = http://dfw1.host/v1/ finally: utils.TRUE_VALUES = orig_trues + def test_non_negative_float(self): + self.assertEqual(0, utils.non_negative_float('0.0')) + self.assertEqual(0, utils.non_negative_float(0.0)) + self.assertEqual(1.1, utils.non_negative_float(1.1)) + self.assertEqual(1.1, utils.non_negative_float('1.1')) + self.assertEqual(1.0, utils.non_negative_float('1')) + self.assertEqual(1, utils.non_negative_float(True)) + self.assertEqual(0, utils.non_negative_float(False)) + + with self.assertRaises(ValueError): + utils.non_negative_float(-1.1) + with self.assertRaises(ValueError): + utils.non_negative_float('-1.1') + with self.assertRaises(ValueError): + utils.non_negative_float('one') + + def test_non_negative_int(self): + self.assertEqual(0, utils.non_negative_int('0')) + self.assertEqual(0, utils.non_negative_int(0.0)) + self.assertEqual(1, utils.non_negative_int(1)) + self.assertEqual(1, utils.non_negative_int('1')) + self.assertEqual(1, utils.non_negative_int(True)) + self.assertEqual(0, utils.non_negative_int(False)) + + with self.assertRaises(ValueError): + utils.non_negative_int(-1) + with self.assertRaises(ValueError): + utils.non_negative_int('-1') + with self.assertRaises(ValueError): + utils.non_negative_int('-1.1') + with self.assertRaises(ValueError): + utils.non_negative_int('1.1') + with self.assertRaises(ValueError): + utils.non_negative_int('1.0') + with self.assertRaises(ValueError): + utils.non_negative_int('one') + def test_config_positive_int_value(self): expectations = { # value : expected,