Change a few default config values and improve application of common storage config

CHANGELOG.md

@@ -11,6 +11,7 @@
 * [4453](https://github.com/grafana/loki/pull/4453) **liguozhong**: Loki: Implement retry to s3 chunk storage
 * [4542](https://github.com/grafana/loki/pull/4542) **owen-d**: Introduce the `loki_overrides_defaults` metric and only export diffs for tenant limits.
 * [4498](https://github.com/grafana/loki/pull/4498) **trevorwhitney**: Feature: add virtual read and write targets
+* [4543](https://github.com/grafana/loki/pull/4543) **trevorwhitney**: Change more default values and improve application of common storage config
 
 # 2.3.0 (2021/08/06)
 

docs/sources/upgrading/_index.md

@@ -19,6 +19,29 @@ If possible try to stay current and do sequential updates. If you want to skip v
 
 ### Loki
 
+#### Ingester WAL now defaults to on, and chunk transfers are disabled by default
+
+* [4543](https://github.com/grafana/loki/pull/4543) **trevorwhitney**: Change more default values and improve application of common storage config
+
+This changes a few default values, resulting in the ingester WAL now being on by default,
+and chunk transfer retries are disabled by default. Note, this now means Loki will depend on local disk by default for it's WAL (write ahead log) directory. This defaults to `wal` but can be overridden via the `--ingester.wal-dir` or via `path_prefix` in the common configuration section. Below are config snippets with the previous defaults, and another with the new values.
+
+Previous defaults:
+```yaml
+ingester:
+  max_transfer_retries: 10
+  wal:
+    enabled: false
+```
+
+New defaults:
+```yaml
+ingester:
+  max_transfer_retries: 0
+  wal:
+    enabled: true
+```
+
 #### Distributor now stores ring in memory by default instead of Consul
 
 PR [4440](https://github.com/grafana/loki/pull/4440) **DylanGuedes**: Config: Override distributor's default ring KV store

pkg/ingester/flush_test.go

@@ -290,6 +290,7 @@ func defaultIngesterTestConfig(t testing.TB) Config {
 	cfg.LifecyclerConfig.MinReadyDuration = 0
 	cfg.BlockSize = 256 * 1024
 	cfg.TargetChunkSize = 1500 * 1024
+	cfg.WAL.Enabled = false
 	return cfg
 }
 

pkg/ingester/ingester.go

@@ -90,7 +90,7 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) {
 	cfg.LifecyclerConfig.RegisterFlags(f)
 	cfg.WAL.RegisterFlags(f)
 
-	f.IntVar(&cfg.MaxTransferRetries, "ingester.max-transfer-retries", 10, "Number of times to try and transfer chunks before falling back to flushing. If set to 0 or negative value, transfers are disabled.")
+	f.IntVar(&cfg.MaxTransferRetries, "ingester.max-transfer-retries", 0, "Number of times to try and transfer chunks before falling back to flushing. If set to 0 or negative value, transfers are disabled.")
 	f.IntVar(&cfg.ConcurrentFlushes, "ingester.concurrent-flushes", 16, "")
 	f.DurationVar(&cfg.FlushCheckPeriod, "ingester.flush-check-period", 30*time.Second, "")
 	f.DurationVar(&cfg.FlushOpTimeout, "ingester.flush-op-timeout", 10*time.Second, "")

pkg/ingester/wal.go

@@ -41,7 +41,7 @@ func (cfg *WALConfig) Validate() error {
 // RegisterFlags adds the flags required to config this to the given FlagSet
 func (cfg *WALConfig) RegisterFlags(f *flag.FlagSet) {
 	f.StringVar(&cfg.Dir, "ingester.wal-dir", "wal", "Directory to store the WAL and/or recover from WAL.")
-	f.BoolVar(&cfg.Enabled, "ingester.wal-enabled", false, "Enable writing of ingested data into WAL.")
+	f.BoolVar(&cfg.Enabled, "ingester.wal-enabled", true, "Enable writing of ingested data into WAL.")
 	f.DurationVar(&cfg.CheckpointDuration, "ingester.checkpoint-duration", 5*time.Minute, "Interval at which checkpoints should be created.")
 	f.BoolVar(&cfg.FlushOnShutdown, "ingester.flush-on-shutdown", false, "When WAL is enabled, should chunks be flushed to long-term storage on shutdown.")
 

pkg/loki/common/common.go

@@ -1,6 +1,8 @@
 package common
 
 import (
+	"flag"
+
 	"github.com/grafana/loki/pkg/storage/chunk/aws"
 	"github.com/grafana/loki/pkg/storage/chunk/azure"
 	"github.com/grafana/loki/pkg/storage/chunk/gcp"
@@ -14,10 +16,22 @@ type Config struct {
 	Storage    Storage `yaml:"storage"`
 }
 
+func (c *Config) RegisterFlags(f *flag.FlagSet) {
+	c.Storage.RegisterFlagsWithPrefix("common.storage", f)
+}
+
 type Storage struct {
-	S3       *aws.S3Config            `yaml:"s3"`
-	GCS      *gcp.GCSConfig           `yaml:"gcs"`
-	Azure    *azure.BlobStorageConfig `yaml:"azure"`
-	Swift    *openstack.SwiftConfig   `yaml:"swift"`
-	FSConfig *local.FSConfig          `yaml:"filesystem"`
+	S3       aws.S3Config            `yaml:"s3"`
+	GCS      gcp.GCSConfig           `yaml:"gcs"`
+	Azure    azure.BlobStorageConfig `yaml:"azure"`
+	Swift    openstack.SwiftConfig   `yaml:"swift"`
+	FSConfig local.FSConfig          `yaml:"filesystem"`
+}
+
+func (s *Storage) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) {
+	s.S3.RegisterFlagsWithPrefix(prefix+".s3", f)
+	s.GCS.RegisterFlagsWithPrefix(prefix+".gcs", f)
+	s.Azure.RegisterFlagsWithPrefix(prefix+".azure", f)
+	s.Swift.RegisterFlagsWithPrefix(prefix+".swift", f)
+	s.FSConfig.RegisterFlagsWithPrefix(prefix+".filesystem", f)
 }

pkg/loki/config_wrapper.go

@@ -4,6 +4,7 @@ import (
 	"flag"
 	"fmt"
 	"reflect"
+	"strings"
 
 	cortexcache "github.com/cortexproject/cortex/pkg/chunk/cache"
 	"github.com/grafana/dskit/flagext"
@@ -66,17 +67,6 @@ func (c *ConfigWrapper) ApplyDynamicConfig() cfg.Source {
 			return errors.New("dst is not a Loki ConfigWrapper")
 		}
 
-		// Apply all our custom logic here to set values in the Loki config from values in the common config
-		if r.Common.PathPrefix != "" {
-			if r.Ruler.RulePath == defaults.Ruler.RulePath {
-				r.Ruler.RulePath = fmt.Sprintf("%s/rules", r.Common.PathPrefix)
-			}
-
-			if r.Ingester.WAL.Dir == defaults.Ingester.WAL.Dir {
-				r.Ingester.WAL.Dir = fmt.Sprintf("%s/wal", r.Common.PathPrefix)
-			}
-		}
-
 		// If nobody has defined any frontend address or scheduler address
 		// we can default to using the query scheduler ring for scheduler discovery.
 		if r.Worker.FrontendAddress == "" &&
@@ -86,6 +76,7 @@ func (c *ConfigWrapper) ApplyDynamicConfig() cfg.Source {
 		}
 
 		applyIngesterRingConfig(r)
+		applyPathPrefixDefaults(r, defaults)
 		applyMemberlistConfig(r)
 
 		if err := applyStorageConfig(r, &defaults); err != nil {
@@ -149,6 +140,24 @@ func applyIngesterRingConfig(r *ConfigWrapper) {
 	r.QueryScheduler.SchedulerRing.KVStore.StoreConfig = sc
 }
 
+func applyPathPrefixDefaults(r *ConfigWrapper, defaults ConfigWrapper) {
+	if r.Common.PathPrefix != "" {
+		prefix := strings.TrimSuffix(r.Common.PathPrefix, "/")
+
+		if r.Ruler.RulePath == defaults.Ruler.RulePath {
+			r.Ruler.RulePath = fmt.Sprintf("%s/rules", prefix)
+		}
+
+		if r.Ingester.WAL.Dir == defaults.Ingester.WAL.Dir {
+			r.Ingester.WAL.Dir = fmt.Sprintf("%s/wal", prefix)
+		}
+
+		if r.CompactorConfig.WorkingDirectory == defaults.CompactorConfig.WorkingDirectory {
+			r.CompactorConfig.WorkingDirectory = fmt.Sprintf("%s/compactor", prefix)
+		}
+	}
+}
+
 // applyMemberlistConfig will change the default ingester, distributor, ruler, and query scheduler ring configurations to use memberlist
 // if the -memberlist.join_members config is provided. The idea here is that if a user explicitly configured the
 // memberlist configuration section, they probably want to be using memberlist for all their ring configurations.
@@ -171,78 +180,62 @@ var ErrTooManyStorageConfigs = errors.New("too many storage configs provided in
 // configuration file, applyStorageConfig will not override them.
 // If multiple storage configurations are provided, applyStorageConfig will return an error
 func applyStorageConfig(cfg, defaults *ConfigWrapper) error {
-	var applyRulerStoreConfig func(*ConfigWrapper)
-	var applyChunkStorageConfig func(*ConfigWrapper)
+	var applyConfig func(*ConfigWrapper)
 
 	//only one config is allowed
 	configsFound := 0
 
-	if cfg.Common.Storage.Azure != nil {
+	if !reflect.DeepEqual(cfg.Common.Storage.Azure, defaults.StorageConfig.AzureStorageConfig) {
 		configsFound++
 
-		applyRulerStoreConfig = func(r *ConfigWrapper) {
+		applyConfig = func(r *ConfigWrapper) {
 			r.Ruler.StoreConfig.Type = "azure"
 			r.Ruler.StoreConfig.Azure = r.Common.Storage.Azure.ToCortexAzureConfig()
-		}
-
-		applyChunkStorageConfig = func(r *ConfigWrapper) {
-			r.StorageConfig.AzureStorageConfig = *r.Common.Storage.Azure
+			r.StorageConfig.AzureStorageConfig = r.Common.Storage.Azure
 			r.CompactorConfig.SharedStoreType = chunk_storage.StorageTypeAzure
 		}
 	}
 
-	if cfg.Common.Storage.FSConfig != nil {
+	if !reflect.DeepEqual(cfg.Common.Storage.FSConfig, defaults.StorageConfig.FSConfig) {
 		configsFound++
 
-		applyRulerStoreConfig = func(r *ConfigWrapper) {
+		applyConfig = func(r *ConfigWrapper) {
 			r.Ruler.StoreConfig.Type = "local"
 			r.Ruler.StoreConfig.Local = r.Common.Storage.FSConfig.ToCortexLocalConfig()
-		}
-
-		applyChunkStorageConfig = func(r *ConfigWrapper) {
-			r.StorageConfig.FSConfig = *r.Common.Storage.FSConfig
+			r.StorageConfig.FSConfig = r.Common.Storage.FSConfig
 			r.CompactorConfig.SharedStoreType = chunk_storage.StorageTypeFileSystem
 		}
 	}
 
-	if cfg.Common.Storage.GCS != nil {
+	if !reflect.DeepEqual(cfg.Common.Storage.GCS, defaults.StorageConfig.GCSConfig) {
 		configsFound++
 
-		applyRulerStoreConfig = func(r *ConfigWrapper) {
+		applyConfig = func(r *ConfigWrapper) {
 			r.Ruler.StoreConfig.Type = "gcs"
 			r.Ruler.StoreConfig.GCS = r.Common.Storage.GCS.ToCortexGCSConfig()
-		}
-
-		applyChunkStorageConfig = func(r *ConfigWrapper) {
-			r.StorageConfig.GCSConfig = *r.Common.Storage.GCS
+			r.StorageConfig.GCSConfig = r.Common.Storage.GCS
 			r.CompactorConfig.SharedStoreType = chunk_storage.StorageTypeGCS
 		}
 	}
 
-	if cfg.Common.Storage.S3 != nil {
+	if !reflect.DeepEqual(cfg.Common.Storage.S3, defaults.StorageConfig.AWSStorageConfig.S3Config) {
 		configsFound++
 
-		applyRulerStoreConfig = func(r *ConfigWrapper) {
+		applyConfig = func(r *ConfigWrapper) {
 			r.Ruler.StoreConfig.Type = "s3"
 			r.Ruler.StoreConfig.S3 = r.Common.Storage.S3.ToCortexS3Config()
-		}
-
-		applyChunkStorageConfig = func(r *ConfigWrapper) {
-			r.StorageConfig.AWSStorageConfig.S3Config = *r.Common.Storage.S3
+			r.StorageConfig.AWSStorageConfig.S3Config = r.Common.Storage.S3
 			r.CompactorConfig.SharedStoreType = chunk_storage.StorageTypeS3
 		}
 	}
 
-	if cfg.Common.Storage.Swift != nil {
+	if !reflect.DeepEqual(cfg.Common.Storage.Swift, defaults.StorageConfig.Swift) {
 		configsFound++
 
-		applyRulerStoreConfig = func(r *ConfigWrapper) {
+		applyConfig = func(r *ConfigWrapper) {
 			r.Ruler.StoreConfig.Type = "swift"
 			r.Ruler.StoreConfig.Swift = r.Common.Storage.Swift.ToCortexSwiftConfig()
-		}
-
-		applyChunkStorageConfig = func(r *ConfigWrapper) {
-			r.StorageConfig.Swift = *r.Common.Storage.Swift
+			r.StorageConfig.Swift = r.Common.Storage.Swift
 			r.CompactorConfig.SharedStoreType = chunk_storage.StorageTypeSwift
 		}
 	}
@@ -251,22 +244,11 @@ func applyStorageConfig(cfg, defaults *ConfigWrapper) error {
 		return ErrTooManyStorageConfigs
 	}
 
-	applyRulerStoreConfigs(cfg, defaults, applyRulerStoreConfig)
-	applyChunkStorageConfigs(cfg, defaults, applyChunkStorageConfig)
-
-	return nil
-}
-
-func applyRulerStoreConfigs(cfg, defaults *ConfigWrapper, apply func(*ConfigWrapper)) {
-	if apply != nil && reflect.DeepEqual(cfg.Ruler.StoreConfig, defaults.Ruler.StoreConfig) {
-		apply(cfg)
+	if applyConfig != nil {
+		applyConfig(cfg)
 	}
-}
 
-func applyChunkStorageConfigs(cfg, defaults *ConfigWrapper, apply func(*ConfigWrapper)) {
-	if apply != nil && reflect.DeepEqual(cfg.StorageConfig, defaults.StorageConfig) {
-		apply(cfg)
-	}
+	return nil
 }
 
 func betterBoltdbShipperDefaults(cfg, defaults *ConfigWrapper) {
@@ -280,6 +262,18 @@ func betterBoltdbShipperDefaults(cfg, defaults *ConfigWrapper) {
 	if cfg.CompactorConfig.SharedStoreType == defaults.CompactorConfig.SharedStoreType {
 		cfg.CompactorConfig.SharedStoreType = currentSchema.ObjectType
 	}
+
+	if cfg.Common.PathPrefix != "" {
+		prefix := strings.TrimSuffix(cfg.Common.PathPrefix, "/")
+
+		if cfg.StorageConfig.BoltDBShipperConfig.ActiveIndexDirectory == "" {
+			cfg.StorageConfig.BoltDBShipperConfig.ActiveIndexDirectory = fmt.Sprintf("%s/boltdb-shipper-active", prefix)
+		}
+
+		if cfg.StorageConfig.BoltDBShipperConfig.CacheLocation == "" {
+			cfg.StorageConfig.BoltDBShipperConfig.CacheLocation = fmt.Sprintf("%s/boltdb-shipper-cache", prefix)
+		}
+	}
 }
 
 // applyFIFOCacheConfig turns on FIFO cache for the chunk store and for the query range results,