Keep the 1.21 query range and HA failover timeout settings working

CHANGELOG.md

@@ -34,10 +34,11 @@
   - `-ingester.max-series-per-query` (a chunks-storage limit, ignored since blocks storage; use `-querier.max-fetched-series-per-query`)
 * [CHANGE] Ingester: Formally deprecate `-blocks-storage.tsdb.max-exemplars`, scheduled for removal in v1.24.0. Use the per-tenant `max_exemplars` limit instead. The flag still works as the global fallback when `max_exemplars` is 0, but setting it now logs a warning and increments `deprecated_flags_inuse_total`. #7793
 * [CHANGE] Ingester: Graduate native histogram ingestion (`-blocks-storage.tsdb.enable-native-histograms`) from experimental. #7789
-* [CHANGE] Querier: Make query time range configurations per-tenant: `query_ingesters_within`, `query_store_after`, and `shuffle_sharding_ingesters_lookback_period`. Uses `model.Duration` instead of `time.Duration` to support serialization but has minimum unit of 1ms (nanoseconds/microseconds not supported). #7160 #7323
+* [CHANGE] Querier: Make query time range configurations per-tenant: `query_ingesters_within`, `query_store_after`, and `shuffle_sharding_ingesters_lookback_period`. Uses `model.Duration` instead of `time.Duration` to support serialization but has minimum unit of 1ms (nanoseconds/microseconds not supported). The settings move from the `querier` block to `limits`, and their flags from `-querier.*` to `-limits.*`. The old YAML fields and flags still work, but are deprecated and will be removed in v1.24.0: using them logs a warning and increments `deprecated_flags_inuse_total`. As before, a flag on the command line wins over the YAML file, whether it uses the old or the new name. Setting the old and the new name to different values in the same place, both in the YAML file or both on the command line, fails at startup. #7160 #7323 #7919
+* [CHANGE] Querier: `query_ingesters_within` must now be strictly less than `-blocks-storage.tsdb.close-idle-tsdb-timeout` when both are positive. Earlier releases did not check this, and documented the two being equal as valid, so a config that sets them equal now makes Cortex **exit at startup** with `invalid query routing config: tenant default: query_ingesters_within (6h50m0s) must be less than close_idle_tsdb_timeout (6h50m0s)`. The check also runs on every per-tenant override in the runtime config (on the `all`, `distributor`, `querier` and `ruler` targets), so a single tenant override that sets `query_ingesters_within` at or above `close_idle_tsdb_timeout` makes the whole runtime config fail to load: Cortex exits at startup, and on an already-running process every reload fails, pinning the last good overrides and dropping `cortex_runtime_config_last_reload_successful` to 0. Before upgrading, raise `close_idle_tsdb_timeout` above the largest `query_ingesters_within` in your config and runtime overrides, or set it to 0 to disable closing idle TSDBs. The existing checks that `query_store_after` is less than `query_ingesters_within`, and that `shuffle_sharding_ingesters_lookback_period` is at least `query_store_after`, now also apply to per-tenant overrides. #7323
 * [CHANGE] Alertmanager: Remove the obsolete startup migration of local state files into per-tenant directories (scheduled for removal in 1.11.0). Upgrading from a release older than 1.9.0 with a persisted local state directory now requires upgrading to an intermediate release first, so the migration can run. #7513
 * [CHANGE] Cache: Setting `-blocks-storage.bucket-store.metadata-cache.bucket-index-content-ttl` to 0 will disable the bucket-index cache. #7446
-* [CHANGE] HA Tracker: Move `-distributor.ha-tracker.failover-timeout` from a global config to a per-tenant runtime config. The flag name and default value (30s) remain the same. #7481
+* [CHANGE] HA Tracker: Move `-distributor.ha-tracker.failover-timeout` from a global config to a per-tenant runtime config. The flag name and default value (30s) remain the same. The YAML field moves from `distributor.ha_tracker.ha_tracker_failover_timeout` to `limits.ha_tracker_failover_timeout`. The old field still works, but is deprecated and will be removed in v1.24.0: using it logs a warning and increments `deprecated_flags_inuse_total`. As before, the flag on the command line wins over either YAML field. Setting the old and the new YAML field to different values fails at startup. #7481 #7919
 * [FEATURE] Parquet: Support sharded parquet file conversion and querying. #7610
 * [FEATURE] Parquet Converter: Add experimental `-parquet-converter.max-num-columns` flag to automatically shard parquet files when the number of columns exceeds the configured limit. This prevents failures when a TSDB block has more unique label names than the parquet library's column limit (32767). #7624
 * [FEATURE] Distributor: Add experimental `-distributor.num-query-workers` flag to use a goroutine worker pool for query fan-out calls to ingesters. Reuses pre-grown goroutine stacks to eliminate the `runtime.copystack` overhead (~8% CPU) observed on rulers with wide ingester fan-out. Falls back to spawning a new goroutine when no worker is available. #7623

cmd/cortex/main.go

@@ -131,6 +131,8 @@ func main() {
 		return
 	}
 
+	cfg.SetCommandLineFlags(flag.CommandLine)
+
 	// Validate the config once both the config file has been loaded
 	// and CLI flags parsed.
 	err = cfg.Validate(util_log.Logger)
@@ -264,6 +266,11 @@ func LoadConfig(filename string, expandENV bool, cfg *cortex.Config) error {
 		return errors.Wrap(err, "Error parsing config file")
 	}
 
+	err = cfg.SetYAMLKeys(buf)
+	if err != nil {
+		return errors.Wrap(err, "Error parsing config file")
+	}
+
 	err = yaml.UnmarshalStrict(buf, cfg)
 	if err != nil {
 		return errors.Wrap(err, "Error parsing config file")

cmd/cortex/main_test.go

@@ -62,6 +62,12 @@ func TestFlagParsing(t *testing.T) {
 			stdoutMessage: "target: distributor\n",
 		},
 
+		"new flag on the command line overrides the deprecated YAML field, as in 1.21": {
+			yaml:          "distributor:\n  ha_tracker:\n    ha_tracker_failover_timeout: 45s",
+			arguments:     []string{"-distributor.ha-tracker.failover-timeout=30s"},
+			stdoutMessage: "ha_tracker_failover_timeout: 30s\n",
+		},
+
 		"user visible module listing": {
 			arguments:      []string{"-modules"},
 			stdoutMessage:  "ingester *\n",

docs/configuration/config-file-reference.md

@@ -4805,8 +4805,8 @@ The `limits_config` configures default and per-tenant limits imposed by Cortex s
 
 # Maximum lookback duration for querying data from ingesters. Queries for data
 # older than this will only query the long-term storage. This is a per-tenant
-# limit that can be overridden in the runtime configuration. Should be less than
-# or equal to close-idle-tsdb-timeout.
+# limit that can be overridden in the runtime configuration. Must be less than
+# -blocks-storage.tsdb.close-idle-tsdb-timeout when both are positive.
 # CLI flag: -limits.query-ingesters-within
 [query_ingesters_within: <duration> | default = 0s]
 

pkg/cortex/cortex.go

@@ -10,6 +10,7 @@ import (
 	"reflect"
 	"slices"
 	"strings"
+	"time"
 
 	"github.com/go-kit/log"
 	"github.com/go-kit/log/level"
@@ -100,6 +101,8 @@ type Config struct {
 	HTTPPrefix           string                 `yaml:"http_prefix"`
 	NameValidationScheme model.ValidationScheme `yaml:"name_validation_scheme"`
 	yamlRootNodeStates   map[string]yamlRootNodeState
+	yamlKeys             map[string]bool // dotted paths of the keys set in the YAML file
+	cliFlags             map[string]bool // names of the flags set on the command line
 
 	ResourceMonitor configs.ResourceMonitor `yaml:"resource_monitor"`
 
@@ -170,6 +173,41 @@ func (c *Config) SetYAMLRootNodeStates(buf []byte) error {
 	return nil
 }
 
+// SetYAMLKeys records the dotted path of every key set in the YAML config file, so that
+// validation can tell a setting the file sets from one left at its default.
+func (c *Config) SetYAMLKeys(buf []byte) error {
+	var root map[any]any
+	if err := yaml.Unmarshal(buf, &root); err != nil {
+		return err
+	}
+
+	c.yamlKeys = map[string]bool{}
+	var walk func(prefix string, node map[any]any)
+	walk = func(prefix string, node map[any]any) {
+		for key, value := range node {
+			name, ok := key.(string)
+			if !ok {
+				continue
+			}
+			c.yamlKeys[prefix+name] = true
+			if child, ok := value.(map[any]any); ok {
+				walk(prefix+name+".", child)
+			}
+		}
+	}
+	walk("", root)
+	return nil
+}
+
+// SetCommandLineFlags records the flags set on the command line, so that validation can give
+// them precedence over the YAML config file. Call it after fs has been parsed.
+func (c *Config) SetCommandLineFlags(fs *flag.FlagSet) {
+	c.cliFlags = map[string]bool{}
+	fs.Visit(func(f *flag.Flag) {
+		c.cliFlags[f.Name] = true
+	})
+}
+
 func isEmptyYAMLRootNode(value any) bool {
 	if value == nil {
 		return true
@@ -242,6 +280,10 @@ func (c *Config) Validate(log log.Logger) error {
 		return err
 	}
 
+	if err := c.applyDeprecatedLimits(); err != nil {
+		return err
+	}
+
 	switch c.NameValidationScheme {
 	case model.LegacyValidation, model.UTF8Validation:
 	default:
@@ -329,6 +371,75 @@ func (c *Config) isModuleEnabled(m string) bool {
 	return slices.Contains(c.Target, m)
 }
 
+type deprecatedLimit struct {
+	oldName, newName string // YAML paths
+	oldFlag, newFlag string // CLI flag names; oldFlag is empty when the flag name did not change
+	old              time.Duration
+	limit            *model.Duration
+	def              model.Duration
+}
+
+// deprecatedLimits lists the settings that moved into the per-tenant limits in 1.22, with
+// their old location. The old one is kept so that 1.21 configs keep working.
+func (c *Config) deprecatedLimits() []deprecatedLimit {
+	defaults := validation.Limits{}
+	flagext.DefaultValues(&defaults)
+
+	return []deprecatedLimit{
+		{"querier.query_ingesters_within", "limits.query_ingesters_within", "querier.query-ingesters-within", "limits.query-ingesters-within", c.Querier.DeprecatedQueryIngestersWithin, &c.LimitsConfig.QueryIngestersWithin, defaults.QueryIngestersWithin},
+		{"querier.query_store_after", "limits.query_store_after", "querier.query-store-after", "limits.query-store-after", c.Querier.DeprecatedQueryStoreAfter, &c.LimitsConfig.QueryStoreAfter, defaults.QueryStoreAfter},
+		{"querier.shuffle_sharding_ingesters_lookback_period", "limits.shuffle_sharding_ingesters_lookback_period", "querier.shuffle-sharding-ingesters-lookback-period", "limits.shuffle-sharding-ingesters-lookback-period", c.Querier.DeprecatedShuffleShardingIngestersLookbackPeriod, &c.LimitsConfig.ShuffleShardingIngestersLookbackPeriod, defaults.ShuffleShardingIngestersLookbackPeriod},
+		{"distributor.ha_tracker.ha_tracker_failover_timeout", "limits.ha_tracker_failover_timeout", "", "distributor.ha-tracker.failover-timeout", c.Distributor.HATrackerConfig.DeprecatedFailoverTimeout, &c.LimitsConfig.HATrackerFailoverTimeout, defaults.HATrackerFailoverTimeout},
+	}
+}
+
+// sources reports where the deprecated and the new setting were set. Without the sources
+// recorded by SetYAMLKeys and SetCommandLineFlags, as when the config is built in code, a
+// setting counts as set when it is not at its default value.
+func (c *Config) sources(d deprecatedLimit) (oldCLI, oldYAML, newCLI, newYAML bool) {
+	if c.yamlKeys == nil && c.cliFlags == nil {
+		return false, d.old != 0, false, *d.limit != d.def
+	}
+	return d.oldFlag != "" && c.cliFlags[d.oldFlag], c.yamlKeys[d.oldName], c.cliFlags[d.newFlag], c.yamlKeys[d.newName]
+}
+
+// applyDeprecatedLimits copies each deprecated setting that is set into its limit, with the
+// precedence that CLI flags have over the YAML file: a setting on the command line wins over
+// one in the file, whichever name each uses. Within one source, the old and the new name
+// must agree. It must run before the limits are validated and before they become the
+// defaults for the runtime config overrides.
+func (c *Config) applyDeprecatedLimits() error {
+	for _, d := range c.deprecatedLimits() {
+		oldCLI, oldYAML, newCLI, newYAML := c.sources(d)
+
+		switch {
+		case oldCLI && newCLI, !oldCLI && !newCLI && oldYAML && newYAML:
+			if time.Duration(*d.limit) != d.old {
+				oldName, newName := d.oldName, d.newName
+				if oldCLI {
+					oldName, newName = "-"+d.oldFlag, "-"+d.newFlag
+				}
+				return fmt.Errorf("%s (%s) and the deprecated %s (%s) are both set: remove %s", newName, time.Duration(*d.limit), oldName, d.old, oldName)
+			}
+		case newCLI:
+		case oldCLI, oldYAML && !newYAML:
+			*d.limit = model.Duration(d.old)
+		}
+	}
+	return nil
+}
+
+// warnDeprecatedLimits logs each deprecated setting that is set. It is separate from
+// applyDeprecatedLimits because Validate runs before the logger is initialised.
+func (c *Config) warnDeprecatedLimits(logger log.Logger) {
+	for _, d := range c.deprecatedLimits() {
+		if oldCLI, oldYAML, _, _ := c.sources(d); oldCLI || oldYAML {
+			flagext.DeprecatedFlagsUsed.Inc()
+			level.Warn(logger).Log("msg", "running with DEPRECATED config "+d.oldName+", use "+d.newName+" instead, it will be removed in v1.24.0")
+		}
+	}
+}
+
 // validateYAMLEmptyNodes ensure that no empty node has been specified in the YAML config file.
 // When an empty node is defined in YAML, the YAML parser sets the whole struct to its zero value
 // and so we loose all default values. It's very difficult to detect this case for the user, so we
@@ -442,6 +553,8 @@ func New(cfg Config) (*Cortex, error) {
 		os.Exit(0)
 	}
 
+	cfg.warnDeprecatedLimits(util_log.Logger)
+
 	// Swap out the default resolver to support multiple tenant IDs separated by a '|'
 	if cfg.TenantFederation.Enabled {
 		users.WithDefaultResolver(users.NewMultiResolver())

pkg/cortex/cortex_test.go

@@ -21,6 +21,7 @@ import (
 	"go.uber.org/atomic"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/credentials/insecure"
+	"gopkg.in/yaml.v2"
 
 	"github.com/cortexproject/cortex/pkg/alertmanager"
 	"github.com/cortexproject/cortex/pkg/alertmanager/alertstore"
@@ -304,6 +305,136 @@ func TestConfigValidation(t *testing.T) {
 	}
 }
 
+func TestConfigValidation_DeprecatedLimits(t *testing.T) {
+	// The CLI flags and YAML fields that moved into the limits in 1.22 must keep working.
+	for _, tc := range []struct {
+		name          string
+		args          []string
+		yaml          string
+		expectedError string
+		check         func(t *testing.T, cfg *Config)
+	}{
+		{
+			name: "old querier YAML fields set the default limits",
+			yaml: "querier:\n  query_ingesters_within: 13h\n  query_store_after: 12h\n  shuffle_sharding_ingesters_lookback_period: 13h\n",
+			check: func(t *testing.T, cfg *Config) {
+				assert.Equal(t, model.Duration(13*time.Hour), cfg.LimitsConfig.QueryIngestersWithin)
+				assert.Equal(t, model.Duration(12*time.Hour), cfg.LimitsConfig.QueryStoreAfter)
+				assert.Equal(t, model.Duration(13*time.Hour), cfg.LimitsConfig.ShuffleShardingIngestersLookbackPeriod)
+			},
+		},
+		{
+			name: "old querier CLI flags set the default limits",
+			args: []string{"-querier.query-ingesters-within=13h", "-querier.query-store-after=12h", "-querier.shuffle-sharding-ingesters-lookback-period=13h"},
+			check: func(t *testing.T, cfg *Config) {
+				assert.Equal(t, model.Duration(13*time.Hour), cfg.LimitsConfig.QueryIngestersWithin)
+				assert.Equal(t, model.Duration(12*time.Hour), cfg.LimitsConfig.QueryStoreAfter)
+				assert.Equal(t, model.Duration(13*time.Hour), cfg.LimitsConfig.ShuffleShardingIngestersLookbackPeriod)
+			},
+		},
+		{
+			name: "old HA tracker YAML field sets the default limit",
+			yaml: "distributor:\n  ha_tracker:\n    ha_tracker_failover_timeout: 45s\n",
+			check: func(t *testing.T, cfg *Config) {
+				assert.Equal(t, model.Duration(45*time.Second), cfg.LimitsConfig.HATrackerFailoverTimeout)
+			},
+		},
+		{
+			name: "old and new set to the same value",
+			yaml: "querier:\n  query_store_after: 12h\nlimits:\n  query_store_after: 12h\n",
+			check: func(t *testing.T, cfg *Config) {
+				assert.Equal(t, model.Duration(12*time.Hour), cfg.LimitsConfig.QueryStoreAfter)
+			},
+		},
+		{
+			name:          "old and new set to different values",
+			yaml:          "querier:\n  query_store_after: 12h\nlimits:\n  query_store_after: 6h\n",
+			expectedError: "limits.query_store_after (6h0m0s) and the deprecated querier.query_store_after (12h0m0s) are both set: remove querier.query_store_after",
+		},
+		{
+			name:          "old HA tracker YAML field and the new one set to different values",
+			yaml:          "distributor:\n  ha_tracker:\n    ha_tracker_failover_timeout: 45s\nlimits:\n  ha_tracker_failover_timeout: 1m\n",
+			expectedError: "limits.ha_tracker_failover_timeout (1m0s) and the deprecated distributor.ha_tracker.ha_tracker_failover_timeout (45s) are both set: remove distributor.ha_tracker.ha_tracker_failover_timeout",
+		},
+		{
+			name: "a new flag on the command line wins over the old YAML field, even at its default value",
+			args: []string{"-distributor.ha-tracker.failover-timeout=30s"},
+			yaml: "distributor:\n  ha_tracker:\n    ha_tracker_failover_timeout: 45s\n",
+			check: func(t *testing.T, cfg *Config) {
+				assert.Equal(t, model.Duration(30*time.Second), cfg.LimitsConfig.HATrackerFailoverTimeout)
+			},
+		},
+		{
+			name: "a new querier flag on the command line wins over the old YAML field, even at its default value",
+			args: []string{"-limits.query-store-after=0"},
+			yaml: "querier:\n  query_store_after: 12h\n",
+			check: func(t *testing.T, cfg *Config) {
+				assert.Equal(t, model.Duration(0), cfg.LimitsConfig.QueryStoreAfter)
+			},
+		},
+		{
+			name: "an old flag on the command line wins over the new YAML field",
+			args: []string{"-querier.query-store-after=12h"},
+			yaml: "limits:\n  query_ingesters_within: 13h\n  query_store_after: 6h\n",
+			check: func(t *testing.T, cfg *Config) {
+				assert.Equal(t, model.Duration(12*time.Hour), cfg.LimitsConfig.QueryStoreAfter)
+			},
+		},
+		{
+			name:          "old and new flags on the command line set to different values",
+			args:          []string{"-querier.query-store-after=12h", "-limits.query-store-after=6h"},
+			expectedError: "-limits.query-store-after (6h0m0s) and the deprecated -querier.query-store-after (12h0m0s) are both set: remove -querier.query-store-after",
+		},
+		{
+			name: "old HA tracker YAML field and the new one set to the same value",
+			yaml: "distributor:\n  ha_tracker:\n    ha_tracker_failover_timeout: 45s\nlimits:\n  ha_tracker_failover_timeout: 45s\n",
+			check: func(t *testing.T, cfg *Config) {
+				assert.Equal(t, model.Duration(45*time.Second), cfg.LimitsConfig.HATrackerFailoverTimeout)
+			},
+		},
+		{
+			name:          "old fields are validated like the new ones",
+			yaml:          "querier:\n  query_ingesters_within: 12h\n  query_store_after: 12h\n",
+			expectedError: "tenant default: query_store_after (12h0m0s) must be less than query_ingesters_within (12h0m0s)",
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			// Load the config the way cmd/cortex does: defaults, then the YAML file, then the
+			// command line on top.
+			cfg := &Config{}
+			fs := flag.NewFlagSet("", flag.PanicOnError)
+			cfg.RegisterFlags(fs)
+			require.NoError(t, cfg.SetYAMLKeys([]byte(tc.yaml)))
+			require.NoError(t, yaml.UnmarshalStrict([]byte(tc.yaml), cfg))
+			require.NoError(t, fs.Parse(tc.args))
+			cfg.SetCommandLineFlags(fs)
+
+			err := cfg.Validate(nil)
+			if tc.expectedError != "" {
+				require.ErrorContains(t, err, tc.expectedError)
+				return
+			}
+			require.NoError(t, err)
+			tc.check(t, cfg)
+		})
+	}
+}
+
+func TestConfigValidation_DeprecatedLimitsWithoutSources(t *testing.T) {
+	// A config built in code has no recorded sources, so a setting counts as set when it is
+	// not at its default value.
+	cfg := newDefaultConfig()
+	cfg.Querier.DeprecatedQueryStoreAfter = 12 * time.Hour
+	cfg.LimitsConfig.QueryIngestersWithin = model.Duration(13 * time.Hour)
+	require.NoError(t, cfg.Validate(nil))
+	assert.Equal(t, model.Duration(12*time.Hour), cfg.LimitsConfig.QueryStoreAfter)
+
+	cfg = newDefaultConfig()
+	cfg.Querier.DeprecatedQueryStoreAfter = 12 * time.Hour
+	cfg.LimitsConfig.QueryStoreAfter = model.Duration(6 * time.Hour)
+	require.ErrorContains(t, cfg.Validate(nil), "are both set: remove querier.query_store_after")
+}
+
 func TestGrpcAuthMiddleware(t *testing.T) {
 	prepareGlobalMetricsRegistry(t)