From 9b94c278f19781467cc86df4298011f47667e997 Mon Sep 17 00:00:00 2001 From: Alistair Coles Date: Thu, 24 Jun 2021 14:54:53 +0100 Subject: [PATCH] sharder: add more validation checks on config Add checks that more of the sharder config values make sense with respect to each other. For the daemon, all config options are checked. For swift-manage-shard-ranges only those options relevant to the current subcommand are checked. Change-Id: I2d4dab1d56c31f904161e5338f1bb89a03fd88f7 --- swift/cli/manage_shard_ranges.py | 6 ++ swift/container/sharder.py | 30 ++++--- test/unit/cli/test_manage_shard_ranges.py | 6 +- test/unit/container/test_sharder.py | 99 ++++++++++++++++++++--- 4 files changed, 118 insertions(+), 23 deletions(-) diff --git a/swift/cli/manage_shard_ranges.py b/swift/cli/manage_shard_ranges.py index fc5db89bc7..5f825c9c6a 100644 --- a/swift/cli/manage_shard_ranges.py +++ b/swift/cli/manage_shard_ranges.py @@ -934,6 +934,12 @@ def main(cli_args=None): if v is USE_SHARDER_DEFAULT: setattr(args, k, getattr(conf_args, k)) + try: + ContainerSharderConf.validate_conf(args) + except ValueError as err: + print('Invalid config: %s' % err, file=sys.stderr) + return EXIT_INVALID_ARGS + if args.func in (analyze_shard_ranges,): args.input = args.path_to_file return args.func(args) or 0 diff --git a/swift/container/sharder.py b/swift/container/sharder.py index 4a4bf68db1..fe48f9038e 100644 --- a/swift/container/sharder.py +++ b/swift/container/sharder.py @@ -15,6 +15,7 @@ import collections import errno import json +import operator import time from collections import defaultdict from operator import itemgetter @@ -634,18 +635,28 @@ class ContainerSharderConf(object): 'minimum_shard_size', config_positive_int_value, max(self.rows_per_shard // 5, 1)) - self.validate_conf() - def percent_of_threshold(self, val): return int(config_percent_value(val) * self.shard_container_threshold) - def validate_conf(self): - keys = ('minimum_shard_size', 'rows_per_shard') - vals = [getattr(self, key) for key in keys] - if not vals[0] <= vals[1]: - raise ValueError( - '%s (%d) must be less than %s (%d)' - % (keys[0], vals[0], keys[1], vals[1])) + @classmethod + def validate_conf(cls, namespace): + ops = {'<': operator.lt, + '<=': operator.le} + checks = (('minimum_shard_size', '<=', 'rows_per_shard'), + ('shrink_threshold', '<=', 'minimum_shard_size'), + ('rows_per_shard', '<', 'shard_container_threshold'), + ('expansion_limit', '<', 'shard_container_threshold')) + for key1, op, key2 in checks: + try: + val1 = getattr(namespace, key1) + val2 = getattr(namespace, key2) + except AttributeError: + # swift-manage-shard-ranges uses a subset of conf options for + # each command so only validate those actually in the namespace + continue + if not ops[op](val1, val2): + raise ValueError('%s (%d) must be %s %s (%d)' + % (key1, val1, op, key2, val2)) DEFAULT_SHARDER_CONF = vars(ContainerSharderConf()) @@ -658,6 +669,7 @@ class ContainerSharder(ContainerSharderConf, ContainerReplicator): logger = logger or get_logger(conf, log_route='container-sharder') ContainerReplicator.__init__(self, conf, logger=logger) ContainerSharderConf.__init__(self, conf) + ContainerSharderConf.validate_conf(self) if conf.get('auto_create_account_prefix'): self.logger.warning('Option auto_create_account_prefix is ' 'deprecated. Configure ' diff --git a/test/unit/cli/test_manage_shard_ranges.py b/test/unit/cli/test_manage_shard_ranges.py index 439295cf80..240c4184a7 100644 --- a/test/unit/cli/test_manage_shard_ranges.py +++ b/test/unit/cli/test_manage_shard_ranges.py @@ -574,8 +574,8 @@ class TestManageShardRanges(unittest.TestCase): '--minimum-shard-size', str(minimum)]) self.assertEqual(2, ret) self.assertEqual( - 'Error loading config: minimum_shard_size (%s) must be less ' - 'than rows_per_shard (50)' % minimum, err.getvalue().strip()) + 'Invalid config: minimum_shard_size (%s) must be <= ' + 'rows_per_shard (50)' % minimum, err.getvalue().strip()) assert_too_large_value_handled(51) assert_too_large_value_handled(52) @@ -1461,7 +1461,7 @@ class TestManageShardRanges(unittest.TestCase): with mock.patch('sys.stdout', out), mock.patch('sys.stderr', err): ret = main([broker.db_file, 'compact', '--yes', '--expansion-limit', '20']) - self.assertEqual(0, ret, out.getvalue()) + self.assertEqual(0, ret, err.getvalue()) err_lines = err.getvalue().split('\n') self.assert_starts_with(err_lines[0], 'Loaded db broker for ') out_lines = out.getvalue().rstrip('\n').split('\n') diff --git a/test/unit/container/test_sharder.py b/test/unit/container/test_sharder.py index 07647c8023..0875308d1b 100644 --- a/test/unit/container/test_sharder.py +++ b/test/unit/container/test_sharder.py @@ -14,6 +14,7 @@ # limitations under the License. import json import random +from argparse import Namespace import eventlet import os @@ -210,7 +211,7 @@ class TestSharder(BaseTestSharder): 'rsync_compress': True, 'rsync_module': '{replication_ip}::container_sda/', 'reclaim_age': 86400 * 14, - 'shrink_threshold': 7000000, + 'shrink_threshold': 2000000, 'expansion_limit': 17000000, 'shard_container_threshold': 20000000, 'cleave_batch_size': 4, @@ -226,7 +227,7 @@ class TestSharder(BaseTestSharder): 'existing_shard_replication_quorum': 0, 'max_shrinking': 5, 'max_expanding': 4, - 'rows_per_shard': 13 + 'rows_per_shard': 13000000 } expected = { 'mount_check': False, 'bind_ip': '10.11.12.13', 'port': 62010, @@ -238,8 +239,8 @@ class TestSharder(BaseTestSharder): 'rsync_module': '{replication_ip}::container_sda', 'reclaim_age': 86400 * 14, 'shard_container_threshold': 20000000, - 'rows_per_shard': 13, - 'shrink_threshold': 7000000, + 'rows_per_shard': 13000000, + 'shrink_threshold': 2000000, 'expansion_limit': 17000000, 'cleave_batch_size': 4, 'shard_scanner_batch_size': 8, @@ -295,7 +296,7 @@ class TestSharder(BaseTestSharder): def test_init_deprecated_options(self): # percent values applied if absolute values not given conf = { - 'shard_shrink_point': 15, # trumps shrink_threshold + 'shard_shrink_point': 7, # trumps shrink_threshold 'shard_shrink_merge_point': 95, # trumps expansion_limit 'shard_container_threshold': 20000000, } @@ -310,7 +311,7 @@ class TestSharder(BaseTestSharder): 'reclaim_age': 86400 * 7, 'shard_container_threshold': 20000000, 'rows_per_shard': 10000000, - 'shrink_threshold': 3000000, + 'shrink_threshold': 1400000, 'expansion_limit': 19000000, 'cleave_batch_size': 2, 'shard_scanner_batch_size': 10, @@ -326,10 +327,10 @@ class TestSharder(BaseTestSharder): self._do_test_init(conf, expected) # absolute values override percent values conf = { - 'shard_shrink_point': 15, # trumps shrink_threshold - 'shrink_threshold': 7000000, - 'shard_shrink_merge_point': 95, # trumps expansion_limit - 'expansion_limit': 17000000, + 'shard_shrink_point': 7, + 'shrink_threshold': 1300000, # trumps shard_shrink_point + 'shard_shrink_merge_point': 95, + 'expansion_limit': 17000000, # trumps shard_shrink_merge_point 'shard_container_threshold': 20000000, } expected = { @@ -343,7 +344,7 @@ class TestSharder(BaseTestSharder): 'reclaim_age': 86400 * 7, 'shard_container_threshold': 20000000, 'rows_per_shard': 10000000, - 'shrink_threshold': 7000000, + 'shrink_threshold': 1300000, 'expansion_limit': 17000000, 'cleave_batch_size': 2, 'shard_scanner_batch_size': 10, @@ -4223,6 +4224,7 @@ class TestSharder(BaseTestSharder): account, cont, lower, upper) with self._mock_sharder(conf={'shard_container_threshold': 199, 'minimum_shard_size': 1, + 'shrink_threshold': 0, 'auto_create_account_prefix': '.int_'} ) as sharder: with mock_timestamp_now() as now: @@ -4239,6 +4241,7 @@ class TestSharder(BaseTestSharder): # second invocation finds none with self._mock_sharder(conf={'shard_container_threshold': 199, 'minimum_shard_size': 1, + 'shrink_threshold': 0, 'auto_create_account_prefix': '.int_'} ) as sharder: num_found = sharder._find_shard_ranges(broker) @@ -4324,6 +4327,7 @@ class TestSharder(BaseTestSharder): account, cont, lower, upper) with self._mock_sharder(conf={'shard_container_threshold': 199, 'minimum_shard_size': 1, + 'shrink_threshold': 0, 'auto_create_account_prefix': '.int_'} ) as sharder: with mock_timestamp_now() as now: @@ -4412,6 +4416,7 @@ class TestSharder(BaseTestSharder): # third invocation finds none with self._mock_sharder(conf={'shard_container_threshold': 199, 'shard_scanner_batch_size': 2, + 'shrink_threshold': 0, 'minimum_shard_size': 10} ) as sharder: sharder._send_shard_ranges = mock.MagicMock(return_value=True) @@ -7412,3 +7417,75 @@ class TestContainerSharderConf(unittest.TestCase): ValueError, msg='{%s : %s}' % (key, bad_value)) as cm: ContainerSharderConf({key: bad_value}) self.assertIn('Error setting %s' % key, str(cm.exception)) + + def test_validate(self): + def assert_bad(conf): + with self.assertRaises(ValueError): + ContainerSharderConf.validate_conf(ContainerSharderConf(conf)) + + def assert_ok(conf): + try: + ContainerSharderConf.validate_conf(ContainerSharderConf(conf)) + except ValueError as err: + self.fail('Unexpected ValueError: %s' % err) + + assert_ok({}) + assert_ok({'minimum_shard_size': 100, + 'shrink_threshold': 100, + 'rows_per_shard': 100}) + assert_bad({'minimum_shard_size': 100}) + assert_bad({'shrink_threshold': 100001}) + assert_ok({'minimum_shard_size': 100, + 'shrink_threshold': 100}) + assert_bad({'minimum_shard_size': 100, + 'shrink_threshold': 100, + 'rows_per_shard': 99}) + + assert_ok({'shard_container_threshold': 100, + 'rows_per_shard': 99}) + assert_bad({'shard_container_threshold': 100, + 'rows_per_shard': 100}) + assert_bad({'rows_per_shard': 10000001}) + + assert_ok({'shard_container_threshold': 100, + 'expansion_limit': 99}) + assert_bad({'shard_container_threshold': 100, + 'expansion_limit': 100}) + assert_bad({'expansion_limit': 100000001}) + + def test_validate_subset(self): + # verify that validation is only applied for keys that exist in the + # given namespace + def assert_bad(conf): + with self.assertRaises(ValueError): + ContainerSharderConf.validate_conf(Namespace(**conf)) + + def assert_ok(conf): + try: + ContainerSharderConf.validate_conf(Namespace(**conf)) + except ValueError as err: + self.fail('Unexpected ValueError: %s' % err) + + assert_ok({}) + assert_ok({'minimum_shard_size': 100, + 'shrink_threshold': 100, + 'rows_per_shard': 100}) + assert_ok({'minimum_shard_size': 100}) + assert_ok({'shrink_threshold': 100001}) + assert_ok({'minimum_shard_size': 100, + 'shrink_threshold': 100}) + assert_bad({'minimum_shard_size': 100, + 'shrink_threshold': 100, + 'rows_per_shard': 99}) + + assert_ok({'shard_container_threshold': 100, + 'rows_per_shard': 99}) + assert_bad({'shard_container_threshold': 100, + 'rows_per_shard': 100}) + assert_ok({'rows_per_shard': 10000001}) + + assert_ok({'shard_container_threshold': 100, + 'expansion_limit': 99}) + assert_bad({'shard_container_threshold': 100, + 'expansion_limit': 100}) + assert_ok({'expansion_limit': 100000001})