-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Don't wait for TaskRun to be observed Running. #4773
Conversation
Ok, I also just saw this hit on the
|
As explained in detail in the associated issue, there are multiple problems with expecting the test logic to observe the `TaskRun`s to be `Running`, including them never actually existing in that state in etcd if the cluster is busy and the controller gets backed up, or is sufficiently weak. Since observing the `TaskRun`s `Running` isn't material to testing timeouts, this removes those and simply checks that the `TaskRun`s reach that ultimate state. These same issues apply to the `PipelineRun` as well, and I also just observed that flaking, so hopefully this addresses all of them! Fixes: tektoncd#4772
c168e5b
to
ba18d8f
Compare
t.Logf("Waiting for PipelineRun %s in namespace %s to be timed out", pipelineRun.Name, namespace) | ||
if err := WaitForPipelineRunState(ctx, c, pipelineRun.Name, timeout, FailedWithReason(v1beta1.PipelineRunReasonTimedOut.String(), pipelineRun.Name), "PipelineRunTimedOut"); err != nil { | ||
t.Errorf("Error waiting for PipelineRun %s to finish: %s", pipelineRun.Name, err) |
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.
I moved these checks up from below, so they execute before the taskrunList below is determined, lest we hit another race creating the taskruns 😅
🤞 this fixes these once and for all 🤞
/test pull-tekton-pipeline-go-coverage TLS handshake 🙃 |
/lgtm Death to flakes! |
/lgtm |
/test pull-tekton-pipeline-integration-tests Prow seems to have lost these 🤔 |
/test pull-tekton-pipeline-integration-tests TLS timeouts |
/test pull-tekton-pipeline-integration-tests sidecar test 🙃 |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vdemeester 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 |
As explained in detail in the associated issue, there are multiple problems with expecting the test logic to observe the
TaskRun
s to beRunning
, including them never actually existing in that state in etcd if the cluster is busy and the controller gets backed up, or is sufficiently weak.Since observing the
TaskRun
sRunning
isn't material to testing timeouts, this removes those and simply checks that theTaskRun
s reach that ultimate state.I could have sworn I fixed something like this before, but couldn't find it.
Fixes: #4772
/kind bug
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
(if there are no user facing changes, use release note "NONE")
Release Notes