Fix build image change controller race

pkg/build/api/types.go

@@ -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

pkg/build/api/v1/types.go

@@ -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

pkg/build/api/v1beta1/types.go

@@ -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

pkg/build/api/v1beta3/types.go

@@ -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

pkg/build/controller/factory/factory.go

@@ -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,
 	}

pkg/build/controller/image_change_controller.go

@@ -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,18 +51,14 @@ 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" {
 			continue
 		}
 
 		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)
-}

pkg/build/controller/image_change_controller_test.go

@@ -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{},
 	}
 }