808702 - talos counters needs some debugging

Status: RESOLVED WONTFIX

(Testing :: Talos, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

we have these missing counters which continue to show up.  For xres and %cpu we need to figure out what is going on.  Also there are problems with the process list which was a quick and easy fix.

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: print information about counters and fix terminate process (1.0)

this is green on try, did a few retriggers to make sure we were successful.

Comment 2

[William Lachance (:wlach)]

Comment on attachment 678398 [details] [diff] [review]
print information about counters and fix terminate process (1.0)

I'd like to see us being more careful about which exceptions we're catching. r+ with that addressed.

>--- a/talos/cmanager_linux.py	Mon Nov 05 13:00:05 2012 +0000
>+++ b/talos/cmanager_linux.py	Mon Nov 05 14:20:22 2012 -0500
>@@ -59,12 +59,30 @@
> def GetXRes(pids):
>   """Returns the total bytes used by X or raises an error if total bytes is not available"""
>   XRes = 0
>+  try:
>+    p = subprocess.Popen(["which xrestop"], shell=True, stdout=subprocess.PIPE)
>+    print "we found xres '%s'" % p.communicate()[0].strip()
>+  except:
>+    print "exception running 'which xres'"
>+
>+  try:
>+    p = subprocess.Popen(["which sed"], shell=True, stdout=subprocess.PIPE)
>+    print "we found sed '%s'" % p.communicate()[0].strip()
>+  except:
>+    print "exception running 'which sed'"
>+
>+  try:
>+    p = subprocess.Popen(["which tr"], shell=True, stdout=subprocess.PIPE)
>+    print "we found tr '%s'" % p.communicate()[0].strip()
>+  except:
>+    print "exception running 'which tr'"

The use of a blanket exception here can obscure problems. Instead, consider using subprocess's call() here (http://docs.python.org/2/library/subprocess.html#subprocess.call)

>     except:
>       print "Unexpected error executing '%s': %s", (cmdline, sys.exc_info())
>       raise
>@@ -72,7 +90,7 @@
>       data = float(data)
>       XRes += data
>     except:
>-      print "Invalid data, not a float"
>+      print "Invalid data '%s', not a float" % data
>       raise

Consider changing "except" to "except ValueError" so other errors are more obvious.

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

I had trouble with the subprocess.call() command since it returns 0/1.  I want to print out the results of the call.  I looked at check_output and that works for me.  At this point I might as well use Popen.  I will catch some more specific exceptions there, if you have any I should or shouldn't catch, please let me know.

Comment 4

[William Lachance (:wlach)]

(In reply to Joel Maher (:jmaher) from comment #3)
> I had trouble with the subprocess.call() command since it returns 0/1.  I
> want to print out the results of the call.  I looked at check_output and
> that works for me.  At this point I might as well use Popen.  I will catch
> some more specific exceptions there, if you have any I should or shouldn't
> catch, please let me know.

Hmm, so I actually don't think Popen will throw any exceptions at all if the process returns 0. So you'd need to actually check the return codes to make sure that the executables exist.

>>> p = subprocess.Popen(["which file-does-not-exist"], shell=True, stdout=subprocess.PIPE)
>>> print p.communicate()
('', None)
>>> p = subprocess.Popen(["which bash"], shell=True, stdout=subprocess.PIPE)>>> print p.communicate()
('/bin/bash\n', None)

However, are you sure that you actually care about the result of `which`? Reading the code, it looks like you just want to make sure the executables are within the path (i.e. that we have a non-zero exit code)

In the end though, I don't really care how this cat is skinned so long as we have the right behaviour. :) So feel free to use Popen or whatever is easiest for you.

Also, I forgot to bring this up earlier, but ahal has proposed adding a "which" method to mozfile which we could use for this sort of thing in bug 795329.

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

the intent was to show where the files where that I was using so maybe I could find a pattern on the failure cases.  Let me rework the popen stuff to look at the return codes.

Comment 6

[Jeff Hammel]

> Also, I forgot to bring this up earlier, but ahal has proposed adding a "which" method to mozfile which we could use for this sort of thing in bug 795329.

There is also the which module for python:

(tmp)│python -c 'import which; print which.which("bash")'
/bin/bash

You have to easy_install it first

Comment 7

[Jeff Hammel]

diff -r ec4d0dc9513e talos/ffprocess.py
--- a/talos/ffprocess.py	Mon Nov 05 13:00:05 2012 +0000
+++ b/talos/ffprocess.py	Mon Nov 05 14:20:22 2012 -0500
@@ -45,7 +45,7 @@
         processes_to_kill = filter(lambda n: n, ([process_name, child_process] +
                                                  self.extra_prog))
         utils.debug("Terminating: %s" % ", ".join(str(p) for p in processes_to_kill))
-        terminate_result = self.TerminateAllProcesses(browser_wait, *processes_to_kill)
+        terminate_result = self.TerminateAllProcesses(browser_wait, processes_to_kill)
         #check if anything is left behind
         if self.checkAllProcesses(process_name, child_process):
             #this is for windows machines.  when attempting to send kill messages to win processes the OS
diff -r ec4d0dc9513e talos/ffprocess_remote.py
--- a/talos/ffprocess_remote.py	Mon Nov 05 13:00:05 2012 +0000
+++ b/talos/ffprocess_remote.py	Mon Nov 05 14:20:22 2012 -0500
@@ -129,7 +129,7 @@
                 continue
         return processes_with_names
 
-    def TerminateAllProcesses(self, timeout, *process_names):
+    def TerminateAllProcesses(self, timeout, process_names):
         """Helper function to terminate all processes with the given process name
 
         Args:

why?

Comment 8

[Jeff Hammel]

We should not depend on sed or tr or the horrible process piping we currently have.  Instead we should get the output of xrestop and process it in python.  I also notice we don't check for grep....

Will ticket.

Comment 9

[Jeff Hammel]

(In reply to Jeff Hammel [:jhammel] from comment #8)
> We should not depend on sed or tr or the horrible process piping we
> currently have.  Instead we should get the output of xrestop and process it
> in python.  I also notice we don't check for grep....
> 
> Will ticket.

https://bugzilla.mozilla.org/show_bug.cgi?id=809276

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: counter hack with checking proc.returncode (1.5)

this addresses previous comments and feedback, also green on try.

Comment 11

[Jeff Hammel]

Comment on attachment 679268 [details] [diff] [review]
counter hack with checking proc.returncode (1.5)

So this is probably mostly bitrotted by the landing of bug 809726.  But a few comments:

If we're going to use `which` to check for things, we should probably use http://pypi.python.org/pypi/which .  There's also no longer a need to check for anything other than "xrestop" as the other programs are not used following bug 809726, though if we were checking for them it would probably be better to do so in a loop (or, bonus points, @requires("xrestop", "sed", ...)) vs several copies of the same logic

Should we close as a dupe?