-
Notifications
You must be signed in to change notification settings - Fork 93
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
cylc clean 2: remote clean #4017
Conversation
until all platforms for that install target have been exhausted
- Ensure DB is closed using a try...finally statement in a couple of places where it was lacking - Refactor checking of the DB compatibility slightly
Fixes a bug where if log and share symlink dirs were the same, but share/cycle was different, then share/cycle's target wouldn't get removed
@oliver-sanders |
Move initial checking of whether a workflow can be cleaned to its own function
If you need the diff --git a/dockerfiles/cylc-dev/Dockerfile b/dockerfiles/cylc-dev/Dockerfile
index 257dd9e70..dee4f4914 100644
--- a/dockerfiles/cylc-dev/Dockerfile
+++ b/dockerfiles/cylc-dev/Dockerfile
@@ -33,7 +33,7 @@ COPY "$CYLC_FLOW_DIR" "cylc"
RUN apt-get update && \
# build-deps: build-essential
# run deps: procps, rsync
- apt-get -qq -y install build-essential procps rsync && \
+ apt-get -qq -y install build-essential procps rsync tree && \
# install conda stuff
conda init bash && \
. ./usr/local/etc/profile.d/conda.sh && \ |
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add myself as a reviewer. GH broken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, no problems in testing.
The logging is a little odd, the "Removing directory" notices come through as warnings, looks like I get one per install-target but no information as to where it is coming from.
$ cylc clean generic
2021-01-06T15:46:21Z INFO - Cleaning on install target: _remote_background_indep_poll
(platform: _remote_background_indep_poll)
2021-01-06T15:46:21Z INFO - Cleaning on install target: _remote_background_indep_tcp
(platform: _remote_background_indep_tcp)
2021-01-06T15:46:21Z WARNING - 2021-01-06T15:46:21Z INFO - Removing directory:
/root/cylc-run/generic
2021-01-06T15:46:21Z WARNING - 2021-01-06T15:46:21Z INFO - Removing directory:
/root/cylc-run/generic
2021-01-06T15:46:21Z INFO - Removing directory: /Users/oliver/cylc-run/generic
We will probably want to clean multiple platforms in parallel for 8.0.0 so reporting remote logging is tricky, can just log in the event of error.
plus 1 or 2 other code review suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Read code - Nothing obvious stands out, minor comments.
- Read tests.
- Checked-out and poked at. Satisfied it works in the desired way with some of my test workflows.
Hmmm, what does the ret-code say? Would be good to include that in the logging (note we can use the debug level). |
|
If out and error are coming out funny might be worth checking the function that constructs the SSH command. You may need to tell it to capture/return stdout/err. |
Turns out
and all the logging will still get printed, or try
and none of the logging will get printed
from https://docs.python.org/3/howto/logging.html#advanced-logging-tutorial |
cylc-flow/cylc/flow/option_parsers.py Lines 253 to 271 in 500a7ba
|
That's correct, IMO. (Although I had kind of forgotten about it!) |
Increase default timeout to 2 mins
Ok - logging verbosity, remote timeout and concurrency of ssh commands should be addressed now |
Looking good. Should demote the missing database warning as a missing database is not an error (e.g. flow not run or remote flow installation) suggest:
|
@hjoliver I think this is good to go now if you want to take a poke. |
Updated the latest commit |
The tests have passed, however
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might have to increase the GH workflow timeout
Dammit, just when I though things were stable, will keep an eye out.
Looks good to me. Going to leave for @hjoliver to merge. Will need to rebase squash pre-merge.
How come? I tried to keep my commits logically grouped. I can try to squash them down more before merging if needed, but I think it would pay to keep some of the changes separate |
Tis how we work, most of the commits here document the process of how this branch came to be, this information doesn't really have any value going forward. We aren't going to go back and revert one commit and knowing that one feature was developed slightly earlier than another isn't useful info to anyone. The extra commits do, however, bloat the repo and make bisects longer. Some (I think predominantly agile) projects use "squash and merge" for all PRs to strip out unnecessary commits, makes sense as the most important information to preserve the is the link to the PR, the history and purpose of the branch should be documented there. We don't because our PRs often do multiple things (we aren't very agile). (btw we don't use commits as a measure of work or play number games) |
I don't know why that didn't occur to me before 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great 🎉
Please merge after tidying the commit history as suggested by @oliver-sanders (duh - he suggested squash and merge 🤦 )
These changes partially address #3887
Clean on remote hosts in addition to local filesystem. This is done by getting the platforms from the workflow database and then looking up the install targets for those platforms from
global.cylc
.Requirements check-list
CONTRIBUTING.md
and added my name as a Code Contributor.