Merge pull request #1281 from akrejcir/improve-flaky-metrics-tests

tests: Improve stability of metric tests

Author: kubevirt-bot

tests/commonTemplates_test.go

@@ -189,7 +189,7 @@ var _ = Describe("Common templates", func() {
 			Entry("[test_id:5087]test template", &testTemplate),
 		)
 
-		It("[test_id: 7340] should increase metrics when restoring tamplate", func() {
+		It("[test_id: 7340] should increase metrics when restoring template", func() {
 			expectTemplateUpdateToIncreaseTotalRestoredTemplatesCount(testTemplate)
 		})
 
@@ -355,7 +355,12 @@ var _ = Describe("Common templates", func() {
 })
 
 func expectTemplateUpdateToIncreaseTotalRestoredTemplatesCount(testTemplate testResource) {
-	restoredCountBefore := totalRestoredTemplatesCount()
+	restoredCountBefore, err := totalRestoredTemplatesCount()
+	Expect(err).ToNot(HaveOccurred())
+
 	expectRestoreAfterUpdate(&testTemplate)
-	Expect(totalRestoredTemplatesCount() - restoredCountBefore).To(Equal(1))
+
+	restoredCountAfter, err := totalRestoredTemplatesCount()
+	Expect(err).ToNot(HaveOccurred())
+	Expect(restoredCountAfter - restoredCountBefore).To(Equal(1))
 }

tests/metrics_test.go

@@ -20,6 +20,7 @@ import (
 	common_templates "kubevirt.io/ssp-operator/internal/operands/common-templates"
 	"kubevirt.io/ssp-operator/internal/operands/metrics"
 	"kubevirt.io/ssp-operator/pkg/monitoring/rules"
+	"kubevirt.io/ssp-operator/tests/env"
 )
 
 func mergeMaps(maps ...map[string]string) map[string]string {
@@ -174,25 +175,35 @@ var _ = Describe("Metrics", func() {
 		It("[test_id:TODO]should increment kubevirt_ssp_common_templates_restored_total during normal reconcile", func() {
 			skipIfUpgradeLane()
 
-			restoredCount := totalRestoredTemplatesCount()
+			var restoredCount int
+			Eventually(func() error {
+				var err error
+				restoredCount, err = totalRestoredTemplatesCount()
+				return err
+			}, env.ShortTimeout(), time.Second).Should(Succeed())
 
 			template.Labels[common_templates.TemplateTypeLabel] = "test"
 			Expect(apiClient.Update(ctx, template)).To(Succeed())
 
-			Eventually(func() int {
+			Eventually(func() (int, error) {
 				return totalRestoredTemplatesCount()
 			}, 5*time.Minute, 10*time.Second).Should(Equal(restoredCount + 1))
 		})
 
 		It("[test_id:TODO]should not increment kubevirt_ssp_common_templates_restored_total during upgrades", func() {
-			restoredCount := totalRestoredTemplatesCount()
+			var restoredCount int
+			Eventually(func() error {
+				var err error
+				restoredCount, err = totalRestoredTemplatesCount()
+				return err
+			}, env.ShortTimeout(), time.Second).Should(Succeed())
 
 			template.Labels[common_templates.TemplateTypeLabel] = "test"
 			template.Labels[common_templates.TemplateVersionLabel] = "v" + rand.String(5)
 			Expect(apiClient.Update(ctx, template)).To(Succeed())
 
 			// TODO: replace 'Consistently' with a direct wait for the template update
-			Consistently(func() int {
+			Consistently(func() (int, error) {
 				return totalRestoredTemplatesCount()
 			}, 2*time.Minute, 20*time.Second).Should(Equal(restoredCount))
 		})

tests/metrics_test_utils.go

@@ -3,14 +3,14 @@ package tests
 import (
 	"context"
 	"crypto/tls"
+	"errors"
 	"fmt"
 	"io"
 	"net"
 	"net/http"
 	"regexp"
 	"strconv"
 
-	. "github.com/onsi/gomega"
 	v1 "k8s.io/api/core/v1"
 )
 
@@ -20,10 +20,13 @@ var regexpForMetrics = map[string]*regexp.Regexp{
 	"kubevirt_ssp_operator_reconcile_succeeded":      regexp.MustCompile(`kubevirt_ssp_operator_reconcile_succeeded ([0-9]+)`),
 }
 
-func intMetricValue(metricName string, metricsPort uint16, pod *v1.Pod) int {
+func intMetricValue(metricName string, metricsPort uint16, pod *v1.Pod) (value int, err error) {
 	conn, err := portForwarder.Connect(pod, metricsPort)
-	Expect(err).ToNot(HaveOccurred())
-	defer conn.Close()
+	if err != nil {
+		return 0, fmt.Errorf("failed to connect port-forwarding: %w", err)
+	}
+	defer func() { err = errors.Join(err, conn.Close()) }()
+
 	client := &http.Client{
 		Transport: &http.Transport{
 			DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) {
@@ -33,15 +36,26 @@ func intMetricValue(metricName string, metricsPort uint16, pod *v1.Pod) int {
 		},
 	}
 	resp, err := client.Get(fmt.Sprintf("https://localhost:%d/metrics", metricsPort))
-	Expect(err).ToNot(HaveOccurred(), "Can't get metrics from %s", pod.Name)
-	defer resp.Body.Close()
-	body, _ := io.ReadAll(resp.Body)
+	if err != nil {
+		return 0, fmt.Errorf("failed to get metrics from %s: %w", pod.Name, err)
+	}
+	defer func() { err = errors.Join(err, resp.Body.Close()) }()
+
+	body, err := io.ReadAll(resp.Body)
+	if err != nil {
+		return 0, fmt.Errorf("failed to read metrics response body: %w", err)
+	}
+
 	regex, ok := regexpForMetrics[metricName]
 	if !ok {
 		panic(fmt.Sprintf("metricName %s does not have a defined regexp string, please add one to the regexpForMetrics map", metricName))
 	}
+
 	valueOfMetric := regex.FindSubmatch(body)
 	intValue, err := strconv.Atoi(string(valueOfMetric[1]))
-	Expect(err).ToNot(HaveOccurred())
-	return intValue
+	if err != nil {
+		return 0, fmt.Errorf("failed to convert metric %s value to int: %w", metricName, err)
+	}
+
+	return intValue, nil
 }

tests/misc_test.go

@@ -167,9 +167,9 @@ func validateSspIsFailingToReconcileMetric() {
 		}
 	})
 	// the reconcile cycle should now be failing, so the kubevirt_ssp_operator_reconcile_succeeded metric should be 0
-	Eventually(func() int {
+	Eventually(func() (int, error) {
 		return sspOperatorReconcileSucceededCount()
-	}, env.ShortTimeout(), time.Second).Should(Equal(0))
+	}, env.ShortTimeout(), time.Second).Should(BeZero())
 }
 
 var _ = Describe("SCC annotation", func() {

tests/port-forwarding.go

@@ -40,7 +40,7 @@ func (p *portForwardConn) Close() error {
 func (p *portForwarderImpl) Connect(pod *core.Pod, remotePort uint16) (net.Conn, error) {
 	streamConnection, err := p.createStreamConnection(pod)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to create stream connection: %w", err)
 	}
 
 	requestId := atomic.AddInt32(&p.requestId, 1)
@@ -53,16 +53,18 @@ func (p *portForwarderImpl) Connect(pod *core.Pod, remotePort uint16) (net.Conn,
 	errorStream, err := streamConnection.CreateStream(headers)
 	if err != nil {
 		streamConnection.Close()
-		return nil, err
+		return nil, fmt.Errorf("failed to create error stream: %w", err)
 	}
 	// We will not write to error stream
-	errorStream.Close()
+	if err := errorStream.Close(); err != nil {
+		return nil, fmt.Errorf("failed to close error stream: %w", err)
+	}
 
 	headers.Set(core.StreamType, core.StreamTypeData)
 	dataStream, err := streamConnection.CreateStream(headers)
 	if err != nil {
 		streamConnection.Close()
-		return nil, err
+		return nil, fmt.Errorf("failed to create data stream: %w", err)
 	}
 
 	return &portForwardConn{
@@ -74,7 +76,7 @@ func (p *portForwarderImpl) Connect(pod *core.Pod, remotePort uint16) (net.Conn,
 func (p *portForwarderImpl) createStreamConnection(pod *core.Pod) (httpstream.Connection, error) {
 	transport, upgrader, err := spdy.RoundTripperFor(p.config)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to create RoundTripper: %w", err)
 	}
 
 	req := p.client.Post().
@@ -85,7 +87,11 @@ func (p *portForwarderImpl) createStreamConnection(pod *core.Pod) (httpstream.Co
 
 	dialer := spdy.NewDialer(upgrader, &http.Client{Transport: transport}, "POST", req.URL())
 	streamConn, _, err := dialer.Dial(portforward.PortForwardProtocolV1Name)
-	return streamConn, err
+	if err != nil {
+		return nil, fmt.Errorf("failed to dial: %w", err)
+	}
+
+	return streamConn, nil
 }
 
 func NewPortForwarder(config *rest.Config, client rest.Interface) PortForwarder {

tests/tests_common_test.go

@@ -114,20 +114,27 @@ func expectRecreateAfterDelete(res *testResource) {
 	Expect(err).ToNot(HaveOccurred())
 }
 
-func sspOperatorReconcileSucceededCount() (sum int) {
-	operatorPods, operatorMetricsPort := operatorPodsWithMetricsPort()
-	for _, sspOperator := range operatorPods {
-		sum += intMetricValue("kubevirt_ssp_operator_reconcile_succeeded", operatorMetricsPort, &sspOperator)
-	}
-	return
+func sspOperatorReconcileSucceededCount() (int, error) {
+	return collectMetricFromOperator("kubevirt_ssp_operator_reconcile_succeeded")
+}
+
+func totalRestoredTemplatesCount() (int, error) {
+	return collectMetricFromOperator("kubevirt_ssp_common_templates_restored_total")
 }
 
-func totalRestoredTemplatesCount() (sum int) {
+func collectMetricFromOperator(metricName string) (int, error) {
 	operatorPods, operatorMetricsPort := operatorPodsWithMetricsPort()
+
+	var sum int
 	for _, sspOperator := range operatorPods {
-		sum += intMetricValue("kubevirt_ssp_common_templates_restored_total", operatorMetricsPort, &sspOperator)
+		value, err := intMetricValue(metricName, operatorMetricsPort, &sspOperator)
+		if err != nil {
+			return 0, fmt.Errorf("failed to read int metric %s from pod %s: %w", metricName, sspOperator.Name, err)
+		}
+		sum += value
 	}
-	return
+
+	return sum, nil
 }
 
 // Note we are not assuming only one operator pod here although that is how it should be,

tests/validator_test.go

@@ -941,7 +941,13 @@ func eventuallyFailToCreateVm(vm *kubevirtv1.VirtualMachine) bool {
 }
 
 func failVmCreationToIncreaseRejectedVmsMetrics(template *templatev1.Template) {
-	rejectedCountBefore := totalRejectedVmsMetricsValue()
+	var rejectedCountBefore int
+	Eventually(func() error {
+		var err error
+		rejectedCountBefore, err = totalRejectedVmsMetricsValue()
+		return err
+	}, env.ShortTimeout(), time.Second).Should(Succeed())
+
 	vmi := NewRandomVMIWithBridgeInterface(strategy.GetNamespace())
 	// set values that will fail validation
 	vmi = addDomainResourcesToVMI(vmi, 2, "test", "128M")
@@ -951,22 +957,38 @@ func failVmCreationToIncreaseRejectedVmsMetrics(template *templatev1.Template) {
 		labels.AnnotationTemplateNamespaceKey: template.Namespace,
 	}
 	eventuallyFailToCreateVm(vm)
-	Expect(totalRejectedVmsMetricsValue() - rejectedCountBefore).To(Equal(1))
+
+	var rejectedCountAfter int
+	Eventually(func() error {
+		var err error
+		rejectedCountAfter, err = totalRejectedVmsMetricsValue()
+		return err
+	}, env.ShortTimeout(), time.Second).Should(Succeed())
+
+	Expect(rejectedCountAfter - rejectedCountBefore).To(Equal(1))
+
 	err := apiClient.Delete(ctx, vm)
 	if !errors.IsNotFound(err) {
 		Expect(err).ToNot(HaveOccurred(), "Failed to Delete VM")
 	}
 	waitForDeletion(client.ObjectKeyFromObject(vm), vm)
 }
 
-func totalRejectedVmsMetricsValue() (sum int) {
+func totalRejectedVmsMetricsValue() (int, error) {
 	pods, err := GetRunningPodsByLabel(validator.VirtTemplateValidator, validator.KubevirtIo, strategy.GetNamespace())
 	Expect(err).ToNot(HaveOccurred(), "Could not find the validator pods")
 	Expect(pods.Items).ToNot(BeEmpty())
+
+	var sum int
 	for _, validatorPod := range pods.Items {
-		sum += intMetricValue("kubevirt_ssp_template_validator_rejected_total", validator.MetricsPort, &validatorPod)
+		const metricName = "kubevirt_ssp_template_validator_rejected_total"
+		value, err := intMetricValue(metricName, validator.MetricsPort, &validatorPod)
+		if err != nil {
+			return 0, fmt.Errorf("failed to read int metric %s from pod %s: %w", metricName, validatorPod.Name, err)
+		}
+		sum += value
 	}
-	return
+	return sum, nil
 }
 
 func addObjectsToTemplates(genName, validation string) *templatev1.Template {