@@ -406,6 +406,9 @@ type BuildRequest struct {

 	// Revision is the information from the source for a specific repo snapshot.

 	Revision *SourceRevision `json:"revision,omitempty"`

+

+	// Image is the image that triggered this build.

+	Image string

 }

 // BuildLogOptions is the REST options for a build log

@@ -390,6 +390,9 @@ type BuildRequest struct {

 	// Revision is the information from the source for a specific repo snapshot.

 	Revision *SourceRevision `json:"revision,omitempty"`

+

+	// Image is the image that triggered this build.

+	Image string `json:"image,omitempty"`

 }

 // BuildLogOptions is the REST options for a build log

@@ -455,6 +455,9 @@ type BuildRequest struct {

 	// Revision is the information from the source for a specific repo snapshot.

 	Revision *SourceRevision `json:"revision,omitempty"`

+

+	// Image is the image that triggered this build.

+	Image string `json:"image,omitempty"`

 }

 // BuildLogOptions is the REST options for a build log

@@ -390,6 +390,9 @@ type BuildRequest struct {

 	// Revision is the information from the source for a specific repo snapshot.

 	Revision *SourceRevision `json:"revision,omitempty"`

+

+	// Image is the image that triggered this build.

+	Image string `json:"image,omitempty"`

 }

 // BuildLogOptions is the REST options for a build log

@@ -205,7 +205,6 @@ func (factory *BuildPodControllerFactory) CreateDeleteController() controller.Ru

 type ImageChangeControllerFactory struct {

 	Client                  osclient.Interface

 	BuildConfigInstantiator buildclient.BuildConfigInstantiator

-	BuildConfigUpdater      buildclient.BuildConfigUpdater

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

 	Stop <-chan struct{}

 }

@@ -221,7 +220,6 @@ func (factory *ImageChangeControllerFactory) Create() controller.RunnableControl

 	imageChangeController := &buildcontroller.ImageChangeController{

 		BuildConfigStore:        store,

-		BuildConfigUpdater:      factory.BuildConfigUpdater,

 		BuildConfigInstantiator: factory.BuildConfigInstantiator,

 		Stop: factory.Stop,

 	}

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

 	"fmt"

 	"strings"

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

 	"github.com/golang/glog"

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

@@ -32,7 +33,6 @@ func (e ImageChangeControllerFatalError) Error() string {

 type ImageChangeController struct {

 	BuildConfigStore        cache.Store

 	BuildConfigInstantiator buildclient.BuildConfigInstantiator

-	BuildConfigUpdater      buildclient.BuildConfigUpdater

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

 	Stop <-chan struct{}

 }

@@ -51,11 +51,6 @@ func (c *ImageChangeController) HandleImageRepo(repo *imageapi.ImageStream) erro

 	// TODO: this is inefficient

 	for _, bc := range c.BuildConfigStore.List() {

 		config := bc.(*buildapi.BuildConfig)

-		obj, err := kapi.Scheme.Copy(config)

-		if err != nil {

-			continue

-		}

-		originalConfig := obj.(*buildapi.BuildConfig)

 		from := buildutil.GetImageStreamForStrategy(config.Parameters.Strategy)

 		if from == nil || from.Kind != "ImageStreamTag" {

@@ -63,6 +58,7 @@ func (c *ImageChangeController) HandleImageRepo(repo *imageapi.ImageStream) erro

 		}

 		shouldBuild := false

+		triggeredImage := ""

 		// For every ImageChange trigger find the latest tagged image from the image repository and replace that value

 		// throughout the build strategies. A new build is triggered only if the latest tagged image id or pull spec

 		// differs from the last triggered build recorded on the build config.

@@ -99,68 +95,37 @@ func (c *ImageChangeController) HandleImageRepo(repo *imageapi.ImageStream) erro

 			next := latest.DockerImageReference

 			if len(last) == 0 || (len(next) > 0 && next != last) {

-				trigger.ImageChange.LastTriggeredImageID = next

+				triggeredImage = next

 				shouldBuild = true

+				// it doesn't really make sense to have multiple image change triggers any more,

+				// so just exit the loop now

+				break

 			}

 		}

 		if shouldBuild {

-			// The following update is meant to reduce the chance that the image change controller

-			// will kick off multiple builds on an image change in a HA setup, where multiple controllers

-			// of the same type may be looking at the same etcd data.

-			// If multiple controllers read the same build config (with same ResourceVersion) above and

-			// make a determination that a build needs to be kicked off, the update will only allow one of

-			// those controllers to continue to launch the build, while the rest will return an error and

-			// reset their queue. This won't eliminate the chance of multiple builds, since another controller

-			// can read the build after this update and launch its own build.

-			// TODO: Find a better mechanism to synchronize in a HA setup.

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

-				// Cannot make an update to the original build config. Likely it has been changed by another process

-				glog.V(4).Infof("Cannot update BuildConfig %s/%s when preparing to update LastTriggeredImageID: %v", config.Namespace, config.Name, err)

-				return err

-			}

-

 			glog.V(4).Infof("Running build for BuildConfig %s/%s", config.Namespace, config.Name)

 			// instantiate new build

-			request := &buildapi.BuildRequest{ObjectMeta: kapi.ObjectMeta{Name: config.Name}}

-			if _, err := c.BuildConfigInstantiator.Instantiate(config.Namespace, request); err != nil {

-				return fmt.Errorf("error instantiating Build from BuildConfig %s/%s: %v", config.Namespace, config.Name, err)

+			request := &buildapi.BuildRequest{

+				ObjectMeta: kapi.ObjectMeta{

+					Name: config.Name,

+				},

+				Image: triggeredImage,

 			}

-			// and update the config

-			if err := c.updateConfig(config); err != nil {

-				// This is not a retryable error. 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.

-				return ImageChangeControllerFatalError{

-					Reason: fmt.Sprintf("error updating BuildConfig %s/%s with new LastTriggeredImageID", config.Namespace, config.Name),

-					Err:    err,

+			if _, err := c.BuildConfigInstantiator.Instantiate(config.Namespace, request); err != nil {

+				if kerrors.IsConflict(err) {

+					// This is not a retryable error. 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.

+					return ImageChangeControllerFatalError{

+						Reason: fmt.Sprintf("unable to instantiate Build for BuildConfig %s/%s due to a conflicting update: %v", config.Namespace, config.Name, err),

+						Err:    err,

+					}

 				}

+

+				return fmt.Errorf("error instantiating Build from BuildConfig %s/%s: %v", config.Namespace, config.Name, err)

 			}

 		}

 	}

 	return nil

 }

-

-// updateConfig is responsible for updating current BuildConfig object which was changed

-// during instantiate call, it basically copies LastTriggeredImageID to fresh copy

-// of the BuildConfig object

-func (c *ImageChangeController) updateConfig(config *buildapi.BuildConfig) error {

-	item, _, err := c.BuildConfigStore.Get(config)

-	if err != nil {

-		return err

-	}

-	if item == nil {

-		return fmt.Errorf("unable to retrieve BuildConfig %s/%s for updating", config.Namespace, config.Name)

-	}

-	newConfig := item.(*buildapi.BuildConfig)

-	for i, trigger := range newConfig.Triggers {

-		if trigger.Type != buildapi.ImageChangeBuildTriggerType {

-			continue

-		}

-		change := trigger.ImageChange

-		change.LastTriggeredImageID = config.Triggers[i].ImageChange.LastTriggeredImageID

-	}

-	glog.V(4).Infof("BuildConfig %s/%s is about to be updated", config.Namespace, config.Name)

-

-	return c.BuildConfigUpdater.Update(newConfig)

-}

@@ -1,11 +1,13 @@

 package controller

 import (

+	"errors"

 	"fmt"

 	"strings"

 	"testing"

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

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

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

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

@@ -22,7 +24,7 @@ func TestNewImageID(t *testing.T) {

 	image := mockImage("testImage@id", "registry.com/namespace/imagename:newImageID123")

 	controller := mockImageChangeController(buildcfg, imageStream, image)

 	bcInstantiator := controller.BuildConfigInstantiator.(*buildConfigInstantiator)

-	bcUpdater := controller.BuildConfigUpdater.(*mockBuildConfigUpdater)

+	bcUpdater := bcInstantiator.buildConfigUpdater

 	err := controller.HandleImageRepo(imageStream)

 	if err != nil {

@@ -50,7 +52,7 @@ func TestNewImageIDDefaultTag(t *testing.T) {

 	image := mockImage("testImage@id", "registry.com/namespace/imagename:newImageID123")

 	controller := mockImageChangeController(buildcfg, imageStream, image)

 	bcInstantiator := controller.BuildConfigInstantiator.(*buildConfigInstantiator)

-	bcUpdater := controller.BuildConfigUpdater.(*mockBuildConfigUpdater)

+	bcUpdater := bcInstantiator.buildConfigUpdater

 	err := controller.HandleImageRepo(imageStream)

 	if err != nil {

@@ -78,7 +80,7 @@ func TestNonExistentImageStream(t *testing.T) {

 	image := mockImage("testImage@id", "registry.com/namespace/imagename@id")

 	controller := mockImageChangeController(buildcfg, imageStream, image)

 	bcInstantiator := controller.BuildConfigInstantiator.(*buildConfigInstantiator)

-	bcUpdater := controller.BuildConfigUpdater.(*mockBuildConfigUpdater)

+	bcUpdater := bcInstantiator.buildConfigUpdater

 	err := controller.HandleImageRepo(imageStream)

 	if err != nil {

@@ -99,7 +101,7 @@ func TestNewImageDifferentTagUpdate(t *testing.T) {

 	image := mockImage("testImage@id", "registry.com/namespace/imagename@id")

 	controller := mockImageChangeController(buildcfg, imageStream, image)

 	bcInstantiator := controller.BuildConfigInstantiator.(*buildConfigInstantiator)

-	bcUpdater := controller.BuildConfigUpdater.(*mockBuildConfigUpdater)

+	bcUpdater := bcInstantiator.buildConfigUpdater

 	err := controller.HandleImageRepo(imageStream)

 	if err != nil {

@@ -122,7 +124,7 @@ func TestNewImageDifferentTagUpdate2(t *testing.T) {

 	image := mockImage("testImage@id", "registry.com/namespace/imagename@id")

 	controller := mockImageChangeController(buildcfg, imageStream, image)

 	bcInstantiator := controller.BuildConfigInstantiator.(*buildConfigInstantiator)

-	bcUpdater := controller.BuildConfigUpdater.(*mockBuildConfigUpdater)

+	bcUpdater := bcInstantiator.buildConfigUpdater

 	err := controller.HandleImageRepo(imageStream)

 	if err != nil {

@@ -143,7 +145,7 @@ func TestNewDifferentImageUpdate(t *testing.T) {

 	image := mockImage("testImage@id", "registry.com/namespace/imagename@id")

 	controller := mockImageChangeController(buildcfg, imageStream, image)

 	bcInstantiator := controller.BuildConfigInstantiator.(*buildConfigInstantiator)

-	bcUpdater := controller.BuildConfigUpdater.(*mockBuildConfigUpdater)

+	bcUpdater := bcInstantiator.buildConfigUpdater

 	err := controller.HandleImageRepo(imageStream)

 	if err != nil {

@@ -166,7 +168,7 @@ func TestSameStreamNameDifferentNamespaces(t *testing.T) {

 	image := mockImage("testImage@id", "registry.com/namespace/imagename@id")

 	controller := mockImageChangeController(buildcfg, imageStream, image)

 	bcInstantiator := controller.BuildConfigInstantiator.(*buildConfigInstantiator)

-	bcUpdater := controller.BuildConfigUpdater.(*mockBuildConfigUpdater)

+	bcUpdater := bcInstantiator.buildConfigUpdater

 	err := controller.HandleImageRepo(imageStream)

 	if err != nil {

@@ -188,7 +190,7 @@ func TestBuildConfigWithDifferentTriggerType(t *testing.T) {

 	image := mockImage("testImage@id", "registry.com/namespace/imagename@id")

 	controller := mockImageChangeController(buildcfg, imageStream, image)

 	bcInstantiator := controller.BuildConfigInstantiator.(*buildConfigInstantiator)

-	bcUpdater := controller.BuildConfigUpdater.(*mockBuildConfigUpdater)

+	bcUpdater := bcInstantiator.buildConfigUpdater

 	err := controller.HandleImageRepo(imageStream)

 	if err != nil {

@@ -211,7 +213,7 @@ func TestNoImageIDChange(t *testing.T) {

 	image := mockImage("testImage@id", "registry.com/namespace/imagename@id")

 	controller := mockImageChangeController(buildcfg, imageStream, image)

 	bcInstantiator := controller.BuildConfigInstantiator.(*buildConfigInstantiator)

-	bcUpdater := controller.BuildConfigUpdater.(*mockBuildConfigUpdater)

+	bcUpdater := bcInstantiator.buildConfigUpdater

 	err := controller.HandleImageRepo(imageStream)

 	if err != nil {

@@ -233,7 +235,7 @@ func TestBuildConfigInstantiatorError(t *testing.T) {

 	controller := mockImageChangeController(buildcfg, imageStream, image)

 	bcInstantiator := controller.BuildConfigInstantiator.(*buildConfigInstantiator)

 	bcInstantiator.err = fmt.Errorf("instantiating error")

-	bcUpdater := controller.BuildConfigUpdater.(*mockBuildConfigUpdater)

+	bcUpdater := bcInstantiator.buildConfigUpdater

 	err := controller.HandleImageRepo(imageStream)

 	if err == nil || !strings.Contains(err.Error(), "instantiating error") {

@@ -254,9 +256,8 @@ func TestBuildConfigUpdateError(t *testing.T) {

 	image := mockImage("testImage@id", "registry.com/namespace/imagename@id")

 	controller := mockImageChangeController(buildcfg, imageStream, image)

 	bcInstantiator := controller.BuildConfigInstantiator.(*buildConfigInstantiator)

-	bcUpdater := controller.BuildConfigUpdater.(*mockBuildConfigUpdater)

-	bcUpdater.err = fmt.Errorf("error")

-	bcUpdater.errUpdateCount = 2

+	bcUpdater := bcInstantiator.buildConfigUpdater

+	bcUpdater.err = kerrors.NewConflict("BuildConfig", buildcfg.Name, errors.New("foo"))

 	err := controller.HandleImageRepo(imageStream)

 	if len(bcInstantiator.name) == 0 {

@@ -274,7 +275,7 @@ func TestNewImageIDNoDockerRepo(t *testing.T) {

 	image := mockImage("testImage@id", "registry.com/namespace/imagename@id")

 	controller := mockImageChangeController(buildcfg, imageStream, image)

 	bcInstantiator := controller.BuildConfigInstantiator.(*buildConfigInstantiator)

-	bcUpdater := controller.BuildConfigUpdater.(*mockBuildConfigUpdater)

+	bcUpdater := bcInstantiator.buildConfigUpdater

 	err := controller.HandleImageRepo(imageStream)

 	if err != nil {

@@ -289,21 +290,14 @@ func TestNewImageIDNoDockerRepo(t *testing.T) {

 }

 type mockBuildConfigUpdater struct {

-	updateCount    int

-	buildcfg       *buildapi.BuildConfig

-	err            error

-	errUpdateCount int

+	updateCount int

+	buildcfg    *buildapi.BuildConfig

+	err         error

 }

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

 	m.buildcfg = buildcfg

 	m.updateCount++

-	if m.errUpdateCount > 0 {

-		if m.updateCount == m.errUpdateCount {

-			return m.err

-		}

-		return nil

-	}

 	return m.err

 }

@@ -366,10 +360,11 @@ func mockImage(name, dockerSpec string) *imageapi.Image {

 }

 type buildConfigInstantiator struct {

-	generator buildgenerator.BuildGenerator

-	name      string

-	newBuild  *buildapi.Build

-	err       error

+	generator          buildgenerator.BuildGenerator

+	buildConfigUpdater *mockBuildConfigUpdater

+	name               string

+	newBuild           *buildapi.Build

+	err                error

 }

 func (i *buildConfigInstantiator) Instantiate(namespace string, request *buildapi.BuildRequest) (*buildapi.Build, error) {

@@ -383,6 +378,7 @@ func mockBuildConfigInstantiator(buildcfg *buildapi.BuildConfig, imageStream *im

 		Secrets:    []kapi.ObjectReference{},

 	}

 	instantiator := &buildConfigInstantiator{}

+	instantiator.buildConfigUpdater = &mockBuildConfigUpdater{}

 	generator := buildgenerator.BuildGenerator{

 		Secrets:         testclient.NewSimpleFake(),

 		ServiceAccounts: testclient.NewSimpleFake(&builderAccount),

@@ -391,7 +387,7 @@ func mockBuildConfigInstantiator(buildcfg *buildapi.BuildConfig, imageStream *im

 				return buildcfg, nil

 			},

 			UpdateBuildConfigFunc: func(ctx kapi.Context, buildConfig *buildapi.BuildConfig) error {

-				return nil

+				return instantiator.buildConfigUpdater.Update(buildConfig)

 			},

 			CreateBuildFunc: func(ctx kapi.Context, build *buildapi.Build) error {

 				instantiator.newBuild = build

@@ -418,6 +414,5 @@ func mockImageChangeController(buildcfg *buildapi.BuildConfig, imageStream *imag

 	return &ImageChangeController{

 		BuildConfigStore:        buildtest.NewFakeBuildConfigStore(buildcfg),

 		BuildConfigInstantiator: mockBuildConfigInstantiator(buildcfg, imageStream, image),

-		BuildConfigUpdater:      &mockBuildConfigUpdater{},

 	}

 }

@@ -127,8 +127,29 @@ func (g *BuildGenerator) Instantiate(ctx kapi.Context, request *buildapi.BuildRe

 	if err != nil {

 		return nil, err

 	}

+

 	glog.V(4).Infof("Build %s/%s has been generated from %s/%s BuildConfig", newBuild.Namespace, newBuild.ObjectMeta.Name, bc.Namespace, bc.ObjectMeta.Name)

-	return g.createBuild(ctx, newBuild)

+	updatedBuild, err := g.createBuild(ctx, newBuild)

+	if err != nil {

+		return nil, err

+	}

+

+	if len(request.Image) > 0 {

+		for _, trigger := range bc.Triggers {

+			if trigger.Type != buildapi.ImageChangeBuildTriggerType {

+				continue

+			}

+

+			trigger.ImageChange.LastTriggeredImageID = request.Image

+		}

+	}

+

+	// need to update the BuildConfig because LastVersion and possibly LastTriggeredImageID changed

+	if err := g.Client.UpdateBuildConfig(ctx, bc); err != nil {

+		return nil, err

+	}

+

+	return updatedBuild, nil

 }

 // Clone returns clone of a Build

@@ -162,10 +183,6 @@ func (g *BuildGenerator) createBuild(ctx kapi.Context, build *buildapi.Build) (*

 // the Strategy, or uses the Image field of the Strategy.

 // Takes a BuildConfig to base the build on, and an optional SourceRevision to build.

 func (g *BuildGenerator) generateBuildFromConfig(ctx kapi.Context, bc *buildapi.BuildConfig, revision *buildapi.SourceRevision) (*buildapi.Build, error) {

-	builderSecrets, err := g.FetchServiceAccountSecrets(bc.Namespace)

-	if err != nil {

-		return nil, err

-	}

 	// Need to copy the buildConfig here so that it doesn't share pointers with

 	// the build object which could be (will be) modified later.

 	obj, _ := kapi.Scheme.Copy(bc)

@@ -187,14 +204,15 @@ func (g *BuildGenerator) generateBuildFromConfig(ctx kapi.Context, bc *buildapi.

 	build.Config = &kapi.ObjectReference{Kind: "BuildConfig", Name: bc.Name, Namespace: bc.Namespace}

 	build.Name = getNextBuildName(bc)

-	if err := g.Client.UpdateBuildConfig(ctx, bc); err != nil {

-		return nil, err

-	}

 	if build.Labels == nil {

 		build.Labels = make(map[string]string)

 	}

 	build.Labels[buildapi.BuildConfigLabel] = bcCopy.Name

+	builderSecrets, err := g.FetchServiceAccountSecrets(bc.Namespace)

+	if err != nil {

+		return nil, err

+	}

 	if build.Parameters.Output.PushSecret == nil {

 		build.Parameters.Output.PushSecret = g.resolveImageSecret(ctx, builderSecrets, build.Parameters.Output.To, bc.Namespace)

 	}

@@ -57,6 +57,14 @@ func TestInstantiate(t *testing.T) {

 	}

 }

+// agoldste: I'm not sure the intent of this test. Using the previous logic for

+// the generator, which would try to update the build config before creating

+// the build, I can see why the UpdateBuildConfigFunc is set up to return an

+// error, but nothing is checking the value of instantiationCalls. We could

+// update this test to fail sooner, when the build is created, but that's

+// already handled by TestCreateBuildCreateError. We may just want to delete

+// this test.

+/*

 func TestInstantiateRetry(t *testing.T) {

 	instantiationCalls := 0

 	fakeSecrets := []runtime.Object{}

@@ -80,8 +88,8 @@ func TestInstantiateRetry(t *testing.T) {

 	if err == nil || !strings.Contains(err.Error(), "update-error") {

 		t.Errorf("Expected update-error, got different %v", err)

 	}

-

 }

+*/

 func TestInstantiateGetBuildConfigError(t *testing.T) {

 	generator := BuildGenerator{Client: Client{

@@ -859,9 +859,8 @@ func (c *MasterConfig) RunBuildPodController() {

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

 func (c *MasterConfig) RunBuildImageChangeTriggerController() {

 	bcClient, _ := c.BuildControllerClients()

-	bcUpdater := buildclient.NewOSClientBuildConfigClient(bcClient)

 	bcInstantiator := buildclient.NewOSClientBuildConfigInstantiatorClient(bcClient)

-	factory := buildcontrollerfactory.ImageChangeControllerFactory{Client: bcClient, BuildConfigInstantiator: bcInstantiator, BuildConfigUpdater: bcUpdater}

+	factory := buildcontrollerfactory.ImageChangeControllerFactory{Client: bcClient, BuildConfigInstantiator: bcInstantiator}

 	factory.Create().Run()

 }