Support sharding for swarmed isolated script tests.

Support sharding for swarmed isolated script tests.

Merge the shards' JSON output during the collect step to allow the
calling code to behave the same as before. Process the successes, even
though they're not displayed in the output. Link to individual shards'
output. Added tests for successful and failed sharded runs.

BUG=352807
Committed: https://chromium.googlesource.com/chromium/tools/build/+/179d67173034370fafaf5e8fa772ba766499c1cd

[Ken Russell (switch to Gerrit), 2016-06-16 23:29:03]

kbr@chromium.org changed reviewers:
+ dpranke@chromium.org, vadimsh@chromium.org

[Ken Russell (switch to Gerrit), 2016-06-16 23:29:04]

dpranke, vadimsh: please review. Thanks.

nednguyen, zmo: FYI.

[Vadim Sh., 2016-06-16 23:37:46]

looks mostly good

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

https://codereview.chromium.org/2073923002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/swarming/api.py:724: results_raw =
step_result.raw_io.output_dir[
handle a case of output.json missing. It happens if Swarming task failed in some
horrible way (before producing output.json) or didn't run at all (e.g. expired).

In gtest case, the results are considered invalid in this case (set
merged_results['valid'] = False?).

[Dirk Pranke, 2016-06-16 23:50:53]

lgtm w/ vadim's comment addressed.

[Ken Russell (switch to Gerrit), 2016-06-16 23:51:10]

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

https://codereview.chromium.org/2073923002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/swarming/api.py:724: results_raw =
step_result.raw_io.output_dir[
On 2016/06/16 23:37:46, Vadim Sh. wrote:
> handle a case of output.json missing. It happens if Swarming task failed in
some
> horrible way (before producing output.json) or didn't run at all (e.g.
expired).
> 
> In gtest case, the results are considered invalid in this case (set
> merged_results['valid'] = False?).

I think this is already implicitly handled by the "except Exception" clause in
the caller, _isolated_script_collect_step. We've certainly seen the
"no_results_exc" link in jobs that time out currently, and I saw it in testing
of this CL in intermediate stages.

Do you think more explicit handling is needed?

[Dirk Pranke, 2016-06-16 23:54:04]

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

https://codereview.chromium.org/2073923002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/swarming/api.py:724: results_raw =
step_result.raw_io.output_dir[
On 2016/06/16 23:51:10, Ken Russell wrote:
> On 2016/06/16 23:37:46, Vadim Sh. wrote:
> > handle a case of output.json missing. It happens if Swarming task failed in
> some
> > horrible way (before producing output.json) or didn't run at all (e.g.
> expired).
> > 
> > In gtest case, the results are considered invalid in this case (set
> > merged_results['valid'] = False?).
> 
> I think this is already implicitly handled by the "except Exception" clause in
> the caller, _isolated_script_collect_step. We've certainly seen the
> "no_results_exc" link in jobs that time out currently, and I saw it in testing
> of this CL in intermediate stages.
> 
> Do you think more explicit handling is needed?

Did that correctly fail the step? It's not obvious to me that that is what will
happen ...

[Ken Russell (switch to Gerrit), 2016-06-16 23:57:35]

On 2016/06/16 23:54:04, Dirk Pranke wrote:
>
https://codereview.chromium.org/2073923002/diff/20001/scripts/slave/recipe_mo...
> File scripts/slave/recipe_modules/swarming/api.py (right):
> 
>
https://codereview.chromium.org/2073923002/diff/20001/scripts/slave/recipe_mo...
> scripts/slave/recipe_modules/swarming/api.py:724: results_raw =
> step_result.raw_io.output_dir[
> On 2016/06/16 23:51:10, Ken Russell wrote:
> > On 2016/06/16 23:37:46, Vadim Sh. wrote:
> > > handle a case of output.json missing. It happens if Swarming task failed
in
> > some
> > > horrible way (before producing output.json) or didn't run at all (e.g.
> > expired).
> > > 
> > > In gtest case, the results are considered invalid in this case (set
> > > merged_results['valid'] = False?).
> > 
> > I think this is already implicitly handled by the "except Exception" clause
in
> > the caller, _isolated_script_collect_step. We've certainly seen the
> > "no_results_exc" link in jobs that time out currently, and I saw it in
testing
> > of this CL in intermediate stages.
> > 
> > Do you think more explicit handling is needed?
> 
> Did that correctly fail the step? It's not obvious to me that that is what
will
> happen ...

It did -- see
https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu...
for example.

[Vadim Sh., 2016-06-16 23:58:06]

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

https://codereview.chromium.org/2073923002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/swarming/api.py:724: results_raw =
step_result.raw_io.output_dir[
On 2016/06/16 23:54:03, Dirk Pranke wrote:
> On 2016/06/16 23:51:10, Ken Russell wrote:
> > On 2016/06/16 23:37:46, Vadim Sh. wrote:
> > > handle a case of output.json missing. It happens if Swarming task failed
in
> > some
> > > horrible way (before producing output.json) or didn't run at all (e.g.
> > expired).
> > > 
> > > In gtest case, the results are considered invalid in this case (set
> > > merged_results['valid'] = False?).
> > 
> > I think this is already implicitly handled by the "except Exception" clause
in
> > the caller, _isolated_script_collect_step. We've certainly seen the
> > "no_results_exc" link in jobs that time out currently, and I saw it in
testing
> > of this CL in intermediate stages.
> > 
> > Do you think more explicit handling is needed?
> 
> Did that correctly fail the step? It's not obvious to me that that is what
will
> happen ...

1) In any case, add a test that touches this code path.
2) I think it's better to handle this case explicitly. But I'm not sure how
upstream code would react to "valid: False" in isolated_script_results :( So, as
you prefer.

[Ken Russell (switch to Gerrit), 2016-06-17 00:14:07]

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

https://codereview.chromium.org/2073923002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/swarming/api.py:724: results_raw =
step_result.raw_io.output_dir[
On 2016/06/16 23:58:06, Vadim Sh. wrote:
> On 2016/06/16 23:54:03, Dirk Pranke wrote:
> > On 2016/06/16 23:51:10, Ken Russell wrote:
> > > On 2016/06/16 23:37:46, Vadim Sh. wrote:
> > > > handle a case of output.json missing. It happens if Swarming task failed
> in
> > > some
> > > > horrible way (before producing output.json) or didn't run at all (e.g.
> > > expired).
> > > > 
> > > > In gtest case, the results are considered invalid in this case (set
> > > > merged_results['valid'] = False?).
> > > 
> > > I think this is already implicitly handled by the "except Exception"
clause
> in
> > > the caller, _isolated_script_collect_step. We've certainly seen the
> > > "no_results_exc" link in jobs that time out currently, and I saw it in
> testing
> > > of this CL in intermediate stages.
> > > 
> > > Do you think more explicit handling is needed?
> > 
> > Did that correctly fail the step? It's not obvious to me that that is what
> will
> > happen ...
> 
> 1) In any case, add a test that touches this code path.
> 2) I think it's better to handle this case explicitly. But I'm not sure how
> upstream code would react to "valid: False" in isolated_script_results :( So,
as
> you prefer.

Added a test. Please see whether it's acceptable. I'm not sure of the best way
to handle the missing case (should I test the existence of results_raw, and if
so, what API should be used for that? should I catch an exception trying to call
json.loads? etc.). Any suggestions appreciated.

[Vadim Sh., 2016-06-17 00:31:12]

https://codereview.chromium.org/2073923002/diff/40001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/swarming/api.py (right):

https://codereview.chromium.org/2073923002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/swarming/api.py:724: results_raw =
step_result.raw_io.output_dir[
I think at the very minimum we can acknowledge in code that sometimes
output.json may be missing by writing:

path = self.m.path.join(str(i), 'output.json')
if path not in step_result.raw_io.output_dir:
  raise Exception("no results from shard #%d" % i)

(raw_io.output_dir seems to be a regular dict, so "in" checks presences of the
file)

https://codereview.chromium.org/2073923002/diff/40001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/test_utils/test_api.py (right):

https://codereview.chromium.org/2073923002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/test_utils/test_api.py:144: if not i in
missing_shards:
nit: i not in missing_shards

https://codereview.chromium.org/2073923002/diff/40001/scripts/slave/recipes/c...
File
scripts/slave/recipes/chromium.expected/dynamic_swarmed_sharded_isolated_script_test_missing_shard.json
(right):

https://codereview.chromium.org/2073923002/diff/40001/scripts/slave/recipes/c...
scripts/slave/recipes/chromium.expected/dynamic_swarmed_sharded_isolated_script_test_missing_shard.json:438:
"status_code": 0
this looks wrong... it's success code. So if all swarming shards exited with 0
exit code, but didn't actually generate output.json, the recipe will interpret
it as successful step. Not sure it is red on the waterfall currently.

[Vadim Sh., 2016-06-17 00:33:28]

https://codereview.chromium.org/2073923002/diff/40001/scripts/slave/recipes/c...
File
scripts/slave/recipes/chromium.expected/dynamic_swarmed_sharded_isolated_script_test_missing_shard.json
(right):

https://codereview.chromium.org/2073923002/diff/40001/scripts/slave/recipes/c...
scripts/slave/recipes/chromium.expected/dynamic_swarmed_sharded_isolated_script_test_missing_shard.json:438:
"status_code": 0
On 2016/06/17 00:31:12, Vadim Sh. wrote:
> this looks wrong... it's success code. So if all swarming shards exited with 0
> exit code, but didn't actually generate output.json, the recipe will interpret
> it as successful step. Not sure it is red on the waterfall currently.

*not sure WHY it is red on the waterfall currently

[Ken Russell (switch to Gerrit), 2016-06-17 00:57:55]

https://codereview.chromium.org/2073923002/diff/40001/scripts/slave/recipes/c...
File
scripts/slave/recipes/chromium.expected/dynamic_swarmed_sharded_isolated_script_test_missing_shard.json
(right):

https://codereview.chromium.org/2073923002/diff/40001/scripts/slave/recipes/c...
scripts/slave/recipes/chromium.expected/dynamic_swarmed_sharded_isolated_script_test_missing_shard.json:438:
"status_code": 0
On 2016/06/17 00:33:28, Vadim Sh. wrote:
> On 2016/06/17 00:31:12, Vadim Sh. wrote:
> > this looks wrong... it's success code. So if all swarming shards exited with
0
> > exit code, but didn't actually generate output.json, the recipe will
interpret
> > it as successful step. Not sure it is red on the waterfall currently.
> 
> *not sure WHY it is red on the waterfall currently

This was happening because I synthesized the wrong return code for the step.
I've fixed that in the next patch set and the recipe fails as expected.

[Vadim Sh., 2016-06-17 01:01:55]

lgtm

[Ken Russell (switch to Gerrit), 2016-06-17 01:07:40]

The CQ bit was checked by kbr@chromium.org

[Ken Russell (switch to Gerrit), 2016-06-17 01:07:40]

The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/2073923002/#ps60001
(title: "Explicitly handle missing shards' output.")

[commit-bot: I haz the power, 2016-06-17 01:07:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2073923002/60001

[commit-bot: I haz the power, 2016-06-17 01:11:50]

Description was changed from

==========
Support sharding for swarmed isolated script tests.

Merge the shards' JSON output during the collect step to allow the
calling code to behave the same as before. Process the successes, even
though they're not displayed in the output. Link to individual shards'
output. Added tests for successful and failed sharded runs.

BUG=352807
==========

to

==========
Support sharding for swarmed isolated script tests.

Merge the shards' JSON output during the collect step to allow the
calling code to behave the same as before. Process the successes, even
though they're not displayed in the output. Link to individual shards'
output. Added tests for successful and failed sharded runs.

BUG=352807

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

[commit-bot: I haz the power, 2016-06-17 01:11:55]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/tools/build/+/179d67173034370fafaf...