Common storage backend config #2330

colega · 2022-07-06T14:58:44Z

What this PR does

Adds a common configuration block to yaml, as well as common.-prefixed flags that will allow configuring common values for all services.

In this first iteration, we'll allow configuring a common storage backend.

The logic this follows is: flags override yaml, specific configs override common.

In particular, this means that a specific yaml configuration will be overridden by a common flag value (there's a test case for that). This is not exacty what I desired (I'd prefer to have "specific over common, flags over yaml" instead), but given the complexity of implementing the other option, I think we can accept the tradeoff.

Why is it complex to do the other option?

We would need to know how flags relate to specific struct fields (reflection-based) in order to know whether a common flag name doesn't have to be applied because it was set by yaml.

This is already hard since there's no [decent] way of sniffing what fields are being registered in the flagset with which names (actually, there might be no strict 1-1 relation between flags and fields, since flag.Value is just an interface that does stuff).

If we were able to achieve that, we'd still need to be able to distinguish whether a specific config value is there just because it was a default value or otherwise it's because yaml stated the same value as the default one.

Which issue(s) this PR fixes or relates to

Relates to #2319

Checklist

Tests updated
Documentation added: Common storage backend docs #2347
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

pracucci · 2022-07-07T08:06:32Z

In particular, this means that a specific yaml configuration will be overridden by a common flag value (there's a test case for that). This is not exacty what I desired (I'd prefer to have "specific over common, flags over yaml" instead), but given the complexity of implementing the other option, I think we can accept the tradeoff.

I would preferred that as well, but if it's too complex the current behaviour looks acceptable to me (as far as will be properly documented).

pracucci

Solid work! LGTM.

We need documentation for that, but it can be done in a follow up PR, so that we don't block it on doc reviews.

pkg/mimir/mimir.go

colega · 2022-07-07T14:23:36Z

I've updated some tests and the tutorial to use common flags (I've left couple of tests using specific configurations, to have both of them).

I also extracted root blocks for common and providers configs as you suggested, @pracucci.

Re-requesting review since there have been a bunch of changes here, meanwhile I'll start working on adding some better docs for this (I'll include docs squad as reviewer in those).

pracucci

LGTM! (modulo few minor comments)

Have you manually tried the tutorial? If yes, LGTM, otherwise please test it 😉

CHANGELOG.md

pracucci · 2022-07-07T15:11:04Z

docs/sources/operators-guide/configuring/reference-configuration-parameters/index.md

+  # The s3_backend block configures the connection to Amazon S3 object storage
+  # backend.
+  # The CLI flags prefix for this block configuration is: common.storage
+  [s3: <s3_storage_backend>]


I remember docs squad want to call it as the YAML config field name, so I would rename s3_storage_backend to s3. However, naming is very generic here (e.g. filesystem) and I personally like your naming.

Yes, I followed the same reasoning, wanted to go with just s3, gcs, etc, but they just looked too generic here, especially filesystem, which is not an object-storage per se.

docs/sources/operators-guide/configuring/reference-configuration-parameters/index.md

colega · 2022-07-08T12:15:27Z

Going to merge this as the docs will come in later, and I don't want to spend my days rebasing all the e2e tests.

pracucci · 2022-07-08T14:09:43Z

LGTM merging it.

colega · 2022-07-08T14:13:10Z

Rebased.

We don't want to make the entire Bucket config common, because: - We need them to have different prefixes - They don't necessarily will reuse middlewares (actually, middlewares are not used at all right now). Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

These use three components: ruler, alertmanager and blocks storage, so they're a good candidate for the common config. The config renaming is partial and will be completed in the next change, to make the scope of this change smaller. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com> Co-authored-by: Marco Pracucci <marco.pracucci@grafana.com> Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

dbrennand · 2022-07-10T11:18:21Z

Hi @colega @pracucci

Is there a container image available to use these changes?

The most recently updated image tag available on DockerHub is r193-df12d7b which I don't think includes these changes yet:

error loading config from /etc/mimir/mimir.yml: Error parsing config file: yaml: unmarshal errors:
  line 10: field common not found in type mimir.Config

* Extract storage backend config to a struct We don't want to make the entire Bucket config common, because: - We need them to have different prefixes - They don't necessarily will reuse middlewares (actually, middlewares are not used at all right now). Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com> * Add common flags that can be overriden by specific Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com> * Unmarshal common config yaml Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com> * Fix typo Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com> * make license Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com> * make doc Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com> * Use common config for ruler/alertmaneger e2e tests These use three components: ruler, alertmanager and blocks storage, so they're a good candidate for the common config. The config renaming is partial and will be completed in the next change, to make the scope of this change smaller. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com> * Simplify configs Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com> * Use common.storage in the tutorial Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com> * Add TestQueryFrontendWithBlocksStorageViaCommonFlags Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com> * Extract root blocks for common and storage backends Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com> * Update CHANGELOG.md Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com> * Remove sse root block Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com> * Apply suggested CHANGELOG format Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com> Co-authored-by: Marco Pracucci <marco.pracucci@grafana.com> Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com> * Clarify comment Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com> * Fix anchor after removing sse block Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com> Co-authored-by: Marco Pracucci <marco.pracucci@grafana.com>

pstibrany · 2022-07-11T15:35:42Z

Hi @colega @pracucci

Is there a container image available to use these changes?

The most recently updated image tag available on DockerHub is r193-df12d7b which I don't think includes these changes yet:
error loading config from /etc/mimir/mimir.yml: Error parsing config file: yaml: unmarshal errors:
  line 10: field common not found in type mimir.Config

There is now grafana/mimir:r194-61b589d which has this PR. In general, our weekly ~~releases~~ builds are created on monday morning (EU time).

replay · 2022-08-01T17:28:10Z

This is a great feature, thanks a lot for implementing it!
Do we need to update backend-enterprise so that the GEM specific block storage config also uses the config from common?

pracucci · 2022-08-02T08:14:02Z

This is a great feature, thanks a lot for implementing it! Do we need to update backend-enterprise so that the GEM specific block storage config also uses the config from common?

Already done.

colega · 2022-08-02T10:18:28Z

Now we should start using it 💪

pracucci self-requested a review July 7, 2022 08:04

pracucci approved these changes Jul 7, 2022

View reviewed changes

pkg/mimir/mimir.go Show resolved Hide resolved

colega force-pushed the common-storage-backend-config branch from f5c0916 to fc1f8a4 Compare July 7, 2022 14:21

colega marked this pull request as ready for review July 7, 2022 14:22

colega requested a review from pracucci July 7, 2022 14:23

pracucci approved these changes Jul 7, 2022

View reviewed changes

colega mentioned this pull request Jul 8, 2022

Common storage backend docs #2347

Merged

3 tasks

colega force-pushed the common-storage-backend-config branch from f185a6b to df7d532 Compare July 8, 2022 14:12

colega and others added 16 commits July 8, 2022 16:49

Add common flags that can be overriden by specific

db76b5f

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Unmarshal common config yaml

d3cf2a7

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Fix typo

bb67196

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

make license

004ef01

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

make doc

3921e69

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Simplify configs

1654eb7

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Use common.storage in the tutorial

1db0c0b

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Add TestQueryFrontendWithBlocksStorageViaCommonFlags

a7d7682

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Extract root blocks for common and storage backends

8e1e591

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Update CHANGELOG.md

f0780b6

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Remove sse root block

7efd73b

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Apply suggested CHANGELOG format

cc04205

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com> Co-authored-by: Marco Pracucci <marco.pracucci@grafana.com> Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Clarify comment

fc035df

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Fix anchor after removing sse block

d652246

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

colega force-pushed the common-storage-backend-config branch from df7d532 to d652246 Compare July 8, 2022 14:50

colega enabled auto-merge (squash) July 8, 2022 14:50

colega merged commit 616649a into main Jul 8, 2022

colega deleted the common-storage-backend-config branch July 8, 2022 15:03

brettpalmberg mentioned this pull request Jul 11, 2022

Errors on Tutorial play-with-grafana-mimir docker compose up #2374

Closed

colega mentioned this pull request Jul 12, 2022

[After releasing 2.3] Use common config in the tutorial #2377

Closed

colega mentioned this pull request Oct 19, 2022

Use common storage config in jsonnet #3257

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Common storage backend config #2330

Common storage backend config #2330

colega commented Jul 6, 2022 •

edited

Loading

pracucci commented Jul 7, 2022 •

edited

Loading

pracucci left a comment

colega commented Jul 7, 2022

pracucci left a comment

pracucci Jul 7, 2022

colega Jul 8, 2022

colega commented Jul 8, 2022

pracucci commented Jul 8, 2022

colega commented Jul 8, 2022

dbrennand commented Jul 10, 2022

pstibrany commented Jul 11, 2022 •

edited

Loading

replay commented Aug 1, 2022

pracucci commented Aug 2, 2022

colega commented Aug 2, 2022

Common storage backend config #2330

Common storage backend config #2330

Conversation

colega commented Jul 6, 2022 • edited Loading

What this PR does

Which issue(s) this PR fixes or relates to

Checklist

pracucci commented Jul 7, 2022 • edited Loading

pracucci left a comment

Choose a reason for hiding this comment

colega commented Jul 7, 2022

pracucci left a comment

Choose a reason for hiding this comment

pracucci Jul 7, 2022

Choose a reason for hiding this comment

colega Jul 8, 2022

Choose a reason for hiding this comment

colega commented Jul 8, 2022

pracucci commented Jul 8, 2022

colega commented Jul 8, 2022

dbrennand commented Jul 10, 2022

pstibrany commented Jul 11, 2022 • edited Loading

replay commented Aug 1, 2022

pracucci commented Aug 2, 2022

colega commented Aug 2, 2022

colega commented Jul 6, 2022 •

edited

Loading

pracucci commented Jul 7, 2022 •

edited

Loading

pstibrany commented Jul 11, 2022 •

edited

Loading