swarming: add support for cipd on the server side

swarming: add support for cipd on the server side

- add CipdPackage ndb and rpc types
- add "packages" field to TaskProperties in ndb and rpc
- validate packages before putting task properties
- if idempotent=true, ensure that package versions are hashes
- include packages in bot manifest

R=maruel@chromium.org
BUG=601022
Committed: https://github.com/luci/luci-py/commit/1c6c3d329a42a889001c88214fc7082e0f60c1d9

[nodir, 2016-04-21 00:58:35]

Description was changed from

==========
swarming: add cipd packages on the server side

- add CipdPackage ndb and rpc types.
- add "packages" field to TaskProperties in ndb and rpc
- validate packages before putting task properties
- if idempotent=true, ensure that package versions are hashes
- include packages in bot manifest

R=maruel@chromium.org
BUG=601022
==========

to

==========
swarming: add cipd packages on the server side

- add CipdPackage ndb and rpc types
- add "packages" field to TaskProperties in ndb and rpc
- validate packages before putting task properties
- if idempotent=true, ensure that package versions are hashes
- include packages in bot manifest

R=maruel@chromium.org
BUG=601022
==========

[nodir, 2016-04-22 00:30:02]

Description was changed from

==========
swarming: add cipd packages on the server side

- add CipdPackage ndb and rpc types
- add "packages" field to TaskProperties in ndb and rpc
- validate packages before putting task properties
- if idempotent=true, ensure that package versions are hashes
- include packages in bot manifest

R=maruel@chromium.org
BUG=601022
==========

to

==========
swarming: add support for cipd on the server side

- add CipdPackage ndb and rpc types
- add "packages" field to TaskProperties in ndb and rpc
- validate packages before putting task properties
- if idempotent=true, ensure that package versions are hashes
- include packages in bot manifest

R=maruel@chromium.org
BUG=601022
==========

[nodir, 2016-04-25 18:39:49]

Patchset #2 (id:20001) has been deleted

[nodir, 2016-04-25 18:52:54]

PTAL

[nodir, 2016-04-25 18:53:01]

Patchset #1 (id:1) has been deleted

[M-A Ruel, 2016-04-25 19:09:45]

https://codereview.chromium.org/1910713002/diff/40001/appengine/swarming/cipd.py
File appengine/swarming/cipd.py (right):

https://codereview.chromium.org/1910713002/diff/40001/appengine/swarming/cipd...
appengine/swarming/cipd.py:1: #!/usr/bin/env python
THis file is not executable.

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

https://codereview.chromium.org/1910713002/diff/40001/appengine/swarming/serv...
appengine/swarming/server/task_request.py:239: if not
cipd.is_valid_package_name(self.package_name):
Could you use validator=wrapper_function instead?

You can then just keep lines 237-238 and 242-243, I prefer this to required=True
because required=True breaks loading. :/

https://codereview.chromium.org/1910713002/diff/40001/appengine/swarming/serv...
appengine/swarming/server/task_request.py:244: if not
cipd.is_valid_version(self.version):
Same

https://codereview.chromium.org/1910713002/diff/40001/appengine/swarming/serv...
appengine/swarming/server/task_request.py:362: for p in self.packages:
Can you also sort it? It's important that listing in arbitrary orders still
result in the same hash digest.

https://codereview.chromium.org/1910713002/diff/40001/appengine/swarming/serv...
File appengine/swarming/server/task_request_test.py (right):

https://codereview.chromium.org/1910713002/diff/40001/appengine/swarming/serv...
appengine/swarming/server/task_request_test.py:519: def mkreq(req):
Put it at the top.

https://codereview.chromium.org/1910713002/diff/40001/appengine/swarming/swar...
File appengine/swarming/swarming_rpcs.py (right):

https://codereview.chromium.org/1910713002/diff/40001/appengine/swarming/swar...
appengine/swarming/swarming_rpcs.py:88: package_name = messages.StringField(1,
required=True)
Don't use required=True here, we handle it at application code instead.

[nodir, 2016-04-25 20:27:15]

nodir@chromium.org changed reviewers:
+ vadimsh@chromium.org

[nodir, 2016-04-25 20:27:16]

+vadimsh

https://codereview.chromium.org/1910713002/diff/40001/appengine/swarming/cipd.py
File appengine/swarming/cipd.py (right):

https://codereview.chromium.org/1910713002/diff/40001/appengine/swarming/cipd...
appengine/swarming/cipd.py:1: #!/usr/bin/env python
On 2016/04/25 19:09:45, M-A Ruel wrote:
> THis file is not executable.

Done.

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

https://codereview.chromium.org/1910713002/diff/40001/appengine/swarming/serv...
appengine/swarming/server/task_request.py:239: if not
cipd.is_valid_package_name(self.package_name):
On 2016/04/25 19:09:45, M-A Ruel wrote:
> Could you use validator=wrapper_function instead?
> 
> You can then just keep lines 237-238 and 242-243, I prefer this to
required=True
> because required=True breaks loading. :/

Done.

https://codereview.chromium.org/1910713002/diff/40001/appengine/swarming/serv...
appengine/swarming/server/task_request.py:244: if not
cipd.is_valid_version(self.version):
On 2016/04/25 19:09:45, M-A Ruel wrote:
> Same

Done.

https://codereview.chromium.org/1910713002/diff/40001/appengine/swarming/serv...
appengine/swarming/server/task_request.py:362: for p in self.packages:
On 2016/04/25 19:09:45, M-A Ruel wrote:
> Can you also sort it? It's important that listing in arbitrary orders still
> result in the same hash digest.

good point, done
also add a check that only one version of a package is specified

https://codereview.chromium.org/1910713002/diff/40001/appengine/swarming/serv...
File appengine/swarming/server/task_request_test.py (right):

https://codereview.chromium.org/1910713002/diff/40001/appengine/swarming/serv...
appengine/swarming/server/task_request_test.py:519: def mkreq(req):
On 2016/04/25 19:09:45, M-A Ruel wrote:
> Put it at the top.

Done.

https://codereview.chromium.org/1910713002/diff/40001/appengine/swarming/swar...
File appengine/swarming/swarming_rpcs.py (right):

https://codereview.chromium.org/1910713002/diff/40001/appengine/swarming/swar...
appengine/swarming/swarming_rpcs.py:88: package_name = messages.StringField(1,
required=True)
On 2016/04/25 19:09:45, M-A Ruel wrote:
> Don't use required=True here, we handle it at application code instead.

Done.

[Paweł Hajdan Jr., 2016-04-26 08:45:21]

phajdan.jr@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[Paweł Hajdan Jr., 2016-04-26 08:45:22]

Drive-by with some suggestions/questions. Nice work!

(you don't need for me to land this btw)

https://codereview.chromium.org/1910713002/diff/80001/appengine/swarming/cipd.py
File appengine/swarming/cipd.py (right):

https://codereview.chromium.org/1910713002/diff/80001/appengine/swarming/cipd...
appengine/swarming/cipd.py:10: #
https://chromium.googlesource.com/infra/infra/+/468bb43/appengine/chrome_infr...
How about a TODO to move chrome_infra_packages to luci-py and share the regexes?

Duplicating things is a smell, and they can get out of sync.

Another idea might be to rely on CIPD itself to do the validation. I'm not sure
if we need to duplicate that here.

Just my 2c, not blocking this CL and totally up to you. With the high quality
standards for LUCI though, I thought it'd fit. Does that make sense? :)

https://codereview.chromium.org/1910713002/diff/80001/appengine/swarming/swar...
File appengine/swarming/swarming_rpcs.py (right):

https://codereview.chromium.org/1910713002/diff/80001/appengine/swarming/swar...
appengine/swarming/swarming_rpcs.py:86: """A CIPD package to install in
$CIPD_PATH and $PATH before task execution."""
Quick check: if I'll want to use this to make kitchen use logdog, will I be able
to reference $CIPD_PATH in the swarming command to execute?

We'd request CIPD packages for kitchen and logdog binaries, and I can imagine
the command would look like this:

$CIPD_PATH/kitchen/kitchen ... -logdog-butler-path
$CIPD_PATH/logdog_butler/butler

Does that make sense? I'm also fine with some alternatives, just wanted to make
sure the logdog scenario would work.

This doesn't block the CL of course.

[M-A Ruel, 2016-04-27 17:43:56]

lgtm with two questions.

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

https://codereview.chromium.org/1910713002/diff/80001/appengine/swarming/serv...
appengine/swarming/server/task_request.py:377: # We don't support different
versions of the same package yet.
"yet"? Ever? I don't think this comment is necessary

https://codereview.chromium.org/1910713002/diff/80001/appengine/swarming/serv...
appengine/swarming/server/task_request.py:381: self.packages =
sorted(self.packages, key=lambda p: p.package_name)
self.packages.sort(key=lambda p: p.package_name)
?

[nodir, 2016-04-27 19:28:08]

Patchset #4 (id:100001) has been deleted

[nodir, 2016-04-27 19:28:34]

The CQ bit was checked by nodir@chromium.org

[nodir, 2016-04-27 19:28:34]

The patchset sent to the CQ was uploaded after l-g-t-m from maruel@chromium.org
Link to the patchset: https://codereview.chromium.org/1910713002/#ps120001
(title: "nits")

[commit-bot: I haz the power, 2016-04-27 19:28:42]

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

[nodir, 2016-04-27 19:29:29]

The CQ bit was unchecked by nodir@chromium.org

[nodir, 2016-04-27 19:30:30]

The CQ bit was checked by nodir@chromium.org

[commit-bot: I haz the power, 2016-04-27 19:30:41]

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

[commit-bot: I haz the power, 2016-04-27 19:31:51]

Description was changed from

==========
swarming: add support for cipd on the server side

- add CipdPackage ndb and rpc types
- add "packages" field to TaskProperties in ndb and rpc
- validate packages before putting task properties
- if idempotent=true, ensure that package versions are hashes
- include packages in bot manifest

R=maruel@chromium.org
BUG=601022
==========

to

==========
swarming: add support for cipd on the server side

- add CipdPackage ndb and rpc types
- add "packages" field to TaskProperties in ndb and rpc
- validate packages before putting task properties
- if idempotent=true, ensure that package versions are hashes
- include packages in bot manifest

R=maruel@chromium.org
BUG=601022

Committed:
https://github.com/luci/luci-py/commit/1c6c3d329a42a889001c88214fc7082e0f60c1d9
==========

[commit-bot: I haz the power, 2016-04-27 19:31:53]

Committed patchset #4 (id:120001) as
https://github.com/luci/luci-py/commit/1c6c3d329a42a889001c88214fc7082e0f60c1d9

[nodir, 2016-04-29 22:28:08]

https://codereview.chromium.org/1910713002/diff/80001/appengine/swarming/cipd.py
File appengine/swarming/cipd.py (right):

https://codereview.chromium.org/1910713002/diff/80001/appengine/swarming/cipd...
appengine/swarming/cipd.py:10: #
https://chromium.googlesource.com/infra/infra/+/468bb43/appengine/chrome_infr...
On 2016/04/26 08:45:22, Paweł Hajdan Jr. wrote:
> How about a TODO to move chrome_infra_packages to luci-py and share the
regexes?
> 
> Duplicating things is a smell, and they can get out of sync.
> 
> Another idea might be to rely on CIPD itself to do the validation. I'm not
sure
> if we need to duplicate that here.
> 
> Just my 2c, not blocking this CL and totally up to you. With the high quality
> standards for LUCI though, I thought it'd fit. Does that make sense? :)

I talked to Vadim, he is not planning to move chrome_infra_packages in the near
future and he is fine copying these 20 lines of code

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

https://codereview.chromium.org/1910713002/diff/80001/appengine/swarming/serv...
appengine/swarming/server/task_request.py:377: # We don't support different
versions of the same package yet.
On 2016/04/27 17:43:56, M-A Ruel wrote:
> "yet"? Ever? I don't think this comment is necessary

removed comment

https://codereview.chromium.org/1910713002/diff/80001/appengine/swarming/serv...
appengine/swarming/server/task_request.py:381: self.packages =
sorted(self.packages, key=lambda p: p.package_name)
On 2016/04/27 17:43:56, M-A Ruel wrote:
> self.packages.sort(key=lambda p: p.package_name)
> ?

Done.

https://codereview.chromium.org/1910713002/diff/80001/appengine/swarming/swar...
File appengine/swarming/swarming_rpcs.py (right):

https://codereview.chromium.org/1910713002/diff/80001/appengine/swarming/swar...
appengine/swarming/swarming_rpcs.py:86: """A CIPD package to install in
$CIPD_PATH and $PATH before task execution."""
On 2016/04/26 08:45:22, Paweł Hajdan Jr. wrote:
> Quick check: if I'll want to use this to make kitchen use logdog, will I be
able
> to reference $CIPD_PATH in the swarming command to execute?
> 
> We'd request CIPD packages for kitchen and logdog binaries, and I can imagine
> the command would look like this:
> 
> $CIPD_PATH/kitchen/kitchen ... -logdog-butler-path
> $CIPD_PATH/logdog_butler/butler
> 
> Does that make sense? I'm also fine with some alternatives, just wanted to
make
> sure the logdog scenario would work.
> 
> This doesn't block the CL of course.

Normally CIPD packages have binaries right in the root, which means that both
kitchen and butler binaries will be present in $PATH, so no need to explicitly
reference $CIPD_PATH