From 184f71936bd4ccbdb24df83759b7afb65b30ebee Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Wed, 14 Apr 2021 21:37:14 +0200 Subject: [PATCH 1/3] Update all the config option registration code to expose the same API so all the functions take "ignore_errors" argument. For consistnecy, also pass "ignore_errors=True" in places where we parse options as import time side affects (nasty, but that code has a lot of legacy and removing import time side affects would be a large undertaking). Also add not so ideal workaround for "config option already registered" we occasionaly see. Sadly the workaround is not ideal, but per comment above, real fix would require large refactoring + much bigger time investement. --- st2actions/st2actions/config.py | 10 +++---- st2actions/st2actions/notifier/config.py | 16 +++++----- st2actions/st2actions/scheduler/config.py | 21 +++++++------ st2actions/st2actions/workflows/config.py | 16 +++++----- st2api/st2api/config.py | 20 ++++++++----- st2auth/st2auth/config.py | 14 ++++----- st2common/st2common/config.py | 28 +++++++++++------ st2common/st2common/content/bootstrap.py | 1 + st2common/st2common/script_setup.py | 10 ++++++- st2exporter/st2exporter/config.py | 20 ++++++++----- .../st2reactor/garbage_collector/config.py | 30 ++++++++++++------- st2reactor/st2reactor/rules/config.py | 18 ++++++----- st2reactor/st2reactor/timer/config.py | 10 +++---- st2stream/st2stream/config.py | 16 +++++----- 14 files changed, 135 insertions(+), 95 deletions(-) diff --git a/st2actions/st2actions/config.py b/st2actions/st2actions/config.py index 14dc2c4f58..3ed43c3c6e 100644 --- a/st2actions/st2actions/config.py +++ b/st2actions/st2actions/config.py @@ -35,16 +35,16 @@ def parse_args(args=None): ) -def register_opts(): - _register_common_opts() +def register_opts(ignore_errors=False): + _register_common_opts(ignore_errors=ignore_errors) -def _register_common_opts(): - common_config.register_opts() +def _register_common_opts(ignore_errors=False): + common_config.register_opts(ignore_errors=ignore_errors) def get_logging_config_path(): return CONF.actionrunner.logging -register_opts() +register_opts(ignore_errors=True) diff --git a/st2actions/st2actions/notifier/config.py b/st2actions/st2actions/notifier/config.py index 0322179bbc..e5cb03366e 100644 --- a/st2actions/st2actions/notifier/config.py +++ b/st2actions/st2actions/notifier/config.py @@ -34,20 +34,20 @@ def parse_args(args=None): ) -def register_opts(): - _register_common_opts() - _register_notifier_opts() +def register_opts(ignore_errors=False): + _register_common_opts(ignore_errors=ignore_errors) + _register_notifier_opts(ignore_errors=ignore_errors) def get_logging_config_path(): return cfg.CONF.notifier.logging -def _register_common_opts(): +def _register_common_opts(ignore_errors=False): common_config.register_opts() -def _register_notifier_opts(): +def _register_notifier_opts(ignore_errors=False): notifier_opts = [ cfg.StrOpt( "logging", @@ -56,7 +56,9 @@ def _register_notifier_opts(): ) ] - CONF.register_opts(notifier_opts, group="notifier") + common_config.do_register_opts( + notifier_opts, group="notifier", ignore_errors=ignore_errors + ) -register_opts() +register_opts(ignore_errors=True) diff --git a/st2actions/st2actions/scheduler/config.py b/st2actions/st2actions/scheduler/config.py index 8df6c3ff3e..035c030a93 100644 --- a/st2actions/st2actions/scheduler/config.py +++ b/st2actions/st2actions/scheduler/config.py @@ -34,20 +34,20 @@ def parse_args(args=None): ) -def register_opts(): - _register_common_opts() - _register_service_opts() +def register_opts(ignore_errors=False): + _register_common_opts(ignore_errors=ignore_errors) + _register_service_opts(ignore_errors=ignore_errors) def get_logging_config_path(): return cfg.CONF.scheduler.logging -def _register_common_opts(): - common_config.register_opts() +def _register_common_opts(ignore_errors=False): + common_config.register_opts(ignore_errors=ignore_errors) -def _register_service_opts(): +def _register_service_opts(ignore_errors=False): scheduler_opts = [ cfg.StrOpt( "logging", @@ -88,10 +88,9 @@ def _register_service_opts(): ), ] - cfg.CONF.register_opts(scheduler_opts, group="scheduler") + common_config.do_register_opts( + scheduler_opts, group="scheduler", ignore_errors=ignore_errors + ) -try: - register_opts() -except cfg.DuplicateOptError: - LOG.exception("The scheduler configuration options are already parsed and loaded.") +register_opts(ignore_errors=True) diff --git a/st2actions/st2actions/workflows/config.py b/st2actions/st2actions/workflows/config.py index 6854323ddd..bbfa5b22a1 100644 --- a/st2actions/st2actions/workflows/config.py +++ b/st2actions/st2actions/workflows/config.py @@ -30,20 +30,20 @@ def parse_args(args=None): ) -def register_opts(): - _register_common_opts() - _register_service_opts() +def register_opts(ignore_errors=False): + _register_common_opts(ignore_errors=ignore_errors) + _register_service_opts(ignore_errors=ignore_errors) def get_logging_config_path(): return cfg.CONF.workflow_engine.logging -def _register_common_opts(): - common_config.register_opts() +def _register_common_opts(ignore_errors=False): + common_config.register_opts(ignore_errors=ignore_errors) -def _register_service_opts(): +def _register_service_opts(ignore_errors=False): wf_engine_opts = [ cfg.StrOpt( "logging", @@ -52,7 +52,7 @@ def _register_service_opts(): ) ] - cfg.CONF.register_opts(wf_engine_opts, group="workflow_engine") + common_config.do_register_opts(wf_engine_opts, group="workflow_engine") -register_opts() +register_opts(ignore_errors=True) diff --git a/st2api/st2api/config.py b/st2api/st2api/config.py index 35a21d87d5..5009763cdf 100644 --- a/st2api/st2api/config.py +++ b/st2api/st2api/config.py @@ -39,20 +39,20 @@ def parse_args(args=None): ) -def register_opts(): - _register_common_opts() - _register_app_opts() +def register_opts(ignore_errors=False): + _register_common_opts(ignore_errors=ignore_errors) + _register_app_opts(ignore_errors=ignore_errors) -def _register_common_opts(): - common_config.register_opts() +def _register_common_opts(ignore_errors=False): + common_config.register_opts(ignore_errors=ignore_errors) def get_logging_config_path(): return cfg.CONF.api.logging -def _register_app_opts(): +def _register_app_opts(ignore_errors=False): # Note "host", "port", "allow_origin", "mask_secrets" options are registered as part of # st2common config since they are also used outside st2api static_root = os.path.join(cfg.CONF.system.base_path, "static") @@ -72,7 +72,9 @@ def _register_app_opts(): cfg.DictOpt("errors", default={"__force_dict__": True}), ] - CONF.register_opts(pecan_opts, group="api_pecan") + common_config.do_register_opts( + pecan_opts, group="api_pecan", ignore_errors=ignore_errors + ) logging_opts = [ cfg.BoolOpt("debug", default=False), @@ -89,4 +91,6 @@ def _register_app_opts(): ), ] - CONF.register_opts(logging_opts, group="api") + common_config.do_register_opts( + logging_opts, group="api", ignore_errors=ignore_errors + ) diff --git a/st2auth/st2auth/config.py b/st2auth/st2auth/config.py index dee0d2d064..bd57db956f 100644 --- a/st2auth/st2auth/config.py +++ b/st2auth/st2auth/config.py @@ -35,20 +35,20 @@ def parse_args(args=None): ) -def register_opts(): - _register_common_opts() - _register_app_opts() +def register_opts(ignore_errors=False): + _register_common_opts(ignore_errors=ignore_errors) + _register_app_opts(ignore_errors=ignore_errors) def get_logging_config_path(): return cfg.CONF.auth.logging -def _register_common_opts(): - st2cfg.register_opts() +def _register_common_opts(ignore_errors=False): + st2cfg.register_opts(ignore_errors=ignore_errors) -def _register_app_opts(): +def _register_app_opts(ignore_errors=False): available_backends = auth_backends.get_available_backends() auth_opts = [ @@ -110,4 +110,4 @@ def _register_app_opts(): ), ] - cfg.CONF.register_cli_opts(auth_opts, group="auth") + st2cfg.do_register_cli_opts(auth_opts, group="auth", ignore_errors=ignore_errors) diff --git a/st2common/st2common/config.py b/st2common/st2common/config.py index d97cf08546..0429c81ea2 100644 --- a/st2common/st2common/config.py +++ b/st2common/st2common/config.py @@ -36,15 +36,19 @@ def do_register_opts(opts, group=None, ignore_errors=False): raise -def do_register_cli_opts(opt, ignore_errors=False): +def do_register_cli_opts(opt, ignore_errors=False, group=None): # TODO: This function has broken name, it should work with lists :/ if not isinstance(opt, (list, tuple)): opts = [opt] else: opts = opt + kwargs = {} + if group: + kwargs["group"] = group + try: - cfg.CONF.register_cli_opts(opts) + cfg.CONF.register_cli_opts(opts, **kwargs) except: if not ignore_errors: raise @@ -454,7 +458,9 @@ def register_opts(ignore_errors=False): ), ] - do_register_opts(action_runner_opts, group="actionrunner") + do_register_opts( + action_runner_opts, group="actionrunner", ignore_errors=ignore_errors + ) dispatcher_pool_opts = [ cfg.IntOpt( @@ -469,7 +475,9 @@ def register_opts(ignore_errors=False): ), ] - do_register_opts(dispatcher_pool_opts, group="actionrunner") + do_register_opts( + dispatcher_pool_opts, group="actionrunner", ignore_errors=ignore_errors + ) ssh_runner_opts = [ cfg.StrOpt( @@ -505,7 +513,7 @@ def register_opts(ignore_errors=False): ), ] - do_register_opts(ssh_runner_opts, group="ssh_runner") + do_register_opts(ssh_runner_opts, group="ssh_runner", ignore_errors=ignore_errors) # Common options (used by action runner and sensor container) action_sensor_opts = [ @@ -521,7 +529,9 @@ def register_opts(ignore_errors=False): ), ] - do_register_opts(action_sensor_opts, group="action_sensor") + do_register_opts( + action_sensor_opts, group="action_sensor", ignore_errors=ignore_errors + ) # Common options for content pack_lib_opts = [ @@ -538,7 +548,7 @@ def register_opts(ignore_errors=False): ) ] - do_register_opts(pack_lib_opts, group="packs") + do_register_opts(pack_lib_opts, group="packs", ignore_errors=ignore_errors) # Coordination options coord_opts = [ @@ -719,8 +729,8 @@ def register_opts(ignore_errors=False): ) -def parse_args(args=None): - register_opts() +def parse_args(args=None, ignore_errors=False): + register_opts(ignore_errors=ignore_errors) cfg.CONF( args=args, version=VERSION_STRING, diff --git a/st2common/st2common/content/bootstrap.py b/st2common/st2common/content/bootstrap.py index 31e60bd75c..9ae04664ad 100644 --- a/st2common/st2common/content/bootstrap.py +++ b/st2common/st2common/content/bootstrap.py @@ -437,6 +437,7 @@ def setup(argv): setup_db=True, register_mq_exchanges=True, register_internal_trigger_types=True, + ignore_register_config_opts_errors=True, ) diff --git a/st2common/st2common/script_setup.py b/st2common/st2common/script_setup.py index 0abb7e8269..5b9d50d1e6 100644 --- a/st2common/st2common/script_setup.py +++ b/st2common/st2common/script_setup.py @@ -49,6 +49,7 @@ def setup( setup_db=True, register_mq_exchanges=True, register_internal_trigger_types=False, + ignore_register_config_opts_errors=False, ): """ Common setup function. @@ -67,7 +68,14 @@ def setup( register_common_cli_options() # Parse args to setup config - config.parse_args() + # NOTE: This code is not the best, but it's only realistic option we have at this point. + # Refactoring all the code and config modules to avoid import time side affects would be a big + # rabbit hole. Luckily registering same options twice is not really a big deal or fatal error + # so we simply ignore such errors. + if config.__name__ == "st2common.config" and ignore_register_config_opts_errors: + config.parse_args(ignore_errors=True) + else: + config.parse_args() if cfg.CONF.debug: cfg.CONF.verbose = True diff --git a/st2exporter/st2exporter/config.py b/st2exporter/st2exporter/config.py index 83f4f45d5d..fe293f7cd7 100644 --- a/st2exporter/st2exporter/config.py +++ b/st2exporter/st2exporter/config.py @@ -42,16 +42,16 @@ def get_logging_config_path(): return cfg.CONF.exporter.logging -def register_opts(): - _register_common_opts() - _register_app_opts() +def _register_opts(ignore_errors=False): + _register_common_opts(ignore_errors=ignore_errors) + _register_app_opts(ignore_errors=ignore_errors) -def _register_common_opts(): - common_config.register_opts() +def _register_common_opts(ignore_errors=False): + common_config.register_opts(ignore_errors=ignore_errors) -def _register_app_opts(): +def _register_app_opts(ignore_errors=False): dump_opts = [ cfg.StrOpt( "dump_dir", @@ -60,7 +60,9 @@ def _register_app_opts(): ) ] - CONF.register_opts(dump_opts, group="exporter") + common_config.do_register_opts( + dump_opts, group="exporter", ignore_errors=ignore_errors + ) logging_opts = [ cfg.StrOpt( @@ -70,4 +72,6 @@ def _register_app_opts(): ) ] - CONF.register_opts(logging_opts, group="exporter") + common_config.do_register_opts( + logging_opts, group="exporter", ignore_errors=ignore_errors + ) diff --git a/st2reactor/st2reactor/garbage_collector/config.py b/st2reactor/st2reactor/garbage_collector/config.py index 9a0faf0dec..af579245f0 100644 --- a/st2reactor/st2reactor/garbage_collector/config.py +++ b/st2reactor/st2reactor/garbage_collector/config.py @@ -36,20 +36,20 @@ def parse_args(args=None): ) -def register_opts(): - _register_common_opts() - _register_garbage_collector_opts() +def register_opts(ignore_errors=False): + _register_common_opts(ignore_errors=ignore_errors) + _register_garbage_collector_opts(ignore_errors=ignore_errors) def get_logging_config_path(): return cfg.CONF.garbagecollector.logging -def _register_common_opts(): - common_config.register_opts() +def _register_common_opts(ignore_errors=False): + common_config.register_opts(ignore_errors=ignore_errors) -def _register_garbage_collector_opts(): +def _register_garbage_collector_opts(ignore_errors=False): logging_opts = [ cfg.StrOpt( "logging", @@ -58,7 +58,9 @@ def _register_garbage_collector_opts(): ) ] - CONF.register_opts(logging_opts, group="garbagecollector") + common_config.do_register_opts( + logging_opts, group="garbagecollector", ignore_errors=ignore_errors + ) common_opts = [ cfg.IntOpt( @@ -74,7 +76,9 @@ def _register_garbage_collector_opts(): ), ] - CONF.register_opts(common_opts, group="garbagecollector") + common_config.do_register_opts( + common_opts, group="garbagecollector", ignore_errors=ignore_errors + ) ttl_opts = [ cfg.IntOpt( @@ -96,7 +100,9 @@ def _register_garbage_collector_opts(): ), ] - CONF.register_opts(ttl_opts, group="garbagecollector") + common_config.do_register_opts( + ttl_opts, group="garbagecollector", ignore_errors=ignore_errors + ) inquiry_opts = [ cfg.BoolOpt( @@ -107,7 +113,9 @@ def _register_garbage_collector_opts(): ) ] - CONF.register_opts(inquiry_opts, group="garbagecollector") + common_config.do_register_opts( + inquiry_opts, group="garbagecollector", ignore_errors=ignore_errors + ) -register_opts() +register_opts(ignore_errors=True) diff --git a/st2reactor/st2reactor/rules/config.py b/st2reactor/st2reactor/rules/config.py index 637ef4e457..49922b2314 100644 --- a/st2reactor/st2reactor/rules/config.py +++ b/st2reactor/st2reactor/rules/config.py @@ -34,20 +34,20 @@ def parse_args(args=None): ) -def register_opts(): - _register_common_opts() - _register_rules_engine_opts() +def register_opts(ignore_errors=False): + _register_common_opts(ignore_errors=ignore_errors) + _register_rules_engine_opts(ignore_errors=ignore_errors) def get_logging_config_path(): return cfg.CONF.rulesengine.logging -def _register_common_opts(): - common_config.register_opts() +def _register_common_opts(ignore_errors=False): + common_config.register_opts(ignore_errors=ignore_errors) -def _register_rules_engine_opts(): +def _register_rules_engine_opts(ignore_errors=False): logging_opts = [ cfg.StrOpt( "logging", @@ -56,7 +56,9 @@ def _register_rules_engine_opts(): ) ] - CONF.register_opts(logging_opts, group="rulesengine") + common_config.do_register_opts( + logging_opts, group="rulesengine", ignore_errors=ignore_errors + ) -register_opts() +register_opts(ignore_errors=True) diff --git a/st2reactor/st2reactor/timer/config.py b/st2reactor/st2reactor/timer/config.py index bbc1020cb9..0301ca4a7f 100644 --- a/st2reactor/st2reactor/timer/config.py +++ b/st2reactor/st2reactor/timer/config.py @@ -32,16 +32,16 @@ def parse_args(args=None): ) -def register_opts(): - _register_common_opts() +def register_opts(ignore_errors=False): + _register_common_opts(ignore_errors=ignore_errors) def get_logging_config_path(): return cfg.CONF.timersengine.logging -def _register_common_opts(): - common_config.register_opts() +def _register_common_opts(ignore_errors=False): + common_config.register_opts(ignore_errors=ignore_errors) -register_opts() +register_opts(ignore_errors=True) diff --git a/st2stream/st2stream/config.py b/st2stream/st2stream/config.py index bc117b556a..eef5c55830 100644 --- a/st2stream/st2stream/config.py +++ b/st2stream/st2stream/config.py @@ -39,20 +39,20 @@ def parse_args(args=None): ) -def register_opts(): - _register_common_opts() - _register_app_opts() +def register_opts(ignore_errors=False): + _register_common_opts(ignore_errors=ignore_errors) + _register_app_opts(ignore_errors=ignore_errors) -def _register_common_opts(): - common_config.register_opts() +def _register_common_opts(ignore_errors=False): + common_config.register_opts(ignore_errors=ignore_errors) def get_logging_config_path(): return cfg.CONF.stream.logging -def _register_app_opts(): +def _register_app_opts(ignore_errors=False): # Note "allow_origin", "mask_secrets", "heartbeat" options are registered as part of st2common # config since they are also used outside st2stream api_opts = [ @@ -68,4 +68,6 @@ def _register_app_opts(): ), ] - CONF.register_opts(api_opts, group="stream") + common_config.do_register_opts( + api_opts, group="stream", ignore_errors=ignore_errors + ) From f2b72465ccb429fb39d03dc610ab3e0672babb24 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Wed, 14 Apr 2021 22:56:52 +0200 Subject: [PATCH 2/3] Add changelog entry. --- CHANGELOG.rst | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index c042c845ed..40cfaf3e5a 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -188,6 +188,12 @@ Changed Contributed by @Kami. +* Some of the config option registration code has been refactored to ignore "option already + registered" errors. That was done as a work around for an occasional race in the tests and + also to make all of the config option registration code expose the same consistent API. #5234 + + Contributed by @Kami. + Improvements ~~~~~~~~~~~~ From 5856fa482f3371696ca0574b4576722c34ee3e8d Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Wed, 14 Apr 2021 23:04:28 +0200 Subject: [PATCH 3/3] Fix the test and make it more robust - we want to wait on a specific number of children since rest of the code asserts that 2 childrens are returned. Previously version of code would already return in case there is just 1 children which means the tests would fail and be prone to timing / race conditions. --- .../unit/test_actionchain_notifications.py | 31 +++++++++++++++---- 1 file changed, 25 insertions(+), 6 deletions(-) diff --git a/contrib/runners/action_chain_runner/tests/unit/test_actionchain_notifications.py b/contrib/runners/action_chain_runner/tests/unit/test_actionchain_notifications.py index 20dca00f0a..d74efa37f2 100644 --- a/contrib/runners/action_chain_runner/tests/unit/test_actionchain_notifications.py +++ b/contrib/runners/action_chain_runner/tests/unit/test_actionchain_notifications.py @@ -156,7 +156,7 @@ def test_skip_notify_for_task_with_notify(self): ) self.assertIsNone(task1_live.notify) - execution = self._wait_for_children(execution, retries=300) + execution = self._wait_for_children(execution, expected_children=2, retries=300) self.assertEqual(len(execution.children), 2) # Assert task2 notify is not skipped @@ -194,7 +194,7 @@ def test_skip_notify_default_for_task_with_notify(self): ) self.assertEqual(notify, MOCK_NOTIFY) - execution = self._wait_for_children(execution, retries=300) + execution = self._wait_for_children(execution, expected_children=2, retries=300) self.assertEqual(len(execution.children), 2) # Assert task2 notify is not skipped by default. @@ -203,12 +203,31 @@ def test_skip_notify_default_for_task_with_notify(self): self.assertIsNone(task2_live.notify) MockLiveActionPublisherNonBlocking.wait_all() - def _wait_for_children(self, execution, interval=0.1, retries=100): + def _wait_for_children( + self, execution, expected_children=1, interval=0.1, retries=100 + ): # Wait until the execution has children. for i in range(0, retries): execution = ActionExecution.get_by_id(str(execution.id)) - if len(getattr(execution, "children", [])) <= 0: - eventlet.sleep(interval) - continue + found_children = len(getattr(execution, "children", [])) + + if found_children == expected_children: + return execution + + if found_children > expected_children: + raise AssertionError( + "Expected %s children, but got %s" + % (expected_children, found_children) + ) + + eventlet.sleep(interval) + + found_children = len(getattr(execution, "children", [])) + + if found_children != expected_children: + raise AssertionError( + "Expected %s children, but got %s after %s retry attempts" + % (expected_children, found_children, retries) + ) return execution