Add CIPD pin reporting to swarming.

Add CIPD pin reporting to swarming.

This will cause run_isolated.py to report the fully resolved versions of the CIPD packages that it actually ended up using. DM will use this information to ensure that all executions of a given Attempt use the same versions.

R=maruel@chromium.org, nodir@chromium.org, vadimsh@chromium.org
BUG=639975
Committed: https://github.com/luci/luci-py/commit/1200be8efa2bc9543d6404887b6746f58e0de827

[iannucci, 2016-08-23 21:35:08]

PTAL; no tests yet, but I want to make sure this is on the right track before I
get too deep.

[iannucci, 2016-08-23 21:36:34]

Description was changed from

==========
Add CIPD pin reporting to swarming.

R=maruel@chromium.org, nodir@chromium.org, vadimsh@chromium.org
BUG=639975
==========

to

==========
Add CIPD pin reporting to swarming.

This will cause run_isolated.py to report the fully resolved versions of the
CIPD packages that it actually ended up using. DM will use this information to
ensure that all executions of a given Attempt use the same versions.

R=maruel@chromium.org, nodir@chromium.org, vadimsh@chromium.org
BUG=639975
==========

[M-A Ruel, 2016-08-24 02:13:44]

The code looks good. Still I can't help but think that if we forced clients
triggering tasks to evaluate the package at trigger time, we'd save a lot of
trouble.

I'm saying that because this issue already exists for automatic retry on
internal_failure.

Keep in mind that swarming tasks with unpinned cipd packages are also not marked
as idempotent, which is sad.

So my point is that by not supporting labels, we'd save a fair amount of code. I
forget the discussion, maybe there was a case where it's impossible for the
client doing the trigger to do the cipd label evaluation.

https://codereview.chromium.org/2267363004/diff/1/appengine/swarming/handlers...
File appengine/swarming/handlers_bot.py (right):

https://codereview.chromium.org/2267363004/diff/1/appengine/swarming/handlers...
appengine/swarming/handlers_bot.py:578: u'output_chunk_start', u'outputs_ref',
u'cipd_pins', u'task_id',
keep ordered lists ordered

https://codereview.chromium.org/2267363004/diff/1/appengine/swarming/server/t...
File appengine/swarming/server/task_result.py (right):

https://codereview.chromium.org/2267363004/diff/1/appengine/swarming/server/t...
appengine/swarming/server/task_result.py:798: self.cipd_pins = None
the list was sorted

https://codereview.chromium.org/2267363004/diff/1/client/cipd.py
File client/cipd.py (right):

https://codereview.chromium.org/2267363004/diff/1/client/cipd.py#newcode7
client/cipd.py:7: __version__ = '0.2'
bump

https://codereview.chromium.org/2267363004/diff/1/client/cipd.py#newcode153
client/cipd.py:153: os.write(list_file_handle, '%s %s\n' % (pkg, version))
it's weird that input is txt but output is json

[iannucci, 2016-08-24 02:23:31]

On 2016/08/24 at 02:13:44, maruel wrote:
> The code looks good. Still I can't help but think that if we forced clients
triggering tasks to evaluate the package at trigger time, we'd save a lot of
trouble.
> 
> I'm saying that because this issue already exists for automatic retry on
internal_failure.
> 
> Keep in mind that swarming tasks with unpinned cipd packages are also not
marked as idempotent, which is sad.
> 
> So my point is that by not supporting labels, we'd save a fair amount of code.
I forget the discussion, maybe there was a case where it's impossible for the
client doing the trigger to do the cipd label evaluation.

Yes, but that pushes the burden out to many different clients (CLI, web UI,
etc.). For example, by allowing these pinned-to-ref type task definitions (in
both DM/swarming), it means that 'revert_cl' can really be a simple call to
DM/swarming. If we require every client to resolve everything, it means that
those clients need to know a lot more about the tasks their running than just
e.g. the CL URL (which is what most callers actually care about).

It also makes the ACL story more difficult (at least for DM); I'm comfortable
adding functionality so that "random_contributor@somewher.domain" is able to
trigger "cq(cl)", which then evaluates and runs the latest version of the 'cq'
task. If I trust random_contributor to do all the resolution up-front, it would
hamper the security story (since they could resolve to very old/vulnerable
versions).

OR, it would force us to do pin-updates in all client repos basically any time
we change something. So 'rolling out a new version' couldn't be done with random
sampling among tasks or some other statistical distribution on the infra side,
it would require committing changes to all client repos (and some client repos
get disproportionately huge amounts of traffic, like chromium, which would be a
very big lump to swallow).

I think, for better or worse, package refs are here to stay.


> 
>
https://codereview.chromium.org/2267363004/diff/1/appengine/swarming/handlers...
> File appengine/swarming/handlers_bot.py (right):
> 
>
https://codereview.chromium.org/2267363004/diff/1/appengine/swarming/handlers...
> appengine/swarming/handlers_bot.py:578: u'output_chunk_start', u'outputs_ref',
u'cipd_pins', u'task_id',
> keep ordered lists ordered
> 
>
https://codereview.chromium.org/2267363004/diff/1/appengine/swarming/server/t...
> File appengine/swarming/server/task_result.py (right):
> 
>
https://codereview.chromium.org/2267363004/diff/1/appengine/swarming/server/t...
> appengine/swarming/server/task_result.py:798: self.cipd_pins = None
> the list was sorted
> 
> https://codereview.chromium.org/2267363004/diff/1/client/cipd.py
> File client/cipd.py (right):
> 
> https://codereview.chromium.org/2267363004/diff/1/client/cipd.py#newcode7
> client/cipd.py:7: __version__ = '0.2'
> bump
> 
> https://codereview.chromium.org/2267363004/diff/1/client/cipd.py#newcode153
> client/cipd.py:153: os.write(list_file_handle, '%s %s\n' % (pkg, version))
> it's weird that input is txt but output is json

[iannucci, 2016-08-24 02:31:57]

https://codereview.chromium.org/2267363004/diff/1/appengine/swarming/handlers...
File appengine/swarming/handlers_bot.py (right):

https://codereview.chromium.org/2267363004/diff/1/appengine/swarming/handlers...
appengine/swarming/handlers_bot.py:578: u'output_chunk_start', u'outputs_ref',
u'cipd_pins', u'task_id',
On 2016/08/24 at 02:13:44, M-A Ruel wrote:
> keep ordered lists ordered

Done.

https://codereview.chromium.org/2267363004/diff/1/appengine/swarming/server/t...
File appengine/swarming/server/task_result.py (right):

https://codereview.chromium.org/2267363004/diff/1/appengine/swarming/server/t...
appengine/swarming/server/task_result.py:798: self.cipd_pins = None
On 2016/08/24 at 02:13:44, M-A Ruel wrote:
> the list was sorted

done

https://codereview.chromium.org/2267363004/diff/1/client/cipd.py
File client/cipd.py (right):

https://codereview.chromium.org/2267363004/diff/1/client/cipd.py#newcode7
client/cipd.py:7: __version__ = '0.2'
On 2016/08/24 at 02:13:44, M-A Ruel wrote:
> bump

done

https://codereview.chromium.org/2267363004/diff/1/client/cipd.py#newcode153
client/cipd.py:153: os.write(list_file_handle, '%s %s\n' % (pkg, version))
On 2016/08/24 at 02:13:44, M-A Ruel wrote:
> it's weird that input is txt but output is json

I think the text file is supposed to be easier or something. It also allows
comments. There should probably be a json input format too.

[M-A Ruel, 2016-08-24 02:40:23]

On 2016/08/24 02:23:31, iannucci wrote:
> I think, for better or worse, package refs are here to stay.

Yeah that was kind of the resolution we had came to but had forgot (or I'm just
to tired now and need to go do sleep)

Please be sure to test live on the staging server before committing.

Will finish review tomorrow morning but I didn't see anything wrong so it should
be fine.

https://codereview.chromium.org/2267363004/diff/20001/appengine/swarming/mess...
File appengine/swarming/message_conversion.py (right):

https://codereview.chromium.org/2267363004/diff/20001/appengine/swarming/mess...
appengine/swarming/message_conversion.py:205: for pkg in (entity.cipd_pins or
())
I've always used "or []" and never realized that () worked as an iterator too.
Eh.

https://codereview.chromium.org/2267363004/diff/20001/client/run_isolated.py
File client/run_isolated.py (right):

https://codereview.chromium.org/2267363004/diff/20001/client/run_isolated.py#...
client/run_isolated.py:30: __version__ = '0.8.4'
bump too

[iannucci, 2016-08-24 16:01:11]

https://codereview.chromium.org/2267363004/diff/20001/client/run_isolated.py
File client/run_isolated.py (right):

https://codereview.chromium.org/2267363004/diff/20001/client/run_isolated.py#...
client/run_isolated.py:30: __version__ = '0.8.4'
On 2016/08/24 at 02:40:23, M-A Ruel wrote:
> bump too

done

Can we make a PRESUBMIT hook for these?

[M-A Ruel, 2016-08-24 19:34:08]

https://codereview.chromium.org/2267363004/diff/20001/appengine/swarming/serv...
File appengine/swarming/server/task_request.py (right):

https://codereview.chromium.org/2267363004/diff/20001/appengine/swarming/serv...
appengine/swarming/server/task_request.py:344: that was resolved by the bot at
runtime, and contains a full package name as
This is really part of the result, not the request, so any related function
should be in task_results.py, otherwise it's a layering violation

https://codereview.chromium.org/2267363004/diff/20001/appengine/swarming/temp...
File appengine/swarming/templates/user_task.html (right):

https://codereview.chromium.org/2267363004/diff/20001/appengine/swarming/temp...
appengine/swarming/templates/user_task.html:246: {% for path, pininfos in
request.properties.cipd_input.packages_grouped_by_path(result.cipd_pins) %}
One concern I have here is that it presents outputs in the inputs section.
Normally I'd just print out the output in a separate list in the execution
section below. It's more verbose but it's more strictly separated.

https://codereview.chromium.org/2267363004/diff/20001/client/run_isolated.py
File client/run_isolated.py (right):

https://codereview.chromium.org/2267363004/diff/20001/client/run_isolated.py#...
client/run_isolated.py:30: __version__ = '0.8.4'
On 2016/08/24 16:01:11, iannucci wrote:
> On 2016/08/24 at 02:40:23, M-A Ruel wrote:
> > bump too
> 
> done

Please upload again.

Also please test it on staging so I can look at the results.

> Can we make a PRESUBMIT hook for these?

It's tricky, some changes may not be in the script itself yet still warrant a
bump and the reverse is more often true, small changes do not warrant a bump.

[iannucci, 2016-08-25 00:14:12]

I'm reworking this to be more symmetric with the cipd_inputs (e.g. also include
the pinned version of the actual cipd client too)

https://codereview.chromium.org/2267363004/diff/20001/appengine/swarming/serv...
File appengine/swarming/server/task_request.py (right):

https://codereview.chromium.org/2267363004/diff/20001/appengine/swarming/serv...
appengine/swarming/server/task_request.py:344: that was resolved by the bot at
runtime, and contains a full package name as
On 2016/08/24 at 19:34:07, M-A Ruel wrote:
> This is really part of the result, not the request, so any related function
should be in task_results.py, otherwise it's a layering violation

https://codereview.chromium.org/2267363004/diff/20001/appengine/swarming/temp...
File appengine/swarming/templates/user_task.html (right):

https://codereview.chromium.org/2267363004/diff/20001/appengine/swarming/temp...
appengine/swarming/templates/user_task.html:246: {% for path, pininfos in
request.properties.cipd_input.packages_grouped_by_path(result.cipd_pins) %}
On 2016/08/24 at 19:34:07, M-A Ruel wrote:
> One concern I have here is that it presents outputs in the inputs section.
Normally I'd just print out the output in a separate list in the execution
section below. It's more verbose but it's more strictly separated.

Ah, ok. It's only kind-of an output though (I consider that the code running the
job is an input of the job, not an output). But I can make a pinned section. I
suppose it's more of an output because it only shows up when the task finishes.

[M-A Ruel, 2016-08-25 15:40:26]

https://codereview.chromium.org/2267363004/diff/20001/appengine/swarming/temp...
File appengine/swarming/templates/user_task.html (right):

https://codereview.chromium.org/2267363004/diff/20001/appengine/swarming/temp...
appengine/swarming/templates/user_task.html:246: {% for path, pininfos in
request.properties.cipd_input.packages_grouped_by_path(result.cipd_pins) %}
On 2016/08/25 00:14:12, iannucci wrote:
> On 2016/08/24 at 19:34:07, M-A Ruel wrote:
> > One concern I have here is that it presents outputs in the inputs section.
> Normally I'd just print out the output in a separate list in the execution
> section below. It's more verbose but it's more strictly separated.
> 
> Ah, ok. It's only kind-of an output though (I consider that the code running
the
> job is an input of the job, not an output). But I can make a pinned section. I
> suppose it's more of an output because it only shows up when the task
finishes.

It is an input for the code being run, but from a pure functional point of view,
it's a result of running the test. The page strictly separates what is used for
task deduplication from metadata (e.g. who, when) from whatever is results.

[iannucci, 2016-08-25 22:49:38]

Finally got back to this, PTAL.

This makes cipd_output uniform with cipd_input (includes client_package), and
cipd_output now only modifies the Execution portion of the html.

Let me know where you'd like to see additional tests (if any). I'm going to try
this on staging shortly.

[iannucci, 2016-08-26 00:11:04]

On 2016/08/25 at 22:49:38, iannucci wrote:
> Finally got back to this, PTAL.
> 
> This makes cipd_output uniform with cipd_input (includes client_package), and
cipd_output now only modifies the Execution portion of the html.
> 
> Let me know where you'd like to see additional tests (if any). I'm going to
try this on staging shortly.

Here's it working in staging:
https://chromium-swarm-dev.appspot.com/user/task/30dcff6549539910

[iannucci, 2016-08-26 19:02:07]

Any other comments on this one?

[iannucci, 2016-08-26 21:13:25]

Renamed to CipdPins/cipd_pins per offline conversation.

[M-A Ruel, 2016-08-26 22:42:03]

Description was changed from

==========
Add CIPD pin reporting to swarming.

This will cause run_isolated.py to report the fully resolved versions of the
CIPD packages that it actually ended up using. DM will use this information to
ensure that all executions of a given Attempt use the same versions.

R=maruel@chromium.org, nodir@chromium.org, vadimsh@chromium.org
BUG=639975
==========

to

==========
Add CIPD pin reporting to swarming.

This will cause run_isolated.py to report the fully resolved versions of the
CIPD packages that it actually ended up using. DM will use this information to
ensure that all executions of a given Attempt use the same versions.

R=maruel@chromium.org, nodir@chromium.org, vadimsh@chromium.org
BUG=639975
==========

[M-A Ruel, 2016-08-26 23:19:51]

https://codereview.chromium.org/2267363004/diff/80001/appengine/swarming/hand...
File appengine/swarming/handlers_bot.py (right):

https://codereview.chromium.org/2267363004/diff/80001/appengine/swarming/hand...
appengine/swarming/handlers_bot.py:727: **cipd_pins['client_package']),
arguments aligned at +4 (everywhere)

https://codereview.chromium.org/2267363004/diff/80001/appengine/swarming/hand...
File appengine/swarming/handlers_frontend.py (right):

https://codereview.chromium.org/2267363004/diff/80001/appengine/swarming/hand...
appengine/swarming/handlers_frontend.py:642: pins = outputs.packages
this code assumes the order is the same but this was not yet enforced. Also I
don't find this particularly useful, I'd prefer just to have
packages_grouped_by_path() here and call it once for inputs, once for outputs.

https://codereview.chromium.org/2267363004/diff/80001/appengine/swarming/hand...
appengine/swarming/handlers_frontend.py:644: assert pins is None or len(pins) ==
len(packages)
This assert should be done at the time the entity is saved as commented in
task_result.py.

https://codereview.chromium.org/2267363004/diff/80001/appengine/swarming/hand...
appengine/swarming/handlers_frontend.py:652: pkgs.sort()
see comment in task_request.py

https://codereview.chromium.org/2267363004/diff/80001/appengine/swarming/hand...
appengine/swarming/handlers_frontend.py:705: cipd_pinned = result.cipd_pins is
not None
do one for input, one for pinned without mixing both. That's what the polymer
code will have to do anyway, this code will be deleted soon.

https://codereview.chromium.org/2267363004/diff/80001/appengine/swarming/hand...
appengine/swarming/handlers_frontend.py:733: 
remove

https://codereview.chromium.org/2267363004/diff/80001/appengine/swarming/mess...
File appengine/swarming/message_conversion.py (right):

https://codereview.chromium.org/2267363004/diff/80001/appengine/swarming/mess...
appengine/swarming/message_conversion.py:213: for pkg in
entity.cipd_pins.packages
for pkg in entity.cipd_pins.packages or ()

would remove the need of the condition

https://codereview.chromium.org/2267363004/diff/80001/appengine/swarming/serv...
File appengine/swarming/server/task_request.py (left):

https://codereview.chromium.org/2267363004/diff/80001/appengine/swarming/serv...
appengine/swarming/server/task_request.py:342: pkgs.sort()
I'm not sure this ever worked correctly. But it didn't matter because sorting
happens at ndb.Model.put() time. So sorting here on object retrieved from the DB
is effectively useless.

https://codereview.chromium.org/2267363004/diff/80001/appengine/swarming/serv...
File appengine/swarming/server/task_result.py (right):

https://codereview.chromium.org/2267363004/diff/80001/appengine/swarming/serv...
appengine/swarming/server/task_result.py:313: A part of TaskRunResult.
This is misleading, since this is in TaskResultSummary too.

https://codereview.chromium.org/2267363004/diff/80001/appengine/swarming/serv...
appengine/swarming/server/task_result.py:321: packages =
ndb.LocalStructuredProperty(task_request.CipdPackage,
I'd like the entities to be sorted the same way they are in task_request.py
CipdInput._pre_put_hook()


Also I'd like a _pre_put_hook here to ensure that the items are exactly the same
as in TaskProperties.cipd_input. There shouldn't be asserts in viewing code,
better to assert at DB write time.

https://codereview.chromium.org/2267363004/diff/80001/appengine/swarming/serv...
appengine/swarming/server/task_result.py:625: 
It should explicitly:
if self.cipd_pins:
  self.cipd_pins._pre_put_hook()

https://codereview.chromium.org/2267363004/diff/80001/client/cipd.py
File client/cipd.py (right):

https://codereview.chromium.org/2267363004/diff/80001/client/cipd.py#newcode434
client/cipd.py:434: result = collections.defaultdict(list)
Can it be a flat list instead of a dict, then ordering is kept, then 'i' is not
needed.

https://codereview.chromium.org/2267363004/diff/80001/client/cipd.py#newcode442
client/cipd.py:442: if with_index:
can we not make this a condition?

[iannucci, 2016-08-29 22:08:41]

Ok PTA(nother)L

https://codereview.chromium.org/2267363004/diff/80001/appengine/swarming/hand...
File appengine/swarming/handlers_bot.py (right):

https://codereview.chromium.org/2267363004/diff/80001/appengine/swarming/hand...
appengine/swarming/handlers_bot.py:727: **cipd_pins['client_package']),
On 2016/08/26 at 23:19:51, M-A Ruel wrote:
> arguments aligned at +4 (everywhere)

Done

https://codereview.chromium.org/2267363004/diff/80001/appengine/swarming/hand...
File appengine/swarming/handlers_frontend.py (right):

https://codereview.chromium.org/2267363004/diff/80001/appengine/swarming/hand...
appengine/swarming/handlers_frontend.py:642: pins = outputs.packages
On 2016/08/26 at 23:19:51, M-A Ruel wrote:
> this code assumes the order is the same but this was not yet enforced. Also I
don't find this particularly useful, I'd prefer just to have
packages_grouped_by_path() here and call it once for inputs, once for outputs.

sgtm, I considered this, but thought you might prefer it this way.

https://codereview.chromium.org/2267363004/diff/80001/appengine/swarming/hand...
appengine/swarming/handlers_frontend.py:644: assert pins is None or len(pins) ==
len(packages)
On 2016/08/26 at 23:19:51, M-A Ruel wrote:
> This assert should be done at the time the entity is saved as commented in
task_result.py.

yep, done

https://codereview.chromium.org/2267363004/diff/80001/appengine/swarming/serv...
File appengine/swarming/server/task_request.py (left):

https://codereview.chromium.org/2267363004/diff/80001/appengine/swarming/serv...
appengine/swarming/server/task_request.py:342: pkgs.sort()
On 2016/08/26 at 23:19:51, M-A Ruel wrote:
> I'm not sure this ever worked correctly. But it didn't matter because sorting
happens at ndb.Model.put() time. So sorting here on object retrieved from the DB
is effectively useless.

Ah, ok. So if they're sorted in the DB, then there's no need to sort them at all
here and I can just delete this loop.

https://codereview.chromium.org/2267363004/diff/80001/appengine/swarming/serv...
File appengine/swarming/server/task_result.py (right):

https://codereview.chromium.org/2267363004/diff/80001/appengine/swarming/serv...
appengine/swarming/server/task_result.py:313: A part of TaskRunResult.
On 2016/08/26 at 23:19:51, M-A Ruel wrote:
> This is misleading, since this is in TaskResultSummary too.

erp, sorry.

https://codereview.chromium.org/2267363004/diff/80001/appengine/swarming/serv...
appengine/swarming/server/task_result.py:321: packages =
ndb.LocalStructuredProperty(task_request.CipdPackage,
On 2016/08/26 at 23:19:51, M-A Ruel wrote:
> I'd like the entities to be sorted the same way they are in task_request.py
CipdInput._pre_put_hook()
> 
> 
> Also I'd like a _pre_put_hook here to ensure that the items are exactly the
same as in TaskProperties.cipd_input. There shouldn't be asserts in viewing
code, better to assert at DB write time.

yep, absolutely true, don't know what I was thinking...

That said, I don't think this can be _pre_put_hook because we don't have a
reference to the TaskProperties there?

https://codereview.chromium.org/2267363004/diff/80001/client/cipd.py
File client/cipd.py (right):

https://codereview.chromium.org/2267363004/diff/80001/client/cipd.py#newcode434
client/cipd.py:434: result = collections.defaultdict(list)
On 2016/08/26 at 23:19:51, M-A Ruel wrote:
> Can it be a flat list instead of a dict, then ordering is kept, then 'i' is
not needed.

yeah, that felt more invasive than I wanted at the time, but I think I'm
comfortable doing that now.

https://codereview.chromium.org/2267363004/diff/80001/client/cipd.py#newcode442
client/cipd.py:442: if with_index:
On 2016/08/26 at 23:19:51, M-A Ruel wrote:
> can we not make this a condition?

will make it a list, no condition needed.

[M-A Ruel, 2016-08-30 02:18:08]

https://codereview.chromium.org/2267363004/diff/80001/appengine/swarming/serv...
File appengine/swarming/server/task_result.py (right):

https://codereview.chromium.org/2267363004/diff/80001/appengine/swarming/serv...
appengine/swarming/server/task_result.py:321: packages =
ndb.LocalStructuredProperty(task_request.CipdPackage,
On 2016/08/29 22:08:40, iannucci wrote:
> That said, I don't think this can be _pre_put_hook because we don't have a
> reference to the TaskProperties there?

Yes, so the check will have to be done in a validate() or something.

https://codereview.chromium.org/2267363004/diff/100001/appengine/swarming/cip...
File appengine/swarming/cipd.py (right):

https://codereview.chromium.org/2267363004/diff/100001/appengine/swarming/cip...
appengine/swarming/cipd.py:44: def check(self, original, expanded):
optional nit: I think I'd prefer if this function returned a new instance of
PinChecker instead of mutating.

https://codereview.chromium.org/2267363004/diff/100001/appengine/swarming/cip...
appengine/swarming/cipd.py:45: """Raises ValueError if expanded is not pluasibly
a pinned version of
plausibly

https://codereview.chromium.org/2267363004/diff/100001/appengine/swarming/cip...
appengine/swarming/cipd.py:85: name_regex, self.PARAM_PLATFORM_ESC,
'(?P<platform>\w+-[a-z0-9]+)',
Use r'' otherwise your \ may not do what you think it'll do.

https://codereview.chromium.org/2267363004/diff/100001/appengine/swarming/cip...
appengine/swarming/cipd.py:88: name_regex, self.PARAM_OS_VER_ESC,
'(?P<os_ver>[_a-z0-9]+)',
r'' as per habit.

https://codereview.chromium.org/2267363004/diff/100001/appengine/swarming/cip...
File appengine/swarming/cipd_test.py (right):

https://codereview.chromium.org/2267363004/diff/100001/appengine/swarming/cip...
appengine/swarming/cipd_test.py:24: self.cp =
collections.namedtuple('CipdPackage', 'path package_name version')
I didn't know you like javascript. :P

https://codereview.chromium.org/2267363004/diff/100001/appengine/swarming/han...
File appengine/swarming/handlers_frontend.py (right):

https://codereview.chromium.org/2267363004/diff/100001/appengine/swarming/han...
appengine/swarming/handlers_frontend.py:711: 
remove

https://codereview.chromium.org/2267363004/diff/100001/appengine/swarming/ser...
File appengine/swarming/server/task_result.py (right):

https://codereview.chromium.org/2267363004/diff/100001/appengine/swarming/ser...
appengine/swarming/server/task_result.py:600: check = cipd.PinChecker().check
having check() a staticmethod makes this more sensible.

[iannucci, 2016-08-30 09:13:29]

PTAL

https://codereview.chromium.org/2267363004/diff/100001/appengine/swarming/cip...
File appengine/swarming/cipd.py (right):

https://codereview.chromium.org/2267363004/diff/100001/appengine/swarming/cip...
appengine/swarming/cipd.py:44: def check(self, original, expanded):
On 2016/08/30 at 02:18:07, M-A Ruel wrote:
> optional nit: I think I'd prefer if this function returned a new instance of
PinChecker instead of mutating.

Done.

edit: Actually I changed it to a contextmanager. I tried the 'return an new
instance' thing, but it was super-awkward. I think the new version represents
the semantics correctly; doing checks within some lexical context where all
template parameters must be consistent.

https://codereview.chromium.org/2267363004/diff/100001/appengine/swarming/cip...
appengine/swarming/cipd.py:45: """Raises ValueError if expanded is not pluasibly
a pinned version of
On 2016/08/30 at 02:18:07, M-A Ruel wrote:
> plausibly

Done.

https://codereview.chromium.org/2267363004/diff/100001/appengine/swarming/cip...
appengine/swarming/cipd.py:85: name_regex, self.PARAM_PLATFORM_ESC,
'(?P<platform>\w+-[a-z0-9]+)',
On 2016/08/30 at 02:18:07, M-A Ruel wrote:
> Use r'' otherwise your \ may not do what you think it'll do.

Done

https://codereview.chromium.org/2267363004/diff/100001/appengine/swarming/cip...
appengine/swarming/cipd.py:88: name_regex, self.PARAM_OS_VER_ESC,
'(?P<os_ver>[_a-z0-9]+)',
On 2016/08/30 at 02:18:07, M-A Ruel wrote:
> r'' as per habit.

Done

https://codereview.chromium.org/2267363004/diff/100001/appengine/swarming/cip...
File appengine/swarming/cipd_test.py (right):

https://codereview.chromium.org/2267363004/diff/100001/appengine/swarming/cip...
appengine/swarming/cipd_test.py:24: self.cp =
collections.namedtuple('CipdPackage', 'path package_name version')
On 2016/08/30 at 02:18:07, M-A Ruel wrote:
> I didn't know you like javascript. :P

:(

https://codereview.chromium.org/2267363004/diff/100001/appengine/swarming/han...
File appengine/swarming/handlers_frontend.py (right):

https://codereview.chromium.org/2267363004/diff/100001/appengine/swarming/han...
appengine/swarming/handlers_frontend.py:711: 
On 2016/08/30 at 02:18:07, M-A Ruel wrote:
> remove

Done

https://codereview.chromium.org/2267363004/diff/100001/appengine/swarming/ser...
File appengine/swarming/server/task_result.py (right):

https://codereview.chromium.org/2267363004/diff/100001/appengine/swarming/ser...
appengine/swarming/server/task_result.py:600: check = cipd.PinChecker().check
On 2016/08/30 at 02:18:07, M-A Ruel wrote:
> having check() a staticmethod makes this more sensible.

done

[M-A Ruel, 2016-08-30 18:54:13]

https://codereview.chromium.org/2267363004/diff/120001/appengine/swarming/cip...
File appengine/swarming/cipd.py (right):

https://codereview.chromium.org/2267363004/diff/120001/appengine/swarming/cip...
appengine/swarming/cipd.py:40: def pin_check_fn(platform=None, os_ver=None):
I'd prefer no default value.

https://codereview.chromium.org/2267363004/diff/120001/appengine/swarming/cip...
appengine/swarming/cipd.py:84: ret = param_esc in regex
Then what about:
regex.count(param_esc)

and check if count is higher than 1, instead of the limit in replace() call.

https://codereview.chromium.org/2267363004/diff/120001/appengine/swarming/han...
File appengine/swarming/handlers_frontend.py (right):

https://codereview.chromium.org/2267363004/diff/120001/appengine/swarming/han...
appengine/swarming/handlers_frontend.py:683:
request.properties.cipd_input.packages)
keep trailing comma

https://codereview.chromium.org/2267363004/diff/120001/client/run_isolated.py
File client/run_isolated.py (right):

https://codereview.chromium.org/2267363004/diff/120001/client/run_isolated.py...
client/run_isolated.py:648: 'version': version,
sort

https://codereview.chromium.org/2267363004/diff/120001/client/run_isolated.py...
client/run_isolated.py:696: 'packages': package_pins,
sort

[iannucci, 2016-08-30 19:09:17]

https://chromiumcodereview.appspot.com/2267363004/diff/120001/appengine/swarm...
File appengine/swarming/handlers_frontend.py (right):

https://chromiumcodereview.appspot.com/2267363004/diff/120001/appengine/swarm...
appengine/swarming/handlers_frontend.py:683:
request.properties.cipd_input.packages)
On 2016/08/30 at 18:54:12, M-A Ruel wrote:
> keep trailing comma

Done

https://chromiumcodereview.appspot.com/2267363004/diff/120001/client/run_isol...
File client/run_isolated.py (right):

https://chromiumcodereview.appspot.com/2267363004/diff/120001/client/run_isol...
client/run_isolated.py:648: 'version': version,
On 2016/08/30 at 18:54:12, M-A Ruel wrote:
> sort

done

https://chromiumcodereview.appspot.com/2267363004/diff/120001/client/run_isol...
client/run_isolated.py:696: 'packages': package_pins,
On 2016/08/30 at 18:54:12, M-A Ruel wrote:
> sort

done

[iannucci, 2016-08-30 19:28:22]

Added assertion re: number of patterns in regex in the pin check as well as at
task creation time.

[M-A Ruel, 2016-08-30 19:29:51]

lgtm!

[iannucci, 2016-08-30 19:36:55]

The CQ bit was checked by iannucci@chromium.org

[commit-bot: I haz the power, 2016-08-30 19:37:08]

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, 2016-08-30 20:10:55]

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

[commit-bot: I haz the power, 2016-08-30 20:10:56]

Try jobs failed on following builders:
  Luci-py Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/30f5c8f8d9afd310)

[iannucci, 2016-08-30 22:28:10]

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

[commit-bot: I haz the power, 2016-08-30 22:28:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[iannucci, 2016-08-30 22:37:37]

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

[commit-bot: I haz the power, 2016-08-30 22:37:45]

Dry run: 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, 2016-08-30 22:41:35]

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

[commit-bot: I haz the power, 2016-08-30 22:41:37]

Dry run: This issue passed the CQ dry run.

[iannucci, 2016-08-30 22:48:21]

The CQ bit was checked by iannucci@chromium.org

[iannucci, 2016-08-30 22:48:21]

The patchset sent to the CQ was uploaded after l-g-t-m from maruel@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/2267363004/#ps200001 (title: "Fix
bottest")

[commit-bot: I haz the power, 2016-08-30 22:48:28]

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, 2016-08-30 22:52:24]

Description was changed from

==========
Add CIPD pin reporting to swarming.

This will cause run_isolated.py to report the fully resolved versions of the
CIPD packages that it actually ended up using. DM will use this information to
ensure that all executions of a given Attempt use the same versions.

R=maruel@chromium.org, nodir@chromium.org, vadimsh@chromium.org
BUG=639975
==========

to

==========
Add CIPD pin reporting to swarming.

This will cause run_isolated.py to report the fully resolved versions of the
CIPD packages that it actually ended up using. DM will use this information to
ensure that all executions of a given Attempt use the same versions.

R=maruel@chromium.org, nodir@chromium.org, vadimsh@chromium.org
BUG=639975

Committed:
https://github.com/luci/luci-py/commit/1200be8efa2bc9543d6404887b6746f58e0de827
==========

[commit-bot: I haz the power, 2016-08-30 22:52:25]

Committed patchset #11 (id:200001) as
https://github.com/luci/luci-py/commit/1200be8efa2bc9543d6404887b6746f58e0de827

[nodir, 2016-10-20 19:15:26]

was this deployed? fwiu I should see cipd pins in "Execution" section of any
swarmbucket task, such as
https://chromium-swarm.appspot.com/user/task/30608d0612512710

[Vadim Sh., 2016-10-20 19:20:58]

On 2016/10/20 19:15:26, nodir wrote:
> was this deployed? fwiu I should see cipd pins in "Execution" section of any
> swarmbucket task, such as
> https://chromium-swarm.appspot.com/user/task/30608d0612512710

Odd... Here it works:
https://chromium-swarm.appspot.com/user/task/31fc4a19f73d4310

Maybe it handles failed tasks differently?

[Vadim Sh., 2016-10-20 19:21:55]

On 2016/10/20 19:20:58, Vadim Sh. wrote:
> On 2016/10/20 19:15:26, nodir wrote:
> > was this deployed? fwiu I should see cipd pins in "Execution" section of any
> > swarmbucket task, such as
> > https://chromium-swarm.appspot.com/user/task/30608d0612512710
> 
> Odd... Here it works:
> https://chromium-swarm.appspot.com/user/task/31fc4a19f73d4310
> 
> Maybe it handles failed tasks differently?

Ah, your task is from august :) It wasn't deployed then yet, probably.

[iannucci, 2016-10-20 19:22:35]

Right now the pins are only reported when the task completes. It would
probably make sense to report them as soon as they're known though (I
wasn't sure how to do that, but I didn't look into it super-closely).

On Thu, Oct 20, 2016 at 12:20 PM <vadimsh@chromium.org> wrote:

> On 2016/10/20 19:15:26, nodir wrote:
> > was this deployed? fwiu I should see cipd pins in "Execution" section of
> any
> > swarmbucket task, such as
> > https://chromium-swarm.appspot.com/user/task/30608d0612512710
>
> Odd... Here it works:
> https://chromium-swarm.appspot.com/user/task/31fc4a19f73d4310
>
> Maybe it handles failed tasks differently?
>
> https://codereview.chromium.org/2267363004/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kjlubick, 2016-10-20 19:24:36]

On 2016/10/20 at 19:22:35, iannucci wrote:
> Right now the pins are only reported when the task completes. It would
> probably make sense to report them as soon as they're known though (I
> wasn't sure how to do that, but I didn't look into it super-closely).
> 
> On Thu, Oct 20, 2016 at 12:20 PM <vadimsh@chromium.org> wrote:
> 
> > On 2016/10/20 19:15:26, nodir wrote:
> > > was this deployed? fwiu I should see cipd pins in "Execution" section of
> > any
> > > swarmbucket task, such as
> > > https://chromium-swarm.appspot.com/user/task/30608d0612512710
> >
> > Odd... Here it works:
> > https://chromium-swarm.appspot.com/user/task/31fc4a19f73d4310
> >
> > Maybe it handles failed tasks differently?
> >
> > https://codereview.chromium.org/2267363004/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to chromium-reviews+unsubscribe@chromium.org.
> 

FWIW, It's in the new ui, just hidden under more details: 
https://screenshot.googleplex.com/ZVuLXmK5S7A

https://chromium-swarm.appspot.com/task?id=31fc4a19f73d4310&refresh=10&reques...