Federated ruler proposal #4477

rdooley · 2021-09-14T17:43:49Z

What this PR does:
Proposes a solution to allow federated rules, rules which query data from multiple tenants

Which issue(s) this PR fixes:
Proposes a solution for #4403

Checklist

~~Tests updated~~ NA
~~Documentation added~~ NA
~~CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]~~ NA for proposal

rdooley · 2021-09-14T17:44:58Z

cc @bboreham and @edma2

edma2

this proposal would address my use-cases for federated rule queries, but let's hear from others too

docs/proposals/federated-ruler.md

rdooley · 2021-10-05T22:51:19Z

@edma2 Can you check that this still meets your use case?

edma2 · 2021-10-05T23:00:48Z

docs/proposals/federated-ruler.md

+
+#### Proposal
+
+Since the current default is that a tenant should only be able to write rules against itself, we suggest a config option `ruler.allowed-federated-tenants`, a string slice of OrgIDs like `infra` or `0|1|2|3|4` which are allowed to write rules against all tenants. If a tenant `bar` attempts to create a federated rule, an error should be returned by the ruler api. Similarly an option `ruler.disallowed-federated-tenants` explicitly states a list of tenants for which federated rules are not allowed. Combining these in a `util.AllowedTenants` should allow us to quickly determine if federation is enabled or disabled for a given tenant at rule creation.


Passing both disallowed and allowed flags at once would be ambiguous, so Cortex should only accept one or the other.

The current proposal is similar to the enabled/disabled tenant logic for ruler seen here

interesting, I guess the allowlist effectively takes priority over the disallow list.

docs/proposals/federated-ruler.md

* implements the rest of the proposal cortexproject#4477 * bring in vendored prometheus rules and rulefmt code * add optional src and dest tenant fields * Block the creation of these federated rules behind a feature flag in ruler api * Original issue cortexproject#4403 * a large amount of documentation surrounding this expanded feature set is needed Signed-off-by: Rees Dooley <[email protected]>

rdooley · 2021-10-07T22:38:34Z

I have an initial draft of the implementation here and will be updating the proposal to links with this PR instead of the old one

rdooley · 2021-11-11T17:20:51Z

Feedback from cortex community call, cc @pracucci
An explicit use case for destination tenant is required. I will work on an example use case for destination tenant. If there isn't one, I will remove the explicit dest_tenant field from the proposal and corresponding draft PR, reverting to the current behavior of dest_tenant inheriting from rule ownership.

docs/proposals/federated-ruler.md

pracucci · 2021-11-17T09:16:11Z

docs/proposals/federated-ruler.md

+
+#### Proposal
+
+For composite tenants a random but consistent subtenant of the multiple tenants owning the rule is chosen using a hashmod of the series label to determine the subtenant and for single tenants owning a federated rule the resulting series is saved in the tenant which owns the rule.


I think I understand what you're trying to actually do with this whole work (scale out a single big tenant logically splitting it into many smaller ones) but I think what you describe in this line may not fit for the general use case of federated rules.

Should I make a separate proposal for composite tenant support for ruler or is expanding this proposal to support federated rules and composite tenants in ruler ok? Composite tenant support shares a common element with federated rules (rules querying from multiple tenants), but is distinct.

Going with dropping composite tenant support here

docs/proposals/federated-ruler.md

dimitarvdimitrov · 2021-11-22T08:49:56Z

docs/proposals/federated-ruler.md

+
+## Reasoning
+
+There are two primary use cases for allowing federated rules which query data from multiple tenants; administration of cortex and composite tenants.


@rdooley have you considered putting the src_tenants on a rule group level instead of rule level?

On one hand,

having it on group level reduces repetition between rules. I'd imagine some of the rules will likely read from or write to the same tenants, so it will make them less prone to bugs (and bugs like that can cause gaps in the rules data)

currently in prometheus rules in a group are evaluated sequentially. So rules listed earlier in a group can be used by the rules later in the group. If one of the earlier rules has a dest_tenant, then this invariant will be violated. This will slightly change behaviour.

On the other hand,

having it on rule level allows for more flexibility. Otherwise, federated and non-federated rules cannot be mixed in the same group.

it requires less changes to existing rule groups. Existing rule groups that users want to turn into federated rules can be amended simply by adding a line to a rule. In the case of group-level tenants, rules will likely need to be split up into multiple federated & non-federated groups.

I'm curious as to why you opted for having them per rule.

Opted for making src_tenants a rule instead of group based field just for the most flexibility.
Since dest_tenant has been removed from this proposal the invariant of earlier rules' data being available for later rules still holds.

I can see the case for having src_tenants at the rule level and the group level, and I had a weakly held opinion prioritizing flexibility.

What about allowing src_tenants on either rule or rule group? src_tenants on a rule would by default inherit the value from the rule group, but can be overwritten per rule.

Since my opinion is weakly held, what if we just go with on the rule group instead of per rule as that seems the preference from Dimitar and Andrea?

That's fine with me. This was can always be expanded later to per-rule as well if/when needed.

I'll get that updated today and also remove the composite tenant bits to scope this down further. Composite tenant support can be worked on separately later if we need it.

agardiman · 2021-12-16T15:33:59Z

Hi @rdooley, thanks for this proposal! Federated rules is something we would really like to see in Cortex.
We are particularly interested in defining rules that pertain to the tenant where they are defined but that query from multiple tenants of choice, so your src_tenant option (we haven't encountered a need for the composite tenants yet) with a slightly preference on having that defined per rule group instead of per rule, to not repeat it on every single rule we have (hundreds), but not a super big deal.
Do you know what's the state of the proposal and if there is any help needed to push it forward?

rdooley · 2021-12-16T17:46:13Z

Hi @rdooley, thanks for this proposal! Federated rules is something we would really like to see in Cortex. We are particularly interested in defining rules that pertain to the tenant where they are defined but that query from multiple tenants of choice, so your src_tenant option (we haven't encountered a need for the composite tenants yet) with a slightly preference on having that defined per rule group instead of per rule, to not repeat it on every single rule we have (hundreds), but not a super big deal. Do you know what's the state of the proposal and if there is any help needed to push it forward?

I think the only thing I'm waiting for is feedback from @pracucci around composite tenant support, and of course associated reviews

rdooley · 2022-01-04T18:06:48Z

I've removed composite tenant support from this proposal in the interests of expediency and simplicity.

bboreham · 2022-01-06T13:40:18Z

Linter is objecting to your spelling:

docs/proposals/federated-ruler.md:14:139: "refered" is a misspelling of "referred"

rdooley · 2022-01-06T16:32:16Z

Looks like its failing build as well, fixing
EDIT
Bad slug my bad

* unsure on the status here * cortexproject#4403 Signed-off-by: Rees Dooley <[email protected]>

Signed-off-by: Rees Dooley <[email protected]>

* switch from `_` to `-` for feature flag consistency * Consitency: use only `src_tenants` to match draft implementation Signed-off-by: Rees Dooley <[email protected]>

Signed-off-by: Rees Dooley <[email protected]>

pracucci

LGTM, thanks!

pracucci · 2022-01-11T13:56:37Z

docs/proposals/federated-ruler.md

+
+As some use cases will demand that a specific federated rule, querying tenant B and C, is stored in the owning teams tenant A, an option to allow explicit assignment of source tenants for a federated rule is needed.
+
+To support this we suggest an additional field `src_tenants` on the rule group containing an OrgID string e.g. `t0|t1|...|ti` which when present determines which tenants to query for the given rule. Rule group is chosen as it reduces repetition between rules.


Instead of a pipe separated string, you may consider having src_tenants to be an array. Looks a more robust solution to me.

Sounds good to me

Signed-off-by: Rees Dooley <[email protected]>

bboreham

Approved.

rdooley · 2022-01-12T22:16:58Z

The failing integration test seems unrelated to me

FAIL: TestAlertmanagerClustering/bucket_alertstore (1.71s)

* Federated ruler proposal Signed-off-by: Rees Dooley <[email protected]> Co-authored-by: Rees Dooley <[email protected]> Signed-off-by: Alvin Lin <[email protected]>

pull-request-size bot added the size/M label Sep 14, 2021

rdooley force-pushed the proposal-federated-ruler branch from beef98d to 0b3557c Compare September 14, 2021 17:44

rdooley mentioned this pull request Sep 14, 2021

Federated ruler #4470

Closed

3 tasks

edma2 reviewed Sep 15, 2021

View reviewed changes

docs/proposals/federated-ruler.md Outdated Show resolved Hide resolved

docs/proposals/federated-ruler.md Outdated Show resolved Hide resolved

rdooley commented Sep 16, 2021

View reviewed changes

docs/proposals/federated-ruler.md Show resolved Hide resolved

rdooley commented Oct 4, 2021

View reviewed changes

docs/proposals/federated-ruler.md Outdated Show resolved Hide resolved

rdooley commented Oct 5, 2021

View reviewed changes

docs/proposals/federated-ruler.md Outdated Show resolved Hide resolved

rdooley commented Oct 5, 2021

View reviewed changes

docs/proposals/federated-ruler.md Outdated Show resolved Hide resolved

edma2 reviewed Oct 5, 2021

View reviewed changes

pracucci reviewed Oct 6, 2021

View reviewed changes

docs/proposals/federated-ruler.md Outdated Show resolved Hide resolved

docs/proposals/federated-ruler.md Outdated Show resolved Hide resolved

rdooley commented Oct 7, 2021

View reviewed changes

docs/proposals/federated-ruler.md Outdated Show resolved Hide resolved

rdooley mentioned this pull request Oct 7, 2021

Federated ruler draft #4520

Closed

3 tasks

rdooley requested a review from pracucci October 19, 2021 22:46

rdooley force-pushed the proposal-federated-ruler branch from 063540f to a7c771d Compare November 15, 2021 18:13

pracucci reviewed Nov 17, 2021

View reviewed changes

docs/proposals/federated-ruler.md Outdated Show resolved Hide resolved

rdooley force-pushed the proposal-federated-ruler branch from a7c771d to 39248ef Compare November 17, 2021 16:28

dimitarvdimitrov reviewed Nov 22, 2021

View reviewed changes

dimitarvdimitrov mentioned this pull request Nov 22, 2021

Add source_tenants to rule definitions prometheus/prometheus#9814

Closed

rdooley force-pushed the proposal-federated-ruler branch from 9b84f05 to 8950adf Compare January 4, 2022 18:05

rdooley force-pushed the proposal-federated-ruler branch from ded9da2 to f10830f Compare January 6, 2022 16:15

rdooley force-pushed the proposal-federated-ruler branch from f10830f to f788b7a Compare January 6, 2022 16:41

Rees Dooley and others added 9 commits January 6, 2022 10:53

Federated ruler proposal

41a9b2c

* unsure on the status here * cortexproject#4403 Signed-off-by: Rees Dooley <[email protected]>

Add clarifying statements to federated ruler proposal

a2252c9

Signed-off-by: Rees Dooley <[email protected]>

Add Challenge for federated rule access only for configured tenants

c0c680a

Signed-off-by: Rees Dooley <[email protected]>

Updates to federated ruler proposal from implementation

95fe9d1

Signed-off-by: Rees Dooley <[email protected]>

Add explicit definition of federated rule

70d79cc

Signed-off-by: Rees Dooley <[email protected]>

Update federated ruler proposal with link to draft pr

9b89239

Signed-off-by: Rees Dooley <[email protected]>

Remove dest_tenant from federated ruler proposal

7a54397

* switch from `_` to `-` for feature flag consistency * Consitency: use only `src_tenants` to match draft implementation Signed-off-by: Rees Dooley <[email protected]>

rm composite tenants from federated ruler proposal

9c46ad9

Signed-off-by: Rees Dooley <[email protected]>

Referred not refered

63bdfd5

Signed-off-by: Rees Dooley <[email protected]>

rdooley force-pushed the proposal-federated-ruler branch from f788b7a to 63bdfd5 Compare January 6, 2022 19:43

pracucci approved these changes Jan 11, 2022

View reviewed changes

Switch from | sep string to array in rules yaml

ee1410b

Signed-off-by: Rees Dooley <[email protected]>

pracucci assigned alvinlin123 and bboreham Jan 12, 2022

bboreham approved these changes Jan 12, 2022

View reviewed changes

bboreham enabled auto-merge (squash) January 12, 2022 18:12

bboreham merged commit 08279f0 into cortexproject:master Jan 13, 2022

rdooley deleted the proposal-federated-ruler branch January 13, 2022 17:24


		#### Proposal

		Since the current default is that a tenant should only be able to write rules against itself, we suggest a config option `ruler.allowed-federated-tenants`, a string slice of OrgIDs like `infra` or `0\|1\|2\|3\|4` which are allowed to write rules against all tenants. If a tenant `bar` attempts to create a federated rule, an error should be returned by the ruler api. Similarly an option `ruler.disallowed-federated-tenants` explicitly states a list of tenants for which federated rules are not allowed. Combining these in a `util.AllowedTenants` should allow us to quickly determine if federation is enabled or disabled for a given tenant at rule creation.


		#### Proposal

		For composite tenants a random but consistent subtenant of the multiple tenants owning the rule is chosen using a hashmod of the series label to determine the subtenant and for single tenants owning a federated rule the resulting series is saved in the tenant which owns the rule.


		## Reasoning

		There are two primary use cases for allowing federated rules which query data from multiple tenants; administration of cortex and composite tenants.


		As some use cases will demand that a specific federated rule, querying tenant B and C, is stored in the owning teams tenant A, an option to allow explicit assignment of source tenants for a federated rule is needed.

		To support this we suggest an additional field `src_tenants` on the rule group containing an OrgID string e.g. `t0\|t1\|...\|ti` which when present determines which tenants to query for the given rule. Rule group is chosen as it reduces repetition between rules.

Federated ruler proposal #4477

Federated ruler proposal #4477

Uh oh!

Conversation

rdooley commented Sep 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rdooley commented Sep 14, 2021

Uh oh!

edma2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rdooley commented Oct 5, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rdooley commented Oct 7, 2021

Uh oh!

rdooley commented Nov 11, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agardiman commented Dec 16, 2021

Uh oh!

rdooley commented Dec 16, 2021

Uh oh!

rdooley commented Jan 4, 2022

Uh oh!

bboreham commented Jan 6, 2022

Uh oh!

rdooley commented Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bboreham left a comment

Choose a reason for hiding this comment

Uh oh!

rdooley commented Jan 12, 2022

Uh oh!

Uh oh!

rdooley commented Sep 14, 2021 •

edited

Loading

rdooley commented Jan 6, 2022 •

edited

Loading