Rework how gratuitous advertisements (ARP/NDP) work

With our current approach, we only do gratuitous advertisements on the first
SetBalancer() call, and we don't resend any gratuitous advertisements on the next
calls to reduce the amount of "spam" in the network.
This was working pretty well when we were using K8S API to do all the decisions.
Now that we are also using MemberList status, our decisions are based on eventually consistent
information, and there are at least 2 cases where we need to resend gratuitous advertisements
even if the information that we have makes us think there were no changes in ownership:

1) Split brain with no ownership changes:
3 nodes A B C, A owns the LoadBalancer IP I, cluster is clean.
Now for some reason C can't talk to A and B anymore, and our algorithm
in ShouldAnnounce() continues to pick A as the owner of I.
As there were no changes for A, A doesn't send any gratuitous advertisement.
As C thinks it is alone, it thinks it owns I and sends gratuitous advertisements.
Some seconds later C rejoins A & B, C stops sending gratuitous advertisements,
but A continues to be the owner and doesn't send any gratuitous advertisement.
Depending on the switches' inner working, traffic might continue to go to C for a long time.

2) Race condition on ForceSync:
3 nodes A B C, A owns the LoadBalancer IP I, cluster is clean.
A becomes really slow (cpu limits or ...) and memberlist on B and C decides that A is not part of the memberlist cluster
anymore. B and C each start a ForceSync(), one of B or C becomes the owner of I and starts gratuitous advertisements for I.
A starts to respond again to memberlist and rejoins the cluster, while doing its first ForceSync().
A thinks it was always the owner of I and doesn't send any gratuitous advertisement.

The idea of this patch is to send gratuitous advertissements for 5 seconds from the last SetBalancer() call,
instead of the last time we think we became the owner.
To ensure there is only 1 sender for each IP, we use only one goroutine for all gratuitous advertisement calls.
As gratuitous() was using Lock() (ie exclusive lock), we were sending at most 1 gratuitous advertisement at a time,
so we know that this is fine performance wise, but it might be burstier than before.

This fixes #584

Signed-off-by: Etienne Champetier <echampetier@anevia.com>

internal/layer2/announcer.go

@@ -20,6 +20,10 @@ type Announce struct {
 	ndps     map[int]*ndpResponder
 	ips      map[string]net.IP // svcName -> IP
 	ipRefcnt map[string]int    // ip.String() -> number of uses
+
+	// channel can block so do not produce to spamCh while holding the Mutex
+	// else you risk causing a dead lock
+	spamCh chan net.IP
 }
 
 // New returns an initialized Announce.
@@ -30,8 +34,10 @@ func New(l log.Logger) (*Announce, error) {
 		ndps:     map[int]*ndpResponder{},
 		ips:      map[string]net.IP{},
 		ipRefcnt: map[string]int{},
+		spamCh:   make(chan net.IP, 1024),
 	}
 	go ret.interfaceScan()
+	go ret.spamLoop()
 
 	return ret, nil
 }
@@ -131,24 +137,49 @@ func (a *Announce) updateInterfaces() {
 	return
 }
 
-func (a *Announce) spam(name string) {
-	// TODO: should abort if we lose control of the IP mid-spam.
-	start := time.Now()
-	for time.Since(start) < 5*time.Second {
-		if err := a.gratuitous(name); err != nil {
-			a.logger.Log("op", "gratuitousAnnounce", "error", err, "service", name, "msg", "failed to make gratuitous IP announcement")
+func (a *Announce) spamLoop() {
+	m := map[string]time.Time{}
+	// see https://github.com/metallb/metallb/issues/172 for the 1100 choice
+	ticker := time.NewTicker(1100 * time.Millisecond)
+	for {
+		select {
+		case ip := <-a.spamCh:
+			ips := ip.String()
+			_, ok := m[ips]
+			m[ips] = time.Now().Add(5 * time.Second)
+			if !ok {
+				// The first 2 spam can be close
+				// but we don't want to wait 1.1 sec
+				a.spam(ip)
+			}
+		case now := <-ticker.C:
+			for ips, until := range m {
+				if now.After(until) {
+					delete(m, ips)
+				} else {
+					a.spam(net.ParseIP(ips))
+				}
+			}
 		}
-		time.Sleep(1100 * time.Millisecond)
 	}
 }
 
-func (a *Announce) gratuitous(name string) error {
-	a.Lock()
-	defer a.Unlock()
+func (a *Announce) spamStart(ip net.IP) {
+	a.spamCh <- ip
+}
 
-	ip, ok := a.ips[name]
-	if !ok {
-		// No IP means we've lost control of the IP, someone else is
+func (a *Announce) spam(ip net.IP) {
+	if err := a.gratuitous(ip); err != nil {
+		a.logger.Log("op", "gratuitousAnnounce", "error", err, "ip", ip, "msg", "failed to make gratuitous IP announcement")
+	}
+}
+
+func (a *Announce) gratuitous(ip net.IP) error {
+	a.RLock()
+	defer a.RUnlock()
+
+	if a.ipRefcnt[ip.String()] <= 0 {
+		// We've lost control of the IP, someone else is
 		// doing announcements.
 		return nil
 	}
@@ -181,6 +212,8 @@ func (a *Announce) shouldAnnounce(ip net.IP) dropReason {
 
 // SetBalancer adds ip to the set of announced addresses.
 func (a *Announce) SetBalancer(name string, ip net.IP) {
+	// Call spamStart at the end of the function without holding the lock
+	defer a.spamStart(ip)
 	a.Lock()
 	defer a.Unlock()
 
@@ -203,9 +236,6 @@ func (a *Announce) SetBalancer(name string, ip net.IP) {
 			a.logger.Log("op", "watchMulticastGroup", "error", err, "ip", ip, "msg", "failed to watch NDP multicast group for IP, NDP responder will not respond to requests for this address")
 		}
 	}
-
-	go a.spam(name)
-
 }
 
 // DeleteBalancer deletes an address from the set of addresses we should announce.

internal/layer2/announcer_test.go

@@ -9,6 +9,7 @@ func Test_SetBalancer_AddsToAnnouncedServices(t *testing.T) {
 	announce := &Announce{
 		ips:      map[string]net.IP{},
 		ipRefcnt: map[string]int{},
+		spamCh:   make(chan net.IP, 1),
 	}
 
 	services := []struct {
@@ -27,6 +28,8 @@ func Test_SetBalancer_AddsToAnnouncedServices(t *testing.T) {
 
 	for _, service := range services {
 		announce.SetBalancer(service.name, service.ip)
+		//we need to empty spamCh
+		<-announce.spamCh
 
 		if !announce.AnnounceName(service.name) {
 			t.Fatalf("service %v is not anounced", service.name)