openstack: Don't Delete LB in Case of Security Group Reconciliation Errors

[multi-io]

What type of PR is this?

/kind bug

What this PR does / why we need it:

This fixes the legacy Openstack cloud provider's lbaas control loop so the EnsureLoadBalancer() function 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 (

kubernetes/staging/src/k8s.io/legacy-cloud-providers/openstack/openstack_loadbalancer.go

Lines 785 to 788 in c7c89f8

	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 (

kubernetes/pkg/controller/service/service_controller.go

Lines 255 to 256 in 3fe7a57

	runtime.HandleError(fmt.Errorf("error processing service %v (will retry): %v", key, err))
	s.queue.AddRateLimited(key)

) 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 new out-of-tree openstack cloud provider; I've submitted a corresponding PR there (kubernetes/cloud-provider-openstack#743). We'd still like to fix this error in-tree as well and also have the fix backported to 1.15 and 1.14 (please?) because our migration to cloud controller manager is still in the early planning stages and will take more time.

Which issue(s) this PR fixes:

Fixes #35056

Release note:

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

[k8s-ci-robot]

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.

[alvaroaleman]

/ok-to-test

[alvaroaleman]

/assign @dims

[bashofmann]

@dims Is there anything that blocks merging this? In the out-of-tree openstack cloud provider the fix has already been merged: kubernetes/cloud-provider-openstack#743

[dims]

@multi-io Ack!

/approve
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, multi-io

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:

• staging/src/k8s.io/legacy-cloud-providers/openstack/OWNERS [dims]

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

[justaugustus]

/kind bug
/priority important-soon