Implement upload_logs() in recipe_modules/goma

Implement upload_logs() in recipe_modules/goma

* upload_logs uploads goma stats, crash report and ninja log
* add test for build_data_dir and ninja_log
* add no cover for InfraFailure exception

BUG=544330
Committed: https://chromium.googlesource.com/chromium/tools/build/+/dfd62794c4925e42fb06a493149683122f68a791

[tikuta, 2016-08-04 11:14:17]

Description was changed from

==========
Implement upload_logs() in recipe_modules/goma

BUG=544330
==========

to

==========
Implement upload_logs() in recipe_modules/goma

* upload_logs uploads goma stats, crash report and ninja log

BUG=544330
==========

[tikuta, 2016-08-04 11:14:17]

tikuta@chromium.org changed reviewers:
+ shinyak@chromium.org, ukai@chromium.org, yyanagisawa@chromium.org

[tikuta, 2016-08-09 03:40:22]

Please review this CL.

[Yoshisato Yanagisawa, 2016-08-09 04:01:49]

yyanagisawa@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[Yoshisato Yanagisawa, 2016-08-09 04:01:49]

Phajdan, will you review this code from recipe point of view?

https://codereview.chromium.org/2207273003/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/goma/api.py (left):

https://codereview.chromium.org/2207273003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/goma/api.py:73: self._goma_started = False
should clear self._goma_ctl_env?

https://codereview.chromium.org/2207273003/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/goma/api.py (right):

https://codereview.chromium.org/2207273003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/goma/api.py:15: self._env = {}
might be better to say goma_ctl_env or so?

https://codereview.chromium.org/2207273003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/goma/api.py:60: def start(self, build_data_dir,
env=None, **kwargs):
might be better to make build_data_dir optional?
or, always enable this if m.properties have it? (disables it if properties does
not have it?)

https://codereview.chromium.org/2207273003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/goma/api.py:83: args=['restart'], env=self._env,
**kwargs)
can you upload stats on goma_start failure?

[tikuta, 2016-08-09 05:30:40]

https://codereview.chromium.org/2207273003/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/goma/api.py (left):

https://codereview.chromium.org/2207273003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/goma/api.py:73: self._goma_started = False
On 2016/08/09 04:01:49, Yoshisato Yanagisawa wrote:
> should clear self._goma_ctl_env?

Done.

https://codereview.chromium.org/2207273003/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/goma/api.py (right):

https://codereview.chromium.org/2207273003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/goma/api.py:15: self._env = {}
On 2016/08/09 04:01:49, Yoshisato Yanagisawa wrote:
> might be better to say goma_ctl_env or so?

Done.

https://codereview.chromium.org/2207273003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/goma/api.py:60: def start(self, build_data_dir,
env=None, **kwargs):
On 2016/08/09 04:01:49, Yoshisato Yanagisawa wrote:
> might be better to make build_data_dir optional?
> or, always enable this if m.properties have it? (disables it if properties
does
> not have it?)

Done.

https://codereview.chromium.org/2207273003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/goma/api.py:83: args=['restart'], env=self._env,
**kwargs)
On 2016/08/09 04:01:49, Yoshisato Yanagisawa wrote:
> can you upload stats on goma_start failure?

Done.

[ukai, 2016-08-09 05:47:19]

lgtm

https://codereview.chromium.org/2207273003/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/goma/api.py (right):

https://codereview.chromium.org/2207273003/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/api.py:89: self.m.python(
if stop failed, will it raise InfraFailure? (or StepFailure?)

[tikuta, 2016-08-09 06:02:47]

https://codereview.chromium.org/2207273003/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/goma/api.py (right):

https://codereview.chromium.org/2207273003/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/api.py:89: self.m.python(
On 2016/08/09 05:47:18, ukai wrote:
> if stop failed, will it raise InfraFailure? (or StepFailure?)

Done.

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

https://codereview.chromium.org/2207273003/diff/60001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/goma/api.py (right):

https://codereview.chromium.org/2207273003/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/api.py:18: # TODO(tikuta) set these variables
Why is this a TODO? If we're not setting/using some of these variables, why not
remove them from CL?

https://codereview.chromium.org/2207273003/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/api.py:69: if
self.m.properties.get('build_data_dir'):
Would it fit better in the ctor, or a dedicated method this one could call?

https://codereview.chromium.org/2207273003/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/api.py:80: self._goma_ctl_env.update(env)
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.

https://codereview.chromium.org/2207273003/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/api.py:86: args=['restart'],
env=self._goma_ctl_env, **kwargs)
Set infra_step=True here.

https://codereview.chromium.org/2207273003/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/api.py:100: raise
self.m.step.InfraFailure('Infra goma start failure: %s' % e)
This way you'd be able to just re-raise the _same_ exception here.

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

Thank you for review phajdan!

https://codereview.chromium.org/2207273003/diff/60001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/goma/api.py (right):

https://codereview.chromium.org/2207273003/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/api.py:18: # TODO(tikuta) set these variables
On 2016/08/09 07:04:36, Paweł Hajdan Jr. wrote:
> Why is this a TODO? If we're not setting/using some of these variables, why
not
> remove them from CL?

Done.

https://codereview.chromium.org/2207273003/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/api.py:69: if
self.m.properties.get('build_data_dir'):
On 2016/08/09 07:04:36, Paweł Hajdan Jr. wrote:
> Would it fit better in the ctor, or a dedicated method this one could call?

Done.

https://codereview.chromium.org/2207273003/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/api.py:80: self._goma_ctl_env.update(env)
On 2016/08/09 07:04:36, Paweł Hajdan Jr. wrote:
> 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.

Done.

https://codereview.chromium.org/2207273003/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/api.py:86: args=['restart'],
env=self._goma_ctl_env, **kwargs)
On 2016/08/09 07:04:36, Paweł Hajdan Jr. wrote:
> Set infra_step=True here.

Done.

https://codereview.chromium.org/2207273003/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/api.py:100: raise
self.m.step.InfraFailure('Infra goma start failure: %s' % e)
On 2016/08/09 07:04:36, Paweł Hajdan Jr. wrote:
> This way you'd be able to just re-raise the _same_ exception here.

Done.

[Paweł Hajdan Jr., 2016-08-09 16:16:54]

LGTM

[Yoshisato Yanagisawa, 2016-08-10 05:13:09]

lgtm

It is fine to be the other cl, but can we also have tests with build_data_dir
properties and ninja_log?

[tikuta, 2016-08-10 06:29:22]

On 2016/08/10 05:13:09, Yoshisato Yanagisawa wrote:
> lgtm
> 
> It is fine to be the other cl, but can we also have tests with build_data_dir
> properties and ninja_log?

I created another CL for test.
https://codereview.chromium.org/2232733002/

[tikuta, 2016-08-10 08:44:49]

The CQ bit was checked by tikuta@chromium.org

[tikuta, 2016-08-10 08:44:50]

The patchset sent to the CQ was uploaded after l-g-t-m from ukai@chromium.org,
phajdan.jr@chromium.org
Link to the patchset: https://codereview.chromium.org/2207273003/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2016-08-10 08:44:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 08:48:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 08:48:27]

Try jobs failed on following builders:
  Build Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/308c71f59404cc10)

[tikuta, 2016-08-10 09:01:34]

Description was changed from

==========
Implement upload_logs() in recipe_modules/goma

* upload_logs uploads goma stats, crash report and ninja log

BUG=544330
==========

to

==========
Implement upload_logs() in recipe_modules/goma

* upload_logs uploads goma stats, crash report and ninja log
* add test for build_data_dir and ninja_log
* add no cover for InfraFailure exception

BUG=544330
==========

[tikuta, 2016-08-10 09:06:44]

PTAL.

I added test in this CL.

[shinyak, 2016-08-10 11:17:58]

lgtm

[tikuta, 2016-08-12 01:39:56]

The CQ bit was checked by tikuta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-12 01:40:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 01:43:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-12 01:43:32]

Dry run: This issue passed the CQ dry run.

[tikuta, 2016-08-12 01:47:46]

The CQ bit was checked by tikuta@chromium.org

[tikuta, 2016-08-12 01:47:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
phajdan.jr@chromium.org, ukai@chromium.org, yyanagisawa@chromium.org
Link to the patchset: https://codereview.chromium.org/2207273003/#ps140001
(title: "change for coverage")

[commit-bot: I haz the power, 2016-08-12 01:47:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 01:51:31]

Description was changed from

==========
Implement upload_logs() in recipe_modules/goma

* upload_logs uploads goma stats, crash report and ninja log
* add test for build_data_dir and ninja_log
* add no cover for InfraFailure exception

BUG=544330
==========

to

==========
Implement upload_logs() in recipe_modules/goma

* upload_logs uploads goma stats, crash report and ninja log
* add test for build_data_dir and ninja_log
* add no cover for InfraFailure exception

BUG=544330

Committed:
https://chromium.googlesource.com/chromium/tools/build/+/dfd62794c4925e42fb06...
==========

[commit-bot: I haz the power, 2016-08-12 01:51:32]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/tools/build/+/dfd62794c4925e42fb06...