Last Comment Bug 1070041 - Unify in-tree configs, move run_filename into the tree and move more options into the tree
: Unify in-tree configs, move run_filename into the tree and move more options ...
Status: RESOLVED FIXED
[easier-mozharness] comment 38 for su...
:
Product: Release Engineering
Classification: Other
Component: Mozharness (show other bugs)
: unspecified
: x86_64 Linux
-- normal (vote)
: ---
Assigned To: Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4)
: Jordan Lund (:jlund)
:
Mentors:
: 1040383 (view as bug list)
Depends on:
Blocks: 1067535
  Show dependency treegraph
 
Reported: 2014-09-19 10:42 PDT by Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4)
Modified: 2015-03-31 14:44 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
[mozharness] be able to read both formats of in-tree configs (6.05 KB, patch)
2014-12-11 07:17 PST, Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4)
no flags Details | Diff | Splinter Review
create_new_structure.py (4.23 KB, text/plain)
2014-12-11 09:54 PST, Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4)
no flags Details
[mc] new structure for in-tree mozharness configs (37.79 KB, patch)
2014-12-11 10:06 PST, Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4)
ahalberstadt: feedback+
Details | Diff | Splinter Review
[mozharness] be able to read both formats of in-tree configs (9.78 KB, patch)
2014-12-12 09:34 PST, Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4)
jlund: review+
armenzg: checked‑in+
Details | Diff | Splinter Review
commands.txt (1.55 KB, text/plain)
2014-12-13 14:36 PST, Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4)
no flags Details
[mc] (not uplifted) new structure for in-tree mozharness configs (37.65 KB, patch)
2014-12-16 08:25 PST, Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4)
ahalberstadt: review+
armenzg: checked‑in+
Details | Diff | Splinter Review
create_new_structure.py (4.28 KB, text/x-python-script)
2014-12-16 08:27 PST, Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4)
no flags Details
wip - [mc] move more into the tree (39.76 KB, patch)
2014-12-19 14:09 PST, Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4)
no flags Details | Diff | Splinter Review
wip - [mh] drop all job definitions from mozharness (36.11 KB, patch)
2014-12-19 14:10 PST, Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4)
no flags Details | Diff | Splinter Review

Description User image Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4) 2014-09-19 10:42:48 PDT
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 User image Andrew Halberstadt [:ahal] 2014-09-19 11:04:26 PDT
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?
Comment 2 User image Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4) 2014-09-19 11:12:25 PDT
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 User image Andrew Halberstadt [:ahal] 2014-09-19 11:23:32 PDT
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
Comment 4 User image Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4) 2014-09-19 11:28:19 PDT
This is similar to the self.live_config (r/w) that jlund proposed in a different bug.
Comment 5 User image Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4) 2014-09-23 13:33:58 PDT
This will help with this: https://wiki.mozilla.org/User:Armenzg/Proposals/Mozharness_changes#Move_and_extend_information_used_from_in-tree_mozharness_configs
Comment 6 User image Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4) 2014-09-24 12:30:24 PDT
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
Comment 7 User image Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4) 2014-10-02 10:41:40 PDT
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
Comment 8 User image Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4) 2014-11-26 10:48:31 PST
*** Bug 1040383 has been marked as a duplicate of this bug. ***
Comment 9 User image Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4) 2014-11-26 14:22:04 PST
https://tbpl.mozilla.org/?tree=Try&rev=26a3e29fea1b
Comment 10 User image Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4) 2014-11-28 06:48:27 PST
https://tbpl.mozilla.org/?tree=Try&rev=a161796eaf46
Comment 11 User image Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4) 2014-12-05 13:06:18 PST
https://tbpl.mozilla.org/?tree=Try&rev=115e15f814bf
Comment 12 User image Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4) 2014-12-09 16:41:06 PST
https://tbpl.mozilla.org/?tree=Try&rev=56e430cbf2cf
Comment 13 User image Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4) 2014-12-10 09:10:48 PST
https://tbpl.mozilla.org/?tree=Try&rev=384270b1b109
Comment 14 User image Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4) 2014-12-10 13:46:32 PST
https://tbpl.mozilla.org/?tree=Try&rev=9b5c28a190e0
Comment 15 User image Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4) 2014-12-11 07:17:18 PST
Created attachment 8534996 [details] [diff] [review]
[mozharness] be able to read both formats of in-tree configs

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
Comment 16 User image Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4) 2014-12-11 09:49:08 PST
https://tbpl.mozilla.org/?tree=Try&rev=12ca297ec4c8
Comment 17 User image Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4) 2014-12-11 09:54:01 PST
Created attachment 8535080 [details]
create_new_structure.py

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
Comment 18 User image Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4) 2014-12-11 10:06:01 PST
Created attachment 8535086 [details] [diff] [review]
[mc] new structure for in-tree mozharness configs

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).
Comment 19 User image Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4) 2014-12-11 11:09:32 PST
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.
Comment 20 User image Andrew Halberstadt [:ahal] 2014-12-12 06:18:06 PST
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.
Comment 21 User image Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4) 2014-12-12 06:32:01 PST
NI for me.

Keep an eye on https://hg.mozilla.org/try/rev/0ef6e54638db to make it.
Comment 22 User image Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4) 2014-12-12 06:32:59 PST
(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.
Comment 23 User image Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4) 2014-12-12 09:34:04 PST
Created attachment 8535683 [details] [diff] [review]
[mozharness] be able to read both formats of in-tree configs

This time around it works.
Comment 24 User image Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4) 2014-12-13 14:30:36 PST
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
Comment 26 User image Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4) 2014-12-15 10:29:11 PST
https://tbpl.mozilla.org/?tree=Try&rev=9671596cb0bc
Comment 27 User image Jordan Lund (:jlund) 2014-12-15 12:47:06 PST
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?
Comment 28 User image Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4) 2014-12-15 17:20:27 PST
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.
Comment 29 User image Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4) 2014-12-15 17:20:54 PST
https://tbpl.mozilla.org/?tree=Try&rev=42d6a28a455b
Comment 30 User image Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4) 2014-12-16 05:44:42 PST
https://hg.mozilla.org/build/mozharness/rev/1698a3ff2a77
Comment 31 User image Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4) 2014-12-16 06:55:32 PST
Wrt, to my comment 24 about the perma orange; the issue is gone!
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e9c3afb6702a
Comment 32 User image Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4) 2014-12-16 07:10:15 PST
mozharness change merged to production.
Comment 33 User image Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4) 2014-12-16 08:25:57 PST
Created attachment 8537235 [details] [diff] [review]
[mc] (not uplifted) new structure for in-tree mozharness configs

Without extra white spaces and the official r? (rather than f?)

As I mentioned the perma-orange issue is no more.
Comment 34 User image Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4) 2014-12-16 08:27:42 PST
Created attachment 8537236 [details]
create_new_structure.py

Adding separators in json.dump() removed the whitespaces issue.

Found in here:
http://bugs.python.org/issue16333
Comment 35 User image Andrew Halberstadt [:ahal] 2014-12-17 06:34:00 PST
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!
Comment 36 User image Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4) 2014-12-17 07:14:25 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/f89cbdabd57c
Comment 37 User image Ryan VanderMeulen [:RyanVM] 2014-12-17 18:08:22 PST
https://hg.mozilla.org/mozilla-central/rev/f89cbdabd57c
Comment 38 User image Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4) 2014-12-18 13:24:56 PST
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.
Comment 39 User image Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4) 2014-12-19 14:09:34 PST
Created attachment 8539532 [details] [diff] [review]
wip - [mc] move more into the tree
Comment 40 User image Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4) 2014-12-19 14:10:38 PST
Created attachment 8539533 [details] [diff] [review]
wip - [mh] drop all job definitions from mozharness

We will see how far we can take this approach.
This does not work. It is a wip.
Comment 41 User image Armen Zambrano - Back on March 27th [:armenzg] (EDT/UTC-4) 2015-03-31 14:44:08 PDT
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

Note You need to log in before you can comment on or make changes to this bug.