Closed
Bug 1070041
Opened 10 years ago
Closed 9 years ago
Unify in-tree configs, move run_filename into the tree and move more options into the tree
Categories
(Release Engineering :: Applications: MozharnessCore, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: armenzg, Assigned: armenzg)
References
Details
(Whiteboard: [easier-mozharness] comment 38 for summary)
Attachments
(5 files, 4 obsolete files)
9.78 KB,
patch
|
jlund
:
review+
armenzg
:
checked-in+
|
Details | Diff | Splinter Review |
37.65 KB,
patch
|
ahal
:
review+
armenzg
:
checked-in+
|
Details | Diff | Splinter Review |
4.28 KB,
text/x-python-script
|
Details | |
39.76 KB,
patch
|
Details | Diff | Splinter Review | |
36.11 KB,
patch
|
Details | Diff | Splinter Review |
We currently only read $suite_options, however, there are a bunch of other options that could be configured from the tree. For now, I would like to read every value from in-tree and overwrite self.config values. This would require to unzip config/* as early in the process or grab directly from the hg web. Thoughts? Comments?
Comment 1•10 years ago
|
||
I proposed this awhile back and aki was strongly opposed to it. He argued that he made self.config read only on purpose so that it wouldn't get modified halfway through a job. This would make it easier to trace the execution path of jobs. In the end we ended up creating a second config called self.tree_config. Would just continuing to use that be ok?
Assignee | ||
Comment 2•10 years ago
|
||
We can create a function that would always read first from self.tree_config and then fall back to self.config if the value is not there. Eventually we would not have any of this job definition values inside of mozharness and not have to read self.config. Would this address your concerns?
Comment 3•10 years ago
|
||
Yes, I think I like that better. I mean personally I don't really care too much, but aki made a pretty strong argument way back when.. so I have this sense that something ominous will happen if we make self.config writeable :p
Assignee | ||
Updated•10 years ago
|
Summary: Overwrite in-mozharness config values from in-tree configs → Read first in-tree configs (self.tree_config), then mozharness-configs (self.config)
Assignee | ||
Comment 4•10 years ago
|
||
This is similar to the self.live_config (r/w) that jlund proposed in a different bug.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → armenzg
Assignee | ||
Comment 5•10 years ago
|
||
This will help with this: https://wiki.mozilla.org/User:Armenzg/Proposals/Mozharness_changes#Move_and_extend_information_used_from_in-tree_mozharness_configs
Assignee | ||
Comment 6•10 years ago
|
||
As part of this I would like to tackle these: Extend which information is read from in-tree configs * e.g. run_file_names, specific_tests_zip_dirs, in_tree_config, all_$category_suites (see this) * e.g. extra_args from mozilla-tests Splitting mozharness configs into 3: * [mozharness] generic production relevant config ** This config file can be used by many other jobs, however, specific values should go into their own (e.g. default_actions) * [mozharness] job specific values ** This config should have info with regards to which in-tree config to use and which actions to take * [in-tree] suite configuration information An example of this proposal: https://bug1043699.bugzilla.mozilla.org/attachment.cgi?id=8494540 generic and specific configs https://bug1043699.bugzilla.mozilla.org/attachment.cgi?id=8494000 suite config
Assignee | ||
Updated•10 years ago
|
Whiteboard: [easier-mozharness]
Assignee | ||
Comment 7•10 years ago
|
||
Currently, in our in-tree configs we specify the following: - suite_definitions - ${suite}_options (e.g. cppunittest_options, mochitest_options) - run_file_names (only the mulet config - I should adjust it to match the Android configs) - options (wpt - I think I should make it look like the Android configs) Besides unifying their format, I think we should specify in a comment which file in mozharness contains the str_format_values that applies to each config. If a new value is added to a specific config (.e.g "--app=%(app)s") we need to know which dictionary to update in mozharness. The remaining of the work will be to switch the usage of self.tree_config for query_value() from testbase.py [1]. query_value() first checks for self.tree_config and later from self.config. Here's the Android config structure: config = { "suite_definitions": { "$suite_name": { "run_filename": "runtestsremote.py", "options": ["--autorun", ..., "--app=%(app)s", ..., ], }, ... }, # end suite_definitions } [1] http://hg.mozilla.org/build/mozharness/file/default/mozharness/mozilla/testing/testbase.py#l118
Assignee | ||
Comment 9•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=26a3e29fea1b
Assignee | ||
Comment 10•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=a161796eaf46
Assignee | ||
Comment 11•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=115e15f814bf
Assignee | ||
Comment 12•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=56e430cbf2cf
Assignee | ||
Comment 13•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=384270b1b109
Assignee | ||
Comment 14•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=9b5c28a190e0
Assignee | ||
Comment 15•10 years ago
|
||
Do you want me to make a function in an upper class to share the logic? Associated push to try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5c3f319bdf26
Attachment #8534996 -
Flags: review?(jlund)
Assignee | ||
Comment 16•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=12ca297ec4c8
Assignee | ||
Comment 17•10 years ago
|
||
This file helps me create the new in-tree configuration files. The original hand crafted patch passed everything: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e14155abcf68 I'm now re-testing with the patch generated through this script: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a6e190c888a7
Assignee | ||
Comment 18•10 years ago
|
||
f? to know that I'm on the right track. I will turn into r? once I hear about your agreement and I have a green run on Try. The handcrafted patch passed. This one is generated through the script attached. Currently being tested in: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a6e190c888a7 I expected similar results as the original handcrafted patched (which I have not attached).
Attachment #8535086 -
Flags: feedback?(ahalberstadt)
Assignee | ||
Comment 19•10 years ago
|
||
Comment on attachment 8534996 [details] [diff] [review] [mozharness] be able to read both formats of in-tree configs The script I created has allowed to find some issues I had not noticed when handcrafting my patch. I will request again.
Attachment #8534996 -
Flags: review?(jlund)
Comment 20•10 years ago
|
||
Comment on attachment 8535086 [details] [diff] [review] [mc] new structure for in-tree mozharness configs Review of attachment 8535086 [details] [diff] [review]: ----------------------------------------------------------------- There's lots of extra whitespace in each file. I'd recommend setting up some kind of macro or plugin in your text editor that can take care of it. Other than that, this is awesome though! Sets the stage for many more general configuration options to come. I should have had the foresight to set it up like this to begin with.
Attachment #8535086 -
Flags: feedback?(ahalberstadt) → feedback+
Assignee | ||
Comment 21•10 years ago
|
||
NI for me. Keep an eye on https://hg.mozilla.org/try/rev/0ef6e54638db to make it.
Flags: needinfo?(armenzg)
Assignee | ||
Comment 22•10 years ago
|
||
(In reply to Andrew Halberstadt [:ahal] from comment #20) > Comment on attachment 8535086 [details] [diff] [review] > [mc] new structure for in-tree mozharness configs > > Review of attachment 8535086 [details] [diff] [review]: > ----------------------------------------------------------------- > > There's lots of extra whitespace in each file. I'd recommend setting up some > kind of macro or plugin in your text editor that can take care of it. > > Other than that, this is awesome though! Sets the stage for many more > general configuration options to come. I should have had the foresight to > set it up like this to begin with. I have not figured out how to make json.dump() to give me the output without the whitespaces. At worst, I will have to remove it manually.
Flags: needinfo?(armenzg)
Assignee | ||
Comment 23•10 years ago
|
||
This time around it works.
Attachment #8535683 -
Flags: review?(jlund)
Assignee | ||
Comment 24•10 years ago
|
||
gbrow, how can I figure out why jobs 8 and 16 keep on crashing? https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3d7a0598d7fe&filter-searchStr=android%20mochitest It's interesting that they would fail since I don't touch the android_emulator_unittest.py script on the patch for jlund and I don't touch any of the parent classes. I don't even touch the in-tree configs (see attachment 8535086 [details] [diff] [review]). Am I hitting any known issues? jlund, my review request stands as your patch applies to this other try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4464ac06a750
Assignee | ||
Comment 25•10 years ago
|
||
If it helps, both commands on a working and failling job are the same. Unless there is something funky on the build I don't know what it could be. http://ftp.mozilla.org/pub/mozilla.org/mobile/try-builds/armenzg@mozilla.com-3d7a0598d7fe/try-android-api-9/try_ubuntu64_vm_mobile_test-mochitest-16-bm52-tests1-linux64-build279.txt.gz http://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/mozilla-central-android-api-9/1418434667/mozilla-central_ubuntu64_vm_mobile_test-mochitest-16-bm53-tests1-linux64-build6.txt.gz
Assignee | ||
Comment 26•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=9671596cb0bc
Comment 27•10 years ago
|
||
Comment on attachment 8535683 [details] [diff] [review] [mozharness] be able to read both formats of in-tree configs Review of attachment 8535683 [details] [diff] [review]: ----------------------------------------------------------------- > Do you want me to make a function in an upper class to share the logic? sounds like a great idea! there are a few blocks that share pretty similar code. won't stop this patch but having a function like that would clean this up a lot. ::: scripts/b2g_desktop_unittest.py @@ +187,5 @@ > + else: > + suite_options = '%s_options' % suite > + if suite_options in self.tree_config: > + missing_key = False > + options = list(self.tree_config[suite_options]) are you wrapping this in a list() now to be safe or was there somewhere this was broken before? ::: scripts/marionette.py @@ +366,5 @@ > > + def preflight_run_marionette(self): > + """preflight commands for all tests""" > + # If the in tree config hasn't been loaded by a previous step, load it here. > + if len(self.tree_config) == 0: I've seen this in a few places of existing code. why are we not just using the idiom: `if not self.tree_config` which would accomplish the same thing as above? @@ +477,5 @@ > > + if options_group not in self.tree_config: > + # This allows using the new in-tree format > + options_group = options_group.split("_options")[0] > + print self.tree_config.keys() I think this direct print statement might be left in from debugging @@ +478,5 @@ > + if options_group not in self.tree_config: > + # This allows using the new in-tree format > + options_group = options_group.split("_options")[0] > + print self.tree_config.keys() > + assert options_group in self.tree_config["suite_definitions"] can we do mozharness's self.fatal here instead of assert like in the above parts of this patch?
Attachment #8535683 -
Flags: review?(jlund) → review+
Assignee | ||
Comment 28•10 years ago
|
||
Comment on attachment 8535683 [details] [diff] [review] [mozharness] be able to read both formats of in-tree configs Review of attachment 8535683 [details] [diff] [review]: ----------------------------------------------------------------- ::: scripts/b2g_desktop_unittest.py @@ +187,5 @@ > + else: > + suite_options = '%s_options' % suite > + if suite_options in self.tree_config: > + missing_key = False > + options = list(self.tree_config[suite_options]) I have no idea :/ I will remove it and see on my try push. ::: scripts/desktop_unittest.py @@ -311,2 @@ > os.path.join('gecko', 'testing', self.config['in_tree_config']))) > - options = list(self.tree_config[suite_options]) I wonder if the whole "list" thing started because of this original code. I think I can fix the "enumerate" below. I think I copy and pasted from the desktop_unittest.py into the other jobs. ::: scripts/marionette.py @@ +366,5 @@ > > + def preflight_run_marionette(self): > + """preflight commands for all tests""" > + # If the in tree config hasn't been loaded by a previous step, load it here. > + if len(self.tree_config) == 0: I will fix it. @@ +477,5 @@ > > + if options_group not in self.tree_config: > + # This allows using the new in-tree format > + options_group = options_group.split("_options")[0] > + print self.tree_config.keys() Fixed. @@ +478,5 @@ > + if options_group not in self.tree_config: > + # This allows using the new in-tree format > + options_group = options_group.split("_options")[0] > + print self.tree_config.keys() > + assert options_group in self.tree_config["suite_definitions"] Fixed.
Assignee | ||
Comment 29•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=42d6a28a455b
Assignee | ||
Comment 30•10 years ago
|
||
https://hg.mozilla.org/build/mozharness/rev/1698a3ff2a77
Assignee | ||
Comment 31•10 years ago
|
||
Wrt, to my comment 24 about the perma orange; the issue is gone! https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e9c3afb6702a
Assignee | ||
Comment 32•10 years ago
|
||
mozharness change merged to production.
Assignee | ||
Comment 33•10 years ago
|
||
Without extra white spaces and the official r? (rather than f?) As I mentioned the perma-orange issue is no more.
Attachment #8535086 -
Attachment is obsolete: true
Attachment #8537235 -
Flags: review?(ahalberstadt)
Assignee | ||
Updated•10 years ago
|
Attachment #8536104 -
Attachment is obsolete: true
Assignee | ||
Comment 34•10 years ago
|
||
Adding separators in json.dump() removed the whitespaces issue. Found in here: http://bugs.python.org/issue16333
Attachment #8535080 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8534996 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8535683 -
Flags: checked-in+
Comment 35•10 years ago
|
||
Comment on attachment 8537235 [details] [diff] [review] [mc] (not uplifted) new structure for in-tree mozharness configs Review of attachment 8537235 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, looks great! I didn't verify every individual option :p, but if it works on try then ship it!
Attachment #8537235 -
Flags: review?(ahalberstadt) → review+
Assignee | ||
Comment 36•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f89cbdabd57c
Assignee | ||
Updated•10 years ago
|
Attachment #8537235 -
Flags: checked-in+
Assignee | ||
Updated•10 years ago
|
Keywords: leave-open
Assignee | ||
Comment 38•10 years ago
|
||
My Q4 goal was not very clearly defined from the beginning. At the end of the quarter I can say that we have some degree of success since we have made move the most important parameters into the tree and we have discovered many more. Having mozharness try at the end helped a lot, however, the lack of pining in the rest of the trees is what made it miserable to deploy (uplifts, timing, pre-patch, post-patch). In a sense this bug has accomplished the following: * determined that all mozharness configs are indeed in the tree * make the in-tree configs more consistent * move run_filenames into the in-tree configs * move more options into the in-tree configs What we have failed to do is: * make all test jobs to read the value of testsdir (bug 1113309) * drop self.tree_config (bug 1113321) and read in-tree configs through hg web * Review extra_args from mozilla-tests and move into the tree what makes sense (bug 1113326) "We can create a function that would always read first from self.tree_config and then fall back to self.config if the value is not there." Splitting mozharness configs into 3: * [mozharness] generic production relevant config ** This config file can be used by many other jobs, however, specific values should go into their own (e.g. default_actions) * [mozharness] job specific values ** This config should have info with regards to which in-tree config to use and which actions to take * [in-tree] suite configuration information NOTE: Hard to do without pinning, unless, *all* mozharness configs (including releng information) live in the tree Follow up clean up in bug 1113126.
Status: NEW → ASSIGNED
Keywords: leave-open
Summary: Read first in-tree configs (self.tree_config), then mozharness-configs (self.config) → Unify in-tree configs, move run_filename into the tree and move more options into the tree
Whiteboard: [easier-mozharness] → [easier-mozharness] comment 38 for summary
Assignee | ||
Updated•10 years ago
|
Attachment #8537235 -
Attachment description: [mc] new structure for in-tree mozharness configs → [mc] (not uplifted) new structure for in-tree mozharness configs
Assignee | ||
Comment 39•10 years ago
|
||
Assignee | ||
Comment 40•10 years ago
|
||
We will see how far we can take this approach. This does not work. It is a wip.
Assignee | ||
Updated•10 years ago
|
Attachment #8539532 -
Attachment description: wip - move more into the tree → wip - [mc] move more into the tree
Assignee | ||
Comment 41•9 years ago
|
||
Mass comment: With mozharness moving into the tree [1] and pinning of mozharness we can assume that anything left (if any) in this bug will be easily taken care of. [1] http://jordan-lund.ghost.io/mozharness-steps-into-the-forest
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•