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
kubevirt: Ignore completed virt-launcher pods #3990
kubevirt: Ignore completed virt-launcher pods #3990
Conversation
04a5f25
to
d1e5820
Compare
b78d756
to
6740e48
Compare
6740e48
to
e2609f4
Compare
if !IsPodLiveMigratable(pod) { | ||
return false, nil | ||
return true, nil |
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.
This is counter-intuitive. Why don't you do the IsPodLiveMigratable
check first, and return false if the pod is not migratable.
go-controller/pkg/kubevirt/pod.go
Outdated
} | ||
vmPods, err := findVMRelatedPods(client, pod) | ||
if err != nil { | ||
return false, fmt.Errorf("failed finding related pods for pod %s/%s when checking live migration left overs: %v", pod.Namespace, pod.Name, err) | ||
return true, fmt.Errorf("failed finding related pods for pod %s/%s when checking live migration left overs: %v", pod.Namespace, pod.Name, err) |
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.
Why? Does it matter if we are returning an error? false
is the better convention here so it is weird that you had to change this value.
e2609f4
to
043b146
Compare
go-controller/pkg/kubevirt/pod.go
Outdated
@@ -123,11 +123,14 @@ func EnsurePodAnnotationForVM(watchFactory *factory.WatchFactory, kube *kube.Kub | |||
return podAnnotation, nil | |||
} | |||
|
|||
// IsMigratedSourcePodStale return true if there are other pods related to | |||
// to it and any of them has newer creation timestamp. | |||
// IsMigratedSourcePodStale return false is the pod is live migratable and not completed |
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.
IsMigratedSourcePodStale return false if the pod is live migratable, not completed and is the running VM pod with newest creation timestamp
043b146
to
ce91a2c
Compare
5d03941
to
bbe0491
Compare
When a live migration fails at completed virt-launcher pod with newer creation timestamp is lingering in the system so the virt-launcher that is really running the VM is hidden by it and routes are wrongly reconstructed. This change take into account if the pod is completed to ignore it marking it as stale. Signed-off-by: Enrique Llorente <ellorent@redhat.com>
bbe0491
to
4f9b4c2
Compare
Fix included in accepted release 4.15.0-0.nightly-2023-11-16-214550 |
- What this PR does and why is it needed
When a live migration fails at completed virt-launcher pod with newer
creation timestamp is lingering in the system so the virt-launcher that
is really running the VM is hidden by it and routes are wrongly
reconstructed. This change take into account if the pod is completed to
ignore it marking it as stale.
Closes https://issues.redhat.com/browse/OCPBUGS-22767
- How to verify it
It includes a unit test that emulate the result of failing migration, completed virt-launcher pod with newer creation timestamp.
- Description for the changelog
kubevirt: Ignore completed virt-launcher pods