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

RESOLVED FIXED

Status

RESOLVED FIXED
4 years ago
6 months ago

People

(Reporter: zcampbell, Assigned: zcampbell)

Tracking

unspecified
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

(b2g-v2.0 wontfix, b2g-v2.0M wontfix, b2g-v2.1 affected, b2g-v2.1S affected, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(1 attachment, 9 obsolete attachments)

(Assignee)

Comment 1

4 years ago
Created attachment 8506778 [details] [diff] [review]
bug_1084288.patch

f? from Dave before I ask for r? from releng.
Attachment #8506778 - Flags: feedback?(dave.hunt)
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.
Attachment #8506778 - Flags: feedback?(dave.hunt) → feedback-
(Assignee)

Comment 3

4 years ago
Created attachment 8506880 [details] [diff] [review]
bug_1084288.patch

Fix it up proper to avoid this kind of thing in the future.
Attachment #8506778 - Attachment is obsolete: true
Attachment #8506880 - Flags: review?(sbruno)
(Assignee)

Comment 4

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

Updated

4 years ago
Blocks: 1056007
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
Attachment #8506880 - Flags: feedback+
(Assignee)

Comment 6

4 years ago
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"
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'").
(Assignee)

Comment 8

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

Comment 9

4 years ago
Meant to ni? in comment #8.
Flags: needinfo?(sbruno)
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.
Flags: needinfo?(sbruno)
(Assignee)

Comment 11

4 years ago
Simone, I still can't reach that I'm sorry.
Flags: needinfo?(sbruno)
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).
Flags: needinfo?(sbruno)
(Assignee)

Comment 13

4 years ago
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!
Flags: needinfo?(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.
Flags: needinfo?(jgriffin)
(Assignee)

Comment 15

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

Comment 17

4 years ago
The log is in comment #12. Simone ran it on his staging server.
It looks to me like the patch was not applied there; simone, can you confirm it was?
Flags: needinfo?(sbruno)
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.
Flags: needinfo?(sbruno)
(Assignee)

Comment 20

4 years ago
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?
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.
Attachment #8513550 - Attachment is obsolete: true
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.
Attachment #8506880 - Flags: review?(sbruno) → review+
(Assignee)

Comment 23

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

Updated

4 years ago
Depends on: 1092132
(Assignee)

Comment 24

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

Updated

4 years ago
Blocks: 1092123
(Assignee)

Comment 25

4 years ago
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.
Attachment #8506880 - Attachment is obsolete: true
Attachment #8514443 - Attachment is obsolete: true
Attachment #8515588 - Flags: review?(sbruno)
Attachment #8515588 - Flags: feedback?(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.
Attachment #8515588 - Flags: feedback?(jgriffin) → feedback+
(Assignee)

Updated

4 years ago
Blocks: 1091158
(Assignee)

Comment 27

4 years ago
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!
Attachment #8515588 - Attachment is obsolete: true
Attachment #8515588 - Flags: review?(sbruno)
Flags: needinfo?(mgervasini)
(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
Flags: needinfo?(mgervasini)
(Assignee)

Comment 29

4 years ago
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
Attachment #8527634 - Attachment is obsolete: true
Flags: needinfo?(mgervasini)
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
Flags: needinfo?(mgervasini)
(Assignee)

Comment 31

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

Updated

4 years ago
Attachment #8527813 - Flags: review?(jgriffin)
(Assignee)

Comment 32

4 years ago
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
Attachment #8528292 - Flags: review?(jgriffin)
(Assignee)

Updated

4 years ago
Attachment #8527813 - Attachment is obsolete: true
Attachment #8527813 - Flags: review?(jgriffin)
(Assignee)

Updated

4 years ago
Attachment #8528292 - Attachment is obsolete: true
Attachment #8528292 - Flags: review?(jgriffin)
(Assignee)

Comment 33

4 years ago
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.
Flags: needinfo?(mgervasini)
Attachment #8528348 - Flags: review?(jgriffin)
(Assignee)

Updated

4 years ago
Blocks: 1092132
No longer depends on: 1092132
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1056007
Landed in ash-mozharness as requested. http://hg.mozilla.org/build/ash-mozharness/rev/d613af0323e2
Flags: needinfo?(mgervasini)
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.
Attachment #8528348 - Flags: review?(jgriffin) → review-
(Assignee)

Comment 37

4 years ago
Created attachment 8529011 [details] [diff] [review]
bug_1084288_5.patch

Updated as per previous review comments.
Attachment #8528348 - Attachment is obsolete: true
Attachment #8529011 - Flags: review?(jgriffin)
(Assignee)

Comment 38

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

Comment 39

4 years ago
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.
Flags: needinfo?(ahalberstadt)
(Assignee)

Updated

4 years ago
Blocks: 1103964
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.
Attachment #8529011 - Flags: review?(jgriffin) → review+
Flags: needinfo?(ahalberstadt)
Zac Is there anything else to do here?? 
This is blocking multiple bugs can we get this merged?
Flags: needinfo?(zcampbell)
(Assignee)

Comment 42

4 years ago
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)?
Flags: needinfo?(zcampbell) → needinfo?(mgervasini)
(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
Flags: needinfo?(mgervasini)
(Assignee)

Comment 44

4 years ago
I've re-triggered some Ash jobs and I'll review the results soon.
(Assignee)

Comment 45

4 years ago
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.
Keywords: checkin-needed
(Assignee)

Comment 48

4 years ago
Just checked a few test runs on TBPL and everything is running nicely.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Keywords: checkin-needed

Updated

4 years ago
status-b2g-v2.0: --- → wontfix
status-b2g-v2.0M: --- → wontfix
status-b2g-v2.1: --- → affected
status-b2g-v2.1S: --- → affected
status-b2g-v2.2: --- → fixed
status-b2g-master: --- → fixed
Component: General Automation → General
Product: Release Engineering → Release Engineering
You need to log in before you can comment on or make changes to this bug.