From f601dc96b2050ee8a6a3110d24e6f6d30c13cdf6 Mon Sep 17 00:00:00 2001 From: Oliver Sanders Date: Mon, 6 Jun 2022 16:24:06 +0100 Subject: [PATCH 1/2] main loop: improve defaults, docs and error handling * Addresses https://github.com/cylc/cylc-flow/issues/4873 * There is now a default main loop plugin interval of `PT10M`. This prevents any plugins without a hardcoded default from being run with every main loop cycle. * Explicitly set a default for the `auto restart` plugin (also `PT10M`). * Added a note on enabling plugins. * Suppress traceback from reloading the global config in the `auto restart` plugin. This is now logged nicely. * Prevent plugins from being loaded twice when specified both on the CLI (--main-loop) and in the config (global.cylc[scheduler][main loop]plugins). --- cylc/flow/cfgspec/globalcfg.py | 53 ++++++++++++++++++++--- cylc/flow/main_loop/__init__.py | 2 +- cylc/flow/main_loop/auto_restart.py | 14 +++++- tests/unit/main_loop/test_auto_restart.py | 36 +++++++++++++-- 4 files changed, 91 insertions(+), 14 deletions(-) diff --git a/cylc/flow/cfgspec/globalcfg.py b/cylc/flow/cfgspec/globalcfg.py index f14d6c4c165..5b5116fbcdc 100644 --- a/cylc/flow/cfgspec/globalcfg.py +++ b/cylc/flow/cfgspec/globalcfg.py @@ -643,19 +643,42 @@ .. versionadded:: 8.0.0 '''): - Conf('plugins', VDR.V_STRING_LIST, - ['health check', 'reset bad hosts'], - desc=''' - Configure the default main loop plugins to use when - starting new workflows. + Conf( + 'plugins', + VDR.V_STRING_LIST, + ['health check', 'reset bad hosts'], + desc=''' + Configure the default main loop plugins to use when + starting new workflows. + + Only enabled plugins are loaded, plugins can be enabled + in three ways: + + Globally: + To enable a plugin for all workflows add it to: + :cylc:conf:`global.cylc[scheduler][main loop]plugins` + Per-Run: + To enable a plugin for a one-off run of a workflow, + specify it on the command line with + ``cylc play --main-loop``. + + .. hint:: - .. versionadded:: 8.0.0 + This *appends* to the configured list of plugins + rather than *overriding* it. + + .. versionadded:: 8.0.0 ''') with Conf('', desc=''' Configure a main loop plugin. + + .. note:: + + Only the configured list of :cylc:conf:`[..]plugins` + are loaded when a scheduler is started. ''') as MainLoopPlugin: - Conf('interval', VDR.V_INTERVAL, desc=''' + Conf('interval', VDR.V_INTERVAL, DurationFloat(600), desc=''' :Default For: :cylc:conf:`flow.cylc \ [scheduler][main loop][]interval`. @@ -675,6 +698,22 @@ .. versionadded:: 8.0.0 ''') + with Conf('auto restart', meta=MainLoopPlugin, desc=''' + Automatically migrates workflows between servers. + + For more information see: + + * :ref:`Submitting Workflows To a Pool Of Hosts`. + * :py:mod:`cylc.flow.main_loop.auto_restart` + + .. versionadded:: 8.0.0 + '''): + Conf('interval', VDR.V_INTERVAL, DurationFloat(600), desc=''' + The interval with which this plugin is run. + + .. versionadded:: 8.0.0 + ''') + with Conf('reset bad hosts', meta=MainLoopPlugin, desc=''' Periodically clear the scheduler list of unreachable (bad) hosts. diff --git a/cylc/flow/main_loop/__init__.py b/cylc/flow/main_loop/__init__.py index bd48fea294b..23a18c9c077 100644 --- a/cylc/flow/main_loop/__init__.py +++ b/cylc/flow/main_loop/__init__.py @@ -321,7 +321,7 @@ def load(config, additional_plugins=None): 'state': {}, 'timings': {} } - for plugin_name in config['plugins'] + additional_plugins: + for plugin_name in set(config['plugins'] + additional_plugins): # get plugin try: entry_point = entry_points[plugin_name.replace(' ', '_')] diff --git a/cylc/flow/main_loop/auto_restart.py b/cylc/flow/main_loop/auto_restart.py index 019edc9d367..cb1c43ae619 100644 --- a/cylc/flow/main_loop/auto_restart.py +++ b/cylc/flow/main_loop/auto_restart.py @@ -91,10 +91,11 @@ from cylc.flow import LOG from cylc.flow.cfgspec.glbl_cfg import glbl_cfg -from cylc.flow.exceptions import HostSelectException +from cylc.flow.exceptions import CylcConfigError, HostSelectException from cylc.flow.host_select import select_workflow_host from cylc.flow.hostuserutil import get_fqdn_by_host from cylc.flow.main_loop import periodic +from cylc.flow.parsec.exceptions import ParsecError from cylc.flow.workflow_status import AutoRestartMode from cylc.flow.wallclock import ( get_time_string_from_unix_time as time2str @@ -104,7 +105,16 @@ @periodic async def auto_restart(scheduler, _): """Automatically restart the workflow if configured to do so.""" - current_glbl_cfg = glbl_cfg(cached=False) + try: + current_glbl_cfg = glbl_cfg(cached=False) + except (CylcConfigError, ParsecError) as exc: + LOG.error( + 'auto restart: an error in the global config is preventing it from' + f' being reloaded:\n{exc}' + ) + # skip check - we can't do anything until the global config has been + # fixed + return False # return False to make testing easier mode = _should_auto_restart(scheduler, current_glbl_cfg) if mode: diff --git a/tests/unit/main_loop/test_auto_restart.py b/tests/unit/main_loop/test_auto_restart.py index a7c65571df9..20fc303bd0f 100644 --- a/tests/unit/main_loop/test_auto_restart.py +++ b/tests/unit/main_loop/test_auto_restart.py @@ -20,13 +20,14 @@ import pytest from cylc.flow import CYLC_LOG -from cylc.flow.exceptions import HostSelectException -from cylc.flow.hostuserutil import get_fqdn_by_host +from cylc.flow.exceptions import CylcConfigError, HostSelectException from cylc.flow.main_loop.auto_restart import ( - _should_auto_restart, _can_auto_restart, - _set_auto_restart + _set_auto_restart, + _should_auto_restart, + auto_restart, ) +from cylc.flow.parsec.exceptions import ParsecError from cylc.flow.workflow_status import ( AutoRestartMode, StopMode @@ -280,3 +281,30 @@ def workflow_select_pass(**_): [(*_, msg)] = caplog.record_tuples assert 'will automatically restart' in msg assert called + + +@pytest.mark.parametrize('exc_class', [ParsecError, CylcConfigError]) +async def test_log_config_error(caplog, log_filter, monkeypatch, exc_class): + """It should log errors in the global config. + + When errors are present in the global config they should be caught and + logged nicely rather than left to spill over as traceback in the log. + """ + # make the global config raise an error + def global_config_load_error(*args, **kwargs): + nonlocal exc_class + raise exc_class('something event more bizarrely inexplicable') + + monkeypatch.setattr( + 'cylc.flow.main_loop.auto_restart.glbl_cfg', + global_config_load_error, + ) + + # call the auto_restart plugin, the error should be caught + caplog.clear() + assert await auto_restart(None, None) is False + + # the error should have been logged + assert len(caplog.messages) == 1 + assert 'an error in the global config' in caplog.messages[0] + assert 'something event more bizarrely inexplicable' in caplog.messages[0] From ec8865dfafc7a036eb982b88cac5e1e291aceb21 Mon Sep 17 00:00:00 2001 From: Oliver Sanders Date: Tue, 7 Jun 2022 09:26:27 +0100 Subject: [PATCH 2/2] Apply suggestions from code review Co-authored-by: Hilary James Oliver --- cylc/flow/cfgspec/globalcfg.py | 6 +++--- tests/unit/main_loop/test_auto_restart.py | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/cylc/flow/cfgspec/globalcfg.py b/cylc/flow/cfgspec/globalcfg.py index 5b5116fbcdc..b67be463a3c 100644 --- a/cylc/flow/cfgspec/globalcfg.py +++ b/cylc/flow/cfgspec/globalcfg.py @@ -652,7 +652,7 @@ starting new workflows. Only enabled plugins are loaded, plugins can be enabled - in three ways: + in two ways: Globally: To enable a plugin for all workflows add it to: @@ -676,7 +676,7 @@ .. note:: Only the configured list of :cylc:conf:`[..]plugins` - are loaded when a scheduler is started. + is loaded when a scheduler is started. ''') as MainLoopPlugin: Conf('interval', VDR.V_INTERVAL, DurationFloat(600), desc=''' :Default For: :cylc:conf:`flow.cylc \ @@ -709,7 +709,7 @@ .. versionadded:: 8.0.0 '''): Conf('interval', VDR.V_INTERVAL, DurationFloat(600), desc=''' - The interval with which this plugin is run. + The interval between runs of this plugin. .. versionadded:: 8.0.0 ''') diff --git a/tests/unit/main_loop/test_auto_restart.py b/tests/unit/main_loop/test_auto_restart.py index 20fc303bd0f..8b8e33cf40b 100644 --- a/tests/unit/main_loop/test_auto_restart.py +++ b/tests/unit/main_loop/test_auto_restart.py @@ -293,7 +293,7 @@ async def test_log_config_error(caplog, log_filter, monkeypatch, exc_class): # make the global config raise an error def global_config_load_error(*args, **kwargs): nonlocal exc_class - raise exc_class('something event more bizarrely inexplicable') + raise exc_class('something even more bizarrely inexplicable') monkeypatch.setattr( 'cylc.flow.main_loop.auto_restart.glbl_cfg', @@ -307,4 +307,4 @@ def global_config_load_error(*args, **kwargs): # the error should have been logged assert len(caplog.messages) == 1 assert 'an error in the global config' in caplog.messages[0] - assert 'something event more bizarrely inexplicable' in caplog.messages[0] + assert 'something even more bizarrely inexplicable' in caplog.messages[0]