Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12382006:
Check if mantarays are charging, and alert on all Android device errors. (Closed)
|
Created:
7 years, 9 months ago by Siva Chandra Modified:
7 years, 9 months ago CC:
chromium-reviews, klundberg+watch_chromium.org, frankf+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy+watch_chromium.org, cmp Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionCheck if mantarays are charging, and alert on all Android device errors.
BUG=168400
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=189091
Patch Set 1 #
Total comments: 12
Patch Set 2 : Fixed ilevy's comments and returning with error on missing devices #Patch Set 3 : Flunk on missing devices #Patch Set 4 : Fix style #
Total comments: 14
Patch Set 5 : Addressing another round of comments #Patch Set 6 : Address ilevy's comments #Patch Set 7 : #
Total comments: 14
Patch Set 8 : #Patch Set 9 : Dont flunk on missing device #Patch Set 10 : #Patch Set 11 : Some cleanup #Patch Set 12 : Made a string a list of lines #Patch Set 13 : Addressed all of ilevy's comments #
Total comments: 1
Patch Set 14 : Fixed the last nit #Messages
Total messages: 16 (0 generated)
https://chromiumcodereview.appspot.com/12382006/diff/1/build/android/device_s... File build/android/device_status_check.py (right): https://chromiumcodereview.appspot.com/12382006/diff/1/build/android/device_s... build/android/device_status_check.py:46: ac_power = re.findall('AC powered: (\w+)', battery)[0] This will throw an exception if the regex does not match. Does the battery code always have 'AC powered:' section? https://chromiumcodereview.appspot.com/12382006/diff/1/build/android/device_s... build/android/device_status_check.py:66: if device_product_name == 'mantaray' and ac_power == 'false': I would prefer a positive check, i.e. ac_power != 'true' https://chromiumcodereview.appspot.com/12382006/diff/1/build/android/device_s... build/android/device_status_check.py:69: warnings = [''.join(warnings)] What is this code for? https://chromiumcodereview.appspot.com/12382006/diff/1/build/android/device_s... build/android/device_status_check.py:139: err_msg = err Just create the message here and return directly https://chromiumcodereview.appspot.com/12382006/diff/1/build/android/device_s... build/android/device_status_check.py:156: return remove 155-156. Don't call this function if you don't want a message sent. https://chromiumcodereview.appspot.com/12382006/diff/1/build/android/device_s... build/android/device_status_check.py:183: types, builds, reports, warnings, errors = [], [], [], [], [] It's not clear to me that we need a new class of alerts. Maybe we should turn everything into an 'error' and fix the spurious warning here http://goo.gl/afrNM (release-keys/user builds should be exempt from device setup check)
PTAL. Addressed ilevy@'s comments. Also, flunking build if devices are missing. This was discussed during the clank-infra meet last week and cmp@ and others wanted this. https://chromiumcodereview.appspot.com/12382006/diff/1/build/android/device_s... File build/android/device_status_check.py (right): https://chromiumcodereview.appspot.com/12382006/diff/1/build/android/device_s... build/android/device_status_check.py:46: ac_power = re.findall('AC powered: (\w+)', battery)[0] On 2013/03/05 07:12:22, Isaac wrote: > This will throw an exception if the regex does not match. Does the battery code > always have 'AC powered:' section? As per xsdg@, yes. https://chromiumcodereview.appspot.com/12382006/diff/1/build/android/device_s... build/android/device_status_check.py:66: if device_product_name == 'mantaray' and ac_power == 'false': On 2013/03/05 07:12:22, Isaac wrote: > I would prefer a positive check, i.e. ac_power != 'true' Done. https://chromiumcodereview.appspot.com/12382006/diff/1/build/android/device_s... build/android/device_status_check.py:69: warnings = [''.join(warnings)] On 2013/03/05 07:12:22, Isaac wrote: > What is this code for? Now removed. https://chromiumcodereview.appspot.com/12382006/diff/1/build/android/device_s... build/android/device_status_check.py:139: err_msg = err On 2013/03/05 07:12:22, Isaac wrote: > Just create the message here and return directly Done. https://chromiumcodereview.appspot.com/12382006/diff/1/build/android/device_s... build/android/device_status_check.py:156: return On 2013/03/05 07:12:22, Isaac wrote: > remove 155-156. Don't call this function if you don't want a message sent. Done. https://chromiumcodereview.appspot.com/12382006/diff/1/build/android/device_s... build/android/device_status_check.py:183: types, builds, reports, warnings, errors = [], [], [], [], [] On 2013/03/05 07:12:22, Isaac wrote: > It's not clear to me that we need a new class of alerts. Maybe we should turn > everything into an 'error' and fix the spurious warning here http://goo.gl/afrNM > > (release-keys/user builds should be exempt from device setup check) Converted everything into an error.
Looking better but still too verbose. Comments inline. https://chromiumcodereview.appspot.com/12382006/diff/9001/build/android/build... File build/android/buildbot/bb_device_steps.py (right): https://chromiumcodereview.appspot.com/12382006/diff/9001/build/android/build... build/android/buildbot/bb_device_steps.py:242: RunCmd(['build/android/device_status_check.py'], flunk_on_failure=True) just remove this argument, it defaults to True https://chromiumcodereview.appspot.com/12382006/diff/9001/build/android/devic... File build/android/device_status_check.py (right): https://chromiumcodereview.appspot.com/12382006/diff/9001/build/android/devic... build/android/device_status_check.py:119: err = '\n'.join( Return this string directly, instead of making a One-Time-Used variable. https://chromiumcodereview.appspot.com/12382006/diff/9001/build/android/devic... build/android/device_status_check.py:170: missing_devices_msg = CheckForMissingDevices(options, devices) We don't need two variables (missing_devices_msg, deivce_info_err_msg). Just keep one. error_msg. You can then delete lines 171-176. https://chromiumcodereview.appspot.com/12382006/diff/9001/build/android/devic... build/android/device_status_check.py:187: if dev_errors: what is the purpose of this change?
PTAL at patch set 5. https://chromiumcodereview.appspot.com/12382006/diff/9001/build/android/build... File build/android/buildbot/bb_device_steps.py (right): https://chromiumcodereview.appspot.com/12382006/diff/9001/build/android/build... build/android/buildbot/bb_device_steps.py:242: RunCmd(['build/android/device_status_check.py'], flunk_on_failure=True) On 2013/03/13 02:54:11, Isaac wrote: > just remove this argument, it defaults to True Done. https://chromiumcodereview.appspot.com/12382006/diff/9001/build/android/devic... File build/android/device_status_check.py (right): https://chromiumcodereview.appspot.com/12382006/diff/9001/build/android/devic... build/android/device_status_check.py:119: err = '\n'.join( On 2013/03/13 02:54:11, Isaac wrote: > Return this string directly, instead of making a One-Time-Used variable. Done. https://chromiumcodereview.appspot.com/12382006/diff/9001/build/android/devic... build/android/device_status_check.py:170: missing_devices_msg = CheckForMissingDevices(options, devices) On 2013/03/13 02:54:11, Isaac wrote: > We don't need two variables (missing_devices_msg, deivce_info_err_msg). Just > keep one. error_msg. You can then delete lines 171-176. There are two variables because one of them should be preceded with a STEP_FAILURE, and the other one with a warning. We are treating only missing devices as a failure. All other errors will be STEP_WARNINGS, and will send out an alert message, but will not fail the step/flunk the build. Though I have not done what you suggested, I re-did a bit to make it look better. https://chromiumcodereview.appspot.com/12382006/diff/9001/build/android/devic... build/android/device_status_check.py:187: if dev_errors: On 2013/03/13 02:54:11, Isaac wrote: > what is the purpose of this change? We want to add to full_errors only if there were dev_errors. The immediate next line below, which is adding something like a section header, requires that. The line after that does not.
https://chromiumcodereview.appspot.com/12382006/diff/9001/build/android/devic... File build/android/device_status_check.py (right): https://chromiumcodereview.appspot.com/12382006/diff/9001/build/android/devic... build/android/device_status_check.py:170: missing_devices_msg = CheckForMissingDevices(options, devices) On 2013/03/13 20:58:15, Siva Chandra wrote: > On 2013/03/13 02:54:11, Isaac wrote: > > We don't need two variables (missing_devices_msg, deivce_info_err_msg). Just > > keep one. error_msg. You can then delete lines 171-176. > > There are two variables because one of them should be preceded with a > STEP_FAILURE, and the other one with a warning. We are treating only missing > devices as a failure. All other errors will be STEP_WARNINGS, and will send out > an alert message, but will not fail the step/flunk the build. > > Though I have not done what you suggested, I re-did a bit to make it look > better. Since we have turned on flunk_on_failure, the only information here you need is a non-zero error code. Let's save the error code to a separate variable and combine the strings. https://chromiumcodereview.appspot.com/12382006/diff/9001/build/android/devic... build/android/device_status_check.py:172: buildbot_report.PrintError() remove this -- not needed w/ non-zero error code. https://chromiumcodereview.appspot.com/12382006/diff/9001/build/android/devic... build/android/device_status_check.py:187: if dev_errors: On 2013/03/13 20:58:15, Siva Chandra wrote: > On 2013/03/13 02:54:11, Isaac wrote: > > what is the purpose of this change? > We want to add to full_errors only if there were dev_errors. The immediate next > line below, which is adding something like a section header, requires that. The > line after that does not. This code doesn't make sense to me. Can you explain more about what your'e trying to do here? I.e. what are you trying to print?
PTAL https://chromiumcodereview.appspot.com/12382006/diff/9001/build/android/devic... File build/android/device_status_check.py (right): https://chromiumcodereview.appspot.com/12382006/diff/9001/build/android/devic... build/android/device_status_check.py:170: missing_devices_msg = CheckForMissingDevices(options, devices) On 2013/03/13 21:13:49, Isaac wrote: > On 2013/03/13 20:58:15, Siva Chandra wrote: > > On 2013/03/13 02:54:11, Isaac wrote: > > > We don't need two variables (missing_devices_msg, deivce_info_err_msg). > Just > > > keep one. error_msg. You can then delete lines 171-176. > > > > There are two variables because one of them should be preceded with a > > STEP_FAILURE, and the other one with a warning. We are treating only missing > > devices as a failure. All other errors will be STEP_WARNINGS, and will send > out > > an alert message, but will not fail the step/flunk the build. > > > > Though I have not done what you suggested, I re-did a bit to make it look > > better. > > Since we have turned on flunk_on_failure, the only information here you need is > a non-zero error code. Let's save the error code to a separate variable and > combine the strings. Done. https://chromiumcodereview.appspot.com/12382006/diff/9001/build/android/devic... build/android/device_status_check.py:172: buildbot_report.PrintError() On 2013/03/13 21:13:49, Isaac wrote: > remove this -- not needed w/ non-zero error code. Done. https://chromiumcodereview.appspot.com/12382006/diff/9001/build/android/devic... build/android/device_status_check.py:187: if dev_errors: On 2013/03/13 21:13:49, Isaac wrote: > On 2013/03/13 20:58:15, Siva Chandra wrote: > > On 2013/03/13 02:54:11, Isaac wrote: > > > what is the purpose of this change? > > We want to add to full_errors only if there were dev_errors. The immediate > next > > line below, which is adding something like a section header, requires that. > The > > line after that does not. > > This code doesn't make sense to me. Can you explain more about what your'e > trying to do here? I.e. what are you trying to print? Since we are now using only err_msg, it looks slightly different, but the point is this. I want to print errors this way: <serial for device 1> errors: <error 1> <error 2> <serial for device 2> errors: <error 1> <error 2> So, the line which prints "<serial for device> errors:" should be executed only if there were dev_errors for that device. That is why I have the if statement.
Looks better, thanks. https://chromiumcodereview.appspot.com/12382006/diff/6003/build/android/devic... File build/android/device_status_check.py (right): https://chromiumcodereview.appspot.com/12382006/diff/6003/build/android/devic... build/android/device_status_check.py:60: errors = [] we're calling these errors but still treating them as warnings? I'm confused -- what did we decide in the email thread? https://chromiumcodereview.appspot.com/12382006/diff/6003/build/android/devic... build/android/device_status_check.py:63: if not setup_wizard_disabled: Any chance you could fix a different issue that's come up here? Phones with 'user' builds cannot be provisioned so one of our chromium.fyi bots has a permanently yellow device_status_check step. https://chromiumcodereview.appspot.com/12382006/diff/6003/build/android/devic... build/android/device_status_check.py:171: # Return a non-zero value only for missing devices. All other errors Is there some context on why we're treating this error differently -- i.e. turning step red instead of just emailing? I don't think a dropped device is necessarily cause for a red build. https://chromiumcodereview.appspot.com/12382006/diff/6003/build/android/devic... build/android/device_status_check.py:185: err_msg += '\n'.join('\t%s' % error for error in dev_errors) It'd be cleaner to keep err_msg as a list and do the '\n'.join right at the end. Also prefer spaces over tabs.
PTAL at patch set 8. https://chromiumcodereview.appspot.com/12382006/diff/6003/build/android/devic... File build/android/device_status_check.py (right): https://chromiumcodereview.appspot.com/12382006/diff/6003/build/android/devic... build/android/device_status_check.py:60: errors = [] On 2013/03/13 22:51:21, Isaac wrote: > we're calling these errors but still treating them as warnings? I'm confused -- > what did we decide in the email thread? Which email thread? Independent of it, I am looking at these as device errors about which we warn. They are not build errors. https://chromiumcodereview.appspot.com/12382006/diff/6003/build/android/devic... build/android/device_status_check.py:63: if not setup_wizard_disabled: On 2013/03/13 22:51:21, Isaac wrote: > Any chance you could fix a different issue that's come up here? Phones with > 'user' builds cannot be provisioned so one of our chromium.fyi bots has a > permanently yellow device_status_check step. No issues from my side, but two points to keep in mind: 1. Mixing up what this CL is doing. Already mixed up as we are adding a new device 'error'. 2. More rounds of review for this CL which would stop other 'features' from being useful immediately. Up to you. https://chromiumcodereview.appspot.com/12382006/diff/6003/build/android/devic... build/android/device_status_check.py:171: # Return a non-zero value only for missing devices. All other errors On 2013/03/13 22:51:21, Isaac wrote: > Is there some context on why we're treating this error differently -- i.e. > turning step red instead of just emailing? I don't think a dropped device is > necessarily cause for a red build. In the infra meet last week, cmp@ asked to stop the build on missing devices so that troopers can address it immediately. At least, that is what I have understood. And, I think the reason was that an email alert was easy to miss. https://chromiumcodereview.appspot.com/12382006/diff/6003/build/android/devic... build/android/device_status_check.py:185: err_msg += '\n'.join('\t%s' % error for error in dev_errors) On 2013/03/13 22:51:21, Isaac wrote: > It'd be cleaner to keep err_msg as a list and do the '\n'.join right at the end. > > Also prefer spaces over tabs. Using space instead of tab, but I did not change it to a list as err_msg is a string to begin with in this block.
PING
https://chromiumcodereview.appspot.com/12382006/diff/6003/build/android/devic... File build/android/device_status_check.py (right): https://chromiumcodereview.appspot.com/12382006/diff/6003/build/android/devic... build/android/device_status_check.py:63: if not setup_wizard_disabled: I agree with both. That would be a very useful followup CL, but I don't think we should hold this back for that change. Your choice Siva, as to what you decide to do about user builds. https://chromiumcodereview.appspot.com/12382006/diff/6003/build/android/devic... build/android/device_status_check.py:171: # Return a non-zero value only for missing devices. All other errors I'd like to discuss this further. I would prefer we don't stop the the build for a single missing device, especially since Siva and I are the only clank advisors in MTV now. Otherwise, we will be forced to run down to the lab too often.
https://chromiumcodereview.appspot.com/12382006/diff/6003/build/android/devic... File build/android/device_status_check.py (right): https://chromiumcodereview.appspot.com/12382006/diff/6003/build/android/devic... build/android/device_status_check.py:63: if not setup_wizard_disabled: On 2013/03/13 23:13:05, Siva Chandra wrote: > On 2013/03/13 22:51:21, Isaac wrote: > > Any chance you could fix a different issue that's come up here? Phones with > > 'user' builds cannot be provisioned so one of our chromium.fyi bots has a > > permanently yellow device_status_check step. > > No issues from my side, but two points to keep in mind: > 1. Mixing up what this CL is doing. Already mixed up as we are adding a new > device 'error'. > 2. More rounds of review for this CL which would stop other 'features' from > being useful immediately. > > Up to you. Forget it, I'll put it on my list. https://chromiumcodereview.appspot.com/12382006/diff/6003/build/android/devic... build/android/device_status_check.py:125: 'adb devices(GetAttachedDevices): %s\n' % don't add trailing \n here. That's doing something that the outer function should be handling. https://chromiumcodereview.appspot.com/12382006/diff/6003/build/android/devic... build/android/device_status_check.py:171: # Return a non-zero value only for missing devices. All other errors On 2013/03/18 20:29:33, navabi wrote: > I'd like to discuss this further. I would prefer we don't stop the the build for > a single missing device, especially since Siva and I are the only clank advisors > in MTV now. Otherwise, we will be forced to run down to the lab too often. Take this out; we cannot fail when a device goes offline. That goes against our basis of adding extra redundant devices to make the build more stable. More devices == more chance a single device goes offline == main waterfall bot will become flaky. If troopers want to address phones quickly they can subscribe to the alerts listserv. https://chromiumcodereview.appspot.com/12382006/diff/6003/build/android/devic... build/android/device_status_check.py:185: err_msg += '\n'.join('\t%s' % error for error in dev_errors) On 2013/03/13 23:13:05, Siva Chandra wrote: > On 2013/03/13 22:51:21, Isaac wrote: > > It'd be cleaner to keep err_msg as a list and do the '\n'.join right at the > end. > > > > Also prefer spaces over tabs. > > Using space instead of tab, but I did not change it to a list as err_msg is a > string to begin with in this block. Right, you'd need to change all uses of err_msg. This is a new variable and best practice is to join at the very end (as was done before). CheckForMissingDevices would return a list instead of a string, and you would do the '\n'.join on line 189.
PTAL at patch set 13. All comments addressed.
lgtm w/ nit https://chromiumcodereview.appspot.com/12382006/diff/32001/build/android/devi... File build/android/device_status_check.py (right): https://chromiumcodereview.appspot.com/12382006/diff/32001/build/android/devi... build/android/device_status_check.py:178: err_msg += ['%s errors:\n' % serial] remove trailing \n
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sivachandra@chromium.org/12382006/38001
Message was sent while issue was closed.
Change committed as 189091 |
