ensure next build is kicked off when a build completes

https://bugzilla.redhat.com/show_bug.cgi?id=1436395

Author: bparees

pkg/build/controller/buildpod/controller.go

@@ -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.
 	if build.Status.Phase != buildapi.BuildPhaseFailed {
 		switch pod.Status.Phase {
 		case kapi.PodPending:
@@ -176,33 +179,45 @@ func (bc *BuildPodController) HandlePod(pod *kapi.Pod) error {
 			build.Status.Message = ""
 		}
 	}
+
+	needsUpdate := false
 	// Update the build object when it progress to a next state or the reason for
-	// the current state changed.
-	if (!common.HasBuildPodNameAnnotation(build) || build.Status.Phase != nextStatus || build.Status.Phase == buildapi.BuildPhaseFailed) && !buildutil.IsBuildComplete(build) {
+	// 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) && build.Status.Phase != buildapi.BuildPhaseFailed {
+		needsUpdate = true
 		common.SetBuildPodNameAnnotation(build, pod.Name)
 		reason := ""
 		if len(build.Status.Reason) > 0 {
 			reason = " (" + string(build.Status.Reason) + ")"
 		}
 		glog.V(4).Infof("Updating build %s/%s status %s -> %s%s", build.Namespace, build.Name, build.Status.Phase, nextStatus, reason)
 		build.Status.Phase = nextStatus
-
-		if buildutil.IsBuildComplete(build) {
-			common.SetBuildCompletionTimeAndDuration(build)
-		}
 		if build.Status.Phase == buildapi.BuildPhaseRunning {
 			now := unversioned.Now()
 			build.Status.StartTimestamp = &now
 		}
+	}
+
+	// we're going to get pod relist events for completed/failed pods,
+	// 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
+	if !buildWasComplete && buildutil.IsBuildComplete(build) {
+		needsUpdate = common.SetBuildCompletionTimeAndDuration(build)
+	}
+	if needsUpdate {
 		if err := bc.buildUpdater.Update(build.Namespace, build); err != nil {
 			return fmt.Errorf("failed to update build %s/%s: %v", build.Namespace, build.Name, err)
 		}
 		glog.V(4).Infof("Build %s/%s status was updated to %s", build.Namespace, build.Name, build.Status.Phase)
-
-		if buildutil.IsBuildComplete(build) {
-			common.HandleBuildCompletion(build, bc.runPolicies)
-		}
 	}
+	if !buildWasComplete && buildutil.IsBuildComplete(build) {
+		common.HandleBuildCompletion(build, bc.runPolicies)
+	}
+
 	return nil
 }
 

pkg/build/controller/common/util.go

@@ -11,12 +11,23 @@ import (
 	utilruntime "k8s.io/kubernetes/pkg/util/runtime"
 )
 
-func SetBuildCompletionTimeAndDuration(build *buildapi.Build) {
+// SetBuildCompletionTimeAndDuration will set the build completion timestamp
+// to the current time if it is nil.  It will also set the start timestamp to
+// the same value if if it nil.  Returns true if the build object was
+// modified.
+func SetBuildCompletionTimeAndDuration(build *buildapi.Build) boolean {
+	if build.Status.CompletionTimestamp != nil {
+		return false
+	}
 	now := unversioned.Now()
 	build.Status.CompletionTimestamp = &now
-	if build.Status.StartTimestamp != nil {
-		build.Status.Duration = build.Status.CompletionTimestamp.Rfc3339Copy().Time.Sub(build.Status.StartTimestamp.Rfc3339Copy().Time)
+	// apparently this build completed so fast we didn't see the pod running event,
+	// so just use the completion time as the start time.
+	if build.Status.StartTimestamp == nil {
+		build.Status.StartTimestamp = &now
 	}
+	build.Status.Duration = build.Status.CompletionTimestamp.Rfc3339Copy().Time.Sub(build.Status.StartTimestamp.Rfc3339Copy().Time)
+	return true
 }
 
 func HandleBuildCompletion(build *buildapi.Build, runPolicies []policy.RunPolicy) {

pkg/build/controller/policy/policy.go

@@ -142,17 +142,21 @@ func handleComplete(lister buildclient.BuildLister, updater buildclient.BuildUpd
 	}
 	for _, build := range nextBuilds {
 		// TODO: replace with informer notification requeueing in the future
-		build.Annotations[buildapi.BuildAcceptedAnnotation] = uuid.NewRandom().String()
-		err := wait.Poll(500*time.Millisecond, 5*time.Second, func() (bool, error) {
-			err := updater.Update(build.Namespace, build)
-			if err != nil && errors.IsConflict(err) {
-				glog.V(5).Infof("Error updating build %s/%s: %v (will retry)", build.Namespace, build.Name, err)
-				return false, nil
+
+		// only set the annotation once.
+		if _, ok := build.Annotations[buildapi.BuildAcceptedAnnotation]; !ok {
+			build.Annotations[buildapi.BuildAcceptedAnnotation] = uuid.NewRandom().String()
+			err := wait.Poll(500*time.Millisecond, 5*time.Second, func() (bool, error) {
+				err := updater.Update(build.Namespace, build)
+				if err != nil && errors.IsConflict(err) {
+					glog.V(5).Infof("Error updating build %s/%s: %v (will retry)", build.Namespace, build.Name, err)
+					return false, nil
+				}
+				return true, err
+			})
+			if err != nil {
+				return err
 			}
-			return true, err
-		})
-		if err != nil {
-			return err
 		}
 	}
 	return nil