Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Proposal: query path tenancy #6320
Proposal: query path tenancy #6320
Changes from 7 commits
2b86f4b
8a9ffa3
38c85c1
2873c12
d68586b
908accb
52c865e
dc9b64b
aa8f20a
ec47877
b14982c
1ba8493
abc44d8
d325493
f7d1dc0
5e12994
c95450e
c2e5f0e
e9a6fa2
cf71fb6
0001b62
cd34490
98b902b
f80384e
300ed71
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
We can achieve these goals by having separate query instances for individual tenants. A valid goal would be simplifying management for multi-tenant setups and pooling resources to reduce waste.
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.
Don't see any mention of reusing some cortex components that already implement multi-tenancy using an HTTP header. 😦
I wouldn't recommend putting cortex as a dependency. But why not just copy the bits you like here?
we would be happy be deleting code from cortex if it makes it in Thanos 😄
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.
Hey @friedrichg, can you give us some good pointers for this implementation on the Cortex side? I'm not familiar with Cortex and its features, so I'm very unaware of how it implements things.
If it aligns well with how we want to move forward with this, it'll be great to reuse some code (be it importing or copying) so that both projects benefit from it. I know there are some design differences between the two projects, so let's see what can be done and reused.
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.
Thanos and Cortex are very similar, both CNCF projects, sort of rivals in the old days. But nowadays Cortex just reuses a bunch of components from thanos. The compactor, the store-gateway, etc. There is too many things in common.
The thing cortex added from the beginning was the multi-tenancy, so we it adds header to all requests, so all components know what tenant is it about.
And like I said importing cortex into thanos is not a good idea, because it was a thing that happened in the past that created circular dependencies. I think is better you just copy what you need and we reuse it in Cortex (As we already do 😃 )
https://cortexmetrics.io/docs/architecture/
Find me in slack cncf #cortex if you want to chat more about it.
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.
Awesome. I'll get back to this next week and reach out if I have questions or ideas. Thanks. 🙇
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.
Yea, I think it's good point to mention Cortex way of dealing with this as inspiration, but there is nothing wrong in revisiting this feature and checking if there is anything we could improve before implementing.
Ideally, we could have the same solution, so the tools that works on top of Cortex will work on top of Thanos. That's fair advantage to adopting similar patterns. Another advantage is easier migrations between projects, helpful if we will try to unify projects at some point.
Can we mention this somewhere in the doc (advantage and Cortex pattern)? Thanks!
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.
Interesting. I would argue we might need to be careful with adding feature that will block important features like cross tenancy in future. However I feel we get into the point where we talk about different tenancy aspects:
One is tenancy of data, so who owns what. Second is tenancy of query, who should be billed for query. Those write/read tenancy does not need to be 1:1. For example cross tenancy to me means that tenant A can read data of tenant B, but the quotas, settings and billing should be on behalf of one tenant.
All of this to me means we could refine this cross tenancy point. I think cross-tenancy is a goal, just we don't want shared tenancy for reads (?)
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.
Cross-tenancy is definitely a future goal of the project and a good "follow up", but I didn't plan this proposal to go deep into its details. I don't think it'll block in any way a cross-tenant query implementation. Cross-tenant queries will require changes to the logic behind the label enforcement part, but there's no architectural decision that will need to be undone when the time comes.
Totally we will need to develop more on these two aspects of tenancy you mention, @bwplotka. Plus we will need some configuration that will work kinda like an authorization for cross-tenant query (i.e. tenant A can query B and C; tenant B can only query C, etc).
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.
So, it won't be possible to query data from multiple tenants anymore? 😢
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.
@GiedriusS: it will still be possible, by default, to keep backwards compatibility. When the tenancy-enforcement feature is turned on on a Querier, then it won't be possible for requests arriving there. But you can have another set of Queriers with the feature disabled and multi-tenant queries will work, just like today. o/
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.
@GiedriusS: I am trying to ensure we keep backwards compatibility and the possibility of running queries with and without tenant-enforcement in a cluster. Maybe we can collaborate later on a proposal for multi-tenant query with the tenancy enforcement on? It's not a goal of this proposal to outline how this would work. What do you think? :D
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.
To be strict for gRPC - will we pass that tenant as metadata or HTTP2 header? I think the metadata is the answer.
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.
Agreed, @bwplotka. GRPC metadata seems to be the answer for it. Will batch this into updates to the proposal for the next push.