Merge pull request #5506 from MetRonnie/eval-host

Fix `eval_host()` localhost bug & plug testing gap

CHANGES.md

@@ -10,7 +10,16 @@ creating a new release entry be sure to copy & paste the span tag with the
 updated. Only the first match gets replaced, so it's fine to leave the old
 ones in. -->
 -------------------------------------------------------------------------------
+## __cylc-8.1.4 (<span actions:bind='release-date'>Upcoming</span>)__
 
+### Fixes
+
+[#5506](https://github.com/cylc/cylc-flow/pull/5506) -
+Fix bug introduced in 8.1.3 where specifying a subshell command for
+`flow.cylc[runtime][<namespace>][remote]host` (e.g. `$(rose host-select)`)
+would always result in localhost.
+
+-------------------------------------------------------------------------------
 ## __cylc-8.1.3 (<span actions:bind='release-date'>Released 2023-04-27</span>)__
 
 ### Enhancements

cylc/flow/task_remote_mgr.py

@@ -179,7 +179,9 @@ def eval_host(self, host_str: str) -> Optional[str]:
         (e.g. localhost4.localdomain4).
         """
         host = self._subshell_eval(host_str, HOST_REC_COMMAND)
-        return host if is_remote_host(host) else 'localhost'
+        if host is not None and not is_remote_host(host):
+            return 'localhost'
+        return host
 
     def eval_platform(self, platform_str: str) -> Optional[str]:
         """Evaluate a platform from a possible subshell string.

tests/functional/job-submission/19-platform_select.t

@@ -17,13 +17,7 @@
 #-------------------------------------------------------------------------------
 # Test recovery of a failed host select command for a group of tasks.
 . "$(dirname "$0")/test_header"
-set_test_number 5
-
-create_test_global_config "
-[platforms]
-    [[test platform]]
-        hosts = localhost
-"
+set_test_number 6
 
 install_workflow "${TEST_NAME_BASE}"
 
@@ -34,20 +28,22 @@ run_ok "${TEST_NAME_BASE}-run" \
 logfile="${WORKFLOW_RUN_DIR}/log/scheduler/log"
 
 
-# Check that host = $(hostname) is correctly evaluated
+# Check that host = $(cmd) is correctly evaluated
+grep_ok \
+    "1/host_subshell.* evaluated as improbable host name$" \
+    "${logfile}"
 grep_ok \
-    "1/platform_subshell.*evaluated as improbable platform name" \
+    "1/localhost_subshell.* evaluated as localhost$" \
     "${logfile}"
 
-# Check that host = `hostname` is correctly evaluated
+# Check that host = `cmd` is correctly evaluated
 grep_ok \
-    "1/host_subshell_backticks.*\`hostname\` evaluated as localhost" \
+    "1/host_subshell_backticks.* evaluated as improbable host name$" \
     "${logfile}"
 
-# Check that platform = $(echo "improbable platform name") correctly evaluated
+# Check that platform = $(cmd) correctly evaluated
 grep_ok \
-    "1/platform_subshell.*evaluated as improbable platform name" \
+    "1/platform_subshell.* evaluated as improbable platform name$" \
     "${logfile}"
 
 purge
-exit

tests/functional/job-submission/19-platform_select/flow.cylc

@@ -13,11 +13,12 @@ purpose = """
 [scheduling]
     [[dependencies]]
         R1 = """
-            platform_subshell:submit-fail => fin
-            platform_no_subshell:submit-fail => fin
-            host_subshell
             host_no_subshell
-            host_subshell_backticks
+            localhost_subshell
+            platform_subshell:submit-fail => fin_platform
+            platform_no_subshell:submit-fail => fin_platform
+            host_subshell:submit-fail => fin_host
+            host_subshell_backticks:submit-fail => fin_host
         """
 
 [runtime]
@@ -36,11 +37,18 @@ purpose = """
 
     [[host_subshell]]
         [[[remote]]]
-            host = $(hostname)
+            host = $(echo "improbable host name")
 
     [[host_subshell_backticks]]
         [[[remote]]]
-            host = `hostname`
+            host = `echo "improbable host name"`
 
-    [[fin]]
-        script = cylc remove "${CYLC_SUITE_NAME}//1/platform_*"
+    [[localhost_subshell]]
+        [[[remote]]]
+            host = $(echo "localhost4.localdomain4")
+
+    [[fin_platform]]
+        script = cylc remove "${CYLC_WORKFLOW_ID}//1/platform_*"
+
+    [[fin_host]]
+        script = cylc remove "${CYLC_WORKFLOW_ID}//1/host_subshell*"

tests/unit/test_task_remote_mgr.py

@@ -20,11 +20,13 @@
 from typing import (Any, Optional)
 from unittest.mock import MagicMock, Mock
 
+from cylc.flow.exceptions import PlatformError
 from cylc.flow.network.client_factory import CommsMeth
 from cylc.flow.task_remote_mgr import (
     REMOTE_FILE_INSTALL_DONE, REMOTE_INIT_IN_PROGRESS, TaskRemoteMgr)
 from cylc.flow.workflow_files import WorkflowFiles, get_workflow_srv_dir
 
+
 Fixture = Any
 
 
@@ -236,3 +238,100 @@ def flatten_install_targets_map(itm):
                 unreachable in caplog.records[0].msg)
     else:
         assert not caplog.records
+
+
+shared_eval_params = [
+    pytest.param(
+        'localhost', {}, 'localhost', id="localhost"
+    ),
+    pytest.param(
+        '$(some-cmd)', {}, None, id="subshell_1st_eval"
+    ),
+    pytest.param(
+        '$(some-cmd)', {'some-cmd': None}, None,
+        id="subshell_awaiting_eval"
+    ),
+    pytest.param(
+        '$(some-cmd)', {'some-cmd': 'isaac clarke'}, 'isaac clarke',
+        id="subshell_finished_eval"
+    ),
+    pytest.param(
+        '$SOME_ENV', {}, 'nolan stross', id="env_var"
+    ),
+    pytest.param(
+        '$(env-cmd)', {'env-cmd': '$SOME_ENV'}, 'nolan stross',
+        id="subshell_env_var"
+    ),
+    pytest.param(
+        'titan_station', {}, 'titan_station', id="verbatim_name"
+    ),
+    pytest.param(
+        '', {}, 'localhost', id="empty"
+    ),
+]
+
+
+@pytest.fixture
+def task_remote_mgr_eval(monkeypatch: pytest.MonkeyPatch):
+    """Fixture providing a task remote manager for eval_platform() &
+    eval_host() tests."""
+    def _task_remote_mgr_eval(remote_cmd_map: dict) -> TaskRemoteMgr:
+        monkeypatch.setenv('SOME_ENV', 'nolan stross')
+        task_remote_mgr = TaskRemoteMgr(
+            workflow='usg_ishimura',
+            proc_pool=Mock(),
+            bad_hosts=[],
+            db_mgr=None
+        )
+        task_remote_mgr.remote_command_map = remote_cmd_map
+        return task_remote_mgr
+
+    return _task_remote_mgr_eval
+
+
+@pytest.mark.parametrize(
+    'eval_str, remote_cmd_map, expected',
+    [
+        *shared_eval_params,
+        pytest.param(
+            'localhost_tau_volantis', {}, 'localhost_tau_volantis',
+            id="name_begin_w_localhost"
+        )
+    ]
+)
+def test_eval_platform(
+    eval_str: str, remote_cmd_map: dict, expected: Optional[str],
+    task_remote_mgr_eval,
+):
+    task_remote_mgr: TaskRemoteMgr = task_remote_mgr_eval(remote_cmd_map)
+    assert task_remote_mgr.eval_platform(eval_str) == expected
+
+
+def test_eval_platform_bad(task_remote_mgr_eval):
+    exc_msg = "foreign contaminant detected"
+    task_remote_mgr: TaskRemoteMgr = task_remote_mgr_eval(
+        {'cmd': PlatformError(exc_msg, 'aegis_vii')}
+    )
+    with pytest.raises(PlatformError, match=exc_msg):
+        task_remote_mgr.eval_platform('$(cmd)')
+
+
+@pytest.mark.parametrize(
+    'eval_str, remote_cmd_map, expected',
+    [
+        *shared_eval_params,
+        pytest.param(
+            'localhost4.localdomain4', {}, 'localhost', id="localhost_variant"
+        ),
+        pytest.param(
+            '`other cmd`', {'other cmd': 'nicole brennan'}, 'nicole brennan',
+            id="backticks"
+        ),
+    ]
+)
+def test_eval_host(
+    eval_str: str, remote_cmd_map: dict, expected: Optional[str],
+    task_remote_mgr_eval,
+):
+    task_remote_mgr: TaskRemoteMgr = task_remote_mgr_eval(remote_cmd_map)
+    assert task_remote_mgr.eval_host(eval_str) == expected