Merge pull request #6253 from cylc/8.3.x-sync

🤖 Merge 8.3.x-sync into master

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 verbose or debug mode.

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)

cylc/flow/cfgspec/globalcfg.py

@@ -1980,24 +1980,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):
@@ -2010,7 +2015,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")

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 <http://www.gnu.org/licenses/>.
 
+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 set(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'}