[swarming/api] Add text in to indicate whether a shard is failing

[swarming/api] Add text in to indicate whether a shard is failing

BUG=chromium:639321
Committed: https://chromium.googlesource.com/chromium/tools/build/+/68e74d1eef346395009cf05e4ad358c8a762dab0

[nednguyen, 2016-09-22 15:42:06]

Patchset #1 (id:1) has been deleted

[nednguyen, 2016-09-22 15:46:14]

Description was changed from

==========
[swarming/api] Display which shard is failing

BUG=chromium:639321
==========

to

==========
[swarming/api] Add text in to indicate whether a shard is failing

BUG=chromium:639321
==========

[nednguyen, 2016-09-22 15:46:14]

nednguyen@google.com changed reviewers:
+ kbr@chromium.org, maruel@chromium.org

[nednguyen, 2016-09-22 15:46:26]

The CQ bit was checked by nednguyen@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-22 15:46:31]

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-09-22 15:46:32]

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

[commit-bot: I haz the power, 2016-09-22 15:46:33]

Dry run: No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-infra-committers".
Note that this has nothing to do with OWNERS files.

[nednguyen, 2016-09-22 15:52:19]

The CQ bit was checked by nednguyen@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-22 15:52:21]

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-09-22 15:52:22]

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

[commit-bot: I haz the power, 2016-09-22 15:52:23]

Dry run: No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-infra-committers".
Note that this has nothing to do with OWNERS files.

[nednguyen, 2016-09-22 15:54:40]

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

https://codereview.chromium.org/2362733002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/swarming/api.py:850: if
summary['shards'][index].get('exit_code', 0) != 0:
I get to know this by looking at an example summary.json output, i.e:
python swarming.py collect -S https://chromium-swarm.appspot.com
--task-output-dir=foo 30bc1911e6720a10

cat foo/summary.json

Do we have documentation/code pointer about what summary.json is supposed to
contain somewhere?

[M-A Ruel, 2016-09-22 16:05:01]

maruel@chromium.org changed reviewers:
+ vadimsh@chromium.org

[M-A Ruel, 2016-09-22 16:05:02]

+Vadim for correctness.

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

https://codereview.chromium.org/2362733002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/swarming/api.py:850: if
summary['shards'][index].get('exit_code', 0) != 0:
On 2016/09/22 15:54:40, nednguyen wrote:
> I get to know this by looking at an example summary.json output, i.e:
> python swarming.py collect -S https://chromium-swarm.appspot.com
> --task-output-dir=foo 30bc1911e6720a10
> 
> cat foo/summary.json
> 
> Do we have documentation/code pointer about what summary.json is supposed to
> contain somewhere?

No. :/

[Vadim Sh., 2016-09-22 17:36:30]

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

https://codereview.chromium.org/2362733002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/swarming/api.py:850: if
summary['shards'][index].get('exit_code', 0) != 0:
Please also handle a case when summary['shards'][index] is None. 

I think it can happen if Swarming client has trouble collecting the results
(e.g. network timeouts). Though maybe dying with AttributeError is good enough
in this case...

On 2016/09/22 16:05:02, M-A Ruel wrote:
> On 2016/09/22 15:54:40, nednguyen wrote:
> > I get to know this by looking at an example summary.json output, i.e:
> > python swarming.py collect -S https://chromium-swarm.appspot.com
> > --task-output-dir=foo 30bc1911e6720a10
> > 
> > cat foo/summary.json
> > 
> > Do we have documentation/code pointer about what summary.json is supposed to
> > contain somewhere?
> 
> No. :/

I believe each dict in 'shards' list has the structure of TaskResult protorpc
message:
https://github.com/luci/luci-py/blob/master/appengine/swarming/swarming_rpcs....

(based on https://github.com/luci/luci-py/blob/master/client/swarming.py#L547)

[Ken Russell (switch to Gerrit), 2016-09-22 17:42:58]

Thanks for putting this together; it'll be extremely useful. Non-owner LGTM.

(Please watch the trybots after this lands and make sure they're all green:
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel...
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel...
Thanks.)

[nednguyen, 2016-09-22 21:03:54]

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

https://codereview.chromium.org/2362733002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/swarming/api.py:850: if
summary['shards'][index].get('exit_code', 0) != 0:
On 2016/09/22 17:36:30, Vadim Sh. wrote:
> Please also handle a case when summary['shards'][index] is None. 
> 
> I think it can happen if Swarming client has trouble collecting the results
> (e.g. network timeouts). Though maybe dying with AttributeError is good enough
> in this case...
> 
> On 2016/09/22 16:05:02, M-A Ruel wrote:
> > On 2016/09/22 15:54:40, nednguyen wrote:
> > > I get to know this by looking at an example summary.json output, i.e:
> > > python swarming.py collect -S https://chromium-swarm.appspot.com
> > > --task-output-dir=foo 30bc1911e6720a10
> > > 
> > > cat foo/summary.json
> > > 
> > > Do we have documentation/code pointer about what summary.json is supposed
to
> > > contain somewhere?
> > 
> > No. :/
> 
> I believe each dict in 'shards' list has the structure of TaskResult protorpc
> message:
>
https://github.com/luci/luci-py/blob/master/appengine/swarming/swarming_rpcs....
> 
> (based on https://github.com/luci/luci-py/blob/master/client/swarming.py#L547)

Done. However I am missing the code coverage. You have any suggestion how to add
test to cover the case a shard return None?

[M-A Ruel, 2016-09-22 21:09:19]

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

https://codereview.chromium.org/2362733002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/swarming/api.py:797: display_text = 'shard #%d
(failed - result not received)' % index
We should treat that as an infrastructure failure (while at it).

https://codereview.chromium.org/2362733002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/swarming/api.py:798: elif
summary['shards'][index].get('exit_code', 0) != 0:
, None)

since if it's missing this is bad.

[nednguyen, 2016-09-22 21:46:04]

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

https://codereview.chromium.org/2362733002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/swarming/api.py:797: display_text = 'shard #%d
(failed - result not received)' % index
On 2016/09/22 21:09:19, M-A Ruel wrote:
> We should treat that as an infrastructure failure (while at it).

Done.

https://codereview.chromium.org/2362733002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/swarming/api.py:798: elif
summary['shards'][index].get('exit_code', 0) != 0:
On 2016/09/22 21:09:19, M-A Ruel wrote:
> , None)
> 
> since if it's missing this is bad.

Done.

[nednguyen, 2016-09-22 21:47:07]

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

https://codereview.chromium.org/2362733002/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/swarming/api.py:800: display_text = 'shard #%d' %
index
Now I am missing test coverage at this line because all the summary results in
test doesn't contain exit_code. Anyone knows how to modify the summary output
for the expectation tests?

[nednguyen, 2016-09-22 22:11:48]

On 2016/09/22 21:47:07, nednguyen wrote:
>
https://codereview.chromium.org/2362733002/diff/60001/scripts/slave/recipe_mo...
> File scripts/slave/recipe_modules/swarming/api.py (right):
> 
>
https://codereview.chromium.org/2362733002/diff/60001/scripts/slave/recipe_mo...
> scripts/slave/recipe_modules/swarming/api.py:800: display_text = 'shard #%d' %
> index
> Now I am missing test coverage at this line because all the summary results in
> test doesn't contain exit_code. Anyone knows how to modify the summary output
> for the expectation tests?

I got that resolved. PTAL

[nednguyen, 2016-09-22 22:12:08]

The CQ bit was checked by nednguyen@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-22 22:12:17]

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

[Vadim Sh., 2016-09-22 22:13:19]

lgtm

[nednguyen, 2016-09-22 22:18:27]

The CQ bit was unchecked by nednguyen@google.com

[nednguyen, 2016-09-22 22:18:30]

The CQ bit was checked by nednguyen@google.com

[nednguyen, 2016-09-22 22:18:31]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org
Link to the patchset: https://codereview.chromium.org/2362733002/#ps70001
(title: "Update swarming summary.json generation")

[commit-bot: I haz the power, 2016-09-22 22:18:40]

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-09-22 22:22:51]

Description was changed from

==========
[swarming/api] Add text in to indicate whether a shard is failing

BUG=chromium:639321
==========

to

==========
[swarming/api] Add text in to indicate whether a shard is failing

BUG=chromium:639321

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

[commit-bot: I haz the power, 2016-09-22 22:22:52]

Committed patchset #4 (id:70001) as
https://chromium.googlesource.com/chromium/tools/build/+/68e74d1eef346395009c...

[nednguyen, 2016-09-23 00:50:32]

A revert of this CL (patchset #4 id:70001) has been created in
https://codereview.chromium.org/2367623002/ by nednguyen@google.com.

The reason for reverting is: All the shards are annotated as "failed"
(https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64...).

[nednguyen, 2016-09-23 00:53:20]

On 2016/09/22 22:22:52, commit-bot: I haz the power wrote:
> Committed patchset #4 (id:70001) as
>
https://chromium.googlesource.com/chromium/tools/build/+/68e74d1eef346395009c...

Tuned out       "exit_code" is "0", not 0 ~_~

Since the revert fails, I will make a fix then