From f3adce13753f9b32be60a68b49c79321cb552a61 Mon Sep 17 00:00:00 2001 From: Clay Gerrard Date: Thu, 23 May 2024 09:28:58 -0500 Subject: [PATCH] expirer: bad config should not loop forever Change-Id: I9413c72f41465fb8026848f71ec3b39fa990c3b7 --- swift/obj/expirer.py | 35 ++++++----- test/unit/obj/test_expirer.py | 108 ++++++++++++++++++++++++---------- 2 files changed, 99 insertions(+), 44 deletions(-) diff --git a/swift/obj/expirer.py b/swift/obj/expirer.py index 6d8f864b47..122ee80cf5 100644 --- a/swift/obj/expirer.py +++ b/swift/obj/expirer.py @@ -29,7 +29,7 @@ from swift.common.daemon import Daemon from swift.common.internal_client import InternalClient, UnexpectedResponse from swift.common.utils import get_logger, dump_recon_cache, split_path, \ Timestamp, config_true_value, normalize_delete_at_timestamp, \ - RateLimitedIterator, md5, non_negative_float + RateLimitedIterator, md5, non_negative_float, non_negative_int from swift.common.http import HTTP_NOT_FOUND, HTTP_CONFLICT, \ HTTP_PRECONDITION_FAILED from swift.common.recon import RECON_OBJECT_FILE, DEFAULT_RECON_CACHE_PATH @@ -174,8 +174,9 @@ class ObjectExpirer(Daemon): global_conf={'log_name': '%s-ic' % self.conf.get( 'log_name', self.log_route)}) - self.processes = int(self.conf.get('processes', 0)) - self.process = int(self.conf.get('process', 0)) + self.processes = non_negative_int(self.conf.get('processes', 0)) + self.process = non_negative_int(self.conf.get('process', 0)) + self._validate_processes_config() def report(self, final=False): """ @@ -330,7 +331,7 @@ class ObjectExpirer(Daemon): # we shouldn't yield the object during the delay continue - # Only one expirer daemon assigned for one task + # Only one expirer daemon assigned for each task if self.hash_mod('%s/%s' % (task_container, task_object), divisor) != my_index: continue @@ -365,6 +366,9 @@ class ObjectExpirer(Daemon): These will override the values from the config file if provided. """ + # these config values are available to override at the command line, + # blow-up now if they're wrong + self.override_proceses_config_from_command_line(**kwargs) # This if-clause will be removed when general task queue feature is # implemented. if not self.dequeue_from_legacy: @@ -375,7 +379,6 @@ class ObjectExpirer(Daemon): 'with dequeue_from_legacy == true.') return - self.get_process_values(kwargs) pool = GreenPool(self.concurrency) self.report_first_time = self.report_last_time = time() self.report_objects = 0 @@ -430,6 +433,9 @@ class ObjectExpirer(Daemon): :param kwargs: Extra keyword args to fulfill the Daemon interface; this daemon has no additional keyword args. """ + # these config values are available to override at the command line + # blow-up now if they're wrong + self.override_proceses_config_from_command_line(**kwargs) sleep(random() * self.interval) while True: begin = time() @@ -441,7 +447,7 @@ class ObjectExpirer(Daemon): if elapsed < self.interval: sleep(random() * (self.interval - elapsed)) - def get_process_values(self, kwargs): + def override_proceses_config_from_command_line(self, **kwargs): """ Sets self.processes and self.process from the kwargs if those values exist, otherwise, leaves those values as they were set in @@ -452,19 +458,20 @@ class ObjectExpirer(Daemon): line when the daemon is run. """ if kwargs.get('processes') is not None: - self.processes = int(kwargs['processes']) + self.processes = non_negative_int(kwargs['processes']) if kwargs.get('process') is not None: - self.process = int(kwargs['process']) + self.process = non_negative_int(kwargs['process']) - if self.process < 0: - raise ValueError( - 'process must be an integer greater than or equal to 0') + self._validate_processes_config() - if self.processes < 0: - raise ValueError( - 'processes must be an integer greater than or equal to 0') + def _validate_processes_config(self): + """ + Used in constructor and in override_proceses_config_from_command_line + to validate the processes configuration requirements. + :raiess: ValueError if processes config is invalid + """ if self.processes and self.process >= self.processes: raise ValueError( 'process must be less than processes') diff --git a/test/unit/obj/test_expirer.py b/test/unit/obj/test_expirer.py index 434e8fa21e..4728730eb0 100644 --- a/test/unit/obj/test_expirer.py +++ b/test/unit/obj/test_expirer.py @@ -195,94 +195,87 @@ class TestObjectExpirer(TestCase): _do_test_init_ic_log_name({'log_name': 'my-object-expirer'}, 'my-object-expirer-ic') - def test_get_process_values_from_kwargs(self): + def test_set_process_values_from_kwargs(self): x = expirer.ObjectExpirer({}, swift=self.fake_swift) vals = { 'processes': 5, 'process': 1, } - x.get_process_values(vals) + x.override_proceses_config_from_command_line(**vals) self.assertEqual(x.processes, 5) self.assertEqual(x.process, 1) - def test_get_process_values_from_config(self): - vals = { + def test_set_process_values_from_config(self): + conf = { 'processes': 5, 'process': 1, } - x = expirer.ObjectExpirer(vals, swift=self.fake_swift) - x.get_process_values({}) + x = expirer.ObjectExpirer(conf, swift=self.fake_swift) self.assertEqual(x.processes, 5) self.assertEqual(x.process, 1) - def test_get_process_values_negative_process(self): + def test_set_process_values_negative_process(self): vals = { 'processes': 5, 'process': -1, } # from config - x = expirer.ObjectExpirer(vals, swift=self.fake_swift) - expected_msg = 'process must be an integer greater' \ - ' than or equal to 0' + expected_msg = 'must be a non-negative integer' with self.assertRaises(ValueError) as ctx: - x.get_process_values({}) - self.assertEqual(str(ctx.exception), expected_msg) + expirer.ObjectExpirer(vals, swift=self.fake_swift) + self.assertIn(expected_msg, str(ctx.exception)) # from kwargs x = expirer.ObjectExpirer({}, swift=self.fake_swift) with self.assertRaises(ValueError) as ctx: - x.get_process_values(vals) - self.assertEqual(str(ctx.exception), expected_msg) + x.override_proceses_config_from_command_line(**vals) + self.assertIn(expected_msg, str(ctx.exception)) - def test_get_process_values_negative_processes(self): + def test_set_process_values_negative_processes(self): vals = { 'processes': -5, 'process': 1, } # from config - x = expirer.ObjectExpirer(vals, swift=self.fake_swift) - expected_msg = 'processes must be an integer greater' \ - ' than or equal to 0' + expected_msg = 'must be a non-negative integer' with self.assertRaises(ValueError) as ctx: - x.get_process_values({}) - self.assertEqual(str(ctx.exception), expected_msg) + expirer.ObjectExpirer(vals, swift=self.fake_swift) + self.assertIn(expected_msg, str(ctx.exception)) # from kwargs x = expirer.ObjectExpirer({}, swift=self.fake_swift) with self.assertRaises(ValueError) as ctx: - x.get_process_values(vals) - self.assertEqual(str(ctx.exception), expected_msg) + x.override_proceses_config_from_command_line(**vals) + self.assertIn(expected_msg, str(ctx.exception)) - def test_get_process_values_process_greater_than_processes(self): + def test_set_process_values_process_greater_than_processes(self): vals = { 'processes': 5, 'process': 7, } # from config - x = expirer.ObjectExpirer(vals, swift=self.fake_swift) expected_msg = 'process must be less than processes' with self.assertRaises(ValueError) as ctx: - x.get_process_values({}) + x = expirer.ObjectExpirer(vals, swift=self.fake_swift) self.assertEqual(str(ctx.exception), expected_msg) # from kwargs x = expirer.ObjectExpirer({}, swift=self.fake_swift) with self.assertRaises(ValueError) as ctx: - x.get_process_values(vals) + x.override_proceses_config_from_command_line(**vals) self.assertEqual(str(ctx.exception), expected_msg) - def test_get_process_values_process_equal_to_processes(self): + def test_set_process_values_process_equal_to_processes(self): vals = { 'processes': 5, 'process': 5, } # from config - x = expirer.ObjectExpirer(vals, swift=self.fake_swift) expected_msg = 'process must be less than processes' with self.assertRaises(ValueError) as ctx: - x.get_process_values({}) + expirer.ObjectExpirer(vals, swift=self.fake_swift) self.assertEqual(str(ctx.exception), expected_msg) # from kwargs x = expirer.ObjectExpirer({}, swift=self.fake_swift) with self.assertRaises(ValueError) as ctx: - x.get_process_values(vals) + x.override_proceses_config_from_command_line(**vals) self.assertEqual(str(ctx.exception), expected_msg) def test_valid_delay_reaping(self): @@ -1375,6 +1368,61 @@ class TestObjectExpirer(TestCase): self.assertEqual(str(log_kwargs['exc_info'][1]), 'exception 1') + def test_run_forever_bad_process_values_config(self): + conf = { + 'processes': -1, + 'process': -2, + 'interval': 1, + } + iterations = [0] + + def wrap_with_exit(orig_f, exit_after_count=3): + def wrapped_f(*args, **kwargs): + iterations[0] += 1 + if iterations[0] > exit_after_count: + raise SystemExit('that is enough for now') + return orig_f(*args, **kwargs) + return wrapped_f + + with self.assertRaises(ValueError) as ctx: + # we should blow up here + x = expirer.ObjectExpirer(conf, logger=self.logger, + swift=self.fake_swift) + x.pop_queue = lambda a, c, o: None + x.run_once = wrap_with_exit(x.run_once) + # at least we should hopefully we blow up here? + x.run_forever() + + # bad config should exit immediately with ValueError + self.assertIn('must be a non-negative integer', str(ctx.exception)) + + def test_run_forever_bad_process_values_command_line(self): + conf = { + 'interval': 1, + } + bad_kwargs = { + 'processes': -1, + 'process': -2, + } + iterations = [0] + + def wrap_with_exit(orig_f, exit_after_count=3): + def wrapped_f(*args, **kwargs): + iterations[0] += 1 + if iterations[0] > exit_after_count: + raise SystemExit('that is enough for now') + return orig_f(*args, **kwargs) + return wrapped_f + + with self.assertRaises(ValueError) as ctx: + x = expirer.ObjectExpirer(conf, logger=self.logger, + swift=self.fake_swift) + x.run_once = wrap_with_exit(x.run_once) + x.run_forever(**bad_kwargs) + + # bad command args should exit immediately with ValueError + self.assertIn('must be a non-negative integer', str(ctx.exception)) + def test_delete_actual_object(self): got_env = [None]