Add snapshotting for CIPD packages and dimensions to DM.

Add snapshotting for CIPD packages and dimensions to DM.

This makes DM automatically pin all cipd packages used by an attempt, and optionally allows the quest to specify dimensions to pin as well. This prevents confusing bugs where e.g. a certain version of kitchen is used for the first execution of an attempt, but a different version is used for a subsequent execution. This means things like the 'state' that DM passes between executions doesn't need to worry about being interpreted by different versions of the various cipd packages in the job.

Pinning dimensions could be useful for pinning instance ids (for affinity), but more trivially is useful for pinning os/cpu when pinning a VERY generic cipd spec (e.g. things with ${platform} in the spec). In this case you could potentially formulate a quest that could run on ANY platform and get consistent re-executions.

BUG=639975
Committed: https://github.com/luci/luci-go/commit/d5fba5ee301b374e9c161fcfa8c4693b1a058e2c

[iannucci, 2016-09-26 23:01:26]

Description was changed from

==========
Add snapshotting for CIPD packages and dimensions to DM.

BUG=639975
==========

to

==========
Add snapshotting for CIPD packages and dimensions to DM.

This makes DM automatically pin all cipd packages used by an attempt, and
optionally allows the quest to specify dimensions to pin as well. This prevents
confusing bugs where e.g. a certain version of kitchen is used for the first
execution of an attempt, but a different version is used for a subsequent
execution. This means things like the 'state' that DM passes between executions
doesn't need to worry about being interpreted by different versions of the
various cipd packages in the job.

Pinning dimensions could be useful for pinning instance ids (for affinity), but
more trivially is useful for pinning os/cpu when pinning a VERY generic cipd
spec (e.g. things with ${platform} in the spec). In this case you could
potentially formulate a quest that could run on ANY platform and get consistent
re-executions.

BUG=639975
==========

[iannucci, 2016-09-26 23:01:26]

iannucci@chromium.org changed reviewers:
+ dnj@chromium.org, nodir@chromium.org, vadimsh@chromium.org

[iannucci, 2016-09-26 23:01:41]

PTAL

[iannucci, 2016-09-26 23:15:17]

https://codereview.chromium.org/2338153003/diff/100001/dm/api/distributor/swa...
File dm/api/distributor/swarming/v1/result.proto (right):

https://codereview.chromium.org/2338153003/diff/100001/dm/api/distributor/swa...
dm/api/distributor/swarming/v1/result.proto:23: map<string, string>
snapshot_dimensions = 4;
this is where the pin information is preserved across executions.

https://codereview.chromium.org/2338153003/diff/100001/dm/appengine/distribut...
File dm/appengine/distributor/swarming/v1/distributor.go (right):

https://codereview.chromium.org/2338153003/diff/100001/dm/appengine/distribut...
dm/appengine/distributor/swarming/v1/distributor.go:151: }
this is where the pinned cipd inputs are applied

https://codereview.chromium.org/2338153003/diff/100001/dm/appengine/distribut...
dm/appengine/distributor/swarming/v1/distributor.go:172: }
this is where the pinned dimensions are applied

https://codereview.chromium.org/2338153003/diff/100001/dm/appengine/distribut...
dm/appengine/distributor/swarming/v1/distributor.go:291:
retData.SnapshotDimensions = snapshotDimensions(params, rslt.BotDimensions)
this is where the pinning happens

[Vadim Sh., 2016-09-26 23:16:06]

code lgtm, but I'm not convinced we need dimension pinning

https://codereview.chromium.org/2338153003/diff/100001/dm/api/distributor/swa...
File dm/api/distributor/swarming/v1/normalize.go (right):

https://codereview.chromium.org/2338153003/diff/100001/dm/api/distributor/swa...
dm/api/distributor/swarming/v1/normalize.go:112: "job.inputs: at least one of
packages and isolated must be specified")
theoretically a job can call something that is predeployed on bots...

https://codereview.chromium.org/2338153003/diff/100001/dm/api/distributor/swa...
File dm/api/distributor/swarming/v1/params.proto (right):

https://codereview.chromium.org/2338153003/diff/100001/dm/api/distributor/swa...
dm/api/distributor/swarming/v1/params.proto:47: google.protobuf.Duration
io_timeout = 4;
you are changing tags. Are you sure this stuff is never serialized in binary
form?

https://codereview.chromium.org/2338153003/diff/100001/dm/appengine/distribut...
File dm/appengine/distributor/swarming/v1/distributor.go (right):

https://codereview.chromium.org/2338153003/diff/100001/dm/appengine/distribut...
dm/appengine/distributor/swarming/v1/distributor.go:147: if prev != nil {
nit: prevParsed != nil (and below)

https://codereview.chromium.org/2338153003/diff/100001/dm/appengine/distribut...
dm/appengine/distributor/swarming/v1/distributor.go:169: for k, v := range
prevParsed.SnapshotDimensions {
I'm not 100% sure it is what we want all the time.

https://codereview.chromium.org/2338153003/diff/100001/dm/appengine/distribut...
dm/appengine/distributor/swarming/v1/distributor.go:232: allDims[dim.Key] =
dim.Value[len(dim.Value)-1]
I don't think we guarantee the order of dimensions to mean anything. In fact,
they are just sorted alphabetically :)
https://github.com/luci/luci-py/blob/master/appengine/swarming/swarming_bot/a...

[iannucci, 2016-09-27 01:23:56]

https://chromiumcodereview.appspot.com/2338153003/diff/100001/dm/api/distribu...
File dm/api/distributor/swarming/v1/normalize.go (right):

https://chromiumcodereview.appspot.com/2338153003/diff/100001/dm/api/distribu...
dm/api/distributor/swarming/v1/normalize.go:112: "job.inputs: at least one of
packages and isolated must be specified")
On 2016/09/26 23:16:06, Vadim Sh. wrote:
> theoretically a job can call something that is predeployed on bots...

Yeah, I thought of that, but the likelihood that you actually want to do that is
pretty low. If someone hits that they can ask a question and we can discuss on
infra-dev or something :)

https://chromiumcodereview.appspot.com/2338153003/diff/100001/dm/api/distribu...
File dm/api/distributor/swarming/v1/params.proto (right):

https://chromiumcodereview.appspot.com/2338153003/diff/100001/dm/api/distribu...
dm/api/distributor/swarming/v1/params.proto:47: google.protobuf.Duration
io_timeout = 4;
On 2016/09/26 23:16:06, Vadim Sh. wrote:
> you are changing tags. Are you sure this stuff is never serialized in binary
> form?

so far? yes, I'm sure it's never used in binary form.

even later-on it won't be binary-ized, but I probably won't change tags at that
point.

https://chromiumcodereview.appspot.com/2338153003/diff/100001/dm/appengine/di...
File dm/appengine/distributor/swarming/v1/distributor.go (right):

https://chromiumcodereview.appspot.com/2338153003/diff/100001/dm/appengine/di...
dm/appengine/distributor/swarming/v1/distributor.go:147: if prev != nil {
On 2016/09/26 23:16:06, Vadim Sh. wrote:
> nit: prevParsed != nil (and below)

ah yeah. done

https://chromiumcodereview.appspot.com/2338153003/diff/100001/dm/appengine/di...
dm/appengine/distributor/swarming/v1/distributor.go:169: for k, v := range
prevParsed.SnapshotDimensions {
On 2016/09/26 23:16:06, Vadim Sh. wrote:
> I'm not 100% sure it is what we want all the time.

yeah me either, but it's a start. I think even if it's not what we want all the
time, I think it's pretty likely that swarming will need to change the sort
order and/or API, at which point we can update this code.

if, somehow, it's not up to swarming, we could add a 'ranking' for some
dimensions (e.g. prefer values for the dimension 'cpu' in the following order
[a, b, c]). Do you think I should do that now?

[Vadim Sh., 2016-09-27 03:07:11]

lgtm

https://chromiumcodereview.appspot.com/2338153003/diff/100001/dm/appengine/di...
File dm/appengine/distributor/swarming/v1/distributor.go (right):

https://chromiumcodereview.appspot.com/2338153003/diff/100001/dm/appengine/di...
dm/appengine/distributor/swarming/v1/distributor.go:169: for k, v := range
prevParsed.SnapshotDimensions {
On 2016/09/27 01:23:56, iannucci wrote:
> On 2016/09/26 23:16:06, Vadim Sh. wrote:
> > I'm not 100% sure it is what we want all the time.
> 
> yeah me either, but it's a start. I think even if it's not what we want all
the
> time, I think it's pretty likely that swarming will need to change the sort
> order and/or API, at which point we can update this code.
> 
> if, somehow, it's not up to swarming, we could add a 'ranking' for some
> dimensions (e.g. prefer values for the dimension 'cpu' in the following order
> [a, b, c]). Do you think I should do that now?

No, it's fine as is.

[iannucci, 2016-09-27 22:39:08]

The CQ bit was checked by iannucci@chromium.org

[commit-bot: I haz the power, 2016-09-27 22:39:12]

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-09-27 22:55:11]

Description was changed from

==========
Add snapshotting for CIPD packages and dimensions to DM.

This makes DM automatically pin all cipd packages used by an attempt, and
optionally allows the quest to specify dimensions to pin as well. This prevents
confusing bugs where e.g. a certain version of kitchen is used for the first
execution of an attempt, but a different version is used for a subsequent
execution. This means things like the 'state' that DM passes between executions
doesn't need to worry about being interpreted by different versions of the
various cipd packages in the job.

Pinning dimensions could be useful for pinning instance ids (for affinity), but
more trivially is useful for pinning os/cpu when pinning a VERY generic cipd
spec (e.g. things with ${platform} in the spec). In this case you could
potentially formulate a quest that could run on ANY platform and get consistent
re-executions.

BUG=639975
==========

to

==========
Add snapshotting for CIPD packages and dimensions to DM.

This makes DM automatically pin all cipd packages used by an attempt, and
optionally allows the quest to specify dimensions to pin as well. This prevents
confusing bugs where e.g. a certain version of kitchen is used for the first
execution of an attempt, but a different version is used for a subsequent
execution. This means things like the 'state' that DM passes between executions
doesn't need to worry about being interpreted by different versions of the
various cipd packages in the job.

Pinning dimensions could be useful for pinning instance ids (for affinity), but
more trivially is useful for pinning os/cpu when pinning a VERY generic cipd
spec (e.g. things with ${platform} in the spec). In this case you could
potentially formulate a quest that could run on ANY platform and get consistent
re-executions.

BUG=639975

Committed:
https://github.com/luci/luci-go/commit/d5fba5ee301b374e9c161fcfa8c4693b1a058e2c
==========

[commit-bot: I haz the power, 2016-09-27 22:55:12]

Committed patchset #8 (id:140001) as
https://github.com/luci/luci-go/commit/d5fba5ee301b374e9c161fcfa8c4693b1a058e2c