clean up talos options that are not used

RESOLVED FIXED

Status

RESOLVED FIXED
4 years ago
10 months ago

People

(Reporter: jmaher, Assigned: parkouss)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(firefox41 affected)

Details

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
:jmaher, how can we get rid of options used by mozharness ?
Flags: needinfo?(jmaher)
(Reporter)

Comment 2

4 years ago
we would need to patch mozharness- probably a worthwhile attempt right now.  Keep in mind the desktop vs android stuff is different.
Flags: needinfo?(jmaher)
(Assignee)

Comment 3

4 years ago
Created attachment 8632079 [details] [diff] [review]
1172918.patch
Assignee: nobody → j.parkouss
Status: NEW → ASSIGNED
Attachment #8632079 - Flags: review?(jmaher)
(Reporter)

Comment 4

4 years ago
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.
Attachment #8632079 - Flags: review?(jmaher) → review+
(Assignee)

Comment 5

4 years ago
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 ?
(Reporter)

Comment 6

4 years ago
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.
(Assignee)

Comment 7

4 years ago
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.
(Reporter)

Comment 8

4 years ago
yes, I was suggesting removing the entire results_urls option, but we can do that later.
(Assignee)

Comment 9

4 years ago
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!
(Assignee)

Comment 11

4 years ago
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).
(Assignee)

Comment 12

4 years ago
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
(Assignee)

Comment 13

4 years ago
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/j.parkouss@gmail.com-721a80fb8947/try-linux/try_ubuntu32_hw_test-chromez-bm104-tests1-linux-build449.txt.gz
(Reporter)

Comment 14

4 years ago
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.
(Assignee)

Comment 15

4 years ago
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.
(Assignee)

Comment 16

4 years ago
leave-open so we can land mozharness patch then talos patch from this bug.
Keywords: leave-open
(Assignee)

Comment 17

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

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

Comment 18

4 years ago
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
(Assignee)

Comment 19

4 years ago
Created attachment 8636095 [details]
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
Attachment #8636095 - Flags: review?(jmaher)
(Assignee)

Comment 20

4 years ago
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;
(Reporter)

Updated

4 years ago
Attachment #8636095 - Flags: review?(jmaher) → review+
(Reporter)

Comment 21

4 years ago
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.
(Assignee)

Comment 22

4 years ago
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).
Keywords: checkin-needed
(Assignee)

Comment 25

3 years ago
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 ?
(Reporter)

Comment 26

3 years ago
sounds like a plan.
(Assignee)

Comment 27

3 years ago
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. :)
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.