[Telemetry] Properly chose an available device port.

[Telemetry] Properly chose an available device port.

Previously, we'd start WPR on an available host port and then just
assume that the same port was free on the device -- oops.

Now, we pass 0 to the forwarder for the device port so that it chooses
an available port, and then read back the port that it chose to use
for the mapping.

I believe this fixes a major case of device forwarder flake.

BUG=421599
TBR=slamm@chromium.org

Committed: https://crrev.com/6aa01dbdfbce3a03522f8bc0b59916529de54a93
Cr-Commit-Position: refs/heads/master@{#299319}

[tonyg, 2014-10-09 00:17:00]

tonyg@chromium.org changed reviewers:
+ dtu@chromium.org, vmiura@chromium.org

[tonyg, 2014-10-09 01:03:27]

tonyg@chromium.org changed reviewers:
+ slamm@chromium.org

[slamm, 2014-10-09 16:59:59]

Telemetry's handling of local and remote ports needs to be rethought.
This change shows how it is challenging to work with the existing arrangement.

It is surprising how the backend port pairs are overwritten in wpr_server and
then used by the browser backend to set the --fixed-port arg.

The change sets the remote ports using the forwarder after WPR has started.
However, CrOs sets the remote ports during its constructor using
self.GetRemotePort().

I have been noodling over some solutions, however, I do not have it fleshed out
yet.

https://codereview.chromium.org/640133002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/forwarders/android_forwarder.py (right):

https://codereview.chromium.org/640133002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/forwarders/android_forwarder.py:62:
forwarder.Forwarder.DevicePortForHostPort(p.local_port))
In the netsim case, the remote ports should be 80, 443, 53 and not dynamically
assigned ports.

https://codereview.chromium.org/640133002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/wpr_server.py (right):

https://codereview.chromium.org/640133002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/wpr_server.py:74: remote_https_port =
wpr_port_pairs.https.remote_port
Desktop uses the local ports.

[slamm, 2014-10-09 21:47:43]

For Android, could you always use 80 and 443 on the device? Those should not be
used by anything else. Would the same apply to CrOS?

[tonyg, 2014-10-11 00:45:34]

On 2014/10/09 16:59:59, slamm wrote:
> Telemetry's handling of local and remote ports needs to be rethought.
> This change shows how it is challenging to work with the existing arrangement.
> 
> It is surprising how the backend port pairs are overwritten in wpr_server and
> then used by the browser backend to set the --fixed-port arg.


+1000

Given the prominence of the flake, the amount of work it'll take to untangle
this, and the fact that this doesn't really make things worse, are you okay with
fixing this for now and we'll continue trying to make this sane?

[tonyg, 2014-10-11 00:45:42]

https://codereview.chromium.org/640133002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/forwarders/android_forwarder.py (right):

https://codereview.chromium.org/640133002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/forwarders/android_forwarder.py:62:
forwarder.Forwarder.DevicePortForHostPort(p.local_port))
On 2014/10/09 16:59:59, slamm wrote:
> In the netsim case, the remote ports should be 80, 443, 53 and not dynamically
> assigned ports.

This just reads back the port used on the device. It's still 80, 443, 53 if
requested in the Map() command. This only has an effect when 0 is passed as the
port.

https://codereview.chromium.org/640133002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/wpr_server.py (right):

https://codereview.chromium.org/640133002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/wpr_server.py:74: remote_https_port =
wpr_port_pairs.https.remote_port
On 2014/10/09 16:59:59, slamm wrote:
> Desktop uses the local ports.

Good catch. Fixed.

[slamm_google, 2014-10-13 16:42:54]

On 2014/10/11 00:45:42, tonyg wrote:
>
https://codereview.chromium.org/640133002/diff/1/tools/telemetry/telemetry/co...
> File tools/telemetry/telemetry/core/forwarders/android_forwarder.py (right):
> 
>
https://codereview.chromium.org/640133002/diff/1/tools/telemetry/telemetry/co...
> tools/telemetry/telemetry/core/forwarders/android_forwarder.py:62:
> forwarder.Forwarder.DevicePortForHostPort(p.local_port))
> On 2014/10/09 16:59:59, slamm wrote:
> > In the netsim case, the remote ports should be 80, 443, 53 and not
dynamically
> > assigned ports.
> 
> This just reads back the port used on the device. It's still 80, 443, 53 if
> requested in the Map() command. This only has an effect when 0 is passed as
the
> port.
> 
>
https://codereview.chromium.org/640133002/diff/1/tools/telemetry/telemetry/co...
> File tools/telemetry/telemetry/core/wpr_server.py (right):
> 
>
https://codereview.chromium.org/640133002/diff/1/tools/telemetry/telemetry/co...
> tools/telemetry/telemetry/core/wpr_server.py:74: remote_https_port =
> wpr_port_pairs.https.remote_port
> On 2014/10/09 16:59:59, slamm wrote:
> > Desktop uses the local ports.
> 
> Good catch. Fixed.

LGTM. I am still curious if android_browser_backend (and CrOS) could always use
80, 443, & 53 for the device ports.

[tonyg, 2014-10-13 17:18:33]

On 2014/10/13 16:42:54, slamm_google wrote:
> On 2014/10/11 00:45:42, tonyg wrote:
> >
>
https://codereview.chromium.org/640133002/diff/1/tools/telemetry/telemetry/co...
> > File tools/telemetry/telemetry/core/forwarders/android_forwarder.py (right):
> > 
> >
>
https://codereview.chromium.org/640133002/diff/1/tools/telemetry/telemetry/co...
> > tools/telemetry/telemetry/core/forwarders/android_forwarder.py:62:
> > forwarder.Forwarder.DevicePortForHostPort(p.local_port))
> > On 2014/10/09 16:59:59, slamm wrote:
> > > In the netsim case, the remote ports should be 80, 443, 53 and not
> dynamically
> > > assigned ports.
> > 
> > This just reads back the port used on the device. It's still 80, 443, 53 if
> > requested in the Map() command. This only has an effect when 0 is passed as
> the
> > port.
> > 
> >
>
https://codereview.chromium.org/640133002/diff/1/tools/telemetry/telemetry/co...
> > File tools/telemetry/telemetry/core/wpr_server.py (right):
> > 
> >
>
https://codereview.chromium.org/640133002/diff/1/tools/telemetry/telemetry/co...
> > tools/telemetry/telemetry/core/wpr_server.py:74: remote_https_port =
> > wpr_port_pairs.https.remote_port
> > On 2014/10/09 16:59:59, slamm wrote:
> > > Desktop uses the local ports.
> > 
> > Good catch. Fixed.
> 
> LGTM. I am still curious if android_browser_backend (and CrOS) could always
use
> 80, 443, & 53 for the device ports.

That requires a rooted device and we'd like to degrade as gracefully as possible
on non-rooted devices.

[tonyg, 2014-10-13 17:18:37]

The CQ bit was checked by tonyg@chromium.org

[commit-bot: I haz the power, 2014-10-13 17:19:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/640133002/530001

[commit-bot: I haz the power, 2014-10-13 17:19:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-13 17:19:49]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[tonyg, 2014-10-13 17:21:27]

The CQ bit was checked by tonyg@chromium.org

[commit-bot: I haz the power, 2014-10-13 17:22:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/640133002/530001

[commit-bot: I haz the power, 2014-10-13 17:48:19]

Committed patchset #4 (id:530001)

[commit-bot: I haz the power, 2014-10-13 17:48:49]

Patchset 4 (id:??) landed as
https://crrev.com/6aa01dbdfbce3a03522f8bc0b59916529de54a93
Cr-Commit-Position: refs/heads/master@{#299319}