fix(slr): deleting old entries from ipmapping to avoid clogging loadbalancer #5380
Conversation
dosubot
bot
added
the
size:S
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @SkalaNetworks, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request implements a crucial fix to prevent the accumulation of stale IP-to-port mappings within OVN NorthBound Load Balancer entries. By introducing a mechanism to explicitly delete outdated mappings when pods are removed, it ensures the load balancer's configuration remains clean and efficient, resolving a potential 'clogging' issue.
Highlights
- Load Balancer IP Mapping Cleanup: Introduced logic within the
LoadBalancerUpdateIPPortMappingfunction to identify and remove stale IP-to-port mappings from OVN NorthBound Load Balancer entries. This addresses an issue where mappings for deleted pods were not being removed. - Preventing Load Balancer Clogging: Implemented an explicit deletion mechanism for outdated mappings, ensuring that the
ip_port_mappingfield in OVN Load Balancers remains clean and does not accumulate stale data, thereby preventing potential 'clogging' or inefficiency.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request aims to fix an issue where outdated IPPortMappings are not removed from OVN LoadBalancers, potentially leading to clogging. The core change introduces logic to identify and delete stale mappings during an update. While the deletion mechanism itself is sound, a significant concern is that this new logic is currently conditional and will not execute if the set of new mappings is empty (e.g., when all service endpoints are removed). This would prevent the cleanup in a critical scenario, undermining the PR's primary goal. A minor improvement to a log message is also suggested.
Pull Request Test Coverage Report for Build 15767155797Details
💛 - Coveralls |
…alancer Signed-off-by: SkalaNetworks <[email protected]>
SkalaNetworks
force-pushed
the
slr-ipmapping
branch
from
0d5973f to
37e530d
Compare
dosubot
bot
added
size:M
|
/gemini review |
There was a problem hiding this comment.
Code Review
The changes in this PR correctly address the issue of stale ip_port_mapping entries in OVN Load Balancers by ensuring that outdated mappings are deleted before new ones are inserted. The logic is sound and makes the LoadBalancerUpdateIPPortMapping function more robust. I've included a minor suggestion to improve error message clarity and a point for consideration regarding potential micro-optimization of mutation generation.
| return []model.Mutation{ | ||
| { | ||
| Field: &lb.IPPortMappings, | ||
| Value: mappingToDelete, | ||
| Mutator: ovsdb.MutateOperationDelete, | ||
| }, | ||
| { | ||
| Field: &lb.IPPortMappings, | ||
| Value: ipPortMappings, | ||
| Mutator: ovsdb.MutateOperationInsert, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Consider optimizing by only adding mutations if they cause an actual change. If mappingToDelete is empty and ipPortMappings is effectively the same as lb.IPPortMappings, you could return nil from the mutationsFunc.
There was a problem hiding this comment.
I don't think this matters too much, there's other similiar constructs elsewhere, they don't really do much if values are empty
…alancer (#5380) Signed-off-by: SkalaNetworks <[email protected]>
…alancer (#5380) Signed-off-by: SkalaNetworks <[email protected]> Signed-off-by: zhangzujian <[email protected]>
SLRs create Load_Balancer entries on the NorthBound table of OVN.
These LBs have an "ip_port_mapping" field to map backend IPs to a LSP. This is used to determine which LSP is up and which backend is up.
The mapping is refreshed each time a new pod pops up in the endpoint slice, but it isn't updated when a pod is deleted.
This fix introduces a way for IPMapping to be updated correctly by removing any "old" mapping that shouldn't be present anymore