-
Notifications
You must be signed in to change notification settings - Fork 254
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
Fix multiple clone functional test #2704
Conversation
/retest-required |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mhenriks The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
The test was failing due to a type mismatch in the comparison (int vs int32). The destructive lane is failing due to a cdi pod not comping back up in time. Added some extra logging to determine why Signed-off-by: Alexander Wels <awels@redhat.com>
d49986a
to
1e41a4f
Compare
New changes are detected. LGTM label has been removed. |
@awels: The following test failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
@@ -1021,7 +1021,7 @@ var _ = Describe("all clone tests", func() { | |||
Expect(err).ToNot(HaveOccurred()) | |||
restartCount := cloner.Status.ContainerStatuses[0].RestartCount | |||
fmt.Fprintf(GinkgoWriter, "INFO: restart count on clone source pod %s: %d\n", clonerPodName, restartCount) | |||
Expect(restartCount).To(BeNumerically("<", 2)) | |||
Expect(restartCount).To(BeNumerically("<", int32(2))) |
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.
if you look at the failure https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/kubevirt_containerized-data-importer/2701/pull-containerized-data-importer-e2e-ceph/1651557222200643584
the issue here is that 2 is too high for this test to pass
Expected : 2 to be < : 2
We have a PR pending that disables this check (explanation in comment):
#2647 (comment)
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 exactly failure I was seeing is that the type didn't match int
vs int32
so I just cast it.
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 matcher should play along with that, the full output looks like:
{ Failure /root/go/src/kubevirt.io/containerized-data-importer/tests/cloner_test.go:965
Expected
<int32>: 2
to be <
<int>: 2
/root/go/src/kubevirt.io/containerized-data-importer/tests/cloner_test.go:1024}
return len(cdiPods.Items) == len(newCdiPods.Items) | ||
fmt.Fprintf(GinkgoWriter, "number of cdi pods: %d\n new number of cdi pods: %d\n", len(cdiPods.Items), len(newCdiPods.Items)) | ||
for _, pod := range newCdiPods.Items { | ||
fmt.Fprintf(GinkgoWriter, "pod %s/%s\n", pod.Namespace, pod.Name) |
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.
awesome!
pv.Spec.ClaimRef.ResourceVersion = "" | ||
pv.Spec.ClaimRef.UID = "" | ||
_, err = f.K8sClient.CoreV1().PersistentVolumes().Update(context.TODO(), pv, metav1.UpdateOptions{}) | ||
Expect(err).ToNot(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.
Expecting inside an eventually block may fail the test earlier than we want to
to get around this you can do this:
containerized-data-importer/tests/operator_test.go
Lines 99 to 106 in e6d2286
Eventually(func(g Gomega) { | |
cdiCrd, err = f.ExtClient.ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), "cdis.cdi.kubevirt.io", metav1.GetOptions{}) | |
g.Expect(err).ToNot(HaveOccurred()) | |
for _, ver := range cdiCrd.Spec.Versions { | |
g.Expect(ver.Name).Should(Equal("v1beta1")) | |
g.Expect(ver.Storage).Should(BeTrue()) | |
} | |
}, 1*time.Minute, 2*time.Second).Should(Succeed()) |
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.
So I didn't change that particular thing, I just cleaned up the return value a bit so it is easier to see what actually failed. This expect if it return false it is because the update failed. Which should fail the test right there.
/retest-required |
@awels: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
What this PR does / why we need it:
The test was failing due to a type mismatch in the comparison (int vs int32).
The destructive lane is failing due to a cdi pod not comping back up in time. Added some extra logging to determine why
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
Release note: