merge w/ build failure fixup

Author: bparees

pkg/build/controller/buildpod/controller.go

@@ -183,40 +183,56 @@ 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 are complete
+	// 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) {
+		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)
-			switch build.Status.Phase {
-			case buildapi.BuildPhaseComplete:
-				bc.recorder.Eventf(build, kapi.EventTypeNormal, buildapi.BuildCompletedEventReason, fmt.Sprintf(buildapi.BuildCompletedEventMessage, build.Namespace, build.Name))
-			case buildapi.BuildPhaseError, buildapi.BuildPhaseFailed:
-				bc.recorder.Eventf(build, kapi.EventTypeNormal, buildapi.BuildFailedEventReason, fmt.Sprintf(buildapi.BuildFailedEventMessage, build.Namespace, build.Name))
-			}
-		}
 		if build.Status.Phase == buildapi.BuildPhaseRunning {
 			now := unversioned.Now()
 			build.Status.StartTimestamp = &now
 			bc.recorder.Eventf(build, kapi.EventTypeNormal, buildapi.BuildRunningEventReason, fmt.Sprintf(buildapi.BuildRunningEventMessage, build.Namespace, build.Name))
 		}
+	}
+
+	// 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) && build.Status.Phase != buildapi.BuildPhaseCancelled {
+		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 the build was not previously marked complete but it's complete now,
+	// handle completion for it.  otherwise ignore it because we've already
+	// handled its completion previously.
+	if !buildWasComplete && buildutil.IsBuildComplete(build) {
+			common.SetBuildCompletionTimeAndDuration(build)
+			switch build.Status.Phase {
+			case buildapi.BuildPhaseComplete:
+				bc.recorder.Eventf(build, kapi.EventTypeNormal, buildapi.BuildCompletedEventReason, fmt.Sprintf(buildapi.BuildCompletedEventMessage, build.Namespace, build.Name))
+			case buildapi.BuildPhaseError, buildapi.BuildPhaseFailed:
+				bc.recorder.Eventf(build, kapi.EventTypeNormal, buildapi.BuildFailedEventReason, fmt.Sprintf(buildapi.BuildFailedEventMessage, build.Namespace, build.Name))
+			}
+		common.HandleBuildCompletion(build, bc.runPolicies)
+	}
+
 	return nil
 }
 
@@ -260,6 +276,7 @@ func (bc *BuildPodController) HandleBuildPodDeletion(pod *kapi.Pod) error {
 		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)
 		}
+		common.HandleBuildCompletion(build, bc.runPolicies)
 	}
 	return nil
 }

pkg/build/controller/buildpod/controller_test.go

@@ -158,7 +158,7 @@ func TestHandlePod(t *testing.T) {
 			outStatus:           buildapi.BuildPhaseComplete,
 			podStatus:           kapi.PodSucceeded,
 			exitCode:            0,
-			startTimestamp:      nil,
+			startTimestamp:      curtime,
 			completionTimestamp: curtime,
 		},
 		{ // 4
@@ -167,7 +167,7 @@ func TestHandlePod(t *testing.T) {
 			outStatus:           buildapi.BuildPhaseFailed,
 			podStatus:           kapi.PodFailed,
 			exitCode:            -1,
-			startTimestamp:      nil,
+			startTimestamp:      curtime,
 			completionTimestamp: curtime,
 		},
 		{ // 5
@@ -177,7 +177,7 @@ func TestHandlePod(t *testing.T) {
 			podStatus:           kapi.PodSucceeded,
 			exitCode:            0,
 			buildUpdater:        &errBuildUpdater{},
-			startTimestamp:      nil,
+			startTimestamp:      curtime,
 			completionTimestamp: curtime,
 		},
 		{ // 6

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 it is nil.  Returns true if the build object was
+// modified.
+func SetBuildCompletionTimeAndDuration(build *buildapi.Build) bool {
+	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/controller_test.go

@@ -447,23 +447,23 @@ func TestCancelBuild(t *testing.T) {
 			inStatus:            buildapi.BuildPhaseNew,
 			outStatus:           buildapi.BuildPhaseCancelled,
 			exitCode:            0,
-			startTimestamp:      nil,
+			startTimestamp:      curtime,
 			completionTimestamp: curtime,
 		},
 		{ // 1
 			inStatus:            buildapi.BuildPhasePending,
 			outStatus:           buildapi.BuildPhaseCancelled,
 			podStatus:           kapi.PodRunning,
 			exitCode:            0,
-			startTimestamp:      nil,
+			startTimestamp:      curtime,
 			completionTimestamp: curtime,
 		},
 		{ // 2
 			inStatus:            buildapi.BuildPhaseRunning,
 			outStatus:           buildapi.BuildPhaseCancelled,
 			podStatus:           kapi.PodRunning,
 			exitCode:            0,
-			startTimestamp:      nil,
+			startTimestamp:      curtime,
 			completionTimestamp: curtime,
 		},
 		{ // 3

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