check for pending, not running, state in next build start tests #16924
Conversation
openshift-ci-robot
added
the
size/S
openshift-merge-robot
added
the
approved
|
/retest |
|
/retest |
gabemontero
left a comment
gabemontero
left a comment
There was a problem hiding this comment.
A couple of questions (admittedly I did not go back and review / refresh my memory on exactly how builds transition from new to pending)
| sawCompletion = true | ||
| } | ||
| if build.Status.Phase == buildapi.BuildPhaseRunning { | ||
| if build.Status.Phase == buildapi.BuildPhaseRunning || build.Status.Phase == buildapi.BuildPhasePending { |
There was a problem hiding this comment.
do we need to consider builds in new state? or can we safely assume they will always be transitioned in a timely enough fashion to pending
There was a problem hiding this comment.
the point of the test is to create a bunch of builds in the new state, then ensure that one at a time they enter pending(or running), with the next one entering pending(or running) when the previous one completes.
considering new would break the test.
There was a problem hiding this comment.
the point of the change here is that it might take a while for a pod to enter running, which screws up the confirmatoin this test does to say "did this pod enter running sooner after the last build finished".
So instead of needing the pod to enter running, it's sufficient to see that the build entered pending (Which means we created the pod for the build, which we won't do until the previous build completes). This will be less vulnerable to problem w/ scheduling and container creation which might prevent a pod from entering a running state, despite the build controller doing the right thing.
| select { | ||
| case event := <-buildWatch.ResultChan(): | ||
| build := event.Object.(*buildapi.Build) | ||
| if build.Status.Phase == buildapi.BuildPhasePending { |
There was a problem hiding this comment.
same new phase question here
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bparees, csrwng The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
/retest |
test/extended/builds/run_policy.go
Outdated
| exutil "github.com/openshift/origin/test/extended/util" | ||
| ) | ||
|
|
||
| // this test is very latency sensitive so run it by itself (serially). |
openshift-merge-robot
removed
the
lgtm
bparees
added
kind/bug
openshift-merge-robot
removed
the
lgtm
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
Automatic merge from submit-queue. |
|
@bparees: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions 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. |
7 participants
No description provided.