From 5ef023dbc17e4a4dfb1dd2ad79a09a4463bf37e5 Mon Sep 17 00:00:00 2001 From: Andreea Lupu <58118008+Andreea-Lupu@users.noreply.github.com> Date: Wed, 28 Sep 2022 04:06:50 +0300 Subject: [PATCH] add enable/disable option for scrub extension (#827) Signed-off-by: Andreea-Lupu --- examples/config-allextensions.json | 1 + examples/config-scrub.json | 1 + pkg/cli/extensions_test.go | 72 ++++++++++++++++++++++++++++-- pkg/cli/root.go | 17 +++++++ pkg/extensions/config/config.go | 1 + pkg/extensions/extension_scrub.go | 2 +- pkg/extensions/scrub/scrub_test.go | 6 +++ test/blackbox/scrub.bats | 1 + 8 files changed, 96 insertions(+), 5 deletions(-) diff --git a/examples/config-allextensions.json b/examples/config-allextensions.json index 8a4de7c7..1f9bd491 100644 --- a/examples/config-allextensions.json +++ b/examples/config-allextensions.json @@ -46,6 +46,7 @@ } }, "scrub": { + "enable": true, "interval": "24h" } } diff --git a/examples/config-scrub.json b/examples/config-scrub.json index 0ee65bc9..878cff79 100644 --- a/examples/config-scrub.json +++ b/examples/config-scrub.json @@ -12,6 +12,7 @@ }, "extensions": { "scrub": { + "enable": true, "interval": "24h" } } diff --git a/pkg/cli/extensions_test.go b/pkg/cli/extensions_test.go index 75e51b9b..68ed19d7 100644 --- a/pkg/cli/extensions_test.go +++ b/pkg/cli/extensions_test.go @@ -413,7 +413,38 @@ func TestServeScrubExtension(t *testing.T) { defer func() { os.Args = oldArgs }() - Convey("scrub enabled by scrub interval param set", t, func(c C) { + Convey("scrub implicitly enabled", t, func(c C) { + content := `{ + "storage": { + "rootDirectory": "%s" + }, + "http": { + "address": "127.0.0.1", + "port": "%s" + }, + "log": { + "level": "debug", + "output": "%s" + }, + "extensions": { + "scrub": { + } + } + }` + + logPath, err := runCLIWithConfig(t.TempDir(), content) + So(err, ShouldBeNil) + data, err := os.ReadFile(logPath) + So(err, ShouldBeNil) + defer os.Remove(logPath) // clean up + dataStr := string(data) + So(dataStr, ShouldContainSubstring, + "\"Extensions\":{\"Search\":null,\"Sync\":null,\"Metrics\":null,\"Scrub\":{\"Enable\":true,\"Interval\":86400000000000},\"Lint\":null") //nolint:lll // gofumpt conflicts with lll + So(dataStr, ShouldNotContainSubstring, + "Scrub interval set to too-short interval < 2h, changing scrub duration to 2 hours and continuing.") + }) + + Convey("scrub implicitly enabled, but with scrub interval param set", t, func(c C) { content := `{ "storage": { "rootDirectory": "%s" @@ -441,12 +472,44 @@ func TestServeScrubExtension(t *testing.T) { // Even if in config we specified scrub interval=1h, the minimum interval is 2h dataStr := string(data) So(dataStr, ShouldContainSubstring, - "\"Extensions\":{\"Search\":null,\"Sync\":null,\"Metrics\":null,\"Scrub\":{\"Interval\":3600000000000},\"Lint\":null") //nolint:lll // gofumpt conflicts with lll + "\"Extensions\":{\"Search\":null,\"Sync\":null,\"Metrics\":null,\"Scrub\":{\"Enable\":true,\"Interval\":3600000000000},\"Lint\":null") //nolint:lll // gofumpt conflicts with lll So(dataStr, ShouldContainSubstring, "Scrub interval set to too-short interval < 2h, changing scrub duration to 2 hours and continuing.") }) - Convey("scrub not enabled - scrub interval param not set", t, func(c C) { + Convey("scrub explicitly enabled, but without scrub interval param set", t, func(c C) { + content := `{ + "storage": { + "rootDirectory": "%s" + }, + "http": { + "address": "127.0.0.1", + "port": "%s" + }, + "log": { + "level": "debug", + "output": "%s" + }, + "extensions": { + "scrub": { + "enable": true + } + } + }` + + logPath, err := runCLIWithConfig(t.TempDir(), content) + So(err, ShouldBeNil) + data, err := os.ReadFile(logPath) + So(err, ShouldBeNil) + defer os.Remove(logPath) // clean up + dataStr := string(data) + So(dataStr, ShouldContainSubstring, + "\"Extensions\":{\"Search\":null,\"Sync\":null,\"Metrics\":null,\"Scrub\":{\"Enable\":true,\"Interval\":86400000000000},\"Lint\":null") //nolint:lll // gofumpt conflicts with lll + So(dataStr, ShouldNotContainSubstring, + "Scrub interval set to too-short interval < 2h, changing scrub duration to 2 hours and continuing.") + }) + + Convey("scrub explicitly disabled", t, func(c C) { content := `{ "storage": { "rootDirectory": "%s" @@ -461,6 +524,7 @@ func TestServeScrubExtension(t *testing.T) { }, "extensions": { "scrub": { + "enable": false } } }` @@ -472,7 +536,7 @@ func TestServeScrubExtension(t *testing.T) { defer os.Remove(logPath) // clean up dataStr := string(data) So(dataStr, ShouldContainSubstring, - "\"Extensions\":{\"Search\":null,\"Sync\":null,\"Metrics\":null,\"Scrub\":null,\"Lint\":null}") + "\"Extensions\":{\"Search\":null,\"Sync\":null,\"Metrics\":null,\"Scrub\":{\"Enable\":false,\"Interval\":86400000000000},\"Lint\":null}") //nolint:lll // gofumpt conflicts with lll So(dataStr, ShouldContainSubstring, "Scrub config not provided, skipping scrub") So(dataStr, ShouldNotContainSubstring, "Scrub interval set to too-short interval < 2h, changing scrub duration to 2 hours and continuing.") diff --git a/pkg/cli/root.go b/pkg/cli/root.go index 31f663eb..3eb5e739 100644 --- a/pkg/cli/root.go +++ b/pkg/cli/root.go @@ -344,6 +344,13 @@ func applyDefaultValues(config *config.Config, viperInstance *viper.Viper) { // Note: In case search is not empty the config.Extensions will not be nil and we will not reach here config.Extensions.Search = &extconf.SearchConfig{} } + + _, ok = extMap["scrub"] + if ok { + // we found a config like `"extensions": {"scrub:": {}}` + // Note: In case scrub is not empty the config.Extensions will not be nil and we will not reach here + config.Extensions.Scrub = &extconf.ScrubConfig{} + } } if config.Extensions != nil { @@ -378,6 +385,16 @@ func applyDefaultValues(config *config.Config, viperInstance *viper.Viper) { config.Extensions.Metrics.Prometheus = &extconf.PrometheusConfig{Path: constants.DefaultMetricsExtensionRoute} } } + + if config.Extensions.Scrub != nil { + if config.Extensions.Scrub.Enable == nil { + config.Extensions.Scrub.Enable = &defaultVal + } + + if config.Extensions.Scrub.Interval == 0 { + config.Extensions.Scrub.Interval = 24 * time.Hour // nolint: gomnd + } + } } if !config.Storage.GC && viperInstance.Get("storage::gcdelay") == nil { diff --git a/pkg/extensions/config/config.go b/pkg/extensions/config/config.go index 4855ac77..a7d76068 100644 --- a/pkg/extensions/config/config.go +++ b/pkg/extensions/config/config.go @@ -39,5 +39,6 @@ type PrometheusConfig struct { } type ScrubConfig struct { + Enable *bool Interval time.Duration } diff --git a/pkg/extensions/extension_scrub.go b/pkg/extensions/extension_scrub.go index df9c7c5b..5d0fc37e 100644 --- a/pkg/extensions/extension_scrub.go +++ b/pkg/extensions/extension_scrub.go @@ -20,7 +20,7 @@ func EnableScrubExtension(config *config.Config, log log.Logger, storeController sch *scheduler.Scheduler, ) { if config.Extensions.Scrub != nil && - config.Extensions.Scrub.Interval != 0 { + *config.Extensions.Scrub.Enable { minScrubInterval, _ := time.ParseDuration("2h") if config.Extensions.Scrub.Interval < minScrubInterval { diff --git a/pkg/extensions/scrub/scrub_test.go b/pkg/extensions/scrub/scrub_test.go index a695e6a7..7468ba14 100644 --- a/pkg/extensions/scrub/scrub_test.go +++ b/pkg/extensions/scrub/scrub_test.go @@ -48,7 +48,9 @@ func TestScrubExtension(t *testing.T) { substore := config.StorageConfig{RootDirectory: subdir} conf.Storage.SubPaths = map[string]config.StorageConfig{"/a": substore} conf.Log.Output = logFile.Name() + trueValue := true scrubConfig := &extconf.ScrubConfig{ + Enable: &trueValue, Interval: 2, } conf.Extensions = &extconf.ExtensionConfig{ @@ -106,7 +108,9 @@ func TestScrubExtension(t *testing.T) { conf.Storage.RootDirectory = dir conf.Log.Output = logFile.Name() + trueValue := true scrubConfig := &extconf.ScrubConfig{ + Enable: &trueValue, Interval: 2, } conf.Extensions = &extconf.ExtensionConfig{ @@ -171,7 +175,9 @@ func TestScrubExtension(t *testing.T) { conf.Storage.RootDirectory = dir conf.Log.Output = logFile.Name() + trueValue := true scrubConfig := &extconf.ScrubConfig{ + Enable: &trueValue, Interval: 2, } conf.Extensions = &extconf.ExtensionConfig{ diff --git a/test/blackbox/scrub.bats b/test/blackbox/scrub.bats index f82947d0..ad71cbd8 100644 --- a/test/blackbox/scrub.bats +++ b/test/blackbox/scrub.bats @@ -35,6 +35,7 @@ function setup() { }, "extensions": { "scrub": { + "enable": true, "interval": "2h" } }