Skip to content
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

Bug 1793850: Update UsingDeprecatedAPIExtensionsV1Beta1 alert #742

Merged

Conversation

soltysh
Copy link
Member

@soltysh soltysh commented Jan 28, 2020

/assign @tnozicka

@openshift-ci-robot openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Jan 28, 2020
@openshift-ci-robot
Copy link

@soltysh: This pull request references Bugzilla bug 1793850, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Bug 1793850: Update UsingDeprecatedAPIExtensionsV1Beta1 alert

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.

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 28, 2020
@soltysh soltysh force-pushed the update_extensions_alert branch 3 times, most recently from 913c80b to 5e71f13 Compare January 28, 2020 15:22
Copy link
Contributor

@tnozicka tnozicka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 28, 2020
group="extensions",
version="v1beta1",
}[24h]
) > 0
labels:
severity: warning
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a warning still?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

expr: |
apiserver_request_count{group="extensions",version="v1beta1",resource!~"ingresses|",client!~"hyperkube/.*|cluster-policy-controller/.*"}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're losing the exception in here for the velero-server which, as a piece of backup software, will scrape every api endpoint advertised in the system. Are these APIs completely removed in 4.4+, and no longer advertised?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer we add them if we need them. I don't see any reason to add it, yet.

@openshift-bot
Copy link
Contributor

/retest

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

8 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@soltysh
Copy link
Member Author

soltysh commented Jan 29, 2020

/retest

@openshift-bot
Copy link
Contributor

/retest

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

6 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

5 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@crawford
Copy link
Contributor

/hold

Something is wrong with your manifest:

error parsing 0000_90_kube-apiserver-operator_04_servicemonitor-apiserver.yaml: error parsing: error converting YAML to JSON: yaml: line 13: did not find expected key

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 30, 2020
@cblecker
Copy link
Member

cblecker commented Feb 4, 2020

/retest

@cblecker
Copy link
Member

cblecker commented Feb 5, 2020

@soltysh I'm not sure what's up with this. The error @crawford mentioned is consistent, but we don't modify L13 in this file.. and other PRs are merging.

@@ -110,8 +110,16 @@ spec:
rules:
- alert: UsingDeprecatedAPIExtensionsV1Beta1
annotations:
message: A client in the cluster is using deprecated extensions/v1beta1 API that will be removed soon.
summary: "A client is using a deprecated {{"{{"}} $labels.group {{"}}"}}/{{"{{"}} $labels.version {{"}}"}} API version"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't we say we don't need this alert anymore as the client can't use this extensions/v1beta1 anyways? 🤔

@lilic
Copy link
Contributor

lilic commented Feb 10, 2020

In general regarding these alerts about deprecated APIs my two cents:
They are really broken in 4.3, we have a long running cluster and someone created a DS with deprecated API and even after removal the alert has not gone away, because it does not use an increase. We silenced them, but they still show in the main console, so right now users can't get rid of these alerts. This needs to be fixed ASAP in 4.3 onwards. This is inducing alert fatique for users if we do not fix them, and making users ignore alerts in general.

tl;dr:

  • one alert for user namespaces, warning or info level is fine
  • lets group the clients into one, I don't want to see 4 alerts firing if someone creates one DS.
  • no alerts for openshift components only we can control, you can add a recording rule test in origin for this to prevent new merges for happening. Users can't influence this nor fix it, so its not an actionable alert.

Please ping me or @openshift/openshift-team-monitoring for any help, or advice, we should fix this soon. Thanks!

(It seems we have like 3 PRs open for this, forgot which was the original one I was pinged on 🤔 )

@cblecker
Copy link
Member

/retest

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 10, 2020
@soltysh
Copy link
Member Author

soltysh commented Feb 10, 2020

/retest

1 similar comment
@soltysh
Copy link
Member Author

soltysh commented Feb 10, 2020

/retest

@cblecker
Copy link
Member

Looks like CI is good now. Pulling the hold that @crawford put in here to stop the retest loop.

/hold cancel

@openshift-ci-robot 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 10, 2020
@cblecker
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 10, 2020
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker, soltysh, tnozicka

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:

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

@openshift-merge-robot openshift-merge-robot merged commit 4005318 into openshift:master Feb 10, 2020
@openshift-ci-robot
Copy link

@soltysh: All pull requests linked via external trackers have merged. Bugzilla bug 1793850 has been moved to the MODIFIED state.

In response to this:

Bug 1793850: Update UsingDeprecatedAPIExtensionsV1Beta1 alert

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.

@soltysh soltysh deleted the update_extensions_alert branch February 11, 2020 09:09
@s-urbaniak
Copy link
Contributor

@soltysh @cblecker It is a bummer that this got merged despite the comment in #742 (comment) :-( As @lilic mentioned this will cause alert fatigue and should rather be catched in e2e tests.

@lilic will submit a follow-up PR, or do you have concrete follow-up plans here?

@soltysh
Copy link
Member Author

soltysh commented Feb 11, 2020

@s-urbaniak @lilic sorry, this got merged w/o my notice, fix excluding openshift elements in #762

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants