Milo: Use custom config caching layer

milo/appengine/common/config.go

Index: milo/appengine/common/config.go
diff --git a/milo/appengine/common/config.go b/milo/appengine/common/config.go
index 4e50958e478543a38218877182dba5c0a8d391fd..b1bf1e5d78f3f798d6a947da75be373aef803eb6 100644
--- a/milo/appengine/common/config.go
+++ b/milo/appengine/common/config.go
@@ -6,6 +6,7 @@ package common
 
 import (
 	"fmt"
+	"time"
 
 	"github.com/luci/gae/service/datastore"
 	"github.com/luci/gae/service/info"
@@ -29,26 +30,116 @@ type Project struct {
 	Data []byte `gae:",noindex"`
 }
 
+// The key for the service config entity in datastore.
+const ID = "service_config"

[nodir, 2017/04/04 23:12:08]

this looks as common.ID for other packages, which is abstract. Rename to
ServiceConfigID?

[hinoka, 2017/04/05 20:45:06]

Done.

+
+// ServiceConfig is a container for the instance's service config.
+type ServiceConfig struct {
+	// ID is the datastore key.  This should be static, as there should only be
+	// one service config.
+	ID string `gae:"$id"`
+	// Revision is the revision of the config, taken from luci-config.  This is used
+	// to determine if the entry needs to be refreshed.
+	Revision string
+	// Data The binary proto of the config.

[nodir, 2017/04/04 23:12:08]

s/The/is the/ ?

[hinoka, 2017/04/05 20:45:06]

Done.

+	Data []byte `gae:",noindex"`
+	// Text is the text format of the config.  For human consumption only.
+	Text string `gae:",noindex"`
+	// LastUpdated is the time this config was last updated.
+	LastUpdated time.Time
+}
+
 // GetServiceConfig returns the service (aka global) config for the current
-// instance of Milo.
-func GetSettings(c context.Context) (*config.Settings, error) {
-	cs := cfgclient.CurrentServiceConfigSet(c)
-	msg := &config.Settings{}
-	// Our global config name is called settings.cfg.
-	err := cfgclient.Get(c, cfgclient.AsService, cs, "settings.cfg", textproto.Message(msg), nil)
+// instance of Milo from the datastore.  Returns an empty config and warn heavily
+// if none is found.
+// TODO(hinoka): Use process cache to cache configs.
+func GetSettings(c context.Context) *config.Settings {
+	settings := config.Settings{
+		Buildbot: &config.Settings_Buildbot{},
+	}
+
+	msg, err := GetCurrentServiceConfig(c)
+	if err != nil {
+		// The service config does not exist, just return an empty config
+		// and complain loudly in the logs.
+		logging.WithError(err).Errorf(c,
+			"Encountered error while loading service config, using empty config.")
+		return &settings
+	}
+
+	err = proto.Unmarshal(msg.Data, &settings)
+	if err != nil {
+		// The service config does not exist, just return an empty config

[nodir, 2017/04/04 23:12:08]

i think it does exist, but it is broken

[hinoka, 2017/04/05 20:45:06]

Done.

+		// and complain loudly in the logs.
+		logging.WithError(err).Errorf(c,
+			"Encountered error while unmarshalling service config, using empty config.")
+		// Zero out the message just incase something got written in.
+		settings.Buildbot = &config.Settings_Buildbot{}
+		settings.Buildbucket = nil
+		settings.Swarming = nil

[nodir, 2017/04/04 23:12:08]

what if you add more fields there later? it is safer to do settings =
config.Settings{Buildbot:&config.Settings_Buildbot{}}

[hinoka, 2017/04/05 20:45:06]

Done.

+	}
+
+	return &settings
+}
+
+// GetTextSettings gets
+func GetCurrentServiceConfig(c context.Context) (*ServiceConfig, error) {
+	msg := ServiceConfig{ID: ID}
+	err := datastore.Get(c, &msg)
+	if err != nil {
+		return nil, err
+	}
+	logging.Infof(c, "Loaded config entry from %s", msg.LastUpdated.Format(time.RFC3339))

[nodir, 2017/04/04 23:12:08]

this functions is going to be used a lot, sometimes multiple times in a single
http request. This will cause multiple "Loaded config entry ..." logs in a
single request.

You could remove the logging from GetCurrentServiceConfig, or you could use
https://godoc.org/github.com/luci/luci-go/common/data/caching/proccache here to
cache the config in process memory for a short period of time

[hinoka, 2017/04/05 20:45:06]

Done.

+	return &msg, nil
+}
+
+// UpdateServiceConfig fetches the service config from luci-cfg

[nodir, 2017/04/04 23:12:08]

sometimes this to refers to luci-config as luci-config and sometimes as
luci-cfg. Let's consolidate to luci-cfg

[nodir, 2017/04/04 23:27:15]

i meant let's converge on luci-config (not luci-cfg)

[hinoka, 2017/04/05 20:45:06]

Done.

+// and then stores the binary proto in datastore.

[nodir, 2017/04/04 23:12:08]

remove "binary" because it is not only binary?

[hinoka, 2017/04/05 20:45:06]

Done.

+func UpdateServiceConfig(c context.Context) error {
+	msg := ServiceConfig{ID: ID}
+	err := datastore.Get(c, &msg)
 	switch err {
-	case cfgclient.ErrNoConfig:
-		// Just warn very heavily in the logs, but don't 500, instead return an
-		// empty config.
-		logging.WithError(err).Errorf(c, "settings.cfg does not exist")
-		msg.Buildbot = &config.Settings_Buildbot{}
-		err = nil
+	case datastore.ErrNoSuchEntity:
+		// Might be the first time this has run.
+		logging.WithError(err).Warningf(c, "No existing service config.")
 	case nil:
-		// continue
+		// Continue
 	default:
-		return nil, err
+		logging.WithError(err).Errorf(c, "Could not load existing config")

[nodir, 2017/04/04 23:12:08]

general Go suggestion: handle an error only once. If you log it and return it,
the calling code may log it again => many log lines for one error. How about
return fmt.Errorf("could not load existing config: %s", err) and the calling
code can log it?

[hinoka, 2017/04/05 20:45:06]

Ok i'll just wrap it.

+		return err
+	}
+
+	logging.Infof(c, "Loaded config entry (%s) from %s", msg.Revision,
+		msg.LastUpdated.Format(time.RFC3339))
+
+	// Now, load the settings from luci-config.
+	cs := cfgclient.CurrentServiceConfigSet(c)

[nodir, 2017/04/04 23:12:08]

cfgclient is a wrapper around
https://godoc.org/github.com/luci/luci-go/common/config, but this code does not
use any of the features that the wrapper provides. Maybe use
https://godoc.org/github.com/luci/luci-go/common/config directly?

[hinoka, 2017/04/05 20:45:06]

Ah ok.  I didn't know that existed, I like that better.

edit: turns out the impl isn't currently installed in the context.  I'll punch a
hole to grab it from the backend.

+	settings := &config.Settings{}
+	meta := &cfgclient.Meta{}
+	// Our global config name is called settings.cfg.
+	err = cfgclient.Get(
+		c, cfgclient.AsService, cs, "settings.cfg", textproto.Message(settings), meta)
+	if err != nil {
+		logging.WithError(err).Errorf(c, "could not load settings.cfg from luci-cfg")
+		return err
+	}
+
+	// Check to see if we need to update
+	if msg.Revision != "" && msg.Revision == meta.Revision {

[nodir, 2017/04/04 23:12:08]

comparison and put must happen in the same transaction

[nodir, 2017/04/04 23:12:08]

!= "" seems unnecessary. The revision returned by luci-config cannot be empty

[hinoka, 2017/04/05 20:45:06]

Done.

+		logging.Infof(c, "revisions matched (%s), no need to update", msg.Revision)
+		return nil
+	}
+
+	// We do need to update.
+	msg.Text = proto.MarshalTextString(settings)
+	msg.Data, err = proto.Marshal(settings)
+	if err != nil {
+		logging.Errorf(c, "could not marshal proto into binary\n%s", msg.Text)
+		return err
 	}
-	return msg, err
+	msg.Revision = meta.Revision
+	msg.LastUpdated = time.Now().UTC()

[nodir, 2017/04/04 23:12:08]

logging the fact that revisions do not match is more important than logging that
revisions match

[hinoka, 2017/04/05 20:45:06]

Done.

+	return datastore.Put(c, &msg)
 }
 
 // UpdateProjectConfigs internal project configuration based off luci-cfg.