config: store binary configs

config: store binary configs

config.get_async: if last good config is requested and destination type
is a protobuf message, store it in binary in addition to raw format.

When loading from a last good conig, if protobuf message type name
matches what we have in the datastore, load the config from the binary
field.

Also fix tests and include them in PRESUBMIT.py

R=vadimsh@chromium.org
BUG=705002
Review-Url: https://codereview.chromium.org/2778533002
Committed: https://github.com/luci/luci-py/commit/b048c2783c11ff79dfc32f3925a65d1f42828ac7

[nodir, 2017-03-24 22:22:29]

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

[commit-bot: I haz the power, 2017-03-24 22:22:35]

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, 2017-03-24 22:27:05]

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

[commit-bot: I haz the power, 2017-03-24 22:27:06]

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

[nodir, 2017-03-24 22:31:35]

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

[commit-bot: I haz the power, 2017-03-24 22:31:45]

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

[nodir, 2017-03-24 22:31:45]

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

[nodir, 2017-03-24 22:31:46]

PTAL

[nodir, 2017-03-24 22:32:35]

tested on -dev. Here is an updated LastGoodConfig entity
https://pantheon.corp.google.com/datastore/entities/edit?key=0%2F%7C14%2FLast...

[commit-bot: I haz the power, 2017-03-24 22:34:26]

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

[commit-bot: I haz the power, 2017-03-24 22:34:27]

Dry run: This issue passed the CQ dry run.

[nodir, 2017-03-28 22:11:16]

ping

[Vadim Sh., 2017-03-28 22:34:47]

https://codereview.chromium.org/2778533002/diff/20001/appengine/components/co...
File appengine/components/components/config/remote.py (right):

https://codereview.chromium.org/2778533002/diff/20001/appengine/components/co...
appengine/components/components/config/remote.py:245: if current.content_hash !=
content_hash or binary_missing:
why 'binary_missing' check? If 'current.content_hash != content_hash', we
already have the content in the datastore, no need to fetch it again.

https://codereview.chromium.org/2778533002/diff/20001/appengine/components/co...
appengine/components/components/config/remote.py:277: if
config.proto_message_name and not config.content_binary:
nit: convert body of this 'if' into a function that either returns a blob, None
(if proto message is not defined), or raises ConfigFormatError.

https://codereview.chromium.org/2778533002/diff/20001/appengine/components/co...
appengine/components/components/config/remote.py:283: 'Could not load message
type %s. Skipping binary serialization')
%s is not defined

https://codereview.chromium.org/2778533002/diff/20001/appengine/components/co...
appengine/components/components/config/remote.py:333:
last_good.proto_message_name = proto_message_name
if proto_message_name changes, we need to reset content_binary, because it is no
longer valid (wrong proto class). The cron will regenerated it then.

https://codereview.chromium.org/2778533002/diff/20001/appengine/components/co...
appengine/components/components/config/remote.py:363: cfg = cfg or
common._convert_config(last_good.content, dest_type)
nit: do cfg = common._convert_config(last_good.content, dest_type) in 'if not
last_good ...' section above. It's less confusing that way.

[nodir, 2017-03-28 23:20:22]

https://codereview.chromium.org/2778533002/diff/20001/appengine/components/co...
File appengine/components/components/config/remote.py (right):

https://codereview.chromium.org/2778533002/diff/20001/appengine/components/co...
appengine/components/components/config/remote.py:245: if current.content_hash !=
content_hash or binary_missing:
On 2017/03/28 22:34:47, Vadim Sh. wrote:
> why 'binary_missing' check? If 'current.content_hash != content_hash', we
> already have the content in the datastore, no need to fetch it again.

this is how I backfill content_binary. It will happen once per entity

https://codereview.chromium.org/2778533002/diff/20001/appengine/components/co...
appengine/components/components/config/remote.py:277: if
config.proto_message_name and not config.content_binary:
On 2017/03/28 22:34:47, Vadim Sh. wrote:
> nit: convert body of this 'if' into a function that either returns a blob,
None
> (if proto message is not defined), or raises ConfigFormatError.

Done.

https://codereview.chromium.org/2778533002/diff/20001/appengine/components/co...
appengine/components/components/config/remote.py:283: 'Could not load message
type %s. Skipping binary serialization')
On 2017/03/28 22:34:47, Vadim Sh. wrote:
> %s is not defined

Done.

https://codereview.chromium.org/2778533002/diff/20001/appengine/components/co...
appengine/components/components/config/remote.py:333:
last_good.proto_message_name = proto_message_name
On 2017/03/28 22:34:47, Vadim Sh. wrote:
> if proto_message_name changes, we need to reset content_binary, because it is
no
> longer valid (wrong proto class). The cron will regenerated it then.

Done.

https://codereview.chromium.org/2778533002/diff/20001/appengine/components/co...
appengine/components/components/config/remote.py:363: cfg = cfg or
common._convert_config(last_good.content, dest_type)
On 2017/03/28 22:34:47, Vadim Sh. wrote:
> nit: do cfg = common._convert_config(last_good.content, dest_type) in 'if not
> last_good ...' section above. It's less confusing that way.

talked offline, not needed

[Vadim Sh., 2017-03-28 23:36:59]

lgtm

https://codereview.chromium.org/2778533002/diff/20001/appengine/components/co...
File appengine/components/components/config/remote.py (right):

https://codereview.chromium.org/2778533002/diff/20001/appengine/components/co...
appengine/components/components/config/remote.py:245: if current.content_hash !=
content_hash or binary_missing:
On 2017/03/28 23:20:22, nodir wrote:
> On 2017/03/28 22:34:47, Vadim Sh. wrote:
> > why 'binary_missing' check? If 'current.content_hash != content_hash', we
> > already have the content in the datastore, no need to fetch it again.
> 
> this is how I backfill content_binary. It will happen once per entity

I'm saying if you remove "or binary_missing" here, everything will still be
working fine (I think...). Trace the code. It will hit "if ... not
config.content_binary" condition and will rebuild content_binary using
config.content.

https://codereview.chromium.org/2778533002/diff/60001/appengine/components/co...
File appengine/components/components/config/remote.py (right):

https://codereview.chromium.org/2778533002/diff/60001/appengine/components/co...
appengine/components/components/config/remote.py:318: logging.exception(
there'll be a lot of exceptions in Swarming and Isolate once you deploy this.
Consider regenerating all protos in luci-py and infra.git.

[M-A Ruel, 2017-03-28 23:48:22]

lgtm with random comments. Thanks a lot! Sorry for missing it.

https://codereview.chromium.org/2778533002/diff/60001/appengine/components/co...
File appengine/components/components/config/remote.py (right):

https://codereview.chromium.org/2778533002/diff/60001/appengine/components/co...
appengine/components/components/config/remote.py:327: if (last_good is None or
optional and shorter:
not last_good or not last_goog.last_access_ts or

https://codereview.chromium.org/2778533002/diff/60001/appengine/components/co...
appengine/components/components/config/remote.py:350: logging.error(
Is it worth alerting for this? e.g. should it be warning() level?

[nodir, 2017-03-29 22:24:32]

https://codereview.chromium.org/2778533002/diff/20001/appengine/components/co...
File appengine/components/components/config/remote.py (right):

https://codereview.chromium.org/2778533002/diff/20001/appengine/components/co...
appengine/components/components/config/remote.py:245: if current.content_hash !=
content_hash or binary_missing:
On 2017/03/28 23:36:58, Vadim Sh. wrote:
> On 2017/03/28 23:20:22, nodir wrote:
> > On 2017/03/28 22:34:47, Vadim Sh. wrote:
> > > why 'binary_missing' check? If 'current.content_hash != content_hash', we
> > > already have the content in the datastore, no need to fetch it again.
> > 
> > this is how I backfill content_binary. It will happen once per entity
> 
> I'm saying if you remove "or binary_missing" here, everything will still be
> working fine (I think...). Trace the code. It will hit "if ... not
> config.content_binary" condition and will rebuild content_binary using
> config.content.

Done.

https://codereview.chromium.org/2778533002/diff/60001/appengine/components/co...
File appengine/components/components/config/remote.py (right):

https://codereview.chromium.org/2778533002/diff/60001/appengine/components/co...
appengine/components/components/config/remote.py:318: logging.exception(
On 2017/03/28 23:36:58, Vadim Sh. wrote:
> there'll be a lot of exceptions in Swarming and Isolate once you deploy this.
> Consider regenerating all protos in luci-py and infra.git.

Config protos in swarming and isolated are generated with an up-to-date protoc,
they are fine

https://codereview.chromium.org/2778533002/diff/60001/appengine/components/co...
appengine/components/components/config/remote.py:327: if (last_good is None or
On 2017/03/28 23:48:22, M-A Ruel wrote:
> optional and shorter:
> not last_good or not last_goog.last_access_ts or

Done.

https://codereview.chromium.org/2778533002/diff/60001/appengine/components/co...
appengine/components/components/config/remote.py:350: logging.error(
On 2017/03/28 23:48:22, M-A Ruel wrote:
> Is it worth alerting for this? e.g. should it be warning() level?

the idea is to log an error but silence <10 errors. We should not miss if this
persists

[nodir, 2017-03-29 22:24:38]

The CQ bit was checked by nodir@chromium.org

[nodir, 2017-03-29 22:24:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
vadimsh@chromium.org, maruel@chromium.org
Link to the patchset: https://codereview.chromium.org/2778533002/#ps80001
(title: "addressed comments")

[commit-bot: I haz the power, 2017-03-29 22:24:44]

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, 2017-03-29 22:27:16]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490826278575100,
"parent_rev": "c497fefe848a225036b03dc7b44454c0c16a55d3", "commit_rev":
"b048c2783c11ff79dfc32f3925a65d1f42828ac7"}

[commit-bot: I haz the power, 2017-03-29 22:27:21]

Description was changed from

==========
config: store binary configs

config.get_async: if last good config is requested and destination type
is a protobuf message, store it in binary in addition to raw format.

When loading from a last good conig, if protobuf message type name
matches what we have in the datastore, load the config from the binary
field.

Also fix tests and include them in PRESUBMIT.py

R=vadimsh@chromium.org
BUG=705002
==========

to

==========
config: store binary configs

config.get_async: if last good config is requested and destination type
is a protobuf message, store it in binary in addition to raw format.

When loading from a last good conig, if protobuf message type name
matches what we have in the datastore, load the config from the binary
field.

Also fix tests and include them in PRESUBMIT.py

R=vadimsh@chromium.org
BUG=705002

Review-Url: https://codereview.chromium.org/2778533002
Committed:
https://github.com/luci/luci-py/commit/b048c2783c11ff79dfc32f3925a65d1f42828ac7
==========

[commit-bot: I haz the power, 2017-03-29 22:27:22]

Committed patchset #5 (id:80001) as
https://github.com/luci/luci-py/commit/b048c2783c11ff79dfc32f3925a65d1f42828ac7