feat(endpoint_slice): refactor and support arbitrary providers#5414
feat(endpoint_slice): refactor and support arbitrary providers#5414zhangzujian merged 3 commits intokubeovn:masterfrom
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 @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 introduces a significant refactoring of how EndpointSlices are processed, primarily focusing on the detection of the target VPC and subnet for services. The changes aim to make this detection logic more straightforward and extensible by supporting arbitrary network providers, moving beyond a hardcoded 'ovn' assumption. It also enhances the configuration capabilities for headless services by allowing SwitchLBRule to propagate network annotations.
Highlights
- Refactored EndpointSlice VPC/Subnet Detection: The core logic for determining the target VPC and subnet for a service's EndpointSlices has been significantly refactored. The new approach prioritizes explicit
LogicalRouterAnnotationandLogicalSwitchAnnotationon the service itself. If not present, it then infers the VPC/subnet by inspecting the network context of the associated pods via their EndpointSlice entries, rather than relying on a direct listing of service-selected pods. - Support for Arbitrary Network Providers: The system now dynamically identifies and supports different network providers beyond the default 'ovn'. New helper functions (
getPodProviders,getEndpointProvider,getSubnetByProvider,getVpcByProvider,getEndpointVpcAndSubnet) have been introduced to abstract and centralize the process of extracting provider-specific VPC and subnet information from pod annotations, making the endpoint handling more extensible. - Enhanced Headless Service Configuration: Headless services generated by
SwitchLBRulecan now inherit explicit VPC and subnet annotations (LogicalRouterAnnotationandLogicalSwitchAnnotation) directly from theSwitchLBRuleobject. This provides more granular control over the network placement of these generated services.
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 refactors the endpoint slice handling to support arbitrary providers and simplifies the logic for determining the VPC and subnet for a service. There are a few areas for improvement, including a critical fix for a potential panic and some medium-severity issues related to error handling.
Pull Request Test Coverage Report for Build 16322093478Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
SkalaNetworks
force-pushed
the
improve-ep
branch
5 times, most recently
from
3add592 to
7abc4b7
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant refactoring to how endpoint slices are handled, making the detection of VPC/subnet for services more robust and supporting arbitrary providers. The changes are well-structured, introducing new helper functions to encapsulate the logic for retrieving provider-specific network information from pods. The addition of setUserDefinedNetwork and its corresponding tests is a good step towards allowing user-defined network configurations for services. The primary concern identified is a potential logical error in the default assignment of VPC and subnet names, which should be addressed for correctness.
SkalaNetworks
force-pushed
the
improve-ep
branch
3 times, most recently
from
0ef9f61 to
8f9cb75
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the logic for handling endpoint slices to support arbitrary providers and improve clarity. The changes primarily affect pkg/controller/endpoint_slice.go, breaking down large functions into smaller, more focused ones. This improves modularity and makes the code easier to understand. The PR also propagates user-defined network settings from SwitchLBRule to the generated Service.
Signed-off-by: SkalaNetworks <contact@skala.network>
dosubot
bot
added
size:L
|
This should help for people using custom providers. Especially useful on multi-attached VMs that have many network interfaces of type Kube-OVN. I'm gonna add healthchecks on custom endpoints next (right now, only selectors are supported). |
|
@zhangzujian @oilbeater I've prepared a PR to add healthchecks on static endpoints #5435 I'll wait on this one to be merged to get it out of draft as it depends on code present here. |
|
Could you please add an e2e test case for this feature? |
will do |
SkalaNetworks
force-pushed
the
improve-ep
branch
from
6cdf1dd to
6e5491d
Compare
Signed-off-by: SkalaNetworks <contact@skala.network>
SkalaNetworks
force-pushed
the
improve-ep
branch
from
6e5491d to
2f4469c
Compare
|
|
||
| if customProvider { | ||
| nad := framework.MakeOVNNetworkAttachmentDefinition(subnetName, f.Namespace.Namespace, provider, []request.Route{}) | ||
| nadClient.Create(nad) |
There was a problem hiding this comment.
This test case requires Multus-CNI. We can move it to test/e2e/multus/e2e_test.go.
There was a problem hiding this comment.
@zhangzujian Considering it's gonna run pretty much exactly the same tests as the standard SLRs that don't use Multus, how do you envision it? I'd like to minimize code duplication.
Maybe we can force Multus installations for SLR tests?
There was a problem hiding this comment.
We can check whether the CRD exists and skip this test case if not found.
Also, we can add a ginkgo label such as multus-cni that indicates this case requires Multus-CNI.
There was a problem hiding this comment.
I implemented an if to check if Multus is installed. I'll see for the label.
…stalled Signed-off-by: SkalaNetworks <contact@skala.network>
dosubot
bot
added
size:XL
3 participants
Pull Request
What type of this PR
Examples of user facing changes:
This PR refactors the way endpoint slices are handled. It makes the logic of detecting the VPC/subnet targetted by a service more straightforward and supports custom providers instead of just the "ovn" one.
Support for specifying "endpoints" instead of selectors is also improved, with custom providers added to them. The refactor will make the implementation of healthchecks for them easier.
Note that this PR doesn't fix the obvious issue where a service targets pods that are in different VPC/subnets. It also only handles the first network of every Pod. For this, we'd need more checks and to use EndpointSlice mirrors.