Load default values from flagset defaults when unmarshaling to YAML (#…

…122)

* load default values from flagset defaults when unmarshaling to YAML

Implementing yaml.Unmarshaler by setting the default object (i.e., the
"Prometheus way" of doing it) interferes with default values initialized
through command line flags. As the Agent uses config objects from both
Prometheus and Cortex, these two mechanisms of applying defaults were
stepping over each other.

The new methodology for applying defaults is as follows:

1. Every config object within pkg/prom should have a DefaultConfig
   object.

2. When that config object relies on default values from flags, there
   should be a GetDefaultConfig function within the package that creates
   a temporary FlagSet for getting default values. The DefaultConfig
   for the package should be the result of calling this function.

3. Every Config object must implement yaml.Unmarshaler to assign the
   default object.

4. DefaultConfigs that contain other config structs must also initialize
   the subconfig structs by using the DefaultConfig from that given
   package.

This commit also changes some other small things:

1. The scraping service Tanka configs can now be given an explicit
   image set.
2. The shutdown order of the scraping service has been fixed.
3. A logging message has been fixed.

* create shared function to get a DefaultConfig object from Cortex-style config structs

example/k3d/environment/main.jsonnet

@@ -5,6 +5,10 @@ local agent_cluster = import 'grafana-agent/scraping-svc/main.libsonnet';
 local k = import 'ksonnet-util/kausal.libsonnet';
 
 local service = k.core.v1.service;
+local images = {
+  agent: 'grafana/agent:latest',
+  agentctl: 'grafana/agentctl:latest',
+};
 
 {
   default: default.new(namespace='default') {
@@ -21,11 +25,7 @@ local service = k.core.v1.service;
   },
 
   agent: grafana_agent {
-    _images+:: {
-      agent: 'grafana/agent:latest',
-      agentctl: 'grafana/agentctl:latest',
-    },
-
+    _images+:: images, 
     _config+:: {
       namespace: 'default',
 
@@ -62,6 +62,7 @@ local service = k.core.v1.service;
 
   agent_cluster:
     agent_cluster.new('default', 'kube-system') +
+    agent_cluster.withImagesMixin(images) +
     agent_cluster.withConfigMixin({
       local kvstore = {
         store: 'etcd',

pkg/config/config_test.go

@@ -10,6 +10,25 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+// TestConfig_FlagDefaults makes sure that default values of flags are kept
+// when parsing the config.
+func TestConfig_FlagDefaults(t *testing.T) {
+	cfg := `
+prometheus:
+  wal_directory: /tmp/wal
+  global:
+    scrape_timeout: 33s`
+
+	fs := flag.NewFlagSet("test", flag.ExitOnError)
+	c, err := load(fs, []string{"-config.file", "test"}, func(_ string, c *Config) error {
+		return LoadBytes([]byte(cfg), c)
+	})
+	require.NoError(t, err)
+	require.NotEmpty(t, c.Prometheus.ServiceConfig.Lifecycler.InfNames)
+	require.NotZero(t, c.Prometheus.ServiceConfig.Lifecycler.NumTokens)
+	require.NotZero(t, c.Prometheus.ServiceConfig.Lifecycler.HeartbeatPeriod)
+}
+
 func TestConfig_OverrideDefaultsOnLoad(t *testing.T) {
 	cfg := `
 prometheus:

pkg/prom/agent.go

@@ -37,6 +37,8 @@ var (
 	DefaultConfig = Config{
 		Global:                 config.DefaultGlobalConfig,
 		InstanceRestartBackoff: 5 * time.Second,
+		ServiceConfig:          ha.DefaultConfig,
+		ServiceClientConfig:    client.DefaultConfig,
 	}
 )
 
@@ -51,6 +53,7 @@ type Config struct {
 	InstanceRestartBackoff time.Duration       `yaml:"instance_restart_backoff,omitempty"`
 }
 
+// UnmarshalYAML implements yaml.Unmarshaler.
 func (c *Config) UnmarshalYAML(unmarshal func(interface{}) error) error {
 	*c = DefaultConfig
 

pkg/prom/ha/client/client.go

@@ -7,6 +7,7 @@ import (
 	"github.com/cortexproject/cortex/pkg/util/grpcclient"
 
 	"github.com/grafana/agent/pkg/agentproto"
+	"github.com/grafana/agent/pkg/util"
 	otgrpc "github.com/opentracing-contrib/go-grpc"
 	"github.com/opentracing/opentracing-go"
 	"github.com/weaveworks/common/middleware"
@@ -19,11 +20,24 @@ type ScrapingServiceClient interface {
 	io.Closer
 }
 
+var (
+	// DefaultConfig provides default Config values.
+	DefaultConfig = *util.DefaultConfigFromFlags(&Config{}).(*Config)
+)
+
 // Config controls how scraping service clients are created.
 type Config struct {
 	GRPCClientConfig grpcclient.Config `yaml:"grpc_client_config"`
 }
 
+// UnmarshalYAML implements yaml.Unmarshaler.
+func (c *Config) UnmarshalYAML(unmarshal func(interface{}) error) error {
+	*c = DefaultConfig
+
+	type plain Config
+	return unmarshal((*plain)(c))
+}
+
 // RegisterFlags registers flags to the provided flag set.
 func (c *Config) RegisterFlags(f *flag.FlagSet) {
 	c.GRPCClientConfig.RegisterFlags("prometheus.service-client", f)

pkg/prom/ha/server.go

@@ -21,6 +21,7 @@ import (
 	"github.com/grafana/agent/pkg/agentproto"
 	"github.com/grafana/agent/pkg/prom/ha/client"
 	"github.com/grafana/agent/pkg/prom/instance"
+	flagutil "github.com/grafana/agent/pkg/util"
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/promauto"
 	"github.com/prometheus/prometheus/config"
@@ -43,6 +44,9 @@ var (
 		MaxBackoff: 2 * time.Minute,
 		MaxRetries: 10,
 	}
+
+	// DefaultConfig provides default values for the config
+	DefaultConfig = *flagutil.DefaultConfigFromFlags(&Config{}).(*Config)
 )
 
 // InstanceManager is an interface to manipulating a set of running
@@ -69,6 +73,14 @@ type Config struct {
 	Lifecycler      ring.LifecyclerConfig `yaml:"lifecycler"`
 }
 
+// UnmarshalYAML implements yaml.Unmarshaler.
+func (c *Config) UnmarshalYAML(unmarshal func(interface{}) error) error {
+	*c = DefaultConfig
+
+	type plain Config
+	return unmarshal((*plain)(c))
+}
+
 // RegisterFlags adds the flags required to config the Server to the given
 // FlagSet.
 func (c *Config) RegisterFlags(f *flag.FlagSet) {
@@ -157,7 +169,10 @@ func New(cfg Config, globalConfig *config.GlobalConfig, clientConfig client.Conf
 		lc.Addr,
 		r,
 		kvClient,
-		stopServices(r, lc),
+
+		// The lifecycler must stop first since the shutdown process depends on the
+		// ring still polling.
+		stopServices(lc, r),
 	)
 
 	lazy.inner = s
@@ -281,7 +296,7 @@ func (s *Server) notifyReshard(ctx context.Context, desc *ring.IngesterDesc) err
 			break
 		}
 
-		level.Warn(s.logger).Log("failed to tell remote agent to reshard", "err", err, "addr", desc.Addr)
+		level.Warn(s.logger).Log("msg", "failed to tell remote agent to reshard", "err", err, "addr", desc.Addr)
 		backoff.Wait()
 	}
 

pkg/util/defaults.go

@@ -0,0 +1,63 @@
+package util
+
+import "flag"
+
+// DefaultConfigFromFlags will load default values into cfg by
+// retrieving default values that are registered as flags.
+//
+// cfg must implement either PrefixedConfigFlags or ConfigFlags.
+func DefaultConfigFromFlags(cfg interface{}) interface{} {
+	// This function is super ugly but is required for mixing the combination
+	// of mechanisms for providing default for config structs that are used
+	// across both Prometheus (via UnmarshalYAML and assigning the default object)
+	// and Cortex (via RegisterFlags*).
+	//
+	// The issue stems from default values assigned via RegisterFlags being set
+	// at *registration* time, not *flag parse* time. For example, this
+	// flag:
+	//
+	//   fs.BoolVar(&enabled, "enabled", true, "enable everything")
+	//
+	// Sets enabled to true as soon as fs.BoolVar is called. Normally this is
+	// fine, but with how Prometheus implements UnmarshalYAML, these defaults
+	// get overridden:
+	//
+	//   func (c *Config) UnmarshalYAML(unmarshal func(v interface{}) error) error {
+	//     *c = DefaultConfig // <-- !! overrides defaults from flags !!
+	//     type plain Config
+	//     return unmarshal((*plain)(c))
+	//   }
+	//
+	// The solution to this is to make sure that the DefaultConfig object contains
+	// the defaults that are set up through registering flags. Unfortunately, the
+	// best way to do this is this function that creates a temporary flagset just for
+	// the sake of collecting default values.
+	//
+	// This function should be used like so:
+	//
+	//   var DefaultConfig = *DefaultConfigFromFlags(&Config{}).(*Config)
+
+	fs := flag.NewFlagSet("DefaultConfigFromFlags", flag.PanicOnError)
+
+	if v, ok := cfg.(PrefixedConfigFlags); ok {
+		v.RegisterFlagsWithPrefix("", fs)
+	} else if v, ok := cfg.(ConfigFlags); ok {
+		v.RegisterFlags(fs)
+	} else {
+		panic("config does not implement PrefixedConfigFlags or ConfigFlags")
+	}
+
+	return cfg
+}
+
+// ConfigFlags is an interface that will register flags that can control
+// some object.
+type ConfigFlags interface {
+	RegisterFlags(f *flag.FlagSet)
+}
+
+// PrefixedConfigFlags is an interface that, given a prefix for flags
+// and a flagset, will register flags that can control some object.
+type PrefixedConfigFlags interface {
+	RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet)
+}

production/tanka/grafana-agent/scraping-svc/main.libsonnet

@@ -99,6 +99,8 @@ local policyRule = k.rbac.v1beta1.policyRule;
       syncer.new(this._images.agentctl, this._config),
   },
 
+  withImagesMixin(images):: { _images+: images },
+
   // withConfig overrides the config used for the agent.
   withConfig(config):: { _config: config },