Short circuit NB cleanup when service not annotated

Author: okokes-akamai

cloud/linode/loadbalancers.go

@@ -140,6 +140,16 @@ func (l *loadbalancers) getNodeBalancerByStatus(ctx context.Context, service *v1
 // LoadBalancer status; if they are different (because of an updated NodeBalancerID
 // annotation), the old one is deleted.
 func (l *loadbalancers) cleanupOldNodeBalancer(ctx context.Context, service *v1.Service) error {
+	// unless there's an annotation, we can never get a past and current NB to differ,
+	// because they're looked up the same way
+	// TODO(okokes): just copy pasted from elsewhere, encapsulate it somehow
+	rawID, _ := getServiceAnnotation(service, annLinodeNodeBalancerID)
+	id, idErr := strconv.Atoi(rawID)
+	hasIDAnn := idErr == nil && id != 0
+	if !hasIDAnn {
+		return nil
+	}
+
 	previousNB, err := l.getNodeBalancerByStatus(ctx, service)
 	switch err.(type) {
 	case nil:

cloud/linode/loadbalancers_test.go

@@ -166,6 +166,10 @@ func TestCCMLoadBalancers(t *testing.T) {
 			name: "makeLoadBalancerStatus",
 			f:    testMakeLoadBalancerStatus,
 		},
+		{
+			name: "Cleanup does not all the API unless Service annotated",
+			f:    testCleanupDoesntCall,
+		},
 	}
 
 	for _, tc := range testCases {
@@ -1444,6 +1448,50 @@ func testMakeLoadBalancerStatus(t *testing.T, client *linodego.Client, _ *fakeAP
 	}
 }
 
+func testCleanupDoesntCall(t *testing.T, client *linodego.Client, fakeAPI *fakeAPI) {
+	ipv4 := "192.168.0.1"
+	hostname := "nb-192-168-0-1.newark.nodebalancer.linode.com"
+	nb := &linodego.NodeBalancer{
+		IPv4:     &ipv4,
+		Hostname: &hostname,
+	}
+
+	svc := &v1.Service{ObjectMeta: metav1.ObjectMeta{Name: "test"}}
+	svcAnn := &v1.Service{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:        "test",
+			Annotations: map[string]string{annLinodeNodeBalancerID: "12345"},
+		},
+	}
+	svc.Status.LoadBalancer = *makeLoadBalancerStatus(svc, nb)
+	svcAnn.Status.LoadBalancer = *makeLoadBalancerStatus(svc, nb)
+	lb := &loadbalancers{client, "us-west", nil}
+
+	t.Run("non-annotated service shouldn't call the API during cleanup", func(t *testing.T) {
+		if err := lb.cleanupOldNodeBalancer(context.TODO(), svc); err != nil {
+			t.Fatal(err)
+		}
+		if len(fakeAPI.requests) != 0 {
+			t.Fatalf("unexpected API calls: %v", fakeAPI.requests)
+		}
+	})
+
+	// TODO(okokes): note that we're polluting fakeAPI between these subtests (not an issue per-se here, but not ideal)
+	t.Run("annotated service calls the API to load said NB", func(t *testing.T) {
+		if err := lb.cleanupOldNodeBalancer(context.TODO(), svcAnn); err != nil {
+			t.Fatal(err)
+		}
+		if len(fakeAPI.requests) != 1 {
+			t.Fatalf("unexpected API calls: %v", fakeAPI.requests)
+		}
+		// TODO(okokes): we need to change this to `/v4/nodebalancer` once we upgrade to linodego v1
+		if !fakeAPI.didRequestOccur("GET", "/nodebalancers", "") {
+			t.Fatalf("unexpected API calls: %v", fakeAPI.requests)
+		}
+		// TODO(okokes): check for DELETE on the old NB
+	})
+}
+
 func testGetNodeBalancerForServiceIDDoesNotExist(t *testing.T, client *linodego.Client, _ *fakeAPI) {
 	lb := &loadbalancers{client, "us-west", nil}
 	bogusNodeBalancerID := "123456"