Implement upload_logs() in recipe_modules/goma

scripts/slave/recipe_modules/goma/api.py

Index: scripts/slave/recipe_modules/goma/api.py
diff --git a/scripts/slave/recipe_modules/goma/api.py b/scripts/slave/recipe_modules/goma/api.py
index c3fd0dfe2d11f666b16e767ce6a5578a3b4cbcda..6e654f7547941820a2ac26bfbc92af2b74ccd3a0 100644
--- a/scripts/slave/recipe_modules/goma/api.py
+++ b/scripts/slave/recipe_modules/goma/api.py
@@ -12,6 +12,18 @@ class GomaApi(recipe_api.RecipeApi):
     self._goma_dir = None
     self._goma_started = False
 
+    self._goma_ctl_env = {}
+
+    # options for upload_goma_logs.py
+    # TODO(tikuta) set these variables

[Paweł Hajdan Jr., 2016/08/09 07:04:36]

Why is this a TODO? If we're not setting/using some of these variables, why not
remove them from CL?

[tikuta, 2016/08/09 07:58:09]

Done.

+    self._ninja_log_outdir = None
+    self._ninja_log_compiler = None
+    self._ninja_log_command = None
+    self._ninja_log_exit_status = None
+
+    self._build_data_dir = None
+
+
   @property
   def service_account_json_path(self):
     if self.m.platform.is_win:
@@ -41,6 +53,10 @@ class GomaApi(recipe_api.RecipeApi):
           # TODO(phajdan.jr): make failures fatal after experiment.
           return None
 
+  @property
+  def goma_ctl(self):
+    return self.m.path.join(self._goma_dir, 'goma_ctl.py')
+
   def start(self, env=None, **kwargs):
     """Start goma compiler_proxy.
 
@@ -49,16 +65,41 @@ class GomaApi(recipe_api.RecipeApi):
     """
     assert self._goma_dir
     assert not self._goma_started
-    if not env:
-      env = {}
-    env.update({'GOMA_SERVICE_ACCOUNT_JSON_FILE': self.service_account_json_path})
-    self.m.python(
-        name='start_goma',
-        script=self.m.path.join(self._goma_dir, 'goma_ctl.py'),
-        args=['restart'], env=env, **kwargs)
-    self._goma_started = True
 
-  def stop(self, goma_dir=None, **kwargs):
+    if self.m.properties.get('build_data_dir'):

[Paweł Hajdan Jr., 2016/08/09 07:04:36]

Would it fit better in the ctor, or a dedicated method this one could call?

[tikuta, 2016/08/09 07:58:09]

Done.

+      self._build_data_dir = self.m.properties.get('build_data_dir')
+      self._goma_ctl_env['GOMA_DUMP_STATS_FILE'] = (
+          self.m.path.join(self._build_data_dir, 'goma_stats_proto'))
+      self._goma_ctl_env['GOMACTL_CRASH_REPORT_ID_FILE'] = (
+          self.m.path.join(self._build_data_dir, 'crash_report_id_file'))
+
+    self._goma_ctl_env['GOMA_SERVICE_ACCOUNT_JSON_FILE'] = (
+        self.service_account_json_path)
+
+    if env:
+      self._goma_ctl_env.update(env)

[Paweł Hajdan Jr., 2016/08/09 07:04:36]

This introduces state that survives this one method call. I suggest making a
copy of goma_ctl_env and only updating the local copy with this call specific
env.

[tikuta, 2016/08/09 07:58:09]

Done.

+
+    try:
+      self.m.python(
+          name='start_goma',
+          script=self.goma_ctl,
+          args=['restart'], env=self._goma_ctl_env, **kwargs)

[Paweł Hajdan Jr., 2016/08/09 07:04:36]

Set infra_step=True here.

[tikuta, 2016/08/09 07:58:09]

Done.

+      self._goma_started = True
+    except self.m.step.StepFailure as e:
+      upload_logs_name = 'upload_goma_start_failed_logs'
+
+      try:
+        self.m.python(
+            name='stop_goma (start failure)',
+            script=self.goma_ctl,
+            args=['stop'], env=self._goma_ctl_env, **kwargs)
+      except self.m.step.StepFailure:
+        upload_logs_name = 'upload_goma_start_and_stop_failed_logs'
+
+      self.upload_logs(upload_logs_name)
+      raise self.m.step.InfraFailure('Infra goma start failure: %s' % e)

[Paweł Hajdan Jr., 2016/08/09 07:04:36]

This way you'd be able to just re-raise the _same_ exception here.

[tikuta, 2016/08/09 07:58:10]

Done.

+
+  def stop(self, **kwargs):
     """Stop goma compiler_proxy.
 
     A user MUST execute start beforehand.
@@ -68,6 +109,39 @@ class GomaApi(recipe_api.RecipeApi):
     assert self._goma_started
     self.m.python(
         name='stop_goma',
-        script=self.m.path.join(self._goma_dir, 'goma_ctl.py'),
-        args=['stop'], **kwargs)
+        script=self.goma_ctl,
+        args=['stop'], env=self._goma_ctl_env, **kwargs)
+
+    self.upload_logs()
+
     self._goma_started = False
+    self._goma_ctl_env = {}
+
+
+  def upload_logs(self, name=None):
+    args = [
+        '--upload-compiler-proxy-info'
+    ]
+
+    if self._ninja_log_outdir:
+      args.extend([
+        '--ninja-log-outdir', self._ninja_log_outdir,
+        '--ninja-log-compiler', self._ninja_log_compiler,
+        '--ninja-log-command', self._ninja_log_command,
+        '--ninja-log-exit-status', self._ninja_log_exit_status,
+      ])
+
+    if self._build_data_dir:
+      args.extend([
+          '--goma-stats-file', self._goma_ctl_env['GOMA_DUMP_STATS_FILE'],
+          '--goma-crash-report-id-file',
+          self._goma_ctl_env['GOMACTL_CRASH_REPORT_ID_FILE'],
+          '--build-data-dir', self._build_data_dir,
+      ])
+
+    self.m.python(
+      name=name or 'upload_log',
+      script=self.package_repo_resource(
+          'scripts', 'slave', 'upload_goma_logs.py'),
+      args=args
+    )