From 7e29ec1c19ebc05d919b147a3d371c3efa7d4fee Mon Sep 17 00:00:00 2001 From: Marco Pracucci Date: Fri, 27 Dec 2019 14:37:53 +0100 Subject: [PATCH] Changed the default value for -distributor.ha-tracker.prefix in order to not clash with other keys Signed-off-by: Marco Pracucci --- CHANGELOG.md | 1 + pkg/distributor/distributor_ring.go | 2 +- pkg/distributor/ha_tracker.go | 8 ++++++-- pkg/distributor/ha_tracker_test.go | 11 +++++++++++ pkg/ring/kv/client.go | 18 +++++++++--------- pkg/ring/ring.go | 2 +- 6 files changed, 29 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 84467d7c45c..a3db8e89571 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ * [CHANGE] Deprecated `-distributor.limiter-reload-period` flag. #1766 * [CHANGE] Ingesters now write only normalised tokens to the ring, although they can still read denormalised tokens used by other ingesters. `-ingester.normalise-tokens` is now deprecated, and ignored. If you want to switch back to using denormalised tokens, you need to downgrade to Cortex 0.4.0. Previous versions don't handle claiming tokens from normalised ingesters correctly. #1809 * [CHANGE] Overrides mechanism has been renamed to "runtime config", and is now separate from limits. Runtime config is simply a file that is reloaded by Cortex every couple of seconds. Limits and now also multi KV use this mechanism.
New arguments were introduced: `-runtime-config.file` (defaults to empty) and `-runtime-config.reload-period` (defaults to 10 seconds), which replace previously used `-limits.per-user-override-config` and `-limits.per-user-override-period` options. Old options are still used if `-runtime-config.file` is not specified. This change is also reflected in YAML configuration, where old `limits.per_tenant_override_config` and `limits.per_tenant_override_period` fields are replaced with `runtime_config.file` and `runtime_config.period` respectively. #1749 +* [CHANGE] Changed the default value for `-distributor.ha-tracker.prefix` from `collectors/` to `ha-tracker/` in order to not clash with other keys (ie. ring) stored in the same key-value store. #1940 * [FEATURE] The distributor can now drop labels from samples (similar to the removal of the replica label for HA ingestion) per user via the `distributor.drop-label` flag. #1726 * [FEATURE] Added `global` ingestion rate limiter strategy. Deprecated `-distributor.limiter-reload-period` flag. #1766 * [FEATURE] Added support for Microsoft Azure blob storage to be used for storing chunk data. #1913 diff --git a/pkg/distributor/distributor_ring.go b/pkg/distributor/distributor_ring.go index 673da46e7e3..5e3e35d0941 100644 --- a/pkg/distributor/distributor_ring.go +++ b/pkg/distributor/distributor_ring.go @@ -40,7 +40,7 @@ func (cfg *RingConfig) RegisterFlags(f *flag.FlagSet) { } // Ring flags - cfg.KVStore.RegisterFlagsWithPrefix("distributor.ring.", f) + cfg.KVStore.RegisterFlagsWithPrefix("distributor.ring.", "collectors/", f) f.DurationVar(&cfg.HeartbeatPeriod, "distributor.ring.heartbeat-period", 5*time.Second, "Period at which to heartbeat to the ring.") f.DurationVar(&cfg.HeartbeatTimeout, "distributor.ring.heartbeat-timeout", time.Minute, "The heartbeat timeout after which distributors are considered unhealthy within the ring.") diff --git a/pkg/distributor/ha_tracker.go b/pkg/distributor/ha_tracker.go index cba42bab8f7..6130c819ebe 100644 --- a/pkg/distributor/ha_tracker.go +++ b/pkg/distributor/ha_tracker.go @@ -115,8 +115,12 @@ func (cfg *HATrackerConfig) RegisterFlags(f *flag.FlagSet) { "distributor.ha-tracker.failover-timeout", 30*time.Second, "If we don't receive any samples from the accepted replica for a cluster in this amount of time we will failover to the next replica we receive a sample from. This value must be greater than the update timeout") - // We want the ability to use different Consul instances for the ring and for HA cluster tracking. - cfg.KVStore.RegisterFlagsWithPrefix("distributor.ha-tracker.", f) + + // We want the ability to use different Consul instances for the ring and + // for HA cluster tracking. We also customize the default keys prefix, in + // order to not clash with the ring key if they both share the same KVStore + // backend (ie. run on the same consul cluster). + cfg.KVStore.RegisterFlagsWithPrefix("distributor.ha-tracker.", "ha-tracker/", f) } // Validate config and returns error on failure diff --git a/pkg/distributor/ha_tracker_test.go b/pkg/distributor/ha_tracker_test.go index c012ae5fb2a..48e49dfc822 100644 --- a/pkg/distributor/ha_tracker_test.go +++ b/pkg/distributor/ha_tracker_test.go @@ -13,6 +13,7 @@ import ( "github.com/weaveworks/common/user" "github.com/cortexproject/cortex/pkg/ingester/client" + "github.com/cortexproject/cortex/pkg/ring" "github.com/cortexproject/cortex/pkg/ring/kv" "github.com/cortexproject/cortex/pkg/ring/kv/codec" "github.com/cortexproject/cortex/pkg/ring/kv/consul" @@ -492,3 +493,13 @@ func TestFindHALabels(t *testing.T) { assert.Equal(t, c.expected.replica, replica) } } + +func TestHATrackerConfig_ShouldCustomizePrefixDefaultValue(t *testing.T) { + haConfig := HATrackerConfig{} + ringConfig := ring.Config{} + flagext.DefaultValues(&haConfig) + flagext.DefaultValues(&ringConfig) + + assert.Equal(t, "ha-tracker/", haConfig.KVStore.Prefix) + assert.NotEqual(t, haConfig.KVStore.Prefix, ringConfig.KVStore.Prefix) +} diff --git a/pkg/ring/kv/client.go b/pkg/ring/kv/client.go index b31bbc99832..8a74e72c531 100644 --- a/pkg/ring/kv/client.go +++ b/pkg/ring/kv/client.go @@ -45,21 +45,21 @@ type Config struct { // store flag with the prefix ring, so ring.store. For everything else we pass the prefix // to the Consul flags. // If prefix is not an empty string it should end with a period. -func (cfg *Config) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) { +func (cfg *Config) RegisterFlagsWithPrefix(flagsPrefix, defaultPrefix string, f *flag.FlagSet) { // We need Consul flags to not have the ring prefix to maintain compatibility. // This needs to be fixed in the future (1.0 release maybe?) when we normalize flags. // At the moment we have consul., and ring.store, going forward it would // be easier to have everything under ring, so ring.consul. - cfg.Consul.RegisterFlags(f, prefix) - cfg.Etcd.RegisterFlagsWithPrefix(f, prefix) - cfg.Multi.RegisterFlagsWithPrefix(f, prefix) - cfg.Memberlist.RegisterFlags(f, prefix) + cfg.Consul.RegisterFlags(f, flagsPrefix) + cfg.Etcd.RegisterFlagsWithPrefix(f, flagsPrefix) + cfg.Multi.RegisterFlagsWithPrefix(f, flagsPrefix) + cfg.Memberlist.RegisterFlags(f, flagsPrefix) - if prefix == "" { - prefix = "ring." + if flagsPrefix == "" { + flagsPrefix = "ring." } - f.StringVar(&cfg.Prefix, prefix+"prefix", "collectors/", "The prefix for the keys in the store. Should end with a /.") - f.StringVar(&cfg.Store, prefix+"store", "consul", "Backend storage to use for the ring (consul, etcd, inmemory, multi, memberlist [experimental]).") + f.StringVar(&cfg.Prefix, flagsPrefix+"prefix", defaultPrefix, "The prefix for the keys in the store. Should end with a /.") + f.StringVar(&cfg.Store, flagsPrefix+"store", "consul", "Backend storage to use for the ring (consul, etcd, inmemory, multi, memberlist [experimental]).") } // Client is a high-level client for key-value stores (such as Etcd and diff --git a/pkg/ring/ring.go b/pkg/ring/ring.go index 986f1a57c7d..c6a37ecc6e8 100644 --- a/pkg/ring/ring.go +++ b/pkg/ring/ring.go @@ -79,7 +79,7 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) { // RegisterFlagsWithPrefix adds the flags required to config this to the given FlagSet with a specified prefix func (cfg *Config) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) { - cfg.KVStore.RegisterFlagsWithPrefix(prefix, f) + cfg.KVStore.RegisterFlagsWithPrefix(prefix, "collectors/", f) f.DurationVar(&cfg.HeartbeatTimeout, prefix+"ring.heartbeat-timeout", time.Minute, "The heartbeat timeout after which ingesters are skipped for reads/writes.") f.IntVar(&cfg.ReplicationFactor, prefix+"distributor.replication-factor", 3, "The number of ingesters to write to and read from.")