diff --git a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer.go b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer.go index 33d521bf947..5e4bb55f817 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer.go +++ b/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) } diff --git a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go index cda339b8b2d..c3062448dd1 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go +++ b/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 diff --git a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal_test.go b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal_test.go index 5c9ec2e5386..f3b2d902ce3 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal_test.go +++ b/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) }