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

check if ClusterMachineApproverDown UsingDeprecatedAPIExtensionsV1Beta1 alerts are not firing #24071

Merged
merged 1 commit into from Dec 20, 2019

Conversation

paulfantom
Copy link
Contributor

@openshift-ci-robot openshift-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 1, 2019
@s-urbaniak
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 1, 2019
@paulfantom
Copy link
Contributor Author

/retest

@@ -210,7 +210,7 @@ var _ = g.Describe("[Feature:Prometheus][Conformance] Prometheus", func() {
tests := map[string]bool{
// Checking Watchdog alert state is done in "should have a Watchdog alert in firing state".
// FIXME(paulfantom): ClusterMachineApproverDown and UsingDeprecatedAPIExtensionsV1Beta1 shouldn't be firing!
Copy link
Contributor

Choose a reason for hiding this comment

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

this line can be removed

@lilic
Copy link
Contributor

lilic commented Nov 4, 2019

/retest

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 4, 2019
@paulfantom
Copy link
Contributor Author

@simonpasquier removed comment.

Putting this on hold as UsingDeprecatedAPIExtensionsV1Beta1 is firing constantly. BZ for this issue is created at https://bugzilla.redhat.com/show_bug.cgi?id=1768417

/hold

@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 Nov 4, 2019
Copy link
Contributor

@simonpasquier simonpasquier 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 the lgtm Indicates that a PR is ready to be merged. label Nov 4, 2019
@tnozicka
Copy link
Contributor

tnozicka commented Nov 6, 2019

#24089 merged
/retest

@tnozicka
Copy link
Contributor

tnozicka commented Nov 6, 2019

openshift/cluster-kube-apiserver-operator#639 merged
/retest

@tnozicka
Copy link
Contributor

tnozicka commented Nov 7, 2019

/hold cancel
(the 2 PRs above fixed the e2e here)

@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 Nov 7, 2019
@openshift-bot
Copy link
Contributor

/retest

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

@tnozicka
Copy link
Contributor

tnozicka commented Nov 7, 2019

gcp-upgrade flaked on gcp
/retest

@tnozicka
Copy link
Contributor

tnozicka commented Nov 7, 2019

/hold
there seems to be a flake depending on when the test is run

@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 Nov 7, 2019
@tnozicka
Copy link
Contributor

/test e2e-aws-serial
/test e2e-gcp

@paulfantom
Copy link
Contributor Author

@tnozicka failure looks legitimate to me. Log message says:

ALERTS{alertname=\"UsingDeprecatedAPIExtensionsV1Beta1\", alertstate=\"firing\", client=\"kubectl/v0.0.0 (linux/amd64) kubernetes/$Format\", code=\"200\", component=\"apiserver\", contentType=\"application/json\", endpoint=\"https\", group=\"extensions\", instance=\"10.0.0.4:6443\", job=\"apiserver\", namespace=\"default\", resource=\"deployments\", scope=\"namespace\", service=\"kubernetes\", severity=\"warning\", verb=\"DELETE\", version=\"v1beta1\"} => 1
...

@tnozicka
Copy link
Contributor

tnozicka commented Nov 11, 2019

sure, it should fail, yet I see 2 run where it was green https://prow.svc.ci.openshift.org/pr-history/?org=openshift&repo=origin&pr=24071

I am working on a fix, tracing down some more offenders

@paulfantom
Copy link
Contributor Author

sure, I should fail, yet I see 2 run where it was green

Confirmed. Looks like we need to rewrite those tests to be running during a whole test suite run ASAP.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 21, 2019
@paulfantom
Copy link
Contributor Author

Rebased to resolve merge conflict

@paulfantom
Copy link
Contributor Author

/retest

@paulfantom
Copy link
Contributor Author

@tnozicka what is the status of fixing UsingDeprecatedAPIExtensionsV1Beta1 alerts? Asking because I just started 4.3.0-0.ci-2019-11-22-122829 and got 7 alerts with warning type and this isn't something we would like to ship to customers.

@paulfantom
Copy link
Contributor Author

/retest

@paulfantom
Copy link
Contributor Author

/hold cancel
/retest

@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 Dec 10, 2019
…precatedAPIExtensionsV1Beta1 alerts are not firing
@paulfantom
Copy link
Contributor Author

Rebased after changes from #24276

Copy link
Contributor

@lilic lilic 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 the lgtm Indicates that a PR is ready to be merged. label Dec 16, 2019
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LiliC, paulfantom, s-urbaniak, simonpasquier

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-bot
Copy link
Contributor

/retest

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

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

@lilic
Copy link
Contributor

lilic commented Dec 16, 2019

@tnozicka FYI in the latest nightly 4.4 cluster the UsingDeprecatedAPIExtensionsV1Beta1 is still firing, when will this issue be resolved do you know?

@openshift-bot
Copy link
Contributor

/retest

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

@lilic
Copy link
Contributor

lilic commented Dec 16, 2019

Also the UsingDeprecatedAPIExtensionsV1Beta1 is firing 20 times on my nightly 4.4 cluster, this should be amended to group those and only fire once, not sure how the alert is created?

@lilic
Copy link
Contributor

lilic commented Dec 16, 2019

/hold

Until the alert is fixed, so we don't keep retrying.

@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 Dec 16, 2019
@tnozicka
Copy link
Contributor

looks like people managed to merge new offenders in the meantime
one fix is here #24305

@lilic
Copy link
Contributor

lilic commented Dec 18, 2019

@tnozicka Yeah, guess this is why we need to enable them ASAP. Have you considered adding an e2e test to origin instead of an alert for this? As this will be firing on user clusters and I assume a lot of confusion will happen as users can't do anything about this from their side.

@p0lyn0mial
Copy link
Contributor

/retest

@soltysh
Copy link
Member

soltysh commented Dec 20, 2019

With the updated k8s where we DO NOT enable extensions/v1beta1 every tests that uses it should fail so this is free to merge.
/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 Dec 20, 2019
@openshift-merge-robot openshift-merge-robot merged commit 51e7bbc into openshift:master Dec 20, 2019
@paulfantom paulfantom deleted the test_alerts branch December 21, 2019 21:31
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. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants