Refactor Ruler, introduce MultiTenantManager interface #3019

annanay25 · 2020-08-12T08:48:04Z

What this PR does:
This PR refactors ruler package by introducing a MultiTenantManager interface. This is done for multiple reasons.

First, it splits out the Ruler struct into distinct components, and the Ruler is seen as a module that ties together the rule storage, ring for sharding and Manager for evaluation and notification on the rules.

+---------------------------------------------------------------+
|                                                               |
|                   Query       +-------------+                 |
|            +------------------>             |                 |
|            |                  |    Store    |                 |
|            | +----------------+             |                 |
|            | |     Rules      +-------------+                 |
|            | |                                                |
|            | |                                                |
|            | |                                                |
|       +----+-v----+   Filter  +------------+                  |
|       |           +----------->            |                  |
|       |   Ruler   |           |    Ring    |                  |
|       |           <-----------+            |                  |
|       +-------+---+   Rules   +------------+                  |
|               |                                               |
|               |                                               |
|               |                                               |
|               |    Load      +-----------------+              |
|               +-------------->                 |              |
|                              |     Manager     |              |
|                              |                 |              |
|                              +-----------------+              |
|                                                               |
+---------------------------------------------------------------+

Secondly, it allows us to add custom functionality into the Ruler depending on custom fields present in the RuleGroup description, by overriding the DefaultMultiTenantManager. For ex: This brings flexibility in using different appenders based on special config fields present in the RuleGroup definition.

Which issue(s) this PR fixes:
Fixes #

Checklist
Will drop draft status once:

Bump prometheus and thanos to master #3000 is merged (this PR is built on top of that one)
Tests updated
Documentation added
NA CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

/cc @jtlisi

Signed-off-by: Ben Ye <yb532204897@gmail.com>

Signed-off-by: Marco Pracucci <marco@pracucci.com>

Signed-off-by: Annanay <annanayagarwal@gmail.com>

pracucci

Very good job! I patiently went through the diff and looks a no-op refactoring to me, except for the few comments I left.

pkg/ruler/ruler_test.go

pkg/ruler/manager.go

pkg/ruler/ruler.go

pkg/ruler/manager.go

Co-authored-by: Marco Pracucci <marco@pracucci.com> Signed-off-by: Annanay <annanayagarwal@gmail.com>

Signed-off-by: Annanay <annanayagarwal@gmail.com>

pkg/ruler/ruler.go

pstibrany

I have left few nit-picky comments, but overall LGTM.

pkg/ruler/ruler.go

pkg/ruler/manager.go

Signed-off-by: Annanay <annanayagarwal@gmail.com>

annanay25 · 2020-08-17T14:42:50Z

Thanks for the reviews, @pracucci & @pstibrany!

pracucci

LGTM, thanks!

pstibrany · 2020-08-17T14:56:27Z

pkg/ruler/ruler.go

@@ -149,59 +139,64 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) {
 	f.DurationVar(&cfg.ResendDelay, "ruler.resend-delay", time.Minute, `Minimum amount of time to wait before resending an alert to Alertmanager.`)
 }

+// MultiTenantManager is the interface of interaction with a Manager that is tenant aware.
+type MultiTenantManager interface {
+	// SyncRuleGroups is used to sync the Manager with rules from the RuleStore.


In the next PR, we can mention that ruleGroups is per tenant.

pstibrany · 2020-08-17T15:04:07Z

pkg/ruler/ruler.go

+	// GetRules fetches rules for a particular tenant (userID).
+	GetRules(userID string) []*promRules.Group
+	// Stop stops all Manager components.
+	Stop()


Seeing Stop method in the generic interface looks like an anti-pattern to me. As Ruler is not creating the manager, I don't think it should be one stopping it either.

(But that can be addressed in another PR)

I agree, we need the Stop() function but I had a feeling the other function should be named StartOrSyncRuleGroups(). That sounds a little long, but do you have any other suggestions?

pstibrany · 2020-08-17T15:07:06Z

pkg/ruler/ruler.go

@@ -149,59 +139,64 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) {
 	f.DurationVar(&cfg.ResendDelay, "ruler.resend-delay", time.Minute, `Minimum amount of time to wait before resending an alert to Alertmanager.`)
 }

+// MultiTenantManager is the interface of interaction with a Manager that is tenant aware.


Suggested change

// MultiTenantManager is the interface of interaction with a Manager that is tenant aware.

// MultiTenantManager interface describes interaction between Ruler, that periodically

// loads rules from the store, and object that keeps rules in memory for each tenant.

It sounds to me like "multi-tenant aware rules manager" :)

and object that keeps rules in memory for each tenant

The DefaultMultiTenantManager uses a mapper to write rules to a file and passes file path to the Prometheus Manager. Maybe we can modify this to

// MultiTenantManager interface describes interaction between Ruler, that periodically // loads rules from the store, and object that syncs rules to the Prometheus manager // for every tenant.

pull-request-size bot added the size/XXL label Aug 12, 2020

yeya24 and others added 8 commits August 12, 2020 16:01

Bump prometheus and thanos to master

0742bcf

Signed-off-by: Ben Ye <yb532204897@gmail.com>

add changelog entry

11747f2

Signed-off-by: Ben Ye <yb532204897@gmail.com>

Rebased and fixed CHANGELOG entry ordering

b328d5b

Signed-off-by: Marco Pracucci <marco@pracucci.com>

Reverted logic to pass userID to pusherAppender just for clarity

764bf00

Signed-off-by: Marco Pracucci <marco@pracucci.com>

Refactor Ruler, add MultiTenantManager interface

27455d1

Signed-off-by: Annanay <annanayagarwal@gmail.com>

Fix tests

c232c05

Signed-off-by: Annanay <annanayagarwal@gmail.com>

Simplify struct creation

6254d87

Signed-off-by: Annanay <annanayagarwal@gmail.com>

Refactor test

5d571cd

Signed-off-by: Annanay <annanayagarwal@gmail.com>

annanay25 force-pushed the manager-interface branch from 816d7bb to 5d571cd Compare August 12, 2020 16:55

Merge branch 'master' into pr-3000

8616676

Signed-off-by: Annanay <annanayagarwal@gmail.com>

pull-request-size bot added size/L and removed size/XXL labels Aug 12, 2020

annanay25 marked this pull request as ready for review August 12, 2020 16:58

nit, fix lint

3e8eb8b

Signed-off-by: Annanay <annanayagarwal@gmail.com>

pracucci reviewed Aug 14, 2020

View reviewed changes

pkg/ruler/ruler_test.go Outdated Show resolved Hide resolved

pkg/ruler/manager.go Outdated Show resolved Hide resolved

pkg/ruler/ruler.go Show resolved Hide resolved

pkg/ruler/manager.go Outdated Show resolved Hide resolved

pkg/ruler/manager.go Outdated Show resolved Hide resolved

annanay25 and others added 2 commits August 17, 2020 12:30

Apply suggestions from code review

96adac1

Co-authored-by: Marco Pracucci <marco@pracucci.com> Signed-off-by: Annanay <annanayagarwal@gmail.com>

Address review comments

a22a28d

Signed-off-by: Annanay <annanayagarwal@gmail.com>

annanay25 force-pushed the manager-interface branch from 499bf72 to a22a28d Compare August 17, 2020 07:07

annanay25 added 2 commits August 17, 2020 14:23

Refactor NewRuler() to accept a MultiTenantManager

fabc9c2

Signed-off-by: Annanay <annanayagarwal@gmail.com>

Move managersTotal metric under DefaultTenantManager

47942c0

Signed-off-by: Annanay <annanayagarwal@gmail.com>

pull-request-size bot added size/XL and removed size/L labels Aug 17, 2020

annanay25 commented Aug 17, 2020

View reviewed changes

pkg/ruler/ruler.go Show resolved Hide resolved

pstibrany approved these changes Aug 17, 2020

View reviewed changes

pkg/ruler/ruler.go Outdated Show resolved Hide resolved

pkg/ruler/manager.go Outdated Show resolved Hide resolved

pkg/ruler/manager.go Outdated Show resolved Hide resolved

pracucci reviewed Aug 17, 2020

View reviewed changes

pkg/ruler/manager.go Outdated Show resolved Hide resolved

Address review comments

07cb41c

Signed-off-by: Annanay <annanayagarwal@gmail.com>

pracucci approved these changes Aug 17, 2020

View reviewed changes

pstibrany approved these changes Aug 17, 2020

View reviewed changes

pstibrany merged commit 757b9d1 into cortexproject:master Aug 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Ruler, introduce MultiTenantManager interface #3019

Refactor Ruler, introduce MultiTenantManager interface #3019

annanay25 commented Aug 12, 2020 •

edited

Loading

pracucci left a comment

pstibrany left a comment

annanay25 commented Aug 17, 2020

pracucci left a comment

pstibrany Aug 17, 2020

pstibrany Aug 17, 2020

annanay25 Aug 18, 2020

pstibrany Aug 17, 2020

annanay25 Aug 18, 2020

	// MultiTenantManager is the interface of interaction with a Manager that is tenant aware.
	// MultiTenantManager interface describes interaction between Ruler, that periodically
	// loads rules from the store, and object that keeps rules in memory for each tenant.

Refactor Ruler, introduce MultiTenantManager interface #3019

Refactor Ruler, introduce MultiTenantManager interface #3019

Conversation

annanay25 commented Aug 12, 2020 • edited Loading

pracucci left a comment

Choose a reason for hiding this comment

pstibrany left a comment

Choose a reason for hiding this comment

annanay25 commented Aug 17, 2020

pracucci left a comment

Choose a reason for hiding this comment

pstibrany Aug 17, 2020

Choose a reason for hiding this comment

pstibrany Aug 17, 2020

Choose a reason for hiding this comment

annanay25 Aug 18, 2020

Choose a reason for hiding this comment

pstibrany Aug 17, 2020

Choose a reason for hiding this comment

annanay25 Aug 18, 2020

Choose a reason for hiding this comment

annanay25 commented Aug 12, 2020 •

edited

Loading