-
Notifications
You must be signed in to change notification settings - Fork 114
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
e2e: Fix Debug logging should be visible in multus pod
flake
#611
e2e: Fix Debug logging should be visible in multus pod
flake
#611
Conversation
Thanks for your PR,
To skip the vendors CIs use one of:
|
Pull Request Test Coverage Report for Build 7753914139
💛 - Coveralls |
Eventually(func() error { | ||
netAttDef := &netattdefv1.NetworkAttachmentDefinition{} | ||
return clients.Get(context.Background(), runtimeclient.ObjectKey{Name: sriovNetworkName, Namespace: ns1}, netAttDef) | ||
}, (30+snoTimeoutMultiplier*90)*time.Second, 1*time.Second).ShouldNot(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.
two comments:
- can we call waitForNetAttachDef(...) in L841 as well ?
- we now wait 10 seconds and not 30 when muliplier is 0 , if it doesnt fail i guess that is ok.
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.
two comments:
- can we call waitForNetAttachDef(...) in L841 as well ?
sure! didn't see it
- we now wait 10 seconds and not 30 when muliplier is 0 , if it doesnt fail i guess that is ok.
I found no reason for having 30s instead of 10s:
- bed21a0#diff-8841b252b9d2e29927af1036582b38ac8466067019d67dd5a59f22285674310dR507
- 0cfd998#diff-8841b252b9d2e29927af1036582b38ac8466067019d67dd5a59f22285674310dR642
I'd go with this and see if it flakes.
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.
LGTM
Once the SriovNetwork has been created, test routine must wait for NetAttachDefinition to be created by the operator before spawing pods. Create a function `waitForNetAttachDef(...)` and use it in similar situations. The flake error this commit is supposed to fix is: ``` • [FAILED] [2.695 seconds] [sriov] operator Generic SriovNetworkNodePolicy CNI Logging level [It] Debug logging should be visible in multus pod /root/opr-k8s-1/data/sriov-network-operator/sriov-network-operator/test/conformance/tests/test_sriov_operator.go:1077 [FAILED] Unexpected error: <*errors.StatusError | 0xc000fd8640>: admission webhook "network-resources-injector-config.k8s.io" denied the request: could not find network attachment definition 'sriov-conformance-testing/test-log-level-debug-no-file': could not get Network Attachment Definition sriov-conformance-testing/test-log-level-debug-no-file: the server could not find the requested resource { ErrStatus: { TypeMeta: {Kind: "", APIVersion: ""}, ListMeta: { SelfLink: "", ResourceVersion: "", Continue: "", RemainingItemCount: nil, }, Status: "Failure", Message: "admission webhook \"network-resources-injector-config.k8s.io\" denied the request: could not find network attachment definition 'sriov-conformance-testing/test-log-level-debug-no-file': could not get Network Attachment Definition sriov-conformance-testing/test-log-level-debug-no-file: the server could not find the requested resource", Reason: "", Details: nil, Code: 400, }, } occurred In [It] at: /root/opr-k8s-1/data/sriov-network-operator/sriov-network-operator/test/conformance/tests/test_sriov_operator.go:2505 @ 02/01/24 12:44:25.702 ``` Signed-off-by: Andrea Panattoni <[email protected]>
a93061e
to
20c08f5
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
Once the SriovNetwork has been created, the test routine must wait for NetAttachDefinition to be created by the operator before spawning pods. Create a function
waitForNetAttachDef(...)
and use it in similar situations.The flake error this commit is supposed to fix is: