[Findit] Capture versionized metadata for master_flake_analysis

appengine/findit/waterfall/flake/initialize_flake_pipeline.py

 # Copyright 2016 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
-from google.appengine.ext import ndb
-
 from common import appengine_util
 from common import constants
 from model import analysis_status
 from model.flake.master_flake_analysis import MasterFlakeAnalysis
 from waterfall import waterfall_config
 from waterfall.flake.recursive_flake_pipeline import RecursiveFlakePipeline
 
 
-@ndb.transactional
 def NeedANewAnalysis(

[chanli, 2016/09/27 22:15:24]

I think we still need to make it transactional?

[lijeffrey, 2016/09/28 03:12:31]

The Save() call already happens in a transaction, so this should not be
necessary if I understand correctly. Save() also does an assert that ndb is not
in a transaction, which will complain if @ndb.transactional is left up there.

     master_name, builder_name, build_number, step_name, test_name,
     allow_new_analysis=False):
   """Checks status of analysis for the test and decides if a new one is needed.
 
   A MasterFlakeAnalysis entity for the given parameters will be created if none
   exists. When a new analysis is needed, this function will create and
-  save a MasterFlakeAnalysis entity to the datastore.
+  save a MasterFlakeAnalysis entity to the datastore. TODO(lijeffrey): add
+  support for a force flag to rerun this analysis.

[chanli, 2016/09/27 22:15:24]

Nit: Maybe move this TODO to a separate line?

[lijeffrey, 2016/09/28 03:12:31]

Done.

 
   Returns:
     True if an analysis is needed, otherwise False.
   """
-  master_flake_analysis = MasterFlakeAnalysis.Get(
+  master_flake_analysis = MasterFlakeAnalysis.GetVersion(
       master_name, builder_name, build_number, step_name, test_name)
 
   if not master_flake_analysis:
     if not allow_new_analysis:
       return False
     master_flake_analysis = MasterFlakeAnalysis.Create(
         master_name, builder_name, build_number, step_name, test_name)
-    master_flake_analysis.status = analysis_status.PENDING
-    master_flake_analysis.put()
-    return True
+    master_flake_analysis.Reset()

[stgao, 2016/09/28 00:03:24]

Why do we need reset here?

[lijeffrey, 2016/09/28 03:12:31]

We need to clear the fields, especially swarming_rerun_results since a rerun of
this analysis (if the algorithm parameters change or more caching becomes
available) will result in slightly different build numbers that were analyzed.
Additionally all previous fields should be cleared first before being written in
case this task ends prematurely in an error the previous version's fields aren't
accidentally left stale.

[stgao, 2016/09/30 21:07:31]

But it is newly created, there is no leftover state.

[lijeffrey, 2016/10/01 01:28:04]

Ah yes you are correct, I was looking at the wrong branch. I believe this
Reset() call has been removed in the latest patch.

+    _, saved = master_flake_analysis.Save()
+    if saved:

[stgao, 2016/09/28 00:03:24]

Can we just return saved directly?

[lijeffrey, 2016/09/28 03:12:31]

Done.

+      return True
+    else:  # pragma: no cover.
+      # Another transaction has just triggered tihs analysis. TODO(lijeffrey):
+      # Indicate to the user to wait for the other analysis' results amd remove
+      # the no cover pragma.

[chanli, 2016/09/27 22:15:24]

Same here

[lijeffrey, 2016/09/28 03:12:31]

Done.

+      return False
   elif (master_flake_analysis.status == analysis_status.COMPLETED or
         master_flake_analysis.status == analysis_status.PENDING or
         master_flake_analysis.status == analysis_status.RUNNING):
     return False
   else:
-    # TODO(caiw): Reset method.
-    MasterFlakeAnalysis.Get(
-        master_name, builder_name, build_number,
-        step_name, test_name).key.delete()
-    master_flake_analysis = MasterFlakeAnalysis.Create(
-        master_name, builder_name, build_number, step_name, test_name)
-    master_flake_analysis.status = analysis_status.PENDING
-    master_flake_analysis.put()
-    return True
+    # The previous analysis had some error, so reset it and create a new one.
+    master_flake_analysis.Reset()
+    _, saved = master_flake_analysis.Save()
+    if saved:  # pragma: no branch

[stgao, 2016/09/28 00:03:24]

same here.

[lijeffrey, 2016/10/01 01:28:04]

Done.

+      return True
+    else:  # pragma: no cover.
+      # Another transaction has just triggered tihs analysis. TODO(lijeffrey):
+      # Indicate to the user to wait for the other analysis' results amd remove
+      # the no cover pragma.
+      return False
 
 
 # Unused arguments - pylint: disable=W0612, W0613
 def ScheduleAnalysisIfNeeded(master_name, builder_name, build_number, step_name,
                              test_name, allow_new_analysis=False, force=False,
                              queue_name=constants.DEFAULT_QUEUE):
   """Schedules an analysis if needed and returns the MasterFlakeAnalysis.
 
   When the build failure was already analyzed and a new analysis is scheduled,
   the returned WfAnalysis will still have the result of last completed analysis.
 
   Args:
     master_name (str): The master name of the failed test
     builder_name (str): The builder name of the failed test
     build_number (int): The build number of the failed test
     step_name (str): The name of the test suite
     test_name (str): The single test we are checking
     allow_new_analysis (bool): Indicate whether a new analysis is allowed.
     force (bool): Indicate whether to force a rerun of current analysis.
     queue_name (str): The App Engine queue to run the analysis.
 
   Returns:
     A MasterFlakeAnalysis instance.
     None if no analysis was scheduled and the user has no permission to.
   """
   if NeedANewAnalysis(
       master_name, builder_name, build_number, step_name, test_name,
       allow_new_analysis):
+    master_flake_analysis = MasterFlakeAnalysis.GetVersion(
+        master_name, builder_name, build_number, step_name, test_name)
     check_flake_settings = waterfall_config.GetCheckFlakeSettings()
+
+    # TODO(lijeffrey): Allow for reruns with custom parameters if the user is
+    # not satisfied with with the results generated by the default ones provided

[stgao, 2016/09/28 00:03:24]

typo: with with

[lijeffrey, 2016/09/28 03:12:31]

Done.

+    # by waterfall_config and record the custom ones here.
+    master_flake_analysis.algorithm_parameters = check_flake_settings
+    master_flake_analysis.put()
+
     max_build_numbers_to_look_back = check_flake_settings.get(
         'max_build_numbers_to_look_back')
     flakiness_algorithm_results_dict = {
         'flakes_in_a_row': 0,
         'stable_in_a_row': 0,
         'stabled_out': False,
         'flaked_out': False,
         'last_build_number': max(
             0, build_number - max_build_numbers_to_look_back),
         'lower_boundary': None,
         'upper_boundary': None,
         'lower_boundary_result': None,
         'sequential_run_index': 0
     }
+
     pipeline_job = RecursiveFlakePipeline(
         master_name, builder_name, build_number, step_name, test_name,

[stgao, 2016/09/28 00:03:24]

Should we pass the version of the master_flake_analysis into the pipeline so
that only that version will be updated?

[lijeffrey, 2016/09/28 03:12:31]

I think this is a good idea. Originally the design for not including version
number is it's assumed that always the latest version will be the one to update,
but this is not the case:

For experimental purposes it is possible 1 user triggers 2 identical check flake
analyses but with different algorithm parameters side by side, and in this case
without passing version number the incorrect version will be updated.

Passing version in all cases.

[stgao, 2016/09/30 21:07:31]

I'm against running two concurrent analysis for the same flake. That would
introduce unneeded complexity.
Does the function ``NeedANewAnalysis`` above supports this case already? If so,
let's comment the code out for now. Later we could revisit the code.

[lijeffrey, 2016/10/01 01:28:04]

NeedANewAnalysis as is does not support running 2 concurrent analyses, so no
further change here is necessary.

         master_build_number=build_number,
         flakiness_algorithm_results_dict=flakiness_algorithm_results_dict)
     pipeline_job.target = appengine_util.GetTargetNameForModule(
         constants.WATERFALL_BACKEND)
     pipeline_job.start(queue_name=queue_name)
-  return MasterFlakeAnalysis.Get(
+  return MasterFlakeAnalysis.GetVersion(
       master_name, builder_name, build_number, step_name, test_name)