Check for GCE finalizer in GetLoadBalancer.

This is a more accurate check that looking for the forwarding rule, which could be
deleted if delete flow was partially complete and abrupted by an upgrade, or if deleted
by user manually.

Added unit test.
Addressed review comments.

Author: prameshj

staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer.go

@@ -111,6 +111,11 @@ func (g *Cloud) GetLoadBalancer(ctx context.Context, clusterName string, svc *v1
 
 		return status, true, nil
 	}
+	// Checking for finalizer is more accurate because controller restart could happen in the middle of resource
+	// deletion. So even though forwarding rule was deleted, cleanup might not have been complete.
+	if hasFinalizer(svc, ILBFinalizerV1) {
+		return &v1.LoadBalancerStatus{}, true, nil
+	}
 	return nil, false, ignoreNotFound(err)
 }
 

staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go

@@ -62,6 +62,9 @@ func (g *Cloud) ensureInternalLoadBalancer(clusterName, clusterID string, svc *v
 			"Skipped ensureInternalLoadBalancer as service contains '%s' finalizer.", ILBFinalizerV2)
 		return nil, cloudprovider.ImplementedElsewhere
 	}
+
+	loadBalancerName := g.GetLoadBalancerName(context.TODO(), clusterName, svc)
+	klog.V(2).Infof("ensureInternalLoadBalancer(%v): Attaching %q finalizer", loadBalancerName, ILBFinalizerV1)
 	if err := addFinalizer(svc, g.client.CoreV1(), ILBFinalizerV1); err != nil {
 		klog.Errorf("Failed to attach finalizer '%s' on service %s/%s - %v", ILBFinalizerV1, svc.Namespace, svc.Name, err)
 		return nil, err
@@ -85,7 +88,6 @@ func (g *Cloud) ensureInternalLoadBalancer(clusterName, clusterID string, svc *v
 		}
 	}
 
-	loadBalancerName := g.GetLoadBalancerName(context.TODO(), clusterName, svc)
 	sharedBackend := shareBackendService(svc)
 	backendServiceName := makeBackendServiceName(loadBalancerName, clusterID, sharedBackend, scheme, protocol, svc.Spec.SessionAffinity)
 	backendServiceLink := g.getBackendServiceLink(backendServiceName)
@@ -317,10 +319,12 @@ func (g *Cloud) ensureInternalLoadBalancerDeleted(clusterName, clusterID string,
 
 	// Try deleting instance groups - expect ResourceInuse error if needed by other LBs
 	igName := makeInstanceGroupName(clusterID)
+	klog.V(2).Infof("ensureInternalLoadBalancerDeleted(%v): Attempting delete of instanceGroup %v", loadBalancerName, igName)
 	if err := g.ensureInternalInstanceGroupsDeleted(igName); err != nil && !isInUsedByError(err) {
 		return err
 	}
 
+	klog.V(2).Infof("ensureInternalLoadBalancerDeleted(%v): Removing %q finalizer", loadBalancerName, ILBFinalizerV1)
 	if err := removeFinalizer(svc, g.client.CoreV1(), ILBFinalizerV1); err != nil {
 		klog.Errorf("Failed to remove finalizer '%s' on service %s/%s - %v", ILBFinalizerV1, svc.Namespace, svc.Name, err)
 		return err

staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal_test.go

@@ -1590,5 +1590,51 @@ func TestEnsureLoadBalancerSkipped(t *testing.T) {
 	// No loadbalancer resources will be created due to the ILB Feature Gate
 	assert.Empty(t, status)
 	assertInternalLbResourcesDeleted(t, gce, svc, vals, true)
+}
+
+// TestEnsureLoadBalancerPartialDelete simulates a partial delete and checks whether deletion completes after a second
+// attempt.
+func TestEnsureLoadBalancerPartialDelete(t *testing.T) {
+	t.Parallel()
+
+	vals := DefaultTestClusterValues()
+	nodeNames := []string{"test-node-1"}
+
+	gce, err := fakeGCECloud(vals)
+	require.NoError(t, err)
 
+	svc := fakeLoadbalancerService(string(LBTypeInternal))
+	svc, err = gce.client.CoreV1().Services(svc.Namespace).Create(context.TODO(), svc, metav1.CreateOptions{})
+	require.NoError(t, err)
+	status, err := createInternalLoadBalancer(gce, svc, nil, nodeNames, vals.ClusterName, vals.ClusterID, vals.ZoneName)
+	require.NoError(t, err)
+	assert.NotEmpty(t, status.Ingress)
+	assertInternalLbResources(t, gce, svc, vals, nodeNames)
+	svc, err = gce.client.CoreV1().Services(svc.Namespace).Get(context.TODO(), svc.Name, metav1.GetOptions{})
+	require.NoError(t, err)
+	if !hasFinalizer(svc, ILBFinalizerV1) {
+		t.Errorf("Expected finalizer '%s' not found in Finalizer list - %v", ILBFinalizerV1, svc.Finalizers)
+	}
+	// Delete the forwarding rule to simulate controller getting shut down on partial cleanup
+	lbName := gce.GetLoadBalancerName(context.TODO(), "", svc)
+	err = gce.DeleteRegionForwardingRule(lbName, gce.region)
+	require.NoError(t, err)
+	// Check output of GetLoadBalancer
+	_, exists, err := gce.GetLoadBalancer(context.TODO(), vals.ClusterName, svc)
+	require.NoError(t, err)
+	assert.True(t, exists)
+	// call EnsureDeleted again
+	err = gce.EnsureLoadBalancerDeleted(context.TODO(), vals.ClusterName, svc)
+	require.NoError(t, err)
+	// Make sure all resources are gone
+	assertInternalLbResourcesDeleted(t, gce, svc, vals, true)
+	// Ensure that the finalizer has been deleted
+	svc, err = gce.client.CoreV1().Services(svc.Namespace).Get(context.TODO(), svc.Name, metav1.GetOptions{})
+	require.NoError(t, err)
+	if hasFinalizer(svc, ILBFinalizerV1) {
+		t.Errorf("Finalizer '%s' not deleted from service - %v", ILBFinalizerV1, svc.Finalizers)
+	}
+	_, exists, err = gce.GetLoadBalancer(context.TODO(), vals.ClusterName, svc)
+	require.NoError(t, err)
+	assert.False(t, exists)
 }