-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Storage: Allow setting a constant prefix for all created keys #10096
Conversation
8bc9881
to
8a71b0e
Compare
@aschleck Thank you for your PR! We really appreciate it. I think being able to set a prefix may make sense for other storages than just S3. IMO, if we want to implement this, we should support it for all (or most) storage backends. Would you be open to giving that a try? Happy to change my opinion tho :) Here's a PR for Mimir doing something similar. From what I can see, looks like Mimir only support alpha-numeric prefixes, not path-like prefixes (e.g. |
To add to my comment above, looks like (similarly to mimir), we already have a prefixed client: I think we can take the same approach as Mimir and use it when creating the object client as they do here: |
Thank you Salva for looking at this! To make sure I understand, the request is:
|
I can't figure out why this integration test is failing. I know it's something in this PR, it passes on the parent commit, but I'm not seeing the issue. Current theory is maybe the compactor is stuck but no idea why. Any suggestions? |
Thanks, I am currently testing on a 2.8.3 branch + your changes, it builds and run. |
@salvacorts do you know why the integration test is failing? |
Not sure how this is intended to work, but if it circumvents the schema code and stores all data under a single prefix then this is a giant potential footgun for s3 users that should come with documentation and a warning. |
@cstyan thank you for thinking about this! My expectation is that changing this would be equivalent to changing the bucket name in the S3 config, so it's just as much as a footgun to the schema stuff as switching the bucket option would be. Does that make sense? I'm certainly happy to make the documentation clearer on that front. In my original PR this was actually an S3-specific option and lived at the same level as the bucket option, which in light of your point may actually be a better solution. It also passed the CI tests so I kind of like reverting back to it. What do you think of the approach in aschleck@8a71b0e? |
@aschleck Would you mind elaborating as to your use case for wanting to have a constant prefix? Am I misunderstanding that this would mean all objects are stored under just a single prefix, such as |
@cstyan I am not an expert on Loki's codebase, so please correct me if my change is not doing what I intend. I am not trying to force everything into My particular problem is that I have a ton of Kubernetes clusters, and I want to run a separate, isolated Loki instance in each of them. However, given there are only a finite number of AWS S3 buckets allowed per account, I want to make all of these different Loki instances share the same bucket but in isolated parts of it. So my goal is to make objects write into My understanding, and the understanding of others in #5889, is that currently one Loki instance basically takes over an entire bucket. And you get eg |
I am trying to do for Loki what grafana/mimir#1686 did for Mimir and thanos-io/thanos#5337 did for Thanos. |
I had some time today and fixed the integration test problem. @salvacorts and @cstyan please let me know if you have further concerns or if we can merge this. |
Oh! That's super interesting, thank you for following up. I would never have thought about that case so appreciate you finding it. What do you think is the best mitigation? Should we error out if there's a leading / or silently strip it? Personally I think stripping it makes sense. @ningyougang @slim-bean |
Changed the logic to strip leading |
Thank you @slim-bean! Do you think you can approve this PR and merge it? What is left to do here? |
@slim-bean @salvacorts @JStickler can we get this PR approved and merged? |
Hey all!! Great work on this feature 👏 Do we know if this solution is ready to be merged? Happy to assist if there is anything left to do 😄 |
d99f4b5
to
e8da18f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice work @aschleck!
The changes look good to me. I think we need to also make the changes in the code initializing storage for ruler?
I think we need to add some documentation around what the caveats are when you use a single storage bucket with multiple Loki installations under different prefixes. |
An area where the global prefix could cause issues is the caching object client of the index shipper, because it splits object keys by delimiter and checks the length of the resulting parts. Maybe you could add a test case with a configured global prefix to see how it behaves. |
a05b374
to
1b5d176
Compare
I rebase this PR. CI says "Build is blocked, please, contact repo admin in order to proceed", I'm not sure why but I don't think it's my code.
I wouldn't expect this to cause any problems. The prefix is internal to the prefixed client so this caching client shouldn't see it. I added this test case just in case and it passes.
Most users aren't going to hit the S3 concurrency limits where this matters, so I'm not quite sure where you envision putting this. I would imagine it goes in a "Loki tuning guide" but I don't think one exists. This feels like something that might belong in a follow-up PR? |
@aschleck Our security team has added a layer of protection to the repo so that any PRs coming from a fork needs to have their CI builds approved by an admin every time CI is run. It has slowed things down considerably, and we're trying to find a different solution to the security problem. In the meantime, the team has to manually trigger CI builds. I triggered one just now, before I saw that your branch has conflicts that need to be resolved. |
Ahhh another merge conflict, you folks are prolific :) I just rebased it again. Thank you for triggering the CI build! |
Signed-off-by: Edward Welch <[email protected]>
Signed-off-by: Edward Welch <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you everyone for all the involvement here and huge thanks to @aschleck for your patience and persistence with this change ❤️ 🎉
…a#10096) **What this PR does / why we need it**: Adds a new option under the aws stanza named key_prefix. This is useful if you have many Loki installations and do not want to create a million buckets. This is different than `compactor.shared_store_key_prefix` because it also affects eg the `loki_cluster_seed.json` file. **Which issue(s) this PR fixes**: Fixes grafana#5889 **Special notes for your reviewer**: **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) --------- Signed-off-by: Edward Welch <[email protected]> Co-authored-by: Ed Welch <[email protected]>
What this PR does / why we need it:
Adds a new option under the aws stanza named key_prefix. This is useful if you have many Loki installations and do not want to create a million buckets. This is different than
compactor.shared_store_key_prefix
because it also affects eg theloki_cluster_seed.json
file.Which issue(s) this PR fixes:
Fixes #5889
Special notes for your reviewer:
Checklist
CONTRIBUTING.md
guide (required)CHANGELOG.md
updatedadd-to-release-notes
labeldocs/sources/setup/upgrade/_index.md
production/helm/loki/Chart.yaml
and updateproduction/helm/loki/CHANGELOG.md
andproduction/helm/loki/README.md
. Example PR