disable rule groups #5521

rajagopalanand · 2023-08-21T23:17:38Z

What this PR does:

Which issue(s) this PR fixes:
Fixes #

Checklist

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

yeya24 · 2023-08-21T23:37:10Z

pkg/ruler/ruler.go

@@ -187,6 +197,8 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) {
 	f.BoolVar(&cfg.EnableQueryStats, "ruler.query-stats-enabled", false, "Report the wall time for ruler queries to complete as a per user metric and as an info level log message.")
 	f.BoolVar(&cfg.DisableRuleGroupLabel, "ruler.disable-rule-group-label", false, "Disable the rule_group label on exported metrics")

+	f.Var(&cfg.DisabledRuleGroups, "ruler.disabled-rule-groups", "Comma separated list of keys of rule groups this ruler cannot evaluate. If specified, a ruler that would normally pick the specified rule groups for processing will ignore them instead.")


Should we keep the disabled rule groups list in runtime config? It is more flexible

pkg/ruler/ruler_test.go

pkg/util/validation/limits.go

yeya24

Very nice pr. I think what I want to see is one more integration test for this feature

yeya24 · 2023-08-31T04:12:07Z

pkg/ruler/ruler.go

+			if !ruleGroupDisabled(group, disabledRuleGroupsForUser) {
+				filteredGroupsForUser = append(filteredGroupsForUser, group)
+			} else {
+				level.Info(r.logger).Log("msg", "rule group disabled", "rule group name", group.Name, "namespace", group.Namespace, "user", group.User)


Shall we put this as debug level? Wondering if it is really necessary to make info.

Can simplify rule group name to just name

i was thinking that we should do info though, we should have the log to see if we are indeed skip the rule evaluation

You can see the log with debug level as well as long as you change log level.
Can we tell some rule group is disabled by metrics? We dont have to use logs

I think I am good. Let's keep this as info for now…

yeya24 · 2023-08-31T04:14:01Z

pkg/ruler/ruler.go

-			continue
+			switch e := err.(type) {
+			case *DisabledRuleGroupErr:
+				level.Info(log).Log("msg", e.Message)


Is it too verbose to log it every time during filtering? And this doesn't feel like an error to me

To give visibility, I think it needs to be either Info or Warn

alanprot · 2023-08-31T22:29:06Z

pkg/util/validation/limits.go

+			return disabledRuleGroupsForUser
+		}
+	}
+	return DisabledRuleGroups{}


Should we return the default limits here? I think someone may wanna set this on the config (instead runtimeconfig) as well.

I went back and forth on this one. Since user would be required to disable the rule groups to avoid accidentally disabling rule groups from other users that happen to have the same name and namespace, I thought it might be better to allow this config only from runtime config. If we want to allow config, then we need to have user field exposed here so it can be set. If we did that, then someone can configure user under runtime config which is already a map of [user:overrides] which can be confusing

alanprot · 2023-08-31T22:31:45Z

pkg/ruler/ruler.go

+		if len(disabledRuleGroupsForUser) == 0 {
+			continue
+		}


We could do this check right in the beginning of the method and return the original allRuleGroups value if the len is 0, right?

We have to loop thru and check if there are overrides for each user, correct?

oh my bad... this is for all users! ok

emanlodovice · 2023-09-05T17:27:53Z

pkg/ruler/ruler.go

+		if len(disabledRuleGroupsForUser) == 0 {
+			continue
+		}
+		filteredGroupsForUser := rulespb.RuleGroupList{}


Nit: should we use make here to since we have an idea how many elements there will be

We won't know how many rule groups are disabled until the loop is complete

emanlodovice · 2023-09-05T17:37:17Z

pkg/ruler/ruler.go

-			continue
+			switch e := err.(type) {
+			case *DisabledRuleGroupErr:
+				level.Info(log).Log("msg", e.Message)


In this case, all the instance will log this right even if that instance doesn't own the group? I think this will produce too much logs for large clusters.

doesn't seems to be an error to me +1

qinxx108 · 2023-09-06T17:00:32Z

docs/configuration/config-file-reference.md

@@ -3093,6 +3093,9 @@ The `limits_config` configures default and per-tenant limits imposed by Cortex s
 # alerts will fail with a log message and metric increment. 0 = no limit.
 # CLI flag: -alertmanager.max-alerts-size-bytes
 [alertmanager_max_alerts_size_bytes: <int> | default = 0]
+
+# list of rule groups to disable
+[disabled_rule_groups: <list of rule groups to disable> | default = ]


should default be []?

Can we document this format somehow?

We have others "complex configs" like https://cortexmetrics.io/docs/configuration/configuration-file/#memcached_config

Will submit another PR for this. Existing doc generator does not handle slices of structs well

yeya24

Good work!

alanprot · 2023-09-08T16:21:29Z

pkg/ruler/ruler.go

-	return rlrs.Instances[0].Addr == instanceAddr, nil
+	ownsRuleGroup := rlrs.Instances[0].Addr == instanceAddr
+	if ownsRuleGroup && ruleGroupDisabled(g, disabledRuleGroups) {
+		return false, &DisabledRuleGroupErr{Message: fmt.Sprintf("rule group %s, namespace %s, user %s is disabled", g.Name, g.Namespace, g.User)}


nit:

Should this return an error? i see why u did that (u wanna catch on the caller layer) but is weird to return an error on something expected.

But im ok with it.

Agree this is a little bit weird... But seems like a workaround here as Go doesn't have a good way to capture such things...

Signed-off-by: Anand Rajagopal <[email protected]>

pull-request-size bot added the size/L label Aug 21, 2023

yeya24 reviewed Aug 21, 2023

View reviewed changes

rajagopalanand force-pushed the disable-rule-groups branch 4 times, most recently from 46e7185 to fdbb9cb Compare August 23, 2023 00:55

pull-request-size bot added size/M and removed size/L labels Aug 23, 2023

rajagopalanand force-pushed the disable-rule-groups branch 6 times, most recently from b3f858b to 7075adc Compare August 29, 2023 23:07

rajagopalanand force-pushed the disable-rule-groups branch 2 times, most recently from 2c6a3e4 to be3d241 Compare August 30, 2023 19:57

pull-request-size bot added size/L and removed size/M labels Aug 30, 2023

rajagopalanand force-pushed the disable-rule-groups branch 2 times, most recently from 91651bc to 6864bc9 Compare August 30, 2023 20:17

rajagopalanand marked this pull request as ready for review August 30, 2023 20:22

yeya24 reviewed Aug 30, 2023

View reviewed changes

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

pkg/util/validation/limits.go Outdated Show resolved Hide resolved

rajagopalanand force-pushed the disable-rule-groups branch 3 times, most recently from e4eba59 to 80add1d Compare August 30, 2023 21:44

rajagopalanand requested review from alanprot and yeya24 August 30, 2023 21:57

yeya24 reviewed Aug 31, 2023

View reviewed changes

alanprot reviewed Aug 31, 2023

View reviewed changes

emanlodovice reviewed Sep 5, 2023

View reviewed changes

qinxx108 reviewed Sep 6, 2023

View reviewed changes

rajagopalanand force-pushed the disable-rule-groups branch 2 times, most recently from e327544 to 47cc7c4 Compare September 6, 2023 21:49

rajagopalanand requested review from alanprot and yeya24 September 6, 2023 23:44

yeya24 approved these changes Sep 7, 2023

View reviewed changes

emanlodovice approved these changes Sep 7, 2023

View reviewed changes

rajagopalanand force-pushed the disable-rule-groups branch from 47cc7c4 to 622c96d Compare September 7, 2023 21:11

alanprot reviewed Sep 8, 2023

View reviewed changes

alanprot approved these changes Sep 8, 2023

View reviewed changes

rajagopalanand force-pushed the disable-rule-groups branch from 622c96d to 15dbc70 Compare September 8, 2023 16:33

disable rule groups

6fa6f92

Signed-off-by: Anand Rajagopal <[email protected]>

rajagopalanand force-pushed the disable-rule-groups branch from 15dbc70 to 6fa6f92 Compare September 8, 2023 17:33

alanprot merged commit 39be93d into cortexproject:master Sep 8, 2023

disable rule groups #5521

disable rule groups #5521

Uh oh!

Conversation

rajagopalanand commented Aug 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

yeya24 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!

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!

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!

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!

yeya24 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!

Uh oh!

rajagopalanand commented Aug 21, 2023 •

edited

Loading