Changing the severity of "missing runbook_url annotation for critical alerts" test case from flaky to failure #27987

vimalk78 · 2023-06-20T06:34:21Z

The following critical alerts do not have have runbook_url annotation added as exceptions:

OVNKubernetesNorthboundDatabaseClusterIDError
OVNKubernetesSouthboundDatabaseClusterIDError
OVNKubernetesNorthboundDatabaseLeaderError
OVNKubernetesSouthboundDatabaseLeaderError
OVNKubernetesNorthboundDatabaseMultipleLeadersError
OVNKubernetesSouthboundDatabaseMultipleLeadersError
OVNKubernetesNorthdInactive
KubeSchedulerDown
MultipleDefaultStorageClasses
MachineAPIOperatorMetricsCollectionFailing
MCDRebootError
ExtremelyHighIndividualControlPlaneMemory
HAProxyDown
ClusterOperatorDown
ClusterVersionOperatorDown

The check for runbook_url validity has been separated out into a new flaky test

vimalk78 · 2023-06-20T06:36:58Z

should the check for invalid runbook_url have its own list of exceptions?

vimalk78 · 2023-06-20T07:11:26Z

/retest

simonpasquier · 2023-06-20T07:48:54Z

test/extended/prometheus/prometheus.go

+			violations := sets.NewString()
+			runbook := string(alert.Annotations["runbook_url"])
+
+			if runbook != "" {
 				// If there's a 'runbook_url' annotation, make sure it's a
 				// valid URL and that we can fetch the contents.
 				if err := helper.ValidateURL(runbook, 10*time.Second); err != nil {


I would separate the test to perform 1) the semantic validation of the URL (e.g. it can be parsed by url.Parse() and the scheme is "http" or "https") and 2) the actual verification that the URL returns a 200 response code. The former can fail the test if it detects an invalid URL and the latter should only flake as the target might be temporarily unavailable.

The helper.ValidateURL function uses url.Parse(rawURL) and returns error if it fails to parse.
can i add check for "https" scheme in the same validation function?

https://github.com/openshift/origin/blob/master/test/extended/util/prometheus/helpers.go#L355

can i add check for "https" scheme in the same validation function?

yes and I would split the actual request to the URL into another function (e.g. QueryURL()).

vimalk78 · 2023-06-22T10:47:11Z

/retest

simonpasquier · 2023-06-23T09:39:09Z

test/extended/util/prometheus/helpers.go

+		return err
+	}
+
+	if u.Scheme != "https" {


"http" should be fine too.

simonpasquier · 2023-06-23T09:41:22Z

test/extended/util/prometheus/helpers.go

+	}
+
+	if u.Scheme != "https" {
+		errstr := fmt.Sprintf("ValidateURL(%q) URL scheme is not https", rawURL)


Suggested change

errstr := fmt.Sprintf("ValidateURL(%q) URL scheme is not https", rawURL)

errstr := fmt.Sprintf("%q: URL scheme is invalid, it should be 'http' or 'https'", rawURL)

simonpasquier · 2023-06-23T09:42:16Z

test/extended/util/prometheus/helpers.go

+		return fmt.Errorf(errstr)
+	}
+	if u.Host == "" {
+		errstr := fmt.Sprintf("ValidateURL(%q) URL does not have a host part", rawURL)


Suggested change

errstr := fmt.Sprintf("ValidateURL(%q) URL does not have a host part", rawURL)

errstr := fmt.Sprintf("%q: host should not be empty", rawURL)

simonpasquier · 2023-06-23T09:43:02Z

test/extended/util/prometheus/helpers.go

+
+	if u.Scheme != "https" {
+		errstr := fmt.Sprintf("ValidateURL(%q) URL scheme is not https", rawURL)
+		framework.Logf(errstr)


do we need to log it here since it's already done by the caller?

simonpasquier · 2023-06-23T09:43:09Z

test/extended/util/prometheus/helpers.go

+	}
+	if u.Host == "" {
+		errstr := fmt.Sprintf("ValidateURL(%q) URL does not have a host part", rawURL)
+		framework.Logf(errstr)


same question here

simonpasquier · 2023-06-23T09:45:30Z

test/extended/prometheus/prometheus.go

@@ -174,8 +253,6 @@ var _ = g.Describe("[sig-instrumentation][Late] OpenShift alerting rules [apigro
 		})

 		if err != nil {
-			// We are still gathering data on how many alerts need to
-			// be fixed, so this is marked as a flake for now.
 			testresult.Flakef(err.Error())


Suggested change

testresult.Flakef(err.Error())

// We can't fail the test because the runbook URLs might be temporarily unavailable.

// At least we can manually check the CI logs to identify buggy URLs.

testresult.Flakef(err.Error())

simonpasquier · 2023-06-23T09:47:39Z

test/extended/prometheus/prometheus.go

+		}
+	})
+
+	g.It("should have valid runbook URLs", func() {


Suggested change

g.It("should have valid runbook URLs", func() {

g.It("should link to a valid URL if the runbook_url annotation is defined", func() {

(nit) to be more specific and distinguish from the next test

Suggested change

g.It("should have valid runbook URLs", func() {

g.It("should link to an HTTP(S) location if the runbook_url annotation is defined", func() {

simonpasquier · 2023-06-23T09:48:53Z

test/extended/prometheus/prometheus.go

+			violations := sets.NewString()
+			runbook_url := string(alert.Annotations["runbook_url"])
+
+			if runbook_url != "" {


(nit) easier to read.

Suggested change

if runbook_url != "" {

if runbook_url == "" {

return nil

}

simonpasquier · 2023-06-23T09:51:31Z

test/extended/prometheus/prometheus.go

+				// If there's a 'runbook_url' annotation, make sure that it is a valid URL
+				if err := helper.ValidateURL(runbook_url); err != nil {
+					violations.Insert(
+						fmt.Sprintf("WARNING: Alert %q has an 'runbook_url' annotation: %q which is not valid. Error: %v",


ForEachAlertingRule() always include the name of the alert.

Suggested change

fmt.Sprintf("WARNING: Alert %q has an 'runbook_url' annotation: %q which is not valid. Error: %v",

fmt.Sprintf("has an 'runbook_url' annotation which is not valid: %v",

simonpasquier · 2023-06-23T09:52:47Z

test/extended/prometheus/prometheus.go

 					violations.Insert(
-						fmt.Sprintf("WARNING: Alert %q has an invalid 'runbook_url' annotation: %v",
-							alert.Name, err),
+						fmt.Sprintf("WARNING: Alert %q has an 'runbook_url' annotation: %q which cannot be fetched. Error: %v",


same comment about the alert name already being included

Suggested change

fmt.Sprintf("WARNING: Alert %q has an 'runbook_url' annotation: %q which cannot be fetched. Error: %v",

fmt.Sprintf("has a runbook URL which cannot be fetched: %v",

simonpasquier

one last suggestion

simonpasquier · 2023-06-26T09:25:19Z

test/extended/prometheus/prometheus.go

+		}
+	})
+
+	g.It("should have valid runbook URLs", func() {


(nit) to be more specific and distinguish from the next test

Suggested change

g.It("should have valid runbook URLs", func() {

g.It("should link to an HTTP(S) location if the runbook_url annotation is defined", func() {

simonpasquier · 2023-06-27T08:34:21Z

@vimalk78 you need to regenerate some files to fix ci/prow/verify.

… alerts" test case from flaky to failure The following critical alerts do not have have runbook_url annotation added as exceptions: - OVNKubernetesNorthboundDatabaseClusterIDError - OVNKubernetesSouthboundDatabaseClusterIDError - OVNKubernetesNorthboundDatabaseLeaderError - OVNKubernetesSouthboundDatabaseLeaderError - OVNKubernetesNorthboundDatabaseMultipleLeadersError - OVNKubernetesSouthboundDatabaseMultipleLeadersError - OVNKubernetesNorthdInactive - KubeSchedulerDown - MultipleDefaultStorageClasses - MachineAPIOperatorMetricsCollectionFailing - MCDRebootError - ExtremelyHighIndividualControlPlaneMemory - HAProxyDown - ClusterOperatorDown - ClusterVersionOperatorDown The check for runbook_url validity has been separated out into two new tests - test which checks for URL validity. This fails the test if URL is invalid - test which fetches the URL to check its existance. This check makes test flaky, and does not fail the test if a URL cannot be fetched.

simonpasquier · 2023-06-28T09:57:28Z

/skip

danielmellado · 2023-06-28T10:48:07Z

/retest-required

openshift-ci · 2023-06-28T13:02:00Z

@vimalk78: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-aws-ovn-single-node	`681db23`	link	false	`/test e2e-aws-ovn-single-node`
ci/prow/e2e-agnostic-ovn-cmd	`681db23`	link	false	`/test e2e-agnostic-ovn-cmd`
ci/prow/e2e-metal-ipi-sdn	`681db23`	link	false	`/test e2e-metal-ipi-sdn`
ci/prow/e2e-gcp-ovn-rt-upgrade	`681db23`	link	false	`/test e2e-gcp-ovn-rt-upgrade`
ci/prow/e2e-aws-ovn-single-node-serial	`681db23`	link	false	`/test e2e-aws-ovn-single-node-serial`
ci/prow/e2e-aws-ovn-upgrade	`681db23`	link	false	`/test e2e-aws-ovn-upgrade`
ci/prow/e2e-aws-csi	`681db23`	link	false	`/test e2e-aws-csi`
ci/prow/e2e-metal-ipi-ovn-ipv6	`681db23`	link	false	`/test e2e-metal-ipi-ovn-ipv6`

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.

simonpasquier

/lgtm

openshift-ci · 2023-06-28T16:29:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: simonpasquier, vimalk78

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:

~~test/extended/prometheus/OWNERS~~ [simonpasquier]
~~test/extended/util/annotate/generated/OWNERS~~ [simonpasquier]
~~test/extended/util/prometheus/OWNERS~~ [simonpasquier]

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

openshift-ci bot requested review from jan--f and simonpasquier June 20, 2023 06:35

simonpasquier reviewed Jun 20, 2023

View reviewed changes

vimalk78 force-pushed the runbook-url-prom-test branch from 03bd912 to a9b3aa7 Compare June 21, 2023 18:25

simonpasquier reviewed Jun 23, 2023

View reviewed changes

test/extended/util/prometheus/helpers.go Outdated

return err

}

if u.Scheme != "https" {

Copy link

Contributor

simonpasquier Jun 23, 2023

Choose a reason for hiding this comment

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

"http" should be fine too.

simonpasquier reviewed Jun 23, 2023

View reviewed changes

vimalk78 force-pushed the runbook-url-prom-test branch from a9b3aa7 to fd08894 Compare June 26, 2023 08:03

simonpasquier reviewed Jun 26, 2023

View reviewed changes

vimalk78 force-pushed the runbook-url-prom-test branch from fd08894 to 34c3ae0 Compare June 27, 2023 01:16

vimalk78 force-pushed the runbook-url-prom-test branch from 34c3ae0 to 681db23 Compare June 27, 2023 17:02

simonpasquier reviewed Jun 28, 2023

View reviewed changes

openshift-ci bot assigned simonpasquier Jun 28, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 28, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 28, 2023

openshift-merge-robot merged commit 1f3163d into openshift:master Jun 28, 2023
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changing the severity of "missing runbook_url annotation for critical alerts" test case from flaky to failure #27987

Changing the severity of "missing runbook_url annotation for critical alerts" test case from flaky to failure #27987

vimalk78 commented Jun 20, 2023

vimalk78 commented Jun 20, 2023

vimalk78 commented Jun 20, 2023

simonpasquier Jun 20, 2023

vimalk78 Jun 20, 2023

simonpasquier Jun 20, 2023

vimalk78 commented Jun 22, 2023

simonpasquier Jun 23, 2023

simonpasquier Jun 23, 2023

simonpasquier Jun 23, 2023

simonpasquier Jun 23, 2023

simonpasquier Jun 23, 2023

simonpasquier Jun 23, 2023

simonpasquier Jun 23, 2023

simonpasquier Jun 26, 2023

simonpasquier Jun 23, 2023

simonpasquier Jun 23, 2023

simonpasquier Jun 23, 2023

simonpasquier left a comment

simonpasquier Jun 26, 2023

simonpasquier commented Jun 27, 2023

simonpasquier commented Jun 28, 2023

danielmellado commented Jun 28, 2023

openshift-ci bot commented Jun 28, 2023

simonpasquier left a comment

openshift-ci bot commented Jun 28, 2023

	errstr := fmt.Sprintf("ValidateURL(%q) URL scheme is not https", rawURL)
	errstr := fmt.Sprintf("%q: URL scheme is invalid, it should be 'http' or 'https'", rawURL)

	errstr := fmt.Sprintf("ValidateURL(%q) URL does not have a host part", rawURL)
	errstr := fmt.Sprintf("%q: host should not be empty", rawURL)

	g.It("should have valid runbook URLs", func() {
	g.It("should link to a valid URL if the runbook_url annotation is defined", func() {

	g.It("should have valid runbook URLs", func() {
	g.It("should link to an HTTP(S) location if the runbook_url annotation is defined", func() {

-			if runbook_url != "" {
+			if runbook_url == "" {
+				return nil
+			}

	fmt.Sprintf("WARNING: Alert %q has an 'runbook_url' annotation: %q which is not valid. Error: %v",
	fmt.Sprintf("has an 'runbook_url' annotation which is not valid: %v",

	fmt.Sprintf("WARNING: Alert %q has an 'runbook_url' annotation: %q which cannot be fetched. Error: %v",
	fmt.Sprintf("has a runbook URL which cannot be fetched: %v",

Changing the severity of "missing runbook_url annotation for critical alerts" test case from flaky to failure #27987

Changing the severity of "missing runbook_url annotation for critical alerts" test case from flaky to failure #27987

Conversation

vimalk78 commented Jun 20, 2023

vimalk78 commented Jun 20, 2023

vimalk78 commented Jun 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vimalk78 commented Jun 22, 2023

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

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonpasquier commented Jun 27, 2023

simonpasquier commented Jun 28, 2023

danielmellado commented Jun 28, 2023

openshift-ci bot commented Jun 28, 2023

simonpasquier left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jun 28, 2023