From c5db3488ce30dac7a994479e1e7991ed51320b79 Mon Sep 17 00:00:00 2001 From: Oliver Sanders Date: Fri, 19 Jul 2024 13:38:46 +0100 Subject: [PATCH 1/2] glbl_cfg: reload without mutating the original * Use the new `reload` kwarg rather than calling the `.load()` method. * Fixes https://github.com/cylc/cylc-flow/issues/6244 * The `.load()` method mutates the existing config, due to the use of logging within (and outside of) this routine and the use of `glbl_cfg` in the logging, this created a race condition. * The new `reload` kwarg creates a new config instance, then sets this as the default. --- changes.d/6249.fix.md | 1 + cylc/flow/cfgspec/glbl_cfg.py | 4 +- cylc/flow/cfgspec/globalcfg.py | 35 +++++++------ tests/integration/test_config.py | 90 ++++++++++++++++++++++++++++++++ 4 files changed, 113 insertions(+), 17 deletions(-) create mode 100644 changes.d/6249.fix.md diff --git a/changes.d/6249.fix.md b/changes.d/6249.fix.md new file mode 100644 index 00000000000..5bcdbba75c9 --- /dev/null +++ b/changes.d/6249.fix.md @@ -0,0 +1 @@ +Fix a race condition between global config reload and debug logging that caused "platform not defined" errors when running workflows that contained a "rose-suite.conf" file in vebose or debug mode. diff --git a/cylc/flow/cfgspec/glbl_cfg.py b/cylc/flow/cfgspec/glbl_cfg.py index 136a21e2df1..b2ce8503f8c 100644 --- a/cylc/flow/cfgspec/glbl_cfg.py +++ b/cylc/flow/cfgspec/glbl_cfg.py @@ -16,7 +16,7 @@ """Allow lazy loading of `cylc.flow.cfgspec.globalcfg`.""" -def glbl_cfg(cached=True): +def glbl_cfg(**kwargs): """Load and return the global configuration singleton instance.""" from cylc.flow.cfgspec.globalcfg import GlobalConfig - return GlobalConfig.get_inst(cached=cached) + return GlobalConfig.get_inst(**kwargs) diff --git a/cylc/flow/cfgspec/globalcfg.py b/cylc/flow/cfgspec/globalcfg.py index db8c7bdd5ff..7dc04c99a02 100644 --- a/cylc/flow/cfgspec/globalcfg.py +++ b/cylc/flow/cfgspec/globalcfg.py @@ -1978,24 +1978,29 @@ def __init__(self, *args, **kwargs) -> None: super().__init__(*args, **kwargs) @classmethod - def get_inst(cls, cached: bool = True) -> 'GlobalConfig': + def get_inst( + cls, cached: bool = True, reload: bool = False + ) -> 'GlobalConfig': """Return a GlobalConfig instance. Args: - cached (bool): - If cached create if necessary and return the singleton - instance, else return a new instance. + cached: + If True, return a cached instance if present. If False, return + a new instance. + reload: + If true, reload the cached instance (implies cached=True). + """ - if not cached: - # Return an up-to-date global config without affecting the - # singleton. - new_instance = cls(SPEC, upg, validator=cylc_config_validate) - new_instance.load() - return new_instance - elif not cls._DEFAULT: - cls._DEFAULT = cls(SPEC, upg, validator=cylc_config_validate) - cls._DEFAULT.load() - return cls._DEFAULT + if cached and cls._DEFAULT and not reload: + return cls._DEFAULT + + new_instance = cls(SPEC, upg, validator=cylc_config_validate) + new_instance.load() + + if cached or reload: + cls._DEFAULT = new_instance + + return new_instance def _load(self, fname: Union[Path, str], conf_type: str) -> None: if not os.access(fname, os.F_OK | os.R_OK): @@ -2008,7 +2013,7 @@ def _load(self, fname: Union[Path, str], conf_type: str) -> None: raise def load(self) -> None: - """Load or reload configuration from files.""" + """Load configuration from files.""" self.sparse.clear() self.dense.clear() LOG.debug("Loading site/user config files") diff --git a/tests/integration/test_config.py b/tests/integration/test_config.py index 6e7454f1293..231fb12f804 100644 --- a/tests/integration/test_config.py +++ b/tests/integration/test_config.py @@ -14,11 +14,14 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see . +import logging from pathlib import Path import sqlite3 from typing import Any import pytest +from cylc.flow.cfgspec.glbl_cfg import glbl_cfg +from cylc.flow.cfgspec.globalcfg import GlobalConfig from cylc.flow.exceptions import ( ServiceFileError, WorkflowConfigError, @@ -503,3 +506,90 @@ def test_special_task_non_word_names(flow: Fixture, validate: Fixture): }, }) validate(wid) + + +async def test_glbl_cfg(monkeypatch, tmp_path, caplog): + """Test accessing the global config via the glbl_cfg wrapper. + + Test the "cached" and "reload" kwargs to glbl_cfg. + + Also assert that accessing the global config during a reload operation does + not cause issues. See https://github.com/cylc/cylc-flow/issues/6244 + """ + # wipe any previously cached config + monkeypatch.setattr( + 'cylc.flow.cfgspec.globalcfg.GlobalConfig._DEFAULT', None + ) + # load the global config from the test tmp directory + monkeypatch.setenv('CYLC_CONF_PATH', str(tmp_path)) + + def write_global_config(cfg_str): + """Write the global.cylc file.""" + Path(tmp_path, 'global.cylc').write_text(cfg_str) + + def get_platforms(cfg_obj): + """Return the platforms defined in the provided config instance.""" + return {p for p in cfg_obj.get(['platforms']).keys()} + + def expect_platforms_during_reload(platforms): + """Test the platforms defined in glbl_cfg() during reload. + + Assert that the platforms defined in glbl_cfg() match the expected + value, whilst the global config is in the process of being reloaded. + + In other words, this tests that the cached instance is not changed + until after the reload has completed. + + See https://github.com/cylc/cylc-flow/issues/6244 + """ + caplog.set_level(logging.INFO) + + def _capture(fcn): + def _inner(*args, **kwargs): + cfg = glbl_cfg() + assert get_platforms(cfg) == platforms + logging.getLogger('test').info( + 'ran expect_platforms_during_reload test' + ) + return fcn(*args, **kwargs) + return _inner + + monkeypatch.setattr( + 'cylc.flow.cfgspec.globalcfg.GlobalConfig._load', + _capture(GlobalConfig._load) + ) + + # write a global config + write_global_config(''' + [platforms] + [[foo]] + ''') + + # test the platforms defined in it + assert get_platforms(glbl_cfg()) == {'localhost', 'foo'} + + # add a new platform the config + write_global_config(''' + [platforms] + [[foo]] + [[bar]] + ''') + + # the new platform should not appear (due to the cached instance) + assert get_platforms(glbl_cfg()) == {'localhost', 'foo'} + + # if we request an uncached instance, the new platform should appear + assert get_platforms(glbl_cfg(cached=False)) == {'localhost', 'foo', 'bar'} + + # however, this should not affect the cached instance + assert get_platforms(glbl_cfg()) == {'localhost', 'foo'} + + # * if we reload the cached instance, the new platform should appear + # * but during the reload itself, the old config should persist + # see https://github.com/cylc/cylc-flow/issues/6244 + expect_platforms_during_reload({'localhost', 'foo'}) + assert get_platforms(glbl_cfg(reload=True)) == {'localhost', 'foo', 'bar'} + assert 'ran expect_platforms_during_reload test' in caplog.messages + + # the cache should have been updated by the reload + assert get_platforms(glbl_cfg()) == {'localhost', 'foo', 'bar'} From 0e7c61fd38bff6d4fb328c6bf712d5859fa18123 Mon Sep 17 00:00:00 2001 From: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com> Date: Mon, 22 Jul 2024 12:33:02 +0100 Subject: [PATCH 2/2] Tidy --- changes.d/6249.fix.md | 2 +- tests/integration/test_config.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/changes.d/6249.fix.md b/changes.d/6249.fix.md index 5bcdbba75c9..a20b65b9008 100644 --- a/changes.d/6249.fix.md +++ b/changes.d/6249.fix.md @@ -1 +1 @@ -Fix a race condition between global config reload and debug logging that caused "platform not defined" errors when running workflows that contained a "rose-suite.conf" file in vebose or debug mode. +Fix a race condition between global config reload and debug logging that caused "platform not defined" errors when running workflows that contained a "rose-suite.conf" file in verbose or debug mode. diff --git a/tests/integration/test_config.py b/tests/integration/test_config.py index 231fb12f804..6a1099bd6eb 100644 --- a/tests/integration/test_config.py +++ b/tests/integration/test_config.py @@ -529,7 +529,7 @@ def write_global_config(cfg_str): def get_platforms(cfg_obj): """Return the platforms defined in the provided config instance.""" - return {p for p in cfg_obj.get(['platforms']).keys()} + return set(cfg_obj.get(['platforms']).keys()) def expect_platforms_during_reload(platforms): """Test the platforms defined in glbl_cfg() during reload.