Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1211323004:
[Telemetry] Remove adb_commands usage from android_device (Closed)
|
Created:
5 years, 5 months ago by nednguyen Modified:
5 years, 5 months ago Reviewers:
jbudorick CC:
chromium-reviews, jbudorick+watch_chromium.org, klundberg+watch_chromium.org, telemetry-reviews_chromium.org, yfriedman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Telemetry] Remove adb_commands usage from android_device
Reland of https://codereview.chromium.org/1212643002/
This time, we don't use DeviceUtils.HealthyDevices() API to make sure that all
connected devices are processed instead of only healthy devices. This is to make
sure that the cq bots have enough capacity to handle the unittests load since
some of the android bots used by CQ are marked unhealthy.
BUG=476709
Committed: https://crrev.com/d5bb872d537153753f930eab6097afb315612274
Cr-Commit-Position: refs/heads/master@{#337099}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Address John's comment #
Total comments: 2
Patch Set 3 : Create warning for unhealthy devices #Patch Set 4 : Add the step of setting up PATH variable with adb back #
Total comments: 1
Messages
Total messages: 36 (12 generated)
nednguyen@google.com changed reviewers: + jbudorick@chromium.org
From the logs when this landed the first time (e.g. http://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/20...), it appeared that telemetry is calling HealthyDevices (or equivalent) far more than it should -- see the abundance of "XYZ is blacklisted" messages. https://codereview.chromium.org/1211323004/diff/1/build/android/pylib/device/... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1211323004/diff/1/build/android/pylib/device/... build/android/pylib/device/device_utils.py:1749: def GetAttachedDevices(cls): I would prefer to not add this here. Generally, clients should be using HealthyDevices -- we blacklist things for a reason. If clients want to use blacklisted devices, I think it's ok if they use AdbWrapper directly.
https://codereview.chromium.org/1211323004/diff/1/build/android/pylib/device/... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1211323004/diff/1/build/android/pylib/device/... build/android/pylib/device/device_utils.py:1749: def GetAttachedDevices(cls): On 2015/06/29 17:56:23, jbudorick wrote: > I would prefer to not add this here. Generally, clients should be using > HealthyDevices -- we blacklist things for a reason. If clients want to use > blacklisted devices, I think it's ok if they use AdbWrapper directly. Ok. But I am curious about this black listing strategy. How would you make pylib/ an open source project with a hard coded list of device ids?
https://codereview.chromium.org/1211323004/diff/1/build/android/pylib/device/... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1211323004/diff/1/build/android/pylib/device/... build/android/pylib/device/device_utils.py:1749: def GetAttachedDevices(cls): On 2015/06/29 at 17:59:12, nednguyen wrote: > On 2015/06/29 17:56:23, jbudorick wrote: > > I would prefer to not add this here. Generally, clients should be using > > HealthyDevices -- we blacklist things for a reason. If clients want to use > > blacklisted devices, I think it's ok if they use AdbWrapper directly. > > Ok. But I am curious about this black listing strategy. How would you make pylib/ an open source project with a hard coded list of device ids? ...? We don't have a hard-coded list of device IDs. Blacklisting is done on a per-run basis and is handled by https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli.... Typically, we blacklist devices for misbehaving in device_status_check or provision_devices.
https://codereview.chromium.org/1211323004/diff/1/build/android/pylib/device/... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1211323004/diff/1/build/android/pylib/device/... build/android/pylib/device/device_utils.py:1749: def GetAttachedDevices(cls): On 2015/06/29 18:03:45, jbudorick wrote: > On 2015/06/29 at 17:59:12, nednguyen wrote: > > On 2015/06/29 17:56:23, jbudorick wrote: > > > I would prefer to not add this here. Generally, clients should be using > > > HealthyDevices -- we blacklist things for a reason. If clients want to use > > > blacklisted devices, I think it's ok if they use AdbWrapper directly. > > > > Ok. But I am curious about this black listing strategy. How would you make > pylib/ an open source project with a hard coded list of device ids? > > ...? > > We don't have a hard-coded list of device IDs. Blacklisting is done on a per-run > basis and is handled by > https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli.... > Typically, we blacklist devices for misbehaving in device_status_check or > provision_devices. By hard coded, I mean the path to json file bad_devices.json. But look like we should be able to make this configurable.
https://codereview.chromium.org/1211323004/diff/1/build/android/pylib/device/... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1211323004/diff/1/build/android/pylib/device/... build/android/pylib/device/device_utils.py:1749: def GetAttachedDevices(cls): On 2015/06/29 at 18:14:54, nednguyen wrote: > On 2015/06/29 18:03:45, jbudorick wrote: > > On 2015/06/29 at 17:59:12, nednguyen wrote: > > > On 2015/06/29 17:56:23, jbudorick wrote: > > > > I would prefer to not add this here. Generally, clients should be using > > > > HealthyDevices -- we blacklist things for a reason. If clients want to use > > > > blacklisted devices, I think it's ok if they use AdbWrapper directly. > > > > > > Ok. But I am curious about this black listing strategy. How would you make > > pylib/ an open source project with a hard coded list of device ids? > > > > ...? > > > > We don't have a hard-coded list of device IDs. Blacklisting is done on a per-run > > basis and is handled by > > https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli.... > > Typically, we blacklist devices for misbehaving in device_status_check or > > provision_devices. > > By hard coded, I mean the path to json file bad_devices.json. But look like we should be able to make this configurable. Oh, yeah. That part is hard-coded, but I don't think it'll be terribly difficult to change that for extraction from chromium. I thought you meant a hard-coded list of device IDs.
PTAL https://codereview.chromium.org/1211323004/diff/1/build/android/pylib/device/... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1211323004/diff/1/build/android/pylib/device/... build/android/pylib/device/device_utils.py:1749: def GetAttachedDevices(cls): On 2015/06/29 18:27:44, jbudorick wrote: > On 2015/06/29 at 18:14:54, nednguyen wrote: > > On 2015/06/29 18:03:45, jbudorick wrote: > > > On 2015/06/29 at 17:59:12, nednguyen wrote: > > > > On 2015/06/29 17:56:23, jbudorick wrote: > > > > > I would prefer to not add this here. Generally, clients should be using > > > > > HealthyDevices -- we blacklist things for a reason. If clients want to > use > > > > > blacklisted devices, I think it's ok if they use AdbWrapper directly. > > > > > > > > Ok. But I am curious about this black listing strategy. How would you make > > > pylib/ an open source project with a hard coded list of device ids? > > > > > > ...? > > > > > > We don't have a hard-coded list of device IDs. Blacklisting is done on a > per-run > > > basis and is handled by > > > > https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli.... > > > Typically, we blacklist devices for misbehaving in device_status_check or > > > provision_devices. > > > > By hard coded, I mean the path to json file bad_devices.json. But look like we > should be able to make this configurable. > > Oh, yeah. That part is hard-coded, but I don't think it'll be terribly difficult > to change that for extraction from chromium. I thought you meant a hard-coded > list of device IDs. Done.
lgtm https://codereview.chromium.org/1211323004/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/platform/android_device.py (right): https://codereview.chromium.org/1211323004/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/android_device.py:47: # TODO: use pylib.device.device_utils.DeviceUtils.HealthyDevices() to discover It's possible that this isn't actually the end state telemetry wants to be in given its sharding mechanism -- perhaps telemetry really does want to use devices even if they've been previously blacklisted. In that case, it would make more sense to simply log a warning if a device has been blacklisted. wdyt?
https://codereview.chromium.org/1211323004/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/platform/android_device.py (right): https://codereview.chromium.org/1211323004/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/android_device.py:47: # TODO: use pylib.device.device_utils.DeviceUtils.HealthyDevices() to discover On 2015/06/29 19:00:19, jbudorick wrote: > It's possible that this isn't actually the end state telemetry wants to be in > given its sharding mechanism -- perhaps telemetry really does want to use > devices even if they've been previously blacklisted. In that case, it would make > more sense to simply log a warning if a device has been blacklisted. wdyt? SGTM. done.
The CQ bit was checked by nednguyen@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/1211323004/#ps10004 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1211323004/10004
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by nednguyen@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1211323004/10004
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
Patchset #4 (id:50001) has been deleted
The CQ bit was checked by nednguyen@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/1211323004/#ps70001 (title: "REbase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1211323004/70001
Patchset #4 (id:70001) has been deleted
The CQ bit was checked by nednguyen@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/1211323004/#ps90001 (title: "Add the step of setting up PATH variable with adb back")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1211323004/90001
Message was sent while issue was closed.
Committed patchset #4 (id:90001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/d5bb872d537153753f930eab6097afb315612274 Cr-Commit-Position: refs/heads/master@{#337099}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:90001) has been created in https://codereview.chromium.org/1218823006/ by nednguyen@google.com. The reason for reverting is: Speculative revert as this may break broke telemetry perf android tests (http://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/20964).
Message was sent while issue was closed.
On 2015/07/02 at 01:25:25, nednguyen wrote: > A revert of this CL (patchset #4 id:90001) has been created in https://codereview.chromium.org/1218823006/ by nednguyen@google.com. > > The reason for reverting is: Speculative revert as this may break broke telemetry perf android tests (http://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/20964). Four of the devices on that bot are blacklisted. This landed with a trybot run where no devices were blacklisted.
Message was sent while issue was closed.
On 2015/07/02 01:37:09, jbudorick (ooo until july 13) wrote: > On 2015/07/02 at 01:25:25, nednguyen wrote: > > A revert of this CL (patchset #4 id:90001) has been created in > https://codereview.chromium.org/1218823006/ by mailto:nednguyen@google.com. > > > > The reason for reverting is: Speculative revert as this may break broke > telemetry perf android tests > (http://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/20964). > > Four of the devices on that bot are blacklisted. This landed with a trybot run > where no devices were blacklisted. Hmhh, I thought I made sure that this will also list out the blacklisted devices.
Message was sent while issue was closed.
On 2015/07/02 at 02:52:31, nednguyen wrote: > On 2015/07/02 01:37:09, jbudorick (ooo until july 13) wrote: > > On 2015/07/02 at 01:25:25, nednguyen wrote: > > > A revert of this CL (patchset #4 id:90001) has been created in > > https://codereview.chromium.org/1218823006/ by mailto:nednguyen@google.com. > > > > > > The reason for reverting is: Speculative revert as this may break broke > > telemetry perf android tests > > (http://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/20964). > > > > Four of the devices on that bot are blacklisted. This landed with a trybot run > > where no devices were blacklisted. > > Hmhh, I thought I made sure that this will also list out the blacklisted devices. it failed because it _ran_ on blacklisted devices :/
Message was sent while issue was closed.
On 2015/07/02 02:58:28, jbudorick (ooo until july 13) wrote: > On 2015/07/02 at 02:52:31, nednguyen wrote: > > On 2015/07/02 01:37:09, jbudorick (ooo until july 13) wrote: > > > On 2015/07/02 at 01:25:25, nednguyen wrote: > > > > A revert of this CL (patchset #4 id:90001) has been created in > > > https://codereview.chromium.org/1218823006/ by mailto:nednguyen@google.com. > > > > > > > > The reason for reverting is: Speculative revert as this may break broke > > > telemetry perf android tests > > > > (http://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/20964). > > > > > > Four of the devices on that bot are blacklisted. This landed with a trybot > run > > > where no devices were blacklisted. > > > > Hmhh, I thought I made sure that this will also list out the blacklisted > devices. > > it failed because it _ran_ on blacklisted devices :/ I don't understand. I thought the current way (without this patch) also list out blacklisted devices (which is why the first attempt of landing this failed). Or is blacklisted != unhealthy?
Message was sent while issue was closed.
https://codereview.chromium.org/1211323004/diff/90001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/platform/android_device.py (left): https://codereview.chromium.org/1211323004/diff/90001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/android_device.py:47: device_serials = adb_commands.GetAttachedDevices() the old AndroidCommands.GetAttachedDevices uses the blacklist: https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli...
Message was sent while issue was closed.
I've found that my phone all of a sudden keeps getting blacklisted via bad_devices.json. I have had to delete that file twice today now (took a while to find out why the first time it happened). Is there a bug for this blacklisting feature or should I just file a new one? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
