Randomizing and repeating functional tests to detect flakyness #53105
Conversation
openshift-ci
bot
added
the
needs-ok-to-test
|
Hi @jpodivin. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
|
/pj-rehearse |
|
@jpodivin: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@jpodivin: needs-ok-to-test label found, no rehearsals will be run |
|
/ok-to-test |
openshift-ci
bot
added
ok-to-test
|
/pj-rehearse |
|
@lewisdenny: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse ack |
|
@lewisdenny: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
openshift-ci-robot
added
the
rehearsals-ack
| @@ -64,7 +64,7 @@ tests: | |||
| mkdir -p ../operator && cp -r . ../operator | |||
| cd ../operator | |||
| export GOFLAGS= | |||
| make test GINKGO_ARGS='--no-color' | |||
| make test GINKGO_ARGS='--no-color --repeat=5 --randomize-all' | |||
There was a problem hiding this comment.
i woudl not add --repeat=5 in ci
if you want to support repeate for local execution i would add a new ENV var and default it to 1
--randomize-all i do agree with
There was a problem hiding this comment.
Running a single randomized test wouldn't give us as much benefit. Purpose of this PR is to prevent merging of flaky code, which can slip by thanks to a "lucky" sequence of test cases. If run multiple random tests, the probability of something like this happening drops considerably.
Although for obvious reasons I can't say by how much since I don't know how many "lucky" combinations are there. But with successive tests the probability of only hitting "lucky" combinations drops pretty fast to zero.
There was a problem hiding this comment.
-1(to clarify a few things): I think that value hardcoded to 5 is not a good default (unless you have good reasons to think that 5 is the right default).
I agree with @SeanMooney and use a ENV var that we can override in the future without requiring a new change here (and we can test diff combinations with a DNM change in the operator itself)
Also, are you going to propose this for the service operators or this change applies to openstack-operator only? Also, considering that dataplane operator has been merged into openstack operator, shouldn't just remove and cleanup this part?
There was a problem hiding this comment.
Adding var to makefile can help, but it would force repeated runs locally as well. That would quickly become annoying and possibly lead to overriding of that var. I want to avoid that by setting it here. Where it will only affect CI and nothing else.
Furthermore, setting repetitions to 1, as @SeanMooney proposed won't do us any good, unless we get very lucky.
As for dataplane operator merger, yes, that is something I want to do, the PR has been here since before the operators were merged.
There was a problem hiding this comment.
ack, and I agree 1 is definitely invalidating this patch.
If the rest of the team is ok to have 5 as default, then the remaining work here is to cleanup the dataplane-operator part.
There was a problem hiding this comment.
Turned the setting into variable in openstack-operator repo. This means that the PR now has a dependency it has to pull in order to execute. But it's the only way to satisfy conditions laid out.
The leftover from dataplane repo was removded some time ago
| @@ -64,7 +64,7 @@ tests: | |||
| mkdir -p ../operator && cp -r . ../operator | |||
| cd ../operator | |||
| export GOFLAGS= | |||
| make test GINKGO_ARGS='--no-color' | |||
| make test GINKGO_ARGS='--no-color --repeat=5 --randomize-all' | |||
There was a problem hiding this comment.
-1(to clarify a few things): I think that value hardcoded to 5 is not a good default (unless you have good reasons to think that 5 is the right default).
I agree with @SeanMooney and use a ENV var that we can override in the future without requiring a new change here (and we can test diff combinations with a DNM change in the operator itself)
Also, are you going to propose this for the service operators or this change applies to openstack-operator only? Also, considering that dataplane operator has been merged into openstack operator, shouldn't just remove and cleanup this part?
openshift-ci-robot
removed
the
rehearsals-ack
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jpodivin The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This patch changes target executed by the CI to one using flags for repetition and randomization. Signed-off-by: Jiri Podivin <[email protected]>
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
lgtm |
|
Issues in openshift/release go stale after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
Stale issue in openshift/release rot after 15d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
openshift-ci
bot
added
lifecycle/rotten
|
@jpodivin: The following tests failed, say
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-sigs/prow repository. I understand the commands that are listed here. |
|
Rotten issues in openshift/release close after 15d of inactivity. Reopen the issue by commenting /close |
|
@openshift-bot: Closed this PR. 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-sigs/prow repository. |
Depends-On: openstack-k8s-operators/openstack-operator#931