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

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

BUG=672724
Review-Url: https://codereview.chromium.org/2608853002
Committed: https://chromium.googlesource.com/infra/infra/+/69514fdd8d27c2e4bed2e884971f3ce8d9c42185

[lijeffrey, 2016-12-30 04:27:29]

Description was changed from

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

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

to

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

BUG=672724
==========

[lijeffrey, 2016-12-30 04:27:29]

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

[lijeffrey, 2016-12-30 04:31:45]

ptal

[stgao, 2017-01-05 21:28:03]

https://codereview.chromium.org/2608853002/diff/40001/appengine/findit/model/...
File appengine/findit/model/flake/flake_suspected_cl.py (right):

https://codereview.chromium.org/2608853002/diff/40001/appengine/findit/model/...
appengine/findit/model/flake/flake_suspected_cl.py:11: """Represents information
about a flake suspected cl."""
"a suspected CL for a flake"?

As we run try job to verify, should we go with culprit instead of suspect?

https://codereview.chromium.org/2608853002/diff/40001/appengine/findit/model/...
appengine/findit/model/flake/flake_suspected_cl.py:17: code_review_url =
ndb.StringProperty(indexed=False)
Based on the code below, it seems not necessarily the url to the code review. It
could be the url to the commit log too.

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

https://codereview.chromium.org/2608853002/diff/40001/appengine/findit/model/...
appengine/findit/model/flake/master_flake_analysis.py:108: change_log =
self.GIT_REPO.GetChangeLog(revision)
It seems better to move this out to the caller code, and keep the ndb model just
to deal with data persistence and read.

What do you think?

https://codereview.chromium.org/2608853002/diff/40001/appengine/findit/model/...
appengine/findit/model/flake/master_flake_analysis.py:111: code_review_url =
change_log.code_review_url or change_log.commit_url
See comment above.

[lijeffrey, 2017-01-06 05:18:16]

https://codereview.chromium.org/2608853002/diff/40001/appengine/findit/model/...
File appengine/findit/model/flake/flake_suspected_cl.py (right):

https://codereview.chromium.org/2608853002/diff/40001/appengine/findit/model/...
appengine/findit/model/flake/flake_suspected_cl.py:11: """Represents information
about a flake suspected cl."""
On 2017/01/05 21:28:03, stgao (slow on Monday) wrote:
> "a suspected CL for a flake"?
> 
> As we run try job to verify, should we go with culprit instead of suspect?

Renamed FlakeCulprit etc.

https://codereview.chromium.org/2608853002/diff/40001/appengine/findit/model/...
appengine/findit/model/flake/flake_suspected_cl.py:17: code_review_url =
ndb.StringProperty(indexed=False)
On 2017/01/05 21:28:03, stgao (slow on Monday) wrote:
> Based on the code below, it seems not necessarily the url to the code review.
It
> could be the url to the commit log too.

Renamed 'url' to maintain flexibility depending what's available.

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

https://codereview.chromium.org/2608853002/diff/40001/appengine/findit/model/...
appengine/findit/model/flake/master_flake_analysis.py:108: change_log =
self.GIT_REPO.GetChangeLog(revision)
On 2017/01/05 21:28:03, stgao (slow on Monday) wrote:
> It seems better to move this out to the caller code, and keep the ndb model
just
> to deal with data persistence and read.
> 
> What do you think?

Sounds good, by moving change_log outside to the caller, this function is no
longer necessary since it simply crafts a FlakeCulprit entity and sets it. This
function will be implemented in the try job pipeline CL.

https://codereview.chromium.org/2608853002/diff/40001/appengine/findit/model/...
appengine/findit/model/flake/master_flake_analysis.py:111: code_review_url =
change_log.code_review_url or change_log.commit_url
On 2017/01/05 21:28:03, stgao (slow on Monday) wrote:
> See comment above.

Done.

[chanli, 2017-01-07 02:56:28]

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

https://codereview.chromium.org/2608853002/diff/80001/appengine/findit/model/...
appengine/findit/model/flake/master_flake_analysis.py:101: def
GetSuspectedFlakeDataPoint(self):
This function needs to be updated after our discussion at Friday afternoon,
right? since we will have only one group of data_points?

https://codereview.chromium.org/2608853002/diff/80001/appengine/findit/model/...
appengine/findit/model/flake/master_flake_analysis.py:178: # The status of the
try job, if any. None if no suspected flake build number
Flake revision instead of flake build number?

[lijeffrey, 2017-01-09 20:53:31]

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

https://codereview.chromium.org/2608853002/diff/80001/appengine/findit/model/...
appengine/findit/model/flake/master_flake_analysis.py:101: def
GetSuspectedFlakeDataPoint(self):
On 2017/01/07 02:56:28, chanli wrote:
> This function needs to be updated after our discussion at Friday afternoon,
> right? since we will have only one group of data_points?

Yes, per discussion we will have 1 group of data points with both build_number
and commit_position available depending on what was used to derive that point.
Renaming this function to GetDataPointOfSuspectedBuild. The returned data point
from this function is used to get blame_list to run try jobs to further identify
the culprit.

For the first version, I'm thinking we keep build_number since there are 2
stages of analysis (swarming tasks then try jobs). Since both can be time
consuming we should display the suspected build as soon as it's available even
as try jobs are still running

https://codereview.chromium.org/2608853002/diff/80001/appengine/findit/model/...
appengine/findit/model/flake/master_flake_analysis.py:178: # The status of the
try job, if any. None if no suspected flake build number
On 2017/01/07 02:56:28, chanli wrote:
> Flake revision instead of flake build number?

Renamed "None if try jobs have not been triggered" to be more specific.

[chanli, 2017-01-10 00:28:35]

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

https://codereview.chromium.org/2608853002/diff/140001/appengine/findit/model...
appengine/findit/model/flake/flake_culprit.py:10: class
FlakeCulprit(BaseSuspectedCL):
What do you think if we use FlakeSuspectedCL?

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

https://codereview.chromium.org/2608853002/diff/140001/appengine/findit/model...
appengine/findit/model/flake/master_flake_analysis.py:176: # The status of the
try job, if any. None if try jobs have not been triggered.
So we should have a series of try jobs, right? Which try job's status would be
checked?

[lijeffrey, 2017-01-10 01:16:25]

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

https://codereview.chromium.org/2608853002/diff/140001/appengine/findit/model...
appengine/findit/model/flake/flake_culprit.py:10: class
FlakeCulprit(BaseSuspectedCL):
On 2017/01/10 00:28:35, chanli wrote:
> What do you think if we use FlakeSuspectedCL?

I originally called it that but Shuotao suggested FlakeCulprit since it's backed
by a try job. I think either way works, and later if we use heuristics to help
guide these try jobs (i.e. added a new test) we may want to also have
FlakeSuspectedCL as one of the heuristic results. Just an idea though.

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

https://codereview.chromium.org/2608853002/diff/140001/appengine/findit/model...
appengine/findit/model/flake/master_flake_analysis.py:176: # The status of the
try job, if any. None if try jobs have not been triggered.
On 2017/01/10 00:28:35, chanli wrote:
> So we should have a series of try jobs, right? Which try job's status would be
> checked?

Since try jobs will be triggered serially, this is the whole try job pipeline
status. This should be set as RUNNING when the first try job is triggered, and
COMPLETED when the last try job is done. If any try job in the middle fails it
will set it as ERROR and stop.

WDYT?

[chanli, 2017-01-10 01:42:27]

lgtm with a nit

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

https://codereview.chromium.org/2608853002/diff/140001/appengine/findit/model...
appengine/findit/model/flake/master_flake_analysis.py:176: # The status of the
try job, if any. None if try jobs have not been triggered.
On 2017/01/10 01:16:25, lijeffrey wrote:
> On 2017/01/10 00:28:35, chanli wrote:
> > So we should have a series of try jobs, right? Which try job's status would
be
> > checked?
> 
> Since try jobs will be triggered serially, this is the whole try job pipeline
> status. This should be set as RUNNING when the first try job is triggered, and
> COMPLETED when the last try job is done. If any try job in the middle fails it
> will set it as ERROR and stop.
> 
> WDYT?

SGTM. But to be clear, could you update the comment?

[lijeffrey, 2017-01-10 02:07:06]

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

https://codereview.chromium.org/2608853002/diff/140001/appengine/findit/model...
appengine/findit/model/flake/master_flake_analysis.py:176: # The status of the
try job, if any. None if try jobs have not been triggered.
On 2017/01/10 01:42:27, chanli wrote:
> On 2017/01/10 01:16:25, lijeffrey wrote:
> > On 2017/01/10 00:28:35, chanli wrote:
> > > So we should have a series of try jobs, right? Which try job's status
would
> be
> > > checked?
> > 
> > Since try jobs will be triggered serially, this is the whole try job
pipeline
> > status. This should be set as RUNNING when the first try job is triggered,
and
> > COMPLETED when the last try job is done. If any try job in the middle fails
it
> > will set it as ERROR and stop.
> > 
> > WDYT?
> 
> SGTM. But to be clear, could you update the comment?

Done.

[lijeffrey, 2017-01-10 02:07:19]

The CQ bit was checked by lijeffrey@chromium.org

[lijeffrey, 2017-01-10 02:07:19]

The patchset sent to the CQ was uploaded after l-g-t-m from chanli@chromium.org
Link to the patchset: https://codereview.chromium.org/2608853002/#ps160001
(title: "Addressing comment")

[commit-bot: I haz the power, 2017-01-10 02:07:27]

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-10 02:17:25]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1484014039197270,
"parent_rev": "ad415a77da258b6bf8a1ef22c81e1960f5ac5586", "commit_rev":
"69514fdd8d27c2e4bed2e884971f3ce8d9c42185"}

[commit-bot: I haz the power, 2017-01-10 02:17:29]

Description was changed from

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

BUG=672724
==========

to

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

BUG=672724

Review-Url: https://codereview.chromium.org/2608853002
Committed:
https://chromium.googlesource.com/infra/infra/+/69514fdd8d27c2e4bed2e884971f3...
==========

[commit-bot: I haz the power, 2017-01-10 02:17:30]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/infra/infra/+/69514fdd8d27c2e4bed2e884971f3...