Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
334503002:
Refactor wipe option to wipe on default and have option to skip wipe. (Closed)
|
Created:
6 years, 6 months ago by navabi Modified:
6 years, 6 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. |
DescriptionRefactor wipe option to wipe on default and have option to skip wipe.
Also remove the option -w to only wipe without provisioning. This was temporary
functionality to recover from INSUFFICIENT_STORAGE on device error. With the
default wiping, this should be unnecessary. For bots that skip wiping, we can
still manually fix by manually running without --skip-wipe.
BUG=383106
TBR=craigdh@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276866
Patch Set 1 #
Total comments: 11
Patch Set 2 : Re-purpose script to remove -w option and address reviews. #
Total comments: 3
Patch Set 3 : Moved wiping into provision function. #
Total comments: 1
Patch Set 4 : Tried to rebase. Didn't pull change I expected. #Messages
Total messages: 19 (0 generated)
lgtm you might see a conflict with this: https://codereview.chromium.org/292313015/ ...if it ever gets through CQ. https://codereview.chromium.org/334503002/diff/1/build/android/provision_devi... File build/android/provision_devices.py (right): https://codereview.chromium.org/334503002/diff/1/build/android/provision_devi... build/android/provision_devices.py:160: parser.add_option('--skip-wipe', action='store_true', default=False, Do any of the bots use -w or --wipe?
I will do the merge if there is a conflict. Looks like a straightforward merge. Thanks for the heads up. https://codereview.chromium.org/334503002/diff/1/build/android/provision_devi... File build/android/provision_devices.py (right): https://codereview.chromium.org/334503002/diff/1/build/android/provision_devi... build/android/provision_devices.py:160: parser.add_option('--skip-wipe', action='store_true', default=False, On 2014/06/12 00:17:17, jbudorick wrote: > Do any of the bots use -w or --wipe? No, there shouldn't be. If you look below that was used for manually wiping devices. I added that before we figured out a way to wipe on every build so that I had an easy way to wipe when we saw insufficient storage on the devices.
It looks like the usage of this script is fundamentally changing, as you've removed the ability for someone to wipe all devices without provisioning. That goes beyond the scope of the CL, so make sure you update either the code or the CL! https://codereview.chromium.org/334503002/diff/1/build/android/provision_devi... File build/android/provision_devices.py (right): https://codereview.chromium.org/334503002/diff/1/build/android/provision_devi... build/android/provision_devices.py:161: help='Don\'t wipe device data during provisioning.') Just use quotes here; no need to add unnecessary escaping. https://codereview.chromium.org/334503002/diff/1/build/android/provision_devi... build/android/provision_devices.py:176: devices = android_commands.GetAttachedDevices() The old method had an explicit 'wipe' command that wiped all attached devices and then quit. This was a single execution; people would run the script explicitly to wipe devices. The old behavior of './provision_devices.py -w' currently has no way of being invoked now. To wipe all devices, you must also provision them. Additionally, this method will wipe all attached devices even when a single device is provided '-d'. This violates previous behavior and seems very undesirable. Since this script is multi-purposed, this can be recovered by adding subcommands: ./provision_devices.py wipe-all ./provision_devices.py provision [-d device] [--skip-wipe] Alternatively, you can re-purpose the script to just be the provision/wipe case if the 'wipe all w/out provisioning' behavior was never used, but please update the CL to state this. Finally, if you do re-purpose the script to remove the 'wipe all w/out provisioning' behavior, you should still only wipe the device specified with '-d' if one is specified. https://codereview.chromium.org/334503002/diff/1/build/android/provision_devi... build/android/provision_devices.py:182: device_utils.DeviceUtils.parallel(devices).old_interface.Reboot(True) You should block w/ 'pFinish' like the previous code did to make sure they are all rebooting before you test to see if they're booted. Otherwise, you maybe could create a race condition where 'WaitUntilFullyBooted' succeeds because the device hasn't actually started rebooting yet. https://codereview.chromium.org/334503002/diff/1/build/android/provision_devi... build/android/provision_devices.py:186: device.WaitUntilFullyBooted(timeout=90) This 'WaitUntilFullyBooted' will be called twice in a row when provisioning. If this isn't expensive, that's cool, but I'd either remove this block (and let 'ProvisionDevices' handle it) or pass a flag into 'ProvisionDevices' that indicates that it should not also call 'WaitUntilFullyBooted'.
https://codereview.chromium.org/334503002/diff/1/build/android/provision_devi... File build/android/provision_devices.py (right): https://codereview.chromium.org/334503002/diff/1/build/android/provision_devi... build/android/provision_devices.py:182: device_utils.DeviceUtils.parallel(devices).old_interface.Reboot(True) On 2014/06/12 00:35:46, dnj wrote: > You should block w/ 'pFinish' like the previous code did to make sure they are > all rebooting before you test to see if they're booted. Otherwise, you maybe > could create a race condition where 'WaitUntilFullyBooted' succeeds because the > device hasn't actually started rebooting yet. The call to pFinish is vestigial. The call to Reboot will not return before the devices have started rebooting.
Thanks jbudorick and dnj. PTAL. https://codereview.chromium.org/334503002/diff/1/build/android/provision_devi... File build/android/provision_devices.py (right): https://codereview.chromium.org/334503002/diff/1/build/android/provision_devi... build/android/provision_devices.py:161: help='Don\'t wipe device data during provisioning.') On 2014/06/12 00:35:46, dnj wrote: > Just use quotes here; no need to add unnecessary escaping. Done. https://codereview.chromium.org/334503002/diff/1/build/android/provision_devi... build/android/provision_devices.py:176: devices = android_commands.GetAttachedDevices() On 2014/06/12 00:35:46, dnj wrote: > The old method had an explicit 'wipe' command that wiped all attached devices > and then quit. This was a single execution; people would run the script > explicitly to wipe devices. The old behavior of './provision_devices.py -w' > currently has no way of being invoked now. To wipe all devices, you must also > provision them. > > Additionally, this method will wipe all attached devices even when a single > device is provided '-d'. This violates previous behavior and seems very > undesirable. > > Since this script is multi-purposed, this can be recovered by adding > subcommands: > ./provision_devices.py wipe-all > ./provision_devices.py provision [-d device] [--skip-wipe] > > Alternatively, you can re-purpose the script to just be the provision/wipe case > if the 'wipe all w/out provisioning' behavior was never used, but please update > the CL to state this. > > Finally, if you do re-purpose the script to remove the 'wipe all w/out > provisioning' behavior, you should still only wipe the device specified with > '-d' if one is specified. Done. Good catch. The wipe without provisioning should not be necessary anymore after this change lands. I don't think anyone other than me (and maybe you dnj) ever used the wipe option. There is no convincing reason to wipe a bot device, which effectively un-provisions, without re-provisioning the device. Thus, I have re-purposed the script and fixed it to only wipe and provision the one device if -d is specified. https://codereview.chromium.org/334503002/diff/1/build/android/provision_devi... build/android/provision_devices.py:182: device_utils.DeviceUtils.parallel(devices).old_interface.Reboot(True) On 2014/06/12 00:51:30, jbudorick wrote: > On 2014/06/12 00:35:46, dnj wrote: > > You should block w/ 'pFinish' like the previous code did to make sure they are > > all rebooting before you test to see if they're booted. Otherwise, you maybe > > could create a race condition where 'WaitUntilFullyBooted' succeeds because > the > > device hasn't actually started rebooting yet. > > The call to pFinish is vestigial. The call to Reboot will not return before the > devices have started rebooting. Leaving as is. https://codereview.chromium.org/334503002/diff/1/build/android/provision_devi... build/android/provision_devices.py:186: device.WaitUntilFullyBooted(timeout=90) On 2014/06/12 00:35:46, dnj wrote: > This 'WaitUntilFullyBooted' will be called twice in a row when provisioning. If > this isn't expensive, that's cool, but I'd either remove this block (and let > 'ProvisionDevices' handle it) or pass a flag into 'ProvisionDevices' that > indicates that it should not also call 'WaitUntilFullyBooted'. Done. It's not expensive, but I removed the one in ProvisionDevices. I think it makes more sense to have it here, after the reboot, rather in in the function which does not necessarily reboot.
https://codereview.chromium.org/334503002/diff/20001/build/android/provision_... File build/android/provision_devices.py (right): https://codereview.chromium.org/334503002/diff/20001/build/android/provision_... build/android/provision_devices.py:190: ProvisionDevices(options) Sorry, I should have noticed this last time. You are calling 'GetAttachedDevices' twice (in the 'not skip_wipe' case), which could potentially lead to inconsistent results if a device is added or removed during the 'wipe'. Minimally, it is extra 'adb' command-line invocation. I'd update 'ProvisionDevices' to accept a 'devices' argument and forward the one that you constructed here so the set of devices that you work with is consistent. This would also save you from having to recreate the logic to build the 'devices' array.
https://codereview.chromium.org/334503002/diff/20001/build/android/provision_... File build/android/provision_devices.py (right): https://codereview.chromium.org/334503002/diff/20001/build/android/provision_... build/android/provision_devices.py:190: ProvisionDevices(options) On 2014/06/12 01:14:02, dnj wrote: > Sorry, I should have noticed this last time. You are calling > 'GetAttachedDevices' twice (in the 'not skip_wipe' case), which could > potentially lead to inconsistent results if a device is added or removed during > the 'wipe'. Minimally, it is extra 'adb' command-line invocation. > > I'd update 'ProvisionDevices' to accept a 'devices' argument and forward the one > that you constructed here so the set of devices that you work with is > consistent. This would also save you from having to recreate the logic to build > the 'devices' array. Either this or move the wiping logic into ProvisionDevices, since we're now always calling it.
https://codereview.chromium.org/334503002/diff/20001/build/android/provision_... File build/android/provision_devices.py (right): https://codereview.chromium.org/334503002/diff/20001/build/android/provision_... build/android/provision_devices.py:190: ProvisionDevices(options) On 2014/06/12 01:20:10, jbudorick wrote: > On 2014/06/12 01:14:02, dnj wrote: > > Sorry, I should have noticed this last time. You are calling > > 'GetAttachedDevices' twice (in the 'not skip_wipe' case), which could > > potentially lead to inconsistent results if a device is added or removed > during > > the 'wipe'. Minimally, it is extra 'adb' command-line invocation. > > > > I'd update 'ProvisionDevices' to accept a 'devices' argument and forward the > one > > that you constructed here so the set of devices that you work with is > > consistent. This would also save you from having to recreate the logic to > build > > the 'devices' array. > > Either this or move the wiping logic into ProvisionDevices, since we're now > always calling it. Done. Good suggestions.
lgtm
lgtm FYI my CL landed, so you'll probably see a merge conflict. https://codereview.chromium.org/334503002/diff/40001/build/android/provision_... File build/android/provision_devices.py (right): https://codereview.chromium.org/334503002/diff/40001/build/android/provision_... build/android/provision_devices.py:137: device_utils.DeviceUtils.parallel(devices).old_interface.Reboot(True) This should just be device_utils.DeviceUtils.parallel(devices).Reboot(True) when you merge.
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/334503002/60001
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
On 2014/06/12 19:03:53, I haz the power (commit-bot) wrote: > No LGTM from a valid reviewer yet. Only full committers are accepted. > Even if an LGTM may have been provided, it was from a non-committer or > a provisional committer, _not_ a full super star committer. > See http://www.chromium.org/getting-involved/become-a-committer > Note that this has nothing to do with OWNERS files. +craigdh for an actual 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/334503002/60001
Message was sent while issue was closed.
Change committed as 276866 |
