Closed Bug 1200928 Opened 7 years ago Closed 7 years ago

[TaskCluster] Specify the options to use busybox in xpcshell tests

Categories

(Taskcluster :: Services, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla46

People

(Reporter: edgar, Assigned: edgar)

References

Details

Attachments

(3 files, 4 obsolete files)

Attached patch WIP, Patch, v1 (obsolete) — Splinter Review
With this patch, we can use busybox for xpcshell correctly [1].
It works pretty good on ics. But we have another error on x86-kk/kk, 

> 02:21:45     INFO - Calling ['/home/worker/build/venv/bin/python', 'runtestsb2g.py', '--adbpath=/home/worker/build/emulator/b2g-distro/out/host/linux-x86/bin/adb', '--b2gpath=/home/worker/build/emulator/b2g-distro', '--emulator=x86', '--logdir=/home/worker/build', '--manifest=tests/xpcshell.ini', '--use-device-libs', '--testing-modules-dir=/home/worker/build/tests/modules', '--symbols-path=https://queue.taskcluster.net/v1/task/if789mYST52v_N9Y7Y7AaQ/artifacts/public/build/emulator.crashreporter-symbols.zip', '--busybox=/home/worker/build/busybox', '--total-chunks=20', '--this-chunk=1', '--log-raw=/home/worker/build/blobber_upload_dir/xpcshell_raw.log', '--log-errorsummary=/home/worker/build/blobber_upload_dir/xpcshell_errorsummary.log'] with output_timeout 1000
> 02:22:23     INFO -  pushing /system/bin/busybox
> 02:22:23     INFO -  Traceback (most recent call last):
> 02:22:23     INFO -    File "runtestsb2g.py", line 228, in <module>
> 02:22:23     INFO -      main()
> 02:22:23     INFO -    File "runtestsb2g.py", line 221, in main
> 02:22:23     INFO -      run_remote_xpcshell(parser, options, args, log)
> 02:22:23     INFO -    File "runtestsb2g.py", line 183, in run_remote_xpcshell
> 02:22:23     INFO -      marionette = Marionette(**kwargs)
> 02:22:23     INFO -    File "/home/worker/build/venv/local/lib/python2.7/site-packages/marionette_driver/marionette.py", line 628, in __init__
> 02:22:23     INFO -      self.emulator.install_busybox(busybox=busybox)
> 02:22:23     INFO -    File "/home/worker/build/venv/local/lib/python2.7/site-packages/mozrunner/devices/base.py", line 170, in install_busybox
> 02:22:23     INFO -      self.dm._verifyZip()
> 02:22:23     INFO -    File "/home/worker/build/venv/local/lib/python2.7/site-packages/mozdevice/devicemanagerADB.py", line 721, in _verifyZip
> 02:22:23     INFO -      raise DMError("zip not available")
> 02:22:23     INFO -  mozdevice.devicemanager.DMError: zip not available
> 02:22:29     INFO - Return code: 1

[1] https://treeherder.allizom.org/#/jobs?repo=try&revision=85ae9665a1f7
Okay, I just found some clues. When running xpcshell locally, the mach system use different version of busybox for arm and x86 [1]. But in Taskcluster, both arm and x86 emulator share the same binary [2], this probably can explain why the busybox doesn't work on x86-kk.

[1] https://dxr.mozilla.org/mozilla-central/source/testing/xpcshell/mach_commands.py#38-41
[2] http://hg.mozilla.org/build/mozharness/file/f5cb8461d40c/configs/b2g/emulator_automation_config.py#l7
(In reply to John Dai[:johnz][:jdai] from comment #3)
> After I use different version of busybox for arm and x86 [1], Taskcluster
> still has comment #1 issue [2].
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/testing/xpcshell/
> mach_commands.py#38-41
> [2] https://treeherder.allizom.org/#/jobs?repo=try&revision=f17b79e039ea

I think this is right direction, because I saw many of x86 testcases pass.[1]
But I got another error message:

[1] https://treeherder.allizom.org/#/jobs?repo=try&revision=f17b79e039ea

> 10:06:44     INFO - Calling ['/home/worker/build/venv/bin/python', 'runtestsb2g.py', '--adbpath=/home/worker/build/emulator/b2g-distro/out/host/linux-x86/bin/adb', '--b2gpath=/home/worker/build/emulator/b2g-distro', '--emulator=x86', '--logdir=/home/worker/build', '--manifest=tests/xpcshell.ini', '--use-device-libs', '--testing-modules-dir=/home/worker/build/tests/modules', '--symbols-path=https://queue.taskcluster.net/v1/task/qNUgSNmORNiMpg9zSX17cQ/artifacts/public/build/emulator.crashreporter-symbols.zip', '--busybox=/home/worker/build/busybox', '--total-chunks=20', '--this-chunk=11', '--log-raw=/home/worker/build/blobber_upload_dir/xpcshell_raw.log', '--log-errorsummary=/home/worker/build/blobber_upload_dir/xpcshell_errorsummary.log'] with output_timeout 1000
> 10:07:42     INFO -  pushing /system/bin/busybox
> 10:07:42     INFO -  Traceback (most recent call last):
> 10:07:42     INFO -    File "runtestsb2g.py", line 228, in <module>
> 10:07:42     INFO -      main()
> 10:07:42     INFO -    File "runtestsb2g.py", line 221, in main
> 10:07:42     INFO -      run_remote_xpcshell(parser, options, args, log)
> 10:07:42     INFO -    File "runtestsb2g.py", line 183, in run_remote_xpcshell
> 10:07:42     INFO -      marionette = Marionette(**kwargs)
> 10:07:42     INFO -    File "/home/worker/build/venv/local/lib/python2.7/site-packages/marionette_driver/marionette.py", line 628, in __init__
> 10:07:42     INFO -      self.emulator.install_busybox(busybox=busybox)
> 10:07:42     INFO -    File "/home/worker/build/venv/local/lib/python2.7/site-packages/mozrunner/devices/base.py", line 162, in install_busybox
> 10:07:42     INFO -      self.dm.pushFile(busybox, self.app_ctx.remote_busybox, retryLimit=10)
> 10:07:42     INFO -    File "/home/worker/build/venv/local/lib/python2.7/site-packages/mozdevice/devicemanagerADB.py", line 216, in pushFile
> 10:07:42     INFO -      raise DMError("Error pushing file %s -> %s; output: %s" % (localname, destname, proc.output))
> 10:07:42     INFO -  mozdevice.devicemanager.DMError: Error pushing file /home/worker/build/busybox -> /system/bin/busybox; output: ["failed to copy '/home/worker/build/busybox' to '/system/bin/busybox': Read-only file system"]
> 10:07:46     INFO - Return code: 1
Depends on: 1202310, 1202311
No longer depends on: 1202310, 1202311
Depends on: 1188330
Assignee: nobody → echen
Attached patch WIP, patch, v2 (obsolete) — Splinter Review
Attachment #8655792 - Attachment is obsolete: true
Attached patch WIP, patch, v3 (obsolete) — Splinter Review
Attachment #8674182 - Attachment is obsolete: true
Comment on attachment 8674826 [details] [diff] [review]
WIP, patch, v3

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

In order to use busybox on both arm and x86, we need two different binary for them, I extend the option that can accept multiple busybox url and choose the suitable one according to the current emulator type. Hi ahal, I would like to have some feedback from you before providing a formal patch for review. Thank you.
Attachment #8674826 - Flags: feedback?(ahalberstadt)
Comment on attachment 8674826 [details] [diff] [review]
WIP, patch, v3

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

Could we instead create a new config for emulator-x86 that overrides the 'busybox_url' key in 'emulator_automation_config.py'? I sort of suspect we'll want separate configs anyway, as there will probably be other things that are different as well.

::: testing/mozharness/scripts/b2g_emulator_unittest.py
@@ +35,5 @@
>           "default": "browser",
>           "help": "The type of tests to run",
>           }
>      ], [
> +        ["--busybox-urls"],

There isn't really a very good way of specifying a dictionary from the command line. So if someone actually tried to use this, it would fail.
Attachment #8674826 - Flags: feedback?(ahalberstadt) → feedback-
(In reply to Andrew Halberstadt [:ahal] from comment #8)
> Comment on attachment 8674826 [details] [diff] [review]
> WIP, patch, v3
> 
> Review of attachment 8674826 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Could we instead create a new config for emulator-x86 that overrides the
> 'busybox_url' key in 'emulator_automation_config.py'? I sort of suspect
> we'll want separate configs anyway, as there will probably be other things
> that are different as well.

Thanks for the feedback. Overrides the 'busybox_url' sounds good.
But it seems to me that we won't really need to separate the config, because most of config are shared between emulator-x86 and arm. And since emulator-x86 is only enabled on TC, we could generate busybox_url from taskcluster-graph and override the one in 'emulator_automation_config.py' instead. Will attach WIP patch soon.
Bug 1200928 - Part 1: Iterate over all the things in task.extra.locations and build out the test_parameter
Bug 1200928 - Part 2: [Taskcluster] Use correct busybox binary for x86/arm emulator when running xpcshell tests
Attachment #8674826 - Attachment is obsolete: true
Comment on attachment 8677407 [details]
MozReview Request: Bug 1200928 - Part 2: [Taskcluster] Use correct busybox binary for x86/arm emulator when running xpcshell tests

Bug 1200928 - Part 1: Iterate over all the things in task.extra.locations and build out the test_parameter
Comment on attachment 8677408 [details]
MozReview Request: Bug 1200928 - Part 2: [Taskcluster] Use correct busybox binary for x86/arm emulator when running xpcshell tests

Bug 1200928 - Part 2: [Taskcluster] Use correct busybox binary for x86/arm emulator when running xpcshell tests
Comment on attachment 8677407 [details]
MozReview Request: Bug 1200928 - Part 2: [Taskcluster] Use correct busybox binary for x86/arm emulator when running xpcshell tests

Bug 1200928 - Part 1: Iterate over all the things in task.extra.locations and build out the test_parameter
Comment on attachment 8677408 [details]
MozReview Request: Bug 1200928 - Part 2: [Taskcluster] Use correct busybox binary for x86/arm emulator when running xpcshell tests

Bug 1200928 - Part 2: [Taskcluster] Use correct busybox binary for x86/arm emulator when running xpcshell tests
(In reply to Edgar Chen [:edgar][:echen] from comment #9)
> (In reply to Andrew Halberstadt [:ahal] from comment #8)
> > Comment on attachment 8674826 [details] [diff] [review]
> > WIP, patch, v3
> > 
> > Review of attachment 8674826 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Could we instead create a new config for emulator-x86 that overrides the
> > 'busybox_url' key in 'emulator_automation_config.py'? I sort of suspect
> > we'll want separate configs anyway, as there will probably be other things
> > that are different as well.
> 
> Thanks for the feedback. Overrides the 'busybox_url' sounds good.
> But it seems to me that we won't really need to separate the config, because
> most of config are shared between emulator-x86 and arm. And since
> emulator-x86 is only enabled on TC, we could generate busybox_url from
> taskcluster-graph and override the one in 'emulator_automation_config.py'
> instead. Will attach WIP patch soon.

Hi Greg, I would like to see if this changes is okay to you before sending the review request (BTW, I addressed bug 1188330 comment 45 in patch part 1). Thank you.
Flags: needinfo?(garndt)
https://reviewboard.mozilla.org/r/22937/#review22657

::: testing/taskcluster/mach_commands.py:470
(Diff revision 3)
> +            if 'emulator' in build['build_name']:

I think that perhaps maybe this should just be in the task definition for emulators rather than hacking something into the mach target.  Do you think that might work out?
https://reviewboard.mozilla.org/r/22935/#review22655

::: testing/taskcluster/mach_commands.py:464
(Diff revision 3)
> -            if 'tests' in build_task['task']['extra']['locations']:
> +            build_parameters['build_url'] = location_urls['build']

it looks like we are not adding mozharness_url back to the build_parameters unless I'm missing it.

Perhaps were would not be an issue with adding all location urls to the build parameters, and then I dont' think you need to iterate over them lower down where the test tasks are rendered.
Left a couple of comments.
Flags: needinfo?(garndt)
https://reviewboard.mozilla.org/r/22937/#review22657

> I think that perhaps maybe this should just be in the task definition for emulators rather than hacking something into the mach target.  Do you think that might work out?

I will try add them in task definition. Thank you.
Comment on attachment 8677407 [details]
MozReview Request: Bug 1200928 - Part 2: [Taskcluster] Use correct busybox binary for x86/arm emulator when running xpcshell tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22935/diff/3-4/
Comment on attachment 8677408 [details]
MozReview Request: Bug 1200928 - Part 2: [Taskcluster] Use correct busybox binary for x86/arm emulator when running xpcshell tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22937/diff/3-4/
Hi Greg,
I have addressed the comment #17 and comment #18, could you take a look again?
- Add all location urls to the build parameters.
- Put busybox url in build task definition.
- Try result for your reference: https://treeherder.mozilla.org/#/jobs?repo=try&revision=63668400190f&group_state=expanded&exclusion_profile=false

BTW, according to the policy: "Must not rely on resources from sites whose content we do not control/have no SLA" [1], where should we put the busybox binary for x86, tooltool or somewhere in taskcluster? Could you also kindly help me on this or do you know who can help/answer this?

Thank you.

[1] https://wiki.mozilla.org/Sheriffing/Job_Visibility_Policy#Must_avoid_patterns_known_to_cause_non_deterministic_failures
[2] https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/configs/b2g/emulator_automation_config.py#7
Flags: needinfo?(garndt)
Thanks, I don't see anything obviously broken with the try push in comment 23.  Looks like the same ol' stuff we usually see.

You're absolutely right, we should not rely on an external resource like that.  That will almost certainly cause some issues at some point.  

I'm not sure how busybox is used or what versions are required, but if we can pin down a version that works for your needs, it should be put on tooltool just like the other busybox binary and referenced that way.
Flags: needinfo?(garndt)
Attached file busybox-i686 (1.21.1)
(In reply to Greg Arndt [:garndt] from comment #24)
> I'm not sure how busybox is used or what versions are required, but if we
> can pin down a version that works for your needs, it should be put on
> tooltool just like the other busybox binary and referenced that way.

I attach the latest version (1.21.1) which works for us.
I tried to put it on tooltool [1], but it seems I don't have permission to perform upload. Could you help to upload it and share the link to me.

Thanks a again.

[1] https://api.pub.build.mozilla.org/tooltool/
ni for comment #25.
Flags: needinfo?(garndt)
Hrm, I've never uploaded to tooltool either.  Someone in #releng might know what it takes to get it on the pub site.  Let me know if I can help out with that at all.
Flags: needinfo?(garndt)
Hey Jordan, do you mind helping out getting the x86 busybox binary uploaded to tooltool? I'm not sure what the process is these days either.

We'll also need to add it to the mozharness config for the emu-x86 job like here:
https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/configs/b2g/emulator_automation_config.py#7
Flags: needinfo?(jlund)
checking the docs, this is slightly more self serve: https://wiki.mozilla.org/ReleaseEngineering/Applications/Tooltool#How_To_Upload_To_Tooltool

from the above docs, what you need to do is file a bug to get permission to create tokens for uploading to tooltool: https://api.pub.build.mozilla.org/tokenauth/ so you can generate a token. Something like: https://bugzilla.mozilla.org/show_bug.cgi?id=1201730

I've done it this time just to speed it up for you.

You can use the following manifest in your mozharness config so your script can tell the tooltool server what and how to download busybox:

[
{
"size": 883644,
"visibility": "public",
"digest": "227d3189be1eb1e5348255ca100168bae8b3c4d3664384ea3c832fc044d168fd594f0b15991d190eceb1fb9f5b3463912104bcd7d4d0dbdd88d3f4b7187ea73d",
"algorithm": "sha512",
"filename": "busybox-i686"
}
]
Flags: needinfo?(jlund)
Comment on attachment 8677407 [details]
MozReview Request: Bug 1200928 - Part 2: [Taskcluster] Use correct busybox binary for x86/arm emulator when running xpcshell tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22935/diff/4-5/
Attachment #8677407 - Attachment description: MozReview Request: Bug 1200928 - Part 1: Iterate over all the things in task.extra.locations and build out the test_parameter → MozReview Request: Bug 1200928 - Part 2: [Taskcluster] Use correct busybox binary for x86/arm emulator when running xpcshell tests
Attachment #8677408 - Attachment is obsolete: true
Bug 1200928 - Part 1: Iterate over all the things in task.extra.locations and build out the test_parameter
Attachment #8698307 - Flags: review?(garndt)
Comment on attachment 8698307 [details]
MozReview Request: Bug 1200928 - Part 1: Iterate over all the things in task.extra.locations and build out the test_parameter

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27939/diff/1-2/
Comment on attachment 8677407 [details]
MozReview Request: Bug 1200928 - Part 2: [Taskcluster] Use correct busybox binary for x86/arm emulator when running xpcshell tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22935/diff/5-6/
Attachment #8677407 - Flags: review?(garndt)
Comment on attachment 8698307 [details]
MozReview Request: Bug 1200928 - Part 1: Iterate over all the things in task.extra.locations and build out the test_parameter

https://reviewboard.mozilla.org/r/27939/#review25055
Attachment #8698307 - Flags: review?(garndt) → review+
Comment on attachment 8677407 [details]
MozReview Request: Bug 1200928 - Part 2: [Taskcluster] Use correct busybox binary for x86/arm emulator when running xpcshell tests

https://reviewboard.mozilla.org/r/22935/#review25053

Just a couple of comments that need to be addressed.  At a minimum I think having busybox moved under task.extras.locations is ok, and we can remove that special code in the mach target for just the one url.  Also, consider just putting it directly in the one task that needs it defined.  If we have more that will need busybox url we can always change that decision.

::: testing/taskcluster/tasks/builds/b2g_emulator_base.yml:38
(Diff revision 5)
> +    url:

Hrm, is there a reason why this couldn't just be under task.extra.locations.busybox?

::: testing/taskcluster/tasks/tests/b2g_emulator_xpcshell_chunked.yml:23
(Diff revision 5)
> +        --busybox-url {{busybox_url}}

If this is the only place where busybox_url is used in any of the task definitions, why not just put the URL within this definition directly instead of needing special code for handling task.extra.url in the mach target?  Also, see my other comment about maybe placing it with the other URLs if we continue to keep it defined in the build tasks.
Attachment #8677407 - Flags: review?(garndt) → review+
https://reviewboard.mozilla.org/r/22935/#review25053

> Hrm, is there a reason why this couldn't just be under task.extra.locations.busybox?

We iterate all the item under task.extras.locations to consturct real URL with ARTIFACT_URL [1].
If we put busybox under task.extras.locations, we still need a special code to prevent constructing busybox which is already an url.
So I add `url` to put the pure url address.

[1] https://dxr.mozilla.org/mozilla-central/source/testing/taskcluster/mach_commands.py#426

> If this is the only place where busybox_url is used in any of the task definitions, why not just put the URL within this definition directly instead of needing special code for handling task.extra.url in the mach target?  Also, see my other comment about maybe placing it with the other URLs if we continue to keep it defined in the build tasks.

We need to handle `busybox_url` in mach target is because different arch type (arm or x86) needs different busybox binary and they share the same test definition.
Ok, thanks for clarifying.  Someting about it seems off, but not enough that I think this should be blocked.  We can always make it better later.  Thanks.
Thanks for the help, Greg, Andrew and Jordan.
https://hg.mozilla.org/mozilla-central/rev/8284bea565d7
https://hg.mozilla.org/mozilla-central/rev/77ab820ce477
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Duplicate of this bug: 1213725
Duplicate of this bug: 1213724
Moving closed bugs across to new Bugzilla product "TaskCluster".
Component: TaskCluster → Integration
Product: Testing → Taskcluster
Component: Integration → Services
You need to log in before you can comment on or make changes to this bug.