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..2d3248b7923819809d8e73aadbcd57b7efcccdea 100644
--- a/milo/appengine/common/config.go
+++ b/milo/appengine/common/config.go
@@ -6,11 +6,14 @@ package common
 
 import (
 	"fmt"
+	"time"
 
 	"github.com/luci/gae/service/datastore"
 	"github.com/luci/gae/service/info"
+	"github.com/luci/luci-go/common/data/caching/proccache"
 	"github.com/luci/luci-go/common/logging"
 	"github.com/luci/luci-go/luci_config/server/cfgclient"
+	"github.com/luci/luci-go/luci_config/server/cfgclient/backend"
 	"github.com/luci/luci-go/luci_config/server/cfgclient/textproto"
 	"github.com/luci/luci-go/milo/common/config"
 
@@ -20,38 +23,150 @@ import (
 
 // Project is a LUCI project.
 type Project struct {
-	// The ID of the project, as per self defined.  This is not the luci-cfg
+	// The ID of the project, as per self defined.  This is not the luci-config
 	// name.
 	ID string `gae:"$id"`
-	// The luci-cfg name of the project.
+	// The luci-config name of the project.
 	Name string
 	// The Project data in protobuf binary format.
 	Data []byte `gae:",noindex"`
 }
 
+// The key for the service config entity in datastore.
+const ServiceConfigID = "service_config"
+
+// 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 is the binary proto of the config.
+	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)
-	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 nil:
-		// continue
-	default:
+// 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 is broken, just return an empty config
+		// 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 = config.Settings{Buildbot: &config.Settings_Buildbot{}}
+	}
+
+	return &settings
+}
+
+// GetTextSettings gets
+func GetCurrentServiceConfig(c context.Context) (*ServiceConfig, error) {
+	// Try Proccess Cache first
+	if item, ok := proccache.Get(c, ServiceConfigID); ok {

[nodir, 2017/04/05 21:47:50]

if 2 goroutines call GetCurrentServiceConfig at the same time, both will go to
datastore. Use
https://godoc.org/github.com/luci/luci-go/common/data/caching/proccache#GetOr...

[hinoka, 2017/04/06 03:34:33]

Done.

+		if msg, ok := item.(ServiceConfig); ok {
+			logging.Infof(
+				c, "Loaded proccached config entry from %s", msg.LastUpdated.Format(time.RFC3339))
+			return &msg, nil
+		}
+	}
+
+	msg := ServiceConfig{ID: ServiceConfigID}
+	err := datastore.Get(c, &msg)
+	if err != nil {
 		return nil, err
 	}
-	return msg, err
+	// Save it in process cache.
+	proccache.Put(c, ServiceConfigID, msg, time.Minute*10)
+	logging.Infof(c, "Loaded config entry from %s", msg.LastUpdated.Format(time.RFC3339))
+	return &msg, nil
+}
+
+// UpdateServiceConfig fetches the service config from luci-config
+// and then stores a snapshot of the configuration in datastore.
+func UpdateServiceConfig(c context.Context) error {
+	// Load the settings from luci-config.
+	cs := string(cfgclient.CurrentServiceConfigSet(c))
+	// Acquire the raw config client.
+	lucicfg := backend.Get(c).GetConfigInterface(c, backend.AsService)
+	// Our global config name is called settings.cfg.
+	cfg, err := lucicfg.GetConfig(c, cs, "settings.cfg", false)
+	if err != nil {
+		logging.WithError(err).Errorf(c, "could not load settings.cfg from luci-config")
+		return err

[nodir, 2017/04/05 21:47:50]

you said you will wrap it, but this is not wrapping and you still log the error

[hinoka, 2017/04/06 03:34:34]

Done.

+	}
+	settings := &config.Settings{}
+	err = proto.UnmarshalText(cfg.Content, settings)
+	if err != nil {
+		logging.WithError(err).Errorf(
+			c, "could not unmarshal proto from luci-config:\n%s", cfg.Content)

[nodir, 2017/04/05 21:47:50]

did you mean to wrap and return the error? currently it just logs and goes on

[hinoka, 2017/04/06 03:34:34]

Done.

+	}
+	newConfig := ServiceConfig{ID: ServiceConfigID}

[nodir, 2017/04/05 21:47:50]

nit: all fields ut but ID are set with assignment statements. Be consistent.
Consider assigning all fields but Data like you assign ID

[hinoka, 2017/04/06 03:34:33]

Done.

+	newConfig.Text = cfg.Content
+	newConfig.Data, err = proto.Marshal(settings)
+	if err != nil {
+		logging.Errorf(c, "could not marshal proto into binary\n%s", newConfig.Text)

[nodir, 2017/04/05 21:47:50]

this double logs. wrap and return without logging?

+		return err
+	}
+	newConfig.Revision = cfg.Revision
+	newConfig.LastUpdated = time.Now().UTC()
+
+	// Do the revision check & swap in a datastore transaction.
+	err = datastore.RunInTransaction(c, func(c context.Context) error {
+		oldConfig := ServiceConfig{ID: ServiceConfigID}
+		err := datastore.Get(c, &oldConfig)
+		switch err {
+		case datastore.ErrNoSuchEntity:
+			// Might be the first time this has run.
+			logging.WithError(err).Warningf(c, "No existing service config.")
+		case nil:
+			// Continue
+		default:
+			return fmt.Errorf("could not load existing config: %s", err)
+		}
+		// Check to see if we need to update
+		if oldConfig.Revision == newConfig.Revision {
+			logging.Infof(c, "revisions matched (%s), no need to update", oldConfig.Revision)
+			return nil
+		}
+		logging.Infof(c, "revisions differ (old %s, new %s), updating",
+			oldConfig.Revision, newConfig.Revision)
+		return datastore.Put(c, &newConfig)
+	}, nil)
+
+	if err != nil {
+		return fmt.Errorf("failed to update config entry in transaction", err)
+	}
+	logging.Infof(c, "successfully updated to new config")
+
+	// Save it in process cache also.
+	proccache.Put(c, ServiceConfigID, newConfig, time.Minute*10)

[nodir, 2017/04/05 21:47:50]

note that this will affect only one of your many appengine instances. Maybe
remove this and reduce timeout to like 1min?

[hinoka, 2017/04/06 03:34:34]

Done.

+	return nil
 }
 
-// UpdateProjectConfigs internal project configuration based off luci-cfg.
+// UpdateProjectConfigs internal project configuration based off luci-config.
 // update updates Milo's configuration based off luci config.  This includes
 // scanning through all project and extract all console configs.
 func UpdateProjectConfigs(c context.Context) error {
@@ -61,6 +176,7 @@ func UpdateProjectConfigs(c context.Context) error {
 		configs []*config.Project
 		metas   []*cfgclient.Meta
 	)
+	logging.Debugf(c, "fetching configs for %s", cfgName)
 	if err := cfgclient.Projects(c, cfgclient.AsService, cfgName, textproto.Slice(&configs), &metas); err != nil {
 		logging.WithError(err).Errorf(c, "Encountered error while getting project config for %s", cfgName)
 		return err