[Telemetry] Make rndis work on L release

[Telemetry] Make rndis work on L release

- modified android fowarder to override any default device routing
  policy (eg static route to wlan) that could be keeping traffic from
  going to rndis interface
- invoke netd commands directly instead of through ndc which no longer
  work in L

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290923

[wuhu, 2014-07-18 17:27:19]

Please review the changes to android forwarder, thanks

[tonyg, 2014-07-30 18:14:14]

Sorry for the delay. First question: does this still work on J and K?

[wuhu, 2014-07-30 19:14:52]

On 2014/07/30 18:14:14, tonyg wrote:
> Sorry for the delay. First question: does this still work on J and K?

Yes, I've tested it on Nexus 5 for K and Nexus 7 for J

[tonyg, 2014-07-30 19:32:08]

Thanks for confirming that. Here're some more detailed comments then I think
this should be ready to go.

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

https://codereview.chromium.org/404803003/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/forwarders/android_forwarder.py:125: dnschange =
self._adb.device().GetProp('net.dnschange')
I thought these were already landed. Is your client out of date?

https://codereview.chromium.org/404803003/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/forwarders/android_forwarder.py:130:
#self._adb.RunShellCommand('ndc netd resolver setifdns %s %s %s' %
We should remove these lines instead of commenting them out.

https://codereview.chromium.org/404803003/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/forwarders/android_forwarder.py:150: if
(len(policies) > 1 and re.match("^1:.*lookup main", policies[1])) :
nit: lose the outer parens and no space before :

https://codereview.chromium.org/404803003/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/forwarders/android_forwarder.py:445: def
_OverrideRoutingPolicy(self):
Something seems wrong in the fact that one class overrides and the other
restores. Is there a more robust way to structure this?

https://codereview.chromium.org/404803003/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/forwarders/android_forwarder.py:450: if
(len(policies) > 1 and not ("lookup main" in policies[1])) :
Style nit: prefer ' to "

[wuhu, 2014-07-30 21:37:24]

Thanks Tony.  Please see my question regarding override/restore in the comments

[wuhu, 2014-07-30 21:38:28]

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

https://codereview.chromium.org/404803003/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/forwarders/android_forwarder.py:445: def
_OverrideRoutingPolicy(self):
On 2014/07/30 19:32:08, tonyg wrote:
> Something seems wrong in the fact that one class overrides and the other
> restores. Is there a more robust way to structure this?

I agree this is not the most ideal.  The override is needed for
_TestConnectivity to work in AndroidForwarderConfigurator which is only run at
start up.  The only cleanup function related to forwarding is in
AndroidRndisForwarder.  

I guess there are several possible solutions:

1. move the routing table override/restore to the browser backend layer whose
start()/close() wraps around forwarder lifecycle

2. add a "unconfig" method to AndroidForwarderConfigurator.  Store a reference
to configurator in AndroidRndisForwarder so the "unconfig" method can be called
in AndroidRndisForwarder's close()

3. swap order of AndroidRndisForwarder/AndroidRndisForwarder creation (wide code
impact)

Any suggestions?

[wuhu, 2014-08-07 18:44:23]

The CQ bit was checked by wuhu@google.com

[wuhu, 2014-08-07 18:44:26]

The CQ bit was unchecked by wuhu@google.com

[chrishenry - DO NOT USE, 2014-08-13 05:27:30]

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

https://codereview.chromium.org/404803003/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/forwarders/android_forwarder.py:445: def
_OverrideRoutingPolicy(self):
On 2014/07/30 21:38:28, wuhu wrote:
> On 2014/07/30 19:32:08, tonyg wrote:
> > Something seems wrong in the fact that one class overrides and the other
> > restores. Is there a more robust way to structure this?
> 
> I agree this is not the most ideal.  The override is needed for
> _TestConnectivity to work in AndroidForwarderConfigurator which is only run at
> start up.  The only cleanup function related to forwarding is in
> AndroidRndisForwarder.  
> 
> I guess there are several possible solutions:
> 
> 1. move the routing table override/restore to the browser backend layer whose
> start()/close() wraps around forwarder lifecycle
> 
> 2. add a "unconfig" method to AndroidForwarderConfigurator.  Store a reference
> to configurator in AndroidRndisForwarder so the "unconfig" method can be
called
> in AndroidRndisForwarder's close()
> 
> 3. swap order of AndroidRndisForwarder/AndroidRndisForwarder creation (wide
code
> impact)
> 
> Any suggestions?

So these overrides are necessary for RNDIS to work? Does it affect routing in
any way when we don't create a forwarder at all?

1) If the answer to the second question is yes:, we should tear down the routing
policy in configurator. And then whenever we create a new forwarder, we re-add
the config. (As you may now notice, we create > 1 instance of forwarder, one
each for each wpr_server.ReplayServer instance, which we re-create whenever we
change the replay archive path.)

2) If not, we can tear down with the lifecycle of the configurator instead.

Note that we create multiple instances of the forwarder, so the current way to
tear down the rule in forwarder doesn't look right to me.

[wuhu, 2014-08-18 22:29:08]

Sounds good, I've moved everything to AndroidRndisForwarder so it can be
torn-down/recreated each time just like the DNS override currently in
AndroidRndisForwarder

Thanks

[tonyg, 2014-08-20 17:58:20]

The CQ bit was checked by tonyg@chromium.org

[tonyg, 2014-08-20 17:58:21]

lgtm

[commit-bot: I haz the power, 2014-08-20 18:00:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wuhu@google.com/404803003/20001

[commit-bot: I haz the power, 2014-08-20 19:44:10]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-20 21:44:52]

Committed patchset #2 (20001) as 290923

[marja, 2014-08-27 11:08:19]

marja@chromium.org changed reviewers:
+ marja@chromium.org

[marja, 2014-08-27 11:08:19]

https://codereview.chromium.org/404803003/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/forwarders/android_forwarder.py (right):

https://codereview.chromium.org/404803003/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/forwarders/android_forwarder.py:153: policies =
self._device.RunShellCommand('ip rule')
This seems pretty broken: self._device does not exist (note that
AndroidForwarder has a _device, but AndroidRndisForwarder doesn't inherit from
it), so this fails immediately when we hit this function!

Patch set 1 is fine in this regard - this code is in AndroidRndisConfigurator
which has a _device.

What I gather from this is that:
1) the code in patch set 2 was never ran before committing
2) we have no tests which run with netsim on Android.

[marja, 2014-08-27 11:16:46]

Fix here: https://codereview.chromium.org/508923002/