1084288 - Gip suite is having --type set twice and causing incorrect type to be used.

Status: RESOLVED FIXED

Description

[Zac C (:zac)]

In the Gip job, the --type is being custom set:

http://hg.mozilla.org/build/mozharness/file/475f036e77dd/scripts/marionette.py#l425

and then subsequently overridden here:
http://hg.mozilla.org/build/mozharness/file/475f036e77dd/scripts/marionette.py#l445

Comment 1

[Zac C (:zac)]

Created attachment 8506778 [details] [diff] [review]
bug_1084288.patch

f? from Dave before I ask for r? from releng.

Comment 2

[Dave Hunt [:davehunt] ⌚️UTC+1 (PTO until 22-October-2018)]

Comment on attachment 8506778 [details] [diff] [review]
bug_1084288.patch

Review of attachment 8506778 [details] [diff] [review]:
-----------------------------------------------------------------

::: scripts/marionette.py
@@ +459,5 @@
>  
>          if self.config.get('emulator') or self.config.get('gaiatest'):
>              config_fmt_args['symbols_path'] = self.symbols_path
>  
> +        if not config_fmt_args.has_key('type'):

This will always be false because we're adding --type earlier by extending cmd. You could create a test_type variable and if it doesn't get set fall back to the configuration.

Comment 3

[Zac C (:zac)]

Created attachment 8506880 [details] [diff] [review]
bug_1084288.patch

Fix it up proper to avoid this kind of thing in the future.

Comment 4

[Zac C (:zac)]

^ in the above patch I removed the reference to tbpl-manifest.ini as we intend to remove that file.

I removed as many cmd.append calls as I could to avoid clashes between setting via cmd.append and the config_fmt_args object.

Comment 5

[Simone Bruno [:simone]]

Comment on attachment 8506880 [details] [diff] [review]
bug_1084288.patch

Review of attachment 8506880 [details] [diff] [review]:
-----------------------------------------------------------------

Apart from my really minor comment (about a comment), this looks ok at first glance.
Given the extension of the patch, though, I warmly recommend testing on a staging environment before landing. I can do that at the beginning of next week.

::: scripts/marionette.py
@@ +433,5 @@
> +                config_fmt_args['total-chunks'] = self.config.get('total_chunks')
> +
> +            # Bug 1046694
> +            # using a different manifest if a specific gip-suite is specified
> +            # --type parameter depends on gip-suite

I would remove this comment, since --type command line parameter will be no longer used

Comment 6

[Zac C (:zac)]

Comment on attachment 8506880 [details] [diff] [review]
bug_1084288.patch

Review of attachment 8506880 [details] [diff] [review]:
-----------------------------------------------------------------

::: scripts/marionette.py
@@ +433,5 @@
> +                config_fmt_args['total-chunks'] = self.config.get('total_chunks')
> +
> +            # Bug 1046694
> +            # using a different manifest if a specific gip-suite is specified
> +            # --type parameter depends on gip-suite

Simone, we do still use this, but we only set it for "functional"

Comment 7

[Simone Bruno [:simone]]

Hi zac,

I applied your patch to my test environment, see e.g.: http://dev-master1.srv.releng.scl3.mozilla.com:8401/builders/b2g_ubuntu64_vm%20cedar%20opt%20test%20gaia-ui-test-functional-1/builds/6

The mozharness version being used is https://hg.mozilla.org/users/sbruno_mozilla.com/mozharness, which includes your patch; it seems to me that --type parameter is still passed twice (in this case, first with with an entry like "'--type', 'b2g-external'", then with "'--type=b2g'").

Comment 8

[Zac C (:zac)]

(In reply to Simone Bruno [:simone] from comment #7)
> Hi zac,
> 
> I applied your patch to my test environment, see e.g.:
> http://dev-master1.srv.releng.scl3.mozilla.com:8401/builders/
> b2g_ubuntu64_vm%20cedar%20opt%20test%20gaia-ui-test-functional-1/builds/6
> 
Simone, I can't reach this domain from the London office. Can you pop the content of that into a pastebin or bugzilla attachment?

Comment 9

[Zac C (:zac)]

Meant to ni? in comment #8.

Comment 10

[Simone Bruno [:simone]]

Hi zac,

The master was temporary down, now you should be able to access http://dev-master1.srv.releng.scl3.mozilla.com:8401/builders/b2g_ubuntu64_vm%20cedar%20opt%20test%20gaia-ui-test-functional-1/builds/6

If you cannot access it now, I will send you the output in other ways.

Comment 11

[Zac C (:zac)]

Simone, I still can't reach that I'm sorry.

Comment 12

[Simone Bruno [:simone]]

Created attachment 8513550 [details]
gip functional suite execution log in test environment

Hi zac, here is the output of the functional suite, in which --type parameter is set twice on command line.
The mozharness version being used here is https://hg.mozilla.org/users/sbruno_mozilla.com/mozharness (which includes your patch).

Comment 13

[Zac C (:zac)]

Jgriffin, I've spent quite some time on this and I cannot find where the second --type=b2g is coming from.

Can you find the time to help me, or guide me a bit? Happy to do the patch, if only I can find what's causing the problem!

Comment 14

[Jonathan Griffin (:jgriffin)]

One copy is coming from here:

http://hg.mozilla.org/build/mozharness/file/601b5ea63b77/scripts/marionette.py#l432

The other copy is coming from here:

http://dxr.mozilla.org/mozilla-central/source/testing/config/mozharness/marionette.py#47

My preference would be to get rid of the first one.

Comment 15

[Zac C (:zac)]

Jgriffin, my patch from comment #3 gets rid of that (in code) but in practice it's still coming back.

Looking at my patch again, shouldn't the config from mozilla-central be formatted with config_fmt_args on line 467?

Comment 16

[Jonathan Griffin (:jgriffin)]

(In reply to Zac C (:zac) from comment #15)
> Jgriffin, my patch from comment #3 gets rid of that (in code) but in
> practice it's still coming back.
> 
> Looking at my patch again, shouldn't the config from mozilla-central be
> formatted with config_fmt_args on line 467?

It should, and it is, so maybe I don't understand the question.

Could you provide a log for how this fails with your patch?

Comment 17

[Zac C (:zac)]

The log is in comment #12. Simone ran it on his staging server.

Comment 18

[Jonathan Griffin (:jgriffin)]

It looks to me like the patch was not applied there; simone, can you confirm it was?

Comment 19

[Simone Bruno [:simone]]

Sorry guys, my fault :-| ! I pushed the patch to the production branch of my copy of the mozharness repo, but the test is configured to use default... I am going to re-run the test after fixing the environment setup. Apologies for the time you wasted because of this.

Comment 20

[Zac C (:zac)]

I suspect now that the chunking will break because it's not set here:
http://dxr.mozilla.org/mozilla-central/source/testing/config/mozharness/marionette.py#47

I don't have time to revise that today, but Simone can you run it anyway and I will work with that and any other errors there may be?

Comment 21

[Simone Bruno [:simone]]

Created attachment 8514443 [details]
gip functional suite execution log in test environment

zac, the gaia-ui-test-functional-1 test now executes successfully, and the --type parameter duplication is not present anymore.
Also the chunking parameters are set correctly.

Comment 22

[Simone Bruno [:simone]]

Comment on attachment 8506880 [details] [diff] [review]
bug_1084288.patch

Review of attachment 8506880 [details] [diff] [review]:
-----------------------------------------------------------------

Test results in Comment 21 look ok to me, so - unless you have further concerns about how chunking parameters are managed now - this can be landed.

Comment 23

[Zac C (:zac)]

The chunking is not set correctly. I need to file another patch to update the file in m-c file and then probably re-visit this patch again too!

Comment 24

[Zac C (:zac)]

I made a patch in bug 1092132 to set the m-c defaults but it can't be landed without this patch and vice versa.
I need to think a bit more about a way to land portions of each patch separately without such great dependencies on one another.

Comment 25

[Zac C (:zac)]

Created attachment 8515588 [details] [diff] [review]
bug_1084288.patch

Simone, I have had to add back in the cmd.extend code that sets the chunks until bug 1092123 has landed.

Then after these have landed I can start on bug 1092132 which will remove the redundancy and tidy up the scripts in general.

Can you run this again on your staging environment? 

Thanks.

Comment 26

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8515588 [details] [diff] [review]
bug_1084288.patch

Review of attachment 8515588 [details] [diff] [review]:
-----------------------------------------------------------------

::: scripts/marionette.py
@@ +465,5 @@
>  
>          options_group = self._get_options_group(self.config.get('emulator'),
>                                                  self.config.get('gaiatest'))
> +
> +        if config_fmt_args.has_key('binary'):

I know this has just been moved, but it was weird in the original too.  config_fmt_args always has a binary key defined (it's defined in the original definition), so we can just eliminate the 'if' clause here, I think.

Comment 27

[Zac C (:zac)]

Created attachment 8527634 [details] [diff] [review]
bug_1084288_2.patch

Fixing the bit rot.

Mgerva, can you try this on mozharness-ash for me? Thanks!

Comment 28

[Massimo Gervasini [:massimo]]

(In reply to Zac C (:zac) from comment #27)
> Created attachment 8527634 [details] [diff] [review]
> bug_1084288_2.patch
> 
> Fixing the bit rot.
> 
> Mgerva, can you try this on mozharness-ash for me? Thanks!

Landed in ash-mozharness

Comment 29

[Zac C (:zac)]

Created attachment 8527813 [details] [diff] [review]
bug_1084288_3.patch

Mgerva, the last patch was not passing the correct binary.

Can you push this one to mozharness-ash for me? 

Thanks

Comment 30

[Massimo Gervasini [:massimo]]

Created attachment 8528292 [details] [diff] [review]
[ash-mozharness] Bug 1084288 - updated patch for ash-mozharness.patch

hey Zac,

your patch did not apply directly on ash-mozharness so I've applied it to mozharness instead and copied the patched marionette.py back into ash-mozharenss.

Let me know if you have any problems

Comment 31

[Zac C (:zac)]

Ah, it probably depended upon my last one being reverted first (I didn't consider that!)

Comment 32

[Zac C (:zac)]

Comment on attachment 8528292 [details] [diff] [review]
[ash-mozharness] Bug 1084288 - updated patch for ash-mozharness.patch

Review of attachment 8528292 [details] [diff] [review]:
-----------------------------------------------------------------

Jgriffin, can you review these two patches? 

Initially they regress chunking and gecko-log output, but I will follow this up with a patch in here that hopefully we can land asap afterwards.
https://bugzilla.mozilla.org/show_bug.cgi?id=1092132

Comment 33

[Zac C (:zac)]

Created attachment 8528348 [details] [diff] [review]
bug_1084288_4.patch

Now I am sure this one will work! Jgriffin, can you review it? As it will temporarily regress chunking and gecko-log output, I'll prepare the patch as per bug 1092132 so we can land it ASAP afterwards.

Mgerva, can you push this to mozharness-ash please? Thankyou.

Comment 35

[Massimo Gervasini [:massimo]]

Landed in ash-mozharness as requested. http://hg.mozilla.org/build/ash-mozharness/rev/d613af0323e2

Comment 36

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8528348 [details] [diff] [review]
bug_1084288_4.patch

Review of attachment 8528348 [details] [diff] [review]:
-----------------------------------------------------------------

r- to clarify the issue around tbpl-manifest.ini, but overall looks good.

::: scripts/marionette.py
@@ +384,5 @@
> +            'binary': self.binary_path,
> +            'address': self.config.get('marionette_address'),
> +            'raw_log_file': os.path.join(dirs["abs_blob_upload_dir"],
> +                                         "mn_structured_full.log"),
> +            'gecko_log': self.query_abs_dirs()['abs_blob_upload_dir']

might as well use the dirs variable here, rather than calling self.query_abs_dirs() again

@@ -412,5 @@
> -                if not os.access(binary, os.F_OK):
> -                    binary = os.path.join(binary_path, 'b2g')
> -
> -            manifest = os.path.join(dirs['abs_gaiatest_dir'], 'gaiatest', 'tests',
> -                                    'tbpl-manifest.ini')

I _think_ we need to keep this line for tests running on older branches, like 1.4; those won't hit the self.config.get('gip_suite') block below.

Comment 37

[Zac C (:zac)]

Created attachment 8529011 [details] [diff] [review]
bug_1084288_5.patch

Updated as per previous review comments.

Comment 38

[Zac C (:zac)]

(In reply to Jonathan Griffin (:jgriffin) from comment #36)
> I _think_ we need to keep this line for tests running on older branches,
> like 1.4; those won't hit the self.config.get('gip_suite') block below.

Yes I think you're right - at least let's play it safe and put it back!

Comment 39

[Zac C (:zac)]

ahal, are you comfortable reviewing this while jgriffin is off for thanksgiving? We'd really like to get it merged as it's causing some issues with stability on Gip/Treeherder. If you're not or too busy it can wait.

Comment 40

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8529011 [details] [diff] [review]
bug_1084288_5.patch

Review of attachment 8529011 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm; we might want to land and test on ash to make sure there are no surprises.

Comment 41

[Florin Strugariu [:Bebe]]

Zac Is there anything else to do here?? 
This is blocking multiple bugs can we get this merged?

Comment 42

[Zac C (:zac)]

Nothing Bebe! Let's ask Mgerva to push it to Ash as per Jgriffin's request.

Mgerva, can you push it to Ash for a test run (but first remove any other test patches of mine left from previous testing)?

Comment 43

[Massimo Gervasini [:massimo]]

(In reply to Zac C (:zac) from comment #42)
> Nothing Bebe! Let's ask Mgerva to push it to Ash as per Jgriffin's request.
> 
> Mgerva, can you push it to Ash for a test run (but first remove any other
> test patches of mine left from previous testing)?

Landed on ash-mozharness

Comment 44

[Zac C (:zac)]

I've re-triggered some Ash jobs and I'll review the results soon.

Comment 45

[Zac C (:zac)]

Results are good as far as I can see. It 'breaks' the chunking but that is expected and fixed by landing Bug 1092132 immediately afterwards.

Comment 46

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8529011 [details] [diff] [review]
bug_1084288_5.patch

https://hg.mozilla.org/build/mozharness/rev/8a973eeef512

Comment 47

[Jordan Lund (:jlund)]

In production: https://hg.mozilla.org/build/mozharness/rev/8a973eeef512

Comment 48

[Zac C (:zac)]

Just checked a few test runs on TBPL and everything is running nicely.