ensure next build is kicked off when a build completes #13670
Conversation
bparees
changed the title
bparees
force-pushed
the
failed_builds
branch
2 times, most recently
from
8f97e7a to
4bdc449
Compare
bparees
changed the title
|
[test] |
|
[testextended][extended:core(builds)] |
|
@openshift/devex ptal |
bparees
force-pushed
the
failed_builds
branch
2 times, most recently
from
56a8621 to
25fe8b6
Compare
| @@ -121,6 +121,9 @@ func (bc *BuildPodController) HandlePod(pod *kapi.Pod) error { | |||
| nextStatus := build.Status.Phase | |||
| currentReason := build.Status.Reason | |||
|
|
|||
| // if the build is marked failed, the build status reason has already been | |||
| // set (probably by the build pod itself), so don't do any updating here | |||
| // or we'll overwrite the correct value. | |||
There was a problem hiding this comment.
Should we have utility functions that delineate which non-successful BuildPhases are set by the build pod (where it is currently just Failed) and which are set outside of the build pod (i.e. BuildPhaseError), in case those two lists change over time?
There was a problem hiding this comment.
I don't anticipate that set changing. If it does, that would be a good time to introduce a utility function but I don't think it needs to be done here/now.
| // the current state changed. Do not touch builds that were marked failed | ||
| // because we'd potentially be overwriting build state information set by the | ||
| // build pod directly. | ||
| if (!common.HasBuildPodNameAnnotation(build) || build.Status.Phase != nextStatus) && !buildutil.IsBuildComplete(build) { |
There was a problem hiding this comment.
We should split this condition and have a separate
if !common.HasBuildPodNameAnnotation(build) {
needsUpdate = true
common.SetBuildPodNameAnnotation(build, pod.Name)
}
| // there's no reason to re-update the build and rerun | ||
| // HandleBuildCompletion if we've already done it for this | ||
| // build previously. | ||
| buildWasComplete := build.Status.CompletionTimestamp != nil |
There was a problem hiding this comment.
just a nit ... maybe a better name for the variable would be completionTimestampIsSet ? or something shorter
| } | ||
| if !buildWasComplete && buildutil.IsBuildComplete(build) { |
There was a problem hiding this comment.
Add a comment explaining why we do this here
bparees
force-pushed
the
failed_builds
branch
2 times, most recently
from
eb0b01e to
1e37067
Compare
| } | ||
| // if the build was not previously marked complete but it's complete now, | ||
| // handle completion for it. otherwise ignore it because we've already | ||
| // handle its completion previously. |
|
LGTM |
|
Evaluated for origin testextended up to bb80432 |
|
continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended/133/) (Base Commit: b4b7083) (Extended Tests: core(builds)) |
|
[test]
…On Fri, Apr 7, 2017 at 3:13 PM, OpenShift Bot ***@***.***> wrote:
continuous-integration/openshift-jenkins/test FAILURE (
https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/644/)
(Base Commit: b4b7083
<b4b7083>
)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#13670 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEvl3idOgkWTUGWM-q3r-p0GhMRYkjosks5rtordgaJpZM4M223E>
.
--
Ben Parees | OpenShift
|
|
Evaluated for origin test up to bb80432 |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/653/) (Base Commit: 1ea122b) |
|
[merge] |
|
@openshift-bot, the last build failed from the following flakes:
re[merge] |
|
Evaluated for origin merge up to bb80432 |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/294/) (Base Commit: 201ba3f) (Image: devenv-rhel7_6128) |
|
thanks @stevekuznetsov |
fixes bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=1440147 (we weren't immediately starting the next build when a build failed, nor setting the completion timestamp on the failed build)
https://bugzilla.redhat.com/show_bug.cgi?id=1436395 (we weren't setting the start time on a build if it went from new->complete extremely quickly, but we were setting the completion time)