Merge "sharder: add more validation checks on config"

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

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 '

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')

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})