Conversation
Signed-off-by: clyi <clyi@alauda.io>
dosubot
bot
added
the
size:S
Summary of ChangesHello @changluyi, 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 and fixes issues related to Kubevirt virtual machine migrations by enhancing the controller's ability to manage network configurations. It introduces more specific handling for logical switch ports, both when a migration is initiated and when it concludes, ensuring that network settings are correctly adjusted for successful and failed migration scenarios. 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 addresses an issue with VM migration failures by correctly handling the reset of OVN logical switch port options. It introduces separate logic for successful and failed migrations and adds logic to set migration options when a VM pod migration is detected. My review includes suggestions to refactor the code for better readability and maintainability by reducing code duplication and flattening nested conditional statements, and also to correct a stale error message.
| if isVMPod { | ||
| if _, ok := pod.Labels["kubevirt.io/migrationJobUID"]; ok { | ||
| if sourceNode, ok := pod.Labels["kubevirt.io/nodeName"]; ok && sourceNode != pod.Spec.NodeName { | ||
| klog.Infof("VM pod %s/%s is migrating from %s to %s", | ||
| pod.Namespace, pod.Name, sourceNode, pod.Spec.NodeName) | ||
| if err := c.OVNNbClient.SetLogicalSwitchPortMigrateOptions(portName, sourceNode, pod.Spec.NodeName); err != nil { | ||
| klog.Errorf("failed to set migrate options for VM pod lsp %s: %v", portName, err) | ||
| return nil, err | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The nested if statements can be flattened to improve readability.
| if isVMPod { | |
| if _, ok := pod.Labels["kubevirt.io/migrationJobUID"]; ok { | |
| if sourceNode, ok := pod.Labels["kubevirt.io/nodeName"]; ok && sourceNode != pod.Spec.NodeName { | |
| klog.Infof("VM pod %s/%s is migrating from %s to %s", | |
| pod.Namespace, pod.Name, sourceNode, pod.Spec.NodeName) | |
| if err := c.OVNNbClient.SetLogicalSwitchPortMigrateOptions(portName, sourceNode, pod.Spec.NodeName); err != nil { | |
| klog.Errorf("failed to set migrate options for VM pod lsp %s: %v", portName, err) | |
| return nil, err | |
| } | |
| } | |
| } | |
| } | |
| if isVMPod { | |
| _, isMigrating := pod.Labels["kubevirt.io/migrationJobUID"] | |
| sourceNode, hasSourceNode := pod.Labels["kubevirt.io/nodeName"] | |
| if isMigrating && hasSourceNode && sourceNode != pod.Spec.NodeName { | |
| klog.Infof("VM pod %s/%s is migrating from %s to %s", | |
| pod.Namespace, pod.Name, sourceNode, pod.Spec.NodeName) | |
| if err := c.OVNNbClient.SetLogicalSwitchPortMigrateOptions(portName, sourceNode, pod.Spec.NodeName); err != nil { | |
| klog.Errorf("failed to set migrate options for VM pod lsp %s: %v", portName, err) | |
| return nil, err | |
| } | |
| } | |
| } |
Pull Request Test Coverage Report for Build 19155271956Warning: 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 |
Signed-off-by: clyi <clyi@alauda.io>
* fix migrate failed Signed-off-by: clyi <clyi@alauda.io> --------- Signed-off-by: clyi <clyi@alauda.io>
This commit fixes KubeVirt VM migration failures (Issue kubeovn#6220) by: 1. Remove early return on MigrationState.Completed check that could skip cleanup logic for MigrationSucceeded/MigrationFailed phases 2. Add MigrationUID check to ensure we use state from current migration, not stale state from previous migration (from PR kubeovn#6239) 3. Change from ResetLogicalSwitchPortMigrateOptions to CleanLogicalSwitchPortMigrateOptions for migration cleanup: - Reset: keeps requested-chassis with new value, causing stale state - Clean: fully removes requested-chassis and activation-strategy The root cause was PR kubeovn#5873 changing from Clean to Reset, which left stale requested-chassis options that caused subsequent migrations to fail with 'ovs interface not ready after 30s' errors. Fixes: kubeovn#6220 Signed-off-by: Kube-OVN Developer <developer@kubeovn.io> Signed-off-by: zbb88888 <jmdxjsjgcxy@gmail.com>
4 participants
Pull Request
What type of this PR
Examples of user facing changes:
Which issue(s) this PR fixes
Fixes #(issue-number)