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.
This commit is contained in:
Pavithra Ramesh 2020-05-21 17:36:46 -07:00
parent dd552e2059
commit 7b3e25dc25
3 changed files with 57 additions and 2 deletions

View File

@ -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)
}

View File

@ -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

View File

@ -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)
}