Adapt loadbalancer deleting/updating when using cloudprovider openstack in openstack/liberty #38959

daconstenla · 2016-12-19T12:42:45Z

What this PR does / why we need it:
Make an extra verification on the returned listeners and pools because gophercloud query doesn't filter the results by loadbalancerID / listenerID respectively when using openstack/librerty.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #
#33759
Special notes for your reviewer:
#33759 it's supposed to have a pull request which fixes this problem but in the release 1.5 loadbalancers doesn't use that patched code.
Release note:

NONE

k8s-ci-robot · 2016-12-19T12:42:46Z

Hi @daconstenla. 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.

If you have questions or suggestions related to this bot's behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2016-12-19T12:42:47Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

Once you've signed, please reply here (e.g. "I signed it!") and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.

If you have questions or suggestions related to this bot's behavior, please file an issue against the kubernetes/test-infra repository.

k8s-reviewable · 2016-12-19T12:42:51Z

This change is

daconstenla · 2016-12-19T14:06:30Z

cla/linuxfoundation — commit missing GitHub user
Corrected, can the checks be re-launched or should I made a new commit to trigger it?

anguslees · 2016-12-20T23:31:49Z

@k8s-bot ok to test

Wat? I swear this is the 3rd time I've reviewed a fix for this issue :/

Now off to do some git archaeology to see where this is/isn't merged...
Edit: Ah, EnsureLoadBalancer was fixed, but not EnsureLoadBalancerDeleted. Oops.
git grep tells me that these are the last two calls to ExtractListeners and ExtractPools, so hopefully this will be the end of it.

anguslees · 2016-12-20T23:41:55Z

pkg/cloudprovider/providers/openstack/openstack_loadbalancer.go

@@ -1135,7 +1135,9 @@ func (lbaas *LbaasV2) EnsureLoadBalancerDeleted(clusterName string, service *v1.
 		}

 		for _, listener := range listenerList {
-			listenerIDs = append(listenerIDs, listener.ID)
+			if len(listener.Loadbalancers) > 0 && listener.Loadbalancers[0].ID == loadbalancer.ID {


This whole surrounding code should be replaced with a call to getListenersByLoadBalancerID

anguslees · 2016-12-20T23:46:10Z

pkg/cloudprovider/providers/openstack/openstack_loadbalancer.go

@@ -1154,8 +1156,13 @@ func (lbaas *LbaasV2) EnsureLoadBalancerDeleted(clusterName string, service *v1.
 		}

 		for _, pool := range poolsList {
-			poolIDs = append(poolIDs, pool.ID)
-			monitorIDs = append(monitorIDs, pool.MonitorID)
+			for _, listenerID := range listenerIDs {


Replace surrounding code by getPoolByListenerID

kiall · 2016-12-21T13:00:01Z

@anguslees yea, it looks like I missed a case in my last PR for this. No clue how we haven't hit this one :(

kiall · 2016-12-21T17:42:04Z

Can we apply the 1.5 milestone and cherrypick-candidate label? Over eager deletions is defiantly worth pushing into the 1.5 releases!

daconstenla · 2016-12-23T16:44:53Z

@anguslees I have made the changes you suggested (and some more), now I'm waiting confirmation from my organization to sign the cncf agreement.

k8s-ci-robot · 2016-12-23T16:52:14Z

Jenkins CRI GCE Node e2e failed for commit 1a7cb48ce81e56aaba8713ba4b9a37b94189ad28. Full PR test history.

The magic incantation to run this job again is @k8s-bot cri node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

xsgordon · 2016-12-30T16:31:13Z

@idvoretskyi can you please add the sig/openstack label?

daconstenla · 2017-02-07T09:18:53Z

Hi @anguslees, what happend with this PR? :)

/approve

anguslees

/approve

anguslees · 2017-02-13T23:29:08Z

@idvoretskyi As has been mentioned before, this PR should be cherrypicked onto 1.5. No idea if that's a process you kick off with github label magic, or ...

idvoretskyi · 2017-02-14T22:56:11Z

cc @saad-ali on comment above.

daconstenla · 2017-03-03T17:31:11Z

I have made a rebase and now tests fails, I'm not sure what's the problem, can someone help me?

FengyunPan · 2017-03-21T02:12:14Z

pkg/cloudprovider/providers/openstack/openstack_loadbalancer.go

-		if err != nil {
-			return err
+	for _, pool := range poolIDs {
+		membersList, err := getMembersByPoolID(lbaas.network, pool)


same here, like #43056.

daconstenla · 2017-03-30T08:13:48Z

Some \lgtm around? @grahamhayes @anguslees ?

Thanks

anguslees · 2017-04-24T00:30:52Z

/lgtm

(Sorry this PR has dragged out so long, thanks for your persistence!)

anguslees · 2017-04-24T00:32:19Z

/release-note-none

anguslees · 2017-04-24T02:02:19Z

@k8s-bot cvm gce e2e test this

FengyunPan · 2017-04-28T04:09:43Z

@daconstenla Maybe you should combine 4 commits into 1 commit? I find that the first commit is wrong and there are two commits is empty.
try: git push -f ?

FengyunPan · 2017-05-05T05:37:15Z

@erictune @grahamhayes @anguslees @daconstenla
Hi everyone，the pr addresses a big bug and has been submited 5 month ago, but it has not been merged. I think the reason is there are two invaild commit in pr, and the contributor did not remove them.
I can not wait to fix the big bug anymore. Can you merge my pr (#43056)? #43056

@FengyunPan

… the indicated filters when querying pools and/or listeners also added @FengyunPan modifications from PR#43055

dims · 2017-05-05T14:13:17Z

@k8s-bot ok to test

daconstenla · 2017-05-06T14:52:01Z

@k8s-bot bazel test this

daconstenla · 2017-05-07T19:54:04Z

Can somebody mark as /lgtm? @erictune @grahamhayes @anguslees
By the way, it seems like a test is failing, I had to launch 3 times bazel test before ends with a valid result.

daconstenla · 2017-05-11T10:30:25Z

Can somebody please, attend this pull-request and mark it as lgtm?

anguslees · 2017-05-16T16:56:27Z

/lgtm

k8s-github-robot · 2017-05-16T16:56:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anguslees, daconstenla

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

~~pkg/cloudprovider/providers/openstack/OWNERS~~ [anguslees]

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

k8s-ci-robot · 2017-05-21T09:54:00Z

@daconstenla: The following test(s) failed:

Test name	Commit	Details	Rerun command
pull-kubernetes-federation-e2e-gce	`a87d34c`	link	`@k8s-bot pull-kubernetes-federation-e2e-gce test this`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

k8s-github-robot · 2017-05-21T10:22:02Z

Automatic merge from submit-queue

Automatic merge from submit-queue Adapt loadbalancer deleting/updating when using cloudprovider openstack in openstack/liberty **What this PR does / why we need it**: Make an extra verification on the returned listeners and pools because gophercloud query doesn't filter the results by loadbalancerID / listenerID respectively when using **openstack/librerty**. **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes # kubernetes#33759 **Special notes for your reviewer**: kubernetes#33759 it's supposed to have a pull request which fixes this problem but in the release 1.5 loadbalancers doesn't use that patched code. **Release note**: NONE ```release-note ```

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Dec 19, 2016

k8s-github-robot assigned anguslees Dec 19, 2016

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Dec 19, 2016

anguslees suggested changes Dec 20, 2016

View reviewed changes

mmontecelo-gradiant mentioned this pull request Dec 21, 2016

OpenStack LBaaS V2 driver deletes all Pools and Monitors when any single service is deleted #33759

Closed

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 23, 2016

idvoretskyi added the area/provider/openstack Issues or PRs related to openstack provider label Jan 9, 2017

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jan 17, 2017

grahamhayes mentioned this pull request Jan 24, 2017

CAPS-1369: Over Eager Listener Deletion hpcloud/kubernetes#29

Merged

anguslees approved these changes Feb 13, 2017

View reviewed changes

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 21, 2017

grahamhayes mentioned this pull request Mar 20, 2017

Correct filtering of OpenStack LBaaS resources #43056

Closed

FengyunPan reviewed Mar 21, 2017

View reviewed changes

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 29, 2017

grodrigues3 added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 5, 2017

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

k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Apr 24, 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 Apr 24, 2017

added extra filter because in openestack/liberty gopher doesn't apply…

a87d34c

… the indicated filters when querying pools and/or listeners also added @FengyunPan modifications from PR#43055

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 5, 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 May 5, 2017

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

fejta removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label May 21, 2017

k8s-github-robot merged commit 39308b8 into kubernetes:master May 21, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adapt loadbalancer deleting/updating when using cloudprovider openstack in openstack/liberty #38959

Adapt loadbalancer deleting/updating when using cloudprovider openstack in openstack/liberty #38959

daconstenla commented Dec 19, 2016

k8s-ci-robot commented Dec 19, 2016

k8s-ci-robot commented Dec 19, 2016

k8s-reviewable commented Dec 19, 2016

daconstenla commented Dec 19, 2016 •

edited

anguslees commented Dec 20, 2016 •

edited

anguslees Dec 20, 2016

anguslees Dec 20, 2016

kiall commented Dec 21, 2016

kiall commented Dec 21, 2016

daconstenla commented Dec 23, 2016

k8s-ci-robot commented Dec 23, 2016

xsgordon commented Dec 30, 2016

daconstenla commented Feb 7, 2017

anguslees left a comment

anguslees commented Feb 13, 2017

idvoretskyi commented Feb 14, 2017

daconstenla commented Mar 3, 2017

FengyunPan Mar 21, 2017

daconstenla commented Mar 30, 2017

anguslees commented Apr 24, 2017

anguslees commented Apr 24, 2017

anguslees commented Apr 24, 2017

FengyunPan commented Apr 28, 2017 •

edited

FengyunPan commented May 5, 2017

dims commented May 5, 2017

daconstenla commented May 6, 2017

daconstenla commented May 7, 2017

daconstenla commented May 11, 2017

anguslees commented May 16, 2017

k8s-github-robot commented May 16, 2017

k8s-ci-robot commented May 21, 2017 •

edited

k8s-github-robot commented May 21, 2017

Adapt loadbalancer deleting/updating when using cloudprovider openstack in openstack/liberty #38959

Adapt loadbalancer deleting/updating when using cloudprovider openstack in openstack/liberty #38959

Conversation

daconstenla commented Dec 19, 2016

k8s-ci-robot commented Dec 19, 2016

k8s-ci-robot commented Dec 19, 2016

k8s-reviewable commented Dec 19, 2016

daconstenla commented Dec 19, 2016 • edited

anguslees commented Dec 20, 2016 • edited

anguslees Dec 20, 2016

Choose a reason for hiding this comment

anguslees Dec 20, 2016

Choose a reason for hiding this comment

kiall commented Dec 21, 2016

kiall commented Dec 21, 2016

daconstenla commented Dec 23, 2016

k8s-ci-robot commented Dec 23, 2016

xsgordon commented Dec 30, 2016

daconstenla commented Feb 7, 2017

anguslees left a comment

Choose a reason for hiding this comment

anguslees commented Feb 13, 2017

idvoretskyi commented Feb 14, 2017

daconstenla commented Mar 3, 2017

FengyunPan Mar 21, 2017

Choose a reason for hiding this comment

daconstenla commented Mar 30, 2017

anguslees commented Apr 24, 2017

anguslees commented Apr 24, 2017

anguslees commented Apr 24, 2017

FengyunPan commented Apr 28, 2017 • edited

FengyunPan commented May 5, 2017

dims commented May 5, 2017

daconstenla commented May 6, 2017

daconstenla commented May 7, 2017

daconstenla commented May 11, 2017

anguslees commented May 16, 2017

k8s-github-robot commented May 16, 2017

k8s-ci-robot commented May 21, 2017 • edited

k8s-github-robot commented May 21, 2017

daconstenla commented Dec 19, 2016 •

edited

anguslees commented Dec 20, 2016 •

edited

FengyunPan commented Apr 28, 2017 •

edited

k8s-ci-robot commented May 21, 2017 •

edited