Add bisect config validation.

Add bisect config validation.

BUG=485672
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=299450

[qyearsley, 2016-03-11 01:41:33]

qyearsley@chromium.org changed reviewers:
+ chrisphan@chromium.org, robertocn@chromium.org

[qyearsley, 2016-03-11 01:41:34]

After this CL, a step failure would be raised in the config step if a config
parameter has invalid value type.

How else should this be improved?

Chris, you wrote a dict validation function in the perf dashboard recently, how
do you think this can be improved?

https://codereview.chromium.org/1782333002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/auto_bisect/bisector.py (right):

https://codereview.chromium.org/1782333002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/bisector.py:212: raise
self.api.m.step.StepFailure(error.message)
Would this provide everything in the buildbot job UI that we want? Or should I
print the error message in some other way?

https://codereview.chromium.org/1782333002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/auto_bisect/config_validation.py (right):

https://codereview.chromium.org/1782333002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/config_validation.py:25: 'try_job_id':
{'type': 'integer'},
Anything else that should be added? Or anything that should be removed?

https://codereview.chromium.org/1782333002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/config_validation.py:37:
validate_key(config, schema, key)
Potentially to be added: check revision order and distance. This could also be
done after expanding the revision range.

[qyearsley, 2016-03-11 01:41:48]

Description was changed from

==========
Add config validation.

BUG=485672
==========

to

==========
Add bisect config validation.

BUG=485672
==========

[chrisphan, 2016-03-11 02:00:04]

On 2016/03/11 01:41:34, qyearsley wrote:
> After this CL, a step failure would be raised in the config step if a config
> parameter has invalid value type.
> 
> How else should this be improved?

Doesn't need to be in this CL.  But now that we can easily call
bisector.post_result at anytime, we can start thinking of sending bisect
failures to dashboard (those that we can capture).

> 
> Chris, you wrote a dict validation function in the perf dashboard recently,
how
> do you think this can be improved?
> 

Yea, didn't find dashboard.utils.Validate reusable?  If you rather use this, I
think it's fine.

>
https://codereview.chromium.org/1782333002/diff/1/scripts/slave/recipe_module...
> File scripts/slave/recipe_modules/auto_bisect/bisector.py (right):
> 
>
https://codereview.chromium.org/1782333002/diff/1/scripts/slave/recipe_module...
> scripts/slave/recipe_modules/auto_bisect/bisector.py:212: raise
> self.api.m.step.StepFailure(error.message)
> Would this provide everything in the buildbot job UI that we want? Or should I
> print the error message in some other way?
> 
>
https://codereview.chromium.org/1782333002/diff/1/scripts/slave/recipe_module...
> File scripts/slave/recipe_modules/auto_bisect/config_validation.py (right):
> 
>
https://codereview.chromium.org/1782333002/diff/1/scripts/slave/recipe_module...
> scripts/slave/recipe_modules/auto_bisect/config_validation.py:25:
'try_job_id':
> {'type': 'integer'},
> Anything else that should be added? Or anything that should be removed?
> 
>
https://codereview.chromium.org/1782333002/diff/1/scripts/slave/recipe_module...
> scripts/slave/recipe_modules/auto_bisect/config_validation.py:37:
> validate_key(config, schema, key)
> Potentially to be added: check revision order and distance. This could also be
> done after expanding the revision range.

[chrisphan, 2016-03-11 02:00:11]

lgtm

[qyearsley, 2016-03-11 16:30:30]

On 2016/03/11 at 02:00:04, chrisphan wrote:
> On 2016/03/11 01:41:34, qyearsley wrote:
> > After this CL, a step failure would be raised in the config step if a config
> > parameter has invalid value type.
> > 
> > How else should this be improved?
> 
> Doesn't need to be in this CL.  But now that we can easily call
bisector.post_result at anytime, we can start thinking of sending bisect
failures to dashboard (those that we can capture).

Roberto and others, what do you think of potentially trying to send all fatal
failures? (If this sounds like a good idea, we should probably file a separate
bug for it.)

> > 
> > Chris, you wrote a dict validation function in the perf dashboard recently,
how
> > do you think this can be improved?
> > 
> 
> Yea, didn't find dashboard.utils.Validate reusable?  If you rather use this, I
think it's fine.

Copying over that function
(https://github.com/catapult-project/catapult/blob/master/dashboard/dashboard/...)
would also work -- yesterday I wanted to try out writing this to see how it
turned out, but whatever is most maintainable in the long-run should be used
here -- later this might be replaced by a call to a common function in catapult,
or we could use one of the third-party libraries that does this (validate,
jsonschema, validictory).

[qyearsley, 2016-03-13 23:00:00]

The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-13 23:00:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1782333002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1782333002/1

[commit-bot: I haz the power, 2016-03-13 23:06:54]

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

[commit-bot: I haz the power, 2016-03-13 23:06:54]

Dry run: Try jobs failed on following builders:
  Build Presubmit on tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Build%20Presubmit/build...)

[qyearsley, 2016-03-13 23:45:54]

On 2016/03/13 at 23:06:54, commit-bot wrote:
> Dry run: Try jobs failed on following builders:
>   Build Presubmit on tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Build%20Presubmit/build...)

This CL failed the presubmit for the recipe expectations test
bisect_test/example_bisect_results; this will need to be fixed before this CL is
ready to commit.

[RobertoCN, 2016-03-14 01:26:23]

https://codereview.chromium.org/1782333002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/auto_bisect/bisector.py (right):

https://codereview.chromium.org/1782333002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/bisector.py:212: raise
self.api.m.step.StepFailure(error.message)
On 2016/03/11 at 01:41:34, qyearsley wrote:
> Would this provide everything in the buildbot job UI that we want? Or should I
print the error message in some other way?

Just set the step to failed as well before raising the StepFailure:
self.api.m.step.active_result.presentation.status = self.api.m.step.FAILURE

If you wanted the failure to be its own step you could use the halt recipe
module.

https://codereview.chromium.org/1782333002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/auto_bisect/config_validation.py (right):

https://codereview.chromium.org/1782333002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/config_validation.py:25: 'try_job_id':
{'type': 'integer'},
On 2016/03/11 at 01:41:34, qyearsley wrote:
> Anything else that should be added? Or anything that should be removed?

Metric could be required if the bisect_mode is mean or std_dev.
Truncate percent is mostly deprecated, right? (except for clank)

https://codereview.chromium.org/1782333002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/config_validation.py:37:
validate_key(config, schema, key)
On 2016/03/11 at 01:41:34, qyearsley wrote:
> Potentially to be added: check revision order and distance. This could also be
done after expanding the revision range.

could also check for well-formedness of gs location and url, and also metric.

[qyearsley, 2016-03-18 22:50:04]

The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-18 22:50:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1782333002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1782333002/40001

[qyearsley, 2016-03-18 22:50:19]

https://codereview.chromium.org/1782333002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/auto_bisect/bisector.py (right):

https://codereview.chromium.org/1782333002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/bisector.py:212: raise
self.api.m.step.StepFailure(error.message)
On 2016/03/14 at 01:26:23, RobertoCN wrote:
> On 2016/03/11 at 01:41:34, qyearsley wrote:
> > Would this provide everything in the buildbot job UI that we want? Or should
I print the error message in some other way?
> 
> Just set the step to failed as well before raising the StepFailure:
> self.api.m.step.active_result.presentation.status = self.api.m.step.FAILURE
> 
> If you wanted the failure to be its own step you could use the halt recipe
module.

Done; making it it's own step may make it be clearer when it fails.

https://codereview.chromium.org/1782333002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/auto_bisect/config_validation.py (right):

https://codereview.chromium.org/1782333002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/config_validation.py:25: 'try_job_id':
{'type': 'integer'},
On 2016/03/14 at 01:26:23, RobertoCN wrote:
> On 2016/03/11 at 01:41:34, qyearsley wrote:
> > Anything else that should be added? Or anything that should be removed?
> 
> Metric could be required if the bisect_mode is mean or std_dev.
> Truncate percent is mostly deprecated, right? (except for clank)

Check for metric added; truncate_percent removed from the default config schema.

https://codereview.chromium.org/1782333002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/config_validation.py:37:
validate_key(config, schema, key)
On 2016/03/14 at 01:26:23, RobertoCN wrote:
> On 2016/03/11 at 01:41:34, qyearsley wrote:
> > Potentially to be added: check revision order and distance. This could also
be done after expanding the revision range.
> 
> could also check for well-formedness of gs location and url, and also metric.

In most bisect configs, there aren't any gs URLs, but there is a gs bucket.

Example:
https://build.chromium.org/p/tryserver.chromium.perf/builders/linux_perf_bise...

Checking for revision order and metric well-formedness added.

[commit-bot: I haz the power, 2016-03-18 22:54:26]

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

[commit-bot: I haz the power, 2016-03-18 22:54:27]

Dry run: This issue passed the CQ dry run.

[RobertoCN, 2016-03-23 02:12:12]

lgtm

[qyearsley, 2016-03-23 02:30:33]

The CQ bit was checked by qyearsley@chromium.org

[qyearsley, 2016-03-23 02:30:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrisphan@chromium.org
Link to the patchset: https://codereview.chromium.org/1782333002/#ps40001
(title: "Fix bisect_test config, add example failed config expectation, respond
to feedback")

[commit-bot: I haz the power, 2016-03-23 02:30:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1782333002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1782333002/40001

[commit-bot: I haz the power, 2016-03-23 02:32:48]

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

[commit-bot: I haz the power, 2016-03-23 02:32:49]

Try jobs failed on following builders:
  Build Try Recipe Test Mac on tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Build%20Try%20Recipe%20...)
  Build Try Recipe Test Trusty64 on tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Build%20Try%20Recipe%20...)

[qyearsley, 2016-03-23 02:37:50]

On 2016/03/23 at 02:32:49, commit-bot wrote:
> Try jobs failed on following builders:
>   Build Try Recipe Test Mac on tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Build%20Try%20Recipe%20...)
>   Build Try Recipe Test Trusty64 on tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Build%20Try%20Recipe%20...)

(Patch failure, must be rebased)

[qyearsley, 2016-03-23 20:58:27]

The CQ bit was checked by qyearsley@chromium.org

[qyearsley, 2016-03-23 20:58:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertocn@chromium.org, chrisphan@chromium.org
Link to the patchset: https://codereview.chromium.org/1782333002/#ps60001
(title: "Rebased")

[commit-bot: I haz the power, 2016-03-23 20:58:31]

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

[commit-bot: I haz the power, 2016-03-23 21:06:01]

Description was changed from

==========
Add bisect config validation.

BUG=485672
==========

to

==========
Add bisect config validation.

BUG=485672

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=299450
==========

[commit-bot: I haz the power, 2016-03-23 21:06:02]

Committed patchset #4 (id:60001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=299450

[qyearsley, 2016-03-24 16:42:55]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1831843002/ by qyearsley@chromium.org.

The reason for reverting is:
https://bugs.chromium.org/p/chromium/issues/detail?id=597655.