LogDog: Use luci_config/server packages for config

LogDog: Use luci_config/server packages for config

Use the luci_config/server configuration packages for configuration.
This includes:
- Using server/cfgclient to pull configurations.
- Using the server/cfgclient proccache backend for caching.

BUG=chromium:682465
TEST=None

Review-Url: https://codereview.chromium.org/2647083003
Committed: https://github.com/luci/luci-go/commit/95c4ae33d6197d7098ab2b895ccc3a14b1b82602

[dnj, 2017-01-20 23:40:02]

dnj@chromium.org changed reviewers:
+ iannucci@chromium.org

[dnj, 2017-01-20 23:40:02]

PTAL

[iannucci, 2017-01-21 00:23:14]

lgtm

https://codereview.chromium.org/2647083003/diff/2/logdog/server/service/confi...
File logdog/server/service/config/opts.go (right):

https://codereview.chromium.org/2647083003/diff/2/logdog/server/service/confi...
logdog/server/service/config/opts.go:19: CacheExpiration time.Duration
a negative (or 0?) value has no effect

https://codereview.chromium.org/2647083003/diff/2/logdog/server/service/confi...
logdog/server/service/config/opts.go:23: // configured.
Maybe:

  WrapBackend updates the context with a new backend that applies these
CacheOptions.

https://codereview.chromium.org/2647083003/diff/2/logdog/server/service/confi...
logdog/server/service/config/opts.go:30: if o.CacheExpiration > 0 {
== 0 could be problematic... I'd skip proccache in that case as well (especially
since go has this whole zero-value initialization thing)

https://codereview.chromium.org/2647083003/diff/2/logdog/server/service/servi...
File logdog/server/service/service.go (right):

https://codereview.chromium.org/2647083003/diff/2/logdog/server/service/servi...
logdog/server/service/service.go:94: // onGCE is true if we're on GCE. We probe
this exactly once.
probe it... lazily? on init? from environ?

https://codereview.chromium.org/2647083003/diff/2/logdog/server/service/servi...
logdog/server/service/service.go:265: "If non-zero, poll for configuration
changes and kill the application if one is detected.")
ah, got it... that's why it was killfunc before. Could you clarify that as a
possible for the poller? e.g. "This is useful for stateless servers to kill
themselves when they need to take in new configuration data. It's safer+less
code than trying to ingest new configuration dynamically, and it ensures that
the service manager is actually working correctly :)"

https://codereview.chromium.org/2647083003/diff/2/logdog/server/service/servi...
logdog/server/service/service.go:267: "If set, load configuration from a local
filesystem rooted here.")
for testing? or will this be used in production?

https://codereview.chromium.org/2647083003/diff/2/logdog/server/service/servi...
logdog/server/service/service.go:389: OnChange:      s.shutdown,
os.Exit :p

*chants*
crash! crash! crash! crash! crash! crash! crash! crash! crash! crash!

[dnj, 2017-01-21 01:37:32]

https://codereview.chromium.org/2647083003/diff/2/logdog/server/service/confi...
File logdog/server/service/config/opts.go (right):

https://codereview.chromium.org/2647083003/diff/2/logdog/server/service/confi...
logdog/server/service/config/opts.go:19: CacheExpiration time.Duration
On 2017/01/21 00:23:13, iannucci wrote:
> a negative (or 0?) value has no effect

Done.

https://codereview.chromium.org/2647083003/diff/2/logdog/server/service/confi...
logdog/server/service/config/opts.go:23: // configured.
On 2017/01/21 00:23:13, iannucci wrote:
> Maybe:
> 
>   WrapBackend updates the context with a new backend that applies these
> CacheOptions.

Done.

https://codereview.chromium.org/2647083003/diff/2/logdog/server/service/confi...
logdog/server/service/config/opts.go:30: if o.CacheExpiration > 0 {
On 2017/01/21 00:23:13, iannucci wrote:
> == 0 could be problematic... I'd skip proccache in that case as well
(especially
> since go has this whole zero-value initialization thing)

Done.

https://codereview.chromium.org/2647083003/diff/2/logdog/server/service/servi...
File logdog/server/service/service.go (right):

https://codereview.chromium.org/2647083003/diff/2/logdog/server/service/servi...
logdog/server/service/service.go:94: // onGCE is true if we're on GCE. We probe
this exactly once.
On 2017/01/21 00:23:14, iannucci wrote:
> probe it... lazily? on init? from environ?

Once during "Run". updated.

https://codereview.chromium.org/2647083003/diff/2/logdog/server/service/servi...
logdog/server/service/service.go:265: "If non-zero, poll for configuration
changes and kill the application if one is detected.")
On 2017/01/21 00:23:14, iannucci wrote:
> ah, got it... that's why it was killfunc before. Could you clarify that as a
> possible for the poller? e.g. "This is useful for stateless servers to kill
> themselves when they need to take in new configuration data. It's safer+less
> code than trying to ingest new configuration dynamically, and it ensures that
> the service manager is actually working correctly :)"

Done.

https://codereview.chromium.org/2647083003/diff/2/logdog/server/service/servi...
logdog/server/service/service.go:267: "If set, load configuration from a local
filesystem rooted here.")
On 2017/01/21 00:23:13, iannucci wrote:
> for testing? or will this be used in production?

test

https://codereview.chromium.org/2647083003/diff/2/logdog/server/service/servi...
logdog/server/service/service.go:389: OnChange:      s.shutdown,
On 2017/01/21 00:23:14, iannucci wrote:
> os.Exit :p
> 
> *chants*
> crash! crash! crash! crash! crash! crash! crash! crash! crash! crash! 

I want to flush :(

[dnj, 2017-01-21 01:37:45]

The CQ bit was checked by dnj@chromium.org

[dnj, 2017-01-21 01:37:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
iannucci@chromium.org
Link to the patchset: https://codereview.chromium.org/2647083003/#ps50001
(title: "Comments.")

[commit-bot: I haz the power, 2017-01-21 01:37:47]

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-01-21 01:43:43]

CQ is committing da patch.

Bot data: {"patchset_id": 50001, "attempt_start_ts": 1484962665812440,
"parent_rev": "b4bee637a6593dd2b8d67e96cbc31a515c61f38e", "commit_rev":
"95c4ae33d6197d7098ab2b895ccc3a14b1b82602"}

[commit-bot: I haz the power, 2017-01-21 01:43:46]

Description was changed from

==========
LogDog: Use luci_config/server packages for config

Use the luci_config/server configuration packages for configuration.
This includes:
- Using server/cfgclient to pull configurations.
- Using the server/cfgclient proccache backend for caching.

BUG=chromium:682465
TEST=None
==========

to

==========
LogDog: Use luci_config/server packages for config

Use the luci_config/server configuration packages for configuration.
This includes:
- Using server/cfgclient to pull configurations.
- Using the server/cfgclient proccache backend for caching.

BUG=chromium:682465
TEST=None

Review-Url: https://codereview.chromium.org/2647083003
Committed:
https://github.com/luci/luci-go/commit/95c4ae33d6197d7098ab2b895ccc3a14b1b82602
==========

[commit-bot: I haz the power, 2017-01-21 01:43:47]

Committed patchset #4 (id:50001) as
https://github.com/luci/luci-go/commit/95c4ae33d6197d7098ab2b895ccc3a14b1b82602