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 cb6ac22a05..6eb8076ac0 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 d06c516f7f..c06fe3e7c1 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, @@ -4224,6 +4225,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: @@ -4240,6 +4242,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) @@ -4325,6 +4328,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: @@ -4413,6 +4417,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) @@ -7457,3 +7462,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})