Armen [:armenzg] Assignee
	Description • 10 years ago

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?

	Andrew Halberstadt [:ahal]
	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?

	Armen [:armenzg] 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?

	Andrew Halberstadt [:ahal]
	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

	Armen [:armenzg] Assignee
	Comment 4 • 10 years ago

This is similar to the self.live_config (r/w) that jlund proposed in a different bug.

	Armen [: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

	Armen [:armenzg] 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

	Armen [:armenzg] 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

	Armen [:armenzg] Assignee
	Comment 9 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=26a3e29fea1b

	Armen [:armenzg] Assignee
	Comment 10 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=a161796eaf46

	Armen [:armenzg] Assignee
	Comment 11 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=115e15f814bf

	Armen [:armenzg] Assignee
	Comment 12 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=56e430cbf2cf

	Armen [:armenzg] Assignee
	Comment 13 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=384270b1b109

	Armen [:armenzg] Assignee
	Comment 14 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=9b5c28a190e0

	Armen [:armenzg] 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

	Armen [:armenzg] Assignee
	Comment 16 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=12ca297ec4c8

	Armen [:armenzg] 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

	Armen [:armenzg] 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).

	Armen [:armenzg] 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.

	Andrew Halberstadt [:ahal]
	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.

	Armen [:armenzg] Assignee
	Comment 21 • 10 years ago

NI for me.

Keep an eye on https://hg.mozilla.org/try/rev/0ef6e54638db to make it.

	Armen [: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.

	Armen [:armenzg] Assignee
	Comment 23 • 10 years ago

This time around it works.

	Armen [:armenzg] 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

	Armen [:armenzg] 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

	Armen [:armenzg] Assignee
	Comment 26 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=9671596cb0bc

	Jordan Lund (:jlund)
	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?

	Armen [:armenzg] 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.

	Armen [:armenzg] Assignee
	Comment 29 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=42d6a28a455b

	Armen [:armenzg] Assignee
	Comment 30 • 10 years ago

https://hg.mozilla.org/build/mozharness/rev/1698a3ff2a77

	Armen [:armenzg] 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

	Armen [:armenzg] Assignee
	Comment 32 • 10 years ago

mozharness change merged to production.

	Armen [:armenzg] 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.

	Armen [:armenzg] Assignee
	Comment 34 • 10 years ago

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

Found in here:
http://bugs.python.org/issue16333

	Andrew Halberstadt [:ahal]
	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!

	Armen [:armenzg] Assignee
	Comment 36 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/f89cbdabd57c

	Ryan VanderMeulen [:RyanVM]
	Comment 37 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/f89cbdabd57c

	Armen [:armenzg] 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.

	Armen [:armenzg] Assignee
	Comment 40 • 10 years ago

We will see how far we can take this approach.
This does not work. It is a wip.

	Armen [:armenzg] 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