-
Notifications
You must be signed in to change notification settings - Fork 333
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: Propertly remove migration leftovers #4370
kubevirt: Propertly remove migration leftovers #4370
Conversation
71d5668
to
5993aa9
Compare
1ae2e96
to
16efdab
Compare
pod, err := fakeOvn.fakeClient.KubeClient.CoreV1().Pods(t.namespace).Get(context.TODO(), t.podName, metav1.GetOptions{}) | ||
Expect(err).NotTo(HaveOccurred()) | ||
pod.Status.Phase = corev1.PodSucceeded | ||
_, err = fakeOvn.fakeClient.KubeClient.CoreV1().Pods(t.namespace).UpdateStatus(context.TODO(), pod, metav1.UpdateOptions{}) |
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.
nit. does UpdateStatus
expect to pass at first attempt itself ? or do we need multiple attempts with eventually
here ?
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 unit testing, fakeClient has no conflicts mechanism, this is good enough.
Expect(err).NotTo(HaveOccurred()) | ||
pod.Status.Phase = corev1.PodSucceeded | ||
_, err = fakeOvn.fakeClient.KubeClient.CoreV1().Pods(t.namespace).UpdateStatus(context.TODO(), pod, metav1.UpdateOptions{}) | ||
Expect(err).NotTo(HaveOccurred()) |
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.
same here.
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.
ditto
LGTM |
Failure not related |
@qinqon can you add doc text to |
9cbaca9
to
5cae8fc
Compare
Unit test failure not related
|
After merging [1] the live migration leftovers were not removed since all the VM's pods are at completed state so the cleanup was skip. This change simplify that logic by removing left overs if all the VM's pods are completed. [1] 469c4e8 Signed-off-by: Enrique Llorente <[email protected]>
5cae8fc
to
2713fc8
Compare
What this PR does and why is it needed
After merging 469c4e8 the live migration leftovers were not removed since all the VM's pods are at completed state so the cleanup was skip.
This change simplify that logic by removing left overs if all the VM's pods are completed.