Do not Delete LB in Case of Security Group Reconciliation Errors #743

multi-io · 2019-09-03T10:29:51Z

What this PR does / why we need it:

This fixes the lbaas control loop's EnsureLoadBalancer() function so it no longer deletes the LB if something went wrong when reconciling the LB's security groups. With the current master, if you have an LB service and associated LB already up and running and working fine, and then during a reconcile loop (which shouldn't change anything) e.g. the OpenStack API is down temporarily at the wrong moment (i.e. if it's still up during the LB and listener reconciliation, but then down during the SG reconciliation), then the whole LB will be deleted. We saw this exact thing happen in a real world customer application, which went offline because of if (the LB is recreated shortly after, but likely with a different floating IP).

Deleting the LB in case of errors in a "reconcile" (rather than "create") function seems just wrong, and all the other parts of EnsureLoadBalancer() don't do it either: E.g. if a transient error occurs when creating a listener, we just return it and leave the LB in a half-created state (

cloud-provider-openstack/pkg/cloudprovider/providers/openstack/openstack_loadbalancer.go

Lines 1033 to 1036 in 8f386af

    
           if err != nil { 
        
           	// Unknown error, retry later 
        
           	return nil, fmt.Errorf("error creating LB listener: %v", err) 
        
           }

), and the service controller will catch that error and re-queue the work item (https://github.com/kubernetes/kubernetes/blob/3fe7a570001ae0a6773da34a2eee7246ef416094/pkg/controller/service/service_controller.go#L255-L256) so the LB creation will go through eventually.

This PR just fixes the SG reconciliation to follow the same pattern. It seems to me that the current "delete LB in case an an error" approach was originally not part of a "reconcile" function but of a "create" function, where it would've made more sense.

The same bug is present in the legacy in-tree openstack cloud provider; I've submitted a corresponding PR there (kubernetes/kubernetes#82264).

Which issue this PR fixes

kubernetes/kubernetes#35056

Special notes for your reviewer:

Release note:

Do not delete managed LB in case of security group reconciliation errors

k8s-ci-robot · 2019-09-03T10:29:53Z

Welcome @multi-io!

It looks like this is your first PR to kubernetes/cloud-provider-openstack 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/cloud-provider-openstack has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2019-09-03T10:30:00Z

Hi @multi-io. 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 /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.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

theopenlab-ci · 2019-09-03T10:33:38Z

Build succeeded.

cloud-provider-openstack-format : SUCCESS in 3m 06s

ramineni · 2019-09-03T10:35:55Z

/ok-to-test

theopenlab-ci · 2019-09-03T10:37:38Z

Build succeeded.

cloud-provider-openstack-unittest : SUCCESS in 6m 59s

theopenlab-ci · 2019-09-03T10:42:16Z

Build failed.

cloud-provider-openstack-acceptance-test-lb-octavia : NODE_FAILURE in 0s

theopenlab-ci · 2019-09-03T10:44:16Z

Build succeeded.

cloud-provider-openstack-acceptance-test-k8s-cinder : SUCCESS in 13m 07s

theopenlab-ci · 2019-09-03T10:44:27Z

Build succeeded.

cloud-provider-openstack-acceptance-test-csi-cinder : SUCCESS in 13m 16s

theopenlab-ci · 2019-09-03T10:45:50Z

Build failed.

cloud-provider-openstack-acceptance-test-e2e-conformance : POST_FAILURE in 13m 26s

theopenlab-ci · 2019-09-03T10:47:07Z

Build failed.

cloud-provider-openstack-acceptance-test-e2e-conformance-stable-branch-v1.13 : POST_FAILURE in 13m 58s

theopenlab-ci · 2019-09-03T10:49:22Z

Build failed.

cloud-provider-openstack-acceptance-test-e2e-conformance-stable-branch-v1.14 : POST_FAILURE in 15m 27s

theopenlab-ci · 2019-09-03T10:50:14Z

Build failed.

cloud-provider-openstack-acceptance-test-e2e-conformance-stable-branch-v1.15 : POST_FAILURE in 15m 13s

theopenlab-ci · 2019-09-03T10:52:30Z

Build succeeded.

cloud-provider-openstack-acceptance-test-keystone-authentication-authorization : SUCCESS in 21m 25s

theopenlab-ci · 2019-09-03T10:59:58Z

Build succeeded.

cloud-provider-openstack-acceptance-test-flexvolume-cinder : SUCCESS in 28m 44s

multi-io · 2019-09-03T11:04:18Z

force-pushed commit message typo fix

theopenlab-ci · 2019-09-03T11:04:42Z

Build failed.

cloud-provider-openstack-acceptance-test-lb-octavia : NODE_FAILURE in 0s

theopenlab-ci · 2019-09-03T11:08:20Z

Build succeeded.

cloud-provider-openstack-format : SUCCESS in 3m 21s

theopenlab-ci · 2019-09-03T11:10:37Z

Build succeeded.

cloud-provider-openstack-unittest : SUCCESS in 6m 18s

theopenlab-ci · 2019-09-03T11:16:35Z

Build succeeded.

cloud-provider-openstack-acceptance-test-csi-cinder : SUCCESS in 11m 52s

theopenlab-ci · 2019-09-03T11:21:15Z

Build failed.

cloud-provider-openstack-acceptance-test-e2e-conformance-stable-branch-v1.15 : POST_FAILURE in 13m 36s

theopenlab-ci · 2019-09-03T11:37:19Z

Build succeeded.

cloud-provider-openstack-acceptance-test-keystone-authentication-authorization : SUCCESS in 32m 52s

theopenlab-ci · 2019-09-03T11:41:10Z

Build succeeded.

cloud-provider-openstack-acceptance-test-standalone-cinder : SUCCESS in 36m 41s

theopenlab-ci · 2019-09-03T11:49:20Z

Build succeeded.

cloud-provider-openstack-acceptance-test-flexvolume-cinder : SUCCESS in 43m 20s

theopenlab-ci · 2019-09-03T12:20:16Z

Build failed.

cloud-provider-openstack-acceptance-test-lb-octavia : NODE_FAILURE in 0s

theopenlab-ci · 2019-09-03T12:23:54Z

Build succeeded.

cloud-provider-openstack-format : SUCCESS in 3m 06s

multi-io · 2019-09-03T12:24:42Z

force-pushed better error message. I'll stop pushing now unless asked to do it.

theopenlab-ci · 2019-09-03T12:27:09Z

Build succeeded.

cloud-provider-openstack-unittest : SUCCESS in 7m 48s

theopenlab-ci · 2019-09-03T12:31:05Z

Build succeeded.

cloud-provider-openstack-acceptance-test-k8s-cinder : SUCCESS in 12m 00s

theopenlab-ci · 2019-09-03T12:32:34Z

Build succeeded.

cloud-provider-openstack-acceptance-test-csi-cinder : SUCCESS in 13m 08s

theopenlab-ci · 2019-09-03T12:34:17Z

Build failed.

cloud-provider-openstack-acceptance-test-e2e-conformance-stable-branch-v1.13 : POST_FAILURE in 12m 32s

theopenlab-ci · 2019-09-03T12:35:12Z

Build failed.

cloud-provider-openstack-acceptance-test-e2e-conformance : POST_FAILURE in 14m 56s

theopenlab-ci · 2019-09-03T12:36:22Z

Build failed.

cloud-provider-openstack-acceptance-test-e2e-conformance-stable-branch-v1.15 : POST_FAILURE in 12m 28s

theopenlab-ci · 2019-09-03T12:38:59Z

Build failed.

cloud-provider-openstack-acceptance-test-e2e-conformance-stable-branch-v1.14 : POST_FAILURE in 16m 04s

theopenlab-ci · 2019-09-03T12:49:24Z

Build succeeded.

cloud-provider-openstack-acceptance-test-standalone-cinder : SUCCESS in 30m 00s

theopenlab-ci · 2019-09-03T12:50:23Z

Build succeeded.

cloud-provider-openstack-acceptance-test-flexvolume-cinder : SUCCESS in 30m 53s

theopenlab-ci · 2019-09-03T12:54:47Z

Build succeeded.

cloud-provider-openstack-acceptance-test-keystone-authentication-authorization : SUCCESS in 33m 08s

lingxiankong · 2019-09-03T21:36:56Z

/lgtm

adisky · 2019-09-05T12:57:14Z

/approve
@lingxiankong do you want it to merge manually??

k8s-ci-robot · 2019-09-05T12:57:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adisky

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

lingxiankong · 2019-09-05T21:46:42Z

/approve
@lingxiankong do you want it to merge manually??

Yes please, I don't think the CI job failure is related to this PR.

lingxiankong · 2019-09-05T21:47:00Z

/approve
@lingxiankong do you want it to merge manually??

BTW, is there someone working on fixing the CI?

adisky · 2019-09-06T07:16:15Z

/approve
@lingxiankong do you want it to merge manually??

BTW, is there someone working on fixing the CI?

@lingxiankong for conformance we are working, openlab team is looking on octavia job

…e_lb_on_errors Do not Delete LB in Case of Security Group Reconciliation Errors

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 3, 2019

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 Sep 3, 2019

k8s-ci-robot requested review from anguslees and dims September 3, 2019 10:30

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 3, 2019

multi-io changed the title ~~do not delete LB in case on security group reconciliation errors~~ do not delete LB in case of security group reconciliation errors Sep 3, 2019

multi-io changed the title ~~do not delete LB in case of security group reconciliation errors~~ Do not Delete LB in Case of Security Group Reconciliation Errors Sep 3, 2019

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 3, 2019

multi-io mentioned this pull request Sep 3, 2019

openstack: Don't Delete LB in Case of Security Group Reconciliation Errors kubernetes/kubernetes#82264

Merged

multi-io force-pushed the openstack_dont_delete_lb_on_errors branch from afd44da to 0942580 Compare September 3, 2019 11:03

do not delete LB in case of security group reconciliation errors

f365c4e

multi-io force-pushed the openstack_dont_delete_lb_on_errors branch from 0942580 to f365c4e Compare September 3, 2019 12:18

k8s-ci-robot assigned lingxiankong Sep 3, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 3, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 5, 2019

adisky merged commit a452516 into kubernetes:master Sep 6, 2019

powellchristoph pushed a commit to powellchristoph/cloud-provider-openstack that referenced this pull request Jan 19, 2022

Merge pull request kubernetes#743 from syseleven/openstack_dont_delet…

cc930a1

…e_lb_on_errors Do not Delete LB in Case of Security Group Reconciliation Errors

	if err != nil {
	// Unknown error, retry later
	return nil, fmt.Errorf("error creating LB listener: %v", err)
	}

Do not Delete LB in Case of Security Group Reconciliation Errors #743

Do not Delete LB in Case of Security Group Reconciliation Errors #743

Conversation

multi-io commented Sep 3, 2019 • edited Loading

k8s-ci-robot commented Sep 3, 2019

k8s-ci-robot commented Sep 3, 2019

theopenlab-ci bot commented Sep 3, 2019

ramineni commented Sep 3, 2019

theopenlab-ci bot commented Sep 3, 2019

theopenlab-ci bot commented Sep 3, 2019

theopenlab-ci bot commented Sep 3, 2019

theopenlab-ci bot commented Sep 3, 2019

theopenlab-ci bot commented Sep 3, 2019

theopenlab-ci bot commented Sep 3, 2019

theopenlab-ci bot commented Sep 3, 2019

theopenlab-ci bot commented Sep 3, 2019

theopenlab-ci bot commented Sep 3, 2019

theopenlab-ci bot commented Sep 3, 2019

multi-io commented Sep 3, 2019

theopenlab-ci bot commented Sep 3, 2019

theopenlab-ci bot commented Sep 3, 2019

theopenlab-ci bot commented Sep 3, 2019

theopenlab-ci bot commented Sep 3, 2019

theopenlab-ci bot commented Sep 3, 2019

theopenlab-ci bot commented Sep 3, 2019

theopenlab-ci bot commented Sep 3, 2019

theopenlab-ci bot commented Sep 3, 2019

theopenlab-ci bot commented Sep 3, 2019

theopenlab-ci bot commented Sep 3, 2019

multi-io commented Sep 3, 2019 • edited Loading

theopenlab-ci bot commented Sep 3, 2019

theopenlab-ci bot commented Sep 3, 2019

theopenlab-ci bot commented Sep 3, 2019

theopenlab-ci bot commented Sep 3, 2019

theopenlab-ci bot commented Sep 3, 2019

theopenlab-ci bot commented Sep 3, 2019

theopenlab-ci bot commented Sep 3, 2019

theopenlab-ci bot commented Sep 3, 2019

theopenlab-ci bot commented Sep 3, 2019

theopenlab-ci bot commented Sep 3, 2019

lingxiankong commented Sep 3, 2019

adisky commented Sep 5, 2019

k8s-ci-robot commented Sep 5, 2019

lingxiankong commented Sep 5, 2019

lingxiankong commented Sep 5, 2019

adisky commented Sep 6, 2019

multi-io commented Sep 3, 2019 •

edited

Loading

multi-io commented Sep 3, 2019 •

edited

Loading