Set Kubevirt live-migrations multi-chassis options for all VM nics#6241
Conversation
Signed-off-by: Jules Skrill <julesskrill@gmail.com>
dosubot
bot
added
size:L
Summary of ChangesHello @jskrill, 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 significantly enhances the robustness of Kubevirt live migrations by extending network optimizations to all Kube-OVN managed network interfaces attached to a Virtual Machine Instance. Previously, these optimizations were limited to the primary interface, which could lead to sporadic migration failures and prevent near-zero downtime for VMs utilizing multiple network cards. The changes ensure that all relevant network ports are correctly configured during migration, thereby improving the reliability and efficiency of live migrations for complex VM network setups. 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 extends the Kubevirt live migration optimization to support all Kube-OVN managed NICs on a Virtual Machine, not just the primary one. This is a valuable feature that improves the reliability of live migrations for multi-homed VMs. The implementation correctly identifies all relevant logical switch ports associated with a VMI by inspecting the VMI spec and annotations. The changes to apply migration options to all collected ports are also correct. I have one suggestion to improve the code's robustness by handling potential duplicate port names.
Signed-off-by: Jules Skrill <julesskrill@gmail.com>
jskrill
force-pushed
the
add-chassis-options-all-kubevirt-nics
branch
from
31f2266 to
95d4d26
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request extends the Kubevirt live migration optimization to support multiple network interfaces for a single Virtual Machine. However, it introduces a critical security vulnerability by failing to verify port ownership. The controller collects port names based on user-controlled annotations and specs and applies migration options to any matching port found in the OVN database, without checking if the port belongs to the VMI being migrated. This could allow an attacker to disrupt the network connectivity of other users' VMIs. Additionally, the current approach for collecting VMI's logical switch ports is inefficient and can be optimized by using a more targeted query.
Signed-off-by: Jules Skrill <julesskrill@gmail.com>
dosubot
bot
added
size:M
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly extends the Kubevirt live migration optimization to support all VM NICs, not just the primary one. This is achieved by fetching all logical switch ports associated with a Virtual Machine Instance and applying the migration options to each. The implementation is sound and addresses the issue of migration failures for multi-NIC VMs. I have one suggestion to refactor a small piece of duplicated code to improve maintainability.
|
Gemini issues resolved (or not in scope), this should be good to review. |
Pull Request Test Coverage Report for Build 21552442512Details
💛 - Coveralls |
|
Thanks! @jskrill |
…6241) * feat: Set multi-chassis options for all VM nics Signed-off-by: Jules Skrill <julesskrill@gmail.com> * fix: Use set to collect port names Signed-off-by: Jules Skrill <julesskrill@gmail.com> * fix: Simplify logic to discovery all lsp ports Signed-off-by: Jules Skrill <julesskrill@gmail.com> --------- Signed-off-by: Jules Skrill <julesskrill@gmail.com>
…ubeovn#6241) * feat: Set multi-chassis options for all VM nics Signed-off-by: Jules Skrill <julesskrill@gmail.com> * fix: Use set to collect port names Signed-off-by: Jules Skrill <julesskrill@gmail.com> * fix: Simplify logic to discovery all lsp ports Signed-off-by: Jules Skrill <julesskrill@gmail.com> --------- Signed-off-by: Jules Skrill <julesskrill@gmail.com>
4 participants
Pull Request
What type of this PR
Currently the Kubevirt live migration optimizations only are applied to the primary interface lsp. This PR extends that logic to apply to all attached networks that are managed by Kube-OVN.
Without this change, VMs with multiple NICs can fail live migration (this seemed to happen sporadically in my test env) and near-zero downtime migrations are not possible in those configurations.
Which issue(s) this PR fixes
N/A