Implement stateless mode for Thanos Ruler #4250

idoqo · 2021-05-19T08:28:58Z

This implements the stateless mode for ruler described in the Scalable Rule Storage proposal

I added CHANGELOG entry for this change.
Change is not relevant to the end user.

Changes

Verification

test/e2e/rule_test.go

sonatype-lift · 2021-05-19T08:35:07Z

pkg/rules/remotewrite/wal.go

+	}
+
+	//nolint:staticcheck
+	a.w.bufPool.Put(buf)


SA6002: argument should be pointer-like to avoid allocations
(at-me in a reply with help or ignore)

pkg/rules/remotewrite/series.go

bwplotka

Solid work!

This is actually super close to be done 💪🏽 🚀 Thanks!

test/e2e/rule_test.go

bwplotka · 2021-05-19T09:12:57Z

cmd/thanos/config.go

+}
+
+func (rc *ruleRWConfig) registerFlag(cmd extflag.FlagClause) *ruleRWConfig {
+	cmd.Flag("remote-write", "If true, directs ruler to remote-write evaluated samples to the server configured by 'remote-write.config'.").


Why not check if config is provided and only then enable remote write? ;p

Important! User will expect remote write can be added to the TSDB ruler also.. We need to make sure that is not the case, and if you provide remote write it will make ruler stateless (except the WAL buffer)

cmd/thanos/rule.go

pkg/rules/remotewrite/remotewrite.go

bwplotka · 2021-05-19T09:17:32Z

pkg/rules/remotewrite/remotewrite.go

+	if err != nil {
+		return nil, fmt.Errorf("failed applying config to remote storage: %w", err)
+	}
+	fanoutStorage := storage.NewFanout(logger, walStore, remoteStore)


https://thanos.io/tip/contributing/coding-style-guide.md/#avoid-defining-variables-used-only-once

pkg/rules/remotewrite/series.go

test/e2e/rule_test.go

scripts/quickstart.sh

sonatype-lift · 2021-06-01T01:24:39Z

pkg/rules/remotewrite/util.go

+	series  []record.RefSeries
+}
+
+func (c *walDataCollector) Append(samples []record.RefSample) bool {


U1000: func (*walDataCollector).Append is unused
(at-me in a reply with help or ignore)

pkg/rules/remotewrite/util.go

bwplotka

Looks good, just minor nits, tests, fixing build errors and metrics (:

bwplotka · 2021-06-01T08:37:57Z

cmd/thanos/rule.go

@@ -75,6 +78,9 @@ type ruleConfig struct {
 	alertQueryURL          *url.URL
 	alertRelabelConfigYAML []byte

+	rwConfig     ruleRWConfig
+	rwConfigYAML []byte


Do we need this?

bwplotka · 2021-06-01T08:38:52Z

cmd/thanos/rule.go

@@ -75,6 +78,9 @@ type ruleConfig struct {
 	alertQueryURL          *url.URL
 	alertRelabelConfigYAML []byte

+	rwConfig     ruleRWConfig


Suggested change

rwConfig ruleRWConfig

rwConfig *extflag.PathOrContent

bwplotka · 2021-06-01T08:40:31Z

cmd/thanos/rule.go

@@ -165,6 +172,14 @@ func registerRule(app *extkingpin.App) {
 			return errors.New("--query/--query.sd-files and --query.config* parameters cannot be defined at the same time")
 		}

+		// Parse and check remote-write config and enable stateless mode for ruler.
+		if conf.rwConfig.configPath != nil {


What if the user passes empty configuration?

cmd/thanos/rule.go

bwplotka · 2021-06-01T08:41:25Z

cmd/thanos/rule.go

@@ -165,6 +172,14 @@ func registerRule(app *extkingpin.App) {
 			return errors.New("--query/--query.sd-files and --query.config* parameters cannot be defined at the same time")
 		}

+		// Parse and check remote-write config and enable stateless mode for ruler.
+		if conf.rwConfig.configPath != nil {


I think we can remove this

bwplotka · 2021-06-01T08:42:34Z

cmd/thanos/rule.go

+		}
+		var rwCfg remotewrite.Config
+		if len(conf.rwConfigYAML) == 0 {
+			return errors.New("no --remote-write.config was given")


Suggested change

return errors.New("no --remote-write.config was given")

return errors.New("empty --remote-write.config or empty file via --remote-write.config-file was given")

pkg/rules/remotewrite/remotewrite.go

bwplotka · 2021-06-01T08:48:22Z

pkg/rules/remotewrite/remotewrite.go

+		GlobalConfig:       config.DefaultGlobalConfig,
+		RemoteWriteConfigs: []*config.RemoteWriteConfig{rwConfig.RemoteStore},
+	}); err != nil {
+		return nil, fmt.Errorf("failed applying config to remote storage: %w", err)


Suggested change

return nil, fmt.Errorf("failed applying config to remote storage: %w", err)

return nil, errros.Wrapf(err, "applying config to remote storage")

Is Thanos still prefering errors.Wrapf over fmt.Errorf with %w?

bwplotka · 2021-06-01T08:49:41Z

pkg/rules/remotewrite/wal.go

+func newStorageMetrics(r prometheus.Registerer) *storageMetrics {
+	m := storageMetrics{r: r}
+	m.numActiveSeries = prometheus.NewGauge(prometheus.GaugeOpts{
+		Name: "agent_wal_storage_active_series",


Suggested change

Name: "agent_wal_storage_active_series",

Name: "wal_storage_active_series",

Let's start with wal...? Let's remove agent

test/e2e/rule_test.go

idoqo · 2021-06-07T11:59:30Z

cmd/thanos/rule.go

-	if err := removeLockfileIfAny(logger, conf.dataDir); err != nil {
-		return errors.Wrap(err, "remove storage lock files")
-	}
+	if len(rwCfgYAML) > 0 {


@squat @bwplotka

In this case though, an empty --remote-write.config (or --remote-write.config-file)will fall back to using the TSDB-backed ruler. This is because len(rwCfgYAML) == 0 either means:

The flag was provided but it's an empty file or empty string OR

The flag was not provided at all.

I couldn't think of a reliable way to distinguish between them. I've also documented this behavior in the flag help, but I don't know if it is something we want. An alternative would be to provide a separate flag for enabling stateless mode (e.g --remote.write or --remote-write.enabled).

yeya24

Let's fix the conflict and linting issue.

sonatype-lift · 2021-07-06T00:39:43Z

pkg/rules/remotewrite/wal.go

+			}
+
+			//nolint:staticcheck
+			samplesPool.Put(v)


SA6002: argument should be pointer-like to avoid allocations
(at-me in a reply with help or ignore)

bill3tt · 2021-07-09T14:53:12Z

@idoqo feels like we are very close to being ready to rock & roll 🚀 just a couple of linters & conflicts to fix

GiedriusS · 2021-07-28T12:35:49Z

@idoqo any updates?

Signed-off-by: Michael Okoko <okokomichaels@outlook.com>

This removes the need to pass a separate `remote-write` flag to ruler to enable stateless mode. Instead, we now check if a remote-write config is provided and automatically enables stateless mode based off that. Ruler test is also cleaned up to remove unnecessary tests (i.e those that have been performed by other e2e suites). Signed-off-by: Michael Okoko <okokomichaels@outlook.com>

Signed-off-by: Michael Okoko <okokomichaels@outlook.com>

pkg/rules/remotewrite/remotewrite.go

yeya24

Btw I got panic at line 559. In the case of using remote write storage, db will be nil and cause panic later.

	tsdbStore := store.NewTSDBStore(logger, db, component.Rule, conf.lset)

We need to implement a TSDBReader based on the local Wal store and the remote storage as well. However, I don't think the two stores have real Queryable implementation so currently it is still not working.

cmd/thanos/rule.go

Signed-off-by: Michael Okoko <okokomichaels@outlook.com>

yeya24 · 2021-09-18T18:45:24Z

@idoqo The failling E2E test is related to your changes. Can you take a look? Also please resolve the conflict.

pkg/rules/remotewrite/remotewrite.go

yeya24 · 2021-09-18T20:28:08Z

cmd/thanos/rule.go

@@ -522,7 +556,7 @@ func runRule(
 	)

 	// Start gRPC server.
-	{
+	if db != nil {


I think I missed it last time. Even if it is stateless mode, we still need to start the gRPC server because we need the prober and the gRPC rules feature, we can just remove the StoreServer in this case.

Example code:

// Start gRPC server. tlsCfg, err := tls.NewServerConfig(log.With(logger, "protocol", "gRPC"), conf.grpc.tlsSrvCert, conf.grpc.tlsSrvKey, conf.grpc.tlsSrvClientCA) if err != nil { return errors.Wrap(err, "setup gRPC server") } options := []grpcserver.Option{ grpcserver.WithServer(thanosrules.RegisterRulesServer(ruleMgr)), grpcserver.WithListen(conf.grpc.bindAddress), grpcserver.WithGracePeriod(time.Duration(conf.grpc.gracePeriod)), grpcserver.WithTLSConfig(tlsCfg), } if db != nil { tsdbStore := store.NewTSDBStore(logger, db, component.Rule, conf.lset) options = append(options, grpcserver.WithServer(store.RegisterStoreServer(tsdbStore))) } // TODO: Add rules API implementation when ready. s := grpcserver.New(logger, reg, tracer, grpcLogOpts, tagOpts, comp, grpcProbe, options...) g.Add(func() error { statusProber.Ready() return s.ListenAndServe() }, func(err error) { statusProber.NotReady(err) s.Shutdown(err) })

yeya24 · 2021-10-01T06:55:32Z

Hi @idoqo, are you still willing to finish this pr? Should be close to merge I think. If you are busy with other work then I am happy to take over.

idoqo · 2021-10-01T13:45:07Z

Hi @idoqo, are you still willing to finish this pr? Should be close to merge I think. If you are busy with other work then I am happy to take over.

Hi @yeya24, I'm a bit held up by exams atm so, sure

* import wal package from grafana/agent (with string interning disabled) Signed-off-by: Michael Okoko <okokomichaels@outlook.com> * Set up remote-write config and test skeleton Signed-off-by: Michael Okoko <okokomichaels@outlook.com> * Setup fanout and related storages for stateless ruler Signed-off-by: Michael Okoko <okokomichaels@outlook.com> * Optionally run ruler in stateless mode Signed-off-by: Michael Okoko <okokomichaels@outlook.com> * Set up tests and implementations for configuring remote-write for ruler Signed-off-by: Michael Okoko <okokomichaels@outlook.com> * Implement stub querier for WAL storage to fix nil pointer error Signed-off-by: Michael Okoko <okokomichaels@outlook.com> * Setup e2e test for stateless ruler Signed-off-by: Michael Okoko <okokomichaels@outlook.com> * Add copied code commentary to remotewrite packages Signed-off-by: Michael Okoko <okokomichaels@outlook.com> * Use static addresses for am and querier Signed-off-by: Michael Okoko <okokomichaels@outlook.com> * Remove need for separate remote-write flag for stateless ruler This removes the need to pass a separate `remote-write` flag to ruler to enable stateless mode. Instead, we now check if a remote-write config is provided and automatically enables stateless mode based off that. Ruler test is also cleaned up to remove unnecessary tests (i.e those that have been performed by other e2e suites). Signed-off-by: Michael Okoko <okokomichaels@outlook.com> * Generate docs for stateless ruler flags and fix tests Signed-off-by: Michael Okoko <okokomichaels@outlook.com> * Use promauto for prometheus primitives Signed-off-by: Michael Okoko <okokomichaels@outlook.com> * Group imports and satisfy go-lint Signed-off-by: Michael Okoko <okokomichaels@outlook.com> * Always return empty series set from WAL storage Signed-off-by: Michael Okoko <okokomichaels@outlook.com> * re-generate rule documentation Signed-off-by: Michael Okoko <okokomichaels@outlook.com> * copyright headers to satisfy golint Signed-off-by: Michael Okoko <okokomichaels@outlook.com> * Rename wal storage metrics Signed-off-by: Michael Okoko <okokomichaels@outlook.com> * Use Prometheus' remote write config instead of rolling another Signed-off-by: Michael Okoko <okokomichaels@outlook.com> * Fix E2E tests Signed-off-by: Ben Ye <ben.ye@bytedance.com> * add changelog Signed-off-by: yeya24 <yb532204897@gmail.com> * remove wal related tests Signed-off-by: Ben Ye <ben.ye@bytedance.com> * fix e2e Signed-off-by: Ben Ye <ben.ye@bytedance.com> * remove unused structs in tests Signed-off-by: Ben Ye <ben.ye@bytedance.com> * add licence header Signed-off-by: Ben Ye <ben.ye@bytedance.com> * use upstream agent package Signed-off-by: Ben Ye <ben.ye@bytedance.com> * address comments Signed-off-by: Ben Ye <ben.ye@bytedance.com> Co-authored-by: Michael Okoko <okokomichaels@outlook.com>

yeya24 · 2021-11-08T18:44:37Z

Thanks for the work. Close it in favor of #4731

sonatype-lift bot reviewed May 19, 2021

View reviewed changes

test/e2e/rule_test.go Outdated Show resolved Hide resolved

sonatype-lift bot reviewed May 19, 2021

View reviewed changes

pkg/rules/remotewrite/series.go Outdated Show resolved Hide resolved

bwplotka reviewed May 19, 2021

View reviewed changes

squat reviewed May 25, 2021

View reviewed changes

test/e2e/rule_test.go Outdated Show resolved Hide resolved

idoqo mentioned this pull request May 27, 2021

Re-enable e2e test for ruler #4057

Merged

1 task

idoqo force-pushed the remote-write-rule branch from e38865c to 17df499 Compare June 1, 2021 01:19

idoqo commented Jun 1, 2021

View reviewed changes

scripts/quickstart.sh Outdated Show resolved Hide resolved