Merge pull request #1 from soltysh/card282

Added startup handling for ImageChangeController

Author: bparees

pkg/build/api/types.go

@@ -245,6 +245,9 @@ type ImageChangeTrigger struct {
 	ImageRepositoryRef *kapi.ObjectReference `json:"imageRepositoryRef" yaml:"imageRepositoryRef"`
 	// Tag is the name of an image repository tag to watch for changes.
 	Tag string `json:"tag,omitempty" yaml:"tag,omitempty"`
+	// LastTriggeredImageID is used internally by the ImageChangeController to save last
+	// used image ID for build
+	LastTriggeredImageID string `json:"lastTriggeredImageID,omitempty" yaml:"lastTriggeredImageID,omitempty"`
 }
 
 // BuildTriggerPolicy describes a policy for a single trigger that results in a new Build.

pkg/build/api/v1beta1/types.go

@@ -240,6 +240,9 @@ type ImageChangeTrigger struct {
 	ImageRepositoryRef *kapi.ObjectReference `json:"imageRepositoryRef" yaml:"imageRepositoryRef"`
 	// Tag is the name of an image repository tag to watch for changes.
 	Tag string `json:"tag,omitempty" yaml:"tag,omitempty"`
+	// LastTriggeredImageID is used internally by the ImageChangeController to save last
+	// used image ID for build
+	LastTriggeredImageID string `json:"lastTriggeredImageID,omitempty" yaml:"lastTriggeredImageID,omitempty"`
 }
 
 // BuildTriggerPolicy describes a policy for a single trigger that results in a new Build.

pkg/build/controller/factory/factory.go

@@ -85,8 +85,9 @@ func (factory *ImageChangeControllerFactory) Create() *controller.ImageChangeCon
 	cache.NewReflector(&buildConfigLW{client: factory.Client}, &buildapi.BuildConfig{}, store).Run()
 
 	return &controller.ImageChangeController{
-		BuildConfigStore: store,
-		BuildCreator:     &ClientBuildCreator{factory.Client},
+		BuildConfigStore:   store,
+		BuildConfigUpdater: &ClientBuildConfigUpdater{factory.Client},
+		BuildCreator:       &ClientBuildCreator{factory.Client},
 		NextImageRepository: func() *imageapi.ImageRepository {
 			repo := queue.Pop().(*imageapi.ImageRepository)
 			panicIfStopped(factory.Stop, "build image change controller stopped")
@@ -219,7 +220,12 @@ func (c ClientPodManager) CreatePod(namespace string, pod *kapi.Pod) (*kapi.Pod,
 	return c.KubeClient.Pods(namespace).Create(pod)
 }
 
-// ClientBuildUpdater is a buildUpdater which delegates to the OpenShift client interfaces
+// DeletePod destroys a pod using the Kubernetes client.
+func (c ClientPodManager) DeletePod(namespace string, pod *kapi.Pod) error {
+	return c.KubeClient.Pods(namespace).Delete(pod.Name)
+}
+
+// ClientBuildUpdater is a buildUpdater which delegates to the OpenShift client interfaces.
 type ClientBuildUpdater struct {
 	Client osclient.Interface
 }
@@ -229,7 +235,7 @@ func (c ClientBuildUpdater) UpdateBuild(namespace string, build *buildapi.Build)
 	return c.Client.Builds(namespace).Update(build)
 }
 
-// ClientBuildCreator is a buildCreator which delegates to the OpenShift client interfaces
+// ClientBuildCreator is a buildCreator which delegates to the OpenShift client interfaces.
 type ClientBuildCreator struct {
 	Client osclient.Interface
 }
@@ -247,7 +253,13 @@ func (c *ClientBuildCreator) CreateBuild(config *buildapi.BuildConfig, imageSubs
 	return nil
 }
 
-// DeletePod destroys a pod using the Kubernetes client.
-func (c ClientPodManager) DeletePod(namespace string, pod *kapi.Pod) error {
-	return c.KubeClient.Pods(namespace).Delete(pod.Name)
+// ClientBuildConfigUpdater is a buildConfigUpdater which delegates to the OpenShift client interfaces.
+type ClientBuildConfigUpdater struct {
+	Client osclient.Interface
+}
+
+// UpdateBuildConfig updates buildConfig using the OpenShift client.
+func (c *ClientBuildConfigUpdater) UpdateBuildConfig(buildConfig *buildapi.BuildConfig) error {
+	_, err := c.Client.BuildConfigs(buildConfig.Namespace).Update(buildConfig)
+	return err
 }

pkg/build/controller/image_change_controller.go

@@ -16,11 +16,16 @@ import (
 type ImageChangeController struct {
 	NextImageRepository func() *imageapi.ImageRepository
 	BuildConfigStore    cache.Store
+	BuildConfigUpdater  buildConfigUpdater
 	BuildCreator        buildCreator
 	// Stop is an optional channel that controls when the controller exits
 	Stop <-chan struct{}
 }
 
+type buildConfigUpdater interface {
+	UpdateBuildConfig(buildConfig *buildapi.BuildConfig) error
+}
+
 type buildCreator interface {
 	CreateBuild(build *buildapi.BuildConfig, imageSubstitutions map[string]string) error
 }
@@ -42,33 +47,47 @@ func (c *ImageChangeController) HandleImageRepo() {
 		glog.V(4).Infof("Detecting changed images for buildConfig %s", config.Name)
 
 		// Extract relevant triggers for this imageRepo for this config
-		var triggerForConfig *buildapi.ImageChangeTrigger
+		shouldTriggerBuild := false
 		for _, trigger := range config.Triggers {
+			if trigger.Type != buildapi.ImageChangeBuildTriggerType {
+				continue
+			}
 			// for every ImageChange trigger, record the image it substitutes for and get the latest
 			// image id from the imagerepository.  We will substitute all images in the buildconfig
 			// with the latest values from the imagerepositories.
-			if trigger.Type == buildapi.ImageChangeBuildTriggerType {
-				// TODO: we don't really want to create a build for a buildconfig based the "test" tag if the "prod" tag is what just got
-				// updated, but ImageRepository doesn't give us that granularity today, so the only way to avoid these spurious builds is
-				// to check if the new imageid is different from the last time we built this buildcfg.  Need to add this check.
-				// Will be effectively identical the logic needed on startup to spin new builds only if we missed a new image event.
-				var tag string
-				if tag = trigger.ImageChange.Tag; len(tag) == 0 {
-					tag = buildapi.DefaultImageTag
-				}
-				if repoImageID, repoHasTag := imageRepo.Tags[tag]; repoHasTag {
-					imageSubstitutions[trigger.ImageChange.Image] = imageRepo.DockerImageRepository + ":" + repoImageID
-				}
-				if trigger.ImageChange.ImageRepositoryRef.Name == imageRepo.Name {
-					triggerForConfig = trigger.ImageChange
-				}
+			icTrigger := trigger.ImageChange
+
+			// TODO: we don't really want to create a build for a buildconfig based the "test" tag if the "prod" tag is what just got
+			// updated, but ImageRepository doesn't give us that granularity today, so the only way to avoid these spurious builds is
+			// to check if the new imageid is different from the last time we built this buildcfg.  Need to add this check.
+			// Will be effectively identical the logic needed on startup to spin new builds only if we missed a new image event.
+			tag := icTrigger.Tag
+			if len(tag) == 0 {
+				tag = buildapi.DefaultImageTag
+			}
+			imageID, hasTag := imageRepo.Tags[tag]
+			if !hasTag {
+				continue
+			}
+
+			// comparison requires us to match Name of the image and LastTriggeredImageID
+			// (must be different) to trigger a build
+			if icTrigger.ImageRepositoryRef.Name == imageRepo.Name &&
+				icTrigger.LastTriggeredImageID != imageID {
+				imageSubstitutions[icTrigger.Image] = imageRepo.DockerImageRepository + ":" + imageID
+				shouldTriggerBuild = true
+				icTrigger.LastTriggeredImageID = imageID
 			}
 		}
 
-		if triggerForConfig != nil {
+		if shouldTriggerBuild {
 			glog.V(4).Infof("Running build for buildConfig %s", config.Name)
 			if err := c.BuildCreator.CreateBuild(config, imageSubstitutions); err != nil {
 				glog.V(2).Infof("Error starting build for buildConfig %v: %v", config.Name, err)
+			} else {
+				if err := c.BuildConfigUpdater.UpdateBuildConfig(config); err != nil {
+					glog.V(2).Infof("Error updating buildConfig %v: %v", config.Name, err)
+				}
 			}
 		}
 	}