Closed Bug 1084288 Opened 8 years ago Closed 8 years ago

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

Categories

(Release Engineering :: General, defect)

x86_64
Linux
defect
Not set
normal

Tracking

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

RESOLVED FIXED
Tracking Status
b2g-v2.0 --- wontfix
b2g-v2.0M --- wontfix
b2g-v2.1 --- affected
b2g-v2.1S --- affected
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: zcampbell, Assigned: zcampbell)

References

Details

Attachments

(1 file, 9 obsolete files)

Attached patch bug_1084288.patch (obsolete) — Splinter Review
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-
Attached patch bug_1084288.patch (obsolete) — Splinter Review
Fix it up proper to avoid this kind of thing in the future.
Attachment #8506778 - Attachment is obsolete: true
Attachment #8506880 - Flags: review?(sbruno)
^ 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.
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+
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'").
(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?
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)
Simone, I still can't reach that I'm sorry.
Flags: needinfo?(sbruno)
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)
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)
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?
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)
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?
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+
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!
Depends on: 1092132
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.
Blocks: 1092123
Attached patch bug_1084288.patch (obsolete) — Splinter Review
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+
Blocks: 1091158
Attached patch bug_1084288_2.patch (obsolete) — Splinter Review
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)
Attached patch bug_1084288_3.patch (obsolete) — Splinter Review
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)
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)
Ah, it probably depended upon my last one being reverted first (I didn't consider that!)
Attachment #8527813 - Flags: review?(jgriffin)
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)
Attachment #8527813 - Attachment is obsolete: true
Attachment #8527813 - Flags: review?(jgriffin)
Attachment #8528292 - Attachment is obsolete: true
Attachment #8528292 - Flags: review?(jgriffin)
Attached patch bug_1084288_4.patch (obsolete) — Splinter Review
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)
Blocks: 1092132
No longer depends on: 1092132
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-
Updated as per previous review comments.
Attachment #8528348 - Attachment is obsolete: true
Attachment #8529011 - Flags: review?(jgriffin)
(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!
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)
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)
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)
I've re-triggered some Ash jobs and I'll review the results soon.
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
Just checked a few test runs on TBPL and everything is running nicely.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Component: General Automation → General
You need to log in before you can comment on or make changes to this bug.