Return an error from gce.EnsureTCPLoadBalancer with no hosts. #13336
Conversation
|
@a-robinson @fabioy @lavalamp thoughts? |
|
Addresses #13327 |
|
GCE e2e build/test failed for commit 8305d28e48640c44b2f8e100ccf96b24a62cd2b4. |
|
Travis, shippable, and Jenkins are all unhappy very quickly - can you fix whatever the issue is? |
| glog.V(2).Info("Checking if load balancer already exists: %s", name) | ||
| if len(hosts) == 0 { | ||
| glog.V(2).Error("Call to EnsureTCPLoadBalancer with no hosts") | ||
| return nil, errors.New("Cannot EnsureTCPLoadBalancer() with no hosts") |
There was a problem hiding this comment.
Shouldn't this still delete any previous load balancer?
There was a problem hiding this comment.
Not sure. My change would essentially make len(hosts) == 0 an error state for loadBalancers. As long as we are in that state, we are going to continue retrying this call. Does it matter whether we clear out the old LB now or when we actually have hosts?
There was a problem hiding this comment.
I also don't really think we want to do so - it seems best to keep the old load balancer's IP around.
There was a problem hiding this comment.
Responding to @lavalamp's comment: "It seems like a problem if the old load balancer points to a bunch of things that no longer exist?"
The node sync loop part of the service controller already handles resizing the target pools down as the nodes in the cluster change. Not deleting the load balancer entirely enables a use case where you can scale down your cluster's nodes to zero but still have the same IP address for the service if you later re-add any nodes.
|
GCE e2e build/test passed for commit f045ea280fa52f0b25740a10d3dbb360f1191a0a. |
| @@ -352,7 +352,12 @@ func makeFirewallName(name string) string { | |||
| // TODO(a-robinson): Don't just ignore specified IP addresses. Check if they're | |||
| // owned by the project and available to be used, and use them if they are. | |||
| func (gce *GCECloud) EnsureTCPLoadBalancer(name, region string, externalIP net.IP, ports []*api.ServicePort, hosts []string, affinityType api.ServiceAffinity) (*api.LoadBalancerStatus, error) { | |||
| glog.V(2).Info("Checking if load balancer already exists: %s", name) | |||
| if len(hosts) == 0 { | |||
| glog.Errorf("Call to EnsureTCPLoadBalancer with no hosts") | |||
There was a problem hiding this comment.
This log statement isn't really needed, the error you return will be logged at a higher level.
There was a problem hiding this comment.
It seems like a problem if the old load balancer points to a bunch of things that no longer exist?
There was a problem hiding this comment.
Sorry, that was supposed to be a reply to the other comment :)
There was a problem hiding this comment.
Done (to @a-robinson's comment)
cjcullen
force-pushed
the
gce
branch
2 times, most recently
from
1173172
to
7278734
Compare
|
GCE e2e build/test passed for commit cb7d3f0. |
a-robinson
added
lgtm
|
LGTM |
|
@k8s-bot test this [testing build queue, sorry for the noise] |
|
GCE e2e build/test passed for commit cb7d3f0. |
|
Automatic merge from SubmitQueue |
Auto commit by PR queue bot
| @@ -352,7 +352,11 @@ func makeFirewallName(name string) string { | |||
| // TODO(a-robinson): Don't just ignore specified IP addresses. Check if they're | |||
| // owned by the project and available to be used, and use them if they are. | |||
| func (gce *GCECloud) EnsureTCPLoadBalancer(name, region string, externalIP net.IP, ports []*api.ServicePort, hosts []string, affinityType api.ServiceAffinity) (*api.LoadBalancerStatus, error) { | |||
| glog.V(2).Info("Checking if load balancer already exists: %s", name) | |||
| if len(hosts) == 0 { | |||
| return nil, fmt.Errorf("Cannot EnsureTCPLoadBalancer() with no hosts") | |||
There was a problem hiding this comment.
errors should start with a lower case letter :/
https://github.com/golang/go/wiki/CodeReviewComments#error-strings
…3336-upstream-release-1.0 Return an error from gce.EnsureTCPLoadBalancer with no hosts.
It would be nicer if we could treat len(hosts) == 0 as valid, and create all the resources with a 0 VM target pool. Then the syncNode loop would be able to fix up the target pool when the cluster added nodes. But, we have no way of generating the hostTag for the firewall without any nodes. By returning an error, we let the servicecontroller know to keep trying.