From ca81d57e3061be189798877cea1bc4a97ae37dd8 Mon Sep 17 00:00:00 2001 From: Hilary James Oliver Date: Tue, 5 Jul 2022 03:38:17 +1200 Subject: [PATCH] Check symlink target. (#4890) Check symlink target. * abort `cylc install` and or remote-init if symlink dirs target already exists. --- CHANGES.md | 13 +++-- cylc/flow/pathutil.py | 28 ++++++---- cylc/flow/task_remote_cmd.py | 4 +- cylc/flow/workflow_files.py | 8 +-- .../remote/08-symlink-dir-target-exist.t | 54 +++++++++++++++++++ tests/functional/remote/basic/flow.cylc | 4 +- tests/unit/test_pathutil.py | 24 +++++---- tests/unit/test_workflow_files.py | 37 +++++++++++++ 8 files changed, 140 insertions(+), 32 deletions(-) create mode 100644 tests/functional/remote/08-symlink-dir-target-exist.t diff --git a/CHANGES.md b/CHANGES.md index 2e75db72a6e..da39722d144 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -49,11 +49,11 @@ Jinja2 used by Cylc from 2.11 to 3.0. [#4896](https://github.com/cylc/cylc-flow/pull/4896) - Allow the setting of default job runner directives for platforms. +### Fixes + [#4887](https://github.com/cylc/cylc-flow/pull/4887) - Disallow relative paths in `global.cylc[install]source dirs`. -### Fixes - [#4936](https://github.com/cylc/cylc-flow/pull/4936) - Fix incorrect error messages when workflow CLI commands fail. @@ -69,14 +69,17 @@ formatting problem presenting in the UI mutation flow argument info. [#4891](https://github.com/cylc/cylc-flow/pull/4891) - Fix bug that could cause past jobs to be omitted in the UI. -[#4860](https://github.com/cylc/cylc-flow/pull/4860) - Workflow config parsing -will fail if +[#4860](https://github.com/cylc/cylc-flow/pull/4860) - Workflow validation +now fails if [owner setting](https://cylc.github.io/cylc-doc/latest/html/reference/config/workflow.html#flow.cylc[runtime][%3Cnamespace%3E][remote]owner) -owner setting is used, as that setting no longer has any effect. +is used, as that setting no longer has any effect. [#4889](https://github.com/cylc/cylc-flow/pull/4889) - `cylc clean`: don't prompt if no matching workflows. +[#4890](https://github.com/cylc/cylc-flow/pull/4890) - `cylc install`: don't +overwrite symlink dir targets if they were not cleaned properly before. + [#4881](https://github.com/cylc/cylc-flow/pull/4881) - Fix bug where commands targeting a specific cycle point would not work if using an abbreviated cycle point format. diff --git a/cylc/flow/pathutil.py b/cylc/flow/pathutil.py index fc748f9efdf..5b7fc37f18c 100644 --- a/cylc/flow/pathutil.py +++ b/cylc/flow/pathutil.py @@ -158,7 +158,7 @@ def make_localhost_symlinks( Returns: Dictionary of symlinks with sources as keys and - destinations as values: ``{source: destination}`` + destinations as values: ``{target: symlink}`` """ symlinks_created = {} @@ -177,9 +177,9 @@ def make_localhost_symlinks( if '$' in target: raise WorkflowFilesError( f'Unable to create symlink to {target}.' - f' \'{value}\' contains an invalid environment variable.' + f" '{value}' contains an invalid environment variable." ' Please check configuration.') - symlink_success = make_symlink(symlink_path, target) + symlink_success = make_symlink_dir(symlink_path, target) # Symlink info returned for logging purposes. Symlinks should be # created before logs as the log dir may be a symlink. if symlink_success: @@ -191,10 +191,11 @@ def get_dirs_to_symlink( install_target: str, workflow_name: str, symlink_conf: Optional[Dict[str, Dict[str, Any]]] = None -) -> Dict[str, Any]: +) -> Dict[str, str]: """Returns dictionary of directories to symlink. Note the paths should remain unexpanded, to be expanded on the remote. + Args: install_target: Symlinks to be created on this install target flow_name: full name of the run, e.g. myflow/run1 @@ -205,7 +206,7 @@ def get_dirs_to_symlink( Returns: dirs_to_symlink: [directory: symlink_path] """ - dirs_to_symlink: Dict[str, Any] = {} + dirs_to_symlink: Dict[str, str] = {} if symlink_conf is None: symlink_conf = glbl_cfg().get(['install', 'symlink dirs']) if install_target not in symlink_conf.keys(): @@ -223,12 +224,14 @@ def get_dirs_to_symlink( return dirs_to_symlink -def make_symlink(path: Union[Path, str], target: Union[Path, str]) -> bool: +def make_symlink_dir(path: Union[Path, str], target: Union[Path, str]) -> bool: """Makes symlinks for directories. Args: path: Absolute path of the desired symlink. target: Absolute path of the symlink's target directory. + + Returns True if symlink created, or False if skipped. """ path = Path(path) target = Path(target) @@ -251,7 +254,14 @@ def make_symlink(path: Union[Path, str], target: Union[Path, str]) -> bool: except OSError: raise WorkflowFilesError( f"Error when symlinking. Failed to unlink bad symlink {path}.") - target.mkdir(parents=True, exist_ok=True) + try: + target.mkdir(parents=True, exist_ok=False) + except FileExistsError: + raise WorkflowFilesError( + f"Symlink dir target already exists: ({path} ->) {target}\n" + "Tip: in future, use 'cylc clean' instead of manually deleting " + "workflow run dirs." + ) # This is needed in case share and share/cycle have the same symlink dir: if path.exists(): @@ -280,10 +290,10 @@ def remove_dir_and_target(path: Union[Path, str]) -> None: if os.path.exists(path): target = os.path.realpath(path) LOG.info( - f'Removing symlink target directory: ({path} ->) {target}' + "Removing symlink and its target directory: " + f"{path} -> {target}" ) rmtree(target, onerror=handle_rmtree_err) - LOG.info(f'Removing symlink: {path}') else: LOG.info(f'Removing broken symlink: {path}') os.remove(path) diff --git a/cylc/flow/task_remote_cmd.py b/cylc/flow/task_remote_cmd.py index 00c7060a230..aa55ab2a972 100644 --- a/cylc/flow/task_remote_cmd.py +++ b/cylc/flow/task_remote_cmd.py @@ -28,7 +28,7 @@ KeyType, WorkflowFiles ) -from cylc.flow.pathutil import make_symlink +from cylc.flow.pathutil import make_symlink_dir from cylc.flow.resources import get_resources from cylc.flow.task_remote_mgr import ( REMOTE_INIT_DONE, @@ -105,7 +105,7 @@ def remote_init(install_target: str, rund: str, *dirs_to_symlink: str) -> None: print(f'Error occurred when symlinking.' f' {target} contains an invalid environment variable.') return - make_symlink(path, target) + make_symlink_dir(path, target) srvd = os.path.join(rund, WorkflowFiles.Service.DIRNAME) os.makedirs(srvd, exist_ok=True) diff --git a/cylc/flow/workflow_files.py b/cylc/flow/workflow_files.py index 329f0090a68..789f76d2bd3 100644 --- a/cylc/flow/workflow_files.py +++ b/cylc/flow/workflow_files.py @@ -691,8 +691,8 @@ def register( symlinks_created = make_localhost_symlinks( get_workflow_run_dir(workflow_name), workflow_name) if symlinks_created: - for src, dst in symlinks_created.items(): - LOG.info(f"Symlink created from {src} to {dst}") + for target, symlink in symlinks_created.items(): + LOG.info(f"Symlink created: {symlink} -> {target}") # Create service dir if necessary. srv_d = get_workflow_srv_dir(workflow_name) os.makedirs(srv_d, exist_ok=True) @@ -1596,8 +1596,8 @@ def install_workflow( rundir, named_run, symlink_conf=cli_symlink_dirs) install_log = _get_logger(rundir, 'cylc-install') if symlinks_created: - for src, dst in symlinks_created.items(): - install_log.info(f"Symlink created from {src} to {dst}") + for target, symlink in symlinks_created.items(): + install_log.info(f"Symlink created: {symlink} -> {target}") try: rundir.mkdir(exist_ok=True, parents=True) except FileExistsError: diff --git a/tests/functional/remote/08-symlink-dir-target-exist.t b/tests/functional/remote/08-symlink-dir-target-exist.t new file mode 100644 index 00000000000..1a7f52fb693 --- /dev/null +++ b/tests/functional/remote/08-symlink-dir-target-exist.t @@ -0,0 +1,54 @@ +#!/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 . +# ----------------------------------------------------------------------------- +# Test remote init fails if symlink dir target already exists + +export REQUIRE_PLATFORM='loc:remote fs:indep comms:tcp' +. "$(dirname "$0")/test_header" +set_test_number 5 + +SSH_CMD="$(cylc config -d -i "[platforms][${CYLC_TEST_PLATFORM}]ssh command")" + +create_test_global_config "" " +[install] + [[symlink dirs]] + [[[${CYLC_TEST_INSTALL_TARGET}]]] + run = \$TMPDIR/\$USER/sym-run +" +install_workflow "${TEST_NAME_BASE}" basic + +run_ok "${TEST_NAME_BASE}-val" cylc validate "$WORKFLOW_NAME" + +# Run once to setup symlink dirs on remote install target +workflow_run_ok "${TEST_NAME_BASE}-run" cylc play --no-detach "$WORKFLOW_NAME" + +# Remove remote run dir symlink (but not its target) +$SSH_CMD "$CYLC_TEST_HOST" "rm -rf ~/cylc-run/${WORKFLOW_NAME}" + +# New run should abort +delete_db +TEST_NAME="${TEST_NAME_BASE}-run-again" +workflow_run_fail "$TEST_NAME" cylc play --no-detach "$WORKFLOW_NAME" + +grep_ok "ERROR - platform: .* initialisation did not complete" "${TEST_NAME}.stderr" +grep_ok "WorkflowFilesError: Symlink dir target already exists" "${TEST_NAME}.stderr" + +# Clean up remote symlink dir target +# shellcheck disable=SC2016 +$SSH_CMD "$CYLC_TEST_HOST" 'rm -rf "${TMPDIR}/${USER}/sym-run"' + +purge diff --git a/tests/functional/remote/basic/flow.cylc b/tests/functional/remote/basic/flow.cylc index 434da23f196..dc38fc5292c 100644 --- a/tests/functional/remote/basic/flow.cylc +++ b/tests/functional/remote/basic/flow.cylc @@ -1,5 +1,7 @@ #!Jinja2 - +[scheduler] + [[events]] + stall timeout = PT0S [scheduling] [[graph]] R1 = foo diff --git a/tests/unit/test_pathutil.py b/tests/unit/test_pathutil.py index 8a3d00e138f..2909ff916e5 100644 --- a/tests/unit/test_pathutil.py +++ b/tests/unit/test_pathutil.py @@ -293,7 +293,7 @@ def test_make_localhost_symlinks_calls_make_symlink_for_each_key_value_dir( mocked_get_workflow_run_dir.return_value = "rund" for v in ('DOH', 'DEE'): monkeypatch.setenv(v, 'expanded') - mocked_make_symlink = monkeymock('cylc.flow.pathutil.make_symlink') + mocked_make_symlink = monkeymock('cylc.flow.pathutil.make_symlink_dir') make_localhost_symlinks('rund', 'workflow') mocked_make_symlink.assert_has_calls([ @@ -303,24 +303,26 @@ def test_make_localhost_symlinks_calls_make_symlink_for_each_key_value_dir( ]) -@patch('os.path.expandvars') @patch('cylc.flow.pathutil.get_workflow_run_dir') -@patch('cylc.flow.pathutil.make_symlink') +@patch('cylc.flow.pathutil.make_symlink_dir') @patch('cylc.flow.pathutil.get_dirs_to_symlink') def test_incorrect_environment_variables_raise_error( - mocked_dirs_to_symlink, - mocked_make_symlink, - mocked_get_workflow_run_dir, mocked_expandvars): + mocked_dirs_to_symlink, + mocked_make_symlink, + mocked_get_workflow_run_dir, + monkeypatch: pytest.MonkeyPatch +): + monkeypatch.delenv('doh', raising=False) mocked_dirs_to_symlink.return_value = { 'run': '$doh/cylc-run/test_workflow'} mocked_get_workflow_run_dir.return_value = "rund" - mocked_expandvars.return_value = "$doh" - with pytest.raises(WorkflowFilesError, match=r"Unable to create symlink" - r" to \$doh. '\$doh/cylc-run/test_workflow' contains an" - " invalid environment variable. Please check " - "configuration."): + with pytest.raises(WorkflowFilesError) as excinfo: make_localhost_symlinks('rund', 'test_workflow') + assert ( + "'$doh/cylc-run/test_workflow' contains an invalid " + "environment variable" + ) in str(excinfo.value) @pytest.mark.parametrize( diff --git a/tests/unit/test_workflow_files.py b/tests/unit/test_workflow_files.py index 741781e8fcf..726eb40fc3c 100644 --- a/tests/unit/test_workflow_files.py +++ b/tests/unit/test_workflow_files.py @@ -1876,6 +1876,43 @@ def test_install_workflow__next_to_flow_file( install_workflow(src_dir, 'faden') +def test_install_workflow__symlink_target_exists( + tmp_path: Path, + tmp_src_dir: Callable, + tmp_run_dir: Callable, + mock_glbl_cfg: Callable, +): + """Test that you can't install workflow when run dir symlink dir target + already exists.""" + reg = 'smeagol' + src_dir: Path = tmp_src_dir(reg) + tmp_run_dir() + sym_run = tmp_path / 'sym-run' + sym_log = tmp_path / 'sym-log' + mock_glbl_cfg( + 'cylc.flow.pathutil.glbl_cfg', + f''' + [install] + [[symlink dirs]] + [[[localhost]]] + run = {sym_run} + log = {sym_log} + ''' + ) + msg = "Symlink dir target already exists: .*{}" + # Test: + (sym_run / 'cylc-run' / reg / 'run1').mkdir(parents=True) + with pytest.raises(WorkflowFilesError, match=msg.format(sym_run)): + install_workflow(src_dir) + + shutil.rmtree(sym_run) + ( + sym_log / 'cylc-run' / reg / 'run1' / WorkflowFiles.LOG_DIR + ).mkdir(parents=True) + with pytest.raises(WorkflowFilesError, match=msg.format(sym_log)): + install_workflow(src_dir) + + def test_validate_source_dir(tmp_run_dir: Callable, tmp_src_dir: Callable): cylc_run_dir: Path = tmp_run_dir() src_dir: Path = tmp_src_dir('ludlow')