Remove shared_store and shared_store_key_prefix from shipper and compactor #10840

ashwanthgoli · 2023-10-10T09:28:04Z

What this PR does / why we need it:

Removes `shared_store` and `shared_store_key_prefix` from index shipper and compactor configs and their corresponding CLI flags.

-tsdb.shipper.shared-store
-boltdb.shipper.shared-store
-tsdb.shipper.shared-store.key-prefix
-boltdb.shipper.shared-store.key-prefix
-boltdb.shipper.compactor.shared-store
-boltdb.shipper.compactor.shared-store.key-prefix

shared_store has been a confusing option allowing users to easily misconfigure Loki.
Going forward object_store setting in the period_config (which already configured the store for chunks) will be used to configure store for the index.

And the newly added path_prefix option under the index key in period_config will configure the path under which index tables are stored.

This change enforces chunks and index files for a given period reside together in the same storage bucket. More details in the upgrade guide.

-compactor.delete-request-store has to be explicitly configured going forward. Without setting this, loki wouldn't know which object store to use for storing delete requests. Path prefix for storing deletes is decided by -compactor.delete-request-store.key-prefix which defaults to index/.

Checklist

Reviewed the CONTRIBUTING.md guide (required)
Documentation added
Tests updated
CHANGELOG.md updated
- If the change is worth mentioning in the release notes, add add-to-release-notes label
Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR

JStickler

[Docs team] I'm not sure if we should drop some of the examples now that we've simplified things.

docs/sources/configure/examples/11-COS-HMAC-Example.yaml

JStickler · 2023-10-11T15:09:26Z

docs/sources/configure/examples/2-S3-Cluster-Example.yaml

@@ -29,7 +29,6 @@ storage_config:
 tsdb_shipper:
   active_index_directory: /loki/index
   cache_location: /loki/index_cache
-   shared_store: s3


Same question as before I think the names of the examples need to be changed, otherwise it's confusing to not see S3 anywhere in the example. (Same for the other examples, not going to comment on every single one.)

Also kind of wondering if having a dozen examples makes sense anymore if they're all going to mostly be the same...

docs/sources/setup/upgrade/_index.md

salvacorts · 2023-10-12T08:13:27Z

docs/sources/configure/_index.md

-  # Shared store for keeping index files. Supported types: gcs, s3, azure, cos,
-  # filesystem
-  # CLI flag: -tsdb.shipper.shared-store
-  [shared_store: <string> | default = ""]


I'm not sure about removing them. Shouldn't we rather deprecate them first and later remove it?

IIUC, as it currently works, you can write index files and chunks to different storages. I doubt anyone will be doing this but If we remove this, any user in this situation won't be able to upgrade Loki. Instead, by deprecating it, we let the user upgrade and do their diligence to deprecate it on their end.

pkg/storage/config/schema_config.go

JStickler

[docs team]

docs/sources/setup/upgrade/_index.md

JStickler

[docs team] LGTM, just one more suggestion.

JStickler · 2023-10-24T13:24:53Z

docs/sources/operations/storage/logs-deletion.md

@@ -20,6 +20,7 @@ Log entry deletion relies on configuration of the custom logs retention workflow
 ## Configuration

 Enable log entry deletion by setting `retention_enabled` to true in the compactor's configuration and setting and `deletion_mode` to `filter-only` or `filter-and-delete` in the runtime config.
+`delete_request_store` also needs to be configured when retention is enabled to process delete requests, this decides the storage bucket that stores the delete requests.


Suggested change

`delete_request_store` also needs to be configured when retention is enabled to process delete requests, this decides the storage bucket that stores the delete requests.

`delete_request_store` also needs to be configured when retention is enabled to process delete requests, this determines the storage bucket that stores the delete requests.

salvacorts · 2023-10-27T08:24:05Z

@ashwanthgoli can you update the templates and tests for the deprecated config checker tool?
(Once you do it, can you add a link to the commit here?)

salvacorts

LGTM. Awesome work :)

…actor (#10840) **What this PR does / why we need it**: #### Removes `shared_store` and `shared_store_key_prefix` from index shipper and compactor configs and their corresponding CLI flags. - `-tsdb.shipper.shared-store` - `-boltdb.shipper.shared-store` - `-tsdb.shipper.shared-store.key-prefix` - `-boltdb.shipper.shared-store.key-prefix` - `-boltdb.shipper.compactor.shared-store` - `-boltdb.shipper.compactor.shared-store.key-prefix` `shared_store` has been a confusing option allowing users to easily misconfigure Loki. Going forward `object_store` setting in the [period_config](https://grafana.com/docs/loki/latest/configure/#period_config) (which already configured the store for chunks) will be used to configure store for the index. And the newly added `path_prefix` option under the `index` key in `period_config` will configure the path under which index tables are stored. This change enforces chunks and index files for a given period reside together in the same storage bucket. More details in the upgrade guide. --- `-compactor.delete-request-store` has to be **explicitly configured** going forward. Without setting this, loki wouldn't know which object store to use for storing delete requests. Path prefix for storing deletes is decided by `-compactor.delete-request-store.key-prefix` which defaults to `index/`. **Checklist** - [X] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [X] Documentation added - [X] Tests updated - [x] `CHANGELOG.md` updated - [ ] If the change is worth mentioning in the release notes, add `add-to-release-notes` label - [X] Changes that require user attention or interaction to upgrade are documented in `docs/sources/setup/upgrade/_index.md` - [ ] For Helm chart changes bump the Helm chart version in `production/helm/loki/Chart.yaml` and update `production/helm/loki/CHANGELOG.md` and `production/helm/loki/README.md`. [Example PR](d10549e)

…and compactor (#10840)" This reverts commit 037dbda.

…actor (grafana#10840) **What this PR does / why we need it**: #### Removes `shared_store` and `shared_store_key_prefix` from index shipper and compactor configs and their corresponding CLI flags. - `-tsdb.shipper.shared-store` - `-boltdb.shipper.shared-store` - `-tsdb.shipper.shared-store.key-prefix` - `-boltdb.shipper.shared-store.key-prefix` - `-boltdb.shipper.compactor.shared-store` - `-boltdb.shipper.compactor.shared-store.key-prefix` `shared_store` has been a confusing option allowing users to easily misconfigure Loki. Going forward `object_store` setting in the [period_config](https://grafana.com/docs/loki/latest/configure/#period_config) (which already configured the store for chunks) will be used to configure store for the index. And the newly added `path_prefix` option under the `index` key in `period_config` will configure the path under which index tables are stored. This change enforces chunks and index files for a given period reside together in the same storage bucket. More details in the upgrade guide. --- `-compactor.delete-request-store` has to be **explicitly configured** going forward. Without setting this, loki wouldn't know which object store to use for storing delete requests. Path prefix for storing deletes is decided by `-compactor.delete-request-store.key-prefix` which defaults to `index/`. **Checklist** - [X] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [X] Documentation added - [X] Tests updated - [x] `CHANGELOG.md` updated - [ ] If the change is worth mentioning in the release notes, add `add-to-release-notes` label - [X] Changes that require user attention or interaction to upgrade are documented in `docs/sources/setup/upgrade/_index.md` - [ ] For Helm chart changes bump the Helm chart version in `production/helm/loki/Chart.yaml` and update `production/helm/loki/CHANGELOG.md` and `production/helm/loki/README.md`. [Example PR](grafana@d10549e)

ashwanthgoli added 4 commits October 9, 2023 12:05

introduce path prefix to index period config

a043c56

remove shared_store_prefix

6eb0245

remove shared store

450c748

add upgrade notes

b5ff6a9

pull-request-size bot added the size/XXL label Oct 10, 2023

github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Oct 10, 2023

ashwanthgoli added 5 commits October 10, 2023 16:36

Merge branch 'main' into ashwanth/remove-shared-store

bedaac8

fix tests and check-format

3e01c94

wip

b64fd40

misc corrections

7272225

add changelog

a3d0395

ashwanthgoli marked this pull request as ready for review October 11, 2023 10:28

ashwanthgoli requested a review from a team as a code owner October 11, 2023 10:28

JStickler reviewed Oct 11, 2023

View reviewed changes

review comments

b8a7d59

salvacorts reviewed Oct 12, 2023

View reviewed changes

ashwanthgoli added 2 commits October 19, 2023 19:11

compactor: remove shared store

7c5abbe

Merge branch 'main' into ashwanth/remove-shared-store

cd70820

ashwanthgoli changed the title ~~shipper: remove shared_store and shared_store_key_prefix~~ Remove shared_store and shared_store_key_prefix from shipper and compactor Oct 19, 2023

ashwanthgoli added 2 commits October 19, 2023 19:33

fixup! Merge branch 'main' into ashwanth/remove-shared-store

fd8c322

fix compactor test

7bfff9b

JStickler reviewed Oct 19, 2023

View reviewed changes

docs/sources/setup/upgrade/_index.md Outdated Show resolved Hide resolved

docs/sources/setup/upgrade/_index.md Outdated Show resolved Hide resolved

docs/sources/setup/upgrade/_index.md Outdated Show resolved Hide resolved

docs/sources/setup/upgrade/_index.md Outdated Show resolved Hide resolved

review feedback

203c422

JStickler approved these changes Oct 24, 2023

View reviewed changes

ashwanthgoli added 3 commits October 27, 2023 14:32

Merge branch 'main' into ashwanth/remove-shared-store

fb73c48

include the deletes in deprecation config checker

0d4416a

update pull request template

58f7935

salvacorts approved these changes Oct 27, 2023

View reviewed changes

minor corrections

6f2f476

Merge branch 'main' into ashwanth/remove-shared-store

22f8f97

ashwanthgoli enabled auto-merge (squash) October 30, 2023 11:38

ashwanthgoli merged commit a118e99 into main Oct 30, 2023
6 checks passed

ashwanthgoli deleted the ashwanth/remove-shared-store branch October 30, 2023 11:46

ashwanthgoli added a commit that referenced this pull request Nov 1, 2023

Revert "Remove shared_store and shared_store_key_prefix from shipper …

d22aa45

…and compactor (#10840)" This reverts commit 037dbda.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove shared_store and shared_store_key_prefix from shipper and compactor #10840

Remove shared_store and shared_store_key_prefix from shipper and compactor #10840

ashwanthgoli commented Oct 10, 2023 •

edited

JStickler left a comment

JStickler Oct 11, 2023

salvacorts Oct 12, 2023

JStickler left a comment

JStickler left a comment

JStickler Oct 24, 2023

salvacorts commented Oct 27, 2023

salvacorts left a comment

	`delete_request_store` also needs to be configured when retention is enabled to process delete requests, this decides the storage bucket that stores the delete requests.
	`delete_request_store` also needs to be configured when retention is enabled to process delete requests, this determines the storage bucket that stores the delete requests.

Remove shared_store and shared_store_key_prefix from shipper and compactor #10840

Remove shared_store and shared_store_key_prefix from shipper and compactor #10840

Conversation

ashwanthgoli commented Oct 10, 2023 • edited

Removes shared_store and shared_store_key_prefix from index shipper and compactor configs and their corresponding CLI flags.

JStickler left a comment

Choose a reason for hiding this comment

JStickler Oct 11, 2023

Choose a reason for hiding this comment

salvacorts Oct 12, 2023

Choose a reason for hiding this comment

JStickler left a comment

Choose a reason for hiding this comment

JStickler left a comment

Choose a reason for hiding this comment

JStickler Oct 24, 2023

Choose a reason for hiding this comment

salvacorts commented Oct 27, 2023

salvacorts left a comment

Choose a reason for hiding this comment

ashwanthgoli commented Oct 10, 2023 •

edited

Removes `shared_store` and `shared_store_key_prefix` from index shipper and compactor configs and their corresponding CLI flags.