Add e2e tests for metrics #11307

machadovilaca · 2024-02-20T13:54:09Z

What this PR does

Before this PR:

KubeVirt had no e2e tests validating that metrics were being scrapped by Prometheus

After this PR:

KubeVirt has e2e tests validating that metrics from all components are being scrapped by Prometheus

jira-ticket: https://issues.redhat.com/browse/CNV-29328

Fixes #

Why we need it and why it was done in this way

The following tradeoffs were made:

The following alternatives were considered:

Links to places where the discussion took place:

Special notes for your reviewer

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

Design: A design document was considered and is present (link) or not required
PR: The PR description is expressive enough and will help future contributors
Code: Write code that humans can understand and Keep it simple
Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
Upgrade: Impact of this change on upgrade flows was considered and addressed if required
Testing: New code requires new unit tests. New features and bug fixes require at least on e2e test
Documentation: A user-guide update was considered and is present (link) or not required. You want a user-guide update if it's a user facing feature / API change.
Community: Announcement to kubevirt-dev was considered

Release note

Add e2e tests for metrics

machadovilaca · 2024-02-20T13:54:29Z

/cc @sradco @enp0s3

machadovilaca · 2024-02-20T17:21:40Z

/retest

machadovilaca · 2024-02-21T18:18:56Z

/hold
needs #11264

machadovilaca · 2024-02-26T10:56:46Z

/unhold

machadovilaca · 2024-02-26T11:05:14Z

/test pull-kubevirt-e2e-k8s-1.29-sig-monitoring

machadovilaca · 2024-02-26T12:52:59Z

/retest

enp0s3

@machadovilaca Hi, thank you for the PR!
can you please check the file tests/infrastructure/prometheus.go and see if there are no duplicates? if you can arrange all the monitoring e2e under one directory it could be great.

machadovilaca · 2024-02-27T11:28:33Z

@machadovilaca Hi, thank you for the PR! can you please check the file tests/infrastructure/prometheus.go and see if there are no duplicates? if you can arrange all the monitoring e2e under one directory it could be great.

@enp0s3 These tests are a bit different, on the tests/infrastructure/prometheus.go they are testing 2 things:

Prometheus endpoints are working and throttling requests
Each virt-* component, is correctly exposing metrics through their internal endpoints

This new test added validates that:

All metrics registered in the virt-* components are present in Prometheus

They seem to intent to test different things

enp0s3

@machadovilaca Hi. TBH I think we can cover these scenarios with unit tests. Please correct me if I am wrong.

tests/monitoring/metrics.go

enp0s3 · 2024-02-28T10:50:27Z

tests/monitoring/metrics.go

+			}
+		})
+
+		Context("perfscale histogram metrics", func() {


Why can't we cover it with unit test instead?

we would only test that they are registered in the component, not that they are correctly set up, and being exposed correctly
and since we already fetched the list of KubeVirt metrics, its just a simple validation

machadovilaca · 2024-02-28T13:43:59Z

/test pull-kubevirt-e2e-k8s-1.29-sig-monitoring

machadovilaca · 2024-02-29T13:40:31Z

/retest

tests/monitoring/metrics.go

enp0s3 · 2024-03-04T16:14:08Z

tests/monitoring/metrics.go

+	_, err := virtClient.VirtualMachine(vm.Namespace).Create(context.Background(), vm)
+	Expect(err).ToNot(HaveOccurred())
+
+	Eventually(func() error {


Why do you need a VM in this test? why not VMI?

to ensure for example kubevirt_vm_created_total exists

tests/monitoring/metrics.go

tests/libmonitoring/metric_matcher.go

tests/monitoring/metrics.go

machadovilaca · 2024-03-05T14:51:46Z

/test pull-kubevirt-e2e-k8s-1.29-sig-monitoring

enp0s3

@machadovilaca Maybe I am missing something here but the only added value of testing these metrics in e2e is because we bring up a real Prometheus endpoint and query it with web client. But why do we need to test the registration and the query? these are generic Prometheus functionalities IMO.

I think we can test the metrics in unit tests, by creating a fake VM objs, load them into informers and call the relevant functions to observe a particular metric value.
It will be even better since monitoring e2e is only running upon monitoring related files change, it won't run when someone changed VM lifecycle code that can affect metrics.

tests/monitoring/metrics.go

machadovilaca · 2024-03-12T11:38:02Z

@machadovilaca Maybe I am missing something here but the only added value of testing these metrics in e2e is because we bring up a real Prometheus endpoint and query it with web client. But why do we need to test the registration and the query? these are generic Prometheus functionalities IMO.

I think we can test the metrics in unit tests, by creating a fake VM objs, load them into informers and call the relevant functions to observe a particular metric value. It will be even better since monitoring e2e is only running upon monitoring related files change, it won't run when someone changed VM lifecycle code that can affect metrics.

this tests that the metrics are being properly registered by the components
yes, prometheus client tests that if they are registered they will be served
but this ensures that the kubevirt components are actually registering those metrics
and in the case of collectors, that those collectors are being used and pushing all metrics
because of the way collectors work, by pushing a "custom metric", those metrics are not initialized by default and may happen that some of them are not used

machadovilaca · 2024-03-18T19:45:00Z

/test pull-kubevirt-e2e-k8s-1.29-sig-monitoring

avlitman · 2024-03-24T08:54:19Z

tests/libmonitoring/metric_matcher.go

+	return msg
+}
+
+func (matcher *MetricMatcher) NegatedFailureMessage(actual interface{}) (message string) {


why we need both functions? FailureMessage and NegatedFailureMessage?
seems like they do the same thing except the msg

this is a implementation of a gomega interface, so it's mandatory to have both, to handle 2 scenarios

FailureMessage is used when you expect to find something and it is not present, for example, expected [1,2] to contain 3

NegatedFailureMessage is used when you expect to not find something, but it is there, for example, expected [A,B] not to contain A

avlitman · 2024-03-24T08:55:18Z

tests/monitoring/metrics.go

+	"strconv"
+	"time"
+
+	. "github.com/onsi/ginkgo/v2"


please sort the import to groups, I see onsi here and in the second group as well

I think here it makes sense to keep them apart, these 2 are dot imports, and we have this structure on all tests, and are always splitted from the rest

even github.com/onsi/gomega/types being from the same origin I would keep it as is, since is a qualified identifier and is just used for the return type of the matcher

fossedihelm

Thank you @machadovilaca!
Some comments below!

fossedihelm · 2024-03-26T10:11:17Z

tests/libmonitoring/prometheus.go

@@ -79,7 +79,7 @@ func WaitForMetricValueWithLabels(client kubecli.KubevirtClient, metric string,
 			return -1
 		}
 		return i
-	}, 3*time.Minute, 1*time.Second).Should(BeNumerically("==", expectedValue))
+	}, 5*time.Minute, 1*time.Second).Should(BeNumerically("==", expectedValue))


Can you explain why this increase? Have we seen some flakiness?

not sure, reverted

fossedihelm · 2024-03-26T10:14:51Z

tests/libmonitoring/metric_matcher.go

+	if matcher.Labels == nil {
+		return true, nil
+	}


you can drop this, since iterate over nil map is a non-op

removed 👌

fossedihelm · 2024-03-26T10:16:20Z

tests/libmonitoring/metric_matcher.go

+
+	nameToMatch := matcher.Metric.GetOpts().Name
+	if matcher.Metric.GetType() == operatormetrics.HistogramType || matcher.Metric.GetType() == operatormetrics.HistogramVecType {
+		nameToMatch = nameToMatch + prometheusHistogramBucketPrefix


is prometheusHistogramBucketPrefix a prefix or a suffix?

suffix yes, updated the name

fossedihelm · 2024-03-26T10:17:08Z

tests/monitoring/metrics.go

@@ -0,0 +1,139 @@
+package monitoring


missing header

fossedihelm · 2024-03-26T10:17:16Z

tests/libmonitoring/metric_matcher.go

@@ -0,0 +1,75 @@
+package libmonitoring


missing header

fossedihelm · 2024-03-26T10:18:50Z

tests/monitoring/metrics.go

+)
+
+var _ = Describe("[sig-monitoring]Metrics", decorators.SigMonitoring, func() {
+	var err error


please avoid using global err variable. It can generate flakiness. Furthermore, it is never cleaned after each test execution, so the risk is real.

got it, removed

fossedihelm · 2024-03-26T10:24:22Z

tests/monitoring/metrics.go

+}
+
+func createRunningVM(virtClient kubecli.KubevirtClient) *v1.VirtualMachine {
+	vmi := libvmifact.NewGuestless(libvmi.WithNamespace(testsuite.GetTestNamespace(nil)))


we don't need to specify libvmi.WithNamespace since the the Create already has its own

[FAILED] Unexpected error: <*errors.errorString | 0xc002857c80>: an empty namespace may not be set when a resource name is provided { s: "an empty namespace may not be set when a resource name is provided", } occurred In [BeforeEach] at: tests/libwait/wait.go:76 @ 03/26/24 11:04:46.857

Oh yes, the libvmi.wait needs it.
Thank you!

fossedihelm · 2024-03-26T10:25:48Z

tests/monitoring/metrics.go

+	Eventually(func() bool {
+		vm, err := virtClient.VirtualMachine(testsuite.GetTestNamespace(vm)).Get(context.Background(), vm.Name, &metav1.GetOptions{})
+		Expect(err).ToNot(HaveOccurred())
+		return vm.Status.Ready
+	}, 300*time.Second, 1*time.Second).Should(BeTrue())


Isn't the libwait.WaitForSuccessfulVMIStart(vmi) enough?

and perform a vm.Get after it?

The VMI still takes a few seconds to be created after you create the VM. And WaitForSuccessfulVMIStart just waits for the status, but if it does not find the VMI it exits with an error

fossedihelm

Hey @machadovilaca Can you please squash the commits into a single one?
Also, the headers are not compliant with the others.
Thank you!

fossedihelm · 2024-03-26T12:44:17Z

tests/libmonitoring/metric_matcher.go

+/*
+Copyright The KubeVirt Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+


Suggested change

/*

Copyright The KubeVirt Authors.

Licensed under the Apache License, Version 2.0 (the "License");

you may not use this file except in compliance with the License.

You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software

distributed under the License is distributed on an "AS IS" BASIS,

WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.

See the License for the specific language governing permissions and

limitations under the License.

*/

/*

* This file is part of the KubeVirt project

*

* Licensed under the Apache License, Version 2.0 (the "License");

* you may not use this file except in compliance with the License.

* You may obtain a copy of the License at

*

* http://www.apache.org/licenses/LICENSE-2.0

*

* Unless required by applicable law or agreed to in writing, software

* distributed under the License is distributed on an "AS IS" BASIS,

* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.

* See the License for the specific language governing permissions and

* limitations under the License.

*

* Copyright The Kubevirt Authors

*

*/

I use the template from hack/boilerplate/boilerplate.go.txt
not supposed to use that?

Weird, in all the code we are using a different format for the non-generated code.
I would use my suggestion while we will decide something.
Thank you

sounds fine by me, updated!

fossedihelm · 2024-03-26T12:48:26Z

@enp0s3 any chance to get another review here? Thanks

fossedihelm · 2024-03-26T14:15:32Z

tests/monitoring/metrics.go

+		}
+
+		It("should contain virt components metrics", func() {
+			metrics = fetchPrometheusMetrics(virtClient)


sorry for splitted review! Do we need this? this is already performed in BeforeEach block

no worries
nice catch, I think its a leftover of before I added the 2nd test

Signed-off-by: João Vilaça <jvilaca@redhat.com>

fossedihelm · 2024-03-26T14:44:34Z

Nothing else from my side. Thank you for your patience!
/lgtm

enp0s3

/approve

kubevirt-bot · 2024-04-03T10:42:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enp0s3

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:

~~OWNERS~~ [enp0s3]

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

kubevirt-commenter-bot · 2024-04-03T10:42:12Z

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-windows2016
/test pull-kubevirt-e2e-kind-1.27-vgpu
/test pull-kubevirt-e2e-kind-sriov
/test pull-kubevirt-e2e-k8s-1.29-ipv6-sig-network
/test pull-kubevirt-e2e-k8s-1.27-sig-network
/test pull-kubevirt-e2e-k8s-1.27-sig-storage
/test pull-kubevirt-e2e-k8s-1.27-sig-compute
/test pull-kubevirt-e2e-k8s-1.27-sig-operator
/test pull-kubevirt-e2e-k8s-1.28-sig-network
/test pull-kubevirt-e2e-k8s-1.28-sig-storage
/test pull-kubevirt-e2e-k8s-1.28-sig-compute
/test pull-kubevirt-e2e-k8s-1.28-sig-operator

kubevirt-commenter-bot · 2024-04-03T16:52:37Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kubevirt-commenter-bot · 2024-04-03T19:52:37Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kubevirt-bot · 2024-04-04T02:45:38Z

@machadovilaca: 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
pull-kubevirt-e2e-arm64	`719c9f6`	link	false	`/test pull-kubevirt-e2e-arm64`
pull-kubevirt-conformance-arm64	`719c9f6`	link	false	`/test pull-kubevirt-conformance-arm64`

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.

kubevirt-commenter-bot · 2024-04-04T05:52:39Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/L labels Feb 20, 2024

kubevirt-bot requested review from kbidarkar and xpivarc February 20, 2024 13:54

kubevirt-bot requested review from enp0s3 and sradco February 20, 2024 13:54

machadovilaca force-pushed the add-metrics-e2e-test branch from 72bf912 to ee361b6 Compare February 21, 2024 18:17

kubevirt-bot added the area/monitoring label Feb 21, 2024

machadovilaca force-pushed the add-metrics-e2e-test branch from ee361b6 to 901107d Compare February 21, 2024 18:18

kubevirt-bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 21, 2024

machadovilaca force-pushed the add-metrics-e2e-test branch from 901107d to deacbbe Compare February 26, 2024 10:51

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 26, 2024

machadovilaca force-pushed the add-metrics-e2e-test branch from deacbbe to bb82a28 Compare February 26, 2024 10:55

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 26, 2024

enp0s3 reviewed Feb 27, 2024

View reviewed changes

enp0s3 reviewed Feb 28, 2024

View reviewed changes

machadovilaca force-pushed the add-metrics-e2e-test branch from bb82a28 to d162a45 Compare February 28, 2024 13:01

enp0s3 reviewed Mar 4, 2024

View reviewed changes

tests/monitoring/metrics.go Show resolved Hide resolved

enp0s3 reviewed Mar 12, 2024

View reviewed changes

tests/monitoring/metrics.go Show resolved Hide resolved

kubevirt-bot added the sig/buildsystem Denotes an issue or PR that relates to changes in the build system. label Mar 18, 2024

machadovilaca force-pushed the add-metrics-e2e-test branch from fa3efe5 to 0641fac Compare March 18, 2024 19:44

avlitman reviewed Mar 24, 2024

View reviewed changes

fossedihelm reviewed Mar 26, 2024

View reviewed changes

machadovilaca force-pushed the add-metrics-e2e-test branch from 0641fac to 6ba8a83 Compare March 26, 2024 11:37

fossedihelm reviewed Mar 26, 2024

View reviewed changes

machadovilaca force-pushed the add-metrics-e2e-test branch from 6ba8a83 to c79c5a6 Compare March 26, 2024 14:10

fossedihelm reviewed Mar 26, 2024

View reviewed changes

Add e2e tests for metrics

719c9f6

Signed-off-by: João Vilaça <jvilaca@redhat.com>

machadovilaca force-pushed the add-metrics-e2e-test branch from c79c5a6 to 719c9f6 Compare March 26, 2024 14:39

kubevirt-bot assigned fossedihelm Mar 26, 2024

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 26, 2024

enp0s3 approved these changes Apr 3, 2024

View reviewed changes

kubevirt-bot assigned enp0s3 Apr 3, 2024

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 3, 2024

kubevirt-bot merged commit f4aad6e into kubevirt:main Apr 4, 2024
37 of 39 checks passed

machadovilaca mentioned this pull request Apr 8, 2024

Refactor common component metrics #11184

Merged

8 tasks

-/*
-Copyright The KubeVirt Authors.
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-    http://www.apache.org/licenses/LICENSE-2.0
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-See the License for the specific language governing permissions and
-limitations under the License.
-*/
+/*
+ * This file is part of the KubeVirt project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ * Copyright The Kubevirt Authors
+ *
+ */

Add e2e tests for metrics #11307

Add e2e tests for metrics #11307

Conversation

machadovilaca commented Feb 20, 2024

What this PR does

Why we need it and why it was done in this way

Special notes for your reviewer

Checklist

Release note

machadovilaca commented Feb 20, 2024

machadovilaca commented Feb 20, 2024

machadovilaca commented Feb 21, 2024

machadovilaca commented Feb 26, 2024

machadovilaca commented Feb 26, 2024

machadovilaca commented Feb 26, 2024

enp0s3 left a comment

Choose a reason for hiding this comment

machadovilaca commented Feb 27, 2024

enp0s3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

machadovilaca commented Feb 28, 2024

machadovilaca commented Feb 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

machadovilaca commented Mar 5, 2024

enp0s3 left a comment

Choose a reason for hiding this comment

machadovilaca commented Mar 12, 2024

machadovilaca commented Mar 18, 2024

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

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

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

machadovilaca Mar 26, 2024 • edited

Choose a reason for hiding this comment

fossedihelm left a comment • edited

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

fossedihelm commented Mar 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fossedihelm commented Mar 26, 2024

enp0s3 left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Apr 3, 2024

kubevirt-commenter-bot commented Apr 3, 2024

kubevirt-commenter-bot commented Apr 3, 2024

kubevirt-commenter-bot commented Apr 3, 2024

kubevirt-bot commented Apr 4, 2024 • edited

kubevirt-commenter-bot commented Apr 4, 2024

machadovilaca Mar 26, 2024 •

edited

fossedihelm left a comment •

edited

kubevirt-bot commented Apr 4, 2024 •

edited