1194076 - replace ffprocess.py code with psutil API calls

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Julien Pagès (:parkouss)]

ffprocess.py will be obsolete when Talos will require psutil. We should rewrite the functionalities using psutil.

Comment 1

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

now we can tackle this !

Comment 2

[Julien Pagès (:parkouss)]

Attachment: 1194076.patch

So, this should do the job, and a bit more.

I have slightly changed the way we check if the browser is alive. This is now handled in talos_process.run_browser, so this ensure that after this call the browser was killed - or an exception is raised otherwise.

This allow to remove some things, mainly the big try/except we had in the ttest._run_test method.

Comment 3

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

Comment on attachment 8656433 [details] [diff] [review]
1194076.patch

Review of attachment 8656433 [details] [diff] [review]:
-----------------------------------------------------------------

only 1 small nit

::: talos/talos_process.py
@@ +31,5 @@
> +                self.process.wait(3)
> +            except psutil.TimeoutExpired:
> +                self.process.kill()
> +                # will raise TimeoutExpired if unable to kill
> +                self.process.wait(3)

shouldn't we wait, then try to kill again?

Comment 4

[Julien Pagès (:parkouss)]

oh, the logic is:

try to kill nicely, wait three seconds max. If that fail, try to kill not nicely, wait again three seconds max. If that fail, an error is raised.

I took that from the psutil example https://pythonhosted.org/psutil/#psutil.wait_procs, I think we are good.

Comment 5

[Julien Pagès (:parkouss)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=288c117b2be8

Comment 6

[Julien Pagès (:parkouss)]

So far, pretty good!

We had one failure with damp. Browser crash, see http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-288c117b2be8/try-linux/try_ubuntu32_hw_test-g2-e10s-bm105-tests1-linux-build17.txt.gz.

BTW this mean that our minidump is working! Nice to be sure of that. :)

So I will retrigger the failed job a few more times to investigate that issue. But anyway, this is unlikely related to that patch, so we are good to land it.

Comment 7

[Julien Pagès (:parkouss)]

So I retried 5 times each g2 job for e10s on all platforms, no other issue so far.

So, landed that patch in https://hg.mozilla.org/build/talos/rev/3fa2c76a3fb7.

Maybe we could investigate this intermittent by re-triggering more here, or from another bug.