pkg/operator: create Select method for rule selection #5221

slashpai · 2022-12-12T14:13:14Z

Signed-off-by: Jayapriya Pai slashpai9@gmail.com

Description

Describe the big picture of your changes here to communicate to the maintainers why we should accept this pull request.
If it fixes a bug or resolves a feature request, be sure to link to that issue.

Type of change

What type of changes does your code introduce to the Prometheus operator? Put an x in the box that apply.

CHANGE (fix or feature that would cause existing functionality to not work as expected)
FEATURE (non-breaking change which adds functionality)
BUGFIX (non-breaking change which fixes an issue)
ENHANCEMENT (non-breaking change which improves existing functionality)
NONE (if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)

Changelog entry

Please put a one-line changelog entry below. This will be copied to the changelog file during the release process.

Update logic on selecting prometheus rules by rejecting invalid ones instead of failing reconciliation

slashpai · 2022-12-12T14:17:33Z

This is not yet done for Thanos, still working on updating PR and removing duplicates hence moving to draft :)

slashpai · 2022-12-21T12:59:40Z

Removing draft for initial review

pkg/prometheus/rules.go

JoaoBraveCoding

lgtm 👍

slashpai · 2022-12-21T16:29:25Z

Need to implement for Thanos as well. I will look in reducing duplicate code between Thanos and Prometheus

slashpai · 2023-01-03T09:19:22Z

@simonpasquier Can you take another look at this?

simonpasquier

Can we factorize the 2 selectRules() methods (probably in pkg/operator)?

pkg/operator/rules.go

slashpai · 2023-01-18T12:18:50Z

@simonpasquier PTAL when you get a chance :)

pkg/operator/rules.go

slashpai · 2023-01-19T06:53:06Z

I updated with changes suggested, need to remove a few units as well. Updating that as well

simonpasquier

just a few nits :)

pkg/thanos/rules.go

pkg/operator/rules.go

slashpai · 2023-01-20T06:41:04Z

@simonpasquier Addressed all comments except the nsLabeler (added comment in line)

slashpai · 2023-01-20T06:41:54Z

pkg/operator/rules_test.go

 	if !strings.Contains(content, "partial_response_strategy: warn") {
 		t.Fatalf("expected `partial_response_strategy` to be set in PrometheusRule as `warn`")

 	}
 }
+


These are tests moved from Prometheus package

JoaoBraveCoding

Sweet refactoring 👍 Great job JP!

simonpasquier

lgtm but you'll have to rebase on main :)

slashpai · 2023-01-24T12:13:17Z

lgtm but you'll have to rebase on main :)

@simonpasquier rebased

simonpasquier · 2023-01-24T14:25:40Z

I realize that we don't have an e2e test verifying that the operator doesn't select invalid rules now. As the validating webhook config watches for all namespaces, an invalid rule can never reach the operator. I'd suggest to tweak the namespaceSelector field of the webhook configuration to exclude namespaces with a certain label:

namespaceSelector:
  matchExpressions:
  - key: excludeFromWebhook
    operator: NotIn
    values: ["true"]

prometheus-operator/test/framework/resources/prometheus-operator-validatingwebhook.yaml

Line 13 in cb272a6

namespaceSelector: {}

Then a rule created in a namespace with the excludeFromWebhook: "true" would skip the validation and we can check that the operator discards it.

test/e2e/rules_test.go

slashpai · 2023-03-09T11:43:09Z

test/e2e/prometheus_test.go

+	defer testCtx.Cleanup(t)
+	ns := framework.CreateNamespace(context.Background(), t, testCtx)
+	framework.SetupPrometheusRBAC(context.Background(), t, testCtx, ns)
+	ruleFilesNamespaceSelector := map[string]string{"excludeFromWebhook": "true", "role": "rulefile"}


Note on labels added:

"role": "rulefile" label is added here so that operator selects the rule created for evaulation. We use this label in ruleSelector in framework.MakeBasicPrometheus() as well as framework..MakeBasicRule()

"excludeFromWebhook": "true" is added to skip webhook validation and validate if operator doesn't fail reconcilation due to invalid rule

slashpai · 2023-03-09T11:45:10Z

@simonpasquier Rebased and updated the e2e test to check prometheus reconciliation status after invalid rule created

test/e2e/prometheus_test.go

slashpai · 2023-03-10T05:51:29Z

Just for more validation I tested this change manually as well checked the e2e test against main branch on which refactoring is not there. Prometheus fails to reconcile without this change. Also the metric prometheus_operator_managed_resources{controller="prometheus",resource="PrometheusRule",state="rejected"} 1 is updated correctly

slashpai · 2023-03-10T05:51:59Z

@simonpasquier Can you take another look at this one, updated test :)

reorganized rule selection code from prometheus and thanos package to reduce duplicate code. This change will also make sure there is no reconcillation failure on controller when a bad PrometheusRule object is created. Instead of a reconcillation failure, controller will just reject the bad PrometheusRule. Fixes prometheus-operator#5191 Signed-off-by: Jayapriya Pai <slashpai9@gmail.com>

simonpasquier · 2023-03-10T09:41:08Z

Thanks!

slashpai requested a review from a team as a code owner December 12, 2022 14:13

slashpai marked this pull request as draft December 12, 2022 14:17

slashpai marked this pull request as ready for review December 21, 2022 12:59

simonpasquier reviewed Dec 21, 2022

View reviewed changes

pkg/prometheus/rules.go Outdated Show resolved Hide resolved

slashpai force-pushed the refactor_rule_selection branch from f1ad186 to 9f21d50 Compare December 21, 2022 14:36

JoaoBraveCoding reviewed Dec 21, 2022

View reviewed changes

pkg/prometheus/rules.go Outdated Show resolved Hide resolved

slashpai force-pushed the refactor_rule_selection branch from 9f21d50 to 001ddce Compare December 21, 2022 15:58

JoaoBraveCoding approved these changes Dec 21, 2022

View reviewed changes

slashpai force-pushed the refactor_rule_selection branch from 001ddce to 535734b Compare December 27, 2022 06:40

simonpasquier reviewed Jan 4, 2023

View reviewed changes

simonpasquier mentioned this pull request Jan 17, 2023

Reject invalid PrometheusRule objects beforehand rather than failing the reconciliation #5191

Closed

slashpai force-pushed the refactor_rule_selection branch from 71e6b7a to 2761903 Compare January 18, 2023 10:32

slashpai commented Jan 18, 2023

View reviewed changes

pkg/operator/rules.go Outdated Show resolved Hide resolved

slashpai force-pushed the refactor_rule_selection branch from 2761903 to 0c47b7e Compare January 18, 2023 10:36

simonpasquier reviewed Jan 18, 2023

View reviewed changes

pkg/operator/rules.go Outdated Show resolved Hide resolved

pkg/operator/rules.go Outdated Show resolved Hide resolved

pkg/operator/rules.go Outdated Show resolved Hide resolved

pkg/operator/rules.go Outdated Show resolved Hide resolved

slashpai force-pushed the refactor_rule_selection branch from 0c47b7e to 9636cd0 Compare January 19, 2023 06:36

slashpai changed the title ~~pkg/prometheus: Update selectRules method to be robust~~ pkg/operator: create Select method for rule selection Jan 19, 2023

slashpai commented Jan 19, 2023

View reviewed changes

pkg/operator/rules.go Outdated Show resolved Hide resolved

simonpasquier reviewed Jan 19, 2023

View reviewed changes

pkg/thanos/rules.go Show resolved Hide resolved

pkg/operator/rules.go Outdated Show resolved Hide resolved

pkg/operator/rules.go Outdated Show resolved Hide resolved

pkg/operator/rules.go Outdated Show resolved Hide resolved

pkg/operator/rules.go Outdated Show resolved Hide resolved

slashpai force-pushed the refactor_rule_selection branch 2 times, most recently from d0c112f to df1eefe Compare January 20, 2023 06:38

slashpai commented Jan 20, 2023

View reviewed changes

JoaoBraveCoding approved these changes Jan 20, 2023

View reviewed changes

simonpasquier reviewed Jan 23, 2023

View reviewed changes

slashpai force-pushed the refactor_rule_selection branch from df1eefe to 98a2f7f Compare January 24, 2023 12:10

simonpasquier approved these changes Jan 24, 2023

View reviewed changes

slashpai force-pushed the refactor_rule_selection branch 2 times, most recently from 06ac5ac to 7a66710 Compare January 30, 2023 06:33

simonpasquier reviewed Jan 30, 2023

View reviewed changes

test/e2e/rules_test.go Outdated Show resolved Hide resolved

slashpai force-pushed the refactor_rule_selection branch 4 times, most recently from 0af5c62 to d522534 Compare March 9, 2023 11:34

slashpai commented Mar 9, 2023

View reviewed changes

simonpasquier reviewed Mar 9, 2023

View reviewed changes

test/e2e/prometheus_test.go Outdated Show resolved Hide resolved

slashpai force-pushed the refactor_rule_selection branch from d522534 to dd18cc4 Compare March 10, 2023 05:03

slashpai force-pushed the refactor_rule_selection branch from dd18cc4 to aa92da4 Compare March 10, 2023 09:02

simonpasquier approved these changes Mar 10, 2023

View reviewed changes

simonpasquier enabled auto-merge March 10, 2023 09:41

simonpasquier merged commit 16dfbf4 into prometheus-operator:main Mar 10, 2023

sastorsl mentioned this pull request Mar 30, 2023

Reject invalid PrometheusRule objects beforehand rather than failing the reconciliation openshift/cluster-monitoring-operator#1928

Closed

slashpai deleted the refactor_rule_selection branch September 9, 2023 04:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/operator: create Select method for rule selection #5221

pkg/operator: create Select method for rule selection #5221

slashpai commented Dec 12, 2022

slashpai commented Dec 12, 2022

slashpai commented Dec 21, 2022

JoaoBraveCoding left a comment

slashpai commented Dec 21, 2022

slashpai commented Jan 3, 2023

simonpasquier left a comment

slashpai commented Jan 18, 2023

slashpai commented Jan 19, 2023

simonpasquier left a comment

slashpai commented Jan 20, 2023

slashpai Jan 20, 2023

JoaoBraveCoding left a comment

simonpasquier left a comment

slashpai commented Jan 24, 2023

simonpasquier commented Jan 24, 2023

slashpai Mar 9, 2023

slashpai commented Mar 9, 2023

slashpai commented Mar 10, 2023

slashpai commented Mar 10, 2023

simonpasquier commented Mar 10, 2023

pkg/operator: create Select method for rule selection #5221

pkg/operator: create Select method for rule selection #5221

Conversation

slashpai commented Dec 12, 2022

Description

Type of change

Changelog entry

slashpai commented Dec 12, 2022

slashpai commented Dec 21, 2022

JoaoBraveCoding left a comment

Choose a reason for hiding this comment

slashpai commented Dec 21, 2022

slashpai commented Jan 3, 2023

simonpasquier left a comment

Choose a reason for hiding this comment

slashpai commented Jan 18, 2023

slashpai commented Jan 19, 2023

simonpasquier left a comment

Choose a reason for hiding this comment

slashpai commented Jan 20, 2023

slashpai Jan 20, 2023

Choose a reason for hiding this comment

JoaoBraveCoding left a comment

Choose a reason for hiding this comment

simonpasquier left a comment

Choose a reason for hiding this comment

slashpai commented Jan 24, 2023

simonpasquier commented Jan 24, 2023

slashpai Mar 9, 2023

Choose a reason for hiding this comment

slashpai commented Mar 9, 2023

slashpai commented Mar 10, 2023

slashpai commented Mar 10, 2023

simonpasquier commented Mar 10, 2023