RuleStore: implementation using thanos objstore bucket client #3805

jtlisi · 2021-02-09T18:11:54Z

What this PR does:

Implement a rulestore client using the Thanos objstore client

Which issue(s) this PR fixes:
Related #2262

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

pracucci

Thanks a lot @jtlisi for working on this! I left few high level comments, before jumping on a deeper 2nd pass review. 🙏

pkg/cortex/cortex.go

pracucci · 2021-02-11T15:41:42Z

pkg/storage/rulestore/config.go

+		return ErrUnsupportedStorageBackend
+	}
+
+	if cfg.Backend == bucket.S3 {


We shouldn't duplicate the bucket config Validate() but use it. I understanding you did it because the Backend validation wouldn't pass if it's configured configdb.

An option:

Create bucket.Config.ValidateWithExtraBackends(extras []string)

Pass []string{configdb} to ValidateWithExtraBackends()

This function just calls cfg.Bucket.Validate()

pracucci · 2021-02-11T15:42:27Z

pkg/storage/rulestore/config.go

+func (cfg *Config) RegisterFlags(f *flag.FlagSet) {
+	cfg.ConfigDB.RegisterFlagsWithPrefix(prefix, f)
+
+	cfg.S3.RegisterFlagsWithPrefix(prefix, f)


You should call cfg.Bucket.RegisterFlagsWithPrefix() instead of duplicating here.

The only reason I didn't call this function is because it wouldn't print configdb as an option in the help text.

Between this issue and #3805 (comment) I think I found a solution that will accommodate the use case case.

Add ExtraBackends []string field to the bucket.Config struct.

In rulestore.Config.RegisterFlags we set the bucket.Config.ExtraBackends field to []string{ConfigDB}

The bucket.Config.RegisterFlagsWithPrefix and bucket.Config.Validate functions are adjusted to merge the supportedBackends with the embedded ExtraBackends before validating/printing-help-text

Let me know what you think? I think this approach will avoid any additional functions at the cost of an additional hidden field in the config that must be set before the bucket.Config.Validate or bucket.Config.RegisterFlags functions are called.

pkg/storage/rulestore/config.go

pracucci · 2021-02-11T15:44:57Z

pkg/storage/rulestore/bucket_client.go

+	rulePrefix = "rules" + delim
+)
+
+type BucketClient struct {


This is not a bucket client but an implementation of the rule store. I think we should name it accordingly.

How does BucketRuleStore work?

pracucci · 2021-02-11T15:46:13Z

pkg/storage/rulestore/bucket_client.go

+	}
+}
+
+func (b *BucketClient) listNamespacesForUser(ctx context.Context, user string) ([]string, error) {


I'm sorry. In all functions you will have to use UserBucketClient for every operation involving a specific user. We're leveraging on it to add extra functionalities and it's now required that we use it everywhere we leverage on the Thanos objstore client.

We can discuss offline if you need more info about it.

I transitioned all user operations to use a UserBucketClient. However, I had to make some additional changes to the UserBucketClient to allow for a global prefix, since all rules are currently stores on the path rules/<user-id>.

pstibrany

Some initial comments from first quick look.

pkg/storage/bucket/user_bucket_client.go

pkg/storage/rulestore/bucket_client.go

jtlisi · 2021-02-12T21:49:51Z

@pstibrany Thanks for the feedback, this should be ready for another look.

pstibrany

Good job! I've left some comments to take a look at.

pkg/storage/bucket/prefixed_bucket_client.go

pstibrany · 2021-02-16T10:12:43Z

pkg/ruler/rulestore/config.go

+
+// NewRuleStore creates a new bucket client based on the configured backend
+func NewRuleStore(ctx context.Context, cfg Config, logger log.Logger, reg prometheus.Registerer) (rules.RuleStore, error) {
+	if cfg.Backend == ConfigDB {


Why do we need another way of configuring obsolete ConfigDB rule store? It's already possible to use it without having this option.

Based on this discussion we would eventually deprecate the config options currently configured using the -ruler.storage.* flag prefix in favor of the the newly added config field. However, the config service is still in use and needs to be carried over. I'm in favor of deprecating the config service but I think it would be best to scope that separately.

I didn't mean to deprecate configdb (although perhaps we should), just that we don't need a separate way of configuring it.

The way I was thinking about the transition was to add config options to existing RuleStoreConfig, and adding new types like "bucket-s3". But I see that it could create more confusion.

Ideally we will do this without breaking changes for ConfigDB users.

The way I was thinking about the transition was to add config options to existing RuleStoreConfig, and adding new types like "bucket-s3". But I see that it could create more confusion.

I also felt it would be more confusing to try and fit this change into the existing config struct instead of creating a new top level struct. As for the configdb, the upgrade path would be identical to the bucket upgrade path, except only the prefix of the flags would change.

However, another option would be to:

Not add the configdb as an option for the new RuleStore config struct

When deprecating the existing objectclient flags/fields, maintain the config options for the configdb. However, give a clear warning that this client is not recommended for new deployments.

Figure out the future of the configdb service separately

pkg/ruler/rulestore/bucket_client.go

pstibrany

LGTM, good job!

pstibrany · 2021-02-22T08:39:23Z

pkg/storage/bucket/client.go

+
+	// Used to inject additional backends into the config. Allows for this config to
+	// be embedded in multiple contexts and support non-object storage based backends.
+	ExtraBackends []string `yaml:"-"`


This is quite smart way of extending bucket.Config with more options.

pstibrany · 2021-02-22T08:41:06Z

pkg/ruler/rulestore/bucket_client.go

+	rulePrefix = "rules"
+)
+
+type BucketRuleStore struct {


Nit: comment this?

pstibrany · 2021-02-22T08:45:17Z

pkg/ruler/rulestore/bucket_client.go

+		if err := ctx.Err(); err != nil {
+			return err
+		}
+		userBucket := bucket.NewUserBucketClient(userID, b.bucket)


We should only create userBucket once, not for every rule group.

pracucci

Good job! 👏 ! 👏 Please mention it in the list of experimental features at docs/configuration/v1-guarantees.md.

pracucci · 2021-02-22T13:45:01Z

CHANGELOG.md

@@ -7,6 +7,7 @@
 * [CHANGE] Query-frontend: removed `-querier.split-queries-by-day` (deprecated in Cortex 0.4.0). You should use `-querier.split-queries-by-interval` instead. #3813
 * [CHANGE] Store-gateway: the chunks pool controlled by `-blocks-storage.bucket-store.max-chunk-pool-bytes` is now shared across all tenants. #3830
 * [CHANGE] Ingester: return error code 400 instead of 429 when per-user/per-tenant series/metadata limits are reached. #3833
+* [FEATURE] Experimental Ruler Storage: Add a separate set of configuration options to configure the ruler storage backend under the `-ruler-storage.` flag prefix. All TSDB bucket clients and the config service are currently supported. Clients using this implementation will only be enabled if the existing `-ruler.storage` flags are left unset. #3805


Suggested change

* [FEATURE] Experimental Ruler Storage: Add a separate set of configuration options to configure the ruler storage backend under the `-ruler-storage.` flag prefix. All TSDB bucket clients and the config service are currently supported. Clients using this implementation will only be enabled if the existing `-ruler.storage` flags are left unset. #3805

* [FEATURE] Experimental Ruler Storage: Add a separate set of configuration options to configure the ruler storage backend under the `-ruler-storage.` flag prefix. All blocks storage bucket clients and the config service are currently supported. Clients using this implementation will only be enabled if the existing `-ruler.storage` flags are left unset. #3805

pracucci · 2021-02-22T13:48:27Z

pkg/cortex/cortex.go

@@ -115,6 +116,7 @@ type Config struct {
 	TenantFederation tenantfederation.Config         `yaml:"tenant_federation"`

 	Ruler          ruler.Config                               `yaml:"ruler"`
+	RulerStorage   rulestore.Config                           `yaml:"ruler_storage" doc:"hidden"`


Do we wanna keep it hidden for now? I don't have strong opinions but, without having in it in the doc, will be difficult for our users to try it out.

Makes sense, should be good to expose since we explicitly declare it experimental in the guarantees doc

pracucci · 2021-02-22T13:50:40Z

pkg/ruler/rulestore/config.go

+const (
+	ConfigDB = "configdb"
+
+	name   = "rule-store"


Suggested change

name = "rule-store"

name = "ruler-storage"

pracucci · 2021-02-22T13:55:36Z

pkg/ruler/rulestore/config.go

+		return nil, err
+	}
+
+	store := NewBucketRuleStore(bucketClient, 10, logger)


Instead of putting a 10 here for loadConcurrency, why not having a const defined in pkg/ruler/rulestore/bucket_client.go and not letting to configure it at all?

Makes sense. I think we should avoid exposing more config options for this client unless there is a compelling need to do so.

pracucci · 2021-02-22T14:08:55Z

pkg/ruler/rulestore/bucket_client.go

+
+				gr, err := b.getRuleGroup(gCtx, user, namespace, group, gr) // reuse group pointer from the map.
+				if err != nil {
+					level.Error(b.logger).Log("msg", "failed to get rule group", "namespace", namespace, "name", group, "user", user, "err", err)


We log in this case, but not other return error cases. What if we don't log any error here, but wrap the error with info about the namespace/group/user and let the caller log it?

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com> fixup Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com> fix flag Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com> fix integration tests Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com> pr-fix: rename rule-store flag/field to ruler-storage Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com> pr-refactor: reuse bucket client Config validate and pass extra backend Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com> pr-refactor: reuse bucket.Config function with extra backends and rename rulestore.BucketClient Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com> update changelog Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com> remove unused variables Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com> make prefixed userBucket and utilize it in ruler storage Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com> refactor: create PrefixedBucketClient and use it instead of altering UserBucketClient Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com> refactor: mv rulestore to pkg/ruler Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com> refactor: fix per PR suggestions, except embedding buckets Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com> use prefixed bucket by default Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com> add SupportModifications to implementation Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com> add tests for DeleteNamespace Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com> remove utility function and use embedded prefixed bucket Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com> fix lint Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com> add todo Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com> add experimental comment to guarantees document Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

pracucci · 2021-02-22T17:53:32Z

Fantastic job @jtlisi. LGTM! 🚀

pull-request-size bot added the size/L label Feb 9, 2021

jtlisi force-pushed the 20210202_objstore_rulestore branch from 5433e9b to 98998e0 Compare February 9, 2021 20:41

pull-request-size bot added size/XL and removed size/L labels Feb 9, 2021

jtlisi force-pushed the 20210202_objstore_rulestore branch from 76ec1c6 to 0b73060 Compare February 10, 2021 18:55

jtlisi marked this pull request as ready for review February 11, 2021 01:59

jtlisi marked this pull request as draft February 11, 2021 02:00

pracucci reviewed Feb 11, 2021

View reviewed changes

jtlisi force-pushed the 20210202_objstore_rulestore branch from 6216ec7 to f975f93 Compare February 12, 2021 02:59

jtlisi marked this pull request as ready for review February 12, 2021 03:55

pstibrany reviewed Feb 12, 2021

View reviewed changes

jtlisi force-pushed the 20210202_objstore_rulestore branch from 3b0e489 to aa58b75 Compare February 12, 2021 21:49

jtlisi requested a review from pstibrany February 16, 2021 04:10

pstibrany reviewed Feb 16, 2021

View reviewed changes

jtlisi force-pushed the 20210202_objstore_rulestore branch 2 times, most recently from 1d082c2 to 392d959 Compare February 17, 2021 16:39

jtlisi requested a review from pstibrany February 17, 2021 22:36

pstibrany approved these changes Feb 22, 2021

View reviewed changes

pracucci approved these changes Feb 22, 2021

View reviewed changes

jtlisi added 2 commits February 22, 2021 11:23

fix per PR comments

5f1dd3f

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

jtlisi force-pushed the 20210202_objstore_rulestore branch from 4426200 to 5f1dd3f Compare February 22, 2021 16:43

make docs

2dd9a05

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

pull-request-size bot added size/XXL and removed size/XL labels Feb 22, 2021

fix test output check to match new error

864e71b

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

jtlisi merged commit 621222c into cortexproject:master Feb 22, 2021

jtlisi deleted the 20210202_objstore_rulestore branch February 22, 2021 19:28

This was referenced Feb 25, 2021

Refactored alertmanager storage packages #3881

Merged

AlertStore: implementation using thanos objstore bucket client #3888

Merged

pracucci mentioned this pull request Mar 10, 2021

Added local backend support to new ruler storage config #3932

Merged

3 tasks

jtlisi mentioned this pull request Mar 18, 2021

Implement unsupported methods in local rulestore #3965

Closed

3 tasks

pracucci mentioned this pull request Jun 3, 2021

Fix ruler storage single binary mode #4252

Merged

3 tasks

	* [FEATURE] Experimental Ruler Storage: Add a separate set of configuration options to configure the ruler storage backend under the `-ruler-storage.` flag prefix. All TSDB bucket clients and the config service are currently supported. Clients using this implementation will only be enabled if the existing `-ruler.storage` flags are left unset. #3805
	* [FEATURE] Experimental Ruler Storage: Add a separate set of configuration options to configure the ruler storage backend under the `-ruler-storage.` flag prefix. All blocks storage bucket clients and the config service are currently supported. Clients using this implementation will only be enabled if the existing `-ruler.storage` flags are left unset. #3805

RuleStore: implementation using thanos objstore bucket client #3805

RuleStore: implementation using thanos objstore bucket client #3805

Uh oh!

Conversation

jtlisi commented Feb 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pstibrany left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jtlisi commented Feb 12, 2021

Uh oh!

pstibrany left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pstibrany left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jtlisi commented Feb 9, 2021 •

edited

Loading