prometheus: Introduce RuleFile Custom Resource Definition by mxinden · Pull Request #1333 · prometheus-operator/prometheus-operator

mxinden · 2018-05-11T08:58:57Z

This patch introduces a new Custom Resource Definition to the
Prometheus Operator - the Rule CRD. It addresses two main
needs:

Prometheus (alerting and recording) Rule validation during creation time
via Kubernetes Custom Resource Definition validation.
Life-cycle management of Prometheus application Rules alongside the
application itself, inside the applications Kubernetes namespace, not
necessarily the namespace of the scraping Prometheus instance.

A user defines Prometheus alerting and recording Rules via a Kubernetes
Custom Resource Definition. These Custom Resource Definitions can be
fully validated by the Kubernetes API server during creation time via
automatically generated OpenAPI specifications. Instead of the
restriction of a Prometheus instance to only select Rule definitions
inside its own namespace, the Prometheus specification is extended to
also specify namespaces to look for Rule Custom Resource Definitions
outside its own namespace.

Dependent technical changes:

prometheus: Use github.com/jimmidyson/configmap-reload to reload rules
prometheus: Remove Prometheus Statefulset deletion function. Starting
with K8s >=1.8 this is handled via OwnerReferences.
prometheus: Do not add rule files checksum to Prometheus configuration
secret
prometheus: Update StatefulSet only on relevant changes. Instead of
updating the Prometheus StatefulSet on every sync() run, only update
it if the input parameters to makeStatefulSet change. Enforce this
via a checksum of the parameters which is saved inside the annotations
of the statefulset.
e2e/prometheus: Check how often resources (Secret, ConfigMap,
Prometheus CRD, Service) are updated to enforce that Prometheus Operator
only updated created resources if necessary.
contrib/prometheus-config-reloader: Remove logic to retriev K8s
ConfigMaps. These are mounted into the pod right away now.

Design Document

ironcladlou · 2018-05-11T13:36:21Z

contrib/kube-prometheus/manifests/prometheus-k8s/alertingrulefile.yaml

+  name: k8s
+  namespace: monitoring
+spec:
+        Rules: "123"


ironcladlou · 2018-05-11T14:06:54Z

pkg/client/monitoring/v1alpha1/types.go


+// AlertingRuleFile defines alerting rules for a Prometheus instance
+// TODO: Should we go with v1 instead of v1alpha1 right away?
+type AlertingRuleFile struct {


If AlertingRuleFile is used only to hold the serialized contents of an alerting rule file (the type for which is not represented in this API), how does AlertingRuleFile compare to using a ConfigMap?

@ironcladlou My plan is to vendor the upstream Prometheus alerting rule struct. Thereby we would have full OpenAPI generated deep validation on creation time (instant feedback). What do you think?

I still have a couple TODOs on my list :/

https://github.com/coreos/prometheus-operator/pull/1333/files#diff-c8710fb169ff41dfb58c4a26a2f7a755R210

That makes sense to me- cool!

brancz

Awesome start! Really looking forward to this!

brancz · 2018-05-11T18:37:13Z

Documentation/api.md

 | externalUrl | The external URL the Prometheus instances will be available under. This is necessary to generate correct URLs. This is necessary if Prometheus is not served from root of a DNS name. | string | false |
 | routePrefix | The route prefix Prometheus registers HTTP handlers for. This is useful, if using ExternalURL and a proxy is rewriting HTTP routes of a request, and the actual ExternalURL is still true, but the server serves requests under a different route prefix. For example for use with `kubectl proxy`. | string | false |
 | storage | Storage spec to specify how storage shall be used. | *[StorageSpec](#storagespec) | false |
-| ruleSelector | A selector to select which ConfigMaps to mount for loading rule files from. | *[metav1.LabelSelector](https://v1-6.docs.kubernetes.io/docs/api-reference/v1.6/#labelselector-v1-meta) | false |


We will need to continue to support this for the time being, I suggest this should select both configmaps and Rule CRDs for the forseeable future and the configmap selector stays local to the namespace and the CRD selector is respective to the ruleNamespaceSelector.

In fact we should actually do a migration where we re-create rule CRDs from the content of the configmaps.

I brought back the field and added the following to the rule retrieval logic:

// With Prometheus Operator v0.20.0 the 'RuleSelector' field in the Prometheus // CRD Spec is deprecated. Any value in 'RuleSelector' is just copied to the new // field 'RuleFileSelector'. if p.Spec.RuleFileSelector == nil && p.Spec.RuleSelector != nil { p.Spec.RuleFileSelector = p.Spec.RuleSelector }

@brancz Do you mind if we add the migration logic in a separate PR (of course before the release)?

brancz · 2018-05-11T18:37:59Z

Documentation/api.md

 | routePrefix | The route prefix Prometheus registers HTTP handlers for. This is useful, if using ExternalURL and a proxy is rewriting HTTP routes of a request, and the actual ExternalURL is still true, but the server serves requests under a different route prefix. For example for use with `kubectl proxy`. | string | false |
 | storage | Storage spec to specify how storage shall be used. | *[StorageSpec](#storagespec) | false |
-| ruleSelector | A selector to select which ConfigMaps to mount for loading rule files from. | *[metav1.LabelSelector](https://v1-6.docs.kubernetes.io/docs/api-reference/v1.6/#labelselector-v1-meta) | false |
+| alertingRuleFileSelector | A selector to select which AlertingRuleFiles to mount for loading alerting rules from. | *[metav1.LabelSelector](https://v1-6.docs.kubernetes.io/docs/api-reference/v1.6/#labelselector-v1-meta) | false |


It's not just alerting rules, it's also recording rules, therefore usually referred to as "rule"

brancz · 2018-05-11T18:39:30Z

cmd/operator/main.go

 		return 1
 	}

+	fmt.Println("image: ", cfg.ConfigReloaderImage)


remove debug fmt.Println

brancz · 2018-05-11T18:42:57Z

pkg/k8sutil/k8sutil.go

+		ResourceScope:      string(extensionsobj.NamespaceScoped),
+		Group:              group,
+		Kind:               crdKind.Kind,
+		// Version:               monitoringv1.Version,


remove comment

brancz · 2018-05-11T18:44:50Z

pkg/client/monitoring/v1alpha1/types.go


+// AlertingRuleFile defines alerting rules for a Prometheus instance
+// TODO: Should we go with v1 instead of v1alpha1 right away?
+type AlertingRuleFile struct {


I already commented on the name above, I think this should probably be "PrometheusRule" instead.

brancz · 2018-05-11T18:47:00Z

Dockerfile

 FROM quay.io/prometheus/busybox:latest

 ADD .build/linux-amd64/operator /bin/operator
+ADD ./contrib/prometheus-config-reloader/prometheus-config-reloader /bin/prometheus-config-reloader


I've thought of this as well, but I'm not sure I like it, especially if the container image repo is the same, then people will be maximum confused I think. Even today people think the operator runs as a sidecar. It also has a large impact on the container size, the reload container is very very small whereas operator+reloader is a pretty large image.

mxinden · 2018-05-12T17:24:36Z

@brancz RuleFile CRD is currently accessible on monitoring v1alpha1. Would you prefer to go with v1?

metalmatze · 2018-05-13T13:03:25Z

I have a general question about this feature:

How does this align with the current work going on to make jsonnet bases monitoring mixins with alerts? Are these features orthogonal? I guess it tries to solves the problem with a different approach, but on the other hand can also be combined?
So I'd have those mixins, make slight adjustments and then generate a CRD from that?

brancz · 2018-05-14T07:28:49Z

@metalmatze they're unrelated, this CRD is merely so that we can collect Prometheus rules from across namespaces similar to cross namespace ServiceMonitors. In fact as these rules are identical to the Prometheus rule definition they are compatible.

brancz · 2018-05-14T07:29:36Z

@mxinden do they work alongside each other? I seem to recall there was some problem as some point. Otherwise v1alpha1 sounds good as it's a new resource.

ant31 · 2018-05-14T10:09:22Z

AFAIK, CRD are painful to upgrade (from v1alpha1 to v1), I would save us and users this additional migration. Also there is no plan to keep both at the same time alive (configmap + crd), so as soon as we release this, the CRD will replace the configmap, then I would go for v1.
Last The spec itself is well-known, we don't add any feature/fields that exist already in the current configmap.

brancz · 2018-05-14T11:58:29Z

Yeah after thinking about it once more I think putting it in v1 is fine, given that the spec we're using is identical to the Prometheus rule spec, which cannot be broken until the next Prometheus major version anyways.

mxinden · 2018-05-16T08:18:43Z

General design document can be found here.

Feedback by the community is very welcome.

mxinden · 2018-05-17T14:01:13Z

@brancz @ant31 @ironcladlou @metalmatze this PR is ready for review. Would you mind taking another look?

brancz

Nice work. Just a couple of comments and nits, but overall looking very good!

brancz · 2018-05-17T15:28:36Z

pkg/client/monitoring/v1/types.go

+// +k8s:openapi-gen=true
+type RuleFileSpec struct {
+	// Content of Prometheus rule file
+	Content RuleGroups `json:"content,omitempty"`


What speaks against having the Groups field here instead of nested into the RuleGroups struct? Seems double encapsulated.

True. I will address that.

brancz · 2018-05-17T15:29:49Z

pkg/client/monitoring/v1/types.go

 }
+
+func (l *RuleFile) DeepCopyObject() runtime.Object {
+	panic("DeepCopyObject not implemented for ServiceMonitorList")


yeah we should start to actually implement these * hides *

Not entirely sure, but we are auto-generating these, right?

I have replaced the panic with return l.DeepCopy().

brancz · 2018-05-17T15:30:52Z

pkg/prometheus/operator.go

 	return nil
 }

+// TODO: All handleXXX do the same


this doesn't work for update, which is why we chose duplication over confusion

handleSmonUpdate and handleRuleUpdate seem equal to me, no?

yes, that's true, we can look into deduping those

brancz · 2018-05-17T15:34:45Z

pkg/prometheus/operator.go

 		k8sutil.NewCustomResourceDefinition(c.config.CrdKinds.Prometheus, c.config.CrdGroup, c.config.Labels.LabelsMap, c.config.EnableValidation),
 		k8sutil.NewCustomResourceDefinition(c.config.CrdKinds.ServiceMonitor, c.config.CrdGroup, c.config.Labels.LabelsMap, c.config.EnableValidation),
+		k8sutil.NewCustomResourceDefinition(
+			monitoringv1.CrdKind{


This should be configured from the config like the other CRDs.

👍 thanks, I forgot that.

brancz · 2018-05-17T15:35:03Z

pkg/prometheus/operator.go

 	}{
 		{"Prometheus", c.mclient.MonitoringV1().Prometheuses(c.config.Namespace).List},
 		{"ServiceMonitor", c.mclient.MonitoringV1().ServiceMonitors(c.config.Namespace).List},
+		{"Rule", c.mclient.MonitoringV1().RuleFiles(c.config.Namespace).List},


Rule -> RuleFile

brancz · 2018-05-17T15:40:11Z

pkg/prometheus/statefulset.go

-		return nil, err
-	}
-
+// TODO: No need to return an error


this doesn't return an error

Fixed it and forgot to remove the TODO 🤦‍♂️

brancz · 2018-05-17T15:41:27Z

pkg/prometheus/statefulset.go

 	}, nil
 }

+// TODO: Is this used by anyone else?


not sure what this comment is referring to

brancz · 2018-05-17T15:50:38Z

pkg/prometheus/rulefile.go

+			return ruleFiles, err
+		}
+		if marshalErr != nil {
+			return ruleFiles, marshalErr


on error return nil explicitly

~~I don't understand what you mean.~~

~~There are two types of possible errors here:~~
~~1. Error thrown by ListAllByNamespace~~
~~2. Error thrown by yaml.Marshal~~

~~Not very proud of the structure of my code here. Happy for suggestions.~~

Never mind, understood.

brancz · 2018-05-17T15:51:25Z

pkg/prometheus/rulefile.go

+			ruleFiles[fmt.Sprintf("%v-%v.yaml", file.Namespace, file.Name)] = string(content)
+		})
+		if err != nil {
+			return ruleFiles, err


on error return nil explicitly

brancz · 2018-05-17T15:53:05Z

pkg/prometheus/rulefile.go

+				marshalErr = err
+				return
+			}
+			ruleFiles[fmt.Sprintf("%v-%v.yaml", file.Namespace, file.Name)] = string(content)


I'm not entirely sure about the behavior, when file name is different but rule group name is the same, we might have to add the namespace as a prefix to the rule group name. Let's make sure and investigate this.

I have just started a Prometheus with two unique rule files, with matching rule group names. Both the alerts from file one, as well as file two show up in the Prometheus UI.

Perfect, thanks for making sure!

brancz · 2018-05-17T15:59:12Z

One more thing, the RuleFile CRD needs to be generated into examples/prometheus-operator-crds/ and converted to json for the kube-prometheus stack.

This patch introduces a new Custom Resource Definition to the Prometheus Operator - the Rule CRD. It addresses two main needs: 1. Prometheus (alerting and recording) Rule validation during creation time via Kubernetes Custom Resource Definition validation. 2. Life-cycle management of Prometheus application Rules alongside the application itself, inside the applications Kubernetes namespace, not necessarily the namespace of the scraping Prometheus instance. A user defines Prometheus alerting and recording Rules via a Kubernetes Custom Resource Definition. These Custom Resource Definitions can be fully validated by the Kubernetes API server during creation time via automatically generated OpenAPI specifications. Instead of the restriction of a Prometheus instance to only select Rule definitions inside its own namespace, the Prometheus specification is extended to also specify namespaces to look for Rule Custom Resource Definitions outside its own namespace. --- Dependent technical changes: - prometheus: Use github.com/jimmidyson/configmap-reload to reload rules - prometheus: Remove Prometheus Statefulset deletion function. Starting with K8s >=1.8 this is handled via OwnerReferences. - prometheus: Do not add rule files checksum to Prometheus configuration secret - prometheus: Update StatefulSet only on relevant changes. Instead of updating the Prometheus StatefulSet on every `sync()` run, only update it if the input parameters to `makeStatefulSet` change. Enforce this via a checksum of the parameters which is saved inside the annotations of the statefulset. - e2e/prometheus: Check how often resources (Secret, ConfigMap, Prometheus CRD, Service) are updated to enforce that Prometheus Operator only updated created resources if necessary. - contrib/prometheus-config-reloader: Remove logic to retriev K8s ConfigMaps. These are mounted into the pod right away now.

mxinden · 2018-05-18T14:54:08Z

@brancz Thanks for all the comments! Would you mind taking another look?

brancz · 2018-05-18T15:14:25Z

Nice job! lgtm 👍

Atoms · 2018-06-04T18:45:01Z

Documentation/api.md

-| ruleSelector | A selector to select which ConfigMaps to mount for loading rule files from. | *[metav1.LabelSelector](https://v1-6.docs.kubernetes.io/docs/api-reference/v1.6/#labelselector-v1-meta) | false |
+| ruleFileSelector | A selector to select which RuleFiles to mount for loading alerting rules from. | *[metav1.LabelSelector](https://v1-6.docs.kubernetes.io/docs/api-reference/v1.6/#labelselector-v1-meta) | false |
+| ruleSelector | DEPRECATED with Prometheus Operator 'v0.20.0'. Any value in this field will just be copied to 'RuleFileSelector' field | *[metav1.LabelSelector](https://v1-6.docs.kubernetes.io/docs/api-reference/v1.6/#labelselector-v1-meta) | false |
+| ruleFileNamespaceSelector | Namespaces to be selected for RuleFiles discovery. If empty, only check own namespace. | *[metav1.LabelSelector](https://v1-6.docs.kubernetes.io/docs/api-reference/v1.6/#labelselector-v1-meta) | false |


here is written if empy only check own namespace, more down it's written empty will match all objects. so which is it ? :)

Seems to be the former:
https://github.com/mxinden/prometheus-operator/blob/89fc4e306972604eba2dcb961a6d29cc27a668ad/pkg/prometheus/rulefile.go#L103

Most likely to match the behavior of the ServiceMonitor one:
https://github.com/coreos/prometheus-operator/blob/f9676ddc5b55ae47dcdfa43d9290cb28e5504649/pkg/prometheus/promcfg.go#L468

Although @mxinden will confirm but in the RuleFile it seems to implement a standard selector which may offer the possibility to match all namespaces somehow.

Sorry for the confusion. When it is not set, then it will select the same namespace as the Prometheus server is in. With an empty selector explicitly set ({} is the empty selector), it will select all namespaces (maybe better referred to as the "all" selector). Hope this makes it clearer. 🙂 Also note that we will be renaming the resource before releasing it, see #1425.

yes that makes sense :) thanks for explanation

mxinden force-pushed the alerting-rule-file-crd branch 6 times, most recently from 4cee375 to 674b1d0 Compare May 11, 2018 13:44

ironcladlou reviewed May 11, 2018

View reviewed changes

mxinden force-pushed the alerting-rule-file-crd branch from 5784859 to c09c3f3 Compare May 11, 2018 17:42

brancz reviewed May 11, 2018

View reviewed changes

brancz mentioned this pull request May 11, 2018

Question about Prometheus in a shared cluster #1336

Closed

This was referenced May 13, 2018

prometheus-config-reloader - can't update / apply rules (main.go:214 | 500) #597

Closed

Loading alerts does not work on 0.19.0 #1323

Closed

gianrubio mentioned this pull request May 14, 2018

Proposal: Configure Prometheus alerts in the ServiceMonitor #828

Closed

brancz mentioned this pull request May 16, 2018

prometheus: Filter alerts from configmaps by cfg.Namespace #1346

Closed

mxinden force-pushed the alerting-rule-file-crd branch 9 times, most recently from 0ab77a7 to 91c9329 Compare May 17, 2018 10:30

mxinden force-pushed the alerting-rule-file-crd branch from be283e2 to e6620b2 Compare May 17, 2018 13:29

mxinden changed the title ~~[WIP] prometheus: Introduce alerting rule file crd~~ prometheus: Introduce RuleFile Custom Resource Definition May 17, 2018

mxinden force-pushed the alerting-rule-file-crd branch from e6620b2 to 3836dd2 Compare May 17, 2018 13:59

mxinden mentioned this pull request May 17, 2018

[WIP] Add ServiceMonitor for Openshift web-console openshift/cluster-monitoring-operator#14

Closed

brancz reviewed May 17, 2018

View reviewed changes

mxinden force-pushed the alerting-rule-file-crd branch 3 times, most recently from 4797d78 to 8bdb7f7 Compare May 18, 2018 14:19

mxinden force-pushed the alerting-rule-file-crd branch from 8bdb7f7 to 89fc4e3 Compare May 18, 2018 14:27

brancz merged commit b30d5e8 into prometheus-operator:master May 18, 2018

mxinden mentioned this pull request Jun 4, 2018

RuleSelector Extending #1184

Closed

jmcfarlane mentioned this pull request Jun 4, 2018

Support upstream time.ParseDuration (#117) prometheus/common#137

Closed

Atoms reviewed Jun 4, 2018

View reviewed changes

mxinden mentioned this pull request Jun 8, 2018

Prometheus rules not populating #1452

Closed

Conversation

mxinden commented May 11, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

mxinden commented May 12, 2018

Uh oh!

metalmatze commented May 13, 2018

Uh oh!

brancz commented May 14, 2018

Uh oh!

brancz commented May 14, 2018

Uh oh!

ant31 commented May 14, 2018

Uh oh!

brancz commented May 14, 2018

Uh oh!

mxinden commented May 16, 2018

Uh oh!

mxinden commented May 17, 2018

Uh oh!

brancz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

mxinden May 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

mxinden commented May 11, 2018 •

edited

Loading

brancz left a comment •

edited

Loading

mxinden May 18, 2018 •

edited

Loading

mxinden May 18, 2018 •

edited

Loading