Fix last_devices to be quieter, and improve device affinity.

build/android/buildbot/bb_device_status_check.py

 #!/usr/bin/env python
 #
 # Copyright 2013 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """A class to keep track of devices across builds and report state."""
 import json
 import logging
 import optparse
@@ skipping 95 matching lines @@
   if (device_product_name == 'mantaray' and
       battery_info.get('AC powered', None) != 'true'):
     errors += ['Mantaray device not connected to AC power.']
 
   full_report = '\n'.join(report)
 
   return (device_type, device_build, battery_level, full_report, errors,
     dev_good, json_data)
 
 
-def CheckForMissingDevices(options, adb_online_devs):
+def CheckForMissingDevices(options, adb_online_devices):

[jbudorick, 2015/05/23 01:06:49]

rebase, I changed this function a few days ago.

[luqui, 2015/05/27 20:01:11]

Done.

   """Uses file of previous online devices to detect broken phones.
 
   Args:
     options: out_dir parameter of options argument is used as the base
              directory to load and update the cache file.
-    adb_online_devs: A list of serial numbers of the currently visible
-                     and online attached devices.
+    adb_online_devices: A list of serial numbers of the currently visible
+                        and online attached devices.
   """
   # TODO(navabi): remove this once the bug that causes different number
   # of devices to be detected between calls is fixed.
   logger = logging.getLogger()
   logger.setLevel(logging.INFO)
 
   out_dir = os.path.abspath(options.out_dir)
 
-  # last_devices denotes all known devices prior to this run
+  # last_devices denotes all known devices since the last time a new device was
+  # detected
   last_devices_path = os.path.join(out_dir, device_list.LAST_DEVICES_FILENAME)
-  last_missing_devices_path = os.path.join(out_dir,
-      device_list.LAST_MISSING_DEVICES_FILENAME)
   try:
-    last_devices = device_list.GetPersistentDeviceList(last_devices_path)
+    last_devices = device_list.GetOfflineDeviceMap(last_devices_path)
   except IOError:
     # Ignore error, file might not exist
-    last_devices = []
+    last_devices = {}
 
-  try:
-    last_missing_devices = device_list.GetPersistentDeviceList(
-        last_missing_devices_path)
-  except IOError:
-    last_missing_devices = []
+  # Increment the count of any missing devices.

[jbudorick, 2015/05/23 01:06:49]

  last_devices = dict(
      (k, 0 if k in adb_online_devices else last_devices[k] + 1)
      for k, v in last_devices.iteritems())

?

[luqui, 2015/05/27 20:01:12]

Yeah that was a bit clumsy.  Went with a more verbose version of this
suggestion.

+  for k in last_devices.keys():
+    if k not in adb_online_devices:
+      last_devices[k] += 1
 
-  missing_devs = list(set(last_devices) - set(adb_online_devs))
-  new_missing_devs = list(set(missing_devs) - set(last_missing_devices))
+  # Reset the count of any present devices, and find new devices.
+  for k in adb_online_devices:
+    if k in last_devices:
+      last_devices[k] = 0
 
-  if new_missing_devs and os.environ.get('BUILDBOT_SLAVENAME'):
-    logging.info('new_missing_devs %s' % new_missing_devs)
-    devices_missing_msg = '%d devices not detected.' % len(missing_devs)
-    bb_annotations.PrintSummaryText(devices_missing_msg)
+  missing_devices = { k: v for k, v in last_devices.iteeritems() if v != 0 }

[jbudorick, 2015/05/23 01:06:48]

- iteeritems -> iteritems
- if not v
- nit: no spaces at the ends of comprehensions

[luqui, 2015/05/27 20:01:12]

Done, except for "not v" -- mixing up numberness and truthiness here is too
conceptually punny for me

+  if missing_devices:
+    logging.info('Missing devices: %s' % missing_devices)
 
-    from_address = 'chrome-bot@chromium.org'
-    to_addresses = ['chrome-labs-tech-ticket@google.com',
-                    'chrome-android-device-alert@google.com']
-    cc_addresses = ['chrome-android-device-alert@google.com']
-    subject = 'Devices offline on %s, %s, %s' % (
-      os.environ.get('BUILDBOT_SLAVENAME'),
-      os.environ.get('BUILDBOT_BUILDERNAME'),
-      os.environ.get('BUILDBOT_BUILDNUMBER'))
-    msg = ('Please reboot the following devices:\n%s' %
-           '\n'.join(map(str, new_missing_devs)))
-    SendEmail(from_address, to_addresses, cc_addresses, subject, msg)
+  # Warn about devices that are missing once, but take no further action.
+  # This is because sometimes devices are still rebooting when we check.
+  once_missing = [ k for k, v in last_devices.iteritems() if v == 1 ]
+  if once_missing:
+    bb_annotations.PrintSummaryText(
+      '%d devices missing since last run' % len(once_missing))
 
-  all_known_devices = list(set(adb_online_devs) | set(last_devices))
-  device_list.WritePersistentDeviceList(last_devices_path, all_known_devices)
-  device_list.WritePersistentDeviceList(last_missing_devices_path, missing_devs)
+  # Send an email for twice missing devices.  This indicates a real problem.
+  twice_missing = [ k for k, v in last_devices.iteritems() if v == 2 ]
+  if twice_missing:
+    bb_annotations.PrintSummaryText(
+      '%s devices missing for two runs -- notifying' % len(twice_missing))
+    if os.environ.get('BUILDBOT_SLAVENAME'):
+      from_address = 'chrome-bot@chromium.org'
+      to_addresses = ['chrome-labs-tech-ticket@google.com',
+                      'chrome-android-device-alert@google.com']
+      cc_addresses = ['chrome-android-device-alert@google.com']
+      subject = 'Devices offline on %s, %s, %s' % (
+        os.environ.get('BUILDBOT_SLAVENAME'),
+        os.environ.get('BUILDBOT_BUILDERNAME'),
+        os.environ.get('BUILDBOT_BUILDNUMBER'))
+      msg = ('Please reboot the following devices:\n%s' %
+             '\n'.join(map(str, twice_missing)))
+      SendEmail(from_address, to_addresses, cc_addresses, subject, msg)
 
-  if not all_known_devices:
-    # This can happen if for some reason the .last_devices file is not
-    # present or if it was empty.
-    return ['No online devices. Have any devices been plugged in?']
-  if missing_devs:
-    devices_missing_msg = '%d devices not detected.' % len(missing_devs)
-    bb_annotations.PrintSummaryText(devices_missing_msg)
+  quite_missing = [ k for k, v in last_devices.iteritems() if v == 3 ]

[jbudorick, 2015/05/23 01:06:49]

if v > 2

?

[luqui, 2015/05/27 20:01:11]

Done.

+  if quite_missing:
+    bb_annotations.PrintSummaryTest(
+      '%s devices missing for more than two runs' % len(quite_missing))
 
+  if not adb_online_devices:
+      # This can happen if for some reason the .last_devices file is not
+      # present or if it was empty.
+      return ['No online devices. Have any devices been plugged in?']
+  if missing_devices:
     # TODO(navabi): Debug by printing both output from GetCmdOutput and
     # GetAttachedDevices to compare results.
-    crbug_link = ('https://code.google.com/p/chromium/issues/entry?summary='
-                  '%s&comment=%s&labels=Restrict-View-Google,OS-Android,Infra' %
-                  (urllib.quote('Device Offline'),
-                   urllib.quote('Buildbot: %s %s\n'
-                                'Build: %s\n'
-                                '(please don\'t change any labels)' %
-                                (os.environ.get('BUILDBOT_BUILDERNAME'),
-                                 os.environ.get('BUILDBOT_SLAVENAME'),
+    crbug_link = (
+        'https://code.google.com/p/chromium/issues/entry?summary='
+        '%s&comment=%s&labels=Restrict-View-Google,OS-Android,Infra' %
+        (urllib.quote('Device Offline'),
+         urllib.quote('Buildbot: %s %s\n'
+                      'Build: %s\n'
+                      '(please don\'t change any labels)' %
+                      (os.environ.get('BUILDBOT_BUILDERNAME'),
+                       os.environ.get('BUILDBOT_SLAVENAME'),
                                  os.environ.get('BUILDBOT_BUILDNUMBER')))))
-    return ['Current online devices: %s' % adb_online_devs,
-            '%s are no longer visible. Were they removed?\n' % missing_devs,
+    return ['Current online devices: %s' % adb_online_devices,
+            '%s are no longer visible. Were they removed?\n' %
+                missing_devices.keys(),
             'SHERIFF:\n',
             '@@@STEP_LINK@Click here to file a bug@%s@@@\n' % crbug_link,
             'Cache file: %s\n\n' % last_devices_path,
             'adb devices: %s' % GetCmdOutput(['adb', 'devices']),
-            'adb devices(GetAttachedDevices): %s' % adb_online_devs]
+            'adb devices(GetAttachedDevices): %s' % adb_online_devices]
   else:
-    new_devs = set(adb_online_devs) - set(last_devices)
-    if new_devs and os.path.exists(last_devices_path):
+    new_devices = [ k for k in adb_online_devices if k not in last_devices ]
+    if new_devices and os.path.exists(last_devices_path):
       bb_annotations.PrintWarning()
       bb_annotations.PrintSummaryText(
-          '%d new devices detected' % len(new_devs))
+          '%d new devices detected' % len(new_devices))
       print ('New devices detected %s. And now back to your '
-             'regularly scheduled program.' % list(new_devs))
+             'regularly scheduled program.' % list(new_devices))
+      # Reset last_devices since we have probably seen admin intervention, so
+      # we don't keep warning about the same old stuff.
+      last_devices = { k: 0 for k in adb_online_devices }
+
+  device_list.WriteOfflineDeviceMap(last_devices_path, last_devices)
 
 
 def SendEmail(from_address, to_addresses, cc_addresses, subject, msg):
   msg_body = '\r\n'.join(['From: %s' % from_address,
                           'To: %s' % ', '.join(to_addresses),
                           'CC: %s' % ', '.join(cc_addresses),
                           'Subject: %s' % subject, '', msg])
   try:
     server = smtplib.SMTP('localhost')
     server.sendmail(from_address, to_addresses, msg_body)
@@ skipping 74 matching lines @@
                     help='Output JSON information into a specified file.')
 
   options, args = parser.parse_args()
   if args:
     parser.error('Unknown options %s' % args)
 
   # Remove the last build's "bad devices" before checking device statuses.
   device_blacklist.ResetBlacklist()
 
   try:
-    expected_devices = device_list.GetPersistentDeviceList(
-        os.path.join(options.out_dir, device_list.LAST_DEVICES_FILENAME))
+    last_devices_path = os.path.join(
+        options.out_dir, device_list.LAST_DEVICES_FILENAME)
+    expected_devices = device_list.GetOfflineDeviceMap(
+        last_devices_path).keys()
   except IOError:
     expected_devices = []
   devices = android_commands.GetAttachedDevices()
   # Only restart usb if devices are missing.
   if set(expected_devices) != set(devices):
     print 'expected_devices: %s, devices: %s' % (expected_devices, devices)
     KillAllAdb()
     retries = 5
     usb_restarted = True
     if options.restart_usb:
@@ skipping 72 matching lines @@
 
   if num_failed_devs == len(devices):
     return 2
 
   if not devices:
     return 1
 
 
 if __name__ == '__main__':
   sys.exit(main())