operator: Add support for Loki rules reconciliation #5986

periklis · 2022-04-21T13:01:37Z

What this PR does / why we need it:
This PR is first-shot implementation for the proposed enhancement: #5985

Which issue(s) this PR fixes:
Implements #5985, based on/replacing #5188

Special notes for your reviewer:

Checklist

Documentation added
Tests updated
Is this an important fix or new feature? Add an entry in the CHANGELOG.md.
Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

cyriltovena · 2022-05-05T12:31:40Z

Went through it all where is the rules API reconciliation with the ruler ?

periklis · 2022-05-05T13:16:23Z

Went through it all where is the rules API reconciliation with the ruler ?

You mean the RulerConfig reconciliation for AlertManager and RemoteWrite? If yes this will be a separate PR. This one is focusing on AlertingRule and RecordingRule.

xperimental

I'm about half-way through the content of the PR, but here's the comments so far.

operator/internal/status/components.go

operator/internal/status/status.go

operator/PROJECT

operator/api/v1beta1/alertingrule_types.go

operator/api/v1beta1/alertingrule_webhook.go

operator/config/default/manager_webhook_patch.yaml

xperimental · 2022-05-13T18:07:48Z

operator/Makefile

@@ -103,7 +103,7 @@ help: ## Display this help.

 .PHONY: deps
 deps: go.mod go.sum
-	go mod tidy
+	go mod tidy -compat=1.17


I have a really bad feeling about the error message that go mod tidy is outputting when used without the compat option. Maybe we need to discuss splitting out the API dependency as a separate module from Loki?

The problem here is our new dependency against Loki itself, in particular we reference:

github.com/grafana/loki/operator/internal/manifests/internal/rules imports gopkg.in/yaml.v2 tested by gopkg.in/yaml.v2.test imports gopkg.in/check.v1 loaded from gopkg.in/check.v1@v1.0.0-20200902074654-038fdea0a05b, but go 1.16 would select v1.0.0-20201130134442-10cb98267c6c github.com/grafana/loki/operator/api/v1beta1 imports github.com/grafana/loki/pkg/logql/syntax imports github.com/grafana/loki/pkg/util imports github.com/grafana/dskit/kv imports github.com/grafana/dskit/kv/consul imports github.com/hashicorp/consul/api loaded from github.com/hashicorp/consul/api@v1.12.0, but go 1.16 would fail to locate it: ambiguous import: found package github.com/hashicorp/consul/api in multiple modules: github.com/hashicorp/consul v1.2.1 (/home/periklis/.gvm/pkgsets/go1.17.8/global/pkg/mod/github.com/hashicorp/consul@v1.2.1/api) github.com/hashicorp/consul/api v1.12.0 (/home/periklis/.gvm/pkgsets/go1.17.8/global/pkg/mod/github.com/hashicorp/consul/api@v1.12.0) To upgrade to the versions selected by go 1.16, leaving some packages unresolved: go mod tidy -e -go=1.16 && go mod tidy -e -go=1.17 If reproducibility with go 1.16 is not needed: go mod tidy -compat=1.17 For other options, see: https://golang.org/doc/modules/pruning make: *** [Makefile:106: deps] Error 1

Requires a patch into loki go.mod to resolve this issue.

operator/hack/lokistack_gateway_ocp.yaml

operator/internal/manifests/mutate.go

grafanabot · 2022-05-16T08:51:36Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
-        distributor	-0.3%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

xperimental

And another round, still not fully through the files (61/93). The good news is that I combined it with actually running the new version of the operator on a testing cluster.

Generally I have the feeling that there is some "de-duplication" of the alertingrule/recordingrule code possible, maybe by introducing some interfaces?

I'll continue tomorrow.

operator/api/v1beta1/recordingrule_types.go

xperimental · 2022-05-17T15:59:40Z

operator/api/v1beta1/alertingrule_types.go

+}
+
+// AlertingRuleGroup defines a group of Loki alerting rules.
+type AlertingRuleGroup struct {


The result I envisioned was having the checks that are now duplicated in both the webhook for alerting and recording rules only in one place (duplicate group names, durations, etc.). Don't know if the embedded struct is the choice for reaching that goal, there are a few alternatives... in the end it's also the question if the effort is worth it.

operator/config/overlays/openshift/kustomization.yaml

operator/hack/lokirules_ocp.yaml

operator/hack/lokistack_gateway_ocp.yaml

operator/controllers/recordingrule_controller.go

operator/internal/handlers/internal/rules/rules.go

operator/internal/manifests/config.go

operator/internal/manifests/distributor.go

operator/internal/manifests/internal/gateway/options.go

grafanabot · 2022-05-18T12:05:43Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

operator/bundle/manifests/loki-operator.clusterserviceversion.yaml

operator/config/samples/loki_v1beta1_alertingrule.yaml

operator/config/samples/loki_v1beta1_recordingrule.yaml

operator/controllers/lokistack_controller.go

grafanabot · 2022-05-23T13:32:17Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

grafanabot · 2022-05-23T13:37:11Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0.3%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

xperimental

Finally through all the files. 🎉

operator/internal/manifests/internal/rules/marshal.go

operator/internal/manifests/ruler.go

operator/internal/manifests/rules_config.go

grafanabot · 2022-05-23T17:33:54Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
-               loki	-0.7%

grafanabot · 2022-05-24T04:56:17Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

periklis added type/feature Something new we should do sig/operator labels Apr 21, 2022

periklis self-assigned this Apr 21, 2022

periklis requested a review from a team as a code owner April 21, 2022 13:01

pull-request-size bot added the size/XXL label Apr 21, 2022

periklis force-pushed the ruler-support branch 7 times, most recently from 50d749b to 04f4a2c Compare April 26, 2022 09:29

periklis changed the title ~~operator: Add basic support for Loki ruler and rules reconciliation~~ operator: Add basic support for Loki rules reconciliation Apr 26, 2022

periklis force-pushed the ruler-support branch 2 times, most recently from da96af8 to a31504b Compare April 26, 2022 13:17

periklis changed the title ~~operator: Add basic support for Loki rules reconciliation~~ operator: Add support for Loki rules reconciliation Apr 26, 2022

periklis force-pushed the ruler-support branch 5 times, most recently from f05bcd1 to 206d4a9 Compare May 3, 2022 11:59

github-actions bot removed the type/feature Something new we should do label May 4, 2022

xperimental reviewed May 13, 2022

View reviewed changes

xperimental reviewed May 17, 2022

View reviewed changes

periklis mentioned this pull request May 19, 2022

operator: Add ruler config support #6195

Merged

4 tasks

xperimental reviewed May 23, 2022

View reviewed changes

Add rules support

94b8f2d

periklis force-pushed the ruler-support branch from 73da8c7 to 94b8f2d Compare May 24, 2022 04:51

periklis enabled auto-merge (squash) May 24, 2022 04:52

xperimental approved these changes May 24, 2022

View reviewed changes

periklis merged commit fd627f2 into grafana:main May 24, 2022

This was referenced Jun 28, 2022

dannykopping/remove cache stats dannykopping/loki#13

Closed

dannykopping/remove cache stats dannykopping/loki#14

Draft

Red-GV mentioned this pull request Aug 8, 2022

operator: Add support for ruler component #5211

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

operator: Add support for Loki rules reconciliation #5986

operator: Add support for Loki rules reconciliation #5986

periklis commented Apr 21, 2022 •

edited

Loading

cyriltovena commented May 5, 2022

periklis commented May 5, 2022

xperimental left a comment

xperimental May 13, 2022

periklis May 16, 2022

periklis May 23, 2022

grafanabot commented May 16, 2022

xperimental left a comment

xperimental May 17, 2022

grafanabot commented May 18, 2022

grafanabot commented May 23, 2022

grafanabot commented May 23, 2022

xperimental left a comment

grafanabot commented May 23, 2022

grafanabot commented May 24, 2022

operator: Add support for Loki rules reconciliation #5986

operator: Add support for Loki rules reconciliation #5986

Conversation

periklis commented Apr 21, 2022 • edited Loading

cyriltovena commented May 5, 2022

periklis commented May 5, 2022

xperimental left a comment

Choose a reason for hiding this comment

xperimental May 13, 2022

Choose a reason for hiding this comment

periklis May 16, 2022

Choose a reason for hiding this comment

periklis May 23, 2022

Choose a reason for hiding this comment

grafanabot commented May 16, 2022

xperimental left a comment

Choose a reason for hiding this comment

xperimental May 17, 2022

Choose a reason for hiding this comment

grafanabot commented May 18, 2022

grafanabot commented May 23, 2022

grafanabot commented May 23, 2022

xperimental left a comment

Choose a reason for hiding this comment

grafanabot commented May 23, 2022

grafanabot commented May 24, 2022

periklis commented Apr 21, 2022 •

edited

Loading