From 7b3e25dc25685b46e341765422d3f145a3d82706 Mon Sep 17 00:00:00 2001 From: Pavithra Ramesh Date: Thu, 21 May 2020 17:36:46 -0700 Subject: [PATCH] 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. --- .../gce/gce_loadbalancer.go | 5 ++ .../gce/gce_loadbalancer_internal.go | 6 ++- .../gce/gce_loadbalancer_internal_test.go | 48 ++++++++++++++++++- 3 files changed, 57 insertions(+), 2 deletions(-) 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) }