Support variable substitution and tryserver- and waterfall-only cmd line args.

Support variable substitution and tryserver- and waterfall-only cmd line args.

This adds support to the src-side JSON files for substituting the
following variables in the command-line arguments to tests:

  ${buildername}
  ${got_revision}

and also supports setting command line arguments only on the tryservers,
and only on the waterfall bots.

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

[Ken Russell (switch to Gerrit), 2016-01-13 23:45:31]

kbr@chromium.org changed reviewers:
+ iannucci@chromium.org, luqui@chromium.org, phajdan.jr@chromium.org,
sergeyberezin@chromium.org, sergiyb@chromium.org, tandrii@chromium.org

[Ken Russell (switch to Gerrit), 2016-01-13 23:45:31]

Please review. Thanks.

[Sergey Berezin, 2016-01-14 01:39:34]

I didn't have time to review this properly, just got one comment. Hopefully
someone else gets to review this later...

https://codereview.chromium.org/1588793002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/chromium_tests/steps.py (right):

https://codereview.chromium.org/1588793002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/steps.py:359: 'master_name':
api.properties.get('mastername'),
nit: IMHO, master name is an implementation detail of the infrastructure, and
we'll be getting rid of masters anyway in a near future. This would add more
dependencies on master names down the stack, which moves us in the opposite
direction.

If master_name is needed for deduplicating builder names, maybe we should
instead use "mastername:buildername" strings. I don't know...

[Sergiy Byelozyorov, 2016-01-14 09:43:47]

lgtm

https://codereview.chromium.org/1588793002/diff/1/scripts/slave/recipes/chrom...
File scripts/slave/recipes/chromium.expected/dynamic_gtest.json (right):

https://codereview.chromium.org/1588793002/diff/1/scripts/slave/recipes/chrom...
scripts/slave/recipes/chromium.expected/dynamic_gtest.json:151:
"@@@STEP_LOG_LINE@json.output@         
\"--THIS-ARG-SHOULD-NOT-BE-PRESENT\"@@@",
This is confusing.

https://codereview.chromium.org/1588793002/diff/1/scripts/slave/recipes/chrom...
File
scripts/slave/recipes/chromium_trybot.expected/dynamic_gtest_with_args_on_trybot.json
(right):

https://codereview.chromium.org/1588793002/diff/1/scripts/slave/recipes/chrom...
scripts/slave/recipes/chromium_trybot.expected/dynamic_gtest_with_args_on_trybot.json:145:
"@@@STEP_LOG_LINE@json.output@         
\"--THIS-ARG-SHOULD-NOT-BE-PRESENT\"@@@",
This is actually confusing when reading expectations. This is a test spec, so it
should be present here, but not below.

[Paweł Hajdan Jr., 2016-01-14 15:51:02]

"not lgtm"

Thanks for the patch. That gives us something specific to discuss.

FWIW the idea of variable substitution is quite clear to me, but rationale or
use case isn't really.

Please take a look at chromium_tests.get_common_args_for_scripts . Several
properties (including mastername, buildername and buildnumber) are already being
made available to src-side scripts. Let's not introduce another mechanism for
the same thing. Sorry I forgot about this when replying on the bug.

We can discuss further remaining aspects of the patch, such as precommit mode.
I'd really like to understand the rationale - that's my main question.

[iannucci, 2016-01-15 01:48:11]

FWIW, this lgtm. I like the arg substitution approach a lot better than the
pass-the-blob-of-properties approach. We should switch the pass-the-blob
mechanism to use string substitution instead.

The pass-the-blob mechanism has the problem that it makes it impossible to know
which scripts are using which data. The string-substitution mechanism allows us
to grep/audit to see which tests are actually using which data, which is really
important as we do things like transition away from buildbot.

The list of properties that the recipes are willing to substitute should be
clearly documented. It's pretty clear-ish in this CL, but I would make an
explicit list/callout for it in a docstring somewhere prominent and greppable.

https://chromiumcodereview.appspot.com/1588793002/diff/1/scripts/slave/recipe...
File scripts/slave/recipe_modules/chromium_tests/steps.py (right):

https://chromiumcodereview.appspot.com/1588793002/diff/1/scripts/slave/recipe...
scripts/slave/recipe_modules/chromium_tests/steps.py:359: 'master_name':
api.properties.get('mastername'),
On 2016/01/14 at 01:39:34, Sergey Berezin wrote:
> nit: IMHO, master name is an implementation detail of the infrastructure, and
we'll be getting rid of masters anyway in a near future. This would add more
dependencies on master names down the stack, which moves us in the opposite
direction.
> 
> If master_name is needed for deduplicating builder names, maybe we should
instead use "mastername:buildername" strings. I don't know...

What sergey said is true... 

That said, if we're substituting properties, I'd prefer the substitution names
to be 1:1 with the property names for easier tracing/debugging ability.

[Ken Russell (switch to Gerrit), 2016-01-15 02:19:44]

Description was changed from

==========
Support variable substitution and tryserver- and waterfall-only cmd line args.

This adds support to the src-side JSON files for substituting the
following variables in the command-line arguments to tests:

  ${master_name}
  ${builder_name}
  ${build_revision}

and also supports setting command line arguments only on the tryservers,
and only on the waterfall bots.

BUG=573321
==========

to

==========
Support variable substitution and tryserver- and waterfall-only cmd line args.

This adds support to the src-side JSON files for substituting the
following variables in the command-line arguments to tests:

  ${buildername}
  ${got_revision}

and also supports setting command line arguments only on the tryservers,
and only on the waterfall bots.

BUG=573321
==========

[Ken Russell (switch to Gerrit), 2016-01-15 02:51:01]

Addressed review feedback:

1) Removed the mastername property. (The GPU tests didn't need it.)
2) Made the substituted variables' names the same as the build properties'
names.
3) Documented the (now two) supported variables in chromium_tests/steps.py.

https://codereview.chromium.org/1582423004 has been uploaded which will use this
functionality.

Please approve this CL. I investigated the possibility of reusing
api.chromium_tests.get_common_args_for_scripts and it would make the code more
complex, couple the tools/build and Chromium workspaces more tightly, and make
it less clear which properties were being used.

[Paweł Hajdan Jr., 2016-01-15 14:32:51]

On 2016/01/15 at 02:51:01, kbr wrote:
> https://codereview.chromium.org/1582423004 has been uploaded which will use
this functionality.
> 
> Please approve this CL. I investigated the possibility of reusing
api.chromium_tests.get_common_args_for_scripts and it would make the code more
complex, couple the tools/build and Chromium workspaces more tightly, and make
it less clear which properties were being used.

Thanks, the above CL adds some helpful context.

At least for now I'm not convinced by above reasoning. It does refer to a
preference and makes some sense. However, what I find missing is why it'd be
*necessary* to add another mechanism. Arguably, having two ways to do
essentially the same thing does add up to the confusion.

On Robbie's comment: yes, pass-the-blob makes it less clear which properties can
be removed. The idea when it was being introduced was that *all* properties
would be go away, and we'd provide a subset of them for legacy use cases.

Also related to above, and tight coupling argument: my plan is to move these
recipes to the chromium repo. Eventually I see this API boundary disappear - it
was only introduced because the two sides are currently in different repos.

On precommit/postcommit: while these changes seem to accomplish some goal, it's
not obvious to me if that's the right architectural choice. In general, we're
moving towards a world where precommit and postcommit jobs look as similar as
possible. Also, we're moving towards idempotent tasks, where all dependencies
etc. are isolated. Downloads/uploads between the bot and GS don't seem to fit
well within that world. I could see this more as an explicit isolated
input/output, possibly followed by explicit upload to GS, or preceded by
explicit download from GS if really needed.

We can continue discussing the best options here. I'm wondering what you think
about above points.

[Paweł Hajdan Jr., 2016-01-15 15:00:44]

I'd like to clarify the context for my above comment: to some degree I'm getting
an impression you might be pushing for a specific solution and specific changes
here, and I have some concerns about these changes. That "push" tends to
generate some "pushback" from me.

However, you're raising valid questions and I'd like to help finding and
implementing solutions to these.

[iannucci, 2016-01-15 23:28:51]

On 2016/01/15 14:32:51, Paweł Hajdan Jr. wrote:
> On 2016/01/15 at 02:51:01, kbr wrote:
> > https://codereview.chromium.org/1582423004 has been uploaded which will use
> this functionality.
> > 
> > Please approve this CL. I investigated the possibility of reusing
> api.chromium_tests.get_common_args_for_scripts and it would make the code more
> complex, couple the tools/build and Chromium workspaces more tightly, and make
> it less clear which properties were being used.
> 
> Thanks, the above CL adds some helpful context.
> 
> At least for now I'm not convinced by above reasoning. It does refer to a
> preference and makes some sense. However, what I find missing is why it'd be
> *necessary* to add another mechanism. Arguably, having two ways to do
> essentially the same thing does add up to the confusion.
> 
> On Robbie's comment: yes, pass-the-blob makes it less clear which properties
can
> be removed. The idea when it was being introduced was that *all* properties
> would be go away, and we'd provide a subset of them for legacy use cases.
> 

This changes my perspective slightly; if the goal is to remove the entire
properties blob
(which sounds good to me!), then re-using that mechanism instead of refining it
(by doing substitution) sounds fine. Of course, refining it /everywhere/ to do 
substitution would be an incremental step towards completely eliminating it, but
if
no one is signing up to do the transition from blob -> substitution, then it
will most
likely not happen, which does mean we'd end up with two mechanisms :(. If that's
the case, then I agree with Paweł here that we should just add the (1?) new
property
to the blob and use that.

> Also related to above, and tight coupling argument: my plan is to move these
> recipes to the chromium repo. Eventually I see this API boundary disappear -
it
> was only introduced because the two sides are currently in different repos.

Right, but this CL is more immediate than that.

> 
> On precommit/postcommit: while these changes seem to accomplish some goal,
it's
> not obvious to me if that's the right architectural choice. In general, we're
> moving towards a world where precommit and postcommit jobs look as similar as
> possible. Also, we're moving towards idempotent tasks, where all dependencies
> etc. are isolated. Downloads/uploads between the bot and GS don't seem to fit
> well within that world. I could see this more as an explicit isolated
> input/output, possibly followed by explicit upload to GS, or preceded by
> explicit download from GS if really needed.

^ I agree with this point in the medium term, but this may be prohibitively
expensive
to do currently (e.g. the build artifacts would need to round-trip to the bot,
which
sucks from a time/golo bandwidth PoV).

Ken, what is the wallclock time (or just the size of these outputs) that it
takes to
download the isolated versions of these and then re-upload them? If we're
talking 
<45s, then it's probably fine, but if it's more than that then I think some
temporary
step (like passing the master name so the test can do the upload itself) is
probably
warranted. I demand trial by data :)

> 
> We can continue discussing the best options here. I'm wondering what you think
> about above points.

[Ken Russell (switch to Gerrit), 2016-01-20 01:59:50]

Two main points in response.

1. The isolated_script_test step type, which the GPU tests use, do not currently
support passing a blob of build properties from the recipe side like
script_tests do. I am highly reluctant to add this support. As I mentioned
above, doing so will add another contract between the Chromium and infra
workspaces, and the exact same variable substitution will have to be implemented
inside isolated_script_tests anyway. I think it is better to just allow these
variables to be referenced in the arguments that the JSON files *already*
support.

2. The mechanism by which the GPU pixel tests' golden images are produced and
tested against can certainly be redesigned to be more compatible with Swarming's
design goals. This would entail changing the pixel tests to no longer read or
write cloud storage directly, but instead use some other mechanism. It might
allow the distinction between "precommit" and "non-precommit" jobs to be
removed. I would be happy to work with the infra team in the future to make this
change, but this work is *out of scope* with respect to the goal of retiring the
GPU recipe. During this transition, the existing tests need to continue to run
in the same way they currently do.

I ask again that this CL please be unblocked so that
https://codereview.chromium.org/1582423004 can land, and so that the GPU recipe
can be retired. I don't think this CL adds an undue maintenance burden nor
highly undesirable semantics.

[iannucci, 2016-01-20 02:05:19]

On 2016/01/20 01:59:50, Ken Russell wrote:
> Two main points in response.
> 
> 1. The isolated_script_test step type, which the GPU tests use, do not
currently
> support passing a blob of build properties from the recipe side like
> script_tests do. I am highly reluctant to add this support. As I mentioned
> above, doing so will add another contract between the Chromium and infra
> workspaces, and the exact same variable substitution will have to be
implemented
> inside isolated_script_tests anyway. I think it is better to just allow these
> variables to be referenced in the arguments that the JSON files *already*
> support.

Fair enough. isolated_script_test is a new interface which has the opportunity
to have
a smaller api than the existing script_tests do.

> 
> 2. The mechanism by which the GPU pixel tests' golden images are produced and
> tested against can certainly be redesigned to be more compatible with
Swarming's
> design goals. This would entail changing the pixel tests to no longer read or
> write cloud storage directly, but instead use some other mechanism. It might
> allow the distinction between "precommit" and "non-precommit" jobs to be
> removed. I would be happy to work with the infra team in the future to make
this
> change, but this work is *out of scope* with respect to the goal of retiring
the
> GPU recipe. During this transition, the existing tests need to continue to run
> in the same way they currently do.

Followup work seems very reasonable to do. Clearly the existing script_tests use
the various properties already, and the gpu recipes are doing this already too.
The
win for this CL is to allow the gpu recipe to be retired into the main chromium*
recipes
form.

> 
> I ask again that this CL please be unblocked so that
> https://codereview.chromium.org/1582423004 can land, and so that the GPU
recipe
> can be retired. I don't think this CL adds an undue maintenance burden nor
> highly undesirable semantics.

lgtm

[iannucci, 2016-01-20 02:54:55]

To clarify my lgtm; I think this CL should land as-is.

I didn't remember that script_tests and isolated_script_test are different code
paths. isolated_script_test seems a lot simpler (yay!), and this is adding some
functionality to it that already exists in script_tests, but in a very
constrained way (yay!), which doesn't increase the burden on the various users
of this test mode (e.g. telemetry's test scripts don't need to change to
accept-and-ignore --build-properties) (yay!). I don't think the burden of having
two-ways-to-pass build properties is relevant; isolated_script_test has zero
ways to pass build properties before, and 1 way to pass build properties after
this CL. If anything, script_tests should remove --build-properties in favor of
this mechanism (in another CL, probably with a different author).

Re: interacting with google storage, we will definitely want to fix that at some
point (so that we can use the fast cached isolate mechanisms that swarming
provides), but I think that this CL (and the related
https://codereview.chromium.org/1582423004 CL) are PRE-requisites for changing
the way the tests interact with google storage, not the other way around. As we
transition all of chromium + recipes to swarming, I suspect we'll discover a lot
of this sort of cleanup work, and I think we'll end up dealing with it in the
same way: whitelist the weird thing, and transition to swarming, then using the
better tooling around swarming (i.e. luci services) to eliminate the need for
the weird thing and de-whitelist it. Changing the GPU tests to not access GCS
directly /now/ I think would likely be very impractical.

[Ken Russell (switch to Gerrit), 2016-01-20 03:09:00]

On 2016/01/20 02:54:55, iannucci wrote:
> To clarify my lgtm; I think this CL should land as-is.
> 
> I didn't remember that script_tests and isolated_script_test are different
code
> paths. isolated_script_test seems a lot simpler (yay!), and this is adding
some
> functionality to it that already exists in script_tests, but in a very
> constrained way (yay!), which doesn't increase the burden on the various users
> of this test mode (e.g. telemetry's test scripts don't need to change to
> accept-and-ignore --build-properties) (yay!). I don't think the burden of
having
> two-ways-to-pass build properties is relevant; isolated_script_test has zero
> ways to pass build properties before, and 1 way to pass build properties after
> this CL. If anything, script_tests should remove --build-properties in favor
of
> this mechanism (in another CL, probably with a different author).
> 
> Re: interacting with google storage, we will definitely want to fix that at
some
> point (so that we can use the fast cached isolate mechanisms that swarming
> provides), but I think that this CL (and the related
> https://codereview.chromium.org/1582423004 CL) are PRE-requisites for changing
> the way the tests interact with google storage, not the other way around. As
we
> transition all of chromium + recipes to swarming, I suspect we'll discover a
lot
> of this sort of cleanup work, and I think we'll end up dealing with it in the
> same way: whitelist the weird thing, and transition to swarming, then using
the
> better tooling around swarming (i.e. luci services) to eliminate the need for
> the weird thing and de-whitelist it. Changing the GPU tests to not access GCS
> directly /now/ I think would likely be very impractical.

In fairness, I should point out that transitioning the GPU bots from the GPU
recipe to the Chromium recipe doesn't significantly change how these tests
interact with Swarming; they're already swarmed. The goal of this work is to get
more bots onto the Chromium recipe and gain all of the attendant improvements
like the ability to add new tests via the src-side JSON files.

[Paweł Hajdan Jr., 2016-01-20 15:10:38]

On 2016/01/20 at 01:59:50, kbr wrote:
> Two main points in response.
> 
> 1. The isolated_script_test step type, which the GPU tests use, do not
currently support passing a blob of build properties from the recipe side like
script_tests do. I am highly reluctant to add this support. As I mentioned
above, doing so will add another contract between the Chromium and infra
workspaces, and the exact same variable substitution will have to be implemented
inside isolated_script_tests anyway. I think it is better to just allow these
variables to be referenced in the arguments that the JSON files *already*
support.

Thanks for pointing this out. Agreed.

> 2. The mechanism by which the GPU pixel tests' golden images are produced and
tested against can certainly be redesigned to be more compatible with Swarming's
design goals. This would entail changing the pixel tests to no longer read or
write cloud storage directly, but instead use some other mechanism. It might
allow the distinction between "precommit" and "non-precommit" jobs to be
removed. I would be happy to work with the infra team in the future to make this
change, but this work is *out of scope* with respect to the goal of retiring the
GPU recipe. During this transition, the existing tests need to continue to run
in the same way they currently do.

Thanks for the context. I agree. Could you just file a bug to track this work?

> I ask again that this CL please be unblocked so that
https://codereview.chromium.org/1582423004 can land, and so that the GPU recipe
can be retired. I don't think this CL adds an undue maintenance burden nor
highly undesirable semantics.

LGTM w/comment (please do apply it)

https://codereview.chromium.org/1588793002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium_tests/steps.py (right):

https://codereview.chromium.org/1588793002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_tests/steps.py:1177: args =
get_args_for_test(api, chromium_tests_api, spec, bot_update_step)
If the support is intended for isolated script test, can we please limit the
variable substitution just to that?

It'd indeed address my concern about two mechanisms for the same thing. We
indeed do not currently have a way to pass these properties to isolated script
test.

[Ken Russell (switch to Gerrit), 2016-01-21 01:29:27]

https://codereview.chromium.org/1588793002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium_tests/steps.py (right):

https://codereview.chromium.org/1588793002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_tests/steps.py:1177: args =
get_args_for_test(api, chromium_tests_api, spec, bot_update_step)
On 2016/01/20 15:10:38, Paweł Hajdan Jr. wrote:
> If the support is intended for isolated script test, can we please limit the
> variable substitution just to that?
> 
> It'd indeed address my concern about two mechanisms for the same thing. We
> indeed do not currently have a way to pass these properties to isolated script
> test.

Done and tests updated.

[Ken Russell (switch to Gerrit), 2016-01-21 01:30:23]

On 2016/01/20 15:10:38, Paweł Hajdan Jr. wrote:
> On 2016/01/20 at 01:59:50, kbr wrote:
> > 2. The mechanism by which the GPU pixel tests' golden images are produced
and
> tested against can certainly be redesigned to be more compatible with
Swarming's
> design goals. This would entail changing the pixel tests to no longer read or
> write cloud storage directly, but instead use some other mechanism. It might
> allow the distinction between "precommit" and "non-precommit" jobs to be
> removed. I would be happy to work with the infra team in the future to make
this
> change, but this work is *out of scope* with respect to the goal of retiring
the
> GPU recipe. During this transition, the existing tests need to continue to run
> in the same way they currently do.
> 
> Thanks for the context. I agree. Could you just file a bug to track this work?

Done in http://crbug.com/579793 .

[Ken Russell (switch to Gerrit), 2016-01-21 01:30:45]

The CQ bit was checked by kbr@chromium.org

[Ken Russell (switch to Gerrit), 2016-01-21 01:30:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergiyb@chromium.org, iannucci@chromium.org, phajdan.jr@chromium.org
Link to the patchset: https://codereview.chromium.org/1588793002/#ps40001
(title: "Addressed review feedback. Revised tests. Rebased.")

[commit-bot: I haz the power, 2016-01-21 01:30:55]

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

[commit-bot: I haz the power, 2016-01-21 01:33:57]

Description was changed from

==========
Support variable substitution and tryserver- and waterfall-only cmd line args.

This adds support to the src-side JSON files for substituting the
following variables in the command-line arguments to tests:

  ${buildername}
  ${got_revision}

and also supports setting command line arguments only on the tryservers,
and only on the waterfall bots.

BUG=573321
==========

to

==========
Support variable substitution and tryserver- and waterfall-only cmd line args.

This adds support to the src-side JSON files for substituting the
following variables in the command-line arguments to tests:

  ${buildername}
  ${got_revision}

and also supports setting command line arguments only on the tryservers,
and only on the waterfall bots.

BUG=573321

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

[commit-bot: I haz the power, 2016-01-21 01:33:58]

Committed patchset #3 (id:40001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=298331