Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Add tenant_folders option to filesystem store #4333

Closed
wants to merge 9 commits into from

Conversation

jfolz
Copy link
Contributor

@jfolz jfolz commented Sep 15, 2021

What this PR does / why we need it:
Currently the docs for filesystem chunk storage state that chunks for each tenant will be stored in their own folder, but the Base64Encoder function that is used to encode chunk keys prevents this from happening.
Simply fixing the encoder function would break existing deployments, so instead this PR adds a Boolean option tenant_folders to the filesystem store that defaults to false.
Enabling this option means the new TenantBase64Encoder function is used, which is aware of the structure of chunk keys and preserves the tenant folder prefix.

Which issue(s) this PR fixes:
Fixes #4291

Special notes for your reviewer:
This is my first time working with the Loki source, but it seems like the more sensible solution would be to let the stores themselves handle how keys are encoded. Filesystem seems to be the only store to do any kind of encoding anyway. That would mean touching more of the code though, so I opted not do that. However, that meant I had to use the reflect package during client creation to avoid a circular import, which feels nasty.

Checklist

  • Documentation added
  • Tests updated

@CLAassistant
Copy link

CLAassistant commented Sep 15, 2021

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@KMiller-Grafana KMiller-Grafana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make small wording changes suggested. After that, the documentation portion of this PR looks good to me.

docs/sources/operations/storage/filesystem.md Outdated Show resolved Hide resolved
docs/sources/operations/storage/filesystem.md Outdated Show resolved Hide resolved
Thank you!

Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>
@jfolz
Copy link
Contributor Author

jfolz commented Sep 22, 2021

Any idea how to rerun the checks? docker-arm64 is marked as failed with all steps succeeding and previous runs with no code changes worked.

Copy link
Contributor

@KMiller-Grafana KMiller-Grafana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the changes. Docs look good to me now.

Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! I definitely like associating the Encoder with the storage backend. I've left a few suggestions, but this is looking great, especially the considerations for upgrading.

pkg/storage/chunk/objectclient/client.go Outdated Show resolved Hide resolved
pkg/storage/chunk/objectclient/client.go Outdated Show resolved Hide resolved
@jfolz
Copy link
Contributor Author

jfolz commented Sep 29, 2021

@owen-d Made the changes as discussed. Fully moving everything related to chunk encoding into the client would simplify some things and is probably the better solution long term, but this PR is getting quite big as it is ;)

Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, but I realized an external problem we have :( . Because we can only specify one filesystem storage configuration, there's no way for users to actually upgrade; it'll only work on new Loki deployments or ones which haven't used the file system as a backend. Toggling this field will make all previous data unreadable as the expected paths no longer align :( .

However, I have wanted us to add support for multiple storage configurations of the same type for a while now. For instance, two s3 backends used in different schema configurations, or in our case, two filesystem ones. If we added support for that first, we could comfortably merge this as the upgrade path would be as simple as adding a new filesystem storage backend and a corresponding schema_config entry.

The two other ways I can see to do this are rather undesirable:

  1. Make this part of the schema_config which can be rotated, but I dislike putting filesystem specific configs there.
  2. Let the filesystem check both encoding styles on reads. I dislike this for performance reasons.

@jfolz
Copy link
Contributor Author

jfolz commented Sep 30, 2021

Something I considered early on was to check during creation whether there are any files in the chunk directory that don't match the current settings and rename/move them. After some deliberation I decided that doing this implicitly during startup would be a bad idea, as it could take quite a while and doing things implicitly is bad. But it shouldn't be too difficult to provide this functionality as a small tool (similar to chunks-inspect) to manually upgrade and downgrade existing deployments. I see this as similar to changing the directory path. I wouldn't expect Loki to move the files for me, but it's possible for me to move them myself if I want to keep my existing data.

@owen-d
Copy link
Member

owen-d commented Oct 1, 2021

I hear you, but I feel it's a trickier upgrade path than we want to support. Namely, toggling a field like this shouldn't prevent Loki from reading all past data, so I think we'll have to find a safer way to expose this or wait until we have the ability to specify multiple backends of the same type.

@owen-d
Copy link
Member

owen-d commented Oct 25, 2021

Hey @jfolz, just wanted to let you know I haven't forgotten about this PR (and I really want it). I think we'll prioritize some work soon that we can piggyback this change onto.

@stale
Copy link

stale bot commented Jan 8, 2022

Hi! This issue has been automatically marked as stale because it has not had any
activity in the past 30 days.

We use a stalebot among other tools to help manage the state of issues in this project.
A stalebot can be very useful in closing issues in a number of cases; the most common
is closing issues or PRs where the original reporter has not responded.

Stalebots are also emotionless and cruel and can close issues which are still very relevant.

If this issue is important to you, please add a comment to keep it open. More importantly, please add a thumbs-up to the original issue entry.

We regularly sort for closed issues which have a stale label sorted by thumbs up.

We may also:

  • Mark issues as revivable if we think it's a valid issue but isn't something we are likely
    to prioritize in the future (the issue will still remain closed).
  • Add a keepalive label to silence the stalebot if the issue is very common/popular/important.

We are doing our best to respond, organize, and prioritize all issues but it can be a challenging task,
our sincere apologies if you find yourself at the mercy of the stalebot.

@stale stale bot added the stale A stale issue or PR that will automatically be closed. label Jan 8, 2022
@jfolz
Copy link
Contributor Author

jfolz commented Jan 9, 2022

Dear Mr. Bot. While our filesystem hasn't exploded with too many files just yet, I would still very much appreciate if this or something like it is merged eventually.

@stale stale bot removed the stale A stale issue or PR that will automatically be closed. label Jan 9, 2022
@stale
Copy link

stale bot commented Mar 2, 2022

Hi! This issue has been automatically marked as stale because it has not had any
activity in the past 30 days.

We use a stalebot among other tools to help manage the state of issues in this project.
A stalebot can be very useful in closing issues in a number of cases; the most common
is closing issues or PRs where the original reporter has not responded.

Stalebots are also emotionless and cruel and can close issues which are still very relevant.

If this issue is important to you, please add a comment to keep it open. More importantly, please add a thumbs-up to the original issue entry.

We regularly sort for closed issues which have a stale label sorted by thumbs up.

We may also:

  • Mark issues as revivable if we think it's a valid issue but isn't something we are likely
    to prioritize in the future (the issue will still remain closed).
  • Add a keepalive label to silence the stalebot if the issue is very common/popular/important.

We are doing our best to respond, organize, and prioritize all issues but it can be a challenging task,
our sincere apologies if you find yourself at the mercy of the stalebot.

@stale stale bot added the stale A stale issue or PR that will automatically be closed. label Mar 2, 2022
@jfolz
Copy link
Contributor Author

jfolz commented Mar 2, 2022

@owen-d can we mark this as keepalive?

@stale stale bot removed the stale A stale issue or PR that will automatically be closed. label Mar 2, 2022
@stale
Copy link

stale bot commented Apr 16, 2022

Hi! This issue has been automatically marked as stale because it has not had any
activity in the past 30 days.

We use a stalebot among other tools to help manage the state of issues in this project.
A stalebot can be very useful in closing issues in a number of cases; the most common
is closing issues or PRs where the original reporter has not responded.

Stalebots are also emotionless and cruel and can close issues which are still very relevant.

If this issue is important to you, please add a comment to keep it open. More importantly, please add a thumbs-up to the original issue entry.

We regularly sort for closed issues which have a stale label sorted by thumbs up.

We may also:

  • Mark issues as revivable if we think it's a valid issue but isn't something we are likely
    to prioritize in the future (the issue will still remain closed).
  • Add a keepalive label to silence the stalebot if the issue is very common/popular/important.

We are doing our best to respond, organize, and prioritize all issues but it can be a challenging task,
our sincere apologies if you find yourself at the mercy of the stalebot.

@stale stale bot added the stale A stale issue or PR that will automatically be closed. label Apr 16, 2022
@jfolz
Copy link
Contributor Author

jfolz commented Apr 16, 2022

.

@stale stale bot removed the stale A stale issue or PR that will automatically be closed. label Apr 16, 2022
@stale
Copy link

stale bot commented Jun 12, 2022

Hi! This issue has been automatically marked as stale because it has not had any
activity in the past 30 days.

We use a stalebot among other tools to help manage the state of issues in this project.
A stalebot can be very useful in closing issues in a number of cases; the most common
is closing issues or PRs where the original reporter has not responded.

Stalebots are also emotionless and cruel and can close issues which are still very relevant.

If this issue is important to you, please add a comment to keep it open. More importantly, please add a thumbs-up to the original issue entry.

We regularly sort for closed issues which have a stale label sorted by thumbs up.

We may also:

  • Mark issues as revivable if we think it's a valid issue but isn't something we are likely
    to prioritize in the future (the issue will still remain closed).
  • Add a keepalive label to silence the stalebot if the issue is very common/popular/important.

We are doing our best to respond, organize, and prioritize all issues but it can be a challenging task,
our sincere apologies if you find yourself at the mercy of the stalebot.

@stale stale bot added the stale A stale issue or PR that will automatically be closed. label Jun 12, 2022
@jfolz
Copy link
Contributor Author

jfolz commented Jun 12, 2022

.

@stale stale bot removed the stale A stale issue or PR that will automatically be closed. label Jun 12, 2022
@jfolz
Copy link
Contributor Author

jfolz commented Jul 31, 2022

Schema v12 (see #5291) solves the same issue and has a clear upgrade path. Thanks!

@jfolz jfolz closed this Jul 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Base64 encoding of chunk ExternalKey in filesystem object store prevents per-tenant subdirectories
4 participants