The default bug view has changed. See this FAQ.

Unify in-tree configs, move run_filename into the tree and move more options into the tree

RESOLVED FIXED

Status

Release Engineering
Mozharness
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: armenzg, Assigned: armenzg)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [easier-mozharness] comment 38 for summary)

Attachments

(5 attachments, 4 obsolete attachments)

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?
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?
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?
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
Summary: Overwrite in-mozharness config values from in-tree configs → Read first in-tree configs (self.tree_config), then mozharness-configs (self.config)
This is similar to the self.live_config (r/w) that jlund proposed in a different bug.
Assignee: nobody → armenzg
This will help with this: https://wiki.mozilla.org/User:Armenzg/Proposals/Mozharness_changes#Move_and_extend_information_used_from_in-tree_mozharness_configs
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
Whiteboard: [easier-mozharness]
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
Blocks: 1067535
Duplicate of this bug: 1040383
https://tbpl.mozilla.org/?tree=Try&rev=26a3e29fea1b
https://tbpl.mozilla.org/?tree=Try&rev=a161796eaf46
https://tbpl.mozilla.org/?tree=Try&rev=115e15f814bf
https://tbpl.mozilla.org/?tree=Try&rev=56e430cbf2cf
https://tbpl.mozilla.org/?tree=Try&rev=384270b1b109
https://tbpl.mozilla.org/?tree=Try&rev=9b5c28a190e0
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
Attachment #8534996 - Flags: review?(jlund)
https://tbpl.mozilla.org/?tree=Try&rev=12ca297ec4c8
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
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).
Attachment #8535086 - Flags: feedback?(ahalberstadt)
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 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+
NI for me.

Keep an eye on https://hg.mozilla.org/try/rev/0ef6e54638db to make it.
Flags: needinfo?(armenzg)
(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)
Created attachment 8535683 [details] [diff] [review]
[mozharness] be able to read both formats of in-tree configs

This time around it works.
Attachment #8535683 - Flags: review?(jlund)
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
Created attachment 8536104 [details]
commands.txt

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
https://tbpl.mozilla.org/?tree=Try&rev=9671596cb0bc
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+
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.
https://tbpl.mozilla.org/?tree=Try&rev=42d6a28a455b
https://hg.mozilla.org/build/mozharness/rev/1698a3ff2a77
Wrt, to my comment 24 about the perma orange; the issue is gone!
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e9c3afb6702a
mozharness change merged to production.
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.
Attachment #8535086 - Attachment is obsolete: true
Attachment #8537235 - Flags: review?(ahalberstadt)
Attachment #8536104 - Attachment is obsolete: true
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
Attachment #8535080 - Attachment is obsolete: true
Attachment #8534996 - Attachment is obsolete: true
Attachment #8535683 - Flags: checked-in+
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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f89cbdabd57c
Attachment #8537235 - Flags: checked-in+
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/f89cbdabd57c
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
Attachment #8537235 - Attachment description: [mc] new structure for in-tree mozharness configs → [mc] (not uplifted) new structure for in-tree mozharness configs
Created attachment 8539532 [details] [diff] [review]
wip - [mc] move more into the tree
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.
Attachment #8539532 - Attachment description: wip - move more into the tree → wip - [mc] move more into the tree
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
Last Resolved: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.