Merge pull request #1 from soltysh/card282

Added startup handling for ImageChangeController

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)
+ }
  }
  }
  }