Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve SetLoadBalancerAffinityTimeout not being effective #2647

Merged
merged 1 commit into from Apr 13, 2023

Conversation

jiliangsui
Copy link
Contributor

@jiliangsui jiliangsui commented Apr 13, 2023

What type of this PR

Examples of user facing changes:

  • Bug fixes

Which issue(s) this PR fixes:

Fixes #(2462)

WHAT

馃 Generated by Copilot at 2fd08a6

Update load balancer options with affinity timeout. This change modifies the pkg/ovs/ovn-nb-load_balancer.go file to set and store the affinity timeout option for load balancers in the OVN network backend.

馃 Generated by Copilot at 2fd08a6

options map set
load balancer updated
autumn leaves fall

HOW

馃 Generated by Copilot at 2fd08a6

  • Set the affinity timeout option for load balancers in the OVN network backend (link, F

@github-actions
Copy link
Contributor

  • The commit message is missing. It should provide a clear and concise explanation of the changes made in this patch.
  • There are no potential bugs or format errors in this patch.
  • There are no performance issues in this patch.
  • Instead of updating the lb.Options field directly, it would be better to create a new map and copy all the existing options into it before adding the new "affinity_timeout" option. This ensures that the original lb.Options map is not modified and can be used for other purposes if needed.

@jiliangsui
Copy link
Contributor Author

@gugulee Please review

@gugulee
Copy link
Contributor

gugulee commented Apr 13, 2023

can you write a unit test for this issue?

@github-actions
Copy link
Contributor

  • The commit message is missing. It should provide a brief description of the changes made in the patch.
  • There are no potential bugs or format errors in the code diff.
  • The performance of the code is not affected by the changes made in the patch.
  • In the SetLoadBalancerAffinityTimeout function, the lb.Options field is updated with the new options map, but this is not necessary since the UpdateLoadBalancer function already updates the load balancer object with the new options. Therefore, the line lb.Options = options can be removed.
  • In the testSetLoadBalancerAffinityTimeout function, the test case where the affinity timeout is added repeatedly seems redundant since it is testing the same functionality as the previous test case. This test case can be removed to simplify the test suite.

@zhangzujian zhangzujian added the bug Something isn't working label Apr 13, 2023
@zhangzujian zhangzujian added this to In progress in 2023-4 via automation Apr 13, 2023
@github-actions
Copy link
Contributor

  • The commit message is missing, it should provide a brief description of the changes made in this patch.
  • In the SetLoadBalancerAffinityTimeout function, the lb.Options assignment should be moved before the loop that populates the options map to avoid overwriting the affinity_timeout value.
  • In the testSetLoadBalancerAffinityTimeout function, the test case should cover the scenario where an invalid timeout value is passed as input.
  • In the testSetLoadBalancerAffinityTimeout function, the repeated test case should cover the scenario where the affinity_timeout value is removed from the load balancer options.

@oilbeater oilbeater merged commit 148f1bf into kubeovn:master Apr 13, 2023
52 checks passed
2023-4 automation moved this from In progress to Done Apr 13, 2023
@oilbeater
Copy link
Collaborator

Thanks! @jiliangsui

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants