-
Notifications
You must be signed in to change notification settings - Fork 600
Filesystem: optionally report ps -f even when killing many processes
#2114
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
base: main
Are you sure you want to change the base?
Conversation
New helper function ocf_log_pipe, similar to ocf_log, but read messages from stdin. Prefix each line with HA_LOGTAG and timestamp, and output to terminal / syslog / HA_LOGFILE / HA_DEBUGLOG / stderr as appropriate.
…ocesses The previous changes introduced optional `mount --move`, and implicitly used `xargs kill` if "many" (more than 24) active processes are found. Allow to explicitly chose whether to use "kill one by one" or `xargs kill`, and whether to report process details via `ps -f` or not. I leave the default at the established behavior `for pid; do ps -f $pid; kill -s $sig $pid; done`. Though I think the sane default would be to "safe,xargs,nops". The time consuming bit is the scan of the full process list, which ps does on each invocation, even if it is asked to report on one pid only. Reduce the overhead of potentially many `ocf_log` by using a single `ocf_log_pipe`.
|
portability note: to add timestamps, there is |
oalbrigt
left a comment
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 in general.
I've suggested some improvements to avoid issues with positional and opposite force_unmount modifiers.
|
|
||
| set -- $FORCE_UNMOUNT | ||
|
|
||
| if [ $1 = "move" ]; then |
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.
We should use a case for this parameter, and add it to the the for-case below, so we dont depend on it being first.
| done | ||
|
|
||
| # catch typos | ||
| if ! ocf_is_true $FORCE_UNMOUNT && [ $FORCE_UNMOUNT != "safe" ]; then |
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.
This can be removed if you move it to a case-statement in parse_force_unmount_modifiers()
|
|
||
| for m ; do | ||
| case $m in | ||
| move) |
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.
We shouldnt set multiple variables here, as they might be changed or not depending on order later, so we should also make sure that opposites, e.g. only move and nomove are not set.
Let's set one variable to true or false per match.
Filesystem: allow "force_unmount" to contain a comma separated list of "modifiers",
so you can explicitly chose to "move", but report process details and kill each process individually,
or use
xargs killand report no process details, or usexargs psthenxargs kill.Uses: new shellfuncs helper
ocf_log_pipe.See commit messages.