Add PodMonitor #2566

auhlig · 2019-04-20T17:49:19Z

As described in #38.
Still work in progess. To be continued after easter holidays

auhlig · 2019-04-29T09:06:46Z

Could you take a look @brancz, @metalmatze?

brancz

First review. Thanks a ton for working on this!

cc @mxinden @metalmatze

brancz · 2019-04-29T09:15:28Z

Documentation/api.md

+| Field | Description | Scheme | Required |
+| ----- | ----------- | ------ | -------- |
+| jobLabel | The label to use to retrieve the job name from. | string | false |
+| targetLabels | TargetLabels transfers labels on the Kubernetes Service onto the target. | []string | false |


this should just be what podTargetLabels is no?

Yep. Will change

brancz · 2019-04-29T09:20:22Z

pkg/prometheus/promcfg_test.go

-		map[string]*monitoringv1.ServiceMonitor{
-			"testservicemonitor1": &monitoringv1.ServiceMonitor{
+		nil,
+		map[string]*monitoringv1.PodMonitor{


We still want the test that was here previously. Could you extract this into a separate test?

brancz · 2019-04-29T09:25:32Z

Documentation/api.md

+| jobLabel | The label to use to retrieve the job name from. | string | false |
+| targetLabels | TargetLabels transfers labels on the Kubernetes Service onto the target. | []string | false |
+| podTargetLabels | PodTargetLabels transfers labels on the Kubernetes Pod onto the target. | []string | false |
+| endpoints | A list of endpoints allowed as part of this PodMonitor. | [][Endpoint](#endpoint) | true |


This is a little difficult, at this point I'm not comfortable introducing everything the Endpoint struct offers in terms of configuration, for the same reasons why #2524 is happening. I'd prefer to have a duplicate struct for this, that does not have the problematic fields that are the goal to be deprecated as part of the overall effort of #2524. I'd suggest that we just copy the current Endpoint struct and for now remove the TLSConfig and basic auth options.

Yes agreed. We may also want to be cautious here in future naming as endpoints is an existing term in the K8s world and is closely related to the concept of services.

Maybe PodMetricsEndpoint ?

PodMetricsEndpoint sounds good

squat · 2019-04-29T13:07:13Z

pkg/apis/monitoring/v1/types.go

+	// https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#metadata
+	// +k8s:openapi-gen=false
+	metav1.ObjectMeta `json:"metadata,omitempty"`
+	// Specification of desired Pod selection for target discrovery by Prometheus.


s/discrovery/discovery/

squat · 2019-04-29T13:08:04Z

pkg/apis/monitoring/v1/types.go

+type PodMonitorSpec struct {
+	// The label to use to retrieve the job name from.
+	JobLabel string `json:"jobLabel,omitempty"`
+	// TargetLabels transfers labels on the Kubernetes Service onto the target.


This should be agnostic of a Kubernetes Service. Do we mean Kubernetes Pod here instead?

yep. fixed.

squat · 2019-04-29T13:17:33Z

pkg/prometheus/operator.go

+	}
+
+	podMonitors := []string{}
+	for k := range res {


Instead of looping again here, we can add the keys to the slice on line 1533, when we are already looping.

squat · 2019-04-29T13:22:07Z

pkg/prometheus/promcfg.go

+	if version.Major == 1 && version.Minor < 7 {
+		// Filter targets based on the namespace selection configuration.
+		// By default we only discover services within the namespace of the
+		// ServiceMonitor.


s/ServiceMonitor/PodMonitor/g

squat · 2019-04-29T13:23:05Z

pkg/prometheus/promcfg.go

+
+	// By default, generate a safe job name from the pod name.  We also keep
+	// this around if a jobLabel is set in case the targets don't actually have a
+	// value for it. A single service may potentially have multiple metrics


s/service/pod/

auhlig · 2019-04-30T09:06:47Z

pkg/apis/monitoring/v1/types.go

@@ -583,6 +581,41 @@ type PodMonitorSpec struct {
 	SampleLimit uint64 `json:"sampleLimit,omitempty"`
 }

+// PodMetricsEndpoint defines a scrapeable endpoint of a Kubernetes Pod serving Prometheus metrics.
+// +k8s:openapi-gen=true
+type PodMetricsEndpoint struct {


The PodMetricsEndpoint for a PodMonitor copies the Endpoint struct used in a ServiceMonitor. This is used to disentangle both. Ok with this @brancz?

brancz

This is looking pretty good already!

brancz · 2019-04-30T09:38:02Z

pkg/apis/monitoring/v1/types.go

+	// Timeout after which the scrape is ended
+	ScrapeTimeout string `json:"scrapeTimeout,omitempty"`
+	// TLS configuration to use when scraping the endpoint
+	TLSConfig *TLSConfig `json:"tlsConfig,omitempty"`


I'd prefer to leave this out for now, due to the previously mentioned reasons.

right. removed

brancz · 2019-04-30T09:38:12Z

pkg/apis/monitoring/v1/types.go

+	// TLS configuration to use when scraping the endpoint
+	TLSConfig *TLSConfig `json:"tlsConfig,omitempty"`
+	// File to read bearer token for scraping targets.
+	BearerTokenFile string `json:"bearerTokenFile,omitempty"`


Same as TLSConfig

brancz · 2019-04-30T09:38:26Z

pkg/apis/monitoring/v1/types.go

+	HonorLabels bool `json:"honorLabels,omitempty"`
+	// BasicAuth allow an endpoint to authenticate over basic authentication
+	// More info: https://prometheus.io/docs/operating/configuration/#endpoints
+	BasicAuth *BasicAuth `json:"basicAuth,omitempty"`


Same as TLSConfig and BearerTokenFile

auhlig · 2019-04-30T13:33:10Z

Mh. Not sure why the CI runs into a timeout: https://travis-ci.org/coreos/prometheus-operator/jobs/526384616.
Unrelated?

brancz · 2019-04-30T14:04:44Z

Interesting. First time I'm seeing this, and it doesn't look like master is having this problem. I think it might have to do with this PR. Have you tried running the latest state on a new cluster?

auhlig · 2019-04-30T14:11:16Z

Strange: It times out for all commits but at different tests.
I tested the initial state in a cluster. Let me do that for the latest one as well.

metalmatze · 2019-04-30T14:21:49Z

I don't think it matters at what tests it timeouts. Looking at the fact that it needs 50min and we have a limit of 55min for those tests, I suppose that something deadlocks and doesn't finish.
I would guess that this is a test in this PR, so worth trying those tests in isolation.

auhlig · 2019-04-30T14:24:29Z

I'm convinced. Must be related. I look into it.
Thanks for the feedback so far

auhlig · 2019-05-10T12:50:12Z

I cannot see what's causing the timeout in the e2e test. Would appreciate any hint @brancz @metalmatze @squat 🙏

paulfantom · 2019-05-21T14:09:20Z

This needs to be regenerated. Please run make generate-in-docker and commit generated files from example/ directory.

auhlig · 2019-05-21T14:16:18Z

Thanks @paulfantom.
I fixed the e2e test and rebased. Noticed that I have to re-generate but was interrupted by a meeting.
Will do in the evening, then this PR should be ready for a final review.

auhlig · 2019-05-22T07:53:41Z

Done @paulfantom. All tests are green 🎉
Could you take a look again?
/cc @brancz @metalmatze @squat

auhlig · 2019-06-06T09:22:39Z

rebased and fixed e2e test

auhlig · 2019-06-06T13:14:10Z

If you're okay with 788a6b0, I'd rebase & resolve the conflicts again @brancz

brancz

Sorry about the conflict. Once rebased and green, this lgtm 👍 Awesome work! Thank you so much!

brancz · 2019-06-07T14:33:43Z

pkg/prometheus/promcfg.go

+			{Key: "target_label", Value: "namespace"},
+		},
+		{
+			{Key: "source_labels", Value: []string{"__meta_kubernetes_pod_container_name"}},


side note: we should add this in the ServiceMonitor as well 👍

Signed-off-by: Arno Uhlig <arno.uhlig@sap.com>

auhlig · 2019-06-11T14:20:00Z

Rebased and resolved conflicts. LGTY @brancz?

brancz · 2019-06-11T14:41:00Z

lgtm 👍 probably worth one more review as it's such a large one

@s-urbaniak @metalmatze @squat @paulfantom ?

metalmatze

Looked over the non-generated code.
LGTM from my side.

I'd suggest to just move forward with this and then do follow up PRs!

This is super exciting! 🎉

s-urbaniak

lgtm, thanks a lot! 🎉

auhlig · 2019-06-11T16:03:13Z

Thanks @brancz @metalmatze @s-urbaniak @paulfantom @squat :tada

Let me know if anything related needs adjustment. Happy to fix.

auhlig force-pushed the podmonitor branch 8 times, most recently from 8b782ad to 0fc55a4 Compare April 29, 2019 09:05

auhlig changed the title **WIP** Add PodMonitor Add PodMonitor Apr 29, 2019

brancz reviewed Apr 29, 2019

View reviewed changes

squat reviewed Apr 29, 2019

View reviewed changes

auhlig commented Apr 30, 2019

View reviewed changes

brancz reviewed Apr 30, 2019

View reviewed changes

auhlig force-pushed the podmonitor branch from 2af6486 to 3492e64 Compare May 6, 2019 14:25

auhlig force-pushed the podmonitor branch from 5db9b4e to 5fbbdce Compare May 21, 2019 10:45

auhlig force-pushed the podmonitor branch from dfce209 to 8495b65 Compare May 21, 2019 23:40

brancz approved these changes Jun 7, 2019

View reviewed changes

auhlig added 9 commits June 11, 2019 11:20

initial PodMonitor

665b1a9

Signed-off-by: Arno Uhlig <arno.uhlig@sap.com>

incorporate comments

338ad0d

Signed-off-by: Arno Uhlig <arno.uhlig@sap.com>

remove authentication, TLS config as it's being worked on

4dcb1a4

Signed-off-by: Arno Uhlig <arno.uhlig@sap.com>

fix rbac for podmonitors

b91d61b

generate

09eaef8

mark podmonitors as experimental; fix relabeling

86ed2d1

make k8s-gen and fix tests after dropping relabelings

8156e7c

use podmonitor name as default job label

e872960

fix test for job name

dcf800b

Signed-off-by: Arno Uhlig <arno.uhlig@sap.com>

auhlig force-pushed the podmonitor branch from 2df45a0 to e0ac768 Compare June 11, 2019 09:20

auhlig mentioned this pull request Jun 11, 2019

ServicemonitorKey #2638

Closed

auhlig force-pushed the podmonitor branch 2 times, most recently from c120c31 to beda9d9 Compare June 11, 2019 13:02

fix test framework; make generate

672ded9

Signed-off-by: Arno Uhlig <arno.uhlig@sap.com>

auhlig force-pushed the podmonitor branch from beda9d9 to 672ded9 Compare June 11, 2019 13:02

metalmatze approved these changes Jun 11, 2019

View reviewed changes

s-urbaniak approved these changes Jun 11, 2019

View reviewed changes

s-urbaniak merged commit ca400fd into prometheus-operator:master Jun 11, 2019

brancz mentioned this pull request Jun 12, 2019

Add PodMonitor #38

Closed

StevenACoffman mentioned this pull request Jun 24, 2019

Comparison of Pod Collector to PodMonitor zalando-incubator/kube-metrics-adapter#60

Closed

sichvoge mentioned this pull request Aug 28, 2019

Add docs for PodMonitor CRD #2738

Closed

This was referenced Oct 1, 2019

IngressMonitor #2786

Closed

Generic ScrapeConfig CRD #2787

Closed

auhlig deleted the podmonitor branch October 24, 2019 15:34

Add PodMonitor #2566

Add PodMonitor #2566

Conversation

auhlig commented Apr 20, 2019 • edited

auhlig commented Apr 29, 2019

brancz left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

auhlig Apr 30, 2019 • edited

Choose a reason for hiding this comment

brancz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

auhlig commented Apr 30, 2019 • edited

brancz commented Apr 30, 2019

auhlig commented Apr 30, 2019

metalmatze commented Apr 30, 2019

auhlig commented Apr 30, 2019

auhlig commented May 10, 2019

paulfantom commented May 21, 2019

auhlig commented May 21, 2019

auhlig commented May 22, 2019 • edited

auhlig commented Jun 6, 2019

auhlig commented Jun 6, 2019

brancz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

auhlig commented Jun 11, 2019

brancz commented Jun 11, 2019

metalmatze left a comment

Choose a reason for hiding this comment

s-urbaniak left a comment

Choose a reason for hiding this comment

auhlig commented Jun 11, 2019 • edited

auhlig commented Apr 20, 2019 •

edited

brancz left a comment •

edited

auhlig Apr 30, 2019 •

edited

auhlig commented Apr 30, 2019 •

edited

auhlig commented May 22, 2019 •

edited

auhlig commented Jun 11, 2019 •

edited