Deleting FIP sets EIP to ready #5141
Conversation
dosubot
bot
added
the
size:XS
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @andrewlee1089, 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!
Summary of Changes
This pull request appears to address an issue where deleting a Floating IP (FIP) should set the corresponding Elastic IP (EIP) to a 'ready' state. The change involves modifying the patchOvnEipStatus function call within the handleResetOvnEip function in pkg/controller/ovn_eip.go. The original code was setting the status to true, but this PR changes it to false.
Highlights
- Bug Fix: The PR fixes a bug where deleting a FIP was not correctly setting the corresponding EIP to a ready state.
- Status Update: The
patchOvnEipStatusfunction is now called withfalseinstead oftruewhen handling the reset of an OVN EIP.
Changelog
- pkg/controller/ovn_eip.go
- Modified the
handleResetOvnEipfunction to callpatchOvnEipStatuswithfalseto properly reflect the EIP's status after FIP deletion.
- Modified the
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.
A FIP departs, an EIP sighs,
Its status changed before our eyes.
From true to false it must descend,
A ready state, a peaceful end.
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
The pull request modifies the patchOvnEipStatus function to set the OvnEip status to 'not ready' when handling a reset operation. This change seems related to ensuring that EIPs are properly reset when their configuration changes or when they are being deleted. The change itself is small, but it's important to ensure it aligns with the overall logic and intended behavior of the system.
Merge Readiness
The code change appears straightforward and addresses a specific scenario related to resetting OvnEip statuses. However, it's crucial to verify that setting the status to 'false' in this context doesn't inadvertently affect other parts of the system that rely on the EIP status. Given the potential impact, thorough testing and understanding of the broader implications are necessary before merging. I am unable to approve this pull request, and recommend that others review and approve this code before merging. Since there are no critical or high severity issues, the pull request could be merged after the medium severity issue is addressed.
pkg/controller/ovn_eip.go
Outdated
| @@ -193,7 +193,7 @@ func (c *Controller) handleResetOvnEip(key string) error { | |||
| return nil | |||
| } | |||
| klog.Infof("handle reset ovn eip %s", cachedEip.Name) | |||
| if err := c.patchOvnEipStatus(key, true); err != nil { | |||
| if err := c.patchOvnEipStatus(key, false); err != nil { | |||
There was a problem hiding this comment.
Should there be a comment here explaining why we are setting the status to false? It's not immediately obvious why this is the correct behavior during a reset. Consider adding a comment to clarify the intent, especially since the previous value was true.
Style Guide References
| if err := c.patchOvnEipStatus(key, false); err != nil { | |
| // Set the EIP status to 'not ready' during reset to ensure proper cleanup and reconfiguration. | |
| if err := c.patchOvnEipStatus(key, false); err != nil { |
Pull Request Test Coverage Report for Build 14466432184Details
💛 - Coveralls |
andrewlee1089
force-pushed
the
andrewlee1089/5114/deleting-fip-sets-eip-to-ready
branch
from
44b1162 to
9c691a3
Compare
|
I'm not entirely sure of the implications of this PR - hence why this is in draft state. Maybe instead handleResetOvnEip should not try and change the status of the EIP. Please ignore this PR for now |
andrewlee1089
force-pushed
the
andrewlee1089/5114/deleting-fip-sets-eip-to-ready
branch
from
9c691a3 to
7bc126c
Compare
dosubot
bot
added
size:S
andrewlee1089
force-pushed
the
andrewlee1089/5114/deleting-fip-sets-eip-to-ready
branch
2 times, most recently
from
0da053e to
5a54169
Compare
|
maybe you are right
|
Fixes kubeovn#5114 - Previously when deleting a FIP, the function `handleResetOvnEip` was called. - `handleResetOvnEip` called `patchOvnEipStatus` to set the "Ready" field to true, regardless of the current EIP. - This fixes it so when `handleResetOvnEip` is called, the "Ready" is field of the EIP is unchanged Signed-off-by: Andrew Lee <[email protected]>
andrewlee1089
force-pushed
the
andrewlee1089/5114/deleting-fip-sets-eip-to-ready
branch
from
5a54169 to
f2699d7
Compare
|
@zbb88888 I can't get the pipeline to pass (I think it is failing for an unrelated reason) but I think this PR makes sense now. Please can you review? |
andrewlee1089
changed the title
| @@ -364,17 +364,19 @@ func (c *Controller) createOrUpdateOvnEipCR(key, subnet, v4ip, v6ip, mac, usageT | |||
| return nil | |||
| } | |||
|
|
|||
| func (c *Controller) patchOvnEipStatus(key string, ready bool) error { | |||
| func (c *Controller) patchOvnEipStatus(key string, ready, changeReady bool) error { | |||
There was a problem hiding this comment.
how about removing ready, changeReady and eip status ready, keep the ovn-eip just as IP crd？ keep it simple
Pull Request
What type of this PR
handleResetOvnEipwas called.handleResetOvnEipcalledpatchOvnEipStatusto set the "Ready"field to true, regardless of the current EIP.
handleResetOvnEipis called, the "Ready" isfield of the EIP is unchanged
I have rejected a more ambitious fix where all calls of the
handleResetOvnEipfunction set the EIP status to not be ready.Which issue(s) this PR fixes
Fixes #5114