Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automated cherry pick of #91392: Check for GCE finalizer in GetLoadBalancer. #91478

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -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)
}

Expand Down
Expand Up @@ -63,6 +63,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
Expand All @@ -86,7 +89,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)
Expand Down Expand Up @@ -314,10 +316,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
Expand Down
Expand Up @@ -1598,5 +1598,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)
}