Add AlertmanagerConfig CRD #3451

grdryn · 2020-08-27T17:11:52Z

This builds upon @brancz's work in #3265 and will hopefully eventually implement the core of the proposal at https://docs.google.com/document/d/1aVVttvocop8zNezwrNFbHh_HKRW_fhJUZqhmrr8fGK0/edit?usp=sharing

Closes #3261, #2766, #2927, #1528.

s-urbaniak · 2020-08-31T10:40:16Z

@grdryn Is this ready for final review? The build failing and I think we should add some more e2e tests here too. Let us know if you need further assistance :)

grdryn · 2020-08-31T11:14:33Z

@s-urbaniak no, this is not ready for review yet. There's still a lot to do! 🙂 I think GH added reviewers automatically.

I will indeed be in touch if/when I need help, thanks! 🙂

grdryn

@simonpasquier @s-urbaniak @brancz This currently "works" for the CRD type that exists so far (i.e. only the PagerDuty receiver type is implemented, but others should be easy to add after). Probably some tests need to be fixed up & some more added, I'll look in that direction next.

Could you please take a look when you have some time, to see if what I've done so far is generally in the right direction, or if there are things you'd like changed? It's very much inspired by how the prometheus controller handles ServiceMonitor (etc) CRs, and in many cases blatently plagiarised. At least that should make it somewhat familiar! 🙂

pkg/alertmanager/operator.go

pkg/apis/monitoring/v1/alertmanager_config_types.go

pkg/alertmanager/amcfg.go

grdryn · 2020-09-09T17:48:23Z

Existing tests appear to work now! 🎉 However, the prometheus e2e tests are failing in travis with:

The job exceeded the maximum time limit for jobs, and has been terminated.

Is that a known issue, or is it caused by the changes here? I don't think these changes should affect those prometheus controller e2e tests that much 🤷‍♂️

scripts/generate-crds.go

example/prometheus-operator-crd/monitoring.coreos.com_alertmanagerconfigs.yaml

simonpasquier

Thanks a lot for doing this! I've made a first limited review. I'll add more later.

pkg/alertmanager/amcfg.go

pkg/alertmanager/operator.go

grdryn · 2020-09-22T20:03:13Z

I'm going to remove the WIP prefix for this PR now, as I think it's close to a state that could be merged. Only the PagerDuty receiver has been added so far. I can add the others here, or I can do them in follow-up changes if you'd prefer.

lilic · 2020-09-23T07:59:10Z

Only the PagerDuty receiver has been added so far. I can add the others here, or I can do them in follow-up changes if you'd prefer.

I am fine in a follow-up as long as you can open the issues for maybe each one would be a good one for outside contributors to take, but up to @simonpasquier and @brancz on that one.

lilic

Great work on the PR! Thanks so much! Couple of comments, leaving up to Frederic and Simon to have a final look, no blockers on my side.

pkg/alertmanager/amcfg.go

lilic · 2020-09-23T08:13:15Z

pkg/alertmanager/operator.go

+	}
+}
+
+// TODO: Do we need to enque secrets just for the namespace or in general?


Is this TODO still valid before we merge the PR?

I plagiarized this TODO when I plagiarised this function from here 🙈

prometheus-operator/pkg/prometheus/operator.go

Line 891 in 9a0823d

// TODO: Do we need to enque secrets just for the namespace or in general?

I could be wrong, but here's how I see it:

We should enqueue reconciliation for an Alertmanager if any of the Secrets referenced in its spec are deleted (it has a secrets field which is a list, configSecret field, containers or initContainers fields may reference a Secret to mount, etc). These all have to be in the same namespace as the Alertmanager. This function, enqueuing all Alertmanagers in the namespace, covers these cases, I believe.

We may want to also enqueue reconciliation for an Alertmanager, if there's an AlertmanagerConfig selected by it that references a Secret that's deleted (upon reconciliation, if the Secret is missing for an AlertmanagerConfig, then that AlertmanagerConfig will be excluded when building the config file). If that's what we want, then I think it would either mean enqueuing all Alertmanagers in all namespaces, for any Secret changes in any AlertmanagerConfig namespaces; or determining if an Alertmanager should be reconciled by checking if the Secret is referenced in either the Alertmanager spec, or in any AlertmanagerConfigs that it selects.

I'm starting to confuse myself. In any case, if we want the second option in addition to the first, I'm not quite sure how to achieve that here. 🤔 Thoughts?

I need to dig deeper into this.

pkg/apis/monitoring/v1beta1/doc.go

dgrisonnet

Awesome! Just a few comments, but it looks really good.

pkg/alertmanager/amcfg_test.go

pkg/alertmanager/asset_store_test.go

pkg/alertmanager/asset_store.go

test/e2e/alertmanager_test.go

brancz · 2020-09-23T08:55:34Z

I won't get to doing a deep dive on this soon, so it's better for @simonpasquier to do so. Two general comments:

Yes I think starting with just one provider is fine, the rest is pretty mechanical and can be done in follow up PRs.
Let's make sure we start with a v1alpha1 API :)

simonpasquier

A few comments for the Alertmanager controller. Otherwise this is looking really good. Given the amount of the code changes + the fact that the resource is v1alpha1, 👍 for merging this after addressing my last remarks.

simonpasquier · 2020-10-09T14:56:08Z

pkg/alertmanager/operator.go

+	}
+}
+
+func (c *Operator) enqueueForMonitorNamespace(nsName string) {


I don't think we need the intermediate enqueueForMonitorNamespace() function. We can have only func (c *Operator) enqueueForNamespace(nsName string) which would use the c.nsAlrtCfgInf's store.

Sure, I've done that now. I have to admit to being less familiar with this part of the changes, I think it comes from branczs earlier WIP.

I completely understand, you've done an amazing work knowing that you didn't know extensively the code base before :)

simonpasquier · 2020-10-09T14:57:07Z

pkg/alertmanager/operator.go

+	}
+}
+
+// TODO: Do we need to enque secrets just for the namespace or in general?


I need to dig deeper into this.

pkg/alertmanager/operator.go

pkg/alertmanager/amcfg.go

pkg/alertmanager/operator.go

simonpasquier · 2020-10-12T12:25:56Z

👍 for me. Lets give other @prometheus-operator/prometheus-operator-reviewers a chance to review it :)

lilic

lgtm 👍

Thanks again!!

lilic · 2020-10-13T07:33:37Z

The only thing I would ask if the commits can be squashed, just to make it easier in case we need to revert something. 25 seems a bit too much even for this much changes. Do you mind doing that @grdryn and then we can merge 🎉

simonpasquier · 2020-10-13T08:00:35Z

We could also Squash and merge with the GitHub UI.

lilic · 2020-10-13T08:04:17Z

Personally, I like to have generated (API in this example) and actual code separated, but not a blocker for me, happy to do it via github UI as well.

simonpasquier · 2020-10-13T08:15:19Z

Leaving the final call to @grdryn if he's willing to squash the commits and split the generated code in a separate commit :)

Command: make --always-make generate \ pkg/client/versioned/clientset.go \ pkg/client/listers/monitoring/v1/prometheus.go \ pkg/client/listers/monitoring/v1alpha1/prometheus.go \ pkg/client/informers/externalversions/monitoring/v1/prometheus.go \ pkg/client/informers/externalversions/monitoring/v1alpha1/prometheus.go

grdryn · 2020-10-13T20:23:55Z

I've squashed to 3 commits: brancz's original work, then the rest is the split between manual code changes and generated files. Is that the split you're looking for?

lilic · 2020-10-14T07:16:07Z

Thanks sounds good!

lilic · 2020-10-14T07:16:38Z

Thanks so much again for the amazing work and patience! 🎉

moshloop · 2020-10-14T07:32:52Z

Thank you to everyone involved in getting this merged 🎉🙌

keren-benzion · 2022-04-25T15:04:18Z

Hi,
I think this can come in handy in my organisation. Do you have maybe a document which explains how to implement this? to make it easier for me to deploy it

thanks !

grdryn requested review from paulfantom and a team as code owners August 27, 2020 17:11

simonpasquier mentioned this pull request Aug 31, 2020

API deprecation policy #3439

Open

grdryn force-pushed the alertmanagerconfig-crd branch 2 times, most recently from 25d6434 to 893c47c Compare September 7, 2020 18:30

grdryn commented Sep 7, 2020

View reviewed changes

grdryn commented Sep 8, 2020

View reviewed changes

pkg/alertmanager/amcfg.go Show resolved Hide resolved

grdryn force-pushed the alertmanagerconfig-crd branch from 893c47c to b433122 Compare September 8, 2020 16:41

s-urbaniak mentioned this pull request Sep 9, 2020

"targets" field in staticConfig in Probe custom resource is wrongly called "static" #3480

Open

grdryn commented Sep 10, 2020

View reviewed changes

scripts/generate-crds.go Outdated Show resolved Hide resolved

grdryn force-pushed the alertmanagerconfig-crd branch 2 times, most recently from ea2de4e to 3907ed0 Compare September 11, 2020 16:42

lilic reviewed Sep 14, 2020

View reviewed changes

example/prometheus-operator-crd/monitoring.coreos.com_alertmanagerconfigs.yaml Outdated Show resolved Hide resolved

grdryn force-pushed the alertmanagerconfig-crd branch 2 times, most recently from afd1050 to 3082d0d Compare September 15, 2020 13:59

simonpasquier reviewed Sep 15, 2020

View reviewed changes

grdryn force-pushed the alertmanagerconfig-crd branch 3 times, most recently from 69e0b84 to 36f519b Compare September 17, 2020 17:21

janeczku mentioned this pull request Sep 22, 2020

Add ability to specify trustedCA(or use existing from Rancher installation) in Notifier webhook options rancher/rancher#24272

Closed

grdryn force-pushed the alertmanagerconfig-crd branch from 36f519b to 1d19b0d Compare September 22, 2020 18:31

grdryn changed the title ~~WIP: Add AlertmanagerConfig CRD~~ Add AlertmanagerConfig CRD Sep 22, 2020

lilic reviewed Sep 23, 2020

View reviewed changes

dgrisonnet reviewed Sep 23, 2020

View reviewed changes

pkg/alertmanager/amcfg_test.go Show resolved Hide resolved

pkg/alertmanager/asset_store_test.go Show resolved Hide resolved

pkg/alertmanager/asset_store.go Show resolved Hide resolved

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

grdryn force-pushed the alertmanagerconfig-crd branch from fd9e294 to 3c47587 Compare October 8, 2020 16:16

simonpasquier reviewed Oct 9, 2020

View reviewed changes

simonpasquier reviewed Oct 12, 2020

View reviewed changes

pkg/alertmanager/amcfg.go Outdated Show resolved Hide resolved

pkg/alertmanager/operator.go Show resolved Hide resolved

grdryn force-pushed the alertmanagerconfig-crd branch from a25808d to a9ba233 Compare October 12, 2020 10:53

lilic approved these changes Oct 13, 2020

View reviewed changes

brancz and others added 3 commits October 13, 2020 21:18

*: Add AlertmanagerConfig CRD

53eb1eb

Create AM config file from base config and CRs

a3a014e

grdryn force-pushed the alertmanagerconfig-crd branch from a9ba233 to 238b53e Compare October 13, 2020 20:19

abursavich mentioned this pull request Oct 13, 2020

Alertmanager not taking multiple secrets with multiple yaml files #2258

Closed

lilic merged commit e1bea09 into prometheus-operator:master Oct 14, 2020

simonpasquier mentioned this pull request Oct 14, 2020

pkg/alertmanager: add metrics on selected/rejected AlertmanagerConfig #3588

Merged

grdryn deleted the alertmanagerconfig-crd branch October 14, 2020 08:55

TechIsCool mentioned this pull request Oct 14, 2020

WIP: *: Add AlertmanagerConfig CRD #3265

Closed

4n4nd mentioned this pull request Apr 12, 2021

community-operators [N] prometheus (9.9.9) operator-framework/community-operators#2502

Closed

gdemonet mentioned this pull request Sep 16, 2021

Use AlertmanagerConfig resource instead of CSC ConfigMap scality/metalk8s#3539

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AlertmanagerConfig CRD #3451

Add AlertmanagerConfig CRD #3451

grdryn commented Aug 27, 2020

s-urbaniak commented Aug 31, 2020

grdryn commented Aug 31, 2020

grdryn left a comment

grdryn commented Sep 9, 2020

simonpasquier left a comment

grdryn commented Sep 22, 2020

lilic commented Sep 23, 2020

lilic left a comment

lilic Sep 23, 2020

grdryn Sep 23, 2020 •

edited

Loading

simonpasquier Oct 9, 2020

dgrisonnet left a comment

brancz commented Sep 23, 2020

simonpasquier left a comment

simonpasquier Oct 9, 2020

grdryn Oct 9, 2020

simonpasquier Oct 12, 2020

simonpasquier Oct 9, 2020

simonpasquier commented Oct 12, 2020

lilic left a comment

lilic commented Oct 13, 2020

simonpasquier commented Oct 13, 2020

lilic commented Oct 13, 2020

simonpasquier commented Oct 13, 2020

grdryn commented Oct 13, 2020

lilic commented Oct 14, 2020

lilic commented Oct 14, 2020

moshloop commented Oct 14, 2020

keren-benzion commented Apr 25, 2022 •

edited

Loading

Add AlertmanagerConfig CRD #3451

Add AlertmanagerConfig CRD #3451

Conversation

grdryn commented Aug 27, 2020

s-urbaniak commented Aug 31, 2020

grdryn commented Aug 31, 2020

grdryn left a comment

Choose a reason for hiding this comment

grdryn commented Sep 9, 2020

simonpasquier left a comment

Choose a reason for hiding this comment

grdryn commented Sep 22, 2020

lilic commented Sep 23, 2020

lilic left a comment

Choose a reason for hiding this comment

lilic Sep 23, 2020

Choose a reason for hiding this comment

grdryn Sep 23, 2020 • edited Loading

Choose a reason for hiding this comment

simonpasquier Oct 9, 2020

Choose a reason for hiding this comment

dgrisonnet left a comment

Choose a reason for hiding this comment

brancz commented Sep 23, 2020

simonpasquier left a comment

Choose a reason for hiding this comment

simonpasquier Oct 9, 2020

Choose a reason for hiding this comment

grdryn Oct 9, 2020

Choose a reason for hiding this comment

simonpasquier Oct 12, 2020

Choose a reason for hiding this comment

simonpasquier Oct 9, 2020

Choose a reason for hiding this comment

simonpasquier commented Oct 12, 2020

lilic left a comment

Choose a reason for hiding this comment

lilic commented Oct 13, 2020

simonpasquier commented Oct 13, 2020

lilic commented Oct 13, 2020

simonpasquier commented Oct 13, 2020

grdryn commented Oct 13, 2020

lilic commented Oct 14, 2020

lilic commented Oct 14, 2020

moshloop commented Oct 14, 2020

keren-benzion commented Apr 25, 2022 • edited Loading

grdryn Sep 23, 2020 •

edited

Loading

keren-benzion commented Apr 25, 2022 •

edited

Loading