Repository: metallb/metallb
Pull request #1033 (2021-11-09 13:43:39)

E2E improvements
This is a subset of the commits in https://github.com/metallb/metallb/pull/1012 to make review easier and to try to de-flake the CI:

- make the test timeout longer
- have the gauge check inside an eventually loop

The gauge is the most common flake

msherif1234: any change while u are here or maybe in another PR enhance the CI run logs , I got pages of logs and its very hard sometimes to fish for the error , plus most what I saw didn't give much info to help with debugging failure ?
fedepaol: > any change while u are here or maybe in another PR enhance the CI run logs , I got pages of logs and its very hard sometimes to fish for the error , plus most what I saw didn't give much info to help with debugging failure ?

I have some of them in https://github.com/metallb/metallb/pull/1012 where we would dump the content of the frr containers after a failure. I am splitting that in smaller PRs so it's easier to review.

Patch:
diff --git a/controller/main.go b/controller/main.go
index 6f53d988..95759c7f 100644
--- a/controller/main.go
+++ b/controller/main.go
@@ -104,12 +104,12 @@ func (c *controller) SetConfig(l log.Logger, cfg *config.Config) k8s.SyncState {
 
 	if cfg == nil {
 		level.Error(l).Log("op", "setConfig", "error", "no MetalLB configuration in cluster", "msg", "configuration is missing, MetalLB will not function")
-		return k8s.SyncStateError
+		return k8s.SyncStateErrorNoRetry
 	}
 
 	if err := c.ips.SetPools(cfg.Pools); err != nil {
 		level.Error(l).Log("op", "setConfig", "error", err, "msg", "applying new configuration failed")
-		return k8s.SyncStateError
+		return k8s.SyncStateErrorNoRetry
 	}
 	c.config = cfg
 	return k8s.SyncStateReprocessAll
diff --git a/internal/k8s/k8s.go b/internal/k8s/k8s.go
index 7f0f8aed..1b3b1fa4 100644
--- a/internal/k8s/k8s.go
+++ b/internal/k8s/k8s.go
@@ -69,6 +69,9 @@ const (
 	// The update was accepted, but requires reprocessing all watched
 	// services.
 	SyncStateReprocessAll
+	// The update caused a non transient error, the k8s client should
+	// just report and giveup.
+	SyncStateErrorNoRetry
 )
 
 // Config specifies the configuration of the Kubernetes
@@ -425,6 +428,7 @@ func (c *Client) Run(stopCh <-chan struct{}) error {
 		st := c.sync(key)
 		switch st {
 		case SyncStateSuccess:
+		case SyncStateErrorNoRetry:
 			c.queue.Forget(key)
 		case SyncStateError:
 			updateErrors.Inc()
@@ -538,10 +542,10 @@ func (c *Client) sync(key interface{}) SyncState {
 		}
 
 		st := c.configChanged(l, cfg)
-		if st == SyncStateError {
+		if st == SyncStateErrorNoRetry || st == SyncStateError {
 			level.Error(l).Log("event", "configStale", "error", err, "msg", "config (re)load failed, config marked stale")
 			configStale.Set(1)
-			return SyncStateSuccess
+			return st
 		}
 
 		configLoaded.Set(1)
diff --git a/speaker/main.go b/speaker/main.go
index 4247432d..84fe5d13 100644
--- a/speaker/main.go
+++ b/speaker/main.go
@@ -349,7 +349,7 @@ func (c *controller) SetConfig(l log.Logger, cfg *config.Config) k8s.SyncState {
 
 	if cfg == nil {
 		level.Error(l).Log("op", "setConfig", "error", "no MetalLB configuration in cluster", "msg", "configuration is missing, MetalLB will not function")
-		return k8s.SyncStateError
+		return k8s.SyncStateErrorNoRetry
 	}
 
 	for svc, ip := range c.svcIP {
@@ -362,7 +362,7 @@ func (c *controller) SetConfig(l log.Logger, cfg *config.Config) k8s.SyncState {
 	for proto, handler := range c.protocols {
 		if err := handler.SetConfig(l, cfg); err != nil {
 			level.Error(l).Log("op", "setConfig", "protocol", proto, "error", err, "msg", "applying new configuration to protocol handler failed")
-			return k8s.SyncStateError
+			return k8s.SyncStateErrorNoRetry
 		}
 	}
 
diff --git a/tasks.py b/tasks.py
index 71da7ccf..02c35355 100644
--- a/tasks.py
+++ b/tasks.py
@@ -672,7 +672,7 @@ def e2etest(ctx, name="kind", export=None, kubeconfig=None, system_namespaces="k
         ipv6_service_range = get_service_range(6)
 
     testrun = run("cd `git rev-parse --show-toplevel`/e2etest &&"
-            "go test {} {} --provider=local --kubeconfig={} --service-pod-port={} -ipv4-service-range={} -ipv6-service-range={} {}".format(ginkgo_focus, ginkgo_skip, kubeconfig, service_pod_port, ipv4_service_range, ipv6_service_range, opt_skip_docker), warn="True")
+            "go test -timeout 30m {} {} --provider=local --kubeconfig={} --service-pod-port={} -ipv4-service-range={} -ipv6-service-range={} {}".format(ginkgo_focus, ginkgo_skip, kubeconfig, service_pod_port, ipv4_service_range, ipv6_service_range, opt_skip_docker), warn="True")
 
     if export != None:
         run("kind export logs {}".format(export))


Test patch:
diff --git a/controller/controller_test.go b/controller/controller_test.go
index b242ea30..2ff51d40 100644
--- a/controller/controller_test.go
+++ b/controller/controller_test.go
@@ -621,12 +621,12 @@ func TestControllerConfig(t *testing.T) {
 	}
 
 	// Now that an IP is allocated, removing the IP pool is not allowed.
-	if c.SetConfig(l, &config.Config{}) != k8s.SyncStateError {
+	if c.SetConfig(l, &config.Config{}) != k8s.SyncStateErrorNoRetry {
 		t.Fatalf("SetConfig that deletes allocated IPs was accepted")
 	}
 
 	// Deleting the config also makes MetalLB sad.
-	if c.SetConfig(l, nil) != k8s.SyncStateError {
+	if c.SetConfig(l, nil) != k8s.SyncStateErrorNoRetry {
 		t.Fatalf("SetConfig that deletes the config was accepted")
 	}
 }
diff --git a/e2etest/bgp_test.go b/e2etest/bgp_test.go
index 6bdf1c22..5c86206d 100644
--- a/e2etest/bgp_test.go
+++ b/e2etest/bgp_test.go
@@ -267,20 +267,42 @@ var _ = ginkgo.Describe("BGP", func() {
 			}
 
 			ginkgo.By("checking the metrics when no service is added")
-			controllerMetrics, err := metrics.ForPod(controllerPod, controllerPod)
-			framework.ExpectNoError(err)
-			validateGaugeValue(0, "metallb_allocator_addresses_in_use_total", map[string]string{"pool": poolName}, controllerMetrics)
-			validateGaugeValue(272, "metallb_allocator_addresses_total", map[string]string{"pool": poolName}, controllerMetrics)
+			Eventually(func() error {
+				controllerMetrics, err := metrics.ForPod(controllerPod, controllerPod)
+				if err != nil {
+					return err
+				}
+				err = validateGaugeValue(0, "metallb_allocator_addresses_in_use_total", map[string]string{"pool": poolName}, controllerMetrics)
+				if err != nil {
+					return err
+				}
+				err = validateGaugeValue(272, "metallb_allocator_addresses_total", map[string]string{"pool": poolName}, controllerMetrics)
+				if err != nil {
+					return err
+				}
+				return nil
+			}, 2*time.Minute, 1*time.Second).Should(BeNil())
 
 			for _, speaker := range speakerPods {
 				ginkgo.By(fmt.Sprintf("checking speaker %s", speaker.Name))
 
-				speakerMetrics, err := metrics.ForPod(controllerPod, speaker)
-				framework.ExpectNoError(err)
-				for _, addr := range peerAddrs {
-					validateGaugeValue(1, "metallb_bgp_session_up", map[string]string{"peer": addr}, speakerMetrics)
-					validateGaugeValue(0, "metallb_bgp_announced_prefixes_total", map[string]string{"peer": addr}, speakerMetrics)
-				}
+				Eventually(func() error {
+					speakerMetrics, err := metrics.ForPod(controllerPod, speaker)
+					if err != nil {
+						return err
+					}
+					for _, addr := range peerAddrs {
+						err = validateGaugeValue(1, "metallb_bgp_session_up", map[string]string{"peer": addr}, speakerMetrics)
+						if err != nil {
+							return err
+						}
+						err = validateGaugeValue(0, "metallb_bgp_announced_prefixes_total", map[string]string{"peer": addr}, speakerMetrics)
+						if err != nil {
+							return err
+						}
+					}
+					return nil
+				}, 2*time.Minute, 1*time.Second).Should(BeNil())
 			}
 
 			ginkgo.By("creating a service")
@@ -291,23 +313,49 @@ var _ = ginkgo.Describe("BGP", func() {
 			}()
 
 			ginkgo.By("checking the metrics when a service is added")
-			controllerMetrics, err = metrics.ForPod(controllerPod, controllerPod)
-			framework.ExpectNoError(err)
-			validateGaugeValue(1, "metallb_allocator_addresses_in_use_total", map[string]string{"pool": poolName}, controllerMetrics)
+			Eventually(func() error {
+				controllerMetrics, err := metrics.ForPod(controllerPod, controllerPod)
+				if err != nil {
+					return err
+				}
+				err = validateGaugeValue(1, "metallb_allocator_addresses_in_use_total", map[string]string{"pool": poolName}, controllerMetrics)
+				if err != nil {
+					return err
+				}
+				return nil
+			}, 2*time.Minute, 1*time.Second).Should(BeNil())
 
 			for _, speaker := range speakerPods {
 				ginkgo.By(fmt.Sprintf("checking speaker %s", speaker.Name))
 
-				speakerMetrics, err := metrics.ForPod(controllerPod, speaker)
-				framework.ExpectNoError(err)
-				for _, addr := range peerAddrs {
-					validateGaugeValue(1, "metallb_bgp_session_up", map[string]string{"peer": addr}, speakerMetrics)
-					validateGaugeValue(1, "metallb_bgp_announced_prefixes_total", map[string]string{"peer": addr}, speakerMetrics)
-
-					validateCounterValue(1, "metallb_bgp_updates_total", map[string]string{"peer": addr}, speakerMetrics)
-				}
-
-				validateGaugeValue(1, "metallb_speaker_announced", map[string]string{"node": speaker.Spec.NodeName, "protocol": "bgp", "service": fmt.Sprintf("%s/%s", f.Namespace.Name, svc.Name)}, speakerMetrics)
+				Eventually(func() error {
+					speakerMetrics, err := metrics.ForPod(controllerPod, speaker)
+					if err != nil {
+						return err
+					}
+					for _, addr := range peerAddrs {
+						err = validateGaugeValue(1, "metallb_bgp_session_up", map[string]string{"peer": addr}, speakerMetrics)
+						if err != nil {
+							return err
+						}
+
+						err = validateGaugeValue(1, "metallb_bgp_announced_prefixes_total", map[string]string{"peer": addr}, speakerMetrics)
+						if err != nil {
+							return err
+						}
+
+						err = validateCounterValue(1, "metallb_bgp_updates_total", map[string]string{"peer": addr}, speakerMetrics)
+						if err != nil {
+							return err
+						}
+					}
+
+					err = validateGaugeValue(1, "metallb_speaker_announced", map[string]string{"node": speaker.Spec.NodeName, "protocol": "bgp", "service": fmt.Sprintf("%s/%s", f.Namespace.Name, svc.Name)}, speakerMetrics)
+					if err != nil {
+						return err
+					}
+					return nil
+				}, 2*time.Minute, 1*time.Second).Should(BeNil())
 			}
 		})
 	})
@@ -412,25 +460,27 @@ func createServiceWithBackend(cs clientset.Interface, namespace string, policy c
 	return svc, jig
 }
 
-func validateGaugeValue(expectedValue int, metricName string, labels map[string]string, allMetrics []map[string]*dto.MetricFamily) {
-	var err error
-	var value int
+func validateGaugeValue(expectedValue int, metricName string, labels map[string]string, allMetrics []map[string]*dto.MetricFamily) error {
 	found := false
 	for _, m := range allMetrics {
-		value, err = metrics.GaugeForLabels(metricName, labels, m)
+		value, err := metrics.GaugeForLabels(metricName, labels, m)
 		if err != nil {
 			continue
 		}
+		if value != expectedValue {
+			return fmt.Errorf("invalid value %d for %s, expecting %d", value, metricName, expectedValue)
+		}
 		found = true
-		framework.ExpectEqual(value, expectedValue, "invalid value for ", metricName, labels)
 	}
 
 	if !found {
-		framework.Fail(err.Error())
+		return fmt.Errorf("metric %s not found", metricName)
 	}
+	return nil
+
 }
 
-func validateCounterValue(expectedValue int, metricName string, labels map[string]string, allMetrics []map[string]*dto.MetricFamily) {
+func validateCounterValue(expectedMax int, metricName string, labels map[string]string, allMetrics []map[string]*dto.MetricFamily) error {
 	var err error
 	var value int
 	found := false
@@ -440,12 +490,15 @@ func validateCounterValue(expectedValue int, metricName string, labels map[strin
 			continue
 		}
 		found = true
-		framework.ExpectEqual(value >= expectedValue, true, "expecting ", value, "greater than", expectedValue)
+		if value < expectedMax {
+			return fmt.Errorf("invalid value %d for %s, expecting more than %d", value, metricName, expectedMax)
+		}
 	}
 
 	if !found {
-		framework.Fail(err.Error())
+		return fmt.Errorf("metric %s not found", metricName)
 	}
+	return nil
 }
 
 func pairExternalFRRWithNodes(cs clientset.Interface, c *frrcontainer.FRR) {