Ignore ErrNotFound when delete LB resources #46181

FengyunPan · 2017-05-21T14:00:56Z

IsNotFound error is fine since that means the object is
deleted already, so let's check it before return error.

k8s-ci-robot · 2017-05-21T14:01:03Z

Hi @FengyunPan. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

FengyunPan · 2017-05-21T14:07:24Z

/release-note-none

anguslees · 2017-05-22T01:03:55Z

pkg/cloudprovider/providers/openstack/openstack_loadbalancer.go

@@ -140,9 +140,6 @@ func getFloatingIPByPortID(client *gophercloud.ServiceClient, portID string) (*f
 			return false, err
 		}
 		floatingIPList = append(floatingIPList, f...)
-		if len(floatingIPList) > 1 {


This means the loop might keep going, only to error out later (which is fine and I agree it's an error case not worth optimising for).

Perhaps the final return line (L146) could become:

return len(floatingIPList) <= 1, nil

And this would achieve the same early-exit-on-multiple-results but with far less visual overhead.

Since this loop is such a common pattern in the openstack provider, an alternative that I've been meaning to do for a while would be to replace the whole lot with some sort of generic code that takes an extraction function (like floatingips.ExtractFloatingIPs), does this generic "build a list, and abort if you get more than one result" logic, and just returns a single (interface{}, error). Again, I wish golang had parameterised types :(

@anguslees Thank for your review very much.
My modify just prevent it from checking {len(floatingIPList) > 1} again, and I see your comment, I will check it in loop and don't check it at line 157.

anguslees · 2017-05-22T01:14:12Z

pkg/cloudprovider/providers/openstack/openstack_loadbalancer.go

@@ -285,6 +273,9 @@ func getListenersByLoadBalancerID(client *gophercloud.ServiceClient, id string)
 		return true, nil
 	})
 	if err != nil {
+		if isNotFound(err) {


What's the benefit of pushing this down here (and similar elsewhere), rather than have the caller use isNotFound(...)? From experience I'm nervous about ensuring that all functions correctly convert Openstack not-found errors into ErrNotFound - we've certainly had places where this was missed in the past.

Yeah, I know it, when len(...) is 0, K8S will return ErrNotFound and caller can't use isNotFound(...). Ennn, I will pick one.

anguslees · 2017-05-22T01:16:54Z

pkg/cloudprovider/providers/openstack/openstack_loadbalancer.go

-					continue
-				} else {
-					return nil, fmt.Errorf("Error getting address for node %s: %v", node.Name, err)
+		if !isNotFound(err) && members != nil {


You have changed this code to separately use both a literal err == ErrNotFound and isNotFound(err). You should pick one :)

I suggest leaving this to use isNotFound() and remove all the places above where you've pushed this logic into the getFooByBar functions - because it's going to be a lot more fragile to rely on everyone returning just the ErrNotFound value.

You are right, I will update it.

anguslees · 2017-05-22T01:29:47Z

pkg/cloudprovider/providers/openstack/openstack_loadbalancer.go

 			return fmt.Errorf("Error getting pool for listener %s: %v", listener.ID, err)
 		}
+		if err == ErrNotFound {
+			// Unknown error, retry later
+			continue


Will this function be retried if it does not return an error? It's been a while since I read the calling code, but I thought we had to return an error in order to be retried ...

Sorry, this description is incorrect. I will update it.

anguslees · 2017-05-22T01:40:23Z

pkg/cloudprovider/providers/openstack/openstack_loadbalancer.go

+			for _, node := range nodes {
+				addr, err := nodeAddressForLB(node)
+				if err != nil {
+					if err == ErrNoAddressFound {


I think this ErrNotFound -> ErrNoAddressFound and the additional "ignore if not found" check around this foreach-node loop are the behavioural changes in this PR, and the rest is refactoring. Is that correct or did I miss any others?

The nodeAddressForLB() can return ErrNoAddressFound, not ErrNotFound.
I do not know if it is correct, I will check it later.

NickrenREN · 2017-05-22T03:22:15Z

@anguslees @FengyunPan Maybe we should keep consistent style to define Not Found Err.
Function return or caller check ?

FengyunPan · 2017-05-22T06:07:48Z

@NickrenREN Ok.

NickrenREN · 2017-05-22T08:21:57Z

pkg/cloudprovider/providers/openstack/openstack_loadbalancer.go

 		return nil, err
 	}

 	if len(listenerPools) == 0 {
 		return nil, ErrNotFound
-	} else if len(listenerPools) > 1 {
-		return nil, ErrMultipleResults


if len(listenerPools) >1 , do not return err ?

ListenerID is unique, can't get { len(listenerPools) >1}.
The getPoolByListenerID() just return Found or notFound, the check of 'if len(listenerPools) > 1' is useless.

I agree the check should never fire, but I'm nervous about trusting the OpenStack API to always do what we expect :)

On the other hand, I agree these additional checks add visual noise - so I only have a weak preference for keeping them.

NickrenREN · 2017-05-22T08:24:10Z

pkg/cloudprovider/providers/openstack/openstack_loadbalancer.go

-					continue
-				} else {
-					return nil, fmt.Errorf("Error getting address for node %s: %v", node.Name, err)
+		if !lbaas.IsNotFound(err) && members != nil {


Why this ? if members != nil and of course err returned is nil and do we really need this?

You are right, I delete it soon.

NickrenREN · 2017-05-22T08:37:00Z

pkg/cloudprovider/providers/openstack/openstack_loadbalancer.go

@@ -1203,10 +1188,26 @@ func (lbaas *LbaasV2) EnsureLoadBalancerDeleted(clusterName string, service *v1.
 	return nil
 }

+func (lbaas *LbaasV2) IsNotFound(err error) bool {


Is this needed (and so is the similar function below)?

Yes, caller don't know getxxxx() return ErrNotFound or isNotFound err, we can use IsNotFound() to check err.

Just move this into the real isNotFound() - in fact I thought I had already extended that to check ErrNotFound some time ago, but apparently not...

anguslees

(Thanks for your persistence @FengyunPan :)

anguslees · 2017-05-26T01:05:24Z

pkg/cloudprovider/providers/openstack/openstack_loadbalancer.go

@@ -346,13 +323,19 @@ func getMembersByPoolID(client *gophercloud.ServiceClient, id string) ([]v2pools
 			return false, err
 		}
 		members = append(members, membersList...)
-
+		if len(members) > 1 {


This particular function is intended to return multiple results - see pluralised name and array return type.

anguslees · 2017-05-26T01:08:08Z

pkg/cloudprovider/providers/openstack/openstack_loadbalancer.go

@@ -288,6 +268,10 @@ func getListenersByLoadBalancerID(client *gophercloud.ServiceClient, id string)
 		return nil, err
 	}

+	if len(existingListeners) == 0 {


This function is intended to return a (possibly empty) list of results - ie: I think it's up to the caller to decide if len() == 0 is an error.

anguslees · 2017-05-26T01:12:36Z

pkg/cloudprovider/providers/openstack/openstack_loadbalancer.go

 		return nil, err
 	}

 	if len(listenerPools) == 0 {
 		return nil, ErrNotFound
-	} else if len(listenerPools) > 1 {
-		return nil, ErrMultipleResults


I agree the check should never fire, but I'm nervous about trusting the OpenStack API to always do what we expect :)

On the other hand, I agree these additional checks add visual noise - so I only have a weak preference for keeping them.

anguslees · 2017-05-26T01:13:08Z

pkg/cloudprovider/providers/openstack/openstack_loadbalancer.go

 		return true, nil
 	})
 	if err != nil {
 		return nil, err
 	}

+	if len(members) == 0 {


Ditto - anything declared as returning a list is fine to return an empty list.

anguslees · 2017-05-26T01:13:33Z

pkg/cloudprovider/providers/openstack/openstack_loadbalancer.go

@@ -406,13 +388,19 @@ func getSecurityGroupRules(client *gophercloud.ServiceClient, opts rules.ListOpt
 			return false, err
 		}
 		securityRules = append(securityRules, ruleList...)
+		if len(securityRules) > 1 {


Same list return comments as above

anguslees · 2017-05-26T01:17:59Z

pkg/cloudprovider/providers/openstack/openstack_loadbalancer.go

@@ -1203,10 +1188,26 @@ func (lbaas *LbaasV2) EnsureLoadBalancerDeleted(clusterName string, service *v1.
 	return nil
 }

+func (lbaas *LbaasV2) IsNotFound(err error) bool {


Just move this into the real isNotFound() - in fact I thought I had already extended that to check ErrNotFound some time ago, but apparently not...

IsNotFound error is fine since that means the object is deleted already, so let's check it before return error.

FengyunPan · 2017-05-27T10:15:42Z

@ anguslees Hi, I keep them and just ignore ErrNotFound when deleting LB resources.

dims · 2017-06-01T20:02:12Z

@k8s-bot ok to test

dims · 2017-06-01T20:02:39Z

/assign

dims · 2017-06-01T20:03:42Z

/lgtm

dims · 2017-06-01T20:38:51Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this

fejta · 2017-06-01T20:53:49Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this
ref kubernetes/test-infra#2931

fejta · 2017-06-01T20:59:45Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this

NickrenREN · 2017-06-02T03:19:25Z

@k8s-bot pull-kubernetes-kubemark-e2e-gce test this
@k8s-bot pull-kubernetes-federation-e2e-gce test this

NickrenREN · 2017-06-02T03:20:18Z

/approve

FengyunPan · 2017-06-02T05:05:12Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this

wojtek-t · 2017-06-06T00:02:19Z

/approve no issue

wojtek-t · 2017-06-06T00:08:04Z

/approve no-issue

k8s-github-robot · 2017-06-06T00:52:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FengyunPan, NickrenREN, dims, wojtek-t

Associated issue requirement bypassed by: wojtek-t

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~pkg/cloudprovider/providers/openstack/OWNERS~~ [NickrenREN,wojtek-t]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

FengyunPan · 2017-06-23T13:01:26Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this
@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

FengyunPan · 2017-06-23T15:51:57Z

/test pull-kubernetes-federation-e2e-gce

k8s-github-robot · 2017-06-23T16:35:10Z

Automatic merge from submit-queue

Automatic merge from submit-queue Ignore ErrNotFound when delete LB resources IsNotFound error is fine since that means the object is deleted already, so let's check it before return error.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 21, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 21, 2017

k8s-github-robot assigned anguslees and NickrenREN May 21, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels May 21, 2017

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels May 21, 2017

anguslees reviewed May 22, 2017

View reviewed changes

FengyunPan force-pushed the ignore-LBnotfound branch 2 times, most recently from 39650d9 to c22b083 Compare May 22, 2017 07:56

NickrenREN reviewed May 22, 2017

View reviewed changes

FengyunPan force-pushed the ignore-LBnotfound branch from c22b083 to 26d06a2 Compare May 22, 2017 09:24

anguslees reviewed May 26, 2017

View reviewed changes

FengyunPan force-pushed the ignore-LBnotfound branch from 26d06a2 to 3d7dfd5 Compare May 27, 2017 10:06

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 27, 2017

Ignore ErrNotFound when delete LB resources

f5f75f3

IsNotFound error is fine since that means the object is deleted already, so let's check it before return error.

FengyunPan force-pushed the ignore-LBnotfound branch from 3d7dfd5 to f5f75f3 Compare May 27, 2017 10:10

k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 27, 2017

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 1, 2017

k8s-ci-robot assigned dims Jun 1, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 1, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 2, 2017

k8s-github-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 5, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 6, 2017

k8s-github-robot merged commit 72cb080 into kubernetes:master Jun 23, 2017

Ignore ErrNotFound when delete LB resources #46181

Ignore ErrNotFound when delete LB resources #46181

Conversation

FengyunPan commented May 21, 2017

k8s-ci-robot commented May 21, 2017

FengyunPan commented May 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FengyunPan May 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickrenREN commented May 22, 2017

FengyunPan commented May 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anguslees left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FengyunPan commented May 27, 2017

dims commented Jun 1, 2017

dims commented Jun 1, 2017

dims commented Jun 1, 2017

dims commented Jun 1, 2017

fejta commented Jun 1, 2017

fejta commented Jun 1, 2017

NickrenREN commented Jun 2, 2017

NickrenREN commented Jun 2, 2017

FengyunPan commented Jun 2, 2017

wojtek-t commented Jun 6, 2017

wojtek-t commented Jun 6, 2017

k8s-github-robot commented Jun 6, 2017

FengyunPan commented Jun 23, 2017

FengyunPan commented Jun 23, 2017

k8s-github-robot commented Jun 23, 2017

FengyunPan May 22, 2017 •

edited