-
Notifications
You must be signed in to change notification settings - Fork 494
OVN-Interconnect: Only set-up LRP/LSP between the transit switches and default router if we want to. #5834
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
default router if we want to. This allows people to create transit switches and only have non-default routers connected to them Signed-off-by: Andrew Lee <[email protected]>
Summary of ChangesHello @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! This pull request enhances the OVN-Interconnect functionality by introducing a new configuration parameter, 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new configuration option interconnect-default-router to conditionally set up the Logical Router Port (LRP) and Logical Switch Port (LSP) between transit switches and the default router. This change allows for more flexible network topologies where transit switches might not need to be connected to the default router. The implementation is straightforward, adding a check for this new configuration flag before creating the ports. My review includes a suggestion to simplify the boolean flag parsing for better code conciseness.
| var interconnectDefaultRouter = true | ||
| val, ok := config["interconnect-default-router"] | ||
| if ok && val == "false" { | ||
| interconnectDefaultRouter = false | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic for parsing the interconnect-default-router boolean flag can be simplified. Instead of initializing the variable and then conditionally changing it, you can determine its value in a single, more idiomatic line. This improves readability and conciseness.
| var interconnectDefaultRouter = true | |
| val, ok := config["interconnect-default-router"] | |
| if ok && val == "false" { | |
| interconnectDefaultRouter = false | |
| } | |
| interconnectDefaultRouter := config["interconnect-default-router"] != "false" |
|
Hi all - this is small feature change I need for my work prototyping having OVN interconnect with kube-ovn - where I would like to control the interconnect config (I want transit switches per VPC and I don't want the transit switches connected to the ovn-default VPC) |
|
@andrewlee1089 I think if it's for prototyping, we can manually verify this scenario through commands first. This change isn’t a complete solution yet—it still requires many other manual commands to achieve the goal, so it’s not suitable for merging now. |
OVN-Interconnect: Only set-up LRP/LSP between the transit switches and default router if we want to.
This allows people to create transit switches and only have non-default routers connected to them
Pull Request
What type of this PR
Examples of user facing changes:
Which issue(s) this PR fixes
Fixes #(issue-number)