TRT-1452: Separate excessive Back-off restarting tests via namespace #28543
Conversation
|
@DennisPeriquet: This pull request references TRT-1452 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
bot
added
the
approved
DennisPeriquet
force-pushed
the
trt-1452-namespaced-backoffs
branch
from
2a8bb56
to
a4f1e13
Compare
|
/test e2e-agnostic-ovn-cmd |
| // Find the junit with the namespace of openshift-etcd-operator int the testname | ||
| var testJunits []*junitapi.JUnitTestCase | ||
| for _, j := range junits { | ||
| if strings.Contains(j.Name, "openshift-etcd-operator") { |
There was a problem hiding this comment.
Probably better to pass in the namespace you want to find as part of the tests struct in case there is a want / need to test for other namespaces later on
There was a problem hiding this comment.
that makes sense. thx
| }) | ||
| // Add a success for flakes | ||
| if len(result.failures) == 0 && len(result.flakes) > 0 { | ||
| if len(result.failures) == 0 && len(result.flakes) == 0 { |
There was a problem hiding this comment.
Curious about this change, generateFailureOutput looks like it returns an empty string if no flakes or failures, did you see failures recorded when there weren't any?
There was a problem hiding this comment.
yes, it returns a "" which means that when we create the JUnitTestCase in https://github.com/openshift/origin/blob/a4f1e13bbff4eb91494779f68ad5e0414449eb5b/pkg/monitortestlibrary/pathologicaleventlibrary/duplicated_events.go#L148C1-L153C7, we end up with a FailureOutput which results in a failed JUnitTestCase as noted in my first set of test runs including this job. This is why I have the line at 145 to create a success JUnitTestCase where the FailureOutput will be nil. This way, the test will show as a success.
DennisPeriquet
force-pushed
the
trt-1452-namespaced-backoffs
branch
from
a4f1e13
to
002374f
Compare
|
/test verify |
|
/test e2e-gcp-ovn-rt-upgrade |
| } | ||
| } | ||
|
|
||
| for ns, result := range nsResults { |
There was a problem hiding this comment.
Wondering if you can do it without double processing the loop, initializing all namespaces then removing the empty ones
If you had a function like below could you call
nsResults[namespace] = addEventResult(nsResults[namespace], fmt.Sprintf("event [%s] happened %d times", msg, times), true)
etc.
func addEventResult(nsResult *eventResult{}, msg string, flake bool) *eventResult {
if nsResult == nil {
nsResult = &eventResult{}
}
if flake {
nsResult.flakes = append(nsResult.flakes, msg)
} else {
nsResult.failures = append(nsResult.failures, msg)
}
return nsResult
}
There was a problem hiding this comment.
That snippet won't work as is, you would probably have to pass in the map and the namespace and then do the check in the function. Just doesn't feel right initializing it all and then removing the empty ones..
if _, ok := nsResults[namespace]; !ok {
tmp := &eventResult{}
nsResults[namespace] = tmp
}
There was a problem hiding this comment.
wonder if this patch would do it
There was a problem hiding this comment.
Just doesn't feel right initializing it all and then removing the empty ones.
Good instinct.
I pushed a commit that will track only namespaces with failures or flakes so that we only traverse the loop once.
DennisPeriquet
force-pushed
the
trt-1452-namespaced-backoffs
branch
from
419c36b
to
44bf785
Compare
|
/test e2e-gcp-ovn-upgrade |
| // Find the junit with the namespace of openshift-etcd-operator int the testname | ||
| var testJunits []*junitapi.JUnitTestCase | ||
| for _, j := range junits { | ||
| if strings.Contains(j.Name, namespace) { |
There was a problem hiding this comment.
Here what I was thinking was the test would have namespace: "openshift-etcd-operator" and the logic would be
if strings.Contains(j.Name, tt.namespace)
Then if the test was updated later on to test another namespace it would handle it.
But no need to hold this up for that, if / when there is a need it is easy enough to refactor.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: DennisPeriquet, neisw 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 |
|
/test e2e-gcp-ovn-rt-upgrade |
|
/test e2e-aws-ovn-single-node |
|
/label acknowledge-critical-fixes |
|
@DennisPeriquet: The label(s) In response to this:
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. |
|
/label acknowledge-critical-fixes-only |
openshift-ci
bot
added
the
acknowledge-critical-fixes-only
|
FYI: I'm not saying this is a critical fix; I'm only trying to add enough label so this PR can get merged. This PR predates the enactment of the |
|
/retest-required |
|
@DennisPeriquet: all tests passed! Full PR test history. Your PR dashboard. 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. |
openshift-merge-bot
bot
merged commit f533902
into
openshift:master
|
[ART PR BUILD NOTIFIER] This PR has been included in build openshift-enterprise-tests-container-v4.16.0-202402011041.p0.gf533902.assembly.stream for distgit openshift-enterprise-tests. |
This introduces
NamespacedTest()which can be used where theTest()method is used if we want to split out more pathological tests by namespace.To test, we need this test to fail on multiple namespaces: