Create daemon to monitor android device temperatures

scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py

Index: scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py
diff --git a/scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py b/scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py
new file mode 100755
index 0000000000000000000000000000000000000000..a8a4e92ae0a0802f2b0eaea0372ee128d421da2c
--- /dev/null
+++ b/scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py
@@ -0,0 +1,118 @@
+#!/usr/bin/env python
+
+"""
+Launches a daemon to monitor the devices' temperatures by
+uploading them to monarch every 30 seconds.
+"""
+
+import json
+import logging
+import os
+import optparse
+import re
+import signal
+import subprocess
+import sys
+import time
+
+# Common name of sensor found in nexus devices to measure cpu (core0) temp
+_CPU_TEMP_SENSOR = 'tsens_tz_sensor0'
+
+# Location of the infra-python package's run script
+_RUN_PY = '/opt/infra-python/run.py'

[luqui, 2015/09/03 01:10:41]

We can't rely on this location.  Probably have to take it as an arg or env var.

[bpastene, 2015/09/04 03:47:21]

Per offline discussion with Luke, resolving this would be too much of a hassle
at the moment. Adding a todo once it becomes easier/necessary to change.

+
+class SigtermError(Exception):
+  """Exception used to catch a sigterm."""
+  pass
+
+def main(adb_path,
+         devices_json,
+         metric_prefix="android_device",
+         pid_file_path='/tmp/device_monitor_pid'):
+  """Polls the devices for their battery and cpu temperatures
+  every 30 seconds and uploads them to monarch through infra's
+  ts_mon. Fully qualified, the metric names would be
+  /chrome/infra/${metric_prefix}/${device_serial}/(battery|cpu)_temp
+
+  Args:
+    adb_path: Path to adb binary
+    devices_json: Json list of device serials to poll
+    metric_prefix: Prefix of the metric name.
+    pid_file_path: Path to the file to write the pid to
+  """
+
+  def SigtermHandler(_signum, _unused_frame):
+    raise SigtermError()
+  signal.signal(signal.SIGTERM, SigtermHandler)
+
+  if pid_file_path:

[luqui, 2015/09/03 01:10:41]

This whole block and the kill script have nothing to do with monitoring.  Maybe
you could roll this as a feature of daemonizer, to keep this script to the point
and enable future things in its style.

[bpastene, 2015/09/03 17:36:00]

I was trying to emulate the style of the one script that uses daemonizer
(adb_logcat_monitor), but I agree. It would be cleaner to move all that into
daemonizer; I'll look into it.

[bpastene, 2015/09/04 03:47:21]

Uploaded a cl for the changes to daemonizer here:
https://codereview.chromium.org/1328623004/

+    # If an old monitor is still running, kill it. Its list of
+    # devices to poll might be outdated
+    try:
+      with open(pid_file_path, 'r') as pid_file:
+        monitor_pid = int(pid_file.readline())
+      os.kill(monitor_pid, signal.SIGTERM)
+    except (IOError, OSError, ValueError):
+      logging.exception("Unable to kill old monitoring process")
+    try:
+      with open(pid_file_path, 'w+') as pid_file:
+        pid_file.write('%s' % str(os.getpid()))
+    except (IOError):
+      logging.exception("Unable to write pid to file")
+
+  devices = json.loads(devices_json)
+  try:
+    while True:
+      upload_cmd_args = []
+      for device in devices:
+        bat_temp = None
+        cpu_temp = None
+        # Search for the file that the _CPU_TEMP_SENSOR dumps to and cat it
+        cmd = [adb_path, '-s', device, 'shell',
+               'grep "%s" /sys/class/thermal/thermal_zone*/type -l'

[luqui, 2015/09/03 01:10:41]

Stylistically I usually see options first: grep -l ...

[jbudorick, 2015/09/03 17:37:31]

Is this true across all Nexus devices, or just specific models?

We've seen variance in this type of thing across different models in the past.

[bpastene, 2015/09/04 03:47:21]

I'm not sure. All the devices I've come across on the many waterfalls follow
this layout & naming scheme. Is there a better way? If the device in question
doesn't successfully report the cpu temp, we'd still get the battery temp
(assuming 'dumpsys battery' hasn't changed too), so there'd still be thermal
metrics to monitor/alarm on.

[bpastene, 2015/09/04 03:47:21]

Done.

+                 % (_CPU_TEMP_SENSOR)]
+        try:
+          cpu_temp_files = subprocess.check_output(cmd)
+          if (len(cpu_temp_files.splitlines()) == 1):
+              cpu_temp_file = cpu_temp_files.strip()[:-4] + "temp"

[luqui, 2015/09/03 01:10:41]

This is opaque. Can you use re.sub() instead so I know what you're slicing off?

[bpastene, 2015/09/04 03:47:21]

Done.

+              cmd = [adb_path, '-s', device, 'shell',

[jbudorick, 2015/09/03 17:37:31]

Someday I will get infra to use something other than raw, unchecked adb calls.
That day is blocked on getting our more sane utilities somewhere that infra can
use them, though, so for now, you're stuck with this.

+                     'cat %s' % (cpu_temp_file)]
+              file_contents = subprocess.check_output(cmd)
+              cpu_temp = int(file_contents)
+        except (subprocess.CalledProcessError, TypeError, ValueError):
+          cpu_temp = None
+
+        # Dump system battery info and grab the temp

[jbudorick, 2015/09/03 17:37:31]

and as an added bonus, you'd get this for free:
https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli...

+        cmd = [adb_path, '-s', device, 'shell', 'dumpsys battery']
+        try:
+          battery_info = subprocess.check_output(cmd)
+          for line in battery_info.splitlines():
+            m = re.match('^\s*temperature: ([0-9]+)\s*$', line)
+            if m:
+              bat_temp = int(m.group(1))
+        except (subprocess.CalledProcessError, TypeError, ValueError):
+          bat_temp = None
+
+        cpu_dict = {'name': "%s/%s/cpu_temp" % (metric_prefix, device),
+                    'value': cpu_temp}
+        cpu_temp_args = ['--float', json.dumps(cpu_dict)] if cpu_temp else []
+        battery_dict = {'name': '%s/%s/battery_temp' % (metric_prefix,
+                        device), 'value': bat_temp}
+        bat_temp_args = ['--float', 
+                         json.dumps(battery_dict)] if bat_temp else []
+        upload_cmd_args = upload_cmd_args + cpu_temp_args + bat_temp_args

[luqui, 2015/09/03 01:10:41]

+=

It took me a while to realize the point of this loop is to build
upload_cmd_args.  Can you factor it out so it's:

    upload_cmd_args = []
    for device in devices:
       upload_cmd_args += get_device_args(device)

[bpastene, 2015/09/04 03:47:21]

Done.

+
+      cmd = [_RUN_PY, 'infra.tools.send_ts_mon_values'] + upload_cmd_args
+      try:
+        subprocess.Popen(cmd)
+      except OSError:
+        logging.exception('Unable to call %s', _RUN_PY)
+
+      time.sleep(30)
+  except SigtermError:
+    logging.info("Got a SIGTERM, shutting down")
+  except: # pylint: disable=bare-except
+    logging.exception('Unexpected exception in main.')
+
+if __name__ == '__main__':
+  sys.exit(main(*sys.argv[1:]))