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)

(Assignee)

Description

3 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?
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

3 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?
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

3 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

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

Updated

3 years ago
Assignee: nobody → armenzg
(Assignee)

Comment 5

3 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

3 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

3 years ago
Whiteboard: [easier-mozharness]
(Assignee)

Comment 7

3 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
Blocks: 1067535
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1040383
(Assignee)

Comment 9

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=26a3e29fea1b
(Assignee)

Comment 10

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=a161796eaf46
(Assignee)

Comment 11

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=115e15f814bf
(Assignee)

Comment 12

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=56e430cbf2cf
(Assignee)

Comment 13

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=384270b1b109
(Assignee)

Comment 14

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=9b5c28a190e0
(Assignee)

Comment 15

3 years ago
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)
(Assignee)

Comment 16

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=12ca297ec4c8
(Assignee)

Comment 17

3 years ago
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
(Assignee)

Comment 18

3 years ago
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)
(Assignee)

Comment 19

3 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 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

3 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

3 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

3 years ago
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)
(Assignee)

Comment 24

3 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

3 years ago
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
(Assignee)

Comment 26

3 years ago
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+
(Assignee)

Comment 28

3 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

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=42d6a28a455b
(Assignee)

Comment 30

3 years ago
https://hg.mozilla.org/build/mozharness/rev/1698a3ff2a77
(Assignee)

Comment 31

3 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

3 years ago
mozharness change merged to production.
(Assignee)

Comment 33

3 years ago
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)
(Assignee)

Updated

3 years ago
Attachment #8536104 - Attachment is obsolete: true
(Assignee)

Comment 34

3 years ago
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
(Assignee)

Updated

3 years ago
Attachment #8534996 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
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+
(Assignee)

Comment 36

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f89cbdabd57c
(Assignee)

Updated

3 years ago
Attachment #8537235 - Flags: checked-in+
(Assignee)

Updated

3 years ago
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/f89cbdabd57c
(Assignee)

Comment 38

3 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

3 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

3 years ago
Created attachment 8539532 [details] [diff] [review]
wip - [mc] move more into the tree
(Assignee)

Comment 40

3 years ago
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.
(Assignee)

Updated

3 years ago
Attachment #8539532 - Attachment description: wip - move more into the tree → wip - [mc] move more into the tree
(Assignee)

Comment 41

2 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
Last Resolved: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.