From 691b41f5ddb7a9d38ccb77682a27386f5f03c7ea Mon Sep 17 00:00:00 2001 From: Dimitar Dimitrov Date: Fri, 28 Jan 2022 10:45:53 +0100 Subject: [PATCH 1/5] Change final-sleep default value to 0s --- CHANGELOG.md | 1 + ring/lifecycler.go | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 64bf7e49a..d617e0d54 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ * [CHANGE] grpcutil.Update: Remove gRPC LB related metadata. #102 * [CHANGE] concurrency.ForEach: deprecated and reimplemented by new `concurrency.ForEachJob`. #113 * [CHANGE] ring/client: It's now possible to set different value than `consul` as default KV store. #120 +* [CHANGE] Lifecycler: Change default value of `-[ingester.]final-sleep` (YAML: `[ingester.]final_sleep`) to `0s`. * [ENHANCEMENT] Add middleware package. #38 * [ENHANCEMENT] Add the ring package #45 * [ENHANCEMENT] Add limiter package. #41 diff --git a/ring/lifecycler.go b/ring/lifecycler.go index e8d1655e7..68185b748 100644 --- a/ring/lifecycler.go +++ b/ring/lifecycler.go @@ -68,7 +68,7 @@ func (cfg *LifecyclerConfig) RegisterFlagsWithPrefix(prefix string, f *flag.Flag f.DurationVar(&cfg.JoinAfter, prefix+"join-after", 0*time.Second, "Period to wait for a claim from another member; will join automatically after this.") f.DurationVar(&cfg.ObservePeriod, prefix+"observe-period", 0*time.Second, "Observe tokens after generating to resolve collisions. Useful when using gossiping ring.") f.DurationVar(&cfg.MinReadyDuration, prefix+"min-ready-duration", 15*time.Second, "Minimum duration to wait after the internal readiness checks have passed but before succeeding the readiness endpoint. This is used to slowdown deployment controllers (eg. Kubernetes) after an instance is ready and before they proceed with a rolling update, to give the rest of the cluster instances enough time to receive ring updates.") - f.DurationVar(&cfg.FinalSleep, prefix+"final-sleep", 30*time.Second, "Duration to sleep for before exiting, to ensure metrics are scraped.") + f.DurationVar(&cfg.FinalSleep, prefix+"final-sleep", 0*time.Second, "Duration to sleep for before exiting, to ensure metrics are scraped.") f.StringVar(&cfg.TokensFilePath, prefix+"tokens-file-path", "", "File path where tokens are stored. If empty, tokens are not stored at shutdown and restored at startup.") hostname, err := os.Hostname() From 9893b03de8088603888f784f7883baf462aad013 Mon Sep 17 00:00:00 2001 From: Dimitar Dimitrov Date: Fri, 28 Jan 2022 10:47:25 +0100 Subject: [PATCH 2/5] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d617e0d54..4eab52159 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,7 +19,7 @@ * [CHANGE] grpcutil.Update: Remove gRPC LB related metadata. #102 * [CHANGE] concurrency.ForEach: deprecated and reimplemented by new `concurrency.ForEachJob`. #113 * [CHANGE] ring/client: It's now possible to set different value than `consul` as default KV store. #120 -* [CHANGE] Lifecycler: Change default value of `-[ingester.]final-sleep` (YAML: `[ingester.]final_sleep`) to `0s`. +* [CHANGE] Lifecycler: Change default value of `-[ingester.]final-sleep` (YAML: `[ingester.]final_sleep`) to `0s`. #121 * [ENHANCEMENT] Add middleware package. #38 * [ENHANCEMENT] Add the ring package #45 * [ENHANCEMENT] Add limiter package. #41 From 39b411bf477a9d421701aab5fd46ce6446432726 Mon Sep 17 00:00:00 2001 From: Dimitar Dimitrov Date: Fri, 28 Jan 2022 11:24:32 +0100 Subject: [PATCH 3/5] Allow setting default values of final-sleep --- ring/lifecycler.go | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/ring/lifecycler.go b/ring/lifecycler.go index 68185b748..312532fdb 100644 --- a/ring/lifecycler.go +++ b/ring/lifecycler.go @@ -27,12 +27,15 @@ type LifecyclerConfig struct { RingConfig Config `yaml:"ring"` // Config for the ingester lifecycle control - NumTokens int `yaml:"num_tokens"` - HeartbeatPeriod time.Duration `yaml:"heartbeat_period"` - ObservePeriod time.Duration `yaml:"observe_period"` - JoinAfter time.Duration `yaml:"join_after"` - MinReadyDuration time.Duration `yaml:"min_ready_duration"` - InfNames []string `yaml:"interface_names"` + NumTokens int `yaml:"num_tokens"` + HeartbeatPeriod time.Duration `yaml:"heartbeat_period"` + ObservePeriod time.Duration `yaml:"observe_period"` + JoinAfter time.Duration `yaml:"join_after"` + MinReadyDuration time.Duration `yaml:"min_ready_duration"` + InfNames []string `yaml:"interface_names"` + + // FinalSleep's default value can be overridden by + // setting it before calling RegisterFlags or RegisterFlagsWithPrefix. FinalSleep time.Duration `yaml:"final_sleep"` TokensFilePath string `yaml:"tokens_file_path"` Zone string `yaml:"availability_zone"` @@ -48,12 +51,14 @@ type LifecyclerConfig struct { ListenPort int `yaml:"-"` } -// RegisterFlags adds the flags required to config this to the given FlagSet +// RegisterFlags adds the flags required to config this to the given FlagSet. +// The default values of some flags can be changed; see docs of LifecyclerConfig. func (cfg *LifecyclerConfig) RegisterFlags(f *flag.FlagSet) { cfg.RegisterFlagsWithPrefix("", f) } // RegisterFlagsWithPrefix adds the flags required to config this to the given FlagSet. +// The default values of some flags can be changed; see docs of LifecyclerConfig. func (cfg *LifecyclerConfig) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) { cfg.RingConfig.RegisterFlagsWithPrefix(prefix, f) @@ -63,12 +68,17 @@ func (cfg *LifecyclerConfig) RegisterFlagsWithPrefix(prefix string, f *flag.Flag prefix = "ingester." } + // Allow clients to override default value by setting it before calling this method. + if cfg.FinalSleep == 0 { + cfg.FinalSleep = 30 * time.Second + } + f.IntVar(&cfg.NumTokens, prefix+"num-tokens", 128, "Number of tokens for each ingester.") f.DurationVar(&cfg.HeartbeatPeriod, prefix+"heartbeat-period", 5*time.Second, "Period at which to heartbeat to consul. 0 = disabled.") f.DurationVar(&cfg.JoinAfter, prefix+"join-after", 0*time.Second, "Period to wait for a claim from another member; will join automatically after this.") f.DurationVar(&cfg.ObservePeriod, prefix+"observe-period", 0*time.Second, "Observe tokens after generating to resolve collisions. Useful when using gossiping ring.") f.DurationVar(&cfg.MinReadyDuration, prefix+"min-ready-duration", 15*time.Second, "Minimum duration to wait after the internal readiness checks have passed but before succeeding the readiness endpoint. This is used to slowdown deployment controllers (eg. Kubernetes) after an instance is ready and before they proceed with a rolling update, to give the rest of the cluster instances enough time to receive ring updates.") - f.DurationVar(&cfg.FinalSleep, prefix+"final-sleep", 0*time.Second, "Duration to sleep for before exiting, to ensure metrics are scraped.") + f.DurationVar(&cfg.FinalSleep, prefix+"final-sleep", cfg.FinalSleep, "Duration to sleep for before exiting, to ensure metrics are scraped.") f.StringVar(&cfg.TokensFilePath, prefix+"tokens-file-path", "", "File path where tokens are stored. If empty, tokens are not stored at shutdown and restored at startup.") hostname, err := os.Hostname() From 66acab096ac4ccdb1523a9e9d6175e2e29516130 Mon Sep 17 00:00:00 2001 From: Dimitar Dimitrov Date: Fri, 28 Jan 2022 11:24:39 +0100 Subject: [PATCH 4/5] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4eab52159..79b9f58e1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,7 +19,7 @@ * [CHANGE] grpcutil.Update: Remove gRPC LB related metadata. #102 * [CHANGE] concurrency.ForEach: deprecated and reimplemented by new `concurrency.ForEachJob`. #113 * [CHANGE] ring/client: It's now possible to set different value than `consul` as default KV store. #120 -* [CHANGE] Lifecycler: Change default value of `-[ingester.]final-sleep` (YAML: `[ingester.]final_sleep`) to `0s`. #121 +* [CHANGE] Lifecycler: It's now possible to change default value of lifecycler's `final-sleep` to other than `30s`. #121 * [ENHANCEMENT] Add middleware package. #38 * [ENHANCEMENT] Add the ring package #45 * [ENHANCEMENT] Add limiter package. #41 From 57a7fb141493ad06c0dac17f1ab0d31d823eecbb Mon Sep 17 00:00:00 2001 From: Dimitar Dimitrov Date: Fri, 28 Jan 2022 13:51:06 +0100 Subject: [PATCH 5/5] Make lifecycler default value = 0 and configurable --- CHANGELOG.md | 3 ++- ring/lifecycler.go | 5 ----- ring/lifecycler_test.go | 15 +++++++++++++++ 3 files changed, 17 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 79b9f58e1..7701b20d7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,7 +19,8 @@ * [CHANGE] grpcutil.Update: Remove gRPC LB related metadata. #102 * [CHANGE] concurrency.ForEach: deprecated and reimplemented by new `concurrency.ForEachJob`. #113 * [CHANGE] ring/client: It's now possible to set different value than `consul` as default KV store. #120 -* [CHANGE] Lifecycler: It's now possible to change default value of lifecycler's `final-sleep` to other than `30s`. #121 +* [CHANGE] Lifecycler: Default value of lifecycler's `final-sleep` is now `0s` (i.e. no sleep). #121 +* [CHANGE] Lifecycler: It's now possible to change default value of lifecycler's `final-sleep`. #121 * [ENHANCEMENT] Add middleware package. #38 * [ENHANCEMENT] Add the ring package #45 * [ENHANCEMENT] Add limiter package. #41 diff --git a/ring/lifecycler.go b/ring/lifecycler.go index 312532fdb..1b2af3494 100644 --- a/ring/lifecycler.go +++ b/ring/lifecycler.go @@ -68,11 +68,6 @@ func (cfg *LifecyclerConfig) RegisterFlagsWithPrefix(prefix string, f *flag.Flag prefix = "ingester." } - // Allow clients to override default value by setting it before calling this method. - if cfg.FinalSleep == 0 { - cfg.FinalSleep = 30 * time.Second - } - f.IntVar(&cfg.NumTokens, prefix+"num-tokens", 128, "Number of tokens for each ingester.") f.DurationVar(&cfg.HeartbeatPeriod, prefix+"heartbeat-period", 5*time.Second, "Period at which to heartbeat to consul. 0 = disabled.") f.DurationVar(&cfg.JoinAfter, prefix+"join-after", 0*time.Second, "Period to wait for a claim from another member; will join automatically after this.") diff --git a/ring/lifecycler_test.go b/ring/lifecycler_test.go index a0bd9935f..8ce66131c 100644 --- a/ring/lifecycler_test.go +++ b/ring/lifecycler_test.go @@ -848,3 +848,18 @@ func waitRingInstance(t *testing.T, timeout time.Duration, l *Lifecycler, check return check(instance) }) } + +func TestDefaultFinalSleepValue(t *testing.T) { + t.Run("default value is 0", func(t *testing.T) { + cfg := &LifecyclerConfig{} + flagext.DefaultValues(cfg) + assert.Equal(t, time.Duration(0), cfg.FinalSleep) + }) + + t.Run("default value is overridable", func(t *testing.T) { + cfg := &LifecyclerConfig{} + cfg.FinalSleep = time.Minute + flagext.DefaultValues(cfg) + assert.Equal(t, time.Minute, cfg.FinalSleep) + }) +}