Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22286010:
[telemetry] Implement Forwarder using RNDIS. (Closed)
|
Created:
7 years, 4 months ago by szym Modified:
7 years, 3 months ago CC:
chromium-reviews, chrome-speed-team+watch_google.com, telemetry+watch_chromium.org, pauljensen, Philippe, frankf, craigdh, navabi1 Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[telemetry] Implement Forwarder using RNDIS.
Introduces new platform flags:
--android-rndis
--no-android-rndis [default]
When enabled, the backend will configure the RNDIS interface.
Otherwise, the user-space forwarder will be used.
Note, RNDIS requires root. Run adb root.
RNDIS also requires static configuration for usbX network interfaces
in /etc/network/interfaces
BUG=272381
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221047
Patch Set 1 : . #Patch Set 2 : use different networks to avoid the need for rule-based routing with multiple devices #
Total comments: 43
Patch Set 3 : sync #Patch Set 4 : respond to review #Patch Set 5 : move TODO #
Total comments: 1
Patch Set 6 : sync #Patch Set 7 : rely on /etc/network/interfaces to configure host_iface #Patch Set 8 : Exclude dev_iface when looking for address conflicts #
Total comments: 1
Patch Set 9 : fixup #Patch Set 10 : comments #Patch Set 11 : sync #
Total comments: 1
Patch Set 12 : hacky way to disable dns_forwarding #
Total comments: 9
Patch Set 13 : Replace sys.stderr with logging. Fallback to forwarder on any failure. Simplify. #Patch Set 14 : add message to assertion #Patch Set 15 : comment language #Patch Set 16 : add --android_rndis flag #
Total comments: 15
Patch Set 17 : sync #Patch Set 18 : split off android_rndis.py #
Total comments: 2
Patch Set 19 : "cache" _rndis_forwarder #Patch Set 20 : Add _CheckOutput which works on python2.6 #Patch Set 21 : simpler darwin check #
Total comments: 2
Patch Set 22 : sync, add --[no-]android-rndis flag #Patch Set 23 : fix flags #Patch Set 24 : address nits #Patch Set 25 : sync fixup #Messages
Total messages: 29 (0 generated)
This depends on https://codereview.chromium.org/23000006 tonyg, nduca: PTAL. pauljensen: FYI.
probably best to route to bulach and tonyg. Ill just slow you down. :)
This looks really awesome. My comments are mostly nits. bulach@ should definitely review too. https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/chrome/adb_commands.py (right): https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:188: RNDIS_DEVICE = '/sys/class/android_usb/android0' _RNDIS_DEVICE https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:196: assert adb.IsRootEnabled() We should probably put an error message here, e.g assert adb.IsRootEnabled(), 'Root must be enabled to run RNDIS forwarding' https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:197: assert all(pair.remote_port == pair.local_port for pair in port_pairs) Ditto on adding a message explaining this assertion https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:198: self._adb = adb.Adb() Can't we reuse the passed in adb instead of creating a new one? https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:201: self._host_ip = self._device_ip = None Let's do one assignment per line for readability. https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:206: raise Exception('Device does not have rndis') Can we make this exception user-friendly. In particular, tell them what to do to get around this failure. Same applies for other Exceptions throughout. https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:213: self._adb.WaitForDevicePm() Seems like we should just inline this, no? https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:218: 'ls {0}/f_rndis/device'.format(self.RNDIS_DEVICE)) I'm used to the syntax of 'ls %s/f_rndis/device' % self._RNDIS_DEVICE. Does format do something different? If not, I suggest we use % throughout. https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:219: assert result assert result, 'Failed to ls {0}/f_rndis/device' https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:232: # TODO: on MacOS call 'ifconfig' instead TODO(szym): ... Also, this makes me wonder whether we should have an assertion somewhere that the host is linux. https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:285: # TODO(szym) run via su -c if necessary. Add : https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:311: interface_list = subprocess.check_output(['ip', 'addr']).splitlines() Can this be factored out to be shared with the same above? https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:335: # TODO(szym): this method is prone to race conditions and doesn't fail fast. I don't understand this TODO, can you explain? https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:336: def _ip2long(addr): These should all use CamelCase. https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:370: subprocess.check_call(['sudo', 'ifconfig', host_iface, host_ip, Will this work with sudo in it? I'm not sure we can assume sudo on the host for the bots. https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/chrome/android_browser_backend.py (right): https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/android_browser_backend.py:312: if self._root_enabled: Maybe we should output a warning message when the rndis forwarder is not used.
some general comments below: https://chromiumcodereview.appspot.com/22286010/diff/27001/tools/telemetry/te... File tools/telemetry/telemetry/core/chrome/adb_commands.py (right): https://chromiumcodereview.appspot.com/22286010/diff/27001/tools/telemetry/te... tools/telemetry/telemetry/core/chrome/adb_commands.py:217: result = self._adb.RunShellCommand( _adb.Adb().FileExistsOnDevice(..) https://chromiumcodereview.appspot.com/22286010/diff/27001/tools/telemetry/te... tools/telemetry/telemetry/core/chrome/adb_commands.py:234: ether_address = self._adb.RunShellCommand( _adb.Adb().GetFileContents(...) https://chromiumcodereview.appspot.com/22286010/diff/27001/tools/telemetry/te... tools/telemetry/telemetry/core/chrome/adb_commands.py:288: result = self._adb.RunShellCommand('cat {0}.log'.format(script_prefix)) _adb.Adb().GetFileContents() https://chromiumcodereview.appspot.com/22286010/diff/27001/tools/telemetry/te... tools/telemetry/telemetry/core/chrome/adb_commands.py:413: self._adb.RunShellCommand('setprop net.dns2 ' + dns2) fyi, we had in the past quite a few problems with overriding the dns, it was extremely flaky... see long discussion at b/6237355
Thanks for the review. Here's some partial response, before I get to act on the rest. https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/chrome/adb_commands.py (right): https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:198: self._adb = adb.Adb() On 2013/08/15 16:39:11, tonyg wrote: > Can't we reuse the passed in adb instead of creating a new one? We are reusing it. AdbCommands.Adb() is a getter for the wrapped instance of android_commands.AndroidCommands https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:213: self._adb.WaitForDevicePm() On 2013/08/15 16:39:11, tonyg wrote: > Seems like we should just inline this, no? So, I'd really like this to just call "adb wait-for-device" instead of waiting for boot or the pm. I thought AndroidCommands did not expose anything to facilitate that, but now I see I could just: self._adb.Adb().SendCommand('wait-for-device') Would you still prefer this inlined? https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:370: subprocess.check_call(['sudo', 'ifconfig', host_iface, host_ip, On 2013/08/15 16:39:11, tonyg wrote: > Will this work with sudo in it? I'm not sure we can assume sudo on the host for > the bots. Ok, so this is another caveat. We don't need sudo for this ifconfig, if there's only one RNDIS device, because the usbnet driver will automatically configure the first host usbX interface. This scenario is likely for developer workstations, but not for the bots. Furthermore, if we want DNS latency emulation, then we need --dns_forwarding, and in that case WPR needs sudo as well. Will replacing this with "if not root, run with sudo" (as WPR does) work? https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:413: self._adb.RunShellCommand('setprop net.dns2 ' + dns2) On 2013/08/15 17:26:05, bulach wrote: > fyi, we had in the past quite a few problems with overriding the dns, it was > extremely flaky... see long discussion at b/6237355 Since the commit mentioned below, this codepath seems obsolete anyway. net.dnsX is still set for "legacy libraries", but bionic's getaddrinfo uses explicit configuration which can only be changed via direct command to netd.
https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/chrome/adb_commands.py (right): https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:188: RNDIS_DEVICE = '/sys/class/android_usb/android0' On 2013/08/15 16:39:11, tonyg wrote: > _RNDIS_DEVICE Done. https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:196: assert adb.IsRootEnabled() On 2013/08/15 16:39:11, tonyg wrote: > We should probably put an error message here, e.g > > assert adb.IsRootEnabled(), 'Root must be enabled to run RNDIS forwarding' Done. https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:197: assert all(pair.remote_port == pair.local_port for pair in port_pairs) On 2013/08/15 16:39:11, tonyg wrote: > Ditto on adding a message explaining this assertion Done. https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:201: self._host_ip = self._device_ip = None On 2013/08/15 16:39:11, tonyg wrote: > Let's do one assignment per line for readability. Done. https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:206: raise Exception('Device does not have rndis') On 2013/08/15 16:39:11, tonyg wrote: > Can we make this exception user-friendly. In particular, tell them what to do to > get around this failure. There's no real workaround other than "reboot the phone and don't adb root". This makes me think that the backend should have a flag allowing and override. > Same applies for other Exceptions throughout. https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:217: result = self._adb.RunShellCommand( On 2013/08/15 17:26:05, bulach wrote: > _adb.Adb().FileExistsOnDevice(..) Done. https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:218: 'ls {0}/f_rndis/device'.format(self.RNDIS_DEVICE)) On 2013/08/15 16:39:11, tonyg wrote: > I'm used to the syntax of 'ls %s/f_rndis/device' % self._RNDIS_DEVICE. Does > format do something different? If not, I suggest we use % throughout. Done, although 2.7 officially deprecates this syntax. https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:234: ether_address = self._adb.RunShellCommand( On 2013/08/15 17:26:05, bulach wrote: > _adb.Adb().GetFileContents(...) Done. https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:285: # TODO(szym) run via su -c if necessary. On 2013/08/15 16:39:11, tonyg wrote: > Add : Done. https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:288: result = self._adb.RunShellCommand('cat {0}.log'.format(script_prefix)) On 2013/08/15 17:26:05, bulach wrote: > _adb.Adb().GetFileContents() Done. https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:311: interface_list = subprocess.check_output(['ip', 'addr']).splitlines() On 2013/08/15 16:39:11, tonyg wrote: > Can this be factored out to be shared with the same above? Done. https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:335: # TODO(szym): this method is prone to race conditions and doesn't fail fast. On 2013/08/15 16:39:11, tonyg wrote: > I don't understand this TODO, can you explain? I imagine that if two instances of this method run concurrently on the same host, then we might end up with a collision. The problem is that we won't discover this problem until _TestConnectivity. https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:336: def _ip2long(addr): On 2013/08/15 16:39:11, tonyg wrote: > These should all use CamelCase. Done. https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/chrome/android_browser_backend.py (right): https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/android_browser_backend.py:312: if self._root_enabled: On 2013/08/15 16:39:11, tonyg wrote: > Maybe we should output a warning message when the rndis forwarder is not used. Done.
https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/chrome/adb_commands.py (right): https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:213: self._adb.WaitForDevicePm() On 2013/08/15 19:38:39, szym wrote: > On 2013/08/15 16:39:11, tonyg wrote: > > Seems like we should just inline this, no? > > So, I'd really like this to just call "adb wait-for-device" instead of waiting > for boot or the pm. I thought AndroidCommands did not expose anything to > facilitate that, but now I see I could just: > self._adb.Adb().SendCommand('wait-for-device') > sgtm > Would you still prefer this inlined? No, we shouldn't repeat the string https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:218: 'ls {0}/f_rndis/device'.format(self.RNDIS_DEVICE)) On 2013/08/15 22:59:43, szym wrote: > On 2013/08/15 16:39:11, tonyg wrote: > > I'm used to the syntax of 'ls %s/f_rndis/device' % self._RNDIS_DEVICE. Does > > format do something different? If not, I suggest we use % throughout. > > Done, although 2.7 officially deprecates this syntax. Oh, I had no idea it was deprecated. Anyway, all of Telemetry uses it so probably best to be consistent. https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:335: # TODO(szym): this method is prone to race conditions and doesn't fail fast. On 2013/08/15 22:59:43, szym wrote: > On 2013/08/15 16:39:11, tonyg wrote: > > I don't understand this TODO, can you explain? > > I imagine that if two instances of this method run concurrently on the same > host, then we might end up with a collision. The problem is that we won't > discover this problem until _TestConnectivity. Oh, I follow now! Because of the placement, I thought this comment applied to _ip2long instead of _ConfigureNetwork. Maybe consider moving it? https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:370: subprocess.check_call(['sudo', 'ifconfig', host_iface, host_ip, On 2013/08/15 19:38:39, szym wrote: > On 2013/08/15 16:39:11, tonyg wrote: > > Will this work with sudo in it? I'm not sure we can assume sudo on the host > for > > the bots. > > Ok, so this is another caveat. We don't need sudo for this ifconfig, if there's > only one RNDIS device, because the usbnet driver will automatically configure > the first host usbX interface. This scenario is likely for developer > workstations, but not for the bots. Furthermore, if we want DNS latency > emulation, then we need --dns_forwarding, and in that case WPR needs sudo as > well. > > Will replacing this with "if not root, run with sudo" (as WPR does) work? This brings up a more general question. Could we try to first do RNDIS without the DNS latency emulation and then land that as a follow up? The smaller chunks we can break this up into the better.
https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/chrome/adb_commands.py (right): https://codereview.chromium.org/22286010/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:370: subprocess.check_call(['sudo', 'ifconfig', host_iface, host_ip, On 2013/08/16 04:32:11, tonyg wrote: > On 2013/08/15 19:38:39, szym wrote: > > On 2013/08/15 16:39:11, tonyg wrote: > > > Will this work with sudo in it? I'm not sure we can assume sudo on the host > > for > > > the bots. > > > > Ok, so this is another caveat. We don't need sudo for this ifconfig, if > there's > > only one RNDIS device, because the usbnet driver will automatically configure > > the first host usbX interface. This scenario is likely for developer > > workstations, but not for the bots. Furthermore, if we want DNS latency > > emulation, then we need --dns_forwarding, and in that case WPR needs sudo as > > well. > > > > Will replacing this with "if not root, run with sudo" (as WPR does) work? > > This brings up a more general question. Could we try to first do RNDIS without > the DNS latency emulation and then land that as a follow up? The smaller chunks > we can break this up into the better. We could easily avoid DNS forwarding, but this step: "sudo ifconfig usb1" is unavoidable if you have more than one usbX device. It seems the kernel will always configure usb0 to 192.168.42.1 but that's it.
https://codereview.chromium.org/22286010/diff/56001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/chrome/adb_commands.py (right): https://codereview.chromium.org/22286010/diff/56001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:375: 'netmask', netmask, 'up']) On 2013/08/16 16:26:53, szym wrote: > On 2013/08/16 04:32:11, tonyg wrote: > > On 2013/08/15 19:38:39, szym wrote: > > > On 2013/08/15 16:39:11, tonyg wrote: > > > > Will this work with sudo in it? I'm not sure we can assume sudo on the > host > > > for > > > > the bots. > > > > > > Ok, so this is another caveat. We don't need sudo for this ifconfig, if > > there's > > > only one RNDIS device, because the usbnet driver will automatically > configure > > > the first host usbX interface. This scenario is likely for developer > > > workstations, but not for the bots. Furthermore, if we want DNS latency > > > emulation, then we need --dns_forwarding, and in that case WPR needs sudo as > > > well. > > > > > > Will replacing this with "if not root, run with sudo" (as WPR does) work? > > > > This brings up a more general question. Could we try to first do RNDIS without > > the DNS latency emulation and then land that as a follow up? The smaller > chunks > > we can break this up into the better. > > We could easily avoid DNS forwarding, but this step: "sudo ifconfig usb1" is > unavoidable if you have more than one usbX device. It seems the kernel will > always configure usb0 to 192.168.42.1 but that's it. Ha, so it turns out that the usb0 is not configured by the driver, but rather by Goobuntu's /etc/network/interfaces The way to avoid sudo on the bots would be to configure /etc/network/interfaces auto usb0 usb1 usb2 usb3 usb4 usb5 usb6 iface usb0 inet static address 192.168.44.1 netmask 255.255.255.252 iface usb1 inet static address 192.168.44.5 netmask 255.255.255.252 iface usb2 inet static address 192.168.44.9 netmask 255.255.255.252 ... I'll update this code to leverage the auto-configured address.
On 2013/08/16 17:08:28, szym wrote: > https://codereview.chromium.org/22286010/diff/56001/tools/telemetry/telemetry... > File tools/telemetry/telemetry/core/chrome/adb_commands.py (right): > > https://codereview.chromium.org/22286010/diff/56001/tools/telemetry/telemetry... > tools/telemetry/telemetry/core/chrome/adb_commands.py:375: 'netmask', netmask, > 'up']) > On 2013/08/16 16:26:53, szym wrote: > > On 2013/08/16 04:32:11, tonyg wrote: > > > On 2013/08/15 19:38:39, szym wrote: > > > > On 2013/08/15 16:39:11, tonyg wrote: > > > > > Will this work with sudo in it? I'm not sure we can assume sudo on the > > host > > > > for > > > > > the bots. > > > > > > > > Ok, so this is another caveat. We don't need sudo for this ifconfig, if > > > there's > > > > only one RNDIS device, because the usbnet driver will automatically > > configure > > > > the first host usbX interface. This scenario is likely for developer > > > > workstations, but not for the bots. Furthermore, if we want DNS latency > > > > emulation, then we need --dns_forwarding, and in that case WPR needs sudo > as > > > > well. > > > > > > > > Will replacing this with "if not root, run with sudo" (as WPR does) work? > > > > > > This brings up a more general question. Could we try to first do RNDIS > without > > > the DNS latency emulation and then land that as a follow up? The smaller > > chunks > > > we can break this up into the better. > > > > We could easily avoid DNS forwarding, but this step: "sudo ifconfig usb1" is > > unavoidable if you have more than one usbX device. It seems the kernel will > > always configure usb0 to 192.168.42.1 but that's it. > > Ha, so it turns out that the usb0 is not configured by the driver, but rather by > Goobuntu's /etc/network/interfaces > > The way to avoid sudo on the bots would be to configure /etc/network/interfaces > auto usb0 usb1 usb2 usb3 usb4 usb5 usb6 > iface usb0 inet static > address 192.168.44.1 > netmask 255.255.255.252 > iface usb1 inet static > address 192.168.44.5 > netmask 255.255.255.252 > iface usb2 inet static > address 192.168.44.9 > netmask 255.255.255.252 > ... > > I'll update this code to leverage the auto-configured address. Done.
https://codereview.chromium.org/22286010/diff/75001/DEPS File DEPS (right): https://codereview.chromium.org/22286010/diff/75001/DEPS#newcode236 DEPS:236: (Var("googlecode_url") % "web-page-replay") + "/trunk@519", If you like, you could break this into a new cl and we could land right away. The fewer moving parts in this one the better.
https://codereview.chromium.org/22286010/diff/85001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/browser.py (right): https://codereview.chromium.org/22286010/diff/85001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/browser.py:303: self._wpr_server = wpr_server.ReplayServer( I've been trying to remove DNS forwarding from this CL (primarily in order to postpone the issue of WPR's privileges), but it turns out complicated. SetReplayArchivePath is where we create the forwarder and determine host IP, but the "--host-resolver-rules" is applied in Start(). In page_runner._RunState.StartBrowser, the browser is started only if necessary and always before SetReplayArchivePath. Because RndisForwarderWithRoot leaves RNDIS configured, the assigned host IP will effectively be static, but there is no practical way to know it before the RNDIS is first set up. Consequently, the initial RNDIS setup needs to be performed before the browser is started.
On 2013/08/19 21:26:14, szym wrote: > https://codereview.chromium.org/22286010/diff/85001/tools/telemetry/telemetry... > File tools/telemetry/telemetry/core/browser.py (right): > > https://codereview.chromium.org/22286010/diff/85001/tools/telemetry/telemetry... > tools/telemetry/telemetry/core/browser.py:303: self._wpr_server = > wpr_server.ReplayServer( > I've been trying to remove DNS forwarding from this CL (primarily in order to > postpone the issue of WPR's privileges), but it turns out complicated. > > SetReplayArchivePath is where we create the forwarder and determine host IP, but > the "--host-resolver-rules" is applied in Start(). In > page_runner._RunState.StartBrowser, the browser is started only if necessary and > always before SetReplayArchivePath. > > Because RndisForwarderWithRoot leaves RNDIS configured, the assigned host IP > will effectively be static, but there is no practical way to know it before the > RNDIS is first set up. Consequently, the initial RNDIS setup needs to be > performed before the browser is started. To work around it, I set up an RNDIS forwarder at the time the browser flags are determined. PTAL and let me know if you think we need a better solution.
good stuff, looking forward to it! I think my comments now are mostly nits, except making this optional for the time being... I think we need to address that and address the bots configurations in parallel, wdyt? https://codereview.chromium.org/22286010/diff/87001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/chrome/adb_commands.py (right): https://codereview.chromium.org/22286010/diff/87001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:206: raise Exception('Device does not have rndis') nit: maybe do this as an assertion in 194 above? https://codereview.chromium.org/22286010/diff/87001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:228: return candidates[0] perhaps: raise Exception('No RNDIS device found') https://codereview.chromium.org/22286010/diff/87001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:232: return subprocess.check_output(['ip', 'addr']).splitlines() there's an ifconfig on linux too, are the outputs too different? https://codereview.chromium.org/22286010/diff/87001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/chrome/android_browser_backend.py (right): https://codereview.chromium.org/22286010/diff/87001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/android_browser_backend.py:314: if self._root_enabled: I'm concerned that we're toggling this only by the "root-ability" of the device, and yet it requires changes in the host config.. I think this would break the various existing bots both up and downstream.. :( any chance of making it an option, default to false for the time being, then we start running locally and ask the infra team to setup the extra usb ifaces, and later on make the flag enabled by default? https://codereview.chromium.org/22286010/diff/87001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/android_browser_backend.py:322: args = [arg for arg in args nit: args = filter(lambda arg: not arg.startswith('--host-resolver-rules'), args)
PTAL. It will now gracefully fall back to the userspace forwarder, printing a warning explaining why RNDIS failed. Let me know if you'd like to see an explicit flag. https://codereview.chromium.org/22286010/diff/87001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/chrome/adb_commands.py (right): https://codereview.chromium.org/22286010/diff/87001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:206: raise Exception('Device does not have rndis') On 2013/08/21 16:10:54, bulach wrote: > nit: maybe do this as an assertion in 194 above? Done. https://codereview.chromium.org/22286010/diff/87001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:228: return candidates[0] On 2013/08/21 16:10:54, bulach wrote: > perhaps: > > raise Exception('No RNDIS device found') The logic in CheckEnableRndis relies on this returning None in this case. https://codereview.chromium.org/22286010/diff/87001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/adb_commands.py:232: return subprocess.check_output(['ip', 'addr']).splitlines() On 2013/08/21 16:10:54, bulach wrote: > there's an ifconfig on linux too, are the outputs too different? Yes, the format changed. The format of ip addr on linux matches the format of ifconfig on MacOS. I added a sys.platform check to support both platforms. https://codereview.chromium.org/22286010/diff/87001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/chrome/android_browser_backend.py (right): https://codereview.chromium.org/22286010/diff/87001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/android_browser_backend.py:314: if self._root_enabled: On 2013/08/21 16:10:54, bulach wrote: > I'm concerned that we're toggling this only by the "root-ability" of the device, > and yet it requires changes in the host config.. > I think this would break the various existing bots both up and downstream.. :( > > any chance of making it an option, default to false for the time being, then we > start running locally and ask the infra team to setup the extra usb ifaces, and > later on make the flag enabled by default? I modified the backend to set up RNDIS and if it fails, fall back to forwarder. I'm a little bit concerned about the interaction between Nexus S and Goobuntu (the device falls into "offline" mode), so I'm ok with a flag for now.
yeah, the thing without a flag is that we may end up with erratic behavior, for bots that do have the pre-requisites versus those who don't (I think we now have all bots running standard images, but not too long ago we used to have a variety of different linux versions... :( would you be ok with making it a flag so that we'd have a bit more control about the switch over?
On 2013/08/22 16:19:24, bulach wrote: > yeah, the thing without a flag is that we may end up with erratic behavior, for > bots that do have the pre-requisites versus those who don't (I think we now have > all bots running standard images, but not too long ago we used to have a variety > of different linux versions... :( > > would you be ok with making it a flag so that we'd have a bit more control about > the switch over? I added a flag --android_rndis (open to suggestions for better name).
https://codereview.chromium.org/22286010/diff/106001/chrome/test/functional/w... File chrome/test/functional/webpagereplay.py (right): https://codereview.chromium.org/22286010/diff/106001/chrome/test/functional/w... chrome/test/functional/webpagereplay.py:119: '--host', str(self._replay_host), You need different owners for this file. I think dennisjeffrey is a good one. It might be easiest to break this file off into a separate CL and just land that first. https://codereview.chromium.org/22286010/diff/106001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/browser_options.py (right): https://codereview.chromium.org/22286010/diff/106001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/browser_options.py:89: group.add_option('--android_rndis', action='store_true', While I like bulach's goal of having a controlled rollout, the unfortunate thing about a real command line flag is that this will require a master restart to toggle this flag on the chromium.perf waterfall. To avoid this hassle, I suggest we just add a local boolean _USE_RDNIS_FORWARDING and set it to False in this patch. Then when we experiment with enabling this, we can just change the value with a simple chromium src commit. https://codereview.chromium.org/22286010/diff/106001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/browser_options.py:91: 'Fall back to user-space Forwarder on failure.') If this flag is set, I don't think we want to fall back. It should fail hard if rndis doesn't work. https://codereview.chromium.org/22286010/diff/106001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/chrome/adb_commands.py (right): https://codereview.chromium.org/22286010/diff/106001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/chrome/adb_commands.py:183: class RndisForwarderWithRoot(object): This is meaty enough that it I think it warrants its own file. https://codereview.chromium.org/22286010/diff/106001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/chrome/adb_commands.py:210: def OverrideDns(self): Where did we land on this? I thought we were going to continue to use chrome's host resolver map in the first iteration of this change and then do the dns technique after we get rndis working. One option would be to leave the code in for now, but add a _USE_DNS_FORWARDING flag. https://codereview.chromium.org/22286010/diff/106001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/chrome/adb_commands.py:233: return subprocess.check_output(['ip', 'addr']).splitlines() Unfortunately, the bots run python 2.6 and check_output is a 2.7 method. https://codereview.chromium.org/22286010/diff/106001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/chrome/adb_commands.py:234: if sys.platform.startswith('darwin'): elif and you can just use == here.
https://codereview.chromium.org/22286010/diff/106001/chrome/test/functional/w... File chrome/test/functional/webpagereplay.py (right): https://codereview.chromium.org/22286010/diff/106001/chrome/test/functional/w... chrome/test/functional/webpagereplay.py:119: '--host', str(self._replay_host), On 2013/08/22 23:55:03, tonyg wrote: > You need different owners for this file. I think dennisjeffrey is a good one. It > might be easiest to break this file off into a separate CL and just land that > first. Done. https://codereview.chromium.org/23403002/ https://codereview.chromium.org/22286010/diff/106001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/browser_options.py (right): https://codereview.chromium.org/22286010/diff/106001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/browser_options.py:89: group.add_option('--android_rndis', action='store_true', On 2013/08/22 23:55:03, tonyg wrote: > While I like bulach's goal of having a controlled rollout, the unfortunate thing > about a real command line flag is that this will require a master restart to > toggle this flag on the chromium.perf waterfall. To avoid this hassle, I suggest > we just add a local boolean _USE_RDNIS_FORWARDING and set it to False in this > patch. Then when we experiment with enabling this, we can just change the value > with a simple chromium src commit. Ack. https://codereview.chromium.org/22286010/diff/106001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/browser_options.py:91: 'Fall back to user-space Forwarder on failure.') On 2013/08/22 23:55:03, tonyg wrote: > If this flag is set, I don't think we want to fall back. It should fail hard if > rndis doesn't work. What if there's no flag? I'm assuming you are suggesting to remove it above. https://codereview.chromium.org/22286010/diff/106001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/chrome/adb_commands.py (right): https://codereview.chromium.org/22286010/diff/106001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/chrome/adb_commands.py:183: class RndisForwarderWithRoot(object): On 2013/08/22 23:55:03, tonyg wrote: > This is meaty enough that it I think it warrants its own file. Done. https://codereview.chromium.org/22286010/diff/106001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/chrome/adb_commands.py:210: def OverrideDns(self): On 2013/08/22 23:55:03, tonyg wrote: > Where did we land on this? I thought we were going to continue to use chrome's > host resolver map in the first iteration of this change and then do the dns > technique after we get rndis working. > > One option would be to leave the code in for now, but add a _USE_DNS_FORWARDING > flag. Right now, AdbCommands._override_dns = False. https://codereview.chromium.org/22286010/diff/106001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/chrome/adb_commands.py:233: return subprocess.check_output(['ip', 'addr']).splitlines() On 2013/08/22 23:55:03, tonyg wrote: > Unfortunately, the bots run python 2.6 and check_output is a 2.7 method. Makes me wonder why. Python 2.6.X expires (no further releases) in a couple months (October 2013).
> Makes me wonder why. Python 2.6.X expires (no further releases) in a > couple months (October 2013). > > I believe it is to support pyauto (which is deprecated and scheduled for deletion). To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
This is looking really close. Other than the comment below and the question about the command line flag, this lg2m. I'll leave the final approval to bulach@. https://codereview.chromium.org/22286010/diff/74002/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/chrome/android_browser_backend.py (right): https://codereview.chromium.org/22286010/diff/74002/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/android_browser_backend.py:177: forwarder = adb_commands.RndisForwarderWithRoot(self._adb) I imagine it takes a non-trivial amount of time to create an rndis forwarder. Can we rework this so we don't end up creating it twice in the typical flow?
https://codereview.chromium.org/22286010/diff/106001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/chrome/adb_commands.py (right): https://codereview.chromium.org/22286010/diff/106001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/chrome/adb_commands.py:233: return subprocess.check_output(['ip', 'addr']).splitlines() On 2013/08/23 15:33:41, szym wrote: > On 2013/08/22 23:55:03, tonyg wrote: > > Unfortunately, the bots run python 2.6 and check_output is a 2.7 method. > > Makes me wonder why. Python 2.6.X expires (no further releases) in a couple > months (October 2013). Done. https://codereview.chromium.org/22286010/diff/106001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/chrome/adb_commands.py:234: if sys.platform.startswith('darwin'): On 2013/08/22 23:55:03, tonyg wrote: > elif and you can just use == here. Done. https://codereview.chromium.org/22286010/diff/74002/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/chrome/android_browser_backend.py (right): https://codereview.chromium.org/22286010/diff/74002/tools/telemetry/telemetry... tools/telemetry/telemetry/core/chrome/android_browser_backend.py:177: forwarder = adb_commands.RndisForwarderWithRoot(self._adb) On 2013/08/25 22:32:08, tonyg wrote: > I imagine it takes a non-trivial amount of time to create an rndis forwarder. > Can we rework this so we don't end up creating it twice in the typical flow? RndisForwarderWithRoot does very little work which takes no time if RNDIS is already up (netcfg/ifconfig/ping), but even this could be "cached". Done, although the downside is that if something disturbs the connectivity, the test will fail even though re-checking RNDIS could have fixed it.
lgtm, thanks! some food for thought: - I'm adding a bunch of people who have been involved with forwarder and maybe interested in RNDIS... depending on how this goes, we should eventually move down build/android/pylib. - re: flags: yeah, having it as a real flag would probably be a bit easier... reason being: we have downstream bots where the commands are in our "side", i.e., not buildbot.. also, it won't require buildbot master restart... so how about experimenting there, without affecting all downstream + upstream bots at once? my suggestion for controlled roll out is to try downstream first (where we have slightly more control and less people looking), then move it upstream once we're happy with it. again, I'm fine if you both (tonyg / szym) prefer as a hardcoded value, I just think it'd require more coordination.. feel free to go ahead anyways! :) tiny nits: https://codereview.chromium.org/22286010/diff/102007/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/chrome/android_rndis.py (right): https://codereview.chromium.org/22286010/diff/102007/tools/telemetry/telemetr... tools/telemetry/telemetry/core/chrome/android_rndis.py:13: nit: here and 27 below, two \n between top-levels https://codereview.chromium.org/22286010/diff/102007/tools/telemetry/telemetr... tools/telemetry/telemetry/core/chrome/android_rndis.py:20: cmd = kwargs.get("args") nit: ' is more common than "
On 2013/09/02 08:49:00, bulach wrote: > lgtm, thanks! > > some food for thought: > - I'm adding a bunch of people who have been involved with forwarder and maybe > interested in RNDIS... depending on how this goes, we should eventually move > down build/android/pylib. > > - re: flags: yeah, having it as a real flag would probably be a bit easier... > reason being: we have downstream bots where the commands are in our "side", > i.e., not buildbot.. also, it won't require buildbot master restart... > > so how about experimenting there, without affecting all downstream + upstream > bots at once? my suggestion for controlled roll out is to try downstream first > (where we have slightly more control and less people looking), then move it > upstream once we're happy with it. > > again, I'm fine if you both (tonyg / szym) prefer as a hardcoded value, I just > think it'd require more coordination.. feel free to go ahead anyways! :) > I ended up adding new flags --android-rndis / --no-android-rndis [default] This way the default can be changed via chromium CL, and local overrides can be set via bot config. > tiny nits: > > https://codereview.chromium.org/22286010/diff/102007/tools/telemetry/telemetr... > File tools/telemetry/telemetry/core/chrome/android_rndis.py (right): > > https://codereview.chromium.org/22286010/diff/102007/tools/telemetry/telemetr... > tools/telemetry/telemetry/core/chrome/android_rndis.py:13: > nit: here and 27 below, two \n between top-levels > > https://codereview.chromium.org/22286010/diff/102007/tools/telemetry/telemetr... > tools/telemetry/telemetry/core/chrome/android_rndis.py:20: cmd = > kwargs.get("args") > nit: ' is more common than "
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/szym@chromium.org/22286010/136001
Failed to apply patch for
tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file
tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py
Hunk #2 FAILED at 172.
Hunk #3 succeeded at 246 (offset 14 lines).
Hunk #4 succeeded at 325 (offset 8 lines).
1 out of 4 hunks FAILED -- saving rejects to file
tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py.rej
Patch:
tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py
Index: tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py
diff --git
a/tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py
b/tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py
index
16d9f9bd79d5afb13186da78dfb35edb767e2513..90fb2625f47d6b1b875cbb59ff02153f1b2c147e
100644
--- a/tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py
+++ b/tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py
@@ -11,6 +11,7 @@ import time
from telemetry.core import exceptions
from telemetry.core import util
from telemetry.core.backends import adb_commands
+from telemetry.core.backends import android_rndis
from telemetry.core.backends import browser_backend
from telemetry.core.backends.chrome import chrome_browser_backend
@@ -171,6 +172,14 @@ class
AndroidBrowserBackend(chrome_browser_backend.ChromeBrowserBackend):
if options.profile_dir:
self._backend_settings.PushProfile(options.profile_dir)
+ # Pre-configure RNDIS forwarding.
+ self._rndis_forwarder = None
+ if options.android_rndis:
+ self._rndis_forwarder = android_rndis.RndisForwarderWithRoot(self._adb)
+ self.WEBPAGEREPLAY_HOST = self._rndis_forwarder.host_ip
+ # TODO(szym): only override DNS if WPR has privileges to proxy on port 25.
+ self._override_dns = False
+
# Set up the command line.
self._saved_cmdline = ''.join(self._adb.Adb().GetProtectedFileContents(
self._backend_settings.cmdline_file) or [])
@@ -231,6 +240,9 @@ class
AndroidBrowserBackend(chrome_browser_backend.ChromeBrowserBackend):
def GetBrowserStartupArgs(self):
args = super(AndroidBrowserBackend, self).GetBrowserStartupArgs()
+ if self._override_dns:
+ args = [arg for arg in args
+ if not arg.startswith('--host-resolver-rules')]
args.append('--enable-remote-debugging')
args.append('--no-restore-state')
args.append('--disable-fre')
@@ -313,5 +325,21 @@ class
AndroidBrowserBackend(chrome_browser_backend.ChromeBrowserBackend):
stdout=subprocess.PIPE).communicate()[0])
return ret
+ def AddReplayServerOptions(self, options):
+ """Override. Only add --no-dns_forwarding if not using RNDIS."""
+ if not self._override_dns:
+ options.append('--no-dns_forwarding')
+
def CreateForwarder(self, *port_pairs):
+ if self._rndis_forwarder:
+ forwarder = self._rndis_forwarder
+ forwarder.SetPorts(*port_pairs)
+ assert self.WEBPAGEREPLAY_HOST == forwarder.host_ip, (
+ 'Host IP address on the RNDIS interface changed. Must restart
browser!')
+ if self._override_dns:
+ forwarder.OverrideDns()
+ return forwarder
+ assert not self._override_dns, ('The user-space forwarder does not support
'
+ 'DNS override!')
+ logging.warning('Using the user-space forwarder.\n')
return adb_commands.Forwarder(self._adb, *port_pairs)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/szym@chromium.org/22286010/144001
Message was sent while issue was closed.
Change committed as 221047 |
