Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11021004:
Make device_status_check.py email clank-infra if devices go offline. (Closed)
|
Created:
8 years, 2 months ago by navabi1 Modified:
8 years, 2 months ago CC:
chromium-reviews, peter+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMake device_status_check.py email clank-infra if devices go offline.
BUG=152583
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=159819
Patch Set 1 #
Total comments: 11
Patch Set 2 : Fixes and only email first time device goes offline. #
Total comments: 6
Patch Set 3 : Add num devices online and clean WriteDeviceList for missing devs. #
Total comments: 6
Patch Set 4 : Nits and write .last_devices outside of if/else block. #
Total comments: 1
Messages
Total messages: 17 (0 generated)
I tested by changing the clank infra email to my email and changing 'slave' to determine the bot name (see how CL determines the bot name by splitting cwd), and then running the script locally twice, unplugging an attached device between the two runs. I got an email with the device warning message.
looks good. I have a couple suggestions, we might need to pull in factory properties on the bot. https://codereview.chromium.org/11021004/diff/1/build/android/device_status_c... File build/android/device_status_check.py (right): https://codereview.chromium.org/11021004/diff/1/build/android/device_status_c... build/android/device_status_check.py:76: to_address = 'clank-infrastructure-team@google.com' Suggest we add a keyword to this to make it easier to filter, such as clank-infrastructure-team+devicealert@ https://codereview.chromium.org/11021004/diff/1/build/android/device_status_c... build/android/device_status_check.py:78: # Assumes name of bot is after .../build/slave/<BOT NAME>/... I think cleaner to either use environment variable BUILDBOT_BUILDERNAME or parse factory properties. https://codereview.chromium.org/11021004/diff/1/build/android/device_status_c... build/android/device_status_check.py:81: subject = 'Device not detected on %s' % bot_name Several of the testers have the same name (they're on different waterfalls), so maybe we can include the waterfall name in the email too. This means passing in $FACTORY_PROPERTIES to the build script (see bb_zip_build) and then parsing it. We're going to get a lot of messages. Maybe we can put enough info in the subject so that we can decide how important is. Something like: name, waterfall, how many devices left and how many total? https://codereview.chromium.org/11021004/diff/1/build/android/device_status_c... build/android/device_status_check.py:87: body = string.join(( nit: python will implicitly join strings that are next to each other. You can do multiple lines by surrounding with parens. Something like body = ( 'yada yada' 'yada yada' 'yada yada' % ( var1, var2, var3)) might be cleaner
BTW, please run this through gpylint -- the original version passes so you'll only get complaints about stuff you might have changed.
https://codereview.chromium.org/11021004/diff/1/build/android/device_status_c... File build/android/device_status_check.py (right): https://codereview.chromium.org/11021004/diff/1/build/android/device_status_c... build/android/device_status_check.py:76: to_address = 'clank-infrastructure-team@google.com' On 2012/09/29 03:17:04, Isaac wrote: > Suggest we add a keyword to this to make it easier to filter, such as > clank-infrastructure-team+devicealert@ You can ignore this earlier comment. But we can't use we can't use this listserv here. Use chromium-android-device-alerts [@] google.com
https://codereview.chromium.org/11021004/diff/1/build/android/device_status_c... File build/android/device_status_check.py (right): https://codereview.chromium.org/11021004/diff/1/build/android/device_status_c... build/android/device_status_check.py:76: to_address = 'clank-infrastructure-team@google.com' On 2012/09/29 03:33:08, Isaac wrote: > On 2012/09/29 03:17:04, Isaac wrote: > > Suggest we add a keyword to this to make it easier to filter, such as > > clank-infrastructure-team+devicealert@ > > You can ignore this earlier comment. But we can't use we can't use this > listserv here. Use > chromium-android-device-alerts [@] http://google.com > Done. https://codereview.chromium.org/11021004/diff/1/build/android/device_status_c... build/android/device_status_check.py:78: # Assumes name of bot is after .../build/slave/<BOT NAME>/... On 2012/09/29 03:17:04, Isaac wrote: > I think cleaner to either use environment variable BUILDBOT_BUILDERNAME or parse > factory properties. Done. https://codereview.chromium.org/11021004/diff/1/build/android/device_status_c... build/android/device_status_check.py:81: subject = 'Device not detected on %s' % bot_name On 2012/09/29 03:17:04, Isaac wrote: > Several of the testers have the same name (they're on different waterfalls), so > maybe we can include the waterfall name in the email too. This means passing in > $FACTORY_PROPERTIES to the build script (see bb_zip_build) and then parsing it. > > We're going to get a lot of messages. Maybe we can put enough info in the > subject so that we can decide how important is. Something like: name, waterfall, > how many devices left and how many total? Rather than pass FACTORY_PROPERTIES, I'll use the SLAVE_NAME which is part of the environment. That will uniquely identify which bot has missing devices. I think we can leave the number of devices that have went offline in the message text. https://codereview.chromium.org/11021004/diff/1/build/android/device_status_c... build/android/device_status_check.py:87: body = string.join(( On 2012/09/29 03:17:04, Isaac wrote: > nit: python will implicitly join strings that are next to each other. You can > do multiple lines by surrounding with parens. Something like > > body = ( > 'yada yada' > 'yada yada' > 'yada yada' % ( > var1, var2, var3)) > > might be cleaner I prefer to leave it like this. While it is cleaner the way you suggested, I think it is less clear, because it is less obvious which string variable corresponds to the format place holders. string.join is deprecated. changed to '\r\n'.join([...])
ilevy suggested to only email the first time a device drops offline. It's a good suggestion so we don't keep getting emails for a device that drops offline. I implemented this with a new cache file called .last_missing that keeps track of the last devices to go missing. If the missing devices are the same as the .last_missing devices, the warning is still printed but no email is sent.
https://chromiumcodereview.appspot.com/11021004/diff/1/build/android/device_s... File build/android/device_status_check.py (right): https://chromiumcodereview.appspot.com/11021004/diff/1/build/android/device_s... build/android/device_status_check.py:81: subject = 'Device not detected on %s' % bot_name On 2012/10/01 21:52:15, navabi1 wrote: > On 2012/09/29 03:17:04, Isaac wrote: > > Several of the testers have the same name (they're on different waterfalls), > so > > maybe we can include the waterfall name in the email too. This means passing > in > > $FACTORY_PROPERTIES to the build script (see bb_zip_build) and then parsing > it. > > > > We're going to get a lot of messages. Maybe we can put enough info in the > > subject so that we can decide how important is. Something like: name, > waterfall, > > how many devices left and how many total? > > Rather than pass FACTORY_PROPERTIES, I'll use the SLAVE_NAME which is part of > the environment. That will uniquely identify which bot has missing devices. > > I think we can leave the number of devices that have went offline in the message > text. sg https://chromiumcodereview.appspot.com/11021004/diff/1/build/android/device_s... build/android/device_status_check.py:87: body = string.join(( On 2012/10/01 21:52:15, navabi1 wrote: > On 2012/09/29 03:17:04, Isaac wrote: > > nit: python will implicitly join strings that are next to each other. You can > > do multiple lines by surrounding with parens. Something like > > > > body = ( > > 'yada yada' > > 'yada yada' > > 'yada yada' % ( > > var1, var2, var3)) > > > > might be cleaner > > I prefer to leave it like this. While it is cleaner the way you suggested, I > think it is less clear, because it is less obvious which string variable > corresponds to the format place holders. > > string.join is deprecated. changed to '\r\n'.join([...]) sg, was confused by the string.join https://chromiumcodereview.appspot.com/11021004/diff/8001/build/android/devic... File build/android/device_status_check.py (right): https://chromiumcodereview.appspot.com/11021004/diff/8001/build/android/devic... build/android/device_status_check.py:92: subject = 'Device not detected on %s, %s.' % (slave_name, bot_name) I would like len(adb_online_devs) in the subject, which will let us know the urgency of the email. https://chromiumcodereview.appspot.com/11021004/diff/8001/build/android/devic... build/android/device_status_check.py:113: last_missing = ReadDeviceList('.last_missing') Move line 113 to line 134. See comment below https://chromiumcodereview.appspot.com/11021004/diff/8001/build/android/devic... build/android/device_status_check.py:123: else: If a device goes offline immediately after we restart phones, this list will never get reset and we won't receive emails from the bot as devices drop offline again. How about always writing missing_devs to .last_missing and email if there are elements of missing devices not in .last_missing. The means if a offline device ever comes online we will get an email if it goes offline again.
https://chromiumcodereview.appspot.com/11021004/diff/8001/build/android/devic... File build/android/device_status_check.py (right): https://chromiumcodereview.appspot.com/11021004/diff/8001/build/android/devic... build/android/device_status_check.py:92: subject = 'Device not detected on %s, %s.' % (slave_name, bot_name) On 2012/10/01 22:35:45, Isaac wrote: > I would like len(adb_online_devs) in the subject, which will let us know the > urgency of the email. Done. https://chromiumcodereview.appspot.com/11021004/diff/8001/build/android/devic... build/android/device_status_check.py:113: last_missing = ReadDeviceList('.last_missing') On 2012/10/01 22:35:45, Isaac wrote: > Move line 113 to line 134. See comment below Done. https://chromiumcodereview.appspot.com/11021004/diff/8001/build/android/devic... build/android/device_status_check.py:123: else: On 2012/10/01 22:35:45, Isaac wrote: > If a device goes offline immediately after we restart phones, this list will > never get reset and we won't receive emails from the bot as devices drop offline > again. > > How about always writing missing_devs to .last_missing and email if there are > elements of missing devices not in .last_missing. The means if a offline device > ever comes online we will get an email if it goes offline again. Done.
lgtm with indentation fix https://chromiumcodereview.appspot.com/11021004/diff/12001/build/android/devi... File build/android/device_status_check.py (right): https://chromiumcodereview.appspot.com/11021004/diff/12001/build/android/devi... build/android/device_status_check.py:131: WriteDeviceList('.last_devices', (adb_online_devs + last_devices)) unindent this
https://chromiumcodereview.appspot.com/11021004/diff/12001/build/android/devi... File build/android/device_status_check.py (right): https://chromiumcodereview.appspot.com/11021004/diff/12001/build/android/devi... build/android/device_status_check.py:117: new_missing_devs = list(set(missing_devs) - set(last_missing)) nit: you could remove the outer list conversion here
actually two more suggestion, comments inline in the latest patch. Also, please send this to an android trybot so we can verify output. https://chromiumcodereview.appspot.com/11021004/diff/12001/build/android/devi... File build/android/device_status_check.py (right): https://chromiumcodereview.appspot.com/11021004/diff/12001/build/android/devi... build/android/device_status_check.py:100: body = '\r\n'.join( print was using \n. This patch using \r\n. Do you need that? Please make sure it doesn't change the output of device_status_report in buildbot.
https://chromiumcodereview.appspot.com/11021004/diff/12001/build/android/devi... File build/android/device_status_check.py (right): https://chromiumcodereview.appspot.com/11021004/diff/12001/build/android/devi... build/android/device_status_check.py:100: body = '\r\n'.join( On 2012/10/02 04:55:35, Isaac wrote: > print was using \n. This patch using \r\n. Do you need that? Please make sure > it doesn't change the output of device_status_report in buildbot. Done. I checked the output, and also changed it to '\n' from '\r\n' to be the same as it was before and then made sure the email formatting looked right. https://chromiumcodereview.appspot.com/11021004/diff/12001/build/android/devi... build/android/device_status_check.py:117: new_missing_devs = list(set(missing_devs) - set(last_missing)) On 2012/10/02 02:00:28, Isaac wrote: > nit: you could remove the outer list conversion here Done. https://chromiumcodereview.appspot.com/11021004/diff/12001/build/android/devi... build/android/device_status_check.py:131: WriteDeviceList('.last_devices', (adb_online_devs + last_devices)) On 2012/10/02 01:58:21, Isaac wrote: > unindent this Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/navabi@chromium.org/11021004/17001
Change committed as 159819
https://chromiumcodereview.appspot.com/11021004/diff/17001/build/android/devi... File build/android/device_status_check.py (right): https://chromiumcodereview.appspot.com/11021004/diff/17001/build/android/devi... build/android/device_status_check.py:103: 'SHERIFF: See go/clank/engineering/buildbots/troubleshooting', Why did you change this link? Please fix this, tomorrow.
Good catch. I did not change the link, I accidentally copied it from the downstream patch (that is the downstream address for the sheriff). I originally implemented this downstream (locally) and then moved the patch to my upstream tree. |
