add support for provider network vlan interfaces#5949
add support for provider network vlan interfaces#5949oilbeater merged 2 commits intokubeovn:masterfrom
Conversation
dosubot
bot
added
the
size:XL
Summary of ChangesHello @abasitt, 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 enhances Kube-OVN's provider network capabilities by adding robust support for VLAN interfaces. It allows users to either explicitly define VLAN interfaces to be managed by the provider network or enable automatic detection and preservation of existing VLAN configurations. This ensures seamless integration of physical VLAN setups with the Kube-OVN overlay, maintaining IP addresses and routing when interfaces are moved to OVS bridges. Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for provider network VLAN interfaces, allowing for both automatic detection and explicit configuration of VLAN sub-interfaces. The changes include updates to the ProviderNetwork CRD, new fields in the ProviderNetworkSpec struct, and comprehensive logic in the daemon to manage the lifecycle of these VLAN interfaces within the OVS bridge. New utility functions have been added to detect, extract, and manage VLAN interface properties, along with corresponding unit tests. The implementation appears robust and addresses the feature request effectively.
| baseLink, err := netlink.LinkByName(baseInterfaceName) | ||
| if err != nil { | ||
| klog.Warningf("Base interface %s not found, cannot recreate kernel VLAN %s", baseInterfaceName, kernelVlanName) | ||
| return nil |
There was a problem hiding this comment.
In removeProviderVlanInterface, if netlink.LinkByName(baseInterfaceName) fails, the function logs a warning and returns nil. While this might be acceptable for transient issues, it could lead to a situation where addresses and routes are not properly restored if the base interface is genuinely missing or renamed. Consider propagating this error or implementing a retry mechanism to ensure the restoration process is as robust as possible, especially during cleanup operations where state consistency is crucial.
| parts := strings.Split(interfaceName, ".") | ||
| if len(parts) != 2 { | ||
| return 0, fmt.Errorf("invalid VLAN interface name format: %s (expected format: interface.vlanid)", interfaceName) | ||
| } |
There was a problem hiding this comment.
The ExtractVlanIDFromInterface function allows interface names like .10 to be parsed successfully, extracting 10 as the VLAN ID. However, the CRD pattern ^[a-zA-Z0-9_-]+\.[0-9]{1,4}$ for vlanInterfaces explicitly requires at least one character before the dot. While the CRD validation should prevent such inputs, it's generally good practice for the parsing logic to align with the stricter validation rules defined in the CRD, or to have a clear reason for being more lenient. This is a minor consistency point.
| interfaceName: ".10", | ||
| expectedID: 10, | ||
| expectError: false, | ||
| }, |
There was a problem hiding this comment.
The test case interface name starting with dot confirms that ExtractVlanIDFromInterface successfully parses .10 as a valid VLAN interface name. As noted in pkg/util/vlan_interfaces.go, this is more permissive than the CRD pattern, which requires at least one character before the dot. It would be beneficial to either align the function's parsing logic with the CRD's stricter pattern or document why this leniency is necessary.
Pull Request Test Coverage Report for Build 20424107908Details
💛 - Coveralls |
|
@abasitt can you run make lint and sign off the commit? I will try to fix the conflict issue later. |
abasitt
force-pushed
the
pn_vlansubinterfaces
branch
from
0c14243 to
b90ef2a
Compare
|
@oilbeater done, thank you. |
Signed-off-by: abasitt <abdul.basit@rakuten.com>
oilbeater
force-pushed
the
pn_vlansubinterfaces
branch
from
f6ffb2f to
690c22a
Compare
Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com>
oilbeater
force-pushed
the
pn_vlansubinterfaces
branch
from
690c22a to
7b1dd05
Compare
|
Thanks! @abasitt |
3 participants
Pull Request
What type of this PR
Which issue(s) this PR fixes
Fixes #5756