Skip to content

Commit 4bdc449

Browse files
bpareesbparees
committed
ensure next build is kicked off when a build completes
https://bugzilla.redhat.com/show_bug.cgi?id=1440147 https://bugzilla.redhat.com/show_bug.cgi?id=1436395
1 parent 4b16943 commit 4bdc449

File tree

6 files changed

+176
-31
lines changed

6 files changed

+176
-31
lines changed

‎pkg/build/controller/buildpod/controller.go

Lines changed: 25 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,9 @@ func (bc *BuildPodController) HandlePod(pod *kapi.Pod) error {
121121
nextStatus := build.Status.Phase
122122
currentReason := build.Status.Reason
123123

124+
// if the build is marked failed, the build status reason has already been
125+
// set (probably by the build pod itself), so don't do any updating here
126+
// or we'll overwrite the correct value.
124127
if build.Status.Phase != buildapi.BuildPhaseFailed {
125128
switch pod.Status.Phase {
126129
case kapi.PodPending:
@@ -176,33 +179,45 @@ func (bc *BuildPodController) HandlePod(pod *kapi.Pod) error {
176179
build.Status.Message = ""
177180
}
178181
}
182+
183+
needsUpdate := false
179184
// Update the build object when it progress to a next state or the reason for
180-
// the current state changed.
181-
if (!common.HasBuildPodNameAnnotation(build) || build.Status.Phase != nextStatus || build.Status.Phase == buildapi.BuildPhaseFailed) && !buildutil.IsBuildComplete(build) {
185+
// the current state changed. Do not touch builds that were marked failed
186+
// because we'd potentially be overwriting build state information set by the
187+
// build pod directly.
188+
if (!common.HasBuildPodNameAnnotation(build) || build.Status.Phase != nextStatus) && build.Status.Phase != buildapi.BuildPhaseFailed {
189+
needsUpdate = true
182190
common.SetBuildPodNameAnnotation(build, pod.Name)
183191
reason := ""
184192
if len(build.Status.Reason) > 0 {
185193
reason = " (" + string(build.Status.Reason) + ")"
186194
}
187195
glog.V(4).Infof("Updating build %s/%s status %s -> %s%s", build.Namespace, build.Name, build.Status.Phase, nextStatus, reason)
188196
build.Status.Phase = nextStatus
189-
190-
if buildutil.IsBuildComplete(build) {
191-
common.SetBuildCompletionTimeAndDuration(build)
192-
}
193197
if build.Status.Phase == buildapi.BuildPhaseRunning {
194198
now := unversioned.Now()
195199
build.Status.StartTimestamp = &now
196200
}
201+
}
202+
203+
// we're going to get pod relist events for completed/failed pods,
204+
// there's no reason to re-update the build and rerun
205+
// HandleBuildCompletion if we've already done it for this
206+
// build previously.
207+
buildWasComplete := build.Status.CompletionTimestamp != nil
208+
if !buildWasComplete && buildutil.IsBuildComplete(build) {
209+
needsUpdate = common.SetBuildCompletionTimeAndDuration(build)
210+
}
211+
if needsUpdate {
197212
if err := bc.buildUpdater.Update(build.Namespace, build); err != nil {
198213
return fmt.Errorf("failed to update build %s/%s: %v", build.Namespace, build.Name, err)
199214
}
200215
glog.V(4).Infof("Build %s/%s status was updated to %s", build.Namespace, build.Name, build.Status.Phase)
201-
202-
if buildutil.IsBuildComplete(build) {
203-
common.HandleBuildCompletion(build, bc.runPolicies)
204-
}
205216
}
217+
if !buildWasComplete && buildutil.IsBuildComplete(build) {
218+
common.HandleBuildCompletion(build, bc.runPolicies)
219+
}
220+
206221
return nil
207222
}
208223

‎pkg/build/controller/common/util.go

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,12 +11,23 @@ import (
1111
utilruntime "k8s.io/kubernetes/pkg/util/runtime"
1212
)
1313

14-
func SetBuildCompletionTimeAndDuration(build *buildapi.Build) {
14+
// SetBuildCompletionTimeAndDuration will set the build completion timestamp
15+
// to the current time if it is nil. It will also set the start timestamp to
16+
// the same value if if it nil. Returns true if the build object was
17+
// modified.
18+
func SetBuildCompletionTimeAndDuration(build *buildapi.Build) bool {
19+
if build.Status.CompletionTimestamp != nil {
20+
return false
21+
}
1522
now := unversioned.Now()
1623
build.Status.CompletionTimestamp = &now
17-
if build.Status.StartTimestamp != nil {
18-
build.Status.Duration = build.Status.CompletionTimestamp.Rfc3339Copy().Time.Sub(build.Status.StartTimestamp.Rfc3339Copy().Time)
24+
// apparently this build completed so fast we didn't see the pod running event,
25+
// so just use the completion time as the start time.
26+
if build.Status.StartTimestamp == nil {
27+
build.Status.StartTimestamp = &now
1928
}
29+
build.Status.Duration = build.Status.CompletionTimestamp.Rfc3339Copy().Time.Sub(build.Status.StartTimestamp.Rfc3339Copy().Time)
30+
return true
2031
}
2132

2233
func HandleBuildCompletion(build *buildapi.Build, runPolicies []policy.RunPolicy) {

‎pkg/build/controller/policy/policy.go

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -142,17 +142,21 @@ func handleComplete(lister buildclient.BuildLister, updater buildclient.BuildUpd
142142
}
143143
for _, build := range nextBuilds {
144144
// TODO: replace with informer notification requeueing in the future
145-
build.Annotations[buildapi.BuildAcceptedAnnotation] = uuid.NewRandom().String()
146-
err := wait.Poll(500*time.Millisecond, 5*time.Second, func() (bool, error) {
147-
err := updater.Update(build.Namespace, build)
148-
if err != nil && errors.IsConflict(err) {
149-
glog.V(5).Infof("Error updating build %s/%s: %v (will retry)", build.Namespace, build.Name, err)
150-
return false, nil
145+
146+
// only set the annotation once.
147+
if _, ok := build.Annotations[buildapi.BuildAcceptedAnnotation]; !ok {
148+
build.Annotations[buildapi.BuildAcceptedAnnotation] = uuid.NewRandom().String()
149+
err := wait.Poll(500*time.Millisecond, 5*time.Second, func() (bool, error) {
150+
err := updater.Update(build.Namespace, build)
151+
if err != nil && errors.IsConflict(err) {
152+
glog.V(5).Infof("Error updating build %s/%s: %v (will retry)", build.Namespace, build.Name, err)
153+
return false, nil
154+
}
155+
return true, err
156+
})
157+
if err != nil {
158+
return err
151159
}
152-
return true, err
153-
})
154-
if err != nil {
155-
return err
156160
}
157161
}
158162
return nil

‎test/extended/builds/run_policy.go

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,16 @@ var _ = g.Describe("[builds][Slow] using build configuration runPolicy", func()
135135
for {
136136
event := <-buildWatch.ResultChan()
137137
build := event.Object.(*buildapi.Build)
138+
var lastCompletion time.Time
139+
if build.Status.Phase == buildapi.BuildPhaseComplete {
140+
lastCompletion = time.Now()
141+
o.Expect(build.Status.StartTimestamp).ToNot(o.BeNil(), "completed builds should have a valid start time")
142+
o.Expect(build.Status.CompletionTimestamp).ToNot(o.BeNil(), "completed builds should have a valid completion time")
143+
}
138144
if build.Status.Phase == buildapi.BuildPhaseRunning {
145+
latency := lastCompletion.Sub(time.Now())
146+
o.Expect(latency).To(o.BeNumerically("<", 10*time.Second), "next build should have started less than 10s after last completed build")
147+
139148
// Ignore events from complete builds (if there are any) if we already
140149
// verified the build.
141150
if _, exists := buildVerified[build.Name]; exists {
@@ -212,6 +221,78 @@ var _ = g.Describe("[builds][Slow] using build configuration runPolicy", func()
212221
})
213222
})
214223

224+
g.Describe("build configuration with Serial build run policy handling failure", func() {
225+
g.It("starts the next build immediately after one fails", func() {
226+
g.By("starting multiple builds")
227+
bcName := "sample-serial-build-fail"
228+
229+
for i := 0; i < 3; i++ {
230+
_, _, err := exutil.StartBuild(oc, bcName)
231+
o.Expect(err).NotTo(o.HaveOccurred())
232+
}
233+
234+
buildWatch, err := oc.Client().Builds(oc.Namespace()).Watch(kapi.ListOptions{
235+
LabelSelector: buildutil.BuildConfigSelector(bcName),
236+
})
237+
defer buildWatch.Stop()
238+
o.Expect(err).NotTo(o.HaveOccurred())
239+
240+
var failTime, failTime2 time.Time
241+
done, timestamps1, timestamps2, timestamps3 := false, false, false, false
242+
243+
for done == false {
244+
select {
245+
case event := <-buildWatch.ResultChan():
246+
build := event.Object.(*buildapi.Build)
247+
if build.Status.Phase == buildapi.BuildPhaseFailed {
248+
if build.Name == "sample-serial-build-fail-1" {
249+
// this may not be set on the first build modified to failed event because
250+
// the build gets marked failed by the build pod, but the timestamps get
251+
// set by the buildpod controller.
252+
if build.Status.CompletionTimestamp != nil {
253+
o.Expect(build.Status.StartTimestamp).ToNot(o.BeNil(), "failed builds should have a valid start time")
254+
o.Expect(build.Status.CompletionTimestamp).ToNot(o.BeNil(), "failed builds should have a valid completion time")
255+
timestamps1 = true
256+
}
257+
failTime = time.Now()
258+
}
259+
if build.Name == "sample-serial-build-fail-2" {
260+
duration := time.Now().Sub(failTime)
261+
o.Expect(duration).To(o.BeNumerically("<", 10*time.Second), "next build should have started less than 10s after failed build")
262+
if build.Status.CompletionTimestamp != nil {
263+
o.Expect(build.Status.StartTimestamp).ToNot(o.BeNil(), "failed builds should have a valid start time")
264+
o.Expect(build.Status.CompletionTimestamp).ToNot(o.BeNil(), "failed builds should have a valid completion time")
265+
timestamps2 = true
266+
}
267+
failTime2 = time.Now()
268+
}
269+
if build.Name == "sample-serial-build-fail-3" {
270+
duration := time.Now().Sub(failTime2)
271+
o.Expect(duration).To(o.BeNumerically("<", 10*time.Second), "next build should have started less than 10s after failed build")
272+
if build.Status.CompletionTimestamp != nil {
273+
o.Expect(build.Status.StartTimestamp).ToNot(o.BeNil(), "failed builds should have a valid start time")
274+
o.Expect(build.Status.CompletionTimestamp).ToNot(o.BeNil(), "failed builds should have a valid completion time")
275+
timestamps3 = true
276+
}
277+
}
278+
}
279+
// once we have all the expected timestamps, or we run out of time, we can bail.
280+
if timestamps1 && timestamps2 && timestamps3 {
281+
done = true
282+
}
283+
case <-time.After(2 * time.Minute):
284+
// we've waited as long as we dare, go see if we got all the timestamp data we expected.
285+
// if we have the timestamp data, we also know that we checked the next build start latency.
286+
done = true
287+
}
288+
}
289+
o.Expect(timestamps1).To(o.BeTrue(), "failed builds should have start and completion timestamps set")
290+
o.Expect(timestamps2).To(o.BeTrue(), "failed builds should have start and completion timestamps set")
291+
o.Expect(timestamps3).To(o.BeTrue(), "failed builds should have start and completion timestamps set")
292+
293+
})
294+
})
295+
215296
g.Describe("build configuration with SerialLatestOnly build run policy", func() {
216297
g.It("runs the builds in serial order but cancel previous builds", func() {
217298
g.By("starting multiple builds")

‎test/extended/testdata/bindata.go

Lines changed: 21 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎test/extended/testdata/run_policy/serial-bc.yaml

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,24 @@
2424
from:
2525
kind: "DockerImage"
2626
name: "centos/ruby-22-centos7"
27-
resources: {}
28-
status:
29-
lastVersion: 0
30-
27+
-
28+
kind: "BuildConfig"
29+
apiVersion: "v1"
30+
metadata:
31+
name: "sample-serial-build-fail"
32+
spec:
33+
runPolicy: "Serial"
34+
triggers:
35+
-
36+
type: "imageChange"
37+
imageChange: {}
38+
source:
39+
type: "Git"
40+
git:
41+
uri: "git://github.com/openshift/invalidrepo.git"
42+
strategy:
43+
type: "Source"
44+
sourceStrategy:
45+
from:
46+
kind: "DockerImage"
47+
name: "centos/ruby-22-centos7"

0 commit comments

Comments
 (0)