Unrevert #14608 and decrease the latency of GCE load balancer deletions #14964
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,7 @@ import ( | |
|
|
||
| "k8s.io/kubernetes/pkg/api" | ||
| "k8s.io/kubernetes/pkg/cloudprovider" | ||
| "k8s.io/kubernetes/pkg/util/errors" | ||
| "k8s.io/kubernetes/pkg/util/sets" | ||
| "k8s.io/kubernetes/pkg/util/wait" | ||
|
|
||
|
|
@@ -349,6 +350,27 @@ func makeFirewallName(name string) string { | |
| return fmt.Sprintf("k8s-fw-%s", name) | ||
| } | ||
|
|
||
| func (gce *GCECloud) getAddress(name, region string) (string, bool, error) { | ||
| address, err := gce.service.Addresses.Get(gce.projectID, region, name).Do() | ||
| if err == nil { | ||
| return address.Address, true, nil | ||
| } | ||
| if isHTTPErrorCode(err, http.StatusNotFound) { | ||
| return "", false, nil | ||
| } | ||
| return "", false, err | ||
| } | ||
|
|
||
| func ownsAddress(ip net.IP, addrs []*compute.Address) bool { | ||
| ipStr := ip.String() | ||
| for _, addr := range addrs { | ||
| if addr.Address == ipStr { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| // EnsureTCPLoadBalancer is an implementation of TCPLoadBalancer.EnsureTCPLoadBalancer. | ||
| // 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. | ||
|
|
@@ -357,7 +379,44 @@ func (gce *GCECloud) EnsureTCPLoadBalancer(name, region string, loadBalancerIP n | |
| return nil, fmt.Errorf("Cannot EnsureTCPLoadBalancer() with no hosts") | ||
| } | ||
|
|
||
| if loadBalancerIP == nil { | ||
| glog.V(2).Info("Checking if the static IP address already exists: %s", name) | ||
| address, exists, err := gce.getAddress(name, region) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error looking for gce address: %v", err) | ||
| } | ||
| if !exists { | ||
| // Note, though static addresses that _aren't_ in use cost money, ones that _are_ in use don't. | ||
| // However, quota is limited to only 7 addresses per region by default. | ||
| op, err := gce.service.Addresses.Insert(gce.projectID, region, &compute.Address{Name: name}).Do() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error creating gce static IP address: %v", err) | ||
| } | ||
| if err := gce.waitForRegionOp(op, region); err != nil { | ||
| return nil, fmt.Errorf("error waiting for gce static IP address to complete: %v", err) | ||
| } | ||
| address, exists, err = gce.getAddress(name, region) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error re-getting gce static IP address: %v", err) | ||
| } | ||
| if !exists { | ||
| return nil, fmt.Errorf("failed to re-get gce static IP address for %s", name) | ||
| } | ||
| } | ||
| if loadBalancerIP = net.ParseIP(address); loadBalancerIP == nil { | ||
| return nil, fmt.Errorf("error parsing gce static IP address: %s", address) | ||
| } | ||
| } else { | ||
| addresses, err := gce.service.Addresses.List(gce.projectID, region).Do() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to list gce IP addresses: %v", err) | ||
| } | ||
| if !ownsAddress(loadBalancerIP, addresses.Items) { | ||
| return nil, fmt.Errorf("this gce project don't own the IP address: %s", loadBalancerIP.String()) | ||
| } | ||
| } | ||
|
|
||
| glog.V(2).Info("Checking if load balancer already exists: %s", name) | ||
| _, exists, err := gce.GetTCPLoadBalancer(name, region) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error checking if GCE load balancer already exists: %v", err) | ||
|
|
@@ -395,13 +454,11 @@ func (gce *GCECloud) EnsureTCPLoadBalancer(name, region string, loadBalancerIP n | |
| } | ||
| req := &compute.ForwardingRule{ | ||
| Name: name, | ||
| IPAddress: loadBalancerIP.String(), | ||
|
There was a problem hiding this comment. This line seems to duplicate line https://github.com/kubernetes/kubernetes/blob/master/pkg/cloudprovider/providers/gce/gce.go#L403 There was a problem hiding this comment. We can delete L403, as loadBalancerIP won't be nil now. There was a problem hiding this comment. Indeed, good catch. I should have caught that in the initial review |
||
| IPProtocol: "TCP", | ||
| PortRange: fmt.Sprintf("%d-%d", minPort, maxPort), | ||
| Target: gce.targetPoolURL(name, region), | ||
| } | ||
|
|
||
| op, err := gce.service.ForwardingRules.Insert(gce.projectID, region, req).Do() | ||
| if err != nil && !isHTTPErrorCode(err, http.StatusConflict) { | ||
|
|
@@ -556,45 +613,93 @@ func (gce *GCECloud) UpdateTCPLoadBalancer(name, region string, hosts []string) | |
|
|
||
| // EnsureTCPLoadBalancerDeleted is an implementation of TCPLoadBalancer.EnsureTCPLoadBalancerDeleted. | ||
| func (gce *GCECloud) EnsureTCPLoadBalancerDeleted(name, region string) error { | ||
| err := errors.AggregateGoroutines( | ||
| func() error { return gce.deleteFirewall(name, region) }, | ||
| func() error { | ||
| if err := gce.deleteForwardingRule(name, region); err != nil { | ||
| return err | ||
| } | ||
| // The forwarding rule must be deleted before either the target pool or | ||
| // static IP address can, unfortunately. | ||
| err := errors.AggregateGoroutines( | ||
| func() error { return gce.deleteTargetPool(name, region) }, | ||
| func() error { return gce.deleteStaticIP(name, region) }, | ||
| ) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return nil | ||
| }, | ||
| ) | ||
| if err != nil { | ||
| return errors.Flatten(err) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (gce *GCECloud) deleteForwardingRule(name, region string) error { | ||
| op, err := gce.service.ForwardingRules.Delete(gce.projectID, region, name).Do() | ||
| if err != nil && isHTTPErrorCode(err, http.StatusNotFound) { | ||
| glog.Infof("Forwarding rule %s already deleted. Continuing to delete other resources.", name) | ||
| } else if err != nil { | ||
| glog.Warningf("Failed to delete forwarding rule %s: got error %s.", name, err.Error()) | ||
| return err | ||
| } else { | ||
| if err := gce.waitForRegionOp(op, region); err != nil { | ||
| glog.Warningf("Failed waiting for forwarding rule %s to be deleted: got error %s.", name, err.Error()) | ||
| return err | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (gce *GCECloud) deleteTargetPool(name, region string) error { | ||
| op, err := gce.service.TargetPools.Delete(gce.projectID, region, name).Do() | ||
| if err != nil && isHTTPErrorCode(err, http.StatusNotFound) { | ||
| glog.Infof("Target pool %s already deleted. Continuing to delete other resources.", name) | ||
| } else if err != nil { | ||
| glog.Warningf("Failed to delete target pool %s, got error %s.", name, err.Error()) | ||
| return err | ||
| } else { | ||
| if err := gce.waitForRegionOp(op, region); err != nil { | ||
| glog.Warningf("Failed waiting for target pool %s to be deleted: got error %s.", name, err.Error()) | ||
| return err | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (gce *GCECloud) deleteFirewall(name, region string) error { | ||
| fwName := makeFirewallName(name) | ||
| op, err := gce.service.Firewalls.Delete(gce.projectID, fwName).Do() | ||
| if err != nil && isHTTPErrorCode(err, http.StatusNotFound) { | ||
| glog.Infof("Firewall %s already deleted. Continuing to delete other resources.", name) | ||
| } else if err != nil { | ||
| glog.Warningf("Failed to delete firewall %s, got error %v", fwName, err) | ||
| return err | ||
| } else { | ||
| if err := gce.waitForGlobalOp(op); err != nil { | ||
| glog.Warningf("Failed waiting for Firewall %s to be deleted. Got error: %v", fwName, err) | ||
| return err | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (gce *GCECloud) deleteStaticIP(name, region string) error { | ||
| op, err := gce.service.Addresses.Delete(gce.projectID, region, name).Do() | ||
| if err != nil && isHTTPErrorCode(err, http.StatusNotFound) { | ||
| glog.Infof("Static IP address %s already deleted. Continuing to delete other resources.", name) | ||
| } else if err != nil { | ||
| glog.Warningf("Failed to delete static IP address %s, got error %v", name, err) | ||
| return err | ||
| } else { | ||
| if err := gce.waitForRegionOp(op, region); err != nil { | ||
| glog.Warningf("Failed waiting for address %s to be deleted, got error: %v", name, err) | ||
| return err | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // UrlMap management | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be sure, can one static IP be matched to multiple forwarding rules? I assume the answer is yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To answer to my own question, the answer is no, one static IP can only be bound to one forwarding rule.
This doesn't seem to cause any problem, because GCE's API will fail the forwarding rule creation anyway. I'm considering if we can maintain a list of static IPs that have already bound to a forwarding rule, so that we can return an error before making a remote call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct - each static IP can only be bound to one forwarding rule. However, because we name the address and forwarding rule the same (based on the service UID), we shouldn't hit a case where a particular static IP is already bound to a different forwarding rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree.
loadBalancerIPis parsed fromservice.Spec.LoadBalancerIP, it can be any value specified by the user, so it's possible to be a static IP that has already bound to a different forwarding rule.i.e, if a user calls EnsureTCPLoadBalancer with a new name but existing loadBalancerIP, then it will hit the case described above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, this may not be a problem. In such a case, the service controller will keep failing creating the load balancer, which is a fair behavior. I'm just suggesting maybe we can fail such requests before making a remote call to save a roundtrip.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're absolutely right, I forgot we had added that option.
Forwarding rule creation will fail with the error that the IP address is already in use. Relying on that seems fine since it should fail quickly, but we could short-circuit things by using the attributes of the address returned from the list request. The attributes include whether it's in use and by what.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know the attributes already contain the information. It sounds like a good idea. I will not block the PR on this possible improvement. I will LGTM it when you delete line 403.