New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

prometheus: Introduce RuleFile Custom Resource Definition #1333

Merged
merged 1 commit into from May 18, 2018

Conversation

Projects
None yet
7 participants
@mxinden
Copy link
Member

mxinden commented May 11, 2018

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.

Design Document

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

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

This comment has been minimized.

@ironcladlou

ironcladlou May 11, 2018

Contributor

Alignment

@@ -193,6 +193,24 @@ type ServiceMonitorSpec struct {
NamespaceSelector NamespaceSelector `json:"namespaceSelector,omitempty"`
}

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

This comment has been minimized.

@ironcladlou

ironcladlou May 11, 2018

Contributor

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?

This comment has been minimized.

@mxinden

mxinden May 11, 2018

Member

@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?

This comment has been minimized.

This comment has been minimized.

@ironcladlou

ironcladlou May 11, 2018

Contributor

That makes sense to me- cool!

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

@brancz
Copy link
Member

brancz left a comment

Awesome start! Really looking forward to this!

@@ -211,7 +211,8 @@ Specification of the desired behavior of the Prometheus cluster. More info: http
| 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 |

This comment has been minimized.

@brancz

brancz May 11, 2018

Member

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.

This comment has been minimized.

@mxinden

mxinden May 17, 2018

Member

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)?

This comment has been minimized.

@brancz
@@ -211,7 +211,8 @@ Specification of the desired behavior of the Prometheus cluster. More info: http
| 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 |
| 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 |

This comment has been minimized.

@brancz

brancz May 11, 2018

Member

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

@@ -115,6 +115,8 @@ func Main() int {
return 1
}

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

This comment has been minimized.

@brancz

brancz May 11, 2018

Member

remove debug fmt.Println

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

This comment has been minimized.

@brancz

brancz May 11, 2018

Member

remove comment

@@ -193,6 +193,24 @@ type ServiceMonitorSpec struct {
NamespaceSelector NamespaceSelector `json:"namespaceSelector,omitempty"`
}

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

This comment has been minimized.

@brancz

brancz May 11, 2018

Member

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

Dockerfile Outdated
@@ -1,5 +1,6 @@
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

This comment has been minimized.

@brancz

brancz May 11, 2018

Member

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.

This comment has been minimized.

@mxinden
@mxinden

This comment has been minimized.

Copy link
Member

mxinden commented May 12, 2018

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

@metalmatze

This comment has been minimized.

Copy link
Collaborator

metalmatze commented May 13, 2018

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

This comment has been minimized.

Copy link
Member

brancz commented May 14, 2018

@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

This comment has been minimized.

Copy link
Member

brancz commented May 14, 2018

@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

This comment has been minimized.

Copy link
Member

ant31 commented May 14, 2018

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

This comment has been minimized.

Copy link
Member

brancz commented May 14, 2018

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

This comment has been minimized.

Copy link
Member

mxinden commented May 16, 2018

General design document can be found here.

Feedback by the community is very welcome.

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

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

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

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

@mxinden

This comment has been minimized.

Copy link
Member

mxinden commented May 17, 2018

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

@brancz
Copy link
Member

brancz left a comment

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

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

This comment has been minimized.

@brancz

brancz May 17, 2018

Member

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

This comment has been minimized.

@mxinden

mxinden May 18, 2018

Member

True. I will address that.

@@ -550,3 +615,11 @@ func (l *ServiceMonitor) DeepCopyObject() runtime.Object {
func (l *ServiceMonitorList) DeepCopyObject() runtime.Object {
return l.DeepCopy()
}

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

This comment has been minimized.

@brancz

brancz May 17, 2018

Member

yeah we should start to actually implement these * hides *

This comment has been minimized.

@mxinden

mxinden May 18, 2018

Member

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

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

@@ -470,27 +487,56 @@ func (c *Operator) syncNodeEndpoints() error {
return nil
}

// TODO: All handleXXX do the same

This comment has been minimized.

@brancz

brancz May 17, 2018

Member

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

This comment has been minimized.

@mxinden

mxinden May 18, 2018

Member

handleSmonUpdate and handleRuleUpdate seem equal to me, no?

This comment has been minimized.

@brancz

brancz May 18, 2018

Member

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

// won't attempt to do so again.
return nil
}

crds := []*extensionsobj.CustomResourceDefinition{
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 comment has been minimized.

@brancz

brancz May 17, 2018

Member

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

This comment has been minimized.

@mxinden

mxinden May 18, 2018

Member

👍 thanks, I forgot that.

@@ -1143,6 +1138,7 @@ func (c *Operator) createCRDs() error {
}{
{"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},

This comment has been minimized.

@brancz

brancz May 17, 2018

Member

Rule -> RuleFile

return nil, err
}

// TODO: No need to return an error

This comment has been minimized.

@brancz

brancz May 17, 2018

Member

this doesn't return an error

This comment has been minimized.

@mxinden

mxinden May 18, 2018

Member

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

@@ -622,6 +620,7 @@ func makeStatefulSetSpec(p monitoringv1.Prometheus, c *Config, ruleConfigMaps []
}, nil
}

// TODO: Is this used by anyone else?

This comment has been minimized.

@brancz

brancz May 17, 2018

Member

not sure what this comment is referring to

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

This comment has been minimized.

@brancz

brancz May 17, 2018

Member

on error return nil explicitly

This comment has been minimized.

@mxinden

mxinden May 18, 2018

Member

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.

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

This comment has been minimized.

@brancz

brancz May 17, 2018

Member

on error return nil explicitly

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

This comment has been minimized.

@brancz

brancz May 17, 2018

Member

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.

This comment has been minimized.

@mxinden

mxinden May 18, 2018

Member

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.

This comment has been minimized.

@brancz

brancz May 18, 2018

Member

Perfect, thanks for making sure!

@brancz

This comment has been minimized.

Copy link
Member

brancz commented May 17, 2018

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

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

prometheus: Introduce RuleFile Custom Resource Definition
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 mxinden force-pushed the mxinden:alerting-rule-file-crd branch from 8bdb7f7 to 89fc4e3 May 18, 2018

@mxinden

This comment has been minimized.

Copy link
Member

mxinden commented May 18, 2018

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

@brancz

This comment has been minimized.

Copy link
Member

brancz commented May 18, 2018

Nice job! lgtm 👍

@brancz brancz merged commit b30d5e8 into coreos:master May 18, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mxinden mxinden referenced this pull request Jun 4, 2018

Open

RuleSelector Extending #1184

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

This comment has been minimized.

@Atoms

Atoms Jun 4, 2018

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

This comment has been minimized.

@gmauleon

gmauleon Jun 4, 2018

Most likely to match the behavior of the ServiceMonitor one:

namespaces = append(namespaces, m.Namespace)

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.

This comment has been minimized.

@brancz

brancz Jun 5, 2018

Member

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.

This comment has been minimized.

@Atoms

Atoms Jun 5, 2018

yes that makes sense :) thanks for explanation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment