add helper functions for running prometheus query #602

sallyom · 2019-11-24T18:16:04Z

@marun @sanchezl this PR provides helper functions for prometheus query

I placed a fake bump/testing here:

Bug 1771809: Add e2e for PodDisruptionBudgetAtLimit alert cluster-kube-controller-manager-operator#316
Bug 1771810: Add e2e to verify kso metrics accessible cluster-kube-scheduler-operator#191

/cc @sttts

pkg/metrics/query.go

sallyom · 2019-11-26T17:58:12Z

@marun thanks for the review, I've updated, ready for another review. Also, see this functioning in the cluster-kube-controller-operator PR.

marun

+1 to the separation of concerns you are pursuing.

What do you think of adding an e2e test that exercises the code? Some of my comments (e.g. exiting polling on recoverable errors) would be challenging to discover from code alone but would become immediately apparent when executed (e.g. failing immediately when configmap doesn't contain cabundle). I'd also prefer to have these helpers be maintainable in library-go vs requiring co-developement with one or more operators.

pkg/metrics/query.go

soltysh

@sallyom since these are meant as testing helpers please move them to pkg/test/library/metrics I don't want anyone accidentally figuring this one out and using it for other purpose than testing.

marun · 2020-01-07T08:27:50Z

test/library/metrics/query.go

+	}
+	for _, secret := range secrets.Items {
+		if secret.Type != corev1.SecretTypeServiceAccountToken ||
+			!strings.HasPrefix(secret.Name, "prometheus-") {


Would it make sense to specify the more explicit prometheus-k8s- prefix? Tokens sourced from secrets prefixed with both prometheus-k8s- and prometheus-operator- support querying today, but I'm not sure that will always be true.

marun · 2020-01-07T08:49:28Z

@sallyom Nice work! I've updated openshift/service-ca-operator#90 to use it and tests are passing locally. Once this PR merges I'll add another helper that checks that metrics are being collected for a given namespace (e.g. checkMetricsCollected(t, config, namespace)).

marun · 2020-01-07T08:52:26Z

test/library/metrics/query.go

+		return nil, err
+	}
+
+	route, err := rc.RouteV1().Routes("openshift-monitoring").Get("prometheus-k8s", metav1.GetOptions{})


Not something to block merge since the code works as-is., but I seem to recall @s-urbaniak mentioning that it would be preferable to query thanos-querier. However, my naive attempts to use the thanos-querier route were unsuccessful with tokens sourced from prometheus-k8s- and thanos-querier- prefixed secrets.

Indeed, the prometheus-k8s is route is still maintained, but effectively deprecated. Thanos Querier is gated by the same oauth-proxy sidecar as prometheus-k8s, hence the same token secrets should apply, else we have a bug on our side.

@s-urbaniak No bug. I was requesting targets via the thanos-querier route and it wasn't succeeding, but I realize now that it was a 404 rather than an auth failure presumably because thanos doesn't support querying targets. Performing an up query instead works fine.

@sallyom Please update to use the thanos-querier route.

I hit a snag, the thanos-querier route works for a query, but when searching for an alert, that route does not work, have to use the prometheus-k8s route for alerts (like I'm doing in the kube-controller-manager-operator PR). I added a switch and a param to the NewPrometheusClient to pass either 'query' or 'alert'.
@s-urbaniak is that expected? thanos-querier route not for Alerts? I get a 404 for /alerts

I found that you can pass the entire alert as a query, like so:
ALERTS{alertname="PodDisruptionBudgetAtLimit",alertstate="pending",namespace="pdbns",poddisruptionbudget="pdbname",prometheus="openshift-monitoring/k8s",service="kube-state-metrics",severity="warning"}==1
instead of using the deprecated prometheus-k8s route. From what I can find, you have to pass the full alert, since partial responses return an error w/ how we have thanos setup - that is stretching my knowledge of prometheus/thanos, correct me if you can.

@sallyom do you need to pass all of the properties in that query or only those you care about? Seems quite cumbersome at first.

@sallyom indeed, thanos querier does not support /alerts as it is "only" responsible for the query part. It doesn't scrape targets either as it is "just" a proxy in front of prometheus, hence /targets doesn't exist here either. Your ALERTS query is fine from my point of view, but passing just ALERTS{alertname="PodDisruptionBudgetAtLimit"} should work too.

marun · 2020-01-07T15:28:27Z

test/library/metrics/query.go

+		}
+		host = route.Status.Ingress[0].Host
+	case "alert":
+		route, err := rc.RouteV1().Routes("openshift-monitoring").Get("prometheus-k8s", metav1.GetOptions{})


@s-urbaniak This code suggests that thanos-querier doesn't support querying for alerts via the Alerts method of the prometheus client. Is there another way for tests to query for alerts similar to how an up query can replace use of the Targets method of the prometheus client? Or is testing of alerts dependent on the deprecated prometheus-k8s route?

^^ lol I found that out also, see above

as mentioned above you can use the ALERTS query to get the desired result. Indeed, the /alerts endpoint is not accessible via Thanos Querier, as it is "just" responsible for querying metrics.

marun · 2020-01-08T07:11:48Z

LGTM

mfojtik · 2020-01-08T14:52:31Z

/lgtm

deads2k · 2020-01-08T19:37:58Z

test/library/metrics/query.go

+					KeepAlive: 30 * time.Second,
+				}).DialContext,
+				TLSHandshakeTimeout: 10 * time.Second,
+				TLSClientConfig:     &tls.Config{InsecureSkipVerify: true},


You cannot send a bearer token to a destination without verifying the server you're speaking to.

What is the potential risk(s) you are concerned with? As per the test prefix of this path, this code is intended only to support e2e testing of metrics collection.

deads2k · 2020-01-08T19:38:11Z

/lgtm cancel

sallyom · 2020-01-09T19:09:02Z

@deads2k ptal, thanks! and thanks for pointing me in the direction of the correct router-ca configmap, that did the trick, I was initially using the service-ca-operator generated CA, not the correct router-ca

deads2k · 2020-01-13T13:14:00Z

/approve

openshift-ci-robot · 2020-01-13T13:14:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, mfojtik, sallyom, soltysh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [deads2k]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mfojtik · 2020-01-13T15:58:38Z

/lgtm

add helper functions for running prometheus query

openshift-ci-robot requested a review from sttts November 24, 2019 18:16

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 24, 2019

sallyom mentioned this pull request Nov 24, 2019

Bug 1771809: Add e2e for PodDisruptionBudgetAtLimit alert openshift/cluster-kube-controller-manager-operator#316

Merged

sallyom force-pushed the ensure-metrics-available branch from 8a89b34 to e5000e7 Compare November 25, 2019 13:26

sallyom mentioned this pull request Nov 25, 2019

Bug 1771809: add PodDisruptionBudgetAtLimit alert e2e openshift/origin#24169

Closed

sallyom force-pushed the ensure-metrics-available branch from e5000e7 to 7578d03 Compare November 25, 2019 15:45

marun reviewed Nov 26, 2019

View reviewed changes

pkg/metrics/query.go Outdated Show resolved Hide resolved

pkg/metrics/query.go Outdated Show resolved Hide resolved

pkg/metrics/query.go Outdated Show resolved Hide resolved

sallyom force-pushed the ensure-metrics-available branch from 7578d03 to d583fb2 Compare November 26, 2019 17:47

sallyom force-pushed the ensure-metrics-available branch from d583fb2 to 60688ba Compare November 26, 2019 22:40

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 27, 2019

marun suggested changes Nov 27, 2019

View reviewed changes

sallyom force-pushed the ensure-metrics-available branch 2 times, most recently from e3c1277 to dd5e01f Compare November 27, 2019 14:07

soltysh suggested changes Nov 27, 2019

View reviewed changes

sallyom force-pushed the ensure-metrics-available branch 15 times, most recently from ccef27b to be5ca46 Compare November 28, 2019 01:43

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 6, 2020

sallyom mentioned this pull request Jan 6, 2020

test for excessive etcd leadership changes openshift/origin#24291

Merged

marun reviewed Jan 7, 2020

View reviewed changes

marun mentioned this pull request Jan 7, 2020

Configure metrics collection for operator openshift/service-ca-operator#90

Merged

marun reviewed Jan 7, 2020

View reviewed changes

sallyom force-pushed the ensure-metrics-available branch from b7a7638 to af5efab Compare January 7, 2020 15:16

marun reviewed Jan 7, 2020

View reviewed changes

sallyom force-pushed the ensure-metrics-available branch 4 times, most recently from 564ed58 to 6ed7cdc Compare January 7, 2020 18:46

openshift-ci-robot assigned mfojtik Jan 8, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 8, 2020

deads2k reviewed Jan 8, 2020

View reviewed changes

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 8, 2020

sallyom force-pushed the ensure-metrics-available branch from 6ed7cdc to 9b4817f Compare January 9, 2020 19:01

add helper functions for running prometheus query

23320a9

sallyom force-pushed the ensure-metrics-available branch from 9b4817f to 23320a9 Compare January 9, 2020 19:03

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 13, 2020

stevekuznetsov added the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2020

openshift-merge-robot merged commit f2ca9aa into openshift:master Jan 13, 2020

bertinatto pushed a commit to bertinatto/library-go that referenced this pull request Jul 2, 2020

Merge pull request openshift#602 from sallyom/ensure-metrics-available

7f26b38

add helper functions for running prometheus query

add helper functions for running prometheus query #602

add helper functions for running prometheus query #602

Conversation

sallyom commented Nov 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sallyom commented Nov 26, 2019

Uh oh!

marun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

marun commented Jan 7, 2020

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!

marun Jan 7, 2020 • 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!

marun commented Jan 8, 2020

Uh oh!

mfojtik commented Jan 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marun Jan 8, 2020 • 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!

deads2k commented Jan 8, 2020

Uh oh!

sallyom commented Jan 9, 2020

Uh oh!

deads2k commented Jan 13, 2020

Uh oh!

openshift-ci-robot commented Jan 13, 2020

Uh oh!

mfojtik commented Jan 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

sallyom commented Nov 24, 2019 •

edited

Loading

marun Jan 7, 2020 •

edited

Loading

marun Jan 8, 2020 •

edited

Loading