Allow TechPreviewNoUpgrade alerts when running on a TechPreview cluster #26393
Conversation
|
Have now tested these against a live cluster and they pass when the FeatureGate is installed |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dgrisonnet, JoelSpeed 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 |
openshift-ci
bot
added
lgtm
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
20 similar comments
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
10 similar comments
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/hold these tests are broken and not required for this to merge, looking for someone to override the BZ requirement |
openshift-ci
bot
added
the
do-not-merge/hold
| "PrometheusRemoteWriteDesiredShards", | ||
| } | ||
|
|
||
| if isTechPreviewCluster(oc) { |
There was a problem hiding this comment.
Why do you need this and allowedFiringAlerts above?
There was a problem hiding this comment.
These are two separate test cases that cover slightly different things as far as I can tell. The first excludes these two alerts from its check while this one checks they are firing when they're supposed to IIUC
There was a problem hiding this comment.
Oh, so it's not allowedAlertNames, it's requiredAlertNames? if so, can I get an update?
There was a problem hiding this comment.
Nope, I got that wrong, the difference is one is in the normal tests and one is in the late tests. They both check that alerts aren't firing apart from some allow list, so the allow is the correct name.
@dgrisonnet Do you know what the concrete difference is between this test shouldn't report any alerts in firing state apart from Watchdog and AlertmanagerReceiversNotConfigured [Early] and the late shouldn't report any alerts in firing or pending state apart from Watchdog and AlertmanagerReceiversNotConfigured and have no gaps in Watchdog firing?
Edit: Or the motivation for having both?
There was a problem hiding this comment.
The Early one makes sure that any alerts that are supposed to fire instantly, during the cluster initialization are disregarded, whereas the other one disregards alerts that may fire during the e2e tests.
In your case, I suppose that TechPreviewNoUpgrade fires instantly if there are any tech preview features enabled in the cluster right? If so, then it is correct to allow the alert in both cases since it can fire at all times.
There was a problem hiding this comment.
Yep that corrects, it fires instantly and continuously throughout the cluster life as soon as the cluster comes up
deads2k
added
the
bugzilla/valid-bug
|
updating a test for techpreview CI job. bypassing bug requirement |
|
/hold cancel Thanks David! |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
@JoelSpeed: The following tests failed, say
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. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
7 similar comments
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
As a part of openshift/release#19845, we are trying to add TechPreview periodic jobs as release informing jobs.
To enable this, we need to make sure that the tests pass when the cluster is healthy and only valid failures are reported.
Today, two tests fail because of the presence of a
TechPreviewNoUpgradealert created by kube-apiserver when aTechPreviewNoUpgradefeature gate is present. Eg https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_release/19845/rehearse-19845-periodic-ci-openshift-release-master-ci-4.9-e2e-openstack-techpreview-parallel/1425432301084348416To allow these tests to pass, we must allow the
TechPreviewNoUpgradealert when the feature gate is present.