[octavia-lb] Add the logic to reattach the VIP to the load balancer if the IP has changed #2451

yang-wang11 · 2023-10-24T09:13:11Z

What this PR does / why we need it:

Which issue this PR fixes(if applicable):
fixes #2443

Special notes for reviewers:

Release note:

Enable OCCM to reassign the new Floating IP from service.spec when spec.loadBalancerIP is changed.

k8s-ci-robot · 2023-10-24T09:13:20Z

Welcome @yang-wang11!

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 · 2023-10-24T09:13:22Z

Hi @yang-wang11. 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.

yang-wang11 · 2023-10-24T09:14:00Z

/test all

k8s-ci-robot · 2023-10-24T09:14:14Z

@yang-wang11: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test all

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.

dulek · 2023-10-24T11:46:10Z

/ok-to-test

dulek

I think this enables users of the cluster to set spec.loadBalancerIP="", wait for a new FIP to get attached and repeat the process until all FIPs are exhausted. We shouldn't allow that. At minimum old FIP has to be deleted if it was created by the CPO.

dulek · 2023-10-24T17:02:17Z

Seems like this also breaks shared LBs:

E1024 12:12:45.460335      11 controller.go:298] error processing service octavia-lb-test/test-shared-2 (retrying with exponential backoff): failed to ensure load balancer: floating IP  is not available
I1024 12:12:45.460700      11 event.go:376] "Event occurred" object="octavia-lb-test/test-shared-2" fieldPath="" kind="Service" apiVersion="v1" type="Warning" reason="SyncLoadBalancerFailed" message="Error syncing load balancer: failed to ensure load balancer: floating IP  is not available"

Not that I like the feature, but we cannot just break it.

yang-wang11 · 2023-10-26T07:46:16Z

I think this enables users of the cluster to set spec.loadBalancerIP="", wait for a new FIP to get attached and repeat the process until all FIPs are exhausted. We shouldn't allow that. At minimum old FIP has to be deleted if it was created by the CPO.

@dulek Great observation, I will take some time to refactor this function to lessen the if-else.

yang-wang11 · 2023-10-26T12:28:54Z

/retest

pkg/openstack/loadbalancer.go

yang-wang11 · 2023-10-30T05:38:50Z

/assign @yang-wang11

k8s-ci-robot · 2023-10-30T07:13:21Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign zetaab for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

pkg/openstack/loadbalancer.go

docs/openstack-cloud-controller-manager/expose-applications-using-loadbalancer-type-service.md

yang-wang11 · 2023-10-31T10:20:32Z

/test all

yang-wang11 · 2023-11-02T12:12:28Z

/retest

yang-wang11 · 2023-11-06T09:03:00Z

@kayrus I'm not trying to rush you, but kindly look forward to your code review.

kayrus

@yang-wang11 apologies that you had to wait. See my comments. As for the new ensureFloatingIP logic, give me a bit more time.

kayrus · 2023-11-16T15:11:56Z

pkg/openstack/loadbalancer.go

@@ -74,6 +74,7 @@ const (
 	ServiceAnnotationLoadBalancerFloatingSubnetTags   = "loadbalancer.openstack.org/floating-subnet-tags"
 	ServiceAnnotationLoadBalancerClass                = "loadbalancer.openstack.org/class"
 	ServiceAnnotationLoadBalancerKeepFloatingIP       = "loadbalancer.openstack.org/keep-floatingip"
+	ServiceAnnotationLoadBalancerFloatingIP           = "loadbalancer.openstack.org/floating-ip"


in order to comply with the previous naming, I'd suggest to use loadbalancer.openstack.org/floatingip
in future for dual stack we can use loadbalancer.openstack.org/floatingip-v6

I guess you mean loadbalancer.openstack.org/floatingip-v4 as the proposal. That's good for me.

kayrus · 2023-11-16T15:12:52Z

pkg/openstack/loadbalancer.go

@@ -431,6 +432,19 @@ func getSecurityGroupName(service *corev1.Service) string {
 	return securityGroupName
 }

+func getLoadBalancerIP(service *corev1.Service) string {
+	if annotations := service.Annotations; annotations != nil {


we can skip this check. if val, ok := annotations[ServiceAnnotationLoadBalancerFloatingIP]; ok && val != "" { is enough.

Yes, it is used to avoid panic if there is no annotation. adopt your suggestion, it can cover my concern.
https://go.dev/play/p/RDerITo3dqp?v=goprev

kayrus · 2023-11-16T16:11:58Z

@yang-wang11 Unfortunately I have doubts about the proposed ensureFloatingIP logic. I'm afraid the reconciliation port of the code is too fragile, and it requires a wider refactoring.

An example:

cloud-provider-openstack/pkg/openstack/loadbalancer.go

Lines 965 to 968 in d6f294d

    
           if floatIP == nil && !isLBOwner { 
        
           	return "", fmt.Errorf("cannot attach a floating IP to a load balancer for a shared Service %s/%s, only owner Service can do that", 
        
           		service.Namespace, service.Name) 
        
           }

why would we fail in this case? What if the loadbalancer has a service.beta.kubernetes.io/openstack-internal-load-balancer flag? There is no indication that this loadbalancer is going to be updated, therefore there is no reason to return an error.

There are also a bunch of minor concerns, which combination in result raises other questions.

Basically this is the reason why the #2377 PR is still in draft.

IMO the first step that we need to perform is to extend the mock API with Octavia support and add test cases which include a collection of different annotation combinations for services.

@yang-wang11 how critical is the issue for you? Does it occur only on svc update only?

yang-wang11 · 2023-11-17T04:02:28Z

@yang-wang11 Unfortunately I have doubts about the proposed ensureFloatingIP logic. I'm afraid the reconciliation port of the code is too fragile, and it requires a wider refactoring.

An example:

cloud-provider-openstack/pkg/openstack/loadbalancer.go

Lines 965 to 968 in d6f294d

if floatIP == nil && !isLBOwner {

return "", fmt.Errorf("cannot attach a floating IP to a load balancer for a shared Service %s/%s, only owner Service can do that",

service.Namespace, service.Name)

}

why would we fail in this case? What if the loadbalancer has a service.beta.kubernetes.io/openstack-internal-load-balancer flag? There is no indication that this loadbalancer is going to be updated, therefore there is no reason to return an error.

There are also a bunch of minor concerns, which combination in result raises other questions.

Basically this is the reason why the #2377 PR is still in draft.

IMO the first step that we need to perform is to extend the mock API with Octavia support and add test cases which include a collection of different annotation combinations for services.

@yang-wang11 how critical is the issue for you? Does it occur only on svc update only?

@kayrus
I'm unsure if this change belongs to a refactor or not, the thing I did is combine similar logic. It aims to eliminate the if-else logic.
As we discussed in the Slack channel, the issue that occurred from the cluster was the update case, but not sure if there are others. we hope we can fix this issue at the earliest convenience.

k8s-ci-robot · 2023-12-01T18:38:58Z

PR needs rebase.

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.

kayrus · 2023-12-11T12:03:29Z

@yang-wang11 can you resolve the conflicts?

kayrus · 2023-12-11T12:50:50Z

docs/openstack-cloud-controller-manager/expose-applications-using-loadbalancer-type-service.md

+
+  This annotation is utilized to specify the floating IP for the load balancer. 
+
+  If set, both newly created and pre-existing load balancers will utilize it. Furthermore, its precedence is higher than that of `Spec.LoadBalanccerIP`.


Suggested change

If set, both newly created and pre-existing load balancers will utilize it. Furthermore, its precedence is higher than that of `Spec.LoadBalanccerIP`.

If set, both newly created and pre-existing load balancers will utilize it. Furthermore, its precedence is higher than that of `Spec.LoadBalancerIP`.

k8s-triage-robot · 2024-03-10T13:10:57Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2024-04-09T13:17:03Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot · 2024-05-09T14:10:30Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen
Mark this PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2024-05-09T14:10:35Z

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen

Mark this PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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-sigs/prow repository.

add the logic for re-attache VIP to LB if the IP changed

75a2a07

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 24, 2023

k8s-ci-robot requested review from anguslees and FengyunPan2 October 24, 2023 09:13

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 Oct 24, 2023

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 24, 2023

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 Oct 24, 2023

dulek requested changes Oct 24, 2023

View reviewed changes

refactor with readability

802f91e

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 26, 2023

make it simpler

7798f4f

yang-wang11 requested a review from dulek October 26, 2023 12:29

yang-wang11 commented Oct 26, 2023

View reviewed changes

pkg/openstack/loadbalancer.go Show resolved Hide resolved

remove duplicate logic

5881280

jichenjc mentioned this pull request Oct 30, 2023

[occm] loadbalancer doesn't honor the spec.loadBalancerIP on service update #2443

Open

k8s-ci-robot assigned yang-wang11 Oct 30, 2023

add the annotation way

8887cf6

jeffyjf suggested changes Oct 31, 2023

View reviewed changes

pkg/openstack/loadbalancer.go Outdated Show resolved Hide resolved

pkg/openstack/loadbalancer.go Show resolved Hide resolved

docs/openstack-cloud-controller-manager/expose-applications-using-loadbalancer-type-service.md Show resolved Hide resolved

fix issue and update md file

9fbc6de

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 3, 2023

Merge branch 'master' into issue_2443

d6f294d

yang-wang11 requested a review from jeffyjf November 6, 2023 08:58

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 7, 2023

kayrus reviewed Nov 16, 2023

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 1, 2023

kayrus reviewed Dec 11, 2023

View reviewed changes

kayrus mentioned this pull request Dec 11, 2023

[occm] VIP of the LB can be specified during creation #2494

Closed

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 10, 2024

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Apr 9, 2024

k8s-ci-robot closed this May 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[octavia-lb] Add the logic to reattach the VIP to the load balancer if the IP has changed #2451

[octavia-lb] Add the logic to reattach the VIP to the load balancer if the IP has changed #2451

yang-wang11 commented Oct 24, 2023

k8s-ci-robot commented Oct 24, 2023

k8s-ci-robot commented Oct 24, 2023

yang-wang11 commented Oct 24, 2023

k8s-ci-robot commented Oct 24, 2023

dulek commented Oct 24, 2023

dulek left a comment •

edited

dulek commented Oct 24, 2023

yang-wang11 commented Oct 26, 2023

yang-wang11 commented Oct 26, 2023

yang-wang11 commented Oct 30, 2023

k8s-ci-robot commented Oct 30, 2023

yang-wang11 commented Oct 31, 2023

yang-wang11 commented Nov 2, 2023

yang-wang11 commented Nov 6, 2023

kayrus left a comment

kayrus Nov 16, 2023

yang-wang11 Nov 22, 2023

kayrus Nov 16, 2023

yang-wang11 Nov 17, 2023

kayrus commented Nov 16, 2023 •

edited

yang-wang11 commented Nov 17, 2023

k8s-ci-robot commented Dec 1, 2023

kayrus commented Dec 11, 2023

kayrus Dec 11, 2023

k8s-triage-robot commented Mar 10, 2024

k8s-triage-robot commented Apr 9, 2024

k8s-triage-robot commented May 9, 2024

k8s-ci-robot commented May 9, 2024


		This annotation is utilized to specify the floating IP for the load balancer.

		If set, both newly created and pre-existing load balancers will utilize it. Furthermore, its precedence is higher than that of `Spec.LoadBalanccerIP`.

[octavia-lb] Add the logic to reattach the VIP to the load balancer if the IP has changed #2451

[octavia-lb] Add the logic to reattach the VIP to the load balancer if the IP has changed #2451

Conversation

yang-wang11 commented Oct 24, 2023

k8s-ci-robot commented Oct 24, 2023

k8s-ci-robot commented Oct 24, 2023

yang-wang11 commented Oct 24, 2023

k8s-ci-robot commented Oct 24, 2023

dulek commented Oct 24, 2023

dulek left a comment • edited

Choose a reason for hiding this comment

dulek commented Oct 24, 2023

yang-wang11 commented Oct 26, 2023

yang-wang11 commented Oct 26, 2023

yang-wang11 commented Oct 30, 2023

k8s-ci-robot commented Oct 30, 2023

yang-wang11 commented Oct 31, 2023

yang-wang11 commented Nov 2, 2023

yang-wang11 commented Nov 6, 2023

kayrus left a comment

Choose a reason for hiding this comment

kayrus Nov 16, 2023

Choose a reason for hiding this comment

yang-wang11 Nov 22, 2023

Choose a reason for hiding this comment

kayrus Nov 16, 2023

Choose a reason for hiding this comment

yang-wang11 Nov 17, 2023

Choose a reason for hiding this comment

kayrus commented Nov 16, 2023 • edited

yang-wang11 commented Nov 17, 2023

k8s-ci-robot commented Dec 1, 2023

kayrus commented Dec 11, 2023

kayrus Dec 11, 2023

Choose a reason for hiding this comment

k8s-triage-robot commented Mar 10, 2024

k8s-triage-robot commented Apr 9, 2024

k8s-triage-robot commented May 9, 2024

k8s-ci-robot commented May 9, 2024

dulek left a comment •

edited

kayrus commented Nov 16, 2023 •

edited