Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix cylc clean remote re-invocation bug #5631

Merged
merged 3 commits into from
Jul 25, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changes.d/5631.fix.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix bug in remote clean for workflows that generated `flow.cylc` files at runtime.
4 changes: 2 additions & 2 deletions cylc/flow/clean.py
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,7 @@ def remote_clean(
rm_dirs: Optional[List[str]] = None,
timeout: str = '120'
) -> None:
"""Run subprocesses to clean workflows on remote install targets
"""Run subprocesses to clean a workflow on its remote install targets
(skip localhost), given a set of platform names to look up.

Args:
Expand Down Expand Up @@ -446,7 +446,7 @@ def _remote_clean_cmd(
f"Cleaning {id_} on install target: {platform['install target']} "
f"(using platform: {platform['name']})"
)
cmd = ['clean', '--local-only', id_]
cmd = ['clean', '--local-only', '--no-scan', id_]
if rm_dirs is not None:
for item in rm_dirs:
cmd.extend(['--rm', item])
Expand Down
7 changes: 2 additions & 5 deletions cylc/flow/command_polling.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,17 +28,14 @@ def add_to_cmd_options(cls, parser, d_interval=60, d_max_polls=10):
"""Add command line options for commands that can do polling"""
parser.add_option(
"--max-polls",
help="Maximum number of polls (default " + str(d_max_polls) + ").",
help=r"Maximum number of polls (default: %default).",
metavar="INT",
action="store",
dest="max_polls",
default=d_max_polls)
parser.add_option(
"--interval",
help=(
"Polling interval in seconds (default " + str(d_interval) +
")."
),
help=r"Polling interval in seconds (default: %default).",
metavar="SECS",
action="store",
dest="interval",
Expand Down
2 changes: 1 addition & 1 deletion cylc/flow/scheduler_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@
),
OptionSettings(
["--format"],
help="The format of the output: 'plain'=human readable, 'json",
help="The format of the output: 'plain'=human readable, 'json'",
choices=('plain', 'json'),
default="plain",
dest='format',
Expand Down
47 changes: 29 additions & 18 deletions cylc/flow/scripts/clean.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
"""

import asyncio
from optparse import SUPPRESS_HELP
import sys
from typing import TYPE_CHECKING, Iterable, List, Tuple

Expand Down Expand Up @@ -120,10 +121,17 @@ def get_option_parser():
parser.add_option(
'--timeout',
help=("The number of seconds to wait for cleaning to take place on "
"remote hosts before cancelling."),
r"remote hosts before cancelling. Default: %default."),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to document the defaults, I have no idea why optparse does not do this by "default".

We are already defining our own "help formatter", looks like we could override expand_default to add a "Default: %default" bit onto the end of every option where a default is set rather than having to add it to each option manually. (for future work).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you turned this into a new issue?

action='store', default='120', dest='remote_timeout'
)

parser.add_option(
'--no-scan',
help=SUPPRESS_HELP, action='store_true', dest='no_scan'
# Used on remote re-invocation - do not scan for workflows, just
# clean exactly what you were told to clean
)

return parser


Expand Down Expand Up @@ -173,28 +181,31 @@ async def scan(


async def run(*ids: str, opts: 'Values') -> None:
# parse ids from the CLI
workflows, multi_mode = await parse_ids_async(
*ids,
constraint='workflows',
match_workflows=True,
match_active=False,
infer_latest_runs=False, # don't infer latest runs like other cmds
)
if opts.no_scan:
workflows: Iterable[str] = ids
else:
# parse ids from the CLI
workflows, multi_mode = await parse_ids_async(
*ids,
constraint='workflows',
match_workflows=True,
match_active=False,
infer_latest_runs=False, # don't infer latest runs like other cmds
)

# expand partial workflow ids (including run names)
workflows, multi_mode = await scan(workflows, multi_mode)
# expand partial workflow ids (including run names)
workflows, multi_mode = await scan(workflows, multi_mode)

if not workflows:
LOG.warning(f"No workflows matching {', '.join(ids)}")
return
if not workflows:
LOG.warning(f"No workflows matching {', '.join(ids)}")
return

workflows.sort()
if multi_mode and not opts.skip_interactive:
prompt(workflows) # prompt for approval or exit
workflows.sort()
if multi_mode and not opts.skip_interactive:
prompt(workflows) # prompt for approval or exit

failed = {}
for workflow in sorted(workflows):
for workflow in workflows:
try:
init_clean(workflow, opts)
except Exception as exc:
Expand Down
4 changes: 2 additions & 2 deletions cylc/flow/scripts/cycle_point.py
Original file line number Diff line number Diff line change
Expand Up @@ -115,8 +115,8 @@ def get_option_parser() -> COP:

parser.add_option(
"--time-zone", metavar="TEMPLATE",
help="Control the formatting of the result's timezone e.g. "
"(Z, +13:00, -hh",
help="Control the formatting of the result's timezone (e.g. "
"Z, +13:00, -hh)",
action="store", default=None, dest="time_zone")

parser.add_option(
Expand Down
20 changes: 14 additions & 6 deletions cylc/flow/scripts/ext_trigger.py
Original file line number Diff line number Diff line change
Expand Up @@ -89,14 +89,22 @@ def get_option_parser() -> COP:
)

parser.add_option(
"--max-tries", help="Maximum number of send attempts "
"(default %s)." % MAX_N_TRIES, metavar="INT",
action="store", default=MAX_N_TRIES, dest="max_n_tries")
"--max-tries",
help=r"Maximum number of send attempts (default: %default).",
metavar="INT",
action="store",
default=MAX_N_TRIES,
dest="max_n_tries"
)

parser.add_option(
"--retry-interval", help="Delay in seconds before retrying "
"(default %s)." % RETRY_INTVL_SECS, metavar="SEC",
action="store", default=RETRY_INTVL_SECS, dest="retry_intvl_secs")
"--retry-interval",
help=r"Delay in seconds before retrying (default: %default).",
metavar="SEC",
action="store",
default=RETRY_INTVL_SECS,
dest="retry_intvl_secs"
)

return parser

Expand Down
2 changes: 1 addition & 1 deletion cylc/flow/scripts/scan.py
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ def get_option_parser() -> COP:
parser.add_option(
'--format', '-t',
help=(
'Output data and format (default "plain").'
r'Output data and format (default "%default").'
' ("name": list the workflow IDs only)'
' ("plain": name,host:port,PID on one line)'
' ("tree": name,host:port,PID in tree format)'
Expand Down
4 changes: 3 additions & 1 deletion tests/functional/cylc-clean/01-remote.t
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ init_workflow "${TEST_NAME_BASE}" << __FLOW__
[runtime]
[[root]]
platform = ${CYLC_TEST_PLATFORM}
script = touch flow.cylc # testing that remote clean does not scan for workflows
__FLOW__

FUNCTIONAL_DIR="${TEST_SOURCE_DIR_BASE%/*}"
Expand Down Expand Up @@ -94,6 +95,8 @@ ${TEST_DIR}/${SYM_NAME}/other/cylc-run/${CYLC_TEST_REG_BASE}
| \`-- cycle -> ${TEST_DIR}/${SYM_NAME}/cycle/cylc-run/${WORKFLOW_NAME}/share/cycle
\`-- work
\`-- 1
\`-- santa
\`-- flow.cylc
${TEST_DIR}/${SYM_NAME}/run/cylc-run/${CYLC_TEST_REG_BASE}
\`-- ${FUNCTIONAL_DIR}
\`-- cylc-clean
Expand Down Expand Up @@ -135,4 +138,3 @@ ${TEST_DIR}/${SYM_NAME}/cycle/cylc-run/${CYLC_TEST_REG_BASE}
__TREE__

purge
exit
6 changes: 4 additions & 2 deletions tests/unit/test_clean.py
Original file line number Diff line number Diff line change
Expand Up @@ -992,9 +992,11 @@ def test_remote_clean_cmd(
monkeymock('cylc.flow.clean.Popen')

cylc_clean._remote_clean_cmd(id_, platform, rm_dirs, timeout='dunno')
args, kwargs = mock_construct_ssh_cmd.call_args
args, _kwargs = mock_construct_ssh_cmd.call_args
constructed_cmd = args[0]
assert constructed_cmd == ['clean', '--local-only', id_, *expected_args]
assert constructed_cmd == [
'clean', '--local-only', '--no-scan', id_, *expected_args
]


def test_clean_top_level(tmp_run_dir: Callable):
Expand Down