Improve race condition on parallel IP free. (#96)

pkg/controllers/loadbalancer/loadbalancer.go

@@ -206,37 +206,43 @@ func (l *LoadBalancerController) UpdateLoadBalancer(ctx context.Context, cluster
 func (l *LoadBalancerController) EnsureLoadBalancerDeleted(ctx context.Context, clusterName string, service *v1.Service) error {
 	klog.Infof("EnsureLoadBalancerDeleted: clusterName %q, namespace %q, serviceName %q, serviceStatus: %v", clusterName, service.Namespace, service.Name, service.Status)
 
-	s := *service
-	serviceTag := tags.BuildClusterServiceFQNTag(l.clusterID, s.GetNamespace(), s.GetName())
+	serviceTag := tags.BuildClusterServiceFQNTag(l.clusterID, service.GetNamespace(), service.GetName())
+
+	l.ipUpdateMutex.Lock()
+	defer l.ipUpdateMutex.Unlock()
+
 	ips, err := l.MetalService.FindProjectIPsWithTag(ctx, l.projectID, serviceTag)
 	if err != nil {
 		return err
 	}
 
-	l.ipUpdateMutex.Lock()
-	defer l.ipUpdateMutex.Unlock()
-
 	for _, ip := range ips {
 		ip := ip
 		err := retrygo.Do(
 			func() error {
-				newTags, last := l.removeServiceTag(*ip, serviceTag)
-				iu := &models.V1IPUpdateRequest{
-					Ipaddress: ip.Ipaddress,
-					Tags:      newTags,
-				}
-				newIP, err := l.MetalService.UpdateIP(ctx, iu)
-				if err != nil {
-					return fmt.Errorf("could not update ip with new tags: %w", err)
-				}
-				klog.Infof("updated ip: %q", pointer.SafeDeref(newIP.Ipaddress))
-				if *ip.Type == models.V1IPBaseTypeEphemeral && last {
-					klog.Infof("freeing unused ephemeral ip: %s, tags: %s", *ip.Ipaddress, newTags)
+				newTags, delete := l.removeServiceTag(*ip, serviceTag)
+
+				if *ip.Type == models.V1IPBaseTypeEphemeral && delete {
+					klog.Infof("freeing unused ephemeral ip: %s, tags: %s", *ip.Ipaddress, ip.Tags)
+
 					err := l.MetalService.FreeIP(ctx, *ip.Ipaddress)
 					if err != nil {
 						return fmt.Errorf("unable to delete ip %s: %w", *ip.Ipaddress, err)
 					}
+
+					return nil
+				}
+
+				klog.Infof("removing service reference tag %s from ip: %q, old tags: %s, new tags: %s", serviceTag, pointer.SafeDeref(ip.Ipaddress), ip.Tags, newTags)
+
+				_, err := l.MetalService.UpdateIP(ctx, &models.V1IPUpdateRequest{
+					Ipaddress: ip.Ipaddress,
+					Tags:      newTags,
+				})
+				if err != nil {
+					return fmt.Errorf("could not update ip with new tags: %w", err)
 				}
+
 				return nil
 			},
 		)

pkg/controllers/loadbalancer/loadbalancer_test.go

@@ -0,0 +1,97 @@
+package loadbalancer
+
+import (
+	"reflect"
+	"testing"
+
+	"github.com/metal-stack/metal-go/api/models"
+)
+
+func TestLoadBalancerController_removeServiceTag(t *testing.T) {
+	var (
+		testTag1 = "cluster.metal-stack.io/id/namespace/service=6ff712b7-3087-473e-b9d2-0461c2193bdf/istio-ingress/istio-ingressgateway"
+		testTag2 = "cluster.metal-stack.io/id/namespace/service=f9663b93-34bf-411e-a417-792452479d60/istio-ingress/istio-ingressgateway"
+		testTag3 = "cluster.metal-stack.io/id/namespace/service=43026eb9-075c-462f-b279-f4e9f2006e03/istio/istiod"
+	)
+
+	tests := []struct {
+		name       string
+		ip         models.V1IPResponse
+		serviceTag string
+		want       []string
+		wantLast   bool
+	}{
+		{
+			name: "only own service tag",
+			ip: models.V1IPResponse{
+				Tags: []string{testTag1},
+			},
+			serviceTag: testTag1,
+			want:       []string{},
+			wantLast:   true,
+		},
+		{
+			name: "own service tag and other service tag",
+			ip: models.V1IPResponse{
+				Tags: []string{testTag1, testTag2},
+			},
+			serviceTag: testTag1,
+			want:       []string{testTag2},
+			wantLast:   false,
+		},
+		{
+			name: "own service tag and multiple other service tags",
+			ip: models.V1IPResponse{
+				Tags: []string{testTag1, testTag2, testTag3},
+			},
+			serviceTag: testTag1,
+			want:       []string{testTag2, testTag3},
+			wantLast:   false,
+		},
+		// unusual / erroneous cases
+		{
+			// in this case we allow cleanup when it's an ephemeral ip
+			// this handles the case that
+			name: "no service tags",
+			ip: models.V1IPResponse{
+				Tags: nil,
+			},
+			serviceTag: testTag1,
+			want:       []string{},
+			wantLast:   true,
+		},
+		// TODO: this case is not covered:
+		// {
+		// 	name: "two times own service tag",
+		// 	ip: models.V1IPResponse{
+		// 		Tags: []string{testTag1, testTag1},
+		// 	},
+		// 	serviceTag: testTag1,
+		// 	want:       []string{},
+		// 	wantLast:   true,
+		// },
+		// TODO: this case is not covered
+		// {
+		// 	name: "only other service tag",
+		// 	ip: models.V1IPResponse{
+		// 		Tags: []string{testTag2},
+		// 	},
+		// 	serviceTag: testTag1,
+		// 	want:       []string{testTag2},
+		// 	wantLast:   false,
+		// },
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			l := &LoadBalancerController{}
+
+			got, gotLast := l.removeServiceTag(tt.ip, tt.serviceTag)
+			if !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("got = %v, want %v", got, tt.want)
+			}
+			if gotLast != tt.wantLast {
+				t.Errorf("got = %v, want %v", gotLast, tt.wantLast)
+			}
+		})
+	}
+}