Make the trigger controllers cooperate instead of race

api/swagger-spec/oapi-v1.json

@@ -19457,7 +19457,7 @@
     "properties": {
      "automatic": {
       "type": "boolean",
-      "description": "Automatic means that the detection of a new tag value should result in a new deployment."
+      "description": "Automatic means that the detection of a new tag value should result in an image update inside the pod template. Deployment configs that haven't been deployed yet will always have their images updated. Deployment configs that have been deployed at least once, will have their images updated only if this is set to true."
      },
      "containerNames": {
       "type": "array",

pkg/deploy/api/test/ok.go

@@ -9,6 +9,22 @@ import (
 	imageapi "github.com/openshift/origin/pkg/image/api"
 )
 
+const (
+	ImageStreamName      = "test-image-stream"
+	ImageID              = "00000000000000000000000000000001"
+	DockerImageReference = "registry:5000/openshift/test-image-stream@sha256:00000000000000000000000000000001"
+)
+
+func OkDeploymentConfig(version int) *deployapi.DeploymentConfig {
+	return &deployapi.DeploymentConfig{
+		ObjectMeta: kapi.ObjectMeta{
+			Name: "config",
+		},
+		Spec:   OkDeploymentConfigSpec(),
+		Status: OkDeploymentConfigStatus(version),
+	}
+}
+
 func OkDeploymentConfigSpec() deployapi.DeploymentConfigSpec {
 	return deployapi.DeploymentConfigSpec{
 		Replicas: 1,
@@ -17,6 +33,7 @@ func OkDeploymentConfigSpec() deployapi.DeploymentConfigSpec {
 		Template: OkPodTemplate(),
 		Triggers: []deployapi.DeploymentTriggerPolicy{
 			OkImageChangeTrigger(),
+			OkConfigChangeTrigger(),
 		},
 	}
 }
@@ -33,7 +50,7 @@ func OkImageChangeDetails() *deployapi.DeploymentDetails {
 			Type: deployapi.DeploymentTriggerOnImageChange,
 			ImageTrigger: &deployapi.DeploymentCauseImageTrigger{
 				From: kapi.ObjectReference{
-					Name: imageapi.JoinImageStreamTag("test-image-stream", imageapi.DefaultImageTag),
+					Name: imageapi.JoinImageStreamTag(ImageStreamName, imageapi.DefaultImageTag),
 					Kind: "ImageStreamTag",
 				}}}}}
 }
@@ -158,22 +175,12 @@ func OkImageChangeTrigger() deployapi.DeploymentTriggerPolicy {
 			},
 			From: kapi.ObjectReference{
 				Kind: "ImageStreamTag",
-				Name: imageapi.JoinImageStreamTag("test-image-stream", imageapi.DefaultImageTag),
+				Name: imageapi.JoinImageStreamTag(ImageStreamName, imageapi.DefaultImageTag),
 			},
 		},
 	}
 }
 
-func OkDeploymentConfig(version int) *deployapi.DeploymentConfig {
-	return &deployapi.DeploymentConfig{
-		ObjectMeta: kapi.ObjectMeta{
-			Name: "config",
-		},
-		Spec:   OkDeploymentConfigSpec(),
-		Status: OkDeploymentConfigStatus(version),
-	}
-}
-
 func TestDeploymentConfig(config *deployapi.DeploymentConfig) *deployapi.DeploymentConfig {
 	config.Spec.Test = true
 	return config

pkg/deploy/api/types.go

@@ -355,7 +355,10 @@ const (
 
 // DeploymentTriggerImageChangeParams represents the parameters to the ImageChange trigger.
 type DeploymentTriggerImageChangeParams struct {
-	// Automatic means that the detection of a new tag value should result in a new deployment.
+	// Automatic means that the detection of a new tag value should result in an image update
+	// inside the pod template. Deployment configs that haven't been deployed yet will always
+	// have their images updated. Deployment configs that have been deployed at least once, will
+	// have their images updated only if this is set to true.
 	Automatic bool
 	// ContainerNames is used to restrict tag updates to the specified set of container names in a pod.
 	ContainerNames []string

pkg/deploy/api/v1/defaults.go

@@ -94,9 +94,14 @@ func addDefaultingFuncs(scheme *runtime.Scheme) {
 				}
 			}
 		},
-		func(obj *DeploymentTriggerImageChangeParams) {
-			if len(obj.From.Kind) == 0 {
-				obj.From.Kind = "ImageStreamTag"
+		func(obj *DeploymentConfig) {
+			for _, t := range obj.Spec.Triggers {
+				if t.ImageChangeParams != nil {
+					t.ImageChangeParams.From.Kind = "ImageStreamTag"
+					if len(t.ImageChangeParams.From.Name) > 0 && len(t.ImageChangeParams.From.Namespace) == 0 {
+						t.ImageChangeParams.From.Namespace = obj.Namespace
+					}
+				}
 			}
 		},
 	)

pkg/deploy/api/v1/swagger_doc.go

@@ -156,7 +156,7 @@ func (DeploymentStrategy) SwaggerDoc() map[string]string {
 
 var map_DeploymentTriggerImageChangeParams = map[string]string{
 	"":                   "DeploymentTriggerImageChangeParams represents the parameters to the ImageChange trigger.",
-	"automatic":          "Automatic means that the detection of a new tag value should result in a new deployment.",
+	"automatic":          "Automatic means that the detection of a new tag value should result in an image update inside the pod template. Deployment configs that haven't been deployed yet will always have their images updated. Deployment configs that have been deployed at least once, will have their images updated only if this is set to true.",
 	"containerNames":     "ContainerNames is used to restrict tag updates to the specified set of container names in a pod.",
 	"from":               "From is a reference to an image stream tag to watch for changes. From.Name is the only required subfield - if From.Namespace is blank, the namespace of the current deployment trigger will be used.",
 	"lastTriggeredImage": "LastTriggeredImage is the last image to be triggered.",

pkg/deploy/api/v1/types.go

@@ -310,7 +310,10 @@ const (
 
 // DeploymentTriggerImageChangeParams represents the parameters to the ImageChange trigger.
 type DeploymentTriggerImageChangeParams struct {
-	// Automatic means that the detection of a new tag value should result in a new deployment.
+	// Automatic means that the detection of a new tag value should result in an image update
+	// inside the pod template. Deployment configs that haven't been deployed yet will always
+	// have their images updated. Deployment configs that have been deployed at least once, will
+	// have their images updated only if this is set to true.
 	Automatic bool `json:"automatic,omitempty"`
 	// ContainerNames is used to restrict tag updates to the specified set of container names in a pod.
 	ContainerNames []string `json:"containerNames,omitempty"`

pkg/deploy/api/v1beta3/types.go

@@ -324,7 +324,10 @@ const (
 
 // DeploymentTriggerImageChangeParams represents the parameters to the ImageChange trigger.
 type DeploymentTriggerImageChangeParams struct {
-	// Automatic means that the detection of a new tag value should result in a new deployment.
+	// Automatic means that the detection of a new tag value should result in an image update
+	// inside the pod template. Deployment configs that haven't been deployed yet will always
+	// have their images updated. Deployment configs that have been deployed at least once, will
+	// have their images updated only if this is set to true.
 	Automatic bool `json:"automatic,omitempty"`
 	// ContainerNames is used to restrict tag updates to the specified set of container names in a pod.
 	ContainerNames []string `json:"containerNames,omitempty"`

pkg/deploy/controller/configchange/controller.go

@@ -6,7 +6,6 @@ import (
 	"github.com/golang/glog"
 
 	kapi "k8s.io/kubernetes/pkg/api"
-	kerrors "k8s.io/kubernetes/pkg/api/errors"
 	kclient "k8s.io/kubernetes/pkg/client/unversioned"
 
 	osclient "github.com/openshift/origin/pkg/client"
@@ -41,94 +40,124 @@ func (c *DeploymentConfigChangeController) Handle(config *deployapi.DeploymentCo
 		return nil
 	}
 
+	// Try to decode this deployment config from the encoded annotation found in
+	// its latest deployment.
+	decoded, err := c.decodeFromLatest(config)
+	if err != nil {
+		return err
+	}
+
+	// If this is the initial deployment, then wait for any images that need to be resolved, otherwise
+	// automatically start a new deployment.
 	if config.Status.LatestVersion == 0 {
-		_, _, abort, err := c.generateDeployment(config)
-		if err != nil {
-			if kerrors.IsConflict(err) {
-				return fatalError(fmt.Sprintf("deployment config %q updated since retrieval; aborting trigger: %v", deployutil.LabelForDeploymentConfig(config), err))
-			}
-			glog.V(4).Infof("Couldn't create initial deployment for deployment config %q: %v", deployutil.LabelForDeploymentConfig(config), err)
+		canTrigger, causes := canTrigger(config, decoded)
+		if !canTrigger {
+			// If we cannot trigger then we need to wait for the image change controller.
+			glog.V(5).Infof("Ignoring deployment config %q; template image needs to be resolved by the image change controller", deployutil.LabelForDeploymentConfig(config))
 			return nil
 		}
-		if !abort {
-			glog.V(4).Infof("Created initial deployment for deployment config %q", deployutil.LabelForDeploymentConfig(config))
-		}
+		return c.updateStatus(config, causes)
+	}
+
+	// If this is not the initial deployment, check if there is any template difference between
+	// this and the decoded deploymentconfig.
+	if kapi.Semantic.DeepEqual(config.Spec.Template, decoded.Spec.Template) {
 		return nil
 	}
 
+	_, causes := canTrigger(config, decoded)
+	return c.updateStatus(config, causes)
+}
+
+// decodeFromLatest will try to return the decoded version of the current deploymentconfig found
+// in the annotations of its latest deployment. If there is no previous deploymentconfig (ie.
+// latestVersion == 0), the returned deploymentconfig will be the same.
+func (c *DeploymentConfigChangeController) decodeFromLatest(config *deployapi.DeploymentConfig) (*deployapi.DeploymentConfig, error) {
+	if config.Status.LatestVersion == 0 {
+		return config, nil
+	}
+
 	latestDeploymentName := deployutil.LatestDeploymentNameForConfig(config)
 	deployment, err := c.kClient.ReplicationControllers(config.Namespace).Get(latestDeploymentName)
 	if err != nil {
 		// If there's no deployment for the latest config, we have no basis of
 		// comparison. It's the responsibility of the deployment config controller
 		// to make the deployment for the config, so return early.
-		if kerrors.IsNotFound(err) {
-			glog.V(5).Infof("Ignoring change for deployment config %q; no existing deployment found", deployutil.LabelForDeploymentConfig(config))
-			return nil
-		}
-		return fmt.Errorf("couldn't retrieve deployment for deployment config %q: %v", deployutil.LabelForDeploymentConfig(config), err)
+		return nil, fmt.Errorf("couldn't retrieve deployment for deployment config %q: %v", deployutil.LabelForDeploymentConfig(config), err)
 	}
 
-	deployedConfig, err := c.decodeConfig(deployment)
-	if err != nil {
-		return fatalError(fmt.Sprintf("error decoding deployment config from deployment %q for deployment config %s: %v", deployutil.LabelForDeployment(deployment), deployutil.LabelForDeploymentConfig(config), err))
-	}
+	return c.decodeConfig(deployment)
+}
 
-	// Detect template diffs, and return early if there aren't any changes.
-	if kapi.Semantic.DeepEqual(config.Spec.Template, deployedConfig.Spec.Template) {
-		glog.V(5).Infof("Ignoring deployment config change for %q (latestVersion=%d); same as deployment %q", deployutil.LabelForDeploymentConfig(config), config.Status.LatestVersion, deployutil.LabelForDeployment(deployment))
-		return nil
-	}
+// canTrigger is used by the config change controller to determine if the provided config can
+// trigger its initial deployment. The only requirement is set for image change trigger (ICT)
+// deployments - all of the ICTs need to have LastTriggedImage set which means that the image
+// change controller did its job. The second return argument helps in separating between config
+// change and image change causes.
+func canTrigger(config, decoded *deployapi.DeploymentConfig) (bool, []deployapi.DeploymentCause) {
+	ictCount, resolved := 0, 0
+	var causes []deployapi.DeploymentCause
+
+	for _, t := range config.Spec.Triggers {
+		if t.Type != deployapi.DeploymentTriggerOnImageChange {
+			continue
+		}
+		ictCount++
 
-	// There was a template diff, so generate a new config version.
-	fromVersion, toVersion, abort, err := c.generateDeployment(config)
-	if err != nil {
-		if kerrors.IsConflict(err) {
-			return fatalError(fmt.Sprintf("deployment config %q updated since retrieval; aborting trigger: %v", deployutil.LabelForDeploymentConfig(config), err))
+		// If this is the inital deployment then we need to wait for the image change controller
+		// to resolve the image inside the pod template.
+		lastTriggered := t.ImageChangeParams.LastTriggeredImage
+		if len(lastTriggered) == 0 {
+			continue
 		}
-		return fmt.Errorf("couldn't generate deployment for deployment config %q: %v", deployutil.LabelForDeploymentConfig(config), err)
-	}
-	if !abort {
-		glog.V(4).Infof("Updated deployment config %q from version %d to %d for existing deployment %s", deployutil.LabelForDeploymentConfig(config), fromVersion, toVersion, deployutil.LabelForDeployment(deployment))
-	}
-	return nil
-}
+		resolved++
 
-func (c *DeploymentConfigChangeController) generateDeployment(config *deployapi.DeploymentConfig) (int, int, bool, error) {
-	newConfig, err := c.client.DeploymentConfigs(config.Namespace).Generate(config.Name)
-	if err != nil {
-		return -1, -1, false, err
-	}
+		// We need stronger checks in order to validate that this template
+		// change is an image change. Look at the deserialized config's
+		// triggers and compare with the present trigger.
+		if !triggeredByDifferentImage(*t.ImageChangeParams, *decoded) {
+			continue
+		}
 
-	// The generator returns a cause only when there is an image change. If the configchange
-	// controller detects an image change, it should just quit, otherwise it is racing with
-	// the imagechange controller.
-	if newConfig.Status.LatestVersion != config.Status.LatestVersion &&
-		deployutil.CauseFromAutomaticImageChange(newConfig) {
-		return -1, -1, true, nil
+		causes = append(causes, deployapi.DeploymentCause{
+			Type: deployapi.DeploymentTriggerOnImageChange,
+			ImageTrigger: &deployapi.DeploymentCauseImageTrigger{
+				From: kapi.ObjectReference{
+					Name:      t.ImageChangeParams.From.Name,
+					Namespace: t.ImageChangeParams.From.Namespace,
+					Kind:      "ImageStreamTag",
+				},
+			},
+		})
 	}
 
-	if newConfig.Status.LatestVersion == config.Status.LatestVersion {
-		newConfig.Status.LatestVersion++
+	if len(causes) == 0 {
+		causes = []deployapi.DeploymentCause{{Type: deployapi.DeploymentTriggerOnConfigChange}}
 	}
 
-	// set the trigger details for the new deployment config
-	causes := []deployapi.DeploymentCause{
-		{
-			Type: deployapi.DeploymentTriggerOnConfigChange,
-		},
-	}
-	newConfig.Status.Details = &deployapi.DeploymentDetails{
-		Causes: causes,
-	}
+	return ictCount == resolved, causes
+}
 
-	// This update is atomic. If it fails because a newer resource was already persisted, that's
-	// okay - we can just ignore the update for the old resource and any changes to the more
-	// current config will be captured in future events.
-	updatedConfig, err := c.client.DeploymentConfigs(config.Namespace).UpdateStatus(newConfig)
-	if err != nil {
-		return config.Status.LatestVersion, newConfig.Status.LatestVersion, false, err
+func triggeredByDifferentImage(ictParams deployapi.DeploymentTriggerImageChangeParams, previous deployapi.DeploymentConfig) bool {
+	for _, t := range previous.Spec.Triggers {
+		if t.Type != deployapi.DeploymentTriggerOnImageChange {
+			continue
+		}
+
+		if t.ImageChangeParams.From.Name != ictParams.From.Name &&
+			t.ImageChangeParams.From.Namespace != ictParams.From.Namespace {
+			continue
+		}
+
+		return t.ImageChangeParams.LastTriggeredImage != ictParams.LastTriggeredImage
 	}
+	return false
+}
 
-	return config.Status.LatestVersion, updatedConfig.Status.LatestVersion, false, nil
+func (c *DeploymentConfigChangeController) updateStatus(config *deployapi.DeploymentConfig, causes []deployapi.DeploymentCause) error {
+	config.Status.LatestVersion++
+	config.Status.Details = new(deployapi.DeploymentDetails)
+	config.Status.Details.Causes = causes
+	_, err := c.client.DeploymentConfigs(config.Namespace).UpdateStatus(config)
+	return err
 }