1499352 - Allow the use of wifi and adb over tcp to serve pages to raptor

Status: RESOLVED FIXED

(Testing :: Raptor, defect)

Description

[Bob Clary [:bc] (inactive)]

Raptor on Android runs the webserver and control server on the host machine and makes them available to the browser on the device through the use of adb reverse. adb reverse maps a port on localhost on the device to a port on localhost on the host. The transport is not over the network however but is over the usb connection.

In battery testing we need to be able to run the test with usb disconnected in order to measure the battery drain which means we must use adb tcpip, adb connect and must use wifi instead of usb to communicate with the device. This prevents adb reverse from working.

This patch allows the specification of the --host ipaddress (or hostname if available to the device) as the address where the raptor control server and webserver will be run. It prevents the use of adb reverse when the host is not localhost or 127.0.0.1. If --host is not specified, it defaults to localhost and behaves as before.

It depends on the patch in bug 1499102 and an updated mozdevice 1.1.3. Until that is released you can patch your raptor requirements file testing/raptor/requirements.txt

-mozdevice >= 1.1.3
+-e ../mozbase/mozdevice

remove your <objdir>/testing/raptor-venv before running raptor-test.

example run

./mach raptor-test --test raptor-speedometer --app=geckoview --binary="org.mozilla.geckoview_example" --host 192.168.1.7

You can see an try run without and one with this patch at <https://treeherder.mozilla.org/#/jobs?repo=try&tier=1,2,3&author=[e-mail]&group_state=expanded&fromchange=2f14185e563604496aa560ca641f310afdc0bae6>

This does not use adb tcpip or the new --host parameter but shows that it doesn't break the default usage.

This does not include the plumbing to run this in production.

Comment 1

[Bob Clary [:bc] (inactive)]

Attachment: raptor-remote-host.patch v1

Comment 2

[Robert Wood [:rwood]]

Cool! I'll try this out. A couple of questions - will this only be used for battery tests? What kind of tests will the battery tests be? They won't be standard perf tests correct - in that the actual non-power related numbers won't matter right? As running raptor tests via wifi will introduce different timing etc. I'm not sure how they will effect the actual perf numbers or how consistent perf numbers will be when running over a live network. Although I'm assuming the perf numbers won't matter, it's the power-releated numbers that will matter (?) thanks.

Comment 3

[Bob Clary [:bc] (inactive)]

(In reply to Robert Wood [:rwood] from comment #2)
> will this only be used for battery tests?

I am planning using it there and I'm not planning on anything else at the moment, but wouldn't totally exclude the possibility in the future.

>What kind of tests will the battery tests be?

Not sure yet.

> They won't be standard perf tests correct - in that the actual non-power related numbers
> won't matter right?

I'm planning on dealing with the perf numbers from a benchmark either by removing them and replacing them with the battery numbers or reporting them separately from the normal benchmark so the results are not confused with the normal localhost measurements. It is conceivable that the perf numbers might be of value even over wifi.

As for what the actual pages / test will consist of is something I'm not sure of yet. I'm just planning on making raptor the driver.

> As running raptor tests via wifi will introduce
> different timing etc. I'm not sure how they will effect the actual perf
> numbers or how consistent perf numbers will be when running over a live
> network. Although I'm assuming the perf numbers won't matter, it's the
> power-releated numbers that will matter (?) thanks.

The focus is the battery and power draw for me, but this isn't necessarily restricted to only that use-case. If it made sense we could use the tcpip wifi for some other reason though I don't have one in mind at the moment.

I'm definitely not intending to replace the current perf tests running over adb usb with adb reverse with a adb tcpip, adb connect and a remote wifi solution if that is what concerns you. As you can see, the default behavior is to still use the current approach.

Besides, a special and relatively expensive hub is required to turn off/on the usb ports and we'll only have one.

Comment 4

[Robert Wood [:rwood]]

Comment on attachment 9017487 [details] [diff] [review]
raptor-remote-host.patch v1

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

Very cool, thanks! A few small items/nits, please see inline.

This also opens the door if we want to put the benchmark sources on a local networked machine down the road.

Also applied this patch and ran raptor locally on desktop and android with no --host argument. Ran on desktop using --host'127.0.0.1' just to try out the arg, but didn't try it actually using a real remote IP.

Please add the 'host' argument to the raptor unit test:

https://searchfox.org/mozilla-central/source/testing/raptor/test/test_cmdline.py

r+ with green try, thanks!

::: testing/raptor/raptor/cmdline.py
@@ +21,5 @@
>      add_arg('-b', '--binary', dest='binary',
>              help="path to the browser executable that we are testing")
> +    add_arg('--host', dest='host',
> +            help="Hostname from which to serve urls, defaults to localhost.",
> +            default='localhost')

Please change the default to '127.0.0.1' instead, as there is an issue on MacOS High Sierra with 'localhost' resolving slower than using the IP (Bug 1499352) Thanks!

::: testing/raptor/raptor/gen_test_config.py
@@ +12,5 @@
>  webext_dir = os.path.join(os.path.dirname(here), 'webext', 'raptor')
>  LOG = get_proxy_logger(component="raptor-gen-test-config")
>  
>  
> +def gen_test_config(browser, test, cs_port, host='localhost', b_port=0):

Please change host= to '127.0.0.1' thx

::: testing/raptor/raptor/raptor.py
@@ +70,5 @@
>              path = os.path.join(self.profile_data_dir, name)
>              self.log.info("Merging profile: {}".format(path))
>              self.profile.merge(path)
>  
> +        if self.config['app'] == 'geckoview' and self.host != 'localhost':

!= '127.0.0.1'

@@ +111,5 @@
>          self.control_server = RaptorControlServer(self.results_handler)
>          self.control_server.start()
>  
>          # for android we must make the control server available to the device
> +        if self.config['app'] == "geckoview" and self.host == 'localhost':

== '127.0.0.1'

::: testing/raptor/webext/raptor/runner.js
@@ +396,5 @@
>    testName = config.test_name;
>    settingsURL = config.test_settings_url;
>    csPort = config.cs_port;
>    browserName = config.browser;
> +  host = config.host

missing ;

Comment 5

[Bob Clary [:bc] (inactive)]

Attachment: raptor-remote-host.patch

Quite a few changes. Tested with geckoview on android, firefox and chrome on linux64 with and without --host. Removed the branch command line arguments and added is-release-build to enable the MOZ_DISABLE_NONLOCAL_CONNECTIONS. Supports mitmproxy playback. Still need to think about mozharness configs when running in production.

Comment 6

[Bob Clary [:bc] (inactive)]

Attachment: raptor-taskcluster-is-release.patch

In order to modify the extras for mozilla-beta, mozilla-release builds I added taskgraph.Parameters.is_release()

Comment 7

[Bob Clary [:bc] (inactive)]

Attachment: raptor-remote-host.patch

This looks good except for the stylebench failures in <https://treeherder.mozilla.org/#/jobs?repo=try&tier=1%2C2%2C3&group_state=expanded&selectedJob=210829245&revision=3df5712b979afb75ca24dcad4d544bd938a4fb92>

I'm not sure what is up with that though it is consistent after the patch and not before. I see local crashes when I run it and some of the others which is what appears to be the case here but I don't know why. We cab't land this as is with the sb failure. :-(

rwood: If you could review and perhaps look for why stylebench is broken that would be great.

I ended up putting back the --branch-name for the mozharness since it appears to be used for talos try pushes for some reason.

Comment 8

[Bob Clary [:bc] (inactive)]

doh.

-test_url = http://127.0.0.1:<port>/StyleBench/index.html?raptor
+test_url = http://<host>:<port>/SunSpider/sunspider-1.0.1/sunspider-1.0.1/driver.html?raptor

new patch coming up.

Comment 9

[Bob Clary [:bc] (inactive)]

Attachment: raptor-remote-host.patch

New try run: <https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a14f20f715a0db74fb261d419d2a3eb80d0de76>

One thing about --host and the firefox tests is that it doesn't help much since we run the web servers on the same machine as the test framework but if we wanted to really run them separately this is a good first step. We could factor things so that we could optionally run the webservers separately if desired so that they could be run on a different machine.

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

oh, good find

Comment 11

[Bob Clary [:bc] (inactive)]

Attachment: raptor-remote-host.patch

New try run: <https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a14f20f715a0db74fb261d419d2a3eb80d0de76>

One thing about --host and the firefox tests is that it doesn't help much since we run the web servers on the same machine as the test framework but if we wanted to really run them separately this is a good first step. We could factor things so that we could optionally run the webservers separately if desired so that they could be run on a different machine.

Comment 12

[Robert Wood [:rwood]]

Comment on attachment 9023988 [details] [diff] [review]
raptor-remote-host.patch

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

Thanks :bc. LGTM. I applied the patch locally and ran a bunch of the benchmarks, some on Firefox and some on Chrome, and tp6-1. Also ran speedometer on geckoview. Note: I didn't actually test using a different host, I'm assuming you tried that out with your setup.

r+ with an update for stylebench, and green try

Comment 13

[Robert Wood [:rwood]]

Comment on attachment 9024008 [details] [diff] [review]
raptor-remote-host.patch

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

LGTM, carrying fw the r+

Comment 14

[Bob Clary [:bc] (inactive)]

Attachment: raptor-tests-transform-is-release-build.patch

After discussing it with tomprince and filing bug 1506310, I think this is the better approach and does not require adding a new function to the taskgraph parameters.

Comment 15

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 9024162 [details] [diff] [review]
raptor-tests-transform-is-release-build.patch

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

::: taskcluster/taskgraph/transforms/tests.py
@@ +521,5 @@
>              extra_options.append('try')
>  
> +        if config.params['project'] in ('mozilla-beta', 'mozilla-release') or \
> +           config.params['project'].startswith('mozilla-esr'):
> +            extra_options.append('--is-release-build')

should we consider nightly as well?  I know these are branches outside of trunk and they have different release style preferences/configs.

Comment 16

[Bob Clary [:bc] (inactive)]

I don't think so. The purpose of the MOZ_DISABLE_NONLOCAL_CONNECTIONS setting was for beta (and other release builds) to be able to install an unsigned web extension. The whole hack of setting it to '1' then '0' seems to be the "standard" method of working around this on release builds, but I don't think we want it in general on nightly builds which was the reasoning behind adding this command line argument and not doing the hack automatically. This allows us to turn that hack on and off via the command line rather than baking it into the framework and thereby allowing us to control it explicitly. Otherwise we could have left the hack in by default.

Comment 17

[Joel Maher ( :jmaher ) (UTC -8)]

ok, got it, thanks for putting it all together for me in a single comment :)

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2e5edf37c10
Add --is-release-build to raptor extra_options for beta, release and esr builds, r=jmaher.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7995ad2aa744
[raptor] Allow the use of wifi and adb over tcp to serve pages, r=rwood.

Comment 19

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/b2e5edf37c10
https://hg.mozilla.org/mozilla-central/rev/7995ad2aa744