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

Cat log remote tidy fix. #2674

Merged
merged 5 commits into from
May 22, 2018
Merged

Conversation

hjoliver
Copy link
Member

@hjoliver hjoliver commented May 19, 2018

Follow-up aborted #2672 and earlier attempts at avoiding orphaned tail processes on job hosts.

No need for watch-and kill threads or killing process groups. Just kill invoked subprocesses (either ssh command reinvocation or tail-follow command) if main process's PPID or PPPID (etc) changes.

Example: on host A: cylc cat-log --host=B <suite> <task-on-C>

  • => on host A: cat-log spawns subprocess ssh B "cylc cat-log <suite> <task-on-C>"
  • => on host B: cat-log spawns subprocess ssh C "cylc cat-log --remote <suite> <task-on-C>"
  • => on host C: cat-log spawns subprocess tail -f <task-on-C>.out

Ctrl-C (or exit GUI viewer) on host-A:

  • ssh from A to B dies
  • => on B: cat-log detects the previous, and kills its ssh subprocess to C
  • => on C: cat-log detects the previous, and kills its tail subprocess, then exits normally as finished

@hjoliver hjoliver self-assigned this May 19, 2018
@hjoliver hjoliver added this to the next release milestone May 19, 2018
@hjoliver
Copy link
Member Author

I haven't seen any clean-up failures with this implementation, after much testing - with bash-4 (remote exec) and tcsh (no remote exec) as login shell.

@hjoliver
Copy link
Member Author

(If this doesn't do it, much knashing of teeth will ensue 😬 )

@hjoliver
Copy link
Member Author

hjoliver commented May 19, 2018

Note edit mode doesn't really work from a third host (same on master, however). It results in (e.g.): ssh vim <file>. [Update: I disabled this].

Copy link
Contributor

@matthewrmshin matthewrmshin left a comment

Choose a reason for hiding this comment

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

Looks good. Tested as working in my environment for a combination of scenarios.

@matthewrmshin
Copy link
Contributor

@oliver-sanders please sanity check.

@matthewrmshin matthewrmshin added the bug Something is wrong :( label May 21, 2018
@hjoliver
Copy link
Member Author

Phew! Release tomorrow...

Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Sorry! Logic seems sane, one issue with the -m change.

bin/cylc-cat-log Outdated
mode = MODES[options.mode]
except KeyError:
# User chose log form.
mode = options.mode
Copy link
Member

Choose a reason for hiding this comment

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

There are later usages of options.mode which means cylc cat-log <suite> -m tail results in traceback.

@hjoliver
Copy link
Member Author

@oliver-sanders - good spotting, thanks. Will make the long-time-coming release on approval.

Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Looks OK, tested locally, lets hope we've got it this time!

  • Travis-CI failure was spurious, test passes locally.
  • Codacy would prefer /bin/ps to ps, this is something we can address at a later date if of importance.

@oliver-sanders oliver-sanders merged commit 1bd64fd into cylc:master May 22, 2018
@hjoliver hjoliver deleted the cat-log-tidy-fixx branch May 22, 2018 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong :(
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants