add DeprecatedAPIInUse alert #1018

sanchezl · 2020-12-08T15:22:45Z

Warn via an info alert that an API that will be removed in the next OpenShift version is currently in use.

sanchezl · 2020-12-14T03:04:34Z

/retest

sanchezl · 2020-12-14T19:25:52Z

/test k8s-e2e-gcp
/test e2e-aws-operator

openshift-merge-robot · 2020-12-14T20:39:43Z

@sanchezl: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/k8s-e2e-gcp	`974ddc0`	link	`/test k8s-e2e-gcp`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

wking · 2020-12-16T21:02:08Z

manifests/0000_90_kube-apiserver-operator_04_servicemonitor-apiserver.yaml

+          message: |
+            Deprecated API that will be removed in the next version is being used: {{"{{$labels.group}}"}}.{{"{{$labels.version}}"}}/{{"{{$labels.resource}}"}}
+        expr: |
+          group(apiserver_requested_deprecated_apis{removed_release="1.20"}) by (group,version,resource,subresource) * on(group,version,resource,subresource) group_right() apiserver_request_total


master is 1.20, right, so shouldn't this be removed_release="1.21" here? And then changed to 1.20 if/when this gets backported to 4.6?

@wking, you are correct. We weren't on 1.20 yet when I originally started this.

simonpasquier

the problem with such alert is that it will never clear by itself. That's already been discussed in https://bugzilla.redhat.com/show_bug.cgi?id=1793850 and #742 with the UsingDeprecatedAPIExtensionsV1Beta1 alert. Eventually this alert was removed in #764. Are we sure that we want this again? A test in origin e2e might be better suited?

cc @lilic @s-urbaniak

lilic · 2020-12-17T13:25:08Z

Agree with Simon, this was a huge pain for both our SREs and customers (info is in the Bugzilla linked by Simon). We need all alerts to be able to be resolvable by our customers, but our customers cannot control openshift operators only their own workload.

Adding an e2e test based on these metrics makes perfect sense. There was also talk about adding notifications in the console to warn users of deprecated APIs.

deads2k · 2021-01-04T18:02:02Z

@lilic @simonpasquier How should a customer be warned that a deprecated API, which is going to be removed is being used in their cluster and will result in a disruption for their particular workload?

Not all workloads come in OCP payloads, but any workload using the deprecated APIs will be impacted when they are removed in a following release. I'm open to other solutions, but I'd like to know what that solution is and what we should do to manage that in 1.22 as the first round of APIs like CRD/v1beta1 are removed.

A test in origin e2e might be better suited?

An e2e test in origin does not help customers who are running code using these old APIs that is not in an OCP payload.

lilic · 2021-01-05T13:30:21Z

I think the console team has notifications either in place already or are planning on adding, those are more appropriate for this, I would suggest following up with them. Anything that does not page admins, but just shows notifications to users in the console is a better user experience in my opinion.

sttts · 2021-01-05T15:12:32Z

those are more appropriate for this

@lilic technically both are feasible obviously. Sounds like we are moving into product manager or architect territory, how to best present that information to users. Are we sure that with console notifications we won't get "nobody warned me about the broken workloads after upgrade. I never look into the UI." sort of complains?

To repeat what @deads2k said between the lines: we need this before 1.22 is upgraded to. So in 4.8 the latest. If console team (@jhadvig) is not finished with notifications like that (metrics based notification in console), this is going to be too late.

Instead of being too late and breaking customers, I would prefer a little less ideal user experience (to be decided that it actually is) of metric based alerts in 4.8.

cc @mcurry-rh @derekwaynecarr @jhadvig

lilic · 2021-01-06T15:36:25Z

Are we sure that with console notifications we won't get "nobody warned me about the broken workloads after upgrade. I never look into the UI." sort of complains?

Some of our users also do not use alertmanager and never configure it, so same can be said for alerts really.

In any case, I believe we discussed offline and sort of agreed on info level alert for this is fine if there are no other alternatives. And @simonpasquier provided with a better alerti2ng rule, so that users can actually resolve this alert. And that is should be just for non openshift workloads IIRC? cc @s-urbaniak

RiRa12621 · 2021-01-06T15:44:15Z

manifests/0000_90_kube-apiserver-operator_04_servicemonitor-apiserver.yaml

+          group(apiserver_requested_deprecated_apis{removed_release="1.20"}) by (group,version,resource,subresource) * on(group,version,resource,subresource) group_right() apiserver_request_total
+        for: 1h
+        labels:
+          severity: warning


This is barely actionable and neither urgent and shouldn't be higher severity than "info"

s-urbaniak · 2021-01-06T15:56:03Z

I agree that we are leveraging alerting for use cases it has not been envisioned for and am +1 on using info level only for this kind of workaround as discussed OOB.

RiRa12621 · 2021-01-06T18:23:53Z

@s-urbaniak can probably clarify but iirc all alerts are presented in the console. That should include info level.
So we'd get the same effect but no pages that are giving admins the noisy feelings.

lilic

Couple of comments.

lilic · 2021-01-08T15:14:26Z

manifests/0000_90_kube-apiserver-operator_04_servicemonitor-apiserver.yaml

+      - alert: DeprecratedAPIUsed
+        annotations:
+          message: |
+            Deprecated API that will be removed in the next version is being used: {{"{{$labels.group}}"}}.{{"{{$labels.version}}"}}/{{"{{$labels.resource}}"}}


We mentioned to also say how to fix this, e.g. "Remove the workload that is using this API, otherwise " anything that is helpful to end users, who receive this page.

Also is it not possible to get a namespace for this to display?

Updated to:

Deprecated API that will be removed in the next version¹ is being used. Removing the workload that is using the group.version/resource API might² be necessary for a successful upgrade to the next cluster version.

¹ by next version here we mean next y-stream version. Is there a more exact, customer-facing terminology to use here to make it clear that a z-stream version would not remove an API? For example, next minor version, but the term minor version is not well defined in the documentation.
² I say might here because the deprecated API usage might be in the kube control plane itself, which of course would be updated in the next version to not use the deprecated APIs, but I can't easily filter that use in the metric/alert.

Also is it not possible to get a namespace for this to display?
@lilic The namespace is not available.

lilic · 2021-01-08T15:14:35Z

manifests/0000_90_kube-apiserver-operator_04_servicemonitor-apiserver.yaml

+            Deprecated API that will be removed in the next version is being used: {{"{{$labels.group}}"}}.{{"{{$labels.version}}"}}/{{"{{$labels.resource}}"}}
+        expr: |
+          group(apiserver_requested_deprecated_apis{removed_release="1.21"}) by (group,version,resource,subresource) * on(group,version,resource,subresource) group_right() rate(apiserver_request_total[10m]) > 0
+        for: 1m


Any reason why its 1minute?

Fixed typo.

sanchezl · 2021-01-25T15:00:21Z

/retest

lilic

lgtm, thanks so much for addressing our comments!

@simonpasquier leaving up to Simon in case he has any more comments.

sttts · 2021-02-01T13:19:25Z

manifests/0000_90_kube-apiserver-operator_04_servicemonitor-apiserver.yaml

+            the {{"{{$labels.group}}"}}.{{"{{$labels.version}}"}}/{{"{{$labels.resource}}"}} API might be necessary for
+            a successful upgrade to the next cluster version.
+        expr: |
+          group(apiserver_requested_deprecated_apis{removed_release="1.21"}) by (group,version,resource,subresource) * on(group,version,resource,subresource) group_right() rate(apiserver_request_total[10m]) > 0


how do we make sure this version is updated each release?

If you want to you can either edit for the next release so in 4.8 you would replace or add an or for 1.22, or use a regex matcher here that would match against multiple versions, see -> https://prometheus.io/docs/prometheus/latest/querying/basics/#time-series-selectors

my question was more about how we remember this in practice. We could add CI test that compares the value with the running server version, in order to be reminded during rebase.

Yes that is a good idea, or something along the lines of creating one of the deprecated APIs and evaluating if the alert is firing.

@sttts Added an e2e test.

Suggestion to get rid of the on(...) group_right() part

Suggested change

group(apiserver_requested_deprecated_apis{removed_release="1.21"}) by (group,version,resource,subresource) * on(group,version,resource,subresource) group_right() rate(apiserver_request_total[10m]) > 0

group(apiserver_requested_deprecated_apis{removed_release="1.21"}) by (group,version,resource,subresource) and (sum by(group,version,resource,subresource) (rate(apiserver_request_total[10m]))) > 0

You can even remove the subresource label from the by() clauses since it's not used in the annotations.

@simonpasquier Thank you. Updated.

sttts · 2021-02-01T13:19:39Z

manifests/0000_90_kube-apiserver-operator_04_servicemonitor-apiserver.yaml

+          message: >-
+            Deprecated API that will be removed in the next version is being used. Removing the workload that is using
+            the {{"{{$labels.group}}"}}.{{"{{$labels.version}}"}}/{{"{{$labels.resource}}"}} API might be necessary for
+            a successful upgrade to the next cluster version.


Please refer to audit logs to find out the actor.

simonpasquier · 2021-02-04T10:07:06Z

lgtm

deads2k · 2021-02-11T16:40:10Z

test/e2e/deprecated_api_test.go

+	"k8s.io/client-go/discovery"
+)
+
+func TestDeprecatedAPIInUse(t *testing.T) {


good test. It will break every kube bump. I find this acceptable since it won't block the kube bump.

👍 @sanchezl ignore my ask for a test that breaks elsewhere. This is it I guess.

deads2k · 2021-02-11T16:42:04Z

manifests/0000_90_kube-apiserver-operator_04_servicemonitor-apiserver.yaml

+            a successful upgrade to the next cluster version. Refer to the audit logs to identify the workload.
+        expr: |
+          group(apiserver_requested_deprecated_apis{removed_release="1.21"}) by (group,version,resource) and (sum by(group,version,resource) (rate(apiserver_request_total[10m]))) > 0
+        for: 1h


if we use 4h, will that cover the entire timeframe of an e2e test? Is this the spot or the rate the spot? Probably here I guess.

Followed up at #1044.

deads2k · 2021-02-11T16:42:46Z

/lgtm
/hold

holding so we can decide about the exact timeframe.

openshift-ci-robot · 2021-02-11T16:43:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, sanchezl

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

The pull request process is described here

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

sanchezl · 2021-02-12T17:21:28Z

/retest

sanchezl · 2021-02-15T05:56:57Z

/retest

deads2k · 2021-02-15T15:05:36Z

/hold cancel

sanchezl · 2021-02-15T19:51:27Z

/test e2e-aws-serial

openshift-bot · 2021-02-15T21:28:40Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-02-15T23:12:40Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-02-16T00:17:41Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot requested review from mfojtik and soltysh December 8, 2020 15:23

sanchezl force-pushed the deprecated-alert branch 3 times, most recently from f0f5b90 to 974ddc0 Compare December 8, 2020 16:34

wking reviewed Dec 16, 2020

View reviewed changes

simonpasquier reviewed Dec 17, 2020

View reviewed changes

RiRa12621 suggested changes Jan 6, 2021

View reviewed changes

sanchezl force-pushed the deprecated-alert branch from 974ddc0 to 7729380 Compare January 6, 2021 16:24

lilic reviewed Jan 8, 2021

View reviewed changes

sanchezl force-pushed the deprecated-alert branch from 7729380 to 3c30e1b Compare January 19, 2021 16:11

lilic reviewed Feb 1, 2021

View reviewed changes

sttts reviewed Feb 1, 2021

View reviewed changes

sanchezl force-pushed the deprecated-alert branch from 3c30e1b to ab00e11 Compare February 3, 2021 16:22

sanchezl changed the title ~~add DeprecatedAPIUsed alert~~ add DeprecatedAPIInUse alert Feb 3, 2021

sanchezl added 3 commits February 3, 2021 16:12

add DeprecatedAPIInUse alert

bdbb7b7

e2e: ensure removed_release is updated for release

2d5adef

go mod vendor

d4df862

sanchezl force-pushed the deprecated-alert branch from ab00e11 to d4df862 Compare February 3, 2021 21:13

deads2k reviewed Feb 11, 2021

View reviewed changes

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

openshift-ci-robot assigned deads2k Feb 11, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 11, 2021

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 11, 2021

sttts mentioned this pull request Feb 15, 2021

Bug 1949593: rename DeprecatedAPIInUse alert to APIRemovedInNextReleaseInUse #1045

Merged

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 15, 2021

openshift-merge-robot merged commit 249c250 into openshift:master Feb 16, 2021

	group(apiserver_requested_deprecated_apis{removed_release="1.21"}) by (group,version,resource,subresource) * on(group,version,resource,subresource) group_right() rate(apiserver_request_total[10m]) > 0
	group(apiserver_requested_deprecated_apis{removed_release="1.21"}) by (group,version,resource,subresource) and (sum by(group,version,resource,subresource) (rate(apiserver_request_total[10m]))) > 0

add DeprecatedAPIInUse alert #1018

add DeprecatedAPIInUse alert #1018

Conversation

sanchezl commented Dec 8, 2020 • edited

sanchezl commented Dec 14, 2020

sanchezl commented Dec 14, 2020

openshift-merge-robot commented Dec 14, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonpasquier left a comment

Choose a reason for hiding this comment

lilic commented Dec 17, 2020

deads2k commented Jan 4, 2021 • edited

lilic commented Jan 5, 2021

sttts commented Jan 5, 2021

lilic commented Jan 6, 2021

Choose a reason for hiding this comment

s-urbaniak commented Jan 6, 2021

RiRa12621 commented Jan 6, 2021

lilic 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

sanchezl commented Jan 25, 2021

lilic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts Feb 1, 2021 • 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

simonpasquier commented Feb 4, 2021

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

deads2k commented Feb 11, 2021

openshift-ci-robot commented Feb 11, 2021

sanchezl commented Feb 12, 2021

sanchezl commented Feb 15, 2021

deads2k commented Feb 15, 2021

sanchezl commented Feb 15, 2021

openshift-bot commented Feb 15, 2021

openshift-bot commented Feb 15, 2021

openshift-bot commented Feb 16, 2021

sanchezl commented Dec 8, 2020 •

edited

openshift-merge-robot commented Dec 14, 2020 •

edited

deads2k commented Jan 4, 2021 •

edited

sttts Feb 1, 2021 •

edited