Skip to content

Move warning about too many pids to error path #834

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Aug 20, 2018
Merged

Move warning about too many pids to error path #834

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 15 additions & 12 deletions agent/tool-scripts/kvm-spinlock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,42 +258,45 @@ function safe_kill() {
# should not happen
return 1
fi

# check that the pid corresponds to the tool
pidlist=$(pidof -x $tool)
typeset -i len=0
for p in $pidlist ;do
len=$len+1
if [[ $p == $pid ]] ;then
pid_to_kill=$pid
fi
done
# hack: individual tools will have a different number of pids running.
# we might just delete the warning if it gets too cumbersome.
if [[ $len > 4 || ($len > 3 && $tool != "turbostat") ]] ;then
warn_log "Too many pids for $tool: $pidlist -- maybe old tools running? Use pbench-kill-tools."
fi


rc=0
if [ ! -z "$pid_to_kill" ] ;then
kill -s $signal $pid_to_kill 2>/dev/null
rc=$?
fi

[[ $rc == 0 ]] && return 0

# check if the process is still running.
pid_to_kill=""
typeset -i len=0
pidlist=$(pidof -x $tool)
for p in $pidlist ;do
len=$len+1
if [[ $p == $pid ]] ;then
# why is the pid still there?
kill -s KILL $pid 2>/dev/null
return 2
pid_to_kill=$pid
# else
Copy link
Member

@ndokos ndokos May 30, 2018

Choose a reason for hiding this comment

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

Maybe add an explanation of why there are other processess running?

Copy link
Member Author

@portante portante Jul 2, 2018

Choose a reason for hiding this comment

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

I don't think we actually know, but we will emit a message on line 298 if we don't find a pid to kill.

# why are there other tool processes running?
# should we kill them?
fi
done

if [ ! -z "$pid_to_kill" ] ;then
kill -s KILL $pid_to_kill 2>/dev/null
return 2
fi

if [[ $len > 4 || ($len > 3 && $tool != "turbostat") ]] ;then
Copy link
Member

@k-rister k-rister Aug 17, 2018

Choose a reason for hiding this comment

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

Should there be a comment here that explain why there is a special case for turbostat?

Copy link
Member Author

@portante portante Aug 17, 2018

Choose a reason for hiding this comment

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

Yes there should be. Does anybody know the history of that check?

Copy link
Member Author

@portante portante Aug 17, 2018

Choose a reason for hiding this comment

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

@atheurer ?

Copy link
Member Author

@portante portante Aug 17, 2018

Choose a reason for hiding this comment

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

@ndokos, it looks like the turbostat check landed in PR #199, back in v0.39 of the pbench-agent.

Do you remember why we need to check for turbostat?

Copy link
Member

@ndokos ndokos Aug 17, 2018

Choose a reason for hiding this comment

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

Sorry - I don't remember. I'll try to instrument safer_kill and see why turbostat seems to behave differently from everything else.

Copy link
Member

@ndokos ndokos Aug 17, 2018

Choose a reason for hiding this comment

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

I get 3 pids from pidof -x turbostat and also 3 pids from pidof -x iostat. The only difference is that there is a turbostat-datalog between the pbench-tool turbostat call and the "real" one, but pidof does not count the datalog process. So far, there does not seem to be a reason for treating turbostat differently.

Copy link
Member Author

@portante portante Aug 17, 2018

Choose a reason for hiding this comment

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

Okay, can we track this as a separate piece of work in issue #887, and not make further changes here, since this PR did not change that behavior?

Copy link
Member

@ndokos ndokos Aug 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Since I've already approved, I'll wait for an OK from either @k-rister or/and @atheurer and merge it.

warn_log "Too many pids for $tool: $pidlist -- maybe old tools running? Use pbench-kill-tools."
fi
return 0
}

Expand Down
2 changes: 0 additions & 2 deletions agent/util-scripts/gold/pbench-stop-tools/test-05.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
+++ Running test-05 pbench-stop-tools
[warn][1900-01-01T00:00:00.000000] Too many pids for turbostat: 123463 123464 123465 123466 123467 -- maybe old tools running? Use pbench-kill-tools.
--- Finished test-05 pbench-stop-tools (status=1)
+++ pbench tree state
/var/tmp/pbench-test-utils/pbench
Expand All @@ -15,7 +14,6 @@
/var/tmp/pbench-test-utils/pbench/pbench.log:[debug][1900-01-01T00:00:00.000000] [pbench-stop-tools]started: --dir=/var/tmp/pbench-test-utils/pbench/tmp
/var/tmp/pbench-test-utils/pbench/pbench.log:[debug][1900-01-01T00:00:00.000000] [pbench-stop-tools] /var/tmp/pbench-test-utils/opt/pbench-agent/tool-scripts/turbostat --stop --iteration=1 --group=default --dir=/var/tmp/pbench-test-utils/pbench/tmp --interval=3
/var/tmp/pbench-test-utils/pbench/pbench.log:[debug][1900-01-01T00:00:00.000000] stopping turbostat
/var/tmp/pbench-test-utils/pbench/pbench.log:[warn][1900-01-01T00:00:00.000000] Too many pids for turbostat: 123463 123464 123465 123466 123467 -- maybe old tools running? Use pbench-kill-tools.
/var/tmp/pbench-test-utils/pbench/pbench.log:[debug][1900-01-01T00:00:00.000000] [pbench-stop-tools]completed:
--- pbench.log file contents
+++ test-execution.log file contents
Expand Down
2 changes: 0 additions & 2 deletions agent/util-scripts/gold/pbench-stop-tools/test-07.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
+++ Running test-07 pbench-stop-tools
[warn][1900-01-01T00:00:00.000000] Too many pids for vmstat: 123456 123457 123458 123459 -- maybe old tools running? Use pbench-kill-tools.
--- Finished test-07 pbench-stop-tools (status=0)
+++ pbench tree state
/var/tmp/pbench-test-utils/pbench
Expand All @@ -15,7 +14,6 @@
/var/tmp/pbench-test-utils/pbench/pbench.log:[debug][1900-01-01T00:00:00.000000] [pbench-stop-tools]started: --dir=/var/tmp/pbench-test-utils/pbench/tmp
/var/tmp/pbench-test-utils/pbench/pbench.log:[debug][1900-01-01T00:00:00.000000] [pbench-stop-tools] /var/tmp/pbench-test-utils/opt/pbench-agent/tool-scripts/vmstat --stop --iteration=1 --group=default --dir=/var/tmp/pbench-test-utils/pbench/tmp --interval=50
/var/tmp/pbench-test-utils/pbench/pbench.log:[debug][1900-01-01T00:00:00.000000] stopping vmstat
/var/tmp/pbench-test-utils/pbench/pbench.log:[warn][1900-01-01T00:00:00.000000] Too many pids for vmstat: 123456 123457 123458 123459 -- maybe old tools running? Use pbench-kill-tools.
/var/tmp/pbench-test-utils/pbench/pbench.log:[debug][1900-01-01T00:00:00.000000] [pbench-stop-tools]completed:
--- pbench.log file contents
+++ test-execution.log file contents
Expand Down