Federated ruler draft #4520

rdooley · 2021-10-07T22:37:53Z

What this PR does:
This PR introduces the features discussed in #4477 allowing the creation of federated rules which have explicit assignment of source and destination tenant for the rule, rather than assuming source and destination from rule ownership.

A quick example of this being used in a sandbox environment

In a multitenant sandbox where tenants 0, 1, thru 19 were fed data via avalanche I created two recording rules owned by tenant 15

curl -X GET -H "X-Scope-OrgID: 15" localhost:8080/prometheus/api/v1/rules | jq                                                                                 ✭
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   908  100   908    0     0   5710      0 --:--:-- --:--:-- --:--:--  5710
{
  "status": "success",
  "data": {
    "groups": [
      {
        "name": "no_dest",
        "file": "no_dest",
        "rules": [
          {
            "name": "no_dest",
            "query": "sum(avalanche_metric_mmmmm_1_0)",
            "srcTenants": "0|1|2|3|4|5|6|7|8|9|10|11|12|13|14|15|16|17|18|19",
            "labels": {},
            "health": "ok",
            "lastError": "",
            "type": "recording",
            "lastEvaluation": "2021-10-07T21:39:26.964807323Z",
            "evaluationTime": 0.003348246
          }
        ],
        "interval": 10,
        "lastEvaluation": "2021-10-07T21:39:26.96476279Z",
        "evaluationTime": 0.003397023
      },
      {
        "name": "record_rule_eg",
        "file": "record_rule",
        "rules": [
          {
            "name": "record_rule_eg",
            "query": "sum(avalanche_metric_mmmmm_1_0)",
            "srcTenants": "0|1|2|3|4|5|6|7|8|9|10|11|12|13|14|15|16|17|18|19",
            "destTenant": "17",
            "labels": {},
            "health": "ok",
            "lastError": "",
            "type": "recording",
            "lastEvaluation": "2021-10-07T21:39:25.197178261Z",
            "evaluationTime": 0.003211119
          }
        ],
        "interval": 10,
        "lastEvaluation": "2021-10-07T21:39:25.19710275Z",
        "evaluationTime": 0.003291214
      }
    ]
  },
  "errorType": "",
  "error": ""
}
// Alt view
curl -X GET -H "X-Scope-OrgID: 15" localhost:8080/api/v1/rules                                                                                                 ✭
no_dest:
    - name: no_dest
      interval: 10s
      rules:
        - record: no_dest
          expr: sum(avalanche_metric_mmmmm_1_0)
          src_tenants: 0|1|2|3|4|5|6|7|8|9|10|11|12|13|14|15|16|17|18|19
record_rule:
    - name: record_rule_eg
      interval: 10s
      rules:
        - record: record_rule_eg
          expr: sum(avalanche_metric_mmmmm_1_0)
          src_tenants: 0|1|2|3|4|5|6|7|8|9|10|11|12|13|14|15|16|17|18|19
          dest_tenant: "17"

Then viewing this in a grafana configured to query the composite tenant 0|1|2|3|4|5|6|7|8|9|10|11|12|13|14|15|16|17|18|19

we can see that the record_rule_eg and no_dest metrics match the expected value of sum(avalanche_metric_mmmmm_1_0) while being stored in tenants 17 and 15 respectively.

As this is a new feature, there is extensive documentation needed to go alongside this code change. Because of that, I'm leaving this PR in draft for now, to be viewed alongside the proposal.

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

Checklist

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

…crease multitenant query max concurrency Signed-off-by: Rees Dooley <[email protected]>

* Push ruler metrics to a random subtenant if rule is multitenant * random seed is set based off labelset hash to ensure series always pushed to same subtenant * add changelog message about multitenant ruler support Signed-off-by: Rees Dooley <[email protected]>

* expand federated tenant alertmanager test case * Add tenant-federation.max-concurrency config opt (docs need regenerating later) * switch to hash mod instead of random for federated ruler subtenant selection Signed-off-by: Rees Dooley <[email protected]>

* 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:44:34Z

pkg/ruler/compat.go

@@ -72,7 +72,9 @@ func (a *PusherAppender) Commit() error {

 	// Since a.pusher is distributor, client.ReuseSlice will be called in a.pusher.Push.
 	// We shouldn't call client.ReuseSlice here.
-	_, err := a.pusher.Push(user.InjectOrgID(a.ctx, a.userID), cortexpb.ToWriteRequest(a.labels, a.samples, nil, cortexpb.RULE))


This reinjection of the pusherappendable's context was blocking the ability to set the destination tenant context appropriately during the rule group eval

rdooley · 2021-10-07T22:46:16Z

pkg/ruler/rules/manager.go

+			srcTenants := rule.GetSrcTenants()
+			if srcTenants != "" {
+				queryCtx = user.InjectOrgID(ctx, srcTenants)
+			}


This is one of the key additions here, setting a specific query context user if srcTenants is set

rdooley · 2021-10-07T22:47:00Z

pkg/ruler/rules/manager.go

+				i := int(rule.Labels().Copy().Hash()) % len(tenantIDs)
+				tenantID := tenantIDs[i]
+				appCtx = user.InjectOrgID(ctx, tenantID)
+			}


This is the other key addition, setting a dest context based on the dest tenant if set, and if the dest tenant (or owning tenant) is composite, then consistently chosing a subtenant.

rdooley · 2021-10-13T21:33:17Z

pkg/ruler/rules/manager.go

+			tenantIDs, err := tenant.TenantIDs(appCtx)
+			if err == nil && len(tenantIDs) > 1 {
+				// Mod the hash of the series so same series always goes to same subtenant
+				i := int(rule.Labels().Copy().Hash()) % len(tenantIDs)


need to handle the hash here being negative producing a negative i

This probably also needs a test to verify the expected behavior

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

rdooley · 2022-01-06T16:14:20Z

This is going to have to change as the proposal was updated closing

andrejbranch and others added 4 commits October 4, 2021 14:15

allow multitenant queries to get metadata if not shuffle sharding, in…

a18507e

…crease multitenant query max concurrency Signed-off-by: Rees Dooley <[email protected]>

PR feedback

b636d1b

* expand federated tenant alertmanager test case * Add tenant-federation.max-concurrency config opt (docs need regenerating later) * switch to hash mod instead of random for federated ruler subtenant selection Signed-off-by: Rees Dooley <[email protected]>

pull-request-size bot added the size/XXL label Oct 7, 2021

rdooley mentioned this pull request Oct 7, 2021

Federated ruler proposal #4477

Merged

3 tasks

rdooley commented Oct 7, 2021

View reviewed changes

rdooley commented Oct 13, 2021

View reviewed changes

Change subtenant selection for composite dest tenant

415f3b5

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

rdooley closed this Jan 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Federated ruler draft #4520

Federated ruler draft #4520

Uh oh!

rdooley commented Oct 7, 2021

Uh oh!

rdooley Oct 7, 2021

Uh oh!

rdooley Oct 7, 2021

Uh oh!

rdooley Oct 7, 2021

Uh oh!

rdooley Oct 13, 2021

Uh oh!

rdooley Oct 13, 2021

Uh oh!

rdooley commented Jan 6, 2022

Uh oh!

Uh oh!

Federated ruler draft #4520

Federated ruler draft #4520

Uh oh!

Conversation

rdooley commented Oct 7, 2021

Uh oh!

rdooley Oct 7, 2021

Choose a reason for hiding this comment

Uh oh!

rdooley Oct 7, 2021

Choose a reason for hiding this comment

Uh oh!

rdooley Oct 7, 2021

Choose a reason for hiding this comment

Uh oh!

rdooley Oct 13, 2021

Choose a reason for hiding this comment

Uh oh!

rdooley Oct 13, 2021

Choose a reason for hiding this comment

Uh oh!

rdooley commented Jan 6, 2022

Uh oh!

Uh oh!