Buildbot: Only archive core dumps from unexpected crashes.

tools/utils.py

Index: tools/utils.py
diff --git a/tools/utils.py b/tools/utils.py
index cf94961e5165843655b8d7b2e103605bf103f1cf..e0ade7327e43ff38286fa0c9b0f38f9528c7bfe1 100644
--- a/tools/utils.py
+++ b/tools/utils.py
@@ -674,9 +674,18 @@ class ChangedWorkingDirectory(object):
     print "Enter directory = ", self._old_cwd
     os.chdir(self._old_cwd)
 
-# This class finds and archives all core.* files from the current working
-# directory and all binaries copied by UnexpectedCrashDumpArchiver into
-# the current working directory (see tools/testing/dart/test_progress.dart).
+class CoreDump(object):
+  def __init__(self, test, core, binary):
+    self.test = test
+    self.core = core
+    self.binary = binary
+
+  def __str__(self):
+    return "%s: %s %s" % (self.test, self.binary, self.core)
+
+# This class reads coredumps file written by UnexpectedCrashDumpArchiver into
+# the current working directory and uploads all cores and binaries
+# listed in it into Cloud Storage (see tools/testing/dart/test_progress.dart).
 class CoreDumpArchiver(object):
   def __init__(self, args):
     self._enabled = '--copy-coredumps' in args and GuessOS() == 'linux'
@@ -689,11 +698,8 @@ class CoreDumpArchiver(object):
       return
 
     # Cleanup any stale coredumps
-    coredumps = self._find_coredumps()
-    if coredumps:
-      print "WARNING: Found stale coredumps, removing"
-      MarkCurrentStepWarning()
-      self._remove_coredumps(coredumps)
+    if self._cleanup():
+      print "WARNING: Found and removed stale coredumps"
 
     self._old_limits = resource.getrlimit(resource.RLIMIT_CORE)
 
@@ -705,48 +711,78 @@ class CoreDumpArchiver(object):
     if not self._enabled:
       return
 
-    # Restore old core limit.
-    resource.setrlimit(resource.RLIMIT_CORE, self._old_limits)
+    try:
+      # Restore old core limit.
+      resource.setrlimit(resource.RLIMIT_CORE, self._old_limits)
 
-    # Check that kernel was correctly configured to use core.%p
-    # core_pattern.
-    self._check_core_dump_pattern(fatal=True)
+      # Check that kernel was correctly configured to use core.%p
+      # core_pattern.
+      self._check_core_dump_pattern(fatal=True)
 
-    coredumps = self._find_coredumps()
-    if coredumps:
-      # If we get a ton of crashes, only archive 10 dumps.
-      archive_coredumps = coredumps[:10]
-      print 'Archiving coredumps: %s' % ', '.join(archive_coredumps)
-      sys.stdout.flush()
-      self._archive(archive_coredumps)
-      self._remove_coredumps(coredumps)
-    coredumps = self._find_coredumps()
-    assert not coredumps
+      coredumps = self._find_coredumps()
+      if coredumps:
+        # If we get a ton of crashes, only archive 10 dumps.
+        archive_coredumps = coredumps[:10]
+        print 'Archiving coredumps:'
+        for core in archive_coredumps:
+          print '----> %s' % core
 
-  def _find_coredumps(self):
-    return glob.glob(os.path.join(self._search_dir, 'core.*'))
+        sys.stdout.flush()
+        self._archive(archive_coredumps)
+
+    finally:
+      self._cleanup()
+
+  def _cleanup(self):
+    found = False
+    for core in glob.glob(os.path.join(self._search_dir, 'core.*')):
+      found = True
+      os.unlink(core)
+    for binary in glob.glob(os.path.join(self._search_dir, 'binary.*')):
+      found = True
+      os.unlink(binary)
+    try:
+      os.unlink(os.path.join(self._search_dir, 'coredumps'))
+      found = True
+    except:
+      pass
+
+    return found
 
-  def _remove_coredumps(self, coredumps):
-    for name in coredumps:
-      os.unlink(name)
+  # Load coredumps file. Each line has the following format:
+  #
+  #                  test-name,core-file,binary-file
+  #

[kustermann, 2017/01/20 14:56:55]

Normally python docs are like

def _find_core_dumps(self):
  """
  Load coredumps file ....
  """

(also above for the class)

[Vyacheslav Egorov (Google), 2017/01/20 16:44:45]

Done.

+  def _find_coredumps(self):
+    try:
+      with open('coredumps') as f:
+        return [CoreDump(*ln.strip('\n').split(',')) for ln in f.readlines()]
+    except:
+      return []
 
   def _archive(self, coredumps):
+    files = set()
+    for core in coredumps:
+      files.add(core.core)
+      files.add(core.binary)
+    self._upload(files)
+
+  def _upload(self, files):
     bot_utils = GetBotUtils()
     gsutil = bot_utils.GSUtil()
     storage_path = '%s/%s/' % (self._bucket, uuid.uuid4())
     gs_prefix = 'gs://%s' % storage_path
     http_prefix = 'https://storage.cloud.google.com/%s' % storage_path
 
-    for core in coredumps:
+    print '\n--- Uploading into %s (%s) ---' % (gs_prefix, http_prefix)
+    for core in files:
       # Sanitize the name: actual cores follow 'core.%d' pattern, crashed
       # binaries are copied next to cores and named 'core.<binary_name>'.

[kustermann, 2017/01/20 14:56:55]

The binaries are now named: "binary.${mode}_${arch}_${binBaseName}"

[Vyacheslav Egorov (Google), 2017/01/20 16:44:46]

Done.

-      suffix = os.path.basename(core).split('.')[1]
-      try:
-        # Check if suffix is an integer - in this case it's an actual core.
-        clean_name = 'core.%d' % int(suffix)
-      except:
-        # This is not a coredump but a crashed binary.
-        clean_name = suffix
+      parts = os.path.basename(core).split('.')

[kustermann, 2017/01/20 14:56:55]

Maybe simpler:

(prefix, rest) = 'a.b.c.d'.split('.', 1)
clean_name = rest if prefix == 'binary' else os.path.basename(core)

[Vyacheslav Egorov (Google), 2017/01/20 16:44:46]

Done.

+      if parts[0] == 'binary':
+        clean_name = '.'.join(parts[1:])
+      else:
+        clean_name = os.path.basename(core)
 
       tarname = '%s.tar.gz' % clean_name
 
@@ -762,15 +798,12 @@ class CoreDumpArchiver(object):
 
       try:
         gsutil.upload(tarname, gs_url)
-        print '@@@STEP_LOG_LINE@coredumps@%s (%s)@@@' % (gs_url, http_url)
+        print '+++ Uploaded %s (%s)' % (gs_url, http_url)
       except Exception as error:
-        message = "Failed to upload coredump %s, error: %s" % (tarname, error)
-        print '@@@STEP_LOG_LINE@coredumps@%s@@@' % message
+        print '!!! Failed to upload %s, error: %s' % (tarname, error)
 
       os.unlink(tarname)
-
-    print '@@@STEP_LOG_END@coredumps@@@'
-    MarkCurrentStepWarning()
+    print '--- Done ---\n'
 
   def _check_core_dump_pattern(self, fatal=False):
     core_pattern_file = '/proc/sys/kernel/core_pattern'
@@ -779,19 +812,15 @@ class CoreDumpArchiver(object):
     expected_core_pattern = 'core.%p'
     if core_pattern.strip() != expected_core_pattern:
       if fatal:
-        message = ("Invalid core_pattern configuration. "
-            "The configuration of core dump handling is *not* correct for "
-            "a buildbot. The content of {0} must be '{1}' instead of '{2}'."
+        message = ('Invalid core_pattern configuration. '
+            'The configuration of core dump handling is *not* correct for '
+            'a buildbot. The content of {0} must be "{1}" instead of "{2}".'
             .format(core_pattern_file, expected_core_pattern, core_pattern))
         raise Exception(message)
       else:
         return False
     return True
 
-def MarkCurrentStepWarning():
-  print "@@@STEP_WARNINGS@@@"
-  sys.stdout.flush()
-
 if __name__ == "__main__":
   import sys
   Main()