Closed Bug 1172918 Opened 7 years ago Closed 7 years ago

clean up talos options that are not used


(Testing :: Talos, defect)

Not set


(firefox41 affected)

Tracking Status
firefox41 --- affected


(Reporter: jmaher, Assigned: parkouss)




(2 files)

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 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 config, probably not a cli option)

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.
:jmaher, how can we get rid of options used by mozharness ?
Flags: needinfo?(jmaher)
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)
Attached patch 1172918.patchSplinter Review
Assignee: nobody → j.parkouss
Attachment #8632079 - Flags: review?(jmaher)
Comment on attachment 8632079 [details] [diff] [review]

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/
@@ -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+
Thanks Joel!

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

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/ --resultsServer server/ --resultsLink link
python talos/ --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 ?
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.
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.
yes, I was suggesting removing the entire results_urls option, but we can do that later.
Sounds good, maybe with the 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) ?

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:

Anyway, we should clean some things in the mozharness/mozilla/testing/ file. Maybe once we remove android support, or even better after that once we refactor the configuration file (we won't have to call, so this would be required anyway).
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):
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.

ok, we need to modify mozharness proper:

and for android:

now to test this out I would recommend:

keep in mind this week we might get all of the mozharness repo moved into m-c:!topic/

but we can test it out locally and see how it works.
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.
leave-open so we can land mozharness patch then talos patch from this bug.
Keywords: leave-open
It seems like mozharness is in tree now. Pushed again to see:
Yay, seems all good! Pushed to try the whole thing:

(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
Bug 1172918 - clean up talos options that are not used (removing from mozharness). r=jmaher
Attachment #8636095 - Flags: review?(jmaher)
Plenty of errors, like:

Calling ['C:\\slave\\test\\build\\venv\\Scripts\\python', 'C:\\slave\\test\\build\\talos_repo\\talos\\', '-v', '--executablePath', 'C:\\slave\\test\\build\\application\\firefox\\firefox', '--title', 't-xp32-ix-069', '--symbolsPath', '', '--activeTests', u'damp:tps', '--results_url', '', '--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\", line 1146, in <module>
11:14:35     INFO -      sys.exit(main())
11:14:35     INFO -    File "C:\slave\test\build\talos_repo\talos\", 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\", 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\", line 433, in parse_args
11:14:35     INFO -      self(*config)
11:14:35     INFO -    File "C:\slave\test\build\talos_repo\talos\", 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\", line 330, in __call__
11:14:35     INFO -      self.validate()
11:14:35     INFO -    File "C:\slave\test\build\talos_repo\talos\", 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\", 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;
Attachment #8636095 - Flags: review?(jmaher) → review+
Comment on attachment 8636095 [details]
MozReview Request: Bug 1172918 - clean up talos options that are not used (removing from mozharness). r=jmaher

thanks, these changes are great.  I assume this removes all instances of datazilla from talos configs in mozharness land.
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
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 ?
sounds like a plan.

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. :)
Closed: 7 years ago
Resolution: --- → FIXED
Depends on: 1187082
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.