[telemetry] Removing flake in wpr_server.py caused by static port assignment from chrome/test/funct…

[telemetry] Removing flake in wpr_server.py caused by static port assignment from chrome/test/functional/webpagereplay.py

Also fixes a small temporary_http_server bug where the ports would get passed in the wrong order on cros.

NOTRY=true
BUG=157459
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179144

[hartmanng, 2012-11-23 19:50:51]

tonyg, please take a look after the weekend.

[bulach, 2012-11-26 13:53:46]

https://codereview.chromium.org/11348217/diff/2001/chrome/test/functional/web...
File chrome/test/functional/webpagereplay.py (right):

https://codereview.chromium.org/11348217/diff/2001/chrome/test/functional/web...
chrome/test/functional/webpagereplay.py:22: def GetRandomOpenPort():
drive-by: can we re-use / improve this?
http://src.chromium.org/viewvc/chrome/trunk/src/build/android/pylib/ports.py?...

it has a nicer interface to detect whether or not the port is in use, and keep
track of them..

[nduca, 2012-11-26 17:56:30]

A few things stand out as nasty to me:

1. This port pair magic is really obtuse and hard to read. (x,None), (None, x)
make no sense to me, especially in the desktop_browser_backend where we or them?
Like, ew. The defaulting is not giving us much win, is it?

2. Its strange to have wpr compute the local port... I'd almost say we should
leave wpr alone, move WPR server construction earlier that we call after calling
a common method on the backend to get the wpr target and local ports. And, then
lets just have wpr_server set webpagereplay's global vars directly --- and
assert that there's only one wpr_server instance before it runs perhaps?
Net/net, lets get this all done before we get the browser startup args --- that
way we can set up the command line correctly.

[tonyg, 2012-11-26 19:05:54]

+1 to the other comments raised.

[nduca, 2012-12-11 02:50:11]

glenn, if you have time while you're visiting, it might be good to chat with
tonyg about how to fix this. We're hitting this flake on bots.

[hartmanng, 2012-12-11 04:57:27]

On 2012/12/11 02:50:11, nduca wrote:
> We're hitting this flake on bots.

ugh great...

Sorry I've let this one slide, I'll be sure to talk to tonyg if I have time on
Wednesday before I head home.

[tonyg, 2012-12-11 16:55:14]

https://codereview.chromium.org/11348217/diff/2001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/wpr_server.py (right):

https://codereview.chromium.org/11348217/diff/2001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/wpr_server.py:27: (https_port or
webpagereplay.HTTPS_PORT, webpagereplay.HTTPS_PORT))
Can you explain the motivation for "http_port or". I don't get why we don't
always use the now randomly picked webpagereplay.HTTP_PORT.

[hartmanng, 2012-12-11 17:14:26]

https://codereview.chromium.org/11348217/diff/2001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/wpr_server.py (right):

https://codereview.chromium.org/11348217/diff/2001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/wpr_server.py:27: (https_port or
webpagereplay.HTTPS_PORT, webpagereplay.HTTPS_PORT))
On 2012/12/11 16:55:14, tonyg wrote:
> Can you explain the motivation for "http_port or". I don't get why we don't
> always use the now randomly picked webpagereplay.HTTP_PORT.

The webpagereplay.HTTP_PORT is chosen to be an open local port. If we're doing a
test on cros, it isn't guaranteed that webpagereplay.HTTP_PORT is also free on
the remote device. So what I was doing in this patch is getting separate ports
for the device-side of the connection in cros_browser_backend.py and passing
them in to override webpagereplay.HTTP_PORT.

[tonyg, 2012-12-20 21:20:59]

I'm having trouble seeing a way to clean this patch up. Nat, do you have any
ideas?

[nduca, 2012-12-21 00:52:42]

I think we need cleanup patch or two here, or at least as part of this patch to
do some cleanup. Up to you how...

1) Make a PortPair object that has a local_port and remote_port field. Use that
instead of these crazy ((a, b), (c, d)) stuff. Thats really hard to read.
Emphatically.

2) Stop using these http_port = None idiom. Require that it be passed in
everywhere. That makes it hard to read.

3) Dont ever say http_port. Explicitly state local or remote. So, for example,
in cros_brower_backend, that variable really should be remote_http_port and
remote_https_port

4) This might be a patch on its own. Make a fuction in webpagereply.py saying
GetChromeFlags(http_port, https_port). Get rid of all the global vars about port
and make it always be passed in. Get rid of all refernces ot "git grep
CHROME_FLAGS". Make it right.

Glenn, wdyt about this set of steps?

[hartmanng, 2013-01-02 18:40:15]

On 2012/12/21 00:52:42, nduca wrote:
> I think we need cleanup patch or two here, or at least as part of this patch
to
> do some cleanup. Up to you how...
> 
> 1) Make a PortPair object that has a local_port and remote_port field. Use
that
> instead of these crazy ((a, b), (c, d)) stuff. Thats really hard to read.
> Emphatically.
> 
> 2) Stop using these http_port = None idiom. Require that it be passed in
> everywhere. That makes it hard to read.
> 
> 3) Dont ever say http_port. Explicitly state local or remote. So, for example,
> in cros_brower_backend, that variable really should be remote_http_port and
> remote_https_port
> 
> 4) This might be a patch on its own. Make a fuction in webpagereply.py saying
> GetChromeFlags(http_port, https_port). Get rid of all the global vars about
port
> and make it always be passed in. Get rid of all refernces ot "git grep
> CHROME_FLAGS". Make it right.
> 
> Glenn, wdyt about this set of steps?

Sounds good, I'll work on a cleanup patch now. To clarify part 2, do you mean
functions shouldn't have a arguments with default value = None, or we should
never pass in a value of None for a port? I think the reason we did that was
that the cros interface isn't accessible from certain places, but that might be
refactorable.

[tonyg, 2013-01-02 22:47:32]

On 2013/01/02 18:40:15, hartmanng wrote:
> On 2012/12/21 00:52:42, nduca wrote:
> > I think we need cleanup patch or two here, or at least as part of this patch
> to
> > do some cleanup. Up to you how...
> > 
> > 1) Make a PortPair object that has a local_port and remote_port field. Use
> that
> > instead of these crazy ((a, b), (c, d)) stuff. Thats really hard to read.
> > Emphatically.
> > 
> > 2) Stop using these http_port = None idiom. Require that it be passed in
> > everywhere. That makes it hard to read.
> > 
> > 3) Dont ever say http_port. Explicitly state local or remote. So, for
example,
> > in cros_brower_backend, that variable really should be remote_http_port and
> > remote_https_port
> > 
> > 4) This might be a patch on its own. Make a fuction in webpagereply.py
saying
> > GetChromeFlags(http_port, https_port). Get rid of all the global vars about
> port
> > and make it always be passed in. Get rid of all refernces ot "git grep
> > CHROME_FLAGS". Make it right.
> > 
> > Glenn, wdyt about this set of steps?
> 
> Sounds good, I'll work on a cleanup patch now. To clarify part 2, do you mean
> functions shouldn't have a arguments with default value = None, or we should
> never pass in a value of None for a port? I think the reason we did that was
> that the cros interface isn't accessible from certain places, but that might
be
> refactorable.

AIUI, the intent was not to pass None at all. The behavior there is sneaky so it
is hard to tell what is going on at the callsites.

[nduca, 2013-01-03 20:09:34]

+1 to what tonyg said

[hartmanng, 2013-01-07 14:46:24]

Now that https://codereview.chromium.org/11740020/ and
https://codereview.chromium.org/11744007/ have landed, this patch is ready for
another try.

I've removed all the 'http_port = None' stuff, this now requires that explicit
{local,remote}_http[s]_ports are passed into the wpr_server and forwarders every
time.

PTAL

[tonyg, 2013-01-17 00:06:52]

lgtm

[hartmanng, 2013-01-17 14:27:58]

Thanks tony!

[commit-bot: I haz the power, 2013-01-17 14:28:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hartmanng@chromium.org/11348217/24001

[commit-bot: I haz the power, 2013-01-17 14:28:27]

Failed to apply patch for tools/telemetry/telemetry/browser.py:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file tools/telemetry/telemetry/browser.py
  Hunk #1 FAILED at 122.
  1 out of 1 hunk FAILED -- saving rejects to file
tools/telemetry/telemetry/browser.py.rej

Patch:       tools/telemetry/telemetry/browser.py
Index: tools/telemetry/telemetry/browser.py
diff --git a/tools/telemetry/telemetry/browser.py
b/tools/telemetry/telemetry/browser.py
index
62e31c37b7801c80e65ad2f10c73f8f7a7bbb11a..ed6b078ed8b3aae3bf9ff54c9b834e9ad777de41
100644
--- a/tools/telemetry/telemetry/browser.py
+++ b/tools/telemetry/telemetry/browser.py
@@ -122,8 +122,10 @@ class Browser(object):
         archive_path,
         use_record_mode,
         self._backend.WEBPAGEREPLAY_HOST,
-        self._backend.WEBPAGEREPLAY_HTTP_PORT,
-        self._backend.WEBPAGEREPLAY_HTTPS_PORT)
+        self._backend.webpagereplay_local_http_port,
+        self._backend.webpagereplay_local_https_port,
+        self._backend.webpagereplay_remote_http_port,
+        self._backend.webpagereplay_remote_https_port)
 
   def GetStandardOutput(self):
     return self._backend.GetStandardOutput()

[commit-bot: I haz the power, 2013-01-17 15:14:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hartmanng@chromium.org/11348217/26002

[hartmanng, 2013-01-17 19:06:33]

In my previous patch android could fail sometimes with the wpr_server because of
the way that both the desktop browser and the android browser use pylib.ports
(can sometimes end up assigning the same port twice).

tonyg, please take a quick look at this very small fix, just to make sure there
isn't anything I'm unaware of regarding the placement of the
ResetTestServerPortAllocation() call in android_browser_backend.

Thanks!

[bulach, 2013-01-17 19:24:53]

quick drive by below:

https://chromiumcodereview.appspot.com/11348217/diff/30015/tools/telemetry/te...
File tools/telemetry/telemetry/android_browser_backend.py (left):

https://chromiumcodereview.appspot.com/11348217/diff/30015/tools/telemetry/te...
tools/telemetry/telemetry/android_browser_backend.py:30:
adb_commands.ResetTestServerPortAllocation()
drive-by: when we're sharding, we run a single host with multiple android
connected devices... it essentially means running multiple telemetry harnesses
via python's "multiprocess" package.
the "sharder" then takes responsibility for resetting this ports once all
sub-processes are done.

however, in non-sharded usage, it's nice to Reset this allocation, otherwise it
may not be able to allocate a test server port below...

[hartmanng, 2013-01-17 19:29:14]

https://chromiumcodereview.appspot.com/11348217/diff/30015/tools/telemetry/te...
File tools/telemetry/telemetry/android_browser_backend.py (left):

https://chromiumcodereview.appspot.com/11348217/diff/30015/tools/telemetry/te...
tools/telemetry/telemetry/android_browser_backend.py:30:
adb_commands.ResetTestServerPortAllocation()
On 2013/01/17 19:24:53, bulach wrote:
> drive-by: when we're sharding, we run a single host with multiple android
> connected devices... it essentially means running multiple telemetry harnesses
> via python's "multiprocess" package.
> the "sharder" then takes responsibility for resetting this ports once all
> sub-processes are done.
> 
> however, in non-sharded usage, it's nice to Reset this allocation, otherwise
it
> may not be able to allocate a test server port below...

I didn't remove it entirely though, I moved it to the superclass's
(browser_backend.py) __init__, so now it should get called for all browser types
instead of just Android.

I assume keep_test_server_ports is true when sharding to prevent conflicts
there?

[bulach, 2013-01-17 19:33:22]

ahn, right.. :) sorry, I didn't see the other file, my bad..
yes, build/android/bb_run_sharded_steps.py takes of appending 
--keep_test_server_ports..

[hartmanng, 2013-01-23 16:30:15]

Nat doesn't like using pylib for non-android ports (see
https://codereview.chromium.org/12052024/#msg2). I didn't realize pylib was
supposed to be android-only, but now that I know, I've switched things over to
using util.GetAvailableLocalPort().

PTAL, tonyg

[tonyg, 2013-01-25 00:11:20]

lgtm

[commit-bot: I haz the power, 2013-01-28 14:29:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hartmanng@chromium.org/11348217/47014

[commit-bot: I haz the power, 2013-01-28 14:29:28]

Change committed as 179144