Repository: metallb/metallb
Pull request #1035 (2021-11-10 09:24:27)

E2E test: make the check for sharing ip l2 test in eventually
It may take some time to have the event we check available, here instead of checking once, we check inside an eventually loop to avoid flakes.




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) {
diff --git a/e2etest/layer2_test.go b/e2etest/layer2_test.go
index b16283ac..62d195af 100644
--- a/e2etest/layer2_test.go
+++ b/e2etest/layer2_test.go
@@ -20,6 +20,7 @@ package e2e
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net"
 	"strconv"
@@ -28,6 +29,7 @@ import (
 	"github.com/mikioh/ipaddr"
 	"github.com/onsi/ginkgo"
 	"github.com/onsi/ginkgo/extensions/table"
+	"github.com/onsi/gomega"
 	"go.universe.tf/metallb/e2etest/pkg/executor"
 	"go.universe.tf/metallb/e2etest/pkg/mac"
 	internalconfig "go.universe.tf/metallb/internal/config"
@@ -257,21 +259,33 @@ var _ = ginkgo.Describe("L2", func() {
 			})
 		framework.ExpectNoError(err)
 
-		events, err := cs.CoreV1().Events(namespace).List(context.Background(), metav1.ListOptions{FieldSelector: "reason=nodeAssigned"})
-		framework.ExpectNoError(err)
+		gomega.Eventually(func() error {
+			events, err := cs.CoreV1().Events(namespace).List(context.Background(), metav1.ListOptions{FieldSelector: "reason=nodeAssigned"})
+			if err != nil {
+				return err
+			}
 
-		var service1Announce, service2Announce string
-		for _, e := range events.Items {
-			if e.InvolvedObject.Name == svc1.Name {
-				service1Announce = e.Message
+			var service1Announce, service2Announce string
+			for _, e := range events.Items {
+				if e.InvolvedObject.Name == svc1.Name {
+					service1Announce = e.Message
+				}
+				if e.InvolvedObject.Name == svc2.Name {
+					service2Announce = e.Message
+				}
 			}
-			if e.InvolvedObject.Name == svc2.Name {
-				service2Announce = e.Message
+			if service1Announce == "" {
+				return errors.New("service1 not announced")
 			}
-		}
-		framework.ExpectNotEqual(service1Announce, "")
-		framework.ExpectNotEqual(service2Announce, "")
-		framework.ExpectEqual(service1Announce, service2Announce, "Services with the same IP announced from different nodes")
+			if service2Announce == "" {
+				return errors.New("service2 not announced")
+			}
+			if service1Announce != service2Announce {
+				return fmt.Errorf("Service announced from different nodes %s %s", service1Announce, service2Announce)
+			}
+			return nil
+		}, 2*time.Minute, 1*time.Second).Should(gomega.BeNil())
+
 	},
 		table.Entry("IPV4", &ipv4ServiceRange),
 		table.Entry("IPV6", &ipv6ServiceRange))