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

Proposal: Native Multi-tenancy Support #4055

Closed
wants to merge 2 commits into from

Conversation

Abhishek357
Copy link
Contributor

Signed-off-by: Abhishek357 [email protected]

Signed-off-by: Abhishek357 <[email protected]>
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Amazing work!

And good idea to put things into GitHub - this way we can be easier aware of what's the proposal status.

Very well format proposal and I love the verbose sentences. Thanks @Abhishek357 💪🏽 Some small things to figure out still - in comments.

docs/proposals/202104_native_support_of_multitenancy.md Outdated Show resolved Hide resolved
docs/proposals/202104_native_support_of_multitenancy.md Outdated Show resolved Hide resolved
### Goals

1. Introduce a mechanism to identify tenants to support multi-tenancy use cases.
2. Isolate all the incoming query requests.
Copy link
Member

Choose a reason for hiding this comment

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

Nice! Is query API the only goal? If yes let's change this to:

Suggested change
2. Isolate all the incoming query requests.
2. Isolate the incoming read and write API requests across tenants if needed.

If only query then let's make sure to put Metadata, Exemplars APIs etc to ### Non-Goals section

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, for now, let's focus on query API.
other opinions are welcomed

Copy link
Contributor

@yashrsharma44 yashrsharma44 Apr 20, 2021

Choose a reason for hiding this comment

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

We can make a note about store API(only the querier part) that we are focussing on.

Copy link
Member

Choose a reason for hiding this comment

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

Well it would be nice to put it in other APIs too, but we can extend that later. Just important @Abhishek357 to decide now (:


### Solution

* Introduce a new flag to enable or disable the multi-tenancy mode.
Copy link
Member

Choose a reason for hiding this comment

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

I would love to have something more organic. E.g if you specify a certain tenant label the mode will be enabled (:

Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to say exactly how this flag could named, or the injection of tenant ID header/flag/parameter

Copy link
Contributor Author

@Abhishek357 Abhishek357 Apr 20, 2021

Choose a reason for hiding this comment

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

Would be nice to say exactly how this flag could named

receiver already have tenant flags and we will adopt them to be consistent.

Copy link
Member

Choose a reason for hiding this comment

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

Ack, can you add note about that? And maybe provide the flag name here?


* Introduce a new flag to enable or disable the multi-tenancy mode.
* When this flag is enabled we will modify the metrics which can be used to track usage characteristics of a tenant by injecting the tenant’s label (a const. Label ).
* We will make sure that the query request received has a tenant label and if not we have some default tenant label configured.
Copy link
Member

Choose a reason for hiding this comment

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

Why not blocking if tenant is not specified?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can but we can also have a default tenant label to use when none is specified.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we have switched on multi-tenancy mode, let's have no default label. Users know that if they have enabled multitenancy, they should provide a label for that, and not depend on a default tenant label. If they want a default behaviour, then it does not make sense to have multitenancy enabled. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think that would be good. But we need to ensure how we do that on Receiver too (it would be good to do so)

* We will enforce the tenant labels inside the Thanos code base(i.e we assume the authentication is done and the query has access to the tenant's data which it specifies) when we run components with a specified mode(i.e multi-tenancy flag is enabled). We will no longer require any label enforcing proxy.
* Support the cross tenant queries using regular expressions.
* Store API will advertise the tenant label and this how it will help us -
* Let's assume a case of hard tenancy, we have two store-gateway advertising different sets of tenants.
Copy link
Member

Choose a reason for hiding this comment

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

Do we plan to have a special Info method field in proto. If yes, do you think we could design it here in this doc?

Alternatively, why not reusing LabelSet? (I am not suggesting we should, but it would be nice to mention why not e.g in Alternatives section below)

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this suggestion about this proposal - https://thanos.io/tip/proposals/202101_endpoint_discovery.md/

Copy link
Member

Choose a reason for hiding this comment

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

Correct! cc @hitanshu-mehta

docs/proposals/202104_native_support_of_multitenancy.md Outdated Show resolved Hide resolved
### Solution

* Introduce a new flag to enable or disable the multi-tenancy mode.
* When this flag is enabled we will modify the metrics which can be used to track usage characteristics of a tenant by injecting the tenant’s label (a const. Label ).
Copy link
Member

Choose a reason for hiding this comment

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

How we inject?

Copy link
Contributor

Choose a reason for hiding this comment

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

For storing the tenant external label, we follow the current implementation of how Thanos does multitenancy, but for querying, we certainly need to define the implementation details 👍

Copy link
Member

Choose a reason for hiding this comment

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

Ack, can we add this here? (:

docs/proposals/202104_native_support_of_multitenancy.md Outdated Show resolved Hide resolved
2. We can use a prom-label proxy for label enforcement.
* It does not deal with cross-tenant queries. But with the proposed approach we can use regular expression for cross tenant queries.
* Difficult to configure and increases complexity.

Copy link
Member

Choose a reason for hiding this comment

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

  1. Alternative, reuse LabelSet in InfoAPI for Store and others API tenancy info propagation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can you suggest why should we not use it?

Copy link
Member

Choose a reason for hiding this comment

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

We can, was that a plan? I think we did not talk about advertising tenant labels yet, right? I just proposed the alternative, maybe it's worth moving to main idea, let's think about it (:

@kakkoyun kakkoyun changed the title Added multi-tenancy proposal Proposal: Native Multi-tenancy Support Apr 19, 2021
@kakkoyun kakkoyun requested a review from brancz April 19, 2021 15:37
@kakkoyun
Copy link
Member

It'd be great if you could take a look @brancz :)

Signed-off-by: Abhishek357 <[email protected]>

1. Allow admins to obtain tenant information on per tenant queries, operations, and ingestion.
2. Sending tenant-specific alerts to different alert managers.
3. Support cross tenant views.
Copy link
Member

Choose a reason for hiding this comment

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

I think we should elaborate on this, what exactly this use case looks like in practice?

Copy link
Member

Choose a reason for hiding this comment

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

"User can query for multiple tenants" -> How you user specify those tenants?

cc @onprem

Copy link
Member

Choose a reason for hiding this comment

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

Ideally we extend those use case (more descriptive) more life examples, showing also the value of this feature

e.g We can enable running the same Querier/Store for multiple shared tenants


### Goals

1. Introduce a mechanism to identify tenants to support multi-tenancy use cases.
Copy link
Member

Choose a reason for hiding this comment

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

I think another goal is to have a way for user to specify which tenant it wants to access?

@stale
Copy link

stale bot commented Jun 22, 2021

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale label Jun 22, 2021
@stale stale bot closed this Jun 30, 2021
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.

4 participants