fix setting LR option always_learn_from_arp_request #5426
Conversation
![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 @zhangzujian, 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 addresses a critical bug in the network configuration for VPC logical routers by refining how the always_learn_from_arp_request option is determined. The changes introduce a more robust evaluation process that considers not only the VPC's external enablement but also specific properties of its associated subnets, ensuring that the router's ARP learning behavior aligns correctly with the network's requirements. This improves the stability and correctness of network operations within the system.
Highlights
- Bug Fix: Logical Router ARP Learning: This PR fixes a bug where the
always_learn_from_arp_requestoption for OVN logical routers (VPC routers) was not being correctly set. Previously, it only considered theVpc.Spec.EnableExternalfield, leading to incorrect ARP learning behavior in certain scenarios. - Enhanced ARP Learning Logic: The logic for determining
always_learn_from_arp_requesthas been enhanced. It now not only checksVpc.Spec.EnableExternalbut also iterates through the VPC's associated subnets. If any subnet has both aVlanassigned andU2OInterconnectionenabled, thealways_learn_from_arp_requestoption will be set totruefor the VPC router, ensuring proper connectivity for these specific subnet configurations. - VPC Controller Update Trigger: The
enqueueUpdateVpcfunction in the VPC controller has been updated to trigger a VPC reconciliation when there are changes toVpc.Status.Subnets. This ensures that modifications to associated subnets (which can now influence the ARP learning option) properly trigger an update to the VPC router's configuration. - Code Refinement and Consistency: The
createVpcRouterfunction's parameterenableExternalhas been renamed tolearnFromARPRequestfor better clarity, reflecting the new, more comprehensive logic. Additionally,sort.Strings(subnets)was added ingetVpcSubnetsto ensure consistent ordering of subnet lists, which is crucial for reliable comparison in the update enqueueing logic.
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 correctly adjusts the logic for setting the always_learn_from_arp_request option on a logical router, now considering subnet properties like VLAN and U2O interconnection in addition to the VPC's external status. The change to sort subnet lists for reliable change detection is also a good improvement. However, I've identified a high-severity issue where the VPC reconciliation is not triggered upon changes to a subnet's relevant properties, which could lead to a stale router configuration. The fix would likely involve modifying the subnet controller to enqueue the VPC for an update.
dosubot
bot
added
the
size:S
Pull Request Test Coverage Report for Build 16047898469Details
💛 - Coveralls |
dosubot
bot
added
bug
dosubot
bot
added
the
size:M
zhangzujian
force-pushed
the
fix-mac-aging
branch
from
25d031e to
9f42422
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request modifies the logic for setting the always_learn_from_arp_request option on logical routers. It includes changes to subnet and VPC controllers to trigger updates based on U2O interconnection and subnet list changes. The code introduces a new variable, learnFromARPRequest, to determine whether to enable the option based on VPC and subnet configurations. The changes also include sorting the subnets.
zhangzujian
force-pushed
the
fix-mac-aging
branch
from
9f42422 to
e62c50f
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
The code changes modify the logic for setting the always_learn_from_arp_request option on logical routers based on the U2OInterconnection status of associated subnets. The changes enhance the accuracy of this setting, ensuring it reflects the actual need for ARP learning in U2O interconnected VLANs.
zhangzujian
force-pushed
the
fix-mac-aging
branch
from
e62c50f to
479cf60
Compare
Signed-off-by: zhangzujian <[email protected]>
zhangzujian
force-pushed
the
fix-mac-aging
branch
from
479cf60 to
9511d2d
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
The code changes modify the logic for setting the always_learn_from_arp_request option on the logical router. The changes ensure that the option is set to true if any subnet in the VPC has both VLAN and U2OInterconnection enabled, even if the VPC's EnableExternal is false. The code also includes a fix to prevent setting U2OInterconnection on a subnet without a VLAN. The variable names could be improved for readability.
| if oldSubnet.Spec.U2OInterconnection != newSubnet.Spec.U2OInterconnection { | ||
| klog.Infof("enqueue update vpc %s triggered by u2o interconnection change of subnet %s", newSubnet.Spec.Vpc, key) | ||
| c.addOrUpdateVpcQueue.Add(newSubnet.Spec.Vpc) |
There was a problem hiding this comment.
Consider using a more descriptive variable name than U2OInterconnection for better readability. A more descriptive name would improve code understanding, especially for those unfamiliar with the codebase.
| if oldSubnet.Spec.U2OInterconnection != newSubnet.Spec.U2OInterconnection { | |
| klog.Infof("enqueue update vpc %s triggered by u2o interconnection change of subnet %s", newSubnet.Spec.Vpc, key) | |
| c.addOrUpdateVpcQueue.Add(newSubnet.Spec.Vpc) | |
| if oldSubnet.Spec.UserToOvnInterconnection != newSubnet.Spec.UserToOvnInterconnection { |
There was a problem hiding this comment.
这个u2o 应该是 underlayToOverlay含义
| if subnet.Spec.Vlan == "" && subnet.Spec.U2OInterconnection { | ||
| subnet.Spec.U2OInterconnection = false | ||
| changed = true |
There was a problem hiding this comment.
Consider using a more descriptive variable name than U2OInterconnection for better readability. A more descriptive name would improve code understanding, especially for those unfamiliar with the codebase.
| if subnet.Spec.Vlan == "" && subnet.Spec.U2OInterconnection { | |
| subnet.Spec.U2OInterconnection = false | |
| changed = true | |
| if subnet.Spec.Vlan == "" && subnet.Spec.UserToOvnInterconnection { |
| @@ -268,7 +269,25 @@ func (c *Controller) handleAddOrUpdateVpc(key string) error { | |||
| return err | |||
| } | |||
|
|
|||
| if err = c.createVpcRouter(key, vpc.Spec.EnableExternal); err != nil { | |||
| learnFromARPRequest := vpc.Spec.EnableExternal | |||
There was a problem hiding this comment.
Consider using a more descriptive variable name than learnFromARPRequest for better readability. A more descriptive name would improve code understanding, especially for those unfamiliar with the codebase.
| learnFromARPRequest := vpc.Spec.EnableExternal | |
| learnFromArp := vpc.Spec.EnableExternal |
| if !learnFromARPRequest { | ||
| for _, subnetName := range vpc.Status.Subnets { | ||
| subnet, err := c.subnetsLister.Get(subnetName) | ||
| if err != nil { | ||
| if k8serrors.IsNotFound(err) { | ||
| continue | ||
| } | ||
| klog.Errorf("failed to get subnet %s for vpc %s: %v", subnetName, key, err) | ||
| return err | ||
| } | ||
| if subnet.Spec.Vlan != "" && subnet.Spec.U2OInterconnection { | ||
| learnFromARPRequest = true | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
This logic iterates through all subnets of a VPC to determine learnFromARPRequest. If a VPC has a large number of subnets, this could become inefficient. Consider optimizing this logic to avoid unnecessary iterations once learnFromARPRequest is set to true.
| if !learnFromARPRequest { | |
| for _, subnetName := range vpc.Status.Subnets { | |
| subnet, err := c.subnetsLister.Get(subnetName) | |
| if err != nil { | |
| if k8serrors.IsNotFound(err) { | |
| continue | |
| } | |
| klog.Errorf("failed to get subnet %s for vpc %s: %v", subnetName, key, err) | |
| return err | |
| } | |
| if subnet.Spec.Vlan != "" && subnet.Spec.U2OInterconnection { | |
| learnFromARPRequest = true | |
| break | |
| } | |
| } | |
| learnFromARPRequest := vpc.Spec.EnableExternal | |
| if !learnFromARPRequest { | |
| for _, subnetName := range vpc.Status.Subnets { | |
| subnet, err := c.subnetsLister.Get(subnetName) | |
| if err != nil { | |
| if k8serrors.IsNotFound(err) { | |
| continue | |
| } | |
| klog.Errorf("failed to get subnet %s for vpc %s: %v", subnetName, key, err) | |
| return err | |
| } | |
| if subnet.Spec.Vlan != "" && subnet.Spec.U2OInterconnection { | |
| learnFromARPRequest = true | |
| break // Exit loop once learnFromARPRequest is true | |
| } | |
| } | |
| } |
| klog.Errorf("failed to get subnet %s for vpc %s: %v", subnetName, key, err) | ||
| return err | ||
| } | ||
| if subnet.Spec.Vlan != "" && subnet.Spec.U2OInterconnection { |
There was a problem hiding this comment.
Consider using a more descriptive variable name than U2OInterconnection for better readability. A more descriptive name would improve code understanding, especially for those unfamiliar with the codebase.
if subnet.Spec.Vlan != "" && subnet.Spec.UserToOvnInterconnection {| return | ||
| } | ||
|
|
||
| // createVpcRouter create router to connect logical switches in vpc | ||
| func (c *Controller) createVpcRouter(lr string, enableExternal bool) error { | ||
| func (c *Controller) createVpcRouter(lr string, learnFromARPRequest bool) error { |
There was a problem hiding this comment.
Consider using a more descriptive variable name than learnFromARPRequest for better readability. A more descriptive name would improve code understanding, especially for those unfamiliar with the codebase.
func (c *Controller) createVpcRouter(lr string, learnFromArp bool) error {| @@ -1251,7 +1271,7 @@ | |||
| "mac_binding_age_threshold": "300", | |||
| "dynamic_neigh_routers": "true", | |||
| } | |||
| if !enableExternal { | |||
| if !learnFromARPRequest { | |||
There was a problem hiding this comment.
Consider using a more descriptive variable name than learnFromARPRequest for better readability. A more descriptive name would improve code understanding, especially for those unfamiliar with the codebase.
if !learnFromArp {
Signed-off-by: zhangzujian <[email protected]>
Signed-off-by: zhangzujian <[email protected]>
Signed-off-by: zhangzujian <[email protected]>
Pull Request
What type of this PR
Examples of user facing changes:
#5318