Revert "test/extended/prometheus: Validate alerting rules"

[stbenjam]

Reverts #26476

Many jobs are failing - OVN, and vsphere - due to this new test. This test should be changed to flaking, or have a list of exceptions while they are being fixed.

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: stbenjam
To complete the pull request process, please assign bparees after the PR has been reviewed.
You can assign the PR to them by writing /assign @bparees in a comment when ready.

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

Needs approval from an approver in each of these files:

• test/extended/OWNERS

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

[deads2k]

Manually merging revert because of permafails across numerous jobs (to be listed by Stephen). Before being un-reverted, the permafails must be eliminated. Same as product code, we are biased toward revert instead of fixes. This is the third such revert in the past week.

[dgrisonnet]

I haven't looked at all the failures yet, but I think it would have been better to update the exception list rather than reverting the whole test since this test comes with one.
From what I can see we missed the OVN specific alerts when adding this test, but the challenge when merging such a test is that new alerts breaking our guidelines might be merged before this test is, so it will result in yet another batch of permanent failures. To prevent that, I think we should try to run the test on all possible jobs to get the infrastructure-specific alerts allow listed, and then if we are still seeing failures once the test is merged, update the allow list with the few alerts that were not detected beforehand. Otherwise, we will go back and forth with reverting this job every time an alert wasn't tested against this test before being merged. cc @bison

[deads2k]

I haven't looked at all the failures yet, but I think it would have been better to update the exception list rather than reverting the whole test since this test comes with one.
From what I can see we missed the OVN specific alerts when adding this test, but the challenge when merging such a test is that new alerts breaking our guidelines might be merged before this test is, so it will result in yet another batch of permanent failures. To prevent that, I think we should try to run the test on all possible jobs to get the infrastructure-specific alerts allow listed, and then if we are still seeing failures once the test is merged, update the allow list with the few alerts that were not detected beforehand. Otherwise, we will go back and forth with reverting this job every time an alert wasn't tested against this test before being merged. cc @bison

The unrevert can add an exception list and get into a working condition before merging. With large areas of CI stuck, the priority is getting back to a functional state. The unrevert+exceptions can come in another PR that gets the exceptions correct before merging.

Otherwise, we will go back and forth with reverting this job every time an alert wasn't tested against this test before being merged.

I don't see this as the case. If you want to make an unenforcing test to find offenders first, that's a very good idea. If you want to run all the jobs before merging, that's another approach. There is no reason to believe this cannot be done as PR without sacrificing payload promotion and entire platforms of CI jobs.

[simonpasquier]

Looking at the job failures, they all relate to missing annotations (either description or summary or both). We would have added exceptions if we knew about these failures but unfortunately they weren't exercised in #26476. @deads2k is there any way this could have been prevented?

For OVN

Alerting rule "NetworkPodsCrashLooping" (group: cluster-network-operator-ovn.rules) has no 'description' annotation
Alerting rule "NoOvnMasterLeader" (group: cluster-network-operator-master.rules) has no 'description' annotation
Alerting rule "NoRunningOvnMaster" (group: cluster-network-operator-master.rules) has no 'description' annotation
Alerting rule "NodeWithoutOVNKubeNodePodRunning" (group: cluster-network-operator-ovn.rules) has no 'description' annotation
Alerting rule "NorthboundStale" (group: cluster-network-operator-master.rules) has no 'description' annotation
Alerting rule "SouthboundStale" (group: cluster-network-operator-master.rules) has no 'description' annotation
Alerting rule "V4SubnetAllocationThresholdExceeded" (group: cluster-network-operator-master.rules) has no 'description' annotation
Alerting rule "V6SubnetAllocationThresholdExceeded" (group: cluster-network-operator-master.rules) has no 'description' annotation

For vSphere

Alerting rule "VSphereOpenshiftClusterHealthFail" (group: vsphere-problem-detector.rules) has no 'description' annotation, but has a 'message' annotation. OpenShift alerts must use 'description' -- consider renaming the annotation
Alerting rule "VSphereOpenshiftClusterHealthFail" (group: vsphere-problem-detector.rules) has no 'summary' annotation
Alerting rule "VSphereOpenshiftNodeHealthFail" (group: vsphere-problem-detector.rules) has no 'description' annotation, but has a 'message' annotation. OpenShift alerts must use 'description' -- consider renaming the annotation
Alerting rule "VSphereOpenshiftNodeHealthFail" (group: vsphere-problem-detector.rules) has no 'summary' annotation

[deads2k]

Looking at the job failures, they all relate to missing annotations (either description or summary or both). We would have added exceptions if we knew about these failures but unfortunately they weren't exercised in #26476. @deads2k is there any way this could have been prevented?

Yes. You could create a non-failing test to gather data. Or you could manually run vsphere and ovn jobs in the origin repo.

[simonpasquier]

Ack, we didn't anticipated the side effect and we were too confident about the signal we got from the CI on the particular PR. Lesson learned for the future...

[bison]

What's the best way to make a non-failing test? Marking it as a flake means it shows as passing, right? Do we do that, then just manually inspect the logs for runs over the next few days?

[simonpasquier]

@bison my understanding is that we should use testresult.Flakef() instead of framework.Failf(). Then after it merges, we look for offenders with https://search.ci.openshift.org.

origin/test/extended/prometheus/prometheus.go

Line 208 in e000efe

testresult.Flakef("Unexpected alert behavior during test:\n\n%s", strings.Join(flakes.List(), "\n"))

[bison]

Yeah, that makes sense. I just wasn't sure exactly how to search for flakes. Anyway, I'll get a new PR up this morning.

[bison]

New PR: #26504