feat: initial alerting support #1420

theSuess · 2024-02-15T12:38:17Z

This PR adds initial alerting support as described in the Alerting Support Proposal.

To keep changes small, this PR only adds the AlertRuleGroup custom resource.

This is also the first resource implemented using the new auto generated client (relevant for #1357)

theSuess · 2024-02-15T12:40:58Z

To modernize the code & API a bit, I've also performed the following actions for the new CR:

Use kubernetes conditions instead of lastMessage and NoMatchingInstances
Use finalizers instead of relying on instance status fields for cleanup

If there are reasons why these paradigms were not adopted in the first place, please let me know and I'll get this in line with other resources

controllers/client/grafana_client.go

theSuess · 2024-02-15T13:56:30Z

Waiting for grafana/grafana-openapi-client-go#75 to be merged before removing the draft status from the PR

NissesSenap · 2024-02-19T09:57:50Z

@theSuess I'm currently looking through everything, but it's not a small PR, so it takes some time ;).

RBAC

Here our CI is a bit lacking, but there is no magic to get new RBAC rules in to kustomize or helm. Sadly, you will have to copy that yaml manually.

Tests

As you have seen in the code we haven't written many tests, so I can definitely live with merging this PR without having a bunch of them, even though it of course would be ideal to add some. But it would be nice to at least add some e2e test for alerts. This way we have some kind of basic knowledge that it's working.
Take a look in https://github.com/grafana/grafana-operator/tree/master/tests/e2e/example-test for inspiration on how to set it up.

Comment

About conditions, I think we have some old issue about looking in to it, but I think we just never got that far. So I'm happy to see it used.

About finalizers, lets talk about it during our next meeting :)

lsoica · 2024-02-19T13:56:42Z

Will it be able to provision both grafana managed alerts and datasource managed alers ?

NissesSenap

A few minor comments, but in general I think it looks great.

controllers/client/grafana_client.go

controllers/grafanaalertrulegroup_controller.go

NissesSenap · 2024-02-19T15:15:00Z

I started to play around with the alertgroup and I managed to create the following issue.
To recreate, I ran sh hack/kind/start-kind.sh to create my kind cluster.
Then I added applied config/samples/grafana_v1beta1_grafanaalertrulegroup.yaml.

2024-02-19T15:28:43+01:00       ERROR   updating status {"controller": "grafanaalertrulegroup", "controllerGroup": "grafana.integreatly.org", "controllerKind": "GrafanaAlertRuleGroup", "GrafanaAlertRuleGroup": {"name":"grafanaalertrulegroup-sample","namespace":"default"}, "namespace": "default", "name": "grafanaalertrulegroup-sample", "reconcileID": "e60b64c1-74f8-4c9d-ac22-335839f2b149", "error": "Operation cannot be fulfilled on grafanaalertrulegroups.grafana.integreatly.org \"grafanaalertrulegroup-sample\": the object has been modified; please apply your changes to the latest version and try again"}
github.com/grafana/grafana-operator/v5/controllers.(*GrafanaAlertRuleGroupReconciler).Reconcile.func1
        /home/edvin/go/src/github.com/NissesSenap/grafana-operator/controllers/grafanaalertrulegroup_controller.go:102
github.com/grafana/grafana-operator/v5/controllers.(*GrafanaAlertRuleGroupReconciler).Reconcile
        /home/edvin/go/src/github.com/NissesSenap/grafana-operator/controllers/grafanaalertrulegroup_controller.go:158
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Reconcile
        /home/edvin/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.17.0/pkg/internal/controller/controller.go:119
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler
        /home/edvin/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.17.0/pkg/internal/controller/controller.go:316
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem
        /home/edvin/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.17.0/pkg/internal/controller/controller.go:266
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2
        /home/edvin/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.17.0/pkg/internal/controller/controller.go:227

Initially I thought this was related to the error I saw In grafana about the folder missing, which isn't great and we probably need some kind of check for that as well.

logger=ngalert t=2024-02-19T14:27:00.00159844Z level=error msg="failed to fetch alert rule namespace" err="folder not found" uid=4843de5c-4f8a-4af0-9509-23526a04faf8 org=1 namespace_uid=f9b0a98d-2ed3-45a6-9521-18679c74d4f1

I don't have any more time to look into why this error happens but it could also be something around datasources, it seems like Alert actually want to try to reach the datasource instead of just saying that it's there like a dashboard can live with.

Another thing I noticed was that we should probably bump our default grafana instance to 10 before releasing this feature.
If you don't have version 10, there is no export function for alerts, which makes this feature more or less impossible to use.

Another issue I faced was that I couldn't create a folder with a specific UID. Which created issues when I wanted to create the alert using the operator.

If anyone else will be playing around with folders I recommend these two curl commands.

# List all folders
curl -X GET -H Accept:application/json -H Content-Type:application/json -H "Authorization: Bearer SOOO_LONG_TOKEN" http://localhost:3000/api/folders
# Create a folder with a specific UID
curl -X POST -H Accept:application/json -H Content-Type:application/json -H "Authorization: Bearer SOOO_LONG_TOKEN" http://localhost:3000/api/folders -d '{"uid":"f9b0a98d-2ed3-45a6-9521-18679c74d4f1", "title":"foo"}'

We might need to solve the folder creation similarly as we did in dashboards, to make sure we didn't get any strange errors. Or we need to update grafanaFolder to take UID as an option so we can set it there.

theSuess · 2024-02-20T10:46:00Z

the object has been modified; please apply your changes to the latest version and try again

This happens when we try to update the status on a resource which has been changed by another client. It might be caused by adding the finalizer. I'll build a fix for this

Regarding folders: what's your take on adding a folderSelector field in the alertrulegroup? This would allow users to select a folder CR instead of having to find the UID themselves

Adding a UID to the folder spec would work as well. I don't think it makes sense to dynamically create folders for alert rule groups as multiple alert rule groups can be stored in the same folder which would require further matching logic

theSuess · 2024-02-21T16:19:15Z

Alright, most issues ironed out.

We now support a folderSelector as well as folderUID (which overrides the selector)

The only issue is that there is no way to enforce at least one field must be set without an admission hook so currently the CRD just requires the folderSelector to be set.

I've also added an e2e test & fixed the instrumentedroundtripper

NissesSenap · 2024-02-22T08:08:13Z

@theSuess could an option be to use validation rules instead of an adminsion webhook?
It was in beta in 1.25 and GA in 1.29, so I think it's reasonable that we can use it now.
https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#validation-rules

theSuess · 2024-02-22T08:24:28Z

@theSuess could an option be to use validation rules instead of an adminsion webhook? It was in beta in 1.25 and GA in 1.29, so I think it's reasonable that we can use it now. https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#validation-rules

oooh that's interesting - will see if I can get this working with kubebuilder

theSuess · 2024-02-23T13:12:56Z

Validation rules seem to work! Be careful with them though, if you get them wrong while having existing resources, it can be hard to get rid of them

theSuess · 2024-02-27T09:05:04Z

Will it be able to provision both grafana managed alerts and datasource managed alers ?

@lsoica this will only support grafana managed alerts. Datasource managed alerts are much more complex and would require a direct connection to the datasource which is out of scope for the operator.

theSuess · 2024-02-27T09:06:20Z

Regarding the folder selector: maybe we can simplify this to just be a folderRef? Selectors only make sense if it's possible to match multiple resources, which does not make sense for targeting a single folder.

Thoughts?

NissesSenap · 2024-02-28T13:56:44Z

Regarding the folder selector: maybe we can simplify this to just be a folderRef? Selectors only make sense if it's possible to match multiple resources, which does not make sense for targeting a single folder.

Thoughts?

Agree, it sounds very resonable.

theSuess · 2024-02-29T09:55:38Z

Switched over to folderRef in tests & examples now. All open topics should thus be resolved 🤞

NissesSenap

Nit

NissesSenap · 2024-03-05T12:17:11Z

controllers/grafanaalertrulegroup_controller.go

+			return ctrl.Result{}, nil
+		}
+		controllerLog.Error(err, "error getting grafana folder cr")
+		return ctrl.Result{RequeueAfter: RequeueDelay}, err


Started reading the finalizer docs https://book.kubebuilder.io/reference/using-finalizers
I wonder if we shoulden't use return ctrl.Result{}, client.IgnoreNotFound(err) instead.

controllers/grafanaalertrulegroup_controller.go

NissesSenap · 2024-03-05T15:48:01Z

I took the freedom to change a few labels and run some make commands to be more consistent with the current setup.
Added a few minor comments, but I think this looks great. I can't wait for the feature, my self.
Great job @theSuess !

HOCKNAS

Hey @theSuess @NissesSenap ,

Wow, just checked out your PR for alerting in Grafana Operator – absolutely killer work! 🚀 You guys really knocked it out of the park. Integrating alert management like this is a game-changer for us Kubernetes folks. Mad props for the effort and genius you poured into this. Can't wait to see what you hack together next!

Cheers,

pb82 · 2024-03-08T10:21:08Z

Currently testing this using the provided example. I saw that the alerting rule group got applied:

  status:
    conditions:
    - lastTransitionTime: "2024-03-08T10:16:02Z"
      message: Alert Rule Group was successfully applied to 1 instances
      observedGeneration: 3
      reason: ApplySuccesfull
      status: "True"
      type: AlertGroupSynchronized

however, in Grafana I don't see it:

Am I missing something?

this alleviates some race conditions when setting finalizers & status concurrently

this prevents invisible orphaned alert rules (and helps debugging)

Matching multiple folders does not make sense as alert rules can only be stored in one folder

this reduces load on the operator and eases deletion when no instance can be found

NissesSenap

LGTM, great job

theSuess · 2024-03-12T14:45:18Z

I've tried to implement a safeguard for alerting on older Grafana versions but was blocked by not being able to infer the version of the instance. Tracking this in #1451

theSuess commented Feb 15, 2024

View reviewed changes

controllers/client/grafana_client.go Outdated Show resolved Hide resolved

theSuess force-pushed the feat/alerting branch from 794822b to a4a7ea4 Compare February 15, 2024 12:42

NissesSenap mentioned this pull request Feb 15, 2024

Implement CRD's for Grafana 9 Alert Rules, Alert Groups, Contact Points, Notification Policies and Silences #911

Closed

theSuess force-pushed the feat/alerting branch from c68a5f4 to 797c86d Compare February 16, 2024 08:14

theSuess marked this pull request as ready for review February 16, 2024 08:20

NissesSenap reviewed Feb 19, 2024

View reviewed changes

controllers/client/grafana_client.go Outdated Show resolved Hide resolved

controllers/grafanaalertrulegroup_controller.go Outdated Show resolved Hide resolved

controllers/grafanaalertrulegroup_controller.go Show resolved Hide resolved

theSuess force-pushed the feat/alerting branch 4 times, most recently from 8e28b22 to ce00ec2 Compare February 21, 2024 16:15

theSuess requested a review from NissesSenap February 26, 2024 10:50

NissesSenap reviewed Mar 5, 2024

View reviewed changes

controllers/grafanaalertrulegroup_controller.go Show resolved Hide resolved

controllers/grafanaalertrulegroup_controller.go Outdated Show resolved Hide resolved

HOCKNAS approved these changes Mar 7, 2024

View reviewed changes

theSuess and others added 19 commits March 12, 2024 14:38

fix: ignore status updates on alertrulegroups

5a106ff

this alleviates some race conditions when setting finalizers & status concurrently

feat: add folderSelector to grafanaalertrulegroup

e3e12d7

fix: ignore finalization when folder no longer exists

b44d7c7

test: add e2e test for alertrulegroups

ab8f4ff

fix: add linting exception for insecure tls

bc64dc1

fix: use constructor for instrumented roundtripper

3fc7edc

fix: check if folder exists before creation

4076eb8

this prevents invisible orphaned alert rules (and helps debugging)

feat: use validation rules to enforce exclusivity

4bcee25

docs: add docs for alert rule groups

e5aef4b

docs: add section on finalizers

991fede

refactor: use folderRef instead of folderSelector

8744119

Matching multiple folders does not make sense as alert rules can only be stored in one folder

feat: add AllowCrossNamespace parameter

20c4de2

fix: use folderRef in examples

4287d88

fix: add generated deepcopy

c67b1cb

make consistent with other rbac

e0deca0

run make bundle/redhat to have less changes

35aa1b6

remove the last labels

b00b1be

refactor: remove obsolete function

4869fb3

feat: remove finalizer when no instances match

40e32cb

this reduces load on the operator and eases deletion when no instance can be found

theSuess force-pushed the feat/alerting branch from 8226d13 to 40e32cb Compare March 12, 2024 13:49

theSuess requested review from weisdd, ishanjainn and hubeadmin as code owners March 12, 2024 13:49

NissesSenap approved these changes Mar 12, 2024

View reviewed changes

theSuess merged commit 68801e9 into master Mar 12, 2024
10 checks passed

theSuess deleted the feat/alerting branch March 12, 2024 14:42

This was referenced Mar 13, 2024

Add support for Notification Policies #1454

Closed

Add support for Contact Points #1455

Closed

duncan485 mentioned this pull request Mar 26, 2024

Add contactpoint resource #1474

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: initial alerting support #1420

feat: initial alerting support #1420

theSuess commented Feb 15, 2024

theSuess commented Feb 15, 2024

theSuess commented Feb 15, 2024

NissesSenap commented Feb 19, 2024

lsoica commented Feb 19, 2024

NissesSenap left a comment

NissesSenap commented Feb 19, 2024

theSuess commented Feb 20, 2024

theSuess commented Feb 21, 2024

NissesSenap commented Feb 22, 2024

theSuess commented Feb 22, 2024

theSuess commented Feb 23, 2024

theSuess commented Feb 27, 2024

theSuess commented Feb 27, 2024

NissesSenap commented Feb 28, 2024

theSuess commented Feb 29, 2024

NissesSenap left a comment

NissesSenap Mar 5, 2024

NissesSenap commented Mar 5, 2024

HOCKNAS left a comment

pb82 commented Mar 8, 2024

NissesSenap left a comment

theSuess commented Mar 12, 2024

feat: initial alerting support #1420

feat: initial alerting support #1420

Conversation

theSuess commented Feb 15, 2024

theSuess commented Feb 15, 2024

theSuess commented Feb 15, 2024

NissesSenap commented Feb 19, 2024

RBAC

Tests

Comment

lsoica commented Feb 19, 2024

NissesSenap left a comment

Choose a reason for hiding this comment

NissesSenap commented Feb 19, 2024

theSuess commented Feb 20, 2024

theSuess commented Feb 21, 2024

NissesSenap commented Feb 22, 2024

theSuess commented Feb 22, 2024

theSuess commented Feb 23, 2024

theSuess commented Feb 27, 2024

theSuess commented Feb 27, 2024

NissesSenap commented Feb 28, 2024

theSuess commented Feb 29, 2024

NissesSenap left a comment

Choose a reason for hiding this comment

NissesSenap Mar 5, 2024

Choose a reason for hiding this comment

NissesSenap commented Mar 5, 2024

HOCKNAS left a comment

Choose a reason for hiding this comment

pb82 commented Mar 8, 2024

NissesSenap left a comment

Choose a reason for hiding this comment

theSuess commented Mar 12, 2024