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.

Sign up for GitHub

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

Jump to bottom

Do not skip externalLB update if some nodes are not found. #95559

Merged
merged 1 commit into from Oct 20, 2020
Merged

Do not skip externalLB update if some nodes are not found. #95559

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 5 additions & 1 deletion staging/src/k8s.io/legacy-cloud-providers/gce/gce_instances.go
View file
Open in desktop
Expand Up @@ -561,7 +561,11 @@ func (g *Cloud) getInstancesByNames(names []string) ([]*gceInstance, error) {
return nil, err
}
if len(foundInstances) != len(names) {
return nil, cloudprovider.InstanceNotFound
if len(foundInstances) == 0 {
// return error so the TargetPool nodecount does not drop to 0 unexpectedly.
return nil, cloudprovider.InstanceNotFound
}
klog.Warningf("getFoundInstanceByNames - input instances %d, found %d. Continuing LoadBalancer Update", len(names), len(foundInstances))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe not an issue - would there be any risk that we failed to find any node mistakenly and still update the LB?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The one risk I can think of - If GCE instance get calls are failing due to some reason (rate limiting or backend is really down), we will end up updating the TargetPool with 0 instances. Made a change to return error if we got 0 instances back.

}
return foundInstances, nil
}
Expand Down
18 changes: 18 additions & 0 deletions staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_external_test.go
View file
Open in desktop
Expand Up @@ -372,6 +372,24 @@ func TestUpdateExternalLoadBalancer(t *testing.T) {
[]string{fmt.Sprintf("/zones/%s/instances/%s", vals.ZoneName, nodeName)},
pool.Instances,
)

anotherNewNodeName := "test-node-3"
newNodes, err = createAndInsertNodes(gce, []string{nodeName, newNodeName, anotherNewNodeName}, vals.ZoneName)
assert.NoError(t, err)

// delete one of the existing nodes, but include it in the list
err = gce.DeleteInstance(gce.ProjectID(), vals.ZoneName, nodeName)
require.NoError(t, err)

// The update should ignore the reference to non-existent node "test-node-1", but update target pool with rest of the valid nodes.
err = gce.updateExternalLoadBalancer(vals.ClusterName, svc, newNodes)
assert.NoError(t, err)

pool, err = gce.GetTargetPool(lbName, gce.region)
require.NoError(t, err)

namePrefix := fmt.Sprintf("/zones/%s/instances/", vals.ZoneName)
assert.ElementsMatch(t, pool.Instances, []string{namePrefix + newNodeName, namePrefix + anotherNewNodeName})
}

func TestEnsureExternalLoadBalancerDeleted(t *testing.T) {
Expand Down