Validate block range periods config in the compactor (#3534)

pracucci · web-flow · commit beddbff9650e · 2020-11-24T12:16:48.000+01:00
Signed-off-by: Marco Pracucci &lt;marco@pracucci.com&gt;
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -26,6 +26,7 @@
 * [ENHANCEMENT] /metrics now supports OpenMetrics output. HTTP and gRPC servers metrics can now include exemplars. #3524
 * [ENHANCEMENT] Expose gRPC keepalive policy options by gRPC server. #3524
 * [ENHANCEMENT] Blocks storage: enabled caching of `meta.json` attributes, configurable via `-blocks-storage.bucket-store.metadata-cache.metafile-attributes-ttl`. #3528
+* [ENHANCEMENT] Compactor: added a config validation check to fail fast if the compactor has been configured invalid block range periods (each period is expected to be a multiple of the previous one). #3534
 * [BUGFIX] Blocks storage ingester: fixed some cases leading to a TSDB WAL corruption after a partial write to disk. #3423
 * [BUGFIX] Blocks storage: Fix the race between ingestion and `/flush` call resulting in overlapping blocks. #3422
 * [BUGFIX] Querier: fixed `-querier.max-query-into-future` which wasn't correctly enforced on range queries. #3452
diff --git a/pkg/compactor/compactor.go b/pkg/compactor/compactor.go
@@ -29,6 +29,10 @@ import (
 	"github.com/cortexproject/cortex/pkg/util/services"
 )
 
+var (
+	errInvalidBlockRanges = "compactor block range periods should be divisible by the previous one, but %s is not divisible by %s"
+)
+
 // Config holds the Compactor config.
 type Config struct {
 	BlockRanges           cortex_tsdb.DurationList `yaml:"block_ranges"`
@@ -83,6 +87,17 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) {
 	f.Var(&cfg.DisabledTenants, "compactor.disabled-tenants", "Comma separated list of tenants that cannot be compacted by this compactor. If specified, and compactor would normally pick given tenant for compaction (via -compactor.enabled-tenants or sharding), it will be ignored instead.")
 }
 
+func (cfg *Config) Validate() error {
+	// Each block range period should be divisible by the previous one.
+	for i := 1; i < len(cfg.BlockRanges); i++ {
+		if cfg.BlockRanges[i]%cfg.BlockRanges[i-1] != 0 {
+			return errors.Errorf(errInvalidBlockRanges, cfg.BlockRanges[i].String(), cfg.BlockRanges[i-1].String())
+		}
+	}
+
+	return nil
+}
+
 // Compactor is a multi-tenant TSDB blocks compactor based on Thanos.
 type Compactor struct {
 	services.Service
diff --git a/pkg/compactor/compactor_test.go b/pkg/compactor/compactor_test.go
@@ -3,7 +3,6 @@ package compactor
 import (
 	"context"
 	"encoding/json"
-	"errors"
 	"flag"
 	"fmt"
 	"io/ioutil"
@@ -17,6 +16,7 @@ import (
 
 	"github.com/go-kit/kit/log"
 	"github.com/oklog/ulid"
+	"github.com/pkg/errors"
 	"github.com/prometheus/client_golang/prometheus"
 	prom_testutil "github.com/prometheus/client_golang/prometheus/testutil"
 	"github.com/prometheus/prometheus/pkg/labels"
@@ -80,6 +80,44 @@ func TestConfig_ShouldSupportCliFlags(t *testing.T) {
 	assert.Equal(t, 123, cfg.CompactionRetries)
 }
 
+func TestConfig_Validate(t *testing.T) {
+	tests := map[string]struct {
+		setup    func(cfg *Config)
+		expected string
+	}{
+		"should pass with the default config": {
+			setup:    func(cfg *Config) {},
+			expected: "",
+		},
+		"should pass with only 1 block range period": {
+			setup: func(cfg *Config) {
+				cfg.BlockRanges = cortex_tsdb.DurationList{time.Hour}
+			},
+			expected: "",
+		},
+		"should fail with non divisible block range periods": {
+			setup: func(cfg *Config) {
+				cfg.BlockRanges = cortex_tsdb.DurationList{2 * time.Hour, 12 * time.Hour, 24 * time.Hour, 30 * time.Hour}
+			},
+			expected: errors.Errorf(errInvalidBlockRanges, 30*time.Hour, 24*time.Hour).Error(),
+		},
+	}
+
+	for testName, testData := range tests {
+		t.Run(testName, func(t *testing.T) {
+			cfg := &Config{}
+			flagext.DefaultValues(cfg)
+			testData.setup(cfg)
+
+			if actualErr := cfg.Validate(); testData.expected != "" {
+				assert.EqualError(t, actualErr, testData.expected)
+			} else {
+				assert.NoError(t, actualErr)
+			}
+		})
+	}
+}
+
 func TestCompactor_ShouldDoNothingOnNoUserBlocks(t *testing.T) {
 	t.Parallel()
 
diff --git a/pkg/cortex/cortex.go b/pkg/cortex/cortex.go
@@ -208,6 +208,9 @@ func (c *Config) Validate(log log.Logger) error {
 	if err := c.StoreGateway.Validate(c.LimitsConfig); err != nil {
 		return errors.Wrap(err, "invalid store-gateway config")
 	}
+	if err := c.Compactor.Validate(); err != nil {
+		return errors.Wrap(err, "invalid compactor config")
+	}
 
 	if c.Storage.Engine == storage.StorageEngineBlocks && c.Querier.SecondStoreEngine != storage.StorageEngineChunks && len(c.Schema.Configs) > 0 {
 		level.Warn(log).Log("schema configuration is not used by the blocks storage engine, and will have no effect")

Original file line number	Diff line number	Diff line change
`@@ -208,6 +208,9 @@ func (c *Config) Validate(log log.Logger) error {`
`208`	`208`	`if err := c.StoreGateway.Validate(c.LimitsConfig); err != nil {`
`209`	`209`	`return errors.Wrap(err, "invalid store-gateway config")`
`210`	`210`	`}`
	`211`	`+ if err := c.Compactor.Validate(); err != nil {`
	`212`	`+ return errors.Wrap(err, "invalid compactor config")`
	`213`	`+ }`
`211`	`214`
`212`	`215`	`if c.Storage.Engine == storage.StorageEngineBlocks && c.Querier.SecondStoreEngine != storage.StorageEngineChunks && len(c.Schema.Configs) > 0 {`
`213`	`216`	`level.Warn(log).Log("schema configuration is not used by the blocks storage engine, and will have no effect")`