Merge pull request #2215 from csrwng/build_controller_ha

Merged by openshift-bot

Author: OpenShift Bot

pkg/build/controller/image_change_controller.go

@@ -51,6 +51,12 @@ 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)
 		if from == nil || from.Kind != "ImageStreamTag" {
 			continue
@@ -98,6 +104,21 @@ func (c *ImageChangeController) HandleImageRepo(repo *imageapi.ImageStream) erro
 		}
 
 		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 build config %s when preparing to update LastTriggeredImageID: %v", config.Name, err)
+				return err
+			}
+
 			glog.V(4).Infof("Running build for buildConfig %s in namespace %s", config.Name, config.Namespace)
 			// instantiate new build
 			request := &buildapi.BuildRequest{ObjectMeta: kapi.ObjectMeta{Name: config.Name}}

pkg/build/controller/image_change_controller_test.go

@@ -240,7 +240,7 @@ func TestBuildConfigInstantiatorError(t *testing.T) {
 	if actual, expected := bcInstantiator.newBuild.Parameters.Strategy.DockerStrategy.From.Name, "registry.com/namespace/imagename:newImageID123"; actual != expected {
 		t.Errorf("Image substitutions not properly setup for new build. Expected %s, got %s |", expected, actual)
 	}
-	if bcUpdater.buildcfg != nil {
+	if bcUpdater.updateCount > 1 {
 		t.Fatal("Expected no buildConfig update on BuildCreate error!")
 	}
 }
@@ -254,6 +254,7 @@ func TestBuildConfigUpdateError(t *testing.T) {
 	bcInstantiator := controller.BuildConfigInstantiator.(*buildConfigInstantiator)
 	bcUpdater := controller.BuildConfigUpdater.(*mockBuildConfigUpdater)
 	bcUpdater.err = fmt.Errorf("error")
+	bcUpdater.errUpdateCount = 2
 
 	err := controller.HandleImageRepo(imageStream)
 	if len(bcInstantiator.name) == 0 {
@@ -286,12 +287,21 @@ func TestNewImageIDNoDockerRepo(t *testing.T) {
 }
 
 type mockBuildConfigUpdater struct {
-	buildcfg *buildapi.BuildConfig
-	err      error
+	updateCount    int
+	buildcfg       *buildapi.BuildConfig
+	err            error
+	errUpdateCount int
 }
 
 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
 }