1172918 - clean up talos options that are not used

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

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

it seems as though we have a lot of talos options which we rarely if not never use.

2 things I see going away in the near months:
1) graph server (and the recent removal of datazilla)
2) android

but they are still here, so we cannot remove them completely.  If we know they are going away and refactoring stuff, we can add a comment and a small hack that is easy to remove.

some options to remove:
results_urls (hardcode to graph server, except in --develop mode- no cli needed, mozharness depends on this)
datazilla_urls (deprecated, mozharness depends on this)
authfile (deprecated, mozharness depends on this)
resultsServer (should be fine to remove)
resultsUrl (should be fine to remove)
browser_wait (default to a value- verify in test.py there are no other values, we can hack this for android if needed)

noChrome (android uses this- we can hardcode for android)
mainthread (we only collect this with xperf, we can be smarter here, possibly mozharness uses this)
fast (never used this)
test_timeout (might need to hack this for android)
browser_log (should always be the same value- if so, lets hardcode it)
shutdown (I don't believe we use this anymore)
extraPrefs (set in test.py config, probably not a cli option)
tptimeout
tpdelay


also the preferences are hardcoded- and we do things in different spots.  Most likely we don't need all these preferences! that could be saved for a different bug :)

Just removing these options would help reduce the scope of things to make it easier to see the overall picture.

Comment 1

[Julien Pagès (:parkouss)]

:jmaher, how can we get rid of options used by mozharness ?

Comment 2

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

we would need to patch mozharness- probably a worthwhile attempt right now.  Keep in mind the desktop vs android stuff is different.

Comment 3

[Julien Pagès (:parkouss)]

Attachment: 1172918.patch

Comment 4

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

Comment on attachment 8632079 [details] [diff] [review]
1172918.patch

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

would like to see if this:
1) works on try
2) posts to graphserver and perfherder

assuming that works, this is a good first pass!

::: talos/PerfConfigurator.py
@@ -614,5 @@
> -                    "Can't user resultsServer/resultsLink and results_url:"
> -                    " use results_url instead"
> -                )
> -            self.config['results_urls'] = \
> -                ['http://%s%s' % (resultsServer, resultsLink)]

where do we hardcode the graphserver url?  I am fine removing this, but we still need to upload to graph server.

Comment 5

[Julien Pagès (:parkouss)]

Thanks Joel!

In fact the graph server url is not hardcoded, it still defined but with the --results_urls option:

https://github.com/mozilla/build-talos/blob/master/talos/PerfConfigurator.py#L66

These options were only an alternative (the "new" way will be to just use results_url).


So the only thing that may need to be done is to fix external calls, eg:

python talos/PerfConfigurator.py --resultsServer server/ --resultsLink link
to
python talos/PerfConfigurator.py --results_urls http://server/link


I will push that to try as soon as I have the time. I'm not sure how posting to graphserver and perfherder
 can be done, maybe you could help me for that ?

Comment 6

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

we can hardcode the value of results_urls/graphserver since it is static and going away by the end of the year.

perfherder doesn't have a url, we just parse the log file.

Comment 7

[Julien Pagès (:parkouss)]

Do you mean we should also remove the results_urls option ? Well I suggest that we remove it once it is no more used, but for now it does not hurt.

The options I am trying to remove here are obsolete, meaning that for most of them they have another new command line that can achieve the same thing. This patch only adress that.

Comment 8

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

yes, I was suggesting removing the entire results_urls option, but we can do that later.

Comment 9

[Julien Pagès (:parkouss)]

Sounds good, maybe with the output.py refactoring ? I just added a comment in bug 1172918.

But thinking more of it, I can understand that you want to optimize in case we have talos harnesses calls to patch. We'll see on try - If we have external talos calls to patch, it may be simple to try to do that all in one go (and then completely get rid of this option so we don't have to come back to that later). :)

Hmm BTW, can you explain me how I can see/commit patches to the harnesses ? I mean where talos build steps are called (I suppose it is a buildbot script somewhere) ?

Thanks!

Comment 10

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

one place to look for how the script is run:
http://hg.mozilla.org/build/mozharness/file/31dad082f2e4/mozharness/mozilla/testing/talos.py
https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/mozilla/testing/talos.py

and:
https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos.json?from=talos.json&case=true#1

if we are modifying everything, then lets modify it once :)

Comment 11

[Julien Pagès (:parkouss)]

Nice, thanks for digging this up for me. :)

So it looks like what I modified won't impact how mozharness call talos. So, pushed to try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6eaa8930b1ba

Anyway, we should clean some things in the mozharness/mozilla/testing/talos.py file. Maybe once we remove android support, or even better after that once we refactor the configuration file (we won't have to call PerfConfigurator.py, so this would be required anyway).

Comment 12

[Julien Pagès (:parkouss)]

Ok, all failed, I missed something. :)

In fact datazilla stuff was still used in harness, but is no more in talos (marked as deprecated) so I did a dxr search, and killed everything datazilla-related in mozharness. This patch should be landed on mozilla-central before the other one.

Pushed to try (full talos):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=721a80fb8947

Comment 13

[Julien Pagès (:parkouss)]

Hmm, all green but I still see the parameters in the raw log files (see [1] for example) of the try builds. I am still missing something, waiting for help here to understand what is going on.

[1] http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-721a80fb8947/try-linux/try_ubuntu32_hw_test-chromez-bm104-tests1-linux-build449.txt.gz

Comment 14

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

ok, we need to modify mozharness proper:
http://hg.mozilla.org/build/mozharness/file/31dad082f2e4/mozharness/mozilla/testing/talos.py#l415

and for android:
http://hg.mozilla.org/build/mozharness/file/31dad082f2e4/scripts/android_panda_talos.py#l347

now to test this out I would recommend:
http://armenzg.blogspot.co.uk/2014/11/pinning-mozharness-from-in-tree-aka.html

keep in mind this week we might get all of the mozharness repo moved into m-c:
https://groups.google.com/forum/#!topic/mozilla.tools/37-gPzNZ3LQ

but we can test it out locally and see how it works.

Comment 15

[Julien Pagès (:parkouss)]

So we discussed all this a bit more with Joel, and we are waiting for mozharness to be in tree - it would be easier then to land patches on mozharness. Should be < one week.

Comment 16

[Julien Pagès (:parkouss)]

leave-open so we can land mozharness patch then talos patch from this bug.

Comment 17

[Julien Pagès (:parkouss)]

It seems like mozharness is in tree now. Pushed again to see:

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

Comment 18

[Julien Pagès (:parkouss)]

Yay, seems all good! Pushed to try the whole thing:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=949951a9bd17

(navigate trough parent revisions to see that everything is here)

So if that work, I would say that we are good to:

1. land mozharness patch
2. land talos patch

Comment 19

[Julien Pagès (:parkouss)]

Attachment: MozReview Request: Bug 1172918 - clean up talos options that are not used (removing from mozharness). r=jmaher

Bug 1172918 - clean up talos options that are not used (removing from mozharness). r=jmaher

Comment 20

[Julien Pagès (:parkouss)]

Plenty of errors, like:

Calling ['C:\\slave\\test\\build\\venv\\Scripts\\python', 'C:\\slave\\test\\build\\talos_repo\\talos\\PerfConfigurator.py', '-v', '--executablePath', 'C:\\slave\\test\\build\\application\\firefox\\firefox', '--title', 't-xp32-ix-069', '--symbolsPath', 'https://queue.taskcluster.net/v1/task/hJLqM7wnQIiG1B2f3FNwtA/artifacts/public/build/firefox-42.0a1.en-US.win32.crashreporter-symbols.zip', '--activeTests', u'damp:tps', '--results_url', 'http://graphs.mozilla.org/server/collect.cgi', '--output', 'talos.yml', '--branchName', 'Try', '--webServer', 'localhost'] with output_timeout 3600
11:14:35    ERROR -  Traceback (most recent call last):
11:14:35     INFO -    File "C:\slave\test\build\talos_repo\talos\PerfConfigurator.py", line 1146, in <module>
11:14:35     INFO -      sys.exit(main())
11:14:35     INFO -    File "C:\slave\test\build\talos_repo\talos\PerfConfigurator.py", line 1141, in main
11:14:35     INFO -      args = conf.parse_args(args)
11:14:35     INFO -    File "C:\slave\test\build\talos_repo\talos\PerfConfigurator.py", line 750, in parse_args
11:14:35     INFO -      args = Configuration.parse_args(self, *args, **kwargs)
11:14:35     INFO -    File "C:\slave\test\build\talos_repo\talos\configuration.py", line 433, in parse_args
11:14:35     INFO -      self(*config)
11:14:35     INFO -    File "C:\slave\test\build\talos_repo\talos\PerfConfigurator.py", line 529, in __call__
11:14:35     INFO -      return Configuration.__call__(self, *args)
11:14:35     INFO -    File "C:\slave\test\build\talos_repo\talos\configuration.py", line 330, in __call__
11:14:35     INFO -      self.validate()
11:14:35     INFO -    File "C:\slave\test\build\talos_repo\talos\PerfConfigurator.py", line 719, in validate
11:14:35     INFO -      self.tests(activeTests, overrides, global_overrides, counters))
11:14:35     INFO -    File "C:\slave\test\build\talos_repo\talos\PerfConfigurator.py", line 844, in tests
11:14:35     INFO -      % missing)
11:14:35     INFO -  __main__.ConfigurationError: No definition found for test(s): ['tps']

Seems not related though;

Comment 21

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

Comment on attachment 8636095 [details]
MozReview Request: Bug 1172918 - clean up talos options that are not used (removing from mozharness). r=jmaher

https://reviewboard.mozilla.org/r/13625/#review12295

thanks, these changes are great.  I assume this removes all instances of datazilla from talos configs in mozharness land.

Comment 22

[Julien Pagès (:parkouss)]

checkin-needed for patch 2 " MozReview Request: Bug 1172918 - clean up talos options that are not used (removing from mozharness). r=jmaher" only. (patch 1 can't be applied anyway, it is a Talos patch)

Push try was green (see comment 17).

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9437415142ea

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9437415142ea

Comment 25

[Julien Pagès (:parkouss)]

So, mozharness patch has been landed!

We should be good to land the talos patch now (try was done in comment 18 and is good - was bad at first because I used an outdated talos repo).

Joel, can I land the Talos patch ?

Comment 26

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

sounds like a plan.

Comment 27

[Julien Pagès (:parkouss)]

Pushed:

https://hg.mozilla.org/build/talos/rev/adcbadddeeb9

I'm closing this bug even if there are other options we could remove, I think we did enough in this bug. Once android support will be dropped, we will reevaluate what is obsolete anyway.

:jmaher, feel free to reopen it if you think we should. :)

Comment 28

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.