Check symlink target. (#4890)

Check symlink target.

* abort `cylc install` and or remote-init if symlink dirs target already exists.

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.

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)

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)
 

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:

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 <http://www.gnu.org/licenses/>.
+# -----------------------------------------------------------------------------
+# 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

tests/functional/remote/basic/flow.cylc

@@ -1,5 +1,7 @@
 #!Jinja2
-
+[scheduler]
+    [[events]]
+        stall timeout = PT0S
 [scheduling]
     [[graph]]
         R1 = foo

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(

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')