@@ -8,7 +8,6 @@ import (

 	kapi "github.com/GoogleCloudPlatform/kubernetes/pkg/api"

 	errors "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors"

 	"github.com/GoogleCloudPlatform/kubernetes/pkg/client/cache"

-	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"

 	buildapi "github.com/openshift/origin/pkg/build/api"

 	buildclient "github.com/openshift/origin/pkg/build/client"

@@ -17,9 +16,6 @@ import (

 // BuildController watches build resources and manages their state

 type BuildController struct {

-	BuildStore    cache.Store

-	NextBuild     func() *buildapi.Build

-	NextPod       func() *kapi.Pod

 	BuildUpdater  buildclient.BuildUpdater

 	PodManager    podManager

 	BuildStrategy BuildStrategy

@@ -41,32 +37,30 @@ type imageRepositoryClient interface {

 	GetImageRepository(namespace, name string) (*imageapi.ImageRepository, error)

 }

-// Run begins watching and syncing build jobs onto the cluster.

-func (bc *BuildController) Run() {

-	go util.Forever(func() { bc.HandleBuild(bc.NextBuild()) }, 0)

-	go util.Forever(func() { bc.HandlePod(bc.NextPod()) }, 0)

-}

-

-func (bc *BuildController) HandleBuild(build *buildapi.Build) {

+func (bc *BuildController) HandleBuild(build *buildapi.Build) error {

 	glog.V(4).Infof("Handling build %s", build.Name)

 	// We only deal with new builds here

 	if build.Status != buildapi.BuildStatusNew {

-		return

+		return nil

 	}

 	if err := bc.nextBuildStatus(build); err != nil {

 		// TODO: all build errors should be retried, and build error should not be a permanent status change.

 		// Instead, we should requeue this build request using the same backoff logic as the scheduler.

-		// BuildStatusError should be reserved for meaning "permanently errored, no way to try again".

-		glog.V(4).Infof("Build failed with error %s/%s: %#v", build.Namespace, build.Name, err)

-		build.Status = buildapi.BuildStatusError

-		build.Message = err.Error()

+		//build.Status = buildapi.BuildStatusError

+		//build.Message = err.Error()

+		return fmt.Errorf("Build failed with error %s/%s: %#v", build.Namespace, build.Name, err)

 	}

 	if err := bc.BuildUpdater.Update(build.Namespace, build); err != nil {

+		// This is not a retryable error because the build has been created.  The worst case

+		// outcome of not updating the buildconfig is that we might rerun a build for the

+		// same "new" imageid change in the future, which is better than guaranteeing we

+		// run the build 2+ times by retrying it here.

 		glog.V(2).Infof("Failed to record changes to build %s/%s: %#v", build.Namespace, build.Name, err)

 	}

+	return nil

 }

 // nextBuildStatus updates build with any appropriate changes, or returns an error if

@@ -134,7 +128,14 @@ func (bc *BuildController) nextBuildStatus(build *buildapi.Build) error {

 	return nil

 }

-func (bc *BuildController) HandlePod(pod *kapi.Pod) {

+// BuildPodController watches pods running builds and manages the build state

+type BuildPodController struct {

+	BuildStore   cache.Store

+	BuildUpdater buildclient.BuildUpdater

+	PodManager   podManager

+}

+

+func (bc *BuildPodController) HandlePod(pod *kapi.Pod) error {

 	// Find the build for this pod

 	var build *buildapi.Build

 	for _, obj := range bc.BuildStore.List() {

@@ -146,7 +147,7 @@ func (bc *BuildController) HandlePod(pod *kapi.Pod) {

 	}

 	if build == nil {

-		return

+		return nil

 	}

 	// A cancelling event was triggered for the build, delete its pod and update build status.

@@ -154,9 +155,9 @@ func (bc *BuildController) HandlePod(pod *kapi.Pod) {

 		glog.V(2).Infof("Cancelling build %s.", build.Name)

 		if err := bc.CancelBuild(build, pod); err != nil {

-			glog.Errorf("Failed to cancel build %s: %#v", build.Name, err)

+			return fmt.Errorf("Failed to cancel build %s: %#v, will retry", build.Name, err)

 		}

-		return

+		return nil

 	}

 	nextStatus := build.Status

@@ -180,13 +181,14 @@ func (bc *BuildController) HandlePod(pod *kapi.Pod) {

 		glog.V(4).Infof("Updating build %s status %s -> %s", build.Name, build.Status, nextStatus)

 		build.Status = nextStatus

 		if err := bc.BuildUpdater.Update(build.Namespace, build); err != nil {

-			glog.Errorf("Failed to update build %s: %#v", build.Name, err)

+			return fmt.Errorf("Failed to update build %s: %#v", build.Name, err)

 		}

 	}

+	return nil

 }

-// CancelBuild updates a build status to Cancelled, after its associated pod is associated.

-func (bc *BuildController) CancelBuild(build *buildapi.Build, pod *kapi.Pod) error {

+// CancelBuild updates a build status to Cancelled, after its associated pod is deleted.

+func (bc *BuildPodController) CancelBuild(build *buildapi.Build, pod *kapi.Pod) error {

 	if !isBuildCancellable(build) {

 		glog.V(2).Infof("The build can be cancelled only if it has pending/running status, not %s.", build.Status)

 		return nil

@@ -2,6 +2,7 @@ package controller

 import (

 	"errors"

+	"reflect"

 	"testing"

 	kapi "github.com/GoogleCloudPlatform/kubernetes/pkg/api"

@@ -93,8 +94,8 @@ func (_ *errNotFoundImageRepositoryClient) GetImageRepository(namespace, name st

 	return nil, kerrors.NewNotFound("ImageRepository", name)

 }

-func mockBuildAndController(status buildapi.BuildStatus, output buildapi.BuildOutput) (build *buildapi.Build, controller *BuildController) {

-	build = &buildapi.Build{

+func mockBuild(status buildapi.BuildStatus, output buildapi.BuildOutput) *buildapi.Build {

+	return &buildapi.Build{

 		ObjectMeta: kapi.ObjectMeta{

 			Name:      "data-build",

 			Namespace: "namespace",

@@ -119,16 +120,23 @@ func mockBuildAndController(status buildapi.BuildStatus, output buildapi.BuildOu

 		Status:  status,

 		PodName: "-the-pod-id",

 	}

-	controller = &BuildController{

-		BuildStore:            buildtest.NewFakeBuildStore(build),

+}

+

+func mockBuildController() *BuildController {

+	return &BuildController{

 		BuildUpdater:          &okBuildUpdater{},

 		PodManager:            &okPodManager{},

-		NextBuild:             func() *buildapi.Build { return nil },

-		NextPod:               func() *kapi.Pod { return nil },

 		BuildStrategy:         &okStrategy{},

 		ImageRepositoryClient: &okImageRepositoryClient{},

 	}

-	return

+}

+

+func mockBuildPodController(build *buildapi.Build) *BuildPodController {

+	return &BuildPodController{

+		BuildStore:   buildtest.NewFakeBuildStore(build),

+		BuildUpdater: &okBuildUpdater{},

+		PodManager:   &okPodManager{},

+	}

 }

 func mockPod(status kapi.PodPhase, exitCode int) *kapi.Pod {

@@ -257,7 +265,7 @@ func TestHandleBuild(t *testing.T) {

 		},

 		{ // 12

 			inStatus:    buildapi.BuildStatusNew,

-			outStatus:   buildapi.BuildStatusError, // TODO: this should be a retry

+			outStatus:   buildapi.BuildStatusError,

 			imageClient: &errNotFoundImageRepositoryClient{},

 			buildOutput: buildapi.BuildOutput{

 				To: &kapi.ObjectReference{

@@ -275,10 +283,24 @@ func TestHandleBuild(t *testing.T) {

 				},

 			},

 		},

+		{ // 14

+			inStatus:  buildapi.BuildStatusNew,

+			outStatus: buildapi.BuildStatusPending,

+			buildOutput: buildapi.BuildOutput{

+				To: &kapi.ObjectReference{

+					Name:      "foo",

+					Namespace: "bar",

+				},

+			},

+			outputSpec: "image/repo",

+			// an error updating the build is not reported as an error.

+			buildUpdater: &errBuildUpdater{},

+		},

 	}

 	for i, tc := range tests {

-		build, ctrl := mockBuildAndController(tc.inStatus, tc.buildOutput)

+		build := mockBuild(tc.inStatus, tc.buildOutput)

+		ctrl := mockBuildController()

 		if tc.buildStrategy != nil {

 			ctrl.BuildStrategy = tc.buildStrategy

 		}

@@ -292,8 +314,20 @@ func TestHandleBuild(t *testing.T) {

 			ctrl.ImageRepositoryClient = tc.imageClient

 		}

-		ctrl.HandleBuild(build)

+		err := ctrl.HandleBuild(build)

+

+		// ensure we return an error for cases where expected output is an error.

+		// these will be retried by the retrycontroller

+		if tc.inStatus != buildapi.BuildStatusError && tc.outStatus == buildapi.BuildStatusError {

+			if err == nil {

+				t.Errorf("(%d) Expected an error from HandleBuild, got none!", i)

+			}

+			continue

+		}

+		if err != nil {

+			t.Errorf("(%d) Unexpected error %v", err)

+		}

 		if build.Status != tc.outStatus {

 			t.Errorf("(%d) Expected %s, got %s!", i, tc.outStatus, build.Status)

 		}

@@ -317,6 +351,7 @@ func TestHandlePod(t *testing.T) {

 		podStatus    kapi.PodPhase

 		exitCode     int

 		buildUpdater buildclient.BuildUpdater

+		podManager   podManager

 	}

 	tests := []handlePodTest{

@@ -366,7 +401,8 @@ func TestHandlePod(t *testing.T) {

 	}

 	for i, tc := range tests {

-		build, ctrl := mockBuildAndController(tc.inStatus, buildapi.BuildOutput{})

+		build := mockBuild(tc.inStatus, buildapi.BuildOutput{})

+		ctrl := mockBuildPodController(build)

 		pod := mockPod(tc.podStatus, tc.exitCode)

 		if tc.matchID {

 			build.PodName = pod.Name

@@ -375,8 +411,16 @@ func TestHandlePod(t *testing.T) {

 			ctrl.BuildUpdater = tc.buildUpdater

 		}

-		ctrl.HandlePod(pod)

+		err := ctrl.HandlePod(pod)

+		if tc.buildUpdater != nil && reflect.TypeOf(tc.buildUpdater).Elem().Name() == "errBuildUpdater" {

+			if err == nil {

+				t.Errorf("(%d) Expected error, got none", i)

+			}

+			// can't check tc.outStatus because the local build object does get updated

+			// in this test (but would not updated in etcd)

+			continue

+		}

 		if build.Status != tc.outStatus {

 			t.Errorf("(%d) Expected %s, got %s!", i, tc.outStatus, build.Status)

 		}

@@ -385,10 +429,12 @@ func TestHandlePod(t *testing.T) {

 func TestCancelBuild(t *testing.T) {

 	type handleCancelBuildTest struct {

-		inStatus  buildapi.BuildStatus

-		outStatus buildapi.BuildStatus

-		podStatus kapi.PodPhase

-		exitCode  int

+		inStatus     buildapi.BuildStatus

+		outStatus    buildapi.BuildStatus

+		podStatus    kapi.PodPhase

+		exitCode     int

+		buildUpdater buildclient.BuildUpdater

+		podManager   podManager

 	}

 	tests := []handleCancelBuildTest{

@@ -421,13 +467,48 @@ func TestCancelBuild(t *testing.T) {

 			podStatus: kapi.PodFailed,

 			exitCode:  1,

 		},

+		{ // 5

+			inStatus:   buildapi.BuildStatusNew,

+			outStatus:  buildapi.BuildStatusNew,

+			podStatus:  kapi.PodFailed,

+			exitCode:   1,

+			podManager: &errPodManager{},

+		},

+		{ // 6

+			inStatus:     buildapi.BuildStatusNew,

+			outStatus:    buildapi.BuildStatusNew,

+			podStatus:    kapi.PodFailed,

+			exitCode:     1,

+			buildUpdater: &errBuildUpdater{},

+		},

 	}

 	for i, tc := range tests {

-		build, ctrl := mockBuildAndController(tc.inStatus, buildapi.BuildOutput{})

+		build := mockBuild(tc.inStatus, buildapi.BuildOutput{})

+		ctrl := mockBuildPodController(build)

 		pod := mockPod(tc.podStatus, tc.exitCode)

+		if tc.buildUpdater != nil {

+			ctrl.BuildUpdater = tc.buildUpdater

+		}

+		if tc.podManager != nil {

+			ctrl.PodManager = tc.podManager

+		}

-		ctrl.CancelBuild(build, pod)

+		err := ctrl.CancelBuild(build, pod)

+

+		if tc.podManager != nil && reflect.TypeOf(tc.podManager).Elem().Name() == "errPodManager" {

+			if err == nil {

+				t.Errorf("(%d) Expected error, got none", i)

+			}

+		}

+		if tc.buildUpdater != nil && reflect.TypeOf(tc.buildUpdater).Elem().Name() == "errBuildUpdater" {

+			if err == nil {

+				t.Errorf("(%d) Expected error, got none", i)

+			}

+			// can't check tc.outStatus because the local build object does get updated

+			// in this test (but would not updated in etcd)

+			continue

+		}

 		if build.Status != tc.outStatus {

 			t.Errorf("(%d) Expected %s, got %s!", i, tc.outStatus, build.Status)

@@ -11,16 +11,19 @@ import (

 	"github.com/GoogleCloudPlatform/kubernetes/pkg/client/cache"

 	"github.com/GoogleCloudPlatform/kubernetes/pkg/labels"

 	"github.com/GoogleCloudPlatform/kubernetes/pkg/runtime"

+	kutil "github.com/GoogleCloudPlatform/kubernetes/pkg/util"

 	"github.com/GoogleCloudPlatform/kubernetes/pkg/watch"

 	buildapi "github.com/openshift/origin/pkg/build/api"

 	buildclient "github.com/openshift/origin/pkg/build/client"

-	controller "github.com/openshift/origin/pkg/build/controller"

+	buildcontroller "github.com/openshift/origin/pkg/build/controller"

 	strategy "github.com/openshift/origin/pkg/build/controller/strategy"

 	osclient "github.com/openshift/origin/pkg/client"

+	controller "github.com/openshift/origin/pkg/controller"

 	imageapi "github.com/openshift/origin/pkg/image/api"

 )

+// BuildControllerFactory constructs BuildController objects

 type BuildControllerFactory struct {

 	OSClient            osclient.Interface

 	KubeClient          kclient.Interface

@@ -30,16 +33,55 @@ type BuildControllerFactory struct {

 	CustomBuildStrategy *strategy.CustomBuildStrategy

 	// Stop may be set to allow controllers created by this factory to be terminated.

 	Stop <-chan struct{}

+}

+

+// Create constructs a BuildController

+func (factory *BuildControllerFactory) Create() controller.RunnableController {

+	queue := cache.NewFIFO(cache.MetaNamespaceKeyFunc)

+	cache.NewReflector(&buildLW{client: factory.OSClient}, &buildapi.Build{}, queue).Run()

+

+	client := ControllerClient{factory.KubeClient, factory.OSClient}

+	buildController := &buildcontroller.BuildController{

+		BuildUpdater:          factory.BuildUpdater,

+		ImageRepositoryClient: client,

+		PodManager:            client,

+		BuildStrategy: &typeBasedFactoryStrategy{

+			DockerBuildStrategy: factory.DockerBuildStrategy,

+			STIBuildStrategy:    factory.STIBuildStrategy,

+			CustomBuildStrategy: factory.CustomBuildStrategy,

+		},

+	}

+

+	return &controller.RetryController{

+		Queue:        queue,

+		RetryManager: controller.NewQueueRetryManager(queue, cache.MetaNamespaceKeyFunc, -1),

+		ShouldRetry: func(obj interface{}, err error) bool {

+			kutil.HandleError(err)

+			// BuildController currently has no fatal errors.

+			return true

+		},

+		Handle: func(obj interface{}) error {

+			build := obj.(*buildapi.Build)

+			return buildController.HandleBuild(build)

+		},

+	}

+}

+

+// BuildPodControllerFactory construct BuildPodController objects

+type BuildPodControllerFactory struct {

+	OSClient     osclient.Interface

+	KubeClient   kclient.Interface

+	BuildUpdater buildclient.BuildUpdater

+	// Stop may be set to allow controllers created by this factory to be terminated.

+	Stop <-chan struct{}

 	buildStore cache.Store

 }

-func (factory *BuildControllerFactory) Create() *controller.BuildController {

+// Create constructs a BuildPodController

+func (factory *BuildPodControllerFactory) Create() controller.RunnableController {

 	factory.buildStore = cache.NewStore(cache.MetaNamespaceKeyFunc)

-	cache.NewReflector(&buildLW{client: factory.OSClient}, &buildapi.Build{}, factory.buildStore).RunUntil(factory.Stop)

-

-	buildQueue := cache.NewFIFO(cache.MetaNamespaceKeyFunc)

-	cache.NewReflector(&buildLW{client: factory.OSClient}, &buildapi.Build{}, buildQueue).RunUntil(factory.Stop)

+	cache.NewReflector(&buildLW{client: factory.OSClient}, &buildapi.Build{}, factory.buildStore).Run()

 	// Kubernetes does not currently synchronize Pod status in storage with a Pod's container

 	// states. Because of this, we can't receive events related to container (and thus Pod)

@@ -49,29 +91,27 @@ func (factory *BuildControllerFactory) Create() *controller.BuildController {

 	//

 	// TODO: Find a way to get watch events for Pod/container status updates. The polling

 	// strategy is horribly inefficient and should be addressed upstream somehow.

-	podQueue := cache.NewFIFO(cache.MetaNamespaceKeyFunc)

-	cache.NewPoller(factory.pollPods, 10*time.Second, podQueue).RunUntil(factory.Stop)

+	queue := cache.NewFIFO(cache.MetaNamespaceKeyFunc)

+	cache.NewPoller(factory.pollPods, 10*time.Second, queue).RunUntil(factory.Stop)

 	client := ControllerClient{factory.KubeClient, factory.OSClient}

-	return &controller.BuildController{

-		BuildStore:            factory.buildStore,

-		BuildUpdater:          factory.BuildUpdater,

-		ImageRepositoryClient: client,

-		PodManager:            client,

-		NextBuild: func() *buildapi.Build {

-			build := buildQueue.Pop().(*buildapi.Build)

-			panicIfStopped(factory.Stop, "build controller stopped")

-			return build

-		},

-		NextPod: func() *kapi.Pod {

-			pod := podQueue.Pop().(*kapi.Pod)

-			panicIfStopped(factory.Stop, "build controller stopped")

-			return pod

+	buildPodController := &buildcontroller.BuildPodController{

+		BuildStore:   factory.buildStore,

+		BuildUpdater: factory.BuildUpdater,

+		PodManager:   client,

+	}

+

+	return &controller.RetryController{

+		Queue:        queue,

+		RetryManager: controller.NewQueueRetryManager(queue, cache.MetaNamespaceKeyFunc, -1),

+		ShouldRetry: func(obj interface{}, err error) bool {

+			kutil.HandleError(err)

+			// BuildPodController currently has no fatal errors.

+			return true

 		},

-		BuildStrategy: &typeBasedFactoryStrategy{

-			DockerBuildStrategy: factory.DockerBuildStrategy,

-			STIBuildStrategy:    factory.STIBuildStrategy,

-			CustomBuildStrategy: factory.CustomBuildStrategy,

+		Handle: func(obj interface{}) error {

+			pod := obj.(*kapi.Pod)

+			return buildPodController.HandlePod(pod)

 		},

 	}

 }

@@ -88,29 +128,40 @@ type ImageChangeControllerFactory struct {

 // Create creates a new ImageChangeController which is used to trigger builds when a new

 // image is available

-func (factory *ImageChangeControllerFactory) Create() *controller.ImageChangeController {

+func (factory *ImageChangeControllerFactory) Create() controller.RunnableController {

 	queue := cache.NewFIFO(cache.MetaNamespaceKeyFunc)

-	cache.NewReflector(&imageRepositoryLW{factory.Client}, &imageapi.ImageRepository{}, queue).RunUntil(factory.Stop)

+	cache.NewReflector(&imageRepositoryLW{factory.Client}, &imageapi.ImageRepository{}, queue).Run()

 	store := cache.NewStore(cache.MetaNamespaceKeyFunc)

-	cache.NewReflector(&buildConfigLW{client: factory.Client}, &buildapi.BuildConfig{}, store).RunUntil(factory.Stop)

+	cache.NewReflector(&buildConfigLW{client: factory.Client}, &buildapi.BuildConfig{}, store).Run()

-	return &controller.ImageChangeController{

+	imageChangeController := &buildcontroller.ImageChangeController{

 		BuildConfigStore:   store,

 		BuildConfigUpdater: factory.BuildConfigUpdater,

 		BuildCreator:       factory.BuildCreator,

-		NextImageRepository: func() *imageapi.ImageRepository {

-			repo := queue.Pop().(*imageapi.ImageRepository)

-			panicIfStopped(factory.Stop, "build image change controller stopped")

-			return repo

+		Stop:               factory.Stop,

+	}

+

+	return &controller.RetryController{

+		Queue:        queue,

+		RetryManager: controller.NewQueueRetryManager(queue, cache.MetaNamespaceKeyFunc, -1),

+		ShouldRetry: func(obj interface{}, err error) bool {

+			kutil.HandleError(err)

+			if _, isFatal := err.(buildcontroller.ImageChangeControllerFatalError); isFatal {

+				return false

+			}

+			return true

+		},

+		Handle: func(obj interface{}) error {

+			imageRepo := obj.(*imageapi.ImageRepository)

+			return imageChangeController.HandleImageRepo(imageRepo)

 		},

-		Stop: factory.Stop,

 	}

 }

 // pollPods lists pods for all builds in the buildStore which are pending or running and

 // returns an enumerator for cache.Poller. The poll scope is narrowed for efficiency.

-func (factory *BuildControllerFactory) pollPods() (cache.Enumerator, error) {

+func (factory *BuildPodControllerFactory) pollPods() (cache.Enumerator, error) {

 	list := &kapi.PodList{}

 	for _, obj := range factory.buildStore.List() {

@@ -1,10 +1,10 @@

 package controller

 import (

-	"github.com/golang/glog"

+	"fmt"

 	"github.com/GoogleCloudPlatform/kubernetes/pkg/client/cache"

-	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"

+	"github.com/golang/glog"

 	buildapi "github.com/openshift/origin/pkg/build/api"

 	buildclient "github.com/openshift/origin/pkg/build/client"

@@ -12,27 +12,28 @@ import (

 	imageapi "github.com/openshift/origin/pkg/image/api"

 )

+type ImageChangeControllerFatalError struct {

+	Reason string

+	Err    error

+}

+

+func (e ImageChangeControllerFatalError) Error() string {

+	return "fatal error handling ImageRepository change: " + e.Reason

+}

+

 // ImageChangeController watches for changes to ImageRepositories and triggers

 // builds when a new version of a tag referenced by a BuildConfig

 // is available.

 type ImageChangeController struct {

-	NextImageRepository func() *imageapi.ImageRepository

-	BuildConfigStore    cache.Store

-	BuildCreator        buildclient.BuildCreator

-	BuildConfigUpdater  buildclient.BuildConfigUpdater

+	BuildConfigStore   cache.Store

+	BuildCreator       buildclient.BuildCreator

+	BuildConfigUpdater buildclient.BuildConfigUpdater

 	// Stop is an optional channel that controls when the controller exits

 	Stop <-chan struct{}

 }

-// Run processes ImageRepository events one by one.

-func (c *ImageChangeController) Run() {

-	go util.Until(c.HandleImageRepo, 0, c.Stop)

-}

-

 // HandleImageRepo processes the next ImageRepository event.

-func (c *ImageChangeController) HandleImageRepo() {

-	glog.V(4).Infof("Waiting for imagerepo change")

-	imageRepo := c.NextImageRepository()

+func (c *ImageChangeController) HandleImageRepo(imageRepo *imageapi.ImageRepository) error {

 	glog.V(4).Infof("Build image change controller detected imagerepo change %s", imageRepo.DockerImageRepository)

 	imageSubstitutions := make(map[string]string)

@@ -78,12 +79,18 @@ func (c *ImageChangeController) HandleImageRepo() {

 			glog.V(4).Infof("Running build for buildConfig %s in namespace %s", config.Name, config.Namespace)

 			b := buildutil.GenerateBuildFromConfig(config, nil, imageSubstitutions)

 			if err := c.BuildCreator.Create(config.Namespace, b); err != nil {

-				glog.V(2).Infof("Error starting build for buildConfig %v: %v", config.Name, err)

+				return fmt.Errorf("Error starting build for buildConfig %s: %v", config.Name, err)

 			} else {

 				if err := c.BuildConfigUpdater.Update(config); err != nil {

+					// This is not a retryable error because the build has been created.  The worst case

+					// outcome of not updating the buildconfig is that we might rerun a build for the

+					// same "new" imageid change in the future, which is better than guaranteeing we

+					// run the build 2+ times by retrying it here.

 					glog.V(2).Infof("Error updating buildConfig %v: %v", config.Name, err)

+					return ImageChangeControllerFatalError{Reason: fmt.Sprintf("Error updating buildConfig %s with new LastTriggeredImageID", config.Name), Err: err}

 				}

 			}

 		}

 	}

+	return nil

 }

@@ -13,11 +13,12 @@ import (

 type mockBuildConfigUpdater struct {

 	buildcfg *buildapi.BuildConfig

+	err      error

 }

 func (m *mockBuildConfigUpdater) Update(buildcfg *buildapi.BuildConfig) error {

 	m.buildcfg = buildcfg

-	return nil

+	return m.err

 }

 type mockBuildCreator struct {

@@ -71,8 +72,8 @@ func appendTrigger(buildcfg *buildapi.BuildConfig, triggerImage, repoName, repoT

 	})

 }

-func mockImageChangeController(buildcfg *buildapi.BuildConfig, repoName, dockerImageRepo string, tags map[string]string) *ImageChangeController {

-	imageRepo := imageapi.ImageRepository{

+func mockImageRepo(repoName, dockerImageRepo string, tags map[string]string) *imageapi.ImageRepository {

+	return &imageapi.ImageRepository{

 		ObjectMeta: kapi.ObjectMeta{

 			Name: repoName,

 		},

@@ -81,23 +82,28 @@ func mockImageChangeController(buildcfg *buildapi.BuildConfig, repoName, dockerI

 		},

 		Tags: tags,

 	}

+}

+func mockImageChangeController(buildcfg *buildapi.BuildConfig) *ImageChangeController {

 	return &ImageChangeController{

-		NextImageRepository: func() *imageapi.ImageRepository { return &imageRepo },

-		BuildConfigStore:    buildtest.NewFakeBuildConfigStore(buildcfg),

-		BuildCreator:        &mockBuildCreator{},

-		BuildConfigUpdater:  &mockBuildConfigUpdater{},

+		BuildConfigStore:   buildtest.NewFakeBuildConfigStore(buildcfg),

+		BuildCreator:       &mockBuildCreator{},

+		BuildConfigUpdater: &mockBuildConfigUpdater{},

 	}

 }

 func TestNewImageID(t *testing.T) {

 	// valid configuration, new build should be triggered.

 	buildcfg := mockBuildConfig("registry.com/namespace/imagename", "registry.com/namespace/imagename", "testImageRepo", "testTag")

-	controller := mockImageChangeController(buildcfg, "testImageRepo", "registry.com/namespace/imagename", map[string]string{"testTag": "newImageID123"})

-	controller.HandleImageRepo()

+	imagerepo := mockImageRepo("testImageRepo", "registry.com/namespace/imagename", map[string]string{"testTag": "newImageID123"})

+	controller := mockImageChangeController(buildcfg)

+	err := controller.HandleImageRepo(imagerepo)

 	buildCreator := controller.BuildCreator.(*mockBuildCreator)

 	buildConfigUpdater := controller.BuildConfigUpdater.(*mockBuildConfigUpdater)

+	if err != nil {

+		t.Errorf("Unexpected error %v from HandleImageRepo", err)

+	}

 	if buildCreator.build == nil {

 		t.Error("Expected new build when new image was created!")

 	}

@@ -115,11 +121,15 @@ func TestNewImageID(t *testing.T) {

 func TestNewImageIDDefaultTag(t *testing.T) {

 	// valid configuration using default tag, new build should be triggered.

 	buildcfg := mockBuildConfig("registry.com/namespace/imagename", "registry.com/namespace/imagename", "testImageRepo", "")

-	controller := mockImageChangeController(buildcfg, "testImageRepo", "registry.com/namespace/imagename", map[string]string{buildapi.DefaultImageTag: "newImageID123"})

-	controller.HandleImageRepo()

+	imagerepo := mockImageRepo("testImageRepo", "registry.com/namespace/imagename", map[string]string{buildapi.DefaultImageTag: "newImageID123"})

+	controller := mockImageChangeController(buildcfg)

+	err := controller.HandleImageRepo(imagerepo)

 	buildCreator := controller.BuildCreator.(*mockBuildCreator)

 	buildConfigUpdater := controller.BuildConfigUpdater.(*mockBuildConfigUpdater)

+	if err != nil {

+		t.Errorf("Unexpected error %v from HandleImageRepo", err)

+	}

 	if buildCreator.build == nil {

 		t.Error("Expected new build when new image was created!")

 	}

@@ -138,11 +148,15 @@ func TestNonExistentImageRepository(t *testing.T) {

 	// this buildconfig references a non-existent imagerepo, so an update to the real imagerepo should not

 	// trigger a build here.

 	buildcfg := mockBuildConfig("registry.com/namespace/imagename", "registry.com/namespace/imagename", "testImageRepo", "testTag")

-	controller := mockImageChangeController(buildcfg, "otherImageRepo", "registry.com/namespace/imagename", map[string]string{"testTag": "newImageID123"})

-	controller.HandleImageRepo()

+	imagerepo := mockImageRepo("otherImageRepo", "registry.com/namespace/imagename", map[string]string{"testTag": "newImageID123"})

+	controller := mockImageChangeController(buildcfg)

+	err := controller.HandleImageRepo(imagerepo)

 	buildCreator := controller.BuildCreator.(*mockBuildCreator)

 	buildConfigUpdater := controller.BuildConfigUpdater.(*mockBuildConfigUpdater)

+	if err != nil {

+		t.Errorf("Unexpected error %v from HandleImageRepo", err)

+	}

 	if buildCreator.build != nil {

 		t.Error("New build created when a different repository was updated!")

 	}

@@ -154,11 +168,15 @@ func TestNonExistentImageRepository(t *testing.T) {

 func TestNewImageDifferentTagUpdate(t *testing.T) {

 	// this buildconfig references a different tag than the one that will be updated

 	buildcfg := mockBuildConfig("registry.com/namespace/imagename", "registry.com/namespace/imagename", "testImageRepo", "testTag")

-	controller := mockImageChangeController(buildcfg, "testImageRepo", "registry.com/namespace/imagename", map[string]string{"otherTag": "newImageID123"})

-	controller.HandleImageRepo()

+	imagerepo := mockImageRepo("testImageRepo", "registry.com/namespace/imagename", map[string]string{"otherTag": "newImageID123"})

+	controller := mockImageChangeController(buildcfg)

+	err := controller.HandleImageRepo(imagerepo)

 	buildCreator := controller.BuildCreator.(*mockBuildCreator)

 	buildConfigUpdater := controller.BuildConfigUpdater.(*mockBuildConfigUpdater)

+	if err != nil {

+		t.Errorf("Unexpected error %v from HandleImageRepo", err)

+	}

 	if buildCreator.build != nil {

 		t.Error("New build created when a different repository was updated!")

 	}

@@ -172,12 +190,15 @@ func TestNewImageDifferentTagUpdate2(t *testing.T) {

 	// it has previously run a build for the testTagID123 tag.

 	buildcfg := mockBuildConfig("registry.com/namespace/imagename", "registry.com/namespace/imagename", "testImageRepo", "testTag")

 	buildcfg.Triggers[0].ImageChange.LastTriggeredImageID = "testTagID123"

-	controller := mockImageChangeController(buildcfg, "testImageRepo", "registry.com/namespace/imagename",

-		map[string]string{"otherTag": "newImageID123", "testTag": "testTagID123"})

-	controller.HandleImageRepo()

+	imagerepo := mockImageRepo("testImageRepo", "registry.com/namespace/imagename", map[string]string{"otherTag": "newImageID123", "testTag": "testTagID123"})

+	controller := mockImageChangeController(buildcfg)

+	err := controller.HandleImageRepo(imagerepo)

 	buildCreator := controller.BuildCreator.(*mockBuildCreator)

 	buildConfigUpdater := controller.BuildConfigUpdater.(*mockBuildConfigUpdater)

+	if err != nil {

+		t.Errorf("Unexpected error %v from HandleImageRepo", err)

+	}

 	if buildCreator.build != nil {

 		t.Error("New build created when a different repository was updated!")

 	}

@@ -189,11 +210,15 @@ func TestNewImageDifferentTagUpdate2(t *testing.T) {

 func TestNewDifferentImageUpdate(t *testing.T) {

 	// this buildconfig references a different image than the one that will be updated

 	buildcfg := mockBuildConfig("registry.com/namespace/imagename1", "registry.com/namespace/imagename1", "testImageRepo1", "testTag1")

-	controller := mockImageChangeController(buildcfg, "testImageRepo2", "registry.com/namespace/imagename2", map[string]string{"testTag2": "newImageID123"})

-	controller.HandleImageRepo()

+	imagerepo := mockImageRepo("testImageRepo2", "registry.com/namespace/imagename2", map[string]string{"testTag2": "newImageID123"})

+	controller := mockImageChangeController(buildcfg)

+	err := controller.HandleImageRepo(imagerepo)

 	buildCreator := controller.BuildCreator.(*mockBuildCreator)

 	buildConfigUpdater := controller.BuildConfigUpdater.(*mockBuildConfigUpdater)

+	if err != nil {

+		t.Errorf("Unexpected error %v from HandleImageRepo", err)

+	}

 	if buildCreator.build != nil {

 		t.Error("New build created when a different repository was updated!")

 	}

@@ -206,11 +231,15 @@ func TestMultipleTriggers(t *testing.T) {

 	// this buildconfig references multiple images

 	buildcfg := mockBuildConfig("registry.com/namespace/imagename1", "registry.com/namespace/imagename1", "testImageRepo1", "testTag1")

 	appendTrigger(buildcfg, "registry.com/namespace/imagename2", "testImageRepo2", "testTag2")

-	controller := mockImageChangeController(buildcfg, "testImageRepo2", "registry.com/namespace/imagename2", map[string]string{"testTag2": "newImageID123"})

-	controller.HandleImageRepo()

+	imagerepo := mockImageRepo("testImageRepo2", "registry.com/namespace/imagename2", map[string]string{"testTag2": "newImageID123"})

+	controller := mockImageChangeController(buildcfg)

+	err := controller.HandleImageRepo(imagerepo)

 	buildCreator := controller.BuildCreator.(*mockBuildCreator)

 	buildConfigUpdater := controller.BuildConfigUpdater.(*mockBuildConfigUpdater)

+	if err != nil {

+		t.Errorf("Unexpected error %v from HandleImageRepo", err)

+	}

 	if buildCreator.build == nil {

 		t.Error("Expected new build when new image was created!")

 	}

@@ -230,11 +259,15 @@ func TestBuildConfigWithDifferentTriggerType(t *testing.T) {

 	// this buildconfig has different (than ImageChangeTrigger) trigger defined

 	buildcfg := mockBuildConfig("registry.com/namespace/imagename1", "", "", "")

 	buildcfg.Triggers[0].Type = buildapi.GenericWebHookBuildTriggerType

-	controller := mockImageChangeController(buildcfg, "testImageRepo2", "registry.com/namespace/imagename2", map[string]string{"testTag2": "newImageID123"})

-	controller.HandleImageRepo()

+	imagerepo := mockImageRepo("testImageRepo2", "registry.com/namespace/imagename2", map[string]string{"testTag2": "newImageID123"})

+	controller := mockImageChangeController(buildcfg)

+	err := controller.HandleImageRepo(imagerepo)

 	buildCreator := controller.BuildCreator.(*mockBuildCreator)

 	buildConfigUpdater := controller.BuildConfigUpdater.(*mockBuildConfigUpdater)

+	if err != nil {

+		t.Errorf("Unexpected error %v from HandleImageRepo", err)

+	}

 	if buildCreator.build != nil {

 		t.Error("New build created when a different trigger type was defined!")

 	}

@@ -248,11 +281,15 @@ func TestNoImageIDChange(t *testing.T) {

 	// startup when we're checking all the imageRepos

 	buildcfg := mockBuildConfig("registry.com/namespace/imagename", "registry.com/namespace/imagename", "testImageRepo", "testTag")

 	buildcfg.Triggers[0].ImageChange.LastTriggeredImageID = "imageID123"

-	controller := mockImageChangeController(buildcfg, "testImageRepo", "registry.com/namespace/imagename", map[string]string{"testTag": "imageID123"})

-	controller.HandleImageRepo()

+	imagerepo := mockImageRepo("testImageRepo", "registry.com/namespace/imagename", map[string]string{"testTag": "imageID123"})

+	controller := mockImageChangeController(buildcfg)

+	err := controller.HandleImageRepo(imagerepo)

 	buildCreator := controller.BuildCreator.(*mockBuildCreator)

 	buildConfigUpdater := controller.BuildConfigUpdater.(*mockBuildConfigUpdater)

+	if err != nil {

+		t.Errorf("Unexpected error %v from HandleImageRepo", err)

+	}

 	if buildCreator.build != nil {

 		t.Error("New build created when no change happened!")

 	}

@@ -264,12 +301,19 @@ func TestNoImageIDChange(t *testing.T) {

 func TestBuildCreateError(t *testing.T) {

 	// valid configuration, but build creation fails, in that situation the buildconfig should not be updated

 	buildcfg := mockBuildConfig("registry.com/namespace/imagename", "registry.com/namespace/imagename", "testImageRepo", "testTag")

-	controller := mockImageChangeController(buildcfg, "testImageRepo", "registry.com/namespace/imagename", map[string]string{"testTag": "newImageID123"})

+	imagerepo := mockImageRepo("testImageRepo", "registry.com/namespace/imagename", map[string]string{"testTag": "newImageID123"})

+	controller := mockImageChangeController(buildcfg)

 	buildCreator := controller.BuildCreator.(*mockBuildCreator)

 	buildCreator.err = fmt.Errorf("error")

-	controller.HandleImageRepo()

+	err := controller.HandleImageRepo(imagerepo)

 	buildConfigUpdater := controller.BuildConfigUpdater.(*mockBuildConfigUpdater)

+	if err == nil {

+		t.Error("Expected error from HandleImageRepo")

+	}

+	if _, ok := err.(ImageChangeControllerFatalError); ok {

+		t.Error("Expected retryable error from HandleImageRepo")

+	}

 	if buildCreator.build == nil {

 		t.Error("Expected new build when new image was created!")

 	}

@@ -281,14 +325,39 @@ func TestBuildCreateError(t *testing.T) {

 	}

 }

+func TestBuildUpdateError(t *testing.T) {

+	// valid configuration, but build creation fails, in that situation the buildconfig should not be updated

+	buildcfg := mockBuildConfig("registry.com/namespace/imagename", "registry.com/namespace/imagename", "testImageRepo", "testTag")

+	imagerepo := mockImageRepo("testImageRepo", "registry.com/namespace/imagename", map[string]string{"testTag": "newImageID123"})

+	controller := mockImageChangeController(buildcfg)

+	buildCreator := controller.BuildCreator.(*mockBuildCreator)

+	buildConfigUpdater := controller.BuildConfigUpdater.(*mockBuildConfigUpdater)

+	buildConfigUpdater.err = fmt.Errorf("error")

+	err := controller.HandleImageRepo(imagerepo)

+

+	if _, ok := err.(ImageChangeControllerFatalError); !ok {

+		t.Error("Expected fatal error from HandleImageRepo")

+	}

+	if buildCreator.build == nil {

+		t.Error("Expected new build when new image was created!")

+	}

+	if buildCreator.build.Parameters.Strategy.DockerStrategy.Image != "registry.com/namespace/imagename:newImageID123" {

+		t.Errorf("Image substitutions not properly setup for new build.  Expected %s, got %s |", "registry.com/namespace/imagename:newImageID123", buildCreator.build.Parameters.Strategy.DockerStrategy.Image)

+	}

+}

+

 func TestNewImageIDNoDockerRepo(t *testing.T) {

 	// No docker repository associated with the imagerepo, so no build can be created

 	buildcfg := mockBuildConfig("registry.com/namespace/imagename", "registry.com/namespace/imagename", "testImageRepo", "testTag")

-	controller := mockImageChangeController(buildcfg, "testImageRepo", "", map[string]string{"testTag": "newImageID123"})

-	controller.HandleImageRepo()

+	imagerepo := mockImageRepo("testImageRepo", "", map[string]string{"testTag": "newImageID123"})

+	controller := mockImageChangeController(buildcfg)

+	err := controller.HandleImageRepo(imagerepo)

 	buildCreator := controller.BuildCreator.(*mockBuildCreator)

 	buildConfigUpdater := controller.BuildConfigUpdater.(*mockBuildConfigUpdater)

+	if err != nil {

+		t.Errorf("Unexpected error %v from HandleImageRepo", err)

+	}

 	if buildCreator.build != nil {

 		t.Error("New build created when no change happened!")

 	}

@@ -596,6 +596,18 @@ func (c *MasterConfig) RunBuildController() {

 	controller.Run()

 }

+// RunBuildPodController starts the build/pod status sync loop for build status

+func (c *MasterConfig) RunBuildPodController() {

+	osclient, kclient := c.BuildControllerClients()

+	factory := buildcontrollerfactory.BuildPodControllerFactory{

+		OSClient:     osclient,

+		KubeClient:   kclient,

+		BuildUpdater: buildclient.NewOSClientBuildClient(osclient),

+	}

+	controller := factory.Create()

+	controller.Run()

+}

+

 // RunDeploymentController starts the build image change trigger controller process.

 func (c *MasterConfig) RunBuildImageChangeTriggerController() {

 	bcClient, _ := c.BuildControllerClients()

@@ -98,6 +98,7 @@ func (cfg Config) startMaster() error {

 	openshiftConfig.RunAssetServer()

 	openshiftConfig.RunBuildController()

+	openshiftConfig.RunBuildPodController()

 	openshiftConfig.RunBuildImageChangeTriggerController()

 	openshiftConfig.RunDeploymentController()

 	openshiftConfig.RunDeployerPodController()

@@ -203,7 +203,7 @@ func NewTestBuildOpenshift(t *testing.T) *testBuildOpenshift {

 			"github": github.New(),

 		})))

-	factory := buildcontrollerfactory.BuildControllerFactory{

+	bcFactory := buildcontrollerfactory.BuildControllerFactory{

 		OSClient:     osClient,

 		KubeClient:   kubeClient,

 		BuildUpdater: buildclient.NewOSClientBuildClient(osClient),

@@ -219,7 +219,16 @@ func NewTestBuildOpenshift(t *testing.T) *testBuildOpenshift {

 		Stop: openshift.stop,

 	}