Findit] Flake Checker: Pipeline to trigger try jobs to identify flake culprits

Findit] Flake Checker: Pipeline to trigger try jobs to identify flake culprits

BUG=

Landed in https://chromium-review.googlesource.com/#/c/428154/ and https://chromium-review.googlesource.com/#/c/428298/

[lijeffrey, 2017-01-11 21:50:04]

Description was changed from

==========
Findit] Flake Checker: Pipeline to trigger try jobs to identify flake culprits


Using try job key instead of explicit ndb entities


Unbasing from other change


Addressing comments


Addressing comments


Fixing rebase error


Rebase


Rebase


[Findit] Modifying monitor_try_job_pipeline.py to be reusable by both Waterfall
and Flake try jobs


Addressing comments


Adding missing files


[Findit] Adding build model for flake try jobs


[Findit] Adding build model for flake try job


[Findit] Adding build model for flake try job


[Findit] Adding build model for flake try job


Addressing comments (proprely this time)


Addressing comments


Adding missing files


[Findit] Adding build model for flake try jobs


[Findit] Adding build model for flake try job


[Findit] Adding build model for flake try job


Use gae_libs.testcase instead of unittest


Rebase


Addressing comments


2016 -> 2017


Addressing comments


Addressing comments


Rebase


Removing redundant code


Adding try job status


[Findit] Adding suspected CL to master flake analysis in preparation for try
jobs


Addressing comments


Addressing comments


Using try job key instead of explicit ndb entities


Unbasing from other change


Fixing unintentional change


Fixing nits


Fixing unintentional changes


Rebase


Renaming revision to test_revision


Addressing comments


[Findit] Adding schedule_flake_try_job_pipeline.py to trigger flake try jobs


Fixing whitespace


Using try job key instead of explicit ndb entities


Unbasing from other change


[Findit] Get full build info for each flake build analyzed

BUG=
==========

to

==========
Findit] Flake Checker: Pipeline to trigger try jobs to identify flake culprits

BUG=
==========

[lijeffrey, 2017-01-12 07:13:56]

lijeffrey@chromium.org changed reviewers:
+ chanli@chromium.org, robertocn@chromium.org, stgao@chromium.org

[lijeffrey, 2017-01-12 07:13:56]

ptal

[stgao, 2017-01-12 08:04:16]

https://codereview.chromium.org/2630433002/diff/140001/appengine/findit/model...
File appengine/findit/model/flake/master_flake_analysis.py (right):

https://codereview.chromium.org/2630433002/diff/140001/appengine/findit/model...
appengine/findit/model/flake/master_flake_analysis.py:59: (monotonically
increasing by increments of 1) 100% of the time. Should
Per my meeting with Robbie, commit position is reliable. So we could remove this
as we discussed in person.

https://codereview.chromium.org/2630433002/diff/140001/appengine/findit/water...
File appengine/findit/waterfall/flake/process_flake_try_job_result_pipeline.py
(right):

https://codereview.chromium.org/2630433002/diff/140001/appengine/findit/water...
appengine/findit/waterfall/flake/process_flake_try_job_result_pipeline.py:68:
flake_analysis.data_points.append(data_point)
Do we need a put to update it in NDB?

https://codereview.chromium.org/2630433002/diff/140001/appengine/findit/water...
File appengine/findit/waterfall/flake/recursive_flake_try_job_pipeline.py
(right):

https://codereview.chromium.org/2630433002/diff/140001/appengine/findit/water...
appengine/findit/waterfall/flake/recursive_flake_try_job_pipeline.py:65:
flake_analysis.result_status = result_status.NOT_FOUND_UNTRIAGED
If we arrive here, that means we have a suspect build, but can't identify the
exact culprit. Should it be still NOT_FOUND?

https://codereview.chromium.org/2630433002/diff/140001/appengine/findit/water...
appengine/findit/waterfall/flake/recursive_flake_try_job_pipeline.py:101:
try_job.status = analysis_status.PENDING
Why failed try job will have a pending status? What's the scenario here?

[lijeffrey, 2017-01-12 09:52:52]

https://codereview.chromium.org/2630433002/diff/140001/appengine/findit/model...
File appengine/findit/model/flake/master_flake_analysis.py (right):

https://codereview.chromium.org/2630433002/diff/140001/appengine/findit/model...
appengine/findit/model/flake/master_flake_analysis.py:59: (monotonically
increasing by increments of 1) 100% of the time. Should
On 2017/01/12 08:04:16, stgao (slow on Monday) wrote:
> Per my meeting with Robbie, commit position is reliable. So we could remove
this
> as we discussed in person.

Done.

https://codereview.chromium.org/2630433002/diff/140001/appengine/findit/water...
File appengine/findit/waterfall/flake/process_flake_try_job_result_pipeline.py
(right):

https://codereview.chromium.org/2630433002/diff/140001/appengine/findit/water...
appengine/findit/waterfall/flake/process_flake_try_job_result_pipeline.py:68:
flake_analysis.data_points.append(data_point)
On 2017/01/12 08:04:16, stgao (slow on Monday) wrote:
> Do we need a put to update it in NDB?

good catch. Done.

https://codereview.chromium.org/2630433002/diff/140001/appengine/findit/water...
File appengine/findit/waterfall/flake/recursive_flake_try_job_pipeline.py
(right):

https://codereview.chromium.org/2630433002/diff/140001/appengine/findit/water...
appengine/findit/waterfall/flake/recursive_flake_try_job_pipeline.py:65:
flake_analysis.result_status = result_status.NOT_FOUND_UNTRIAGED
On 2017/01/12 08:04:16, stgao (slow on Monday) wrote:
> If we arrive here, that means we have a suspect build, but can't identify the
> exact culprit. Should it be still NOT_FOUND?

Yes. In this case suspected flake build is a false positive and we should not
count it as having found something. Flake swarming tasks guide the analysis to
this point, but the try jobs can verify if that point is correct or not.

https://codereview.chromium.org/2630433002/diff/140001/appengine/findit/water...
appengine/findit/waterfall/flake/recursive_flake_try_job_pipeline.py:101:
try_job.status = analysis_status.PENDING
On 2017/01/12 08:04:16, stgao (slow on Monday) wrote:
> Why failed try job will have a pending status? What's the scenario here?

This is for rerun.

[chanli, 2017-01-13 00:47:34]

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/model...
File appengine/findit/model/flake/master_flake_analysis.py (right):

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/model...
appengine/findit/model/flake/master_flake_analysis.py:21: # data point is
generated as the result of a flake swarming task.
Should we mention this is relevant for build level or something? And for
try_job_id and try_job_url we can say they are only relevant for revision level.

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/model...
appengine/findit/model/flake/master_flake_analysis.py:30: # The git position of
this data point.
I don't think git position is the right way to call it... I think commit
position is just OK as is.

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/model...
appengine/findit/model/flake/master_flake_analysis.py:37: # data point is
generated as the result of a flake swarming taask.
Nit: taask -> task

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/model...
appengine/findit/model/flake/master_flake_analysis.py:42:
previous_build_git_hash = ndb.StringProperty(indexed=False)
Maybe now is a little late.... But why do we need both commit_position and
git_hash of previous build?

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/model...
appengine/findit/model/flake/master_flake_analysis.py:50: try_job_id =
ndb.StringProperty(indexed=False)
As we discussed offline, maybe remove try_job_id for now

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/model...
appengine/findit/model/flake/master_flake_analysis.py:55: def
GetCommitPosition(self, revision):
I think this function is a little hacky... Although we have confirmed
commit_position should be continuous.

BTW, why do we need this?

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/water...
File appengine/findit/waterfall/flake/recursive_flake_pipeline.py (right):

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/water...
appengine/findit/waterfall/flake/recursive_flake_pipeline.py:65: else:
master_flake_analysis.status needs to be updated at all conditions. I prefer to
update it at very beginning of this function.

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/water...
appengine/findit/waterfall/flake/recursive_flake_pipeline.py:66:
master_flake_analysis.suspected_flake_build_number = suspected_build
Question: should we continue triaging at build level? Especially if try jobs
don't have results?

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/water...
File appengine/findit/waterfall/flake/recursive_flake_try_job_pipeline.py
(right):

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/water...
appengine/findit/waterfall/flake/recursive_flake_try_job_pipeline.py:24: from
waterfall.monitor_try_job_pipeline import MonitorTryJobPipeline
Nit: Move this after ln 25.

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/water...
appengine/findit/waterfall/flake/recursive_flake_try_job_pipeline.py:39: def
_CreateCulprit(revision, commit_position, repo_name='chromium'):
Naming Nit: maybe change the function name to CreateCulpritInfo or something?

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/water...
appengine/findit/waterfall/flake/recursive_flake_try_job_pipeline.py:90: # Don't
start start try-jobs if the flake swarming tasks had error or are
Nit: two start

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/water...
appengine/findit/waterfall/flake/recursive_flake_try_job_pipeline.py:98: if
try_job:   # pragma: no cover
Any reason to add no cover here?

Will add # pragma: no branch at line 99 work?

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/water...
appengine/findit/waterfall/flake/recursive_flake_try_job_pipeline.py:109:
flake_analysis.try_job_status = analysis_status.RUNNING
RUNNING or PENDING?

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/water...
appengine/findit/waterfall/flake/recursive_flake_try_job_pipeline.py:185: #
These cases are not needed for try jobs.
What cases are you referring to? "if stables_in_a_row <= max_stable_in_a_row"
should be one case, what else?

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/water...
appengine/findit/waterfall/flake/recursive_flake_try_job_pipeline.py:218: if
commit_position == lower_boundary_commit_position:  # pragma: no branch
Just double check: this means we don't handle dive at all at revision level,
right?

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/water...
appengine/findit/waterfall/flake/recursive_flake_try_job_pipeline.py:232: if
pass_rate - previous_pass_rate > dive_rate_threshold:
If we don't handle dive, maybe we can remove this part?

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/water...
appengine/findit/waterfall/flake/recursive_flake_try_job_pipeline.py:275: in a
MasterFlakeAnalysis entity.
Docstring needs to be updated

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/water...
appengine/findit/waterfall/flake/recursive_flake_try_job_pipeline.py:287: if
all_data_points[i].try_job_id is not None:
Nit: if all_data_points[i].try_job_id should also work. 

BTW, as we discussed offline, we can remove try_job_id and only use try_job_url

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/water...
appengine/findit/waterfall/flake/recursive_flake_try_job_pipeline.py:327:
'max_dive_in_a_row': 4,
If don't handle dive for revision level, maybe we don't need these settings?

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/water...
appengine/findit/waterfall/flake/recursive_flake_try_job_pipeline.py:345:
next_commit_position >= suspected_build_data_point.commit_position):
In what situation next_commit_position can be greater than
suspected_build_data_point.commit_position? I think if sequential run it should
be one by one?

[lijeffrey, 2017-01-13 01:39:24]

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/model...
File appengine/findit/model/flake/master_flake_analysis.py (right):

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/model...
appengine/findit/model/flake/master_flake_analysis.py:21: # data point is
generated as the result of a flake swarming task.
On 2017/01/13 00:47:32, chanli wrote:
> Should we mention this is relevant for build level or something? And for
> try_job_id and try_job_url we can say they are only relevant for revision
level.

Done.

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/model...
appengine/findit/model/flake/master_flake_analysis.py:30: # The git position of
this data point.
On 2017/01/13 00:47:33, chanli wrote:
> I don't think git position is the right way to call it... I think commit
> position is just OK as is.

Done.

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/model...
appengine/findit/model/flake/master_flake_analysis.py:37: # data point is
generated as the result of a flake swarming taask.
On 2017/01/13 00:47:33, chanli wrote:
> Nit: taask -> task

Done.

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/model...
appengine/findit/model/flake/master_flake_analysis.py:42:
previous_build_git_hash = ndb.StringProperty(indexed=False)
On 2017/01/13 00:47:33, chanli wrote:
> Maybe now is a little late.... But why do we need both commit_position and
> git_hash of previous build?

git hash is the useful one, previous build commit position is for UI/display
purposes.

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/model...
appengine/findit/model/flake/master_flake_analysis.py:50: try_job_id =
ndb.StringProperty(indexed=False)
On 2017/01/13 00:47:33, chanli wrote:
> As we discussed offline, maybe remove try_job_id for now

Done.

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/model...
appengine/findit/model/flake/master_flake_analysis.py:55: def
GetCommitPosition(self, revision):
On 2017/01/13 00:47:32, chanli wrote:
> I think this function is a little hacky... Although we have confirmed
> commit_position should be continuous.
> 
> BTW, why do we need this?

Unfortunately blame_list only contains revisions and doesn't have
commit_position, so we have to calculate it. In the end it's is only used for
display purposes and is effectively treated as an index in the blame list for
which to run the next try job, so even if it's off the try job's lookback
algorithm is unaffected.

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/water...
File appengine/findit/waterfall/flake/recursive_flake_pipeline.py (right):

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/water...
appengine/findit/waterfall/flake/recursive_flake_pipeline.py:65: else:
On 2017/01/13 00:47:33, chanli wrote:
> master_flake_analysis.status needs to be updated at all conditions. I prefer
to
> update it at very beginning of this function.

Done.

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/water...
appengine/findit/waterfall/flake/recursive_flake_pipeline.py:66:
master_flake_analysis.suspected_flake_build_number = suspected_build
On 2017/01/13 00:47:33, chanli wrote:
> Question: should we continue triaging at build level? Especially if try jobs
> don't have results?

I think try jobs will be much more reliable than at the build level, because
it's easy at the build level to come up with false positives. A wrong suspected
build number is guaranteed to yield a wrong try job result if it even finds
anything. If the build level does not find anything, then that's definitely not
found (as is the case here), but if it does, it's still not necessarily found
until try jobs determine it has, especially for false positives (i.e. we suspect
a dive from 80% to 20%, but the 80% is already very flaky so I would not have
confidence in the try job finding anything meaningful.

I do plan to redesign the triage model though to differentiate between found at
the build level vs found at the CL level, but since that's not a trivial change
it should be for a separate CL.

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/water...
File appengine/findit/waterfall/flake/recursive_flake_try_job_pipeline.py
(right):

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/water...
appengine/findit/waterfall/flake/recursive_flake_try_job_pipeline.py:24: from
waterfall.monitor_try_job_pipeline import MonitorTryJobPipeline
On 2017/01/13 00:47:33, chanli wrote:
> Nit: Move this after ln 25.

Done.

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/water...
appengine/findit/waterfall/flake/recursive_flake_try_job_pipeline.py:39: def
_CreateCulprit(revision, commit_position, repo_name='chromium'):
On 2017/01/13 00:47:33, chanli wrote:
> Naming Nit: maybe change the function name to CreateCulpritInfo or something?

CulpritInfo sounds like a dict representation which this is not. The name of the
entity is FlakeCulprit, so I'll leave this as is.

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/water...
appengine/findit/waterfall/flake/recursive_flake_try_job_pipeline.py:90: # Don't
start start try-jobs if the flake swarming tasks had error or are
On 2017/01/13 00:47:33, chanli wrote:
> Nit: two start

Done.

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/water...
appengine/findit/waterfall/flake/recursive_flake_try_job_pipeline.py:98: if
try_job:   # pragma: no cover
On 2017/01/13 00:47:33, chanli wrote:
> Any reason to add no cover here?
> 
> Will add # pragma: no branch at line 99 work?

This case actually should not happen for flake try jobs, until we support rerun.
Removed.

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/water...
appengine/findit/waterfall/flake/recursive_flake_try_job_pipeline.py:109:
flake_analysis.try_job_status = analysis_status.RUNNING
On 2017/01/13 00:47:33, chanli wrote:
> RUNNING or PENDING?

As discussed before try job status at the analysis level should be whether or
not try jobs are running, so no PENDING since each try job will have
pending/running/completed and there will be more than 1. This is simply to
indicate that this analysis is currently running try jobs, regardless of the
state of the individual try jobs.

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/water...
appengine/findit/waterfall/flake/recursive_flake_try_job_pipeline.py:185: #
These cases are not needed for try jobs.
On 2017/01/13 00:47:33, chanli wrote:
> What cases are you referring to? "if stables_in_a_row <= max_stable_in_a_row"
> should be one case, what else?

For analysis at the revision level, there is no "flakes in a row" or "stable
region". It's simply lookback until a single stable revision is found, then look
forward. I've removed the comment since I plan to refactor this algorithm
anyway.

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/water...
appengine/findit/waterfall/flake/recursive_flake_try_job_pipeline.py:218: if
commit_position == lower_boundary_commit_position:  # pragma: no branch
On 2017/01/13 00:47:34, chanli wrote:
> Just double check: this means we don't handle dive at all at revision level,
> right?

That's correct, removed.

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/water...
appengine/findit/waterfall/flake/recursive_flake_try_job_pipeline.py:232: if
pass_rate - previous_pass_rate > dive_rate_threshold:
On 2017/01/13 00:47:34, chanli wrote:
> If we don't handle dive, maybe we can remove this part?

Done.

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/water...
appengine/findit/waterfall/flake/recursive_flake_try_job_pipeline.py:275: in a
MasterFlakeAnalysis entity.
On 2017/01/13 00:47:33, chanli wrote:
> Docstring needs to be updated

Done.

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/water...
appengine/findit/waterfall/flake/recursive_flake_try_job_pipeline.py:287: if
all_data_points[i].try_job_id is not None:
On 2017/01/13 00:47:33, chanli wrote:
> Nit: if all_data_points[i].try_job_id should also work. 
> 
> BTW, as we discussed offline, we can remove try_job_id and only use
try_job_url

Done.

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/water...
appengine/findit/waterfall/flake/recursive_flake_try_job_pipeline.py:327:
'max_dive_in_a_row': 4,
On 2017/01/13 00:47:34, chanli wrote:
> If don't handle dive for revision level, maybe we don't need these settings?

Done.

https://codereview.chromium.org/2630433002/diff/180001/appengine/findit/water...
appengine/findit/waterfall/flake/recursive_flake_try_job_pipeline.py:345:
next_commit_position >= suspected_build_data_point.commit_position):
On 2017/01/13 00:47:34, chanli wrote:
> In what situation next_commit_position can be greater than
> suspected_build_data_point.commit_position? I think if sequential run it
should
> be one by one?

Changed to ==.

[chanli, 2017-01-13 06:01:52]

LGTM with 2 more comments.

https://codereview.chromium.org/2630433002/diff/260001/appengine/findit/model...
File appengine/findit/model/flake/master_flake_analysis.py (right):

https://codereview.chromium.org/2630433002/diff/260001/appengine/findit/model...
appengine/findit/model/flake/master_flake_analysis.py:21: # analysis at the at
the build level.
Nit: at the at the

https://codereview.chromium.org/2630433002/diff/260001/appengine/findit/water...
File appengine/findit/waterfall/flake/recursive_flake_try_job_pipeline.py
(right):

https://codereview.chromium.org/2630433002/diff/260001/appengine/findit/water...
appengine/findit/waterfall/flake/recursive_flake_try_job_pipeline.py:213:
previous_pass_rate = data_points[i - 1].pass_rate if i > 0 else 0
The check for previous_pass_rate is essentially for dive. This check can also be
removed.

[chanli, 2017-01-13 17:50:35]

The CQ bit was checked by chanli@chromium.org

[commit-bot: I haz the power, 2017-01-13 17:50:43]

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, 2017-01-13 18:05:56]

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

[commit-bot: I haz the power, 2017-01-13 18:05:57]

Try jobs failed on following builders:
  Infra Linux Trusty 64 Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/33b1ccaa667c2f10)

[lijeffrey, 2017-01-20 18:47:55]

Description was changed from

==========
Findit] Flake Checker: Pipeline to trigger try jobs to identify flake culprits

BUG=
==========

to

==========
Findit] Flake Checker: Pipeline to trigger try jobs to identify flake culprits

BUG=

Landed in https://chromium-review.googlesource.com/#/c/428154/ and
https://chromium-review.googlesource.com/#/c/428298/
==========