Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
255783008:
Add option to wipe device. (Closed)
|
Created:
6 years, 8 months ago by navabi Modified:
6 years, 7 months ago CC:
chromium-reviews, klundberg+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAlways wipe device data, but dont reboot.
We keep seeing the INSTALL_FAILED_INSUFFICIENT_STORAGE error on the devices. The
APK we install to check for this failure succeeds even when this error exists on
the device (presumably because the APK we install is too small). Thus, this CL
will always wipe the device. It removes the reboot, because that would make the
step take too long.
BUG=335549
TBR=yfriedman@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269408
Patch Set 1 #Patch Set 2 : Always wipe devices. #
Total comments: 2
Patch Set 3 : Remove reboot arg from WipeDevices. #Patch Set 4 : Rebase with tonyg's root device change. #Patch Set 5 : Add parallel device reboot after wipe. #
Total comments: 1
Patch Set 6 : Remove printing debug statement. #Patch Set 7 : Remove automatic wipe, and add option for manual wipe. #
Total comments: 5
Patch Set 8 : Fix nits. #
Messages
Total messages: 33 (0 generated)
lgtm
lgtm w/ nits https://codereview.chromium.org/255783008/diff/10001/build/android/provision_... File build/android/provision_devices.py (right): https://codereview.chromium.org/255783008/diff/10001/build/android/provision_... build/android/provision_devices.py:91: def WipeDeviceData(device, reboot=True): Is this ever called with 'reboot=True'? If not, I suggest you remove 'reboot' functionality from this function altogether and let its caller decide to reboot after a wipe independently.
https://codereview.chromium.org/255783008/diff/10001/build/android/provision_... File build/android/provision_devices.py (right): https://codereview.chromium.org/255783008/diff/10001/build/android/provision_... build/android/provision_devices.py:91: def WipeDeviceData(device, reboot=True): On 2014/04/26 01:11:26, Dan Jacques wrote: > Is this ever called with 'reboot=True'? If not, I suggest you remove 'reboot' > functionality from this function altogether and let its caller decide to reboot > after a wipe independently. Done.
On 2014/04/26 01:15:35, navabi wrote: > https://codereview.chromium.org/255783008/diff/10001/build/android/provision_... > File build/android/provision_devices.py (right): > > https://codereview.chromium.org/255783008/diff/10001/build/android/provision_... > build/android/provision_devices.py:91: def WipeDeviceData(device, reboot=True): > On 2014/04/26 01:11:26, Dan Jacques wrote: > > Is this ever called with 'reboot=True'? If not, I suggest you remove 'reboot' > > functionality from this function altogether and let its caller decide to > reboot > > after a wipe independently. > > Done. lgtm
The CQ bit was checked by navabi@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/navabi@google.com/255783008/30001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_clang_dbg tryserver.chromium on linux_chromium_gn_rel
The CQ bit was checked by navabi@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/navabi@google.com/255783008/30001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for build/android/provision_devices.py:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file build/android/provision_devices.py
Hunk #4 FAILED at 124.
1 out of 4 hunks FAILED -- saving rejects to file
build/android/provision_devices.py.rej
Patch: build/android/provision_devices.py
Index: build/android/provision_devices.py
diff --git a/build/android/provision_devices.py
b/build/android/provision_devices.py
index
6dc320ad2c5fde1a5e80deacbfd2d3397a0f9da1..d6ca91bc1251c0fed3706c24be7bf61138ff22f6
100755
--- a/build/android/provision_devices.py
+++ b/build/android/provision_devices.py
@@ -21,7 +21,6 @@ import time
from pylib import android_commands
from pylib import constants
from pylib import device_settings
-from pylib.cmd_helper import GetCmdOutput
from pylib.device import device_utils
def KillHostHeartbeat():
@@ -95,8 +94,8 @@ def WipeDeviceData(device):
After wiping data on a device that has been authorized, adb can still
communicate with the device, but after reboot the device will need to be
re-authorized because the adb keys file is stored in /data/misc/adb/.
- Thus, before reboot the adb_keys file is rewritten so the device does not
need
- to be re-authorized.
+ Thus, adb_keys file is rewritten so the device does not need to be
+ re-authorized.
Arguments:
device: the device to wipe
@@ -105,15 +104,17 @@ def WipeDeviceData(device):
constants.ADB_KEYS_FILE)
if device_authorized:
adb_keys = device.old_interface.RunShellCommandWithSU(
- 'cat %s' % constants.ADB_KEYS_FILE)[0]
+ 'cat %s' % constants.ADB_KEYS_FILE)
device.old_interface.RunShellCommandWithSU('wipe data')
if device_authorized:
path_list = constants.ADB_KEYS_FILE.split('/')
dir_path = '/'.join(path_list[:len(path_list)-1])
device.old_interface.RunShellCommandWithSU('mkdir -p %s' % dir_path)
- adb_keys = device.old_interface.RunShellCommand(
- 'echo %s > %s' % (adb_keys, constants.ADB_KEYS_FILE))
- device.old_interface.Reboot()
+ device.old_interface.RunShellCommandWithSU('touch %s' %
+ constants.ADB_KEYS_FILE)
+ for adb_key in adb_keys:
+ device.old_interface.RunShellCommand(
+ 'echo %s >> %s' % (adb_key, constants.ADB_KEYS_FILE))
def ProvisionDevices(options):
@@ -123,14 +124,7 @@ def ProvisionDevices(options):
devices = android_commands.GetAttachedDevices()
for device_serial in devices:
device = device_utils.DeviceUtils(device_serial)
- install_output = GetCmdOutput(
- ['%s/build/android/adb_install_apk.py' % constants.DIR_SOURCE_ROOT,
- '--apk',
- '%s/build/android/CheckInstallApk-debug.apk' % constants.DIR_SOURCE_ROOT
- ])
- failure_string = 'Failure [INSTALL_FAILED_INSUFFICIENT_STORAGE]'
- if failure_string in install_output:
- WipeDeviceData(device)
+ WipeDeviceData(device)
_ConfigureLocalProperties(device)
device_settings.ConfigureContentSettingsDict(
device, device_settings.DETERMINISTIC_DEVICE_SETTINGS)
The CQ bit was checked by navabi@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/navabi@google.com/255783008/50001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_clang_dbg
On 2014/04/28 17:21:24, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > tryserver.chromium on linux_chromium_clang_dbg Turns out we can't just wipe the devices, not reboot and go on with the build. The install fails. See the android_dbg_triggered_tests failure: http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered... I confirmed that if we reboot after the provisioning script runs the install works. I also confirmed that is in fact the wiping that causes the install to fail (i.e. same script without the call to WipeDeviceData will install fine. Unfortunately, rebooting takes a long time adding a significant time to the cycle time. I think it will be between 3 and 5 mins, so it is doable. Note that most bot reboot the phones anyway before the provisioning. We may be able to move that reboot instead of adding another reboot step. I don't think the reboot at that particular point in the build is that important (i.e. I believe that reboot is there just so we reboot the device before every build). In that case, it would make sense to move it after the wiping the device. Here is what I tried to do after the wipe, besides rebooting, to see if a subsequent install worked: - restart adb server on the hose (i.e. adb kill-server) - kill adbd on the device, so the install has to launch a new adbd process Neither of these worked.
https://codereview.chromium.org/255783008/diff/70001/build/android/pylib/cont... File build/android/pylib/content_settings.py (right): https://codereview.chromium.org/255783008/diff/70001/build/android/pylib/cont... build/android/pylib/content_settings.py:54: print 'asserting %s, %s' % (key, value) remove this debugging print.
Almost there. Except when we wipe device data the subsequent content setting
fails when testing locally:
INFO:root:[a4b6]> su -c content query --uri
content://com.google.settings/partner
Traceback (most recent call last):
File "build/android/provision_devices.py", line 165, in <module>
sys.exit(main(sys.argv))
File "build/android/provision_devices.py", line 161, in main
ProvisionDevices(options)
File "build/android/provision_devices.py", line 133, in ProvisionDevices
device, device_settings.DETERMINISTIC_DEVICE_SETTINGS)
File
"/usr/local/google/code/clankium-master/src/build/android/pylib/device_settings.py",
line 43, in ConfigureContentSettingsDict
for key, value in sorted(settings.iteritems()):
File
"/usr/local/google/code/clankium-master/src/build/android/pylib/content_settings.py",
line 55, in iteritems
assert key, value
AssertionError: None
Without the WipeDeviceData for loop this does not fail. Still investigating why
that is. Going to trigger some try jobs.
PTAL I've changed this slightly to be a -w option to provision_device.py that will wipe the device data and reboot the devices in parallel. The originally goal was to wipe device data, reboot the devices in parallel and continue with provisioning. Unfortunately, the reboot does not wait until the device is ready to be provisioned (as it attempts to do by waiting for package manager). Waiting a couple minutes after the wipe device data and subsequent reboot of the devices works. Until we have a more permanent solution, this will make it easier to fix the problem when it happens.
Looks good otherwise, and I defer to people more familiar with the code in question. https://codereview.chromium.org/255783008/diff/110001/build/android/provision... File build/android/provision_devices.py (right): https://codereview.chromium.org/255783008/diff/110001/build/android/provision... build/android/provision_devices.py:114: device.old_interface.RunShellCommandWithSU('touch %s' % Note that it may have different behavior if the file already exists. Maybe 'echo /dev/null > file' to always clobber it? Otherwise it could keep growing, or at least it'd be more error-prone even though we request a data wipe.
lgtm w/ nits https://codereview.chromium.org/255783008/diff/110001/build/android/provision... File build/android/provision_devices.py (right): https://codereview.chromium.org/255783008/diff/110001/build/android/provision... build/android/provision_devices.py:114: device.old_interface.RunShellCommandWithSU('touch %s' % On 2014/05/08 11:21:31, Paweł Hajdan Jr. wrote: > Note that it may have different behavior if the file already exists. > > Maybe 'echo /dev/null > file' to always clobber it? Otherwise it could keep > growing, or at least it'd be more error-prone even though we request a data > wipe. Good catch - An alternative could be just to make the first "echo" use a single ">" and subsequent ones use a ">>" (see line 118). https://codereview.chromium.org/255783008/diff/110001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/255783008/diff/110001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:25: except Exception as e: The original one explicitly catches DeviceUnresponsiveError. Catching all Exceptions generically could mask some underlying problems. Unless there's a specific reason to switch, I recommend going back to explicitly catching "DeviceUnresponsiveError".
https://codereview.chromium.org/255783008/diff/110001/build/android/provision... File build/android/provision_devices.py (right): https://codereview.chromium.org/255783008/diff/110001/build/android/provision... build/android/provision_devices.py:114: device.old_interface.RunShellCommandWithSU('touch %s' % On 2014/05/08 15:18:16, dnj wrote: > On 2014/05/08 11:21:31, Paweł Hajdan Jr. wrote: > > Note that it may have different behavior if the file already exists. > > > > Maybe 'echo /dev/null > file' to always clobber it? Otherwise it could keep > > growing, or at least it'd be more error-prone even though we request a data > > wipe. > > Good catch - An alternative could be just to make the first "echo" use a single > ">" and subsequent ones use a ">>" (see line 118). Done. https://codereview.chromium.org/255783008/diff/110001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/255783008/diff/110001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:25: except Exception as e: On 2014/05/08 15:18:16, dnj wrote: > The original one explicitly catches DeviceUnresponsiveError. Catching all > Exceptions generically could mask some underlying problems. Unless there's a > specific reason to switch, I recommend going back to explicitly catching > "DeviceUnresponsiveError". Done.
The CQ bit was checked by navabi@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/navabi@google.com/255783008/130001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was checked by navabi@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/navabi@google.com/255783008/130001
Message was sent while issue was closed.
Change committed as 269408
Message was sent while issue was closed.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
