From ff0dbb8e6f2380884ff9081fe74fe5c41a7ad247 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Thu, 2 Feb 2023 10:50:13 +0000 Subject: [PATCH 1/9] Avoid running host check on platform names - this doesn't make any sense. --- CHANGES.md | 6 +++ cylc/flow/task_job_mgr.py | 4 +- .../10-do-not-host-check-platforms.t | 52 +++++++++++++++++++ 3 files changed, 61 insertions(+), 1 deletion(-) create mode 100755 tests/functional/platforms/10-do-not-host-check-platforms.t diff --git a/CHANGES.md b/CHANGES.md index d381efb7d79..03150dba968 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -38,6 +38,12 @@ Rose options (`-O`, `-S` & `-D`) with `cylc view`. [#5363](https://github.com/cylc/cylc-flow/pull/5363) Improvements and bugfixes for `cylc lint`. +------------------------------------------------------------------------------- +## __cylc-8.1.2 (Coming Soon)__ + +[#5343](https://github.com/cylc/cylc-flow/pull/5343) - Fix a bug causing +platform names to be checked as if they were hosts. + ------------------------------------------------------------------------------- ## __cylc-8.1.1 (Released 2023-01-31)__ diff --git a/cylc/flow/task_job_mgr.py b/cylc/flow/task_job_mgr.py index da995bac846..1406b496f11 100644 --- a/cylc/flow/task_job_mgr.py +++ b/cylc/flow/task_job_mgr.py @@ -1125,7 +1125,9 @@ def _prep_submit_task_job( ) else: platform_name = self.task_remote_mgr.subshell_eval( - rtconfig['platform'], PLATFORM_REC_COMMAND + rtconfig['platform'], + PLATFORM_REC_COMMAND, + host_check=False ) except PlatformError as exc: itask.waiting_on_job_prep = False diff --git a/tests/functional/platforms/10-do-not-host-check-platforms.t b/tests/functional/platforms/10-do-not-host-check-platforms.t new file mode 100755 index 00000000000..7ba5dc0218f --- /dev/null +++ b/tests/functional/platforms/10-do-not-host-check-platforms.t @@ -0,0 +1,52 @@ +#!/usr/bin/env bash +# THIS FILE IS PART OF THE CYLC WORKFLOW ENGINE. +# Copyright (C) NIWA & British Crown (Met Office) & Contributors. +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . +#------------------------------------------------------------------------------- +# Check that we don't check whether the platform name is a valid host. +. "$(dirname "$0")/test_header" + +set_test_number 2 + +# shellcheck disable=SC2016 +create_test_global_config '' ' +[platforms] + [[localhost_spice]] + hosts = unreachable +' + +make_rnd_workflow + +cat > "${RND_WORKFLOW_SOURCE}/flow.cylc" <<__HEREDOC__ +[scheduler] + [[events]] + stall timeout = PT0S +[scheduling] + [[graph]] + R1 = foo +[runtime] + [[foo]] + platform = localhost_spice +__HEREDOC__ + +ERR_STR='Unable to find valid host for localhost_spice' + +TEST_NAME="${TEST_NAME_BASE}-vip-workflow" +run_fail "${TEST_NAME}" cylc vip "${RND_WORKFLOW_SOURCE}" --no-detach +grep_ok "${ERR_STR}" \ + "${TEST_NAME}.stderr" -F + +purge_rnd_workflow +exit From eaa11604ba6a1564db13420d200f83da8590957e Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Fri, 17 Feb 2023 15:01:42 +0000 Subject: [PATCH 2/9] clarification of nomenclature --- cylc/flow/task_remote_mgr.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/cylc/flow/task_remote_mgr.py b/cylc/flow/task_remote_mgr.py index 59b6e04cf42..7235bf73679 100644 --- a/cylc/flow/task_remote_mgr.py +++ b/cylc/flow/task_remote_mgr.py @@ -149,7 +149,7 @@ def subshell_eval(self, command, command_pattern, host_check=True): elif value is None: return # command not yet ready else: - command = value # command succeeded + platform_or_host_str = value # command succeeded else: # Command not launched (or already reset) self.proc_pool.put_command( @@ -164,16 +164,17 @@ def subshell_eval(self, command, command_pattern, host_check=True): return self.remote_command_map[cmd_str] # Environment variable substitution - command = os.path.expandvars(command) + platform_or_host_str = os.path.expandvars(platform_or_host_str) + # Remote? # TODO - Remove at Cylc 8.x as this only makes sense with host logic if host_check is True: - if is_remote_host(command): - return command + if is_remote_host(platform_or_host_str): + return platform_or_host_str else: return 'localhost' else: - return command + return platform_or_host_str def subshell_eval_reset(self): """Reset remote eval subshell results. From 6a211574f5ebc8fefd7048ec87a4123463541578 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Fri, 17 Feb 2023 15:45:36 +0000 Subject: [PATCH 3/9] undo mistake --- cylc/flow/task_remote_mgr.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/cylc/flow/task_remote_mgr.py b/cylc/flow/task_remote_mgr.py index 7235bf73679..59b6e04cf42 100644 --- a/cylc/flow/task_remote_mgr.py +++ b/cylc/flow/task_remote_mgr.py @@ -149,7 +149,7 @@ def subshell_eval(self, command, command_pattern, host_check=True): elif value is None: return # command not yet ready else: - platform_or_host_str = value # command succeeded + command = value # command succeeded else: # Command not launched (or already reset) self.proc_pool.put_command( @@ -164,17 +164,16 @@ def subshell_eval(self, command, command_pattern, host_check=True): return self.remote_command_map[cmd_str] # Environment variable substitution - platform_or_host_str = os.path.expandvars(platform_or_host_str) - + command = os.path.expandvars(command) # Remote? # TODO - Remove at Cylc 8.x as this only makes sense with host logic if host_check is True: - if is_remote_host(platform_or_host_str): - return platform_or_host_str + if is_remote_host(command): + return command else: return 'localhost' else: - return platform_or_host_str + return command def subshell_eval_reset(self): """Reset remote eval subshell results. From 83484ef1cd8e4163ba184ab9b595bb3c37ec6b74 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Mon, 13 Mar 2023 10:42:09 +0000 Subject: [PATCH 4/9] update comment on localhost check and add test for case localhost4.localhost42 --- cylc/flow/hostuserutil.py | 2 +- tests/unit/test_hostuserutil.py | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/cylc/flow/hostuserutil.py b/cylc/flow/hostuserutil.py index 782a193746a..36154965246 100644 --- a/cylc/flow/hostuserutil.py +++ b/cylc/flow/hostuserutil.py @@ -198,7 +198,7 @@ def is_remote_host(self, name): """ if name not in self._remote_hosts: if not name or name.split(".")[0].startswith("localhost"): - # e.g. localhost.localdomain + # e.g. localhost42.localdomain42 self._remote_hosts[name] = False else: try: diff --git a/tests/unit/test_hostuserutil.py b/tests/unit/test_hostuserutil.py index f9d9d745262..aa21632530a 100644 --- a/tests/unit/test_hostuserutil.py +++ b/tests/unit/test_hostuserutil.py @@ -35,10 +35,11 @@ def test_is_remote_user_on_current_user(): assert not is_remote_user(os.getenv('USER')) -def test_is_remote_host_on_localhost(): +def test_is_remote_host_on_localhost(monkeypatch): """is_remote_host with localhost.""" assert not is_remote_host(None) assert not is_remote_host('localhost') + assert not is_remote_host('localhost4.localhost42') assert not is_remote_host(os.getenv('HOSTNAME')) assert not is_remote_host(get_host()) From 5cd1a63b78597658f6a54eb724c1e9e545eecae2 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Mon, 20 Mar 2023 14:55:54 +0000 Subject: [PATCH 5/9] small changlog error fix --- CHANGES.md | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 03150dba968..cf193545197 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -24,6 +24,9 @@ Fixes `cylc set-verbosity`. absence of `job name length maximum` in PBS platform settings would cause Cylc to crash when preparing the job script. +[#5343](https://github.com/cylc/cylc-flow/pull/5343) - Fix a bug causing +platform names to be checked as if they were hosts. + ------------------------------------------------------------------------------- ## __cylc-8.1.2 (Released 2023-02-20)__ @@ -38,12 +41,6 @@ Rose options (`-O`, `-S` & `-D`) with `cylc view`. [#5363](https://github.com/cylc/cylc-flow/pull/5363) Improvements and bugfixes for `cylc lint`. -------------------------------------------------------------------------------- -## __cylc-8.1.2 (Coming Soon)__ - -[#5343](https://github.com/cylc/cylc-flow/pull/5343) - Fix a bug causing -platform names to be checked as if they were hosts. - ------------------------------------------------------------------------------- ## __cylc-8.1.1 (Released 2023-01-31)__ From ccdd6cf47241339121243e1c148a5ee3dfe38f07 Mon Sep 17 00:00:00 2001 From: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com> Date: Thu, 13 Apr 2023 09:25:47 +0100 Subject: [PATCH 6/9] Improve readability of host/platform eval code (#53) * WIP improve readability of host/platform eval code * Address review --- cylc/flow/hostuserutil.py | 4 +- cylc/flow/task_job_mgr.py | 12 ++--- cylc/flow/task_remote_mgr.py | 92 +++++++++++++++++++++--------------- 3 files changed, 59 insertions(+), 49 deletions(-) diff --git a/cylc/flow/hostuserutil.py b/cylc/flow/hostuserutil.py index 36154965246..34e033b2d7b 100644 --- a/cylc/flow/hostuserutil.py +++ b/cylc/flow/hostuserutil.py @@ -197,8 +197,8 @@ def is_remote_host(self, name): """ if name not in self._remote_hosts: - if not name or name.split(".")[0].startswith("localhost"): - # e.g. localhost42.localdomain42 + if not name or name.startswith("localhost"): + # e.g. localhost4.localdomain4 self._remote_hosts[name] = False else: try: diff --git a/cylc/flow/task_job_mgr.py b/cylc/flow/task_job_mgr.py index 8e440340dc9..a5e63f6bdfc 100644 --- a/cylc/flow/task_job_mgr.py +++ b/cylc/flow/task_job_mgr.py @@ -58,8 +58,6 @@ ) from cylc.flow.pathutil import get_remote_workflow_run_job_dir from cylc.flow.platforms import ( - HOST_REC_COMMAND, - PLATFORM_REC_COMMAND, get_host_from_platform, get_install_target_from_platform, get_localhost_install_target, @@ -1120,14 +1118,12 @@ def _prep_submit_task_job( host_n, platform_name = None, None try: if rtconfig['remote']['host'] is not None: - host_n = self.task_remote_mgr.subshell_eval( - rtconfig['remote']['host'], HOST_REC_COMMAND + host_n = self.task_remote_mgr.eval_host( + rtconfig['remote']['host'] ) else: - platform_name = self.task_remote_mgr.subshell_eval( - rtconfig['platform'], - PLATFORM_REC_COMMAND, - host_check=False + platform_name = self.task_remote_mgr.eval_platform( + rtconfig['platform'] ) except PlatformError as exc: itask.waiting_on_job_prep = False diff --git a/cylc/flow/task_remote_mgr.py b/cylc/flow/task_remote_mgr.py index 59b6e04cf42..c38c2ce1e4c 100644 --- a/cylc/flow/task_remote_mgr.py +++ b/cylc/flow/task_remote_mgr.py @@ -32,12 +32,14 @@ from subprocess import Popen, PIPE, DEVNULL import tarfile from time import sleep, time -from typing import Any, Deque, Dict, TYPE_CHECKING, List, NamedTuple, Tuple +from typing import ( + Any, Deque, Dict, TYPE_CHECKING, List, + NamedTuple, Optional, Tuple +) from cylc.flow import LOG from cylc.flow.exceptions import PlatformError import cylc.flow.flags -from cylc.flow.hostuserutil import is_remote_host from cylc.flow.network.client_factory import CommsMeth from cylc.flow.pathutil import ( get_dirs_to_symlink, @@ -46,6 +48,8 @@ get_workflow_run_dir, ) from cylc.flow.platforms import ( + HOST_REC_COMMAND, + PLATFORM_REC_COMMAND, NoHostsError, PlatformLookupError, get_host_from_platform, @@ -67,6 +71,7 @@ ) from cylc.flow.loggingutil import get_next_log_number, get_sorted_logs_by_time +from cylc.flow.hostuserutil import is_remote_host if TYPE_CHECKING: from zmq.auth.thread import ThreadAuthenticator @@ -106,39 +111,31 @@ def __init__(self, workflow, proc_pool, bad_hosts): self.is_reload = False self.is_restart = False - def subshell_eval(self, command, command_pattern, host_check=True): - """Evaluate a task platform from a subshell string. - - At Cylc 7, from a host string. + def _subshell_eval( + self, eval_str: str, command_pattern: re.Pattern + ) -> Optional[str]: + """Evaluate a platform or host from a possible subshell string. Arguments: - command (str): - An explicit host name, a command in back-tick or $(command) - format, or an environment variable holding a hostname. - command_pattern (re.Pattern): + eval_str: + An explicit host/platform name, a command, or an environment + variable holding a host/patform name. + command_pattern: A compiled regex pattern designed to match subshell strings. - host_check (bool): - A flag to enable remote testing. If True, and if the command - is running locally, then it will return 'localhost'. - Return (str): + Return: - None if evaluation of command is still taking place. - - If command is not defined or the evaluated name is equivalent - to 'localhost', _and_ host_check is set to True then - 'localhost' - - Otherwise, return the evaluated host name on success. + - 'localhost' if string is empty/not defined. + - Otherwise, return the evaluated host/platform name on success. Raise PlatformError on error. """ - # BACK COMPAT: references to "host" - # remove at: - # Cylc8.x - if not command: + if not eval_str: return 'localhost' # Host selection command: $(command) or `command` - match = command_pattern.match(command) + match = command_pattern.match(eval_str) if match: cmd_str = match.groups()[1] if cmd_str in self.remote_command_map: @@ -146,34 +143,51 @@ def subshell_eval(self, command, command_pattern, host_check=True): value = self.remote_command_map[cmd_str] if isinstance(value, PlatformError): raise value # command failed - elif value is None: - return # command not yet ready - else: - command = value # command succeeded + if value is None: + return None # command not yet ready + eval_str = value # command succeeded else: # Command not launched (or already reset) self.proc_pool.put_command( SubProcContext( 'remote-host-select', ['bash', '-c', cmd_str], - env=dict(os.environ)), + env=dict(os.environ) + ), callback=self._subshell_eval_callback, callback_args=[cmd_str] ) self.remote_command_map[cmd_str] = None - return self.remote_command_map[cmd_str] + return None # Environment variable substitution - command = os.path.expandvars(command) - # Remote? - # TODO - Remove at Cylc 8.x as this only makes sense with host logic - if host_check is True: - if is_remote_host(command): - return command - else: - return 'localhost' - else: - return command + return os.path.expandvars(eval_str) + + # BACK COMPAT: references to "host" + # remove at: + # Cylc8.x + def eval_host(self, host_str: str) -> Optional[str]: + """Evaluate a host from a possible subshell string. + + Args: + host_str: An explicit host name, a command in back-tick or + $(command) format, or an environment variable holding + a hostname. + + Returns 'localhost' if evaluated name is equivalent + (e.g. localhost4.localdomain4). + """ + host = self._subshell_eval(host_str, HOST_REC_COMMAND) + return host if is_remote_host(host) else 'localhost' + + def eval_platform(self, platform_str: str) -> Optional[str]: + """Evaluate a platform from a possible subshell string. + + Args: + platform_str: An explicit platform name, a command in $(command) + format, or an environment variable holding a platform name. + """ + return self._subshell_eval(platform_str, PLATFORM_REC_COMMAND) def subshell_eval_reset(self): """Reset remote eval subshell results. From 52438846f0bb2cdb562ae9f563937527744338a2 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Thu, 13 Apr 2023 20:49:17 +0100 Subject: [PATCH 7/9] Update tests/functional/platforms/10-do-not-host-check-platforms.t Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com> --- tests/functional/platforms/10-do-not-host-check-platforms.t | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/functional/platforms/10-do-not-host-check-platforms.t b/tests/functional/platforms/10-do-not-host-check-platforms.t index 7ba5dc0218f..c52ef7e8383 100755 --- a/tests/functional/platforms/10-do-not-host-check-platforms.t +++ b/tests/functional/platforms/10-do-not-host-check-platforms.t @@ -15,7 +15,9 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see . #------------------------------------------------------------------------------- -# Check that we don't check whether the platform name is a valid host. +# Check that platform names are not treated as host names. E.g. a platform +# name starting with "localhost" should not be treated as localhost. +# https://github.com/cylc/cylc-flow/issues/5342 . "$(dirname "$0")/test_header" set_test_number 2 From 3aa4860d977c38ca7ea2420aa0aa316dc6fe73ce Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Fri, 14 Apr 2023 11:09:06 +0100 Subject: [PATCH 8/9] upgrade cylc message internal help with details of severity levels --- cylc/flow/scripts/message.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/cylc/flow/scripts/message.py b/cylc/flow/scripts/message.py index 9e9e1f258ef..c10a5627ffa 100755 --- a/cylc/flow/scripts/message.py +++ b/cylc/flow/scripts/message.py @@ -59,6 +59,20 @@ The default message severity is INFO. The --severity=SEVERITY option can be used to set the default severity level for all unprefixed messages. +Increased severity will make messages more visible in workflow logs, using +colour and format changes. DEBUG messages will not be shown in logs by default. + +The severity levels are those of the Python Logging Library +https://docs.python.org/3/library/logging.html#logging-levels: + +- CRITICAL +- ERROR +- WARNING +- INFO +- DEBUG +- NOTSET + + Note: To abort a job script with a custom error message, use cylc__job_abort: cylc__job_abort 'message...' From 574e43c5d976bcf734ef8dc88b830e8a2663faa6 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Fri, 14 Apr 2023 16:49:12 +0100 Subject: [PATCH 9/9] Update cylc/flow/scripts/message.py Co-authored-by: Oliver Sanders --- cylc/flow/scripts/message.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/cylc/flow/scripts/message.py b/cylc/flow/scripts/message.py index c10a5627ffa..b2fb01f431c 100755 --- a/cylc/flow/scripts/message.py +++ b/cylc/flow/scripts/message.py @@ -70,8 +70,6 @@ - WARNING - INFO - DEBUG -- NOTSET - Note: To abort a job script with a custom error message, use cylc__job_abort: