Bug 1340735: update dc image at most once on automatic=false

pkg/deploy/controller/imagechange/controller.go

@@ -50,11 +50,11 @@ func (c *ImageChangeController) Handle(stream *imageapi.ImageStream) error {
 				continue
 			}
 
-			// All initial deployments (latestVersion == 0) should have their images resolved in order
-			// to be able to work and not try to pull non-existent images from DockerHub.
-			// Deployments with automatic set to false that have been deployed at least once (latestVersion > 0)
-			// shouldn't have their images updated.
-			if !params.Automatic && config.Status.LatestVersion != 0 {
+			// All initial deployments should have their images resolved in order to
+			// be able to work and not try to pull non-existent images from DockerHub.
+			// Deployments with automatic set to false that have been deployed at least
+			// once shouldn't have their images updated.
+			if !params.Automatic && len(params.LastTriggeredImage) > 0 {
 				continue
 			}
 

pkg/deploy/controller/imagechange/controller_test.go

@@ -33,6 +33,8 @@ func TestHandle_changeForNonAutomaticTag(t *testing.T) {
 			config := testapi.OkDeploymentConfig(1)
 			config.Namespace = kapi.NamespaceDefault
 			config.Spec.Triggers[0].ImageChangeParams.Automatic = false
+			// The image has been resolved at least once before.
+			config.Spec.Triggers[0].ImageChangeParams.LastTriggeredImage = testapi.DockerImageReference
 
 			return []*deployapi.DeploymentConfig{config}, nil
 		},
@@ -118,11 +120,14 @@ func TestHandle_changeForUnregisteredTag(t *testing.T) {
 // match) properly.
 func TestHandle_matchScenarios(t *testing.T) {
 	tests := []struct {
+		name string
+
 		param   *deployapi.DeploymentTriggerImageChangeParams
 		matches bool
 	}{
-		// Update from empty last image ID to a new one with explicit namespaces
 		{
+			name: "automatic=true, initial trigger, explicit namespace",
+
 			param: &deployapi.DeploymentTriggerImageChangeParams{
 				Automatic:          true,
 				ContainerNames:     []string{"container1"},
@@ -131,8 +136,9 @@ func TestHandle_matchScenarios(t *testing.T) {
 			},
 			matches: true,
 		},
-		// Update from empty last image ID to a new one with implicit namespaces
 		{
+			name: "automatic=true, initial trigger, implicit namespace",
+
 			param: &deployapi.DeploymentTriggerImageChangeParams{
 				Automatic:          true,
 				ContainerNames:     []string{"container1"},
@@ -141,18 +147,31 @@ func TestHandle_matchScenarios(t *testing.T) {
 			},
 			matches: true,
 		},
-		// Update from empty last image ID to a new one, but not marked automatic
 		{
+			name: "automatic=false, initial trigger",
+
 			param: &deployapi.DeploymentTriggerImageChangeParams{
 				Automatic:          false,
 				ContainerNames:     []string{"container1"},
 				From:               kapi.ObjectReference{Namespace: kapi.NamespaceDefault, Name: imageapi.JoinImageStreamTag(testapi.ImageStreamName, imageapi.DefaultImageTag)},
 				LastTriggeredImage: "",
 			},
+			matches: true,
+		},
+		{
+			name: "(no-op) automatic=false, already triggered",
+
+			param: &deployapi.DeploymentTriggerImageChangeParams{
+				Automatic:          false,
+				ContainerNames:     []string{"container1"},
+				From:               kapi.ObjectReference{Namespace: kapi.NamespaceDefault, Name: imageapi.JoinImageStreamTag(testapi.ImageStreamName, imageapi.DefaultImageTag)},
+				LastTriggeredImage: testapi.DockerImageReference,
+			},
 			matches: false,
 		},
-		// Updated image ID is equal to the last triggered ID
 		{
+			name: "(no-op) automatic=true, image is already deployed",
+
 			param: &deployapi.DeploymentTriggerImageChangeParams{
 				Automatic:          true,
 				ContainerNames:     []string{"container1"},
@@ -161,8 +180,9 @@ func TestHandle_matchScenarios(t *testing.T) {
 			},
 			matches: false,
 		},
-		// Trigger stream reference doesn't match
 		{
+			name: "(no-op) trigger doesn't match the stream",
+
 			param: &deployapi.DeploymentTriggerImageChangeParams{
 				Automatic:          true,
 				ContainerNames:     []string{"container1"},
@@ -173,13 +193,13 @@ func TestHandle_matchScenarios(t *testing.T) {
 		},
 	}
 
-	for i, test := range tests {
+	for _, test := range tests {
 		updated := false
 
 		fake := &testclient.Fake{}
 		fake.AddReactor("update", "deploymentconfigs", func(action ktestclient.Action) (handled bool, ret runtime.Object, err error) {
 			if !test.matches {
-				t.Fatalf("unexpected deploymentconfig update for scenario %d", i)
+				t.Fatal("unexpected deploymentconfig update")
 			}
 			updated = true
 			return true, nil, nil
@@ -201,15 +221,15 @@ func TestHandle_matchScenarios(t *testing.T) {
 			client: fake,
 		}
 
-		t.Logf("running scenario: %d", i)
+		t.Logf("running test %q", test.name)
 		stream := makeStream(testapi.ImageStreamName, imageapi.DefaultImageTag, testapi.DockerImageReference, testapi.ImageID)
 		if err := controller.Handle(stream); err != nil {
-			t.Fatalf("unexpected error for scenario %v: %v", i, err)
+			t.Fatalf("unexpected error: %v", err)
 		}
 
 		// assert updates occurred
 		if test.matches && !updated {
-			t.Fatalf("expected update for scenario: %v", test)
+			t.Fatal("expected an update")
 		}
 	}
 }

test/integration/deploy_trigger_test.go

@@ -5,6 +5,7 @@ package integration
 import (
 	"fmt"
 	"testing"
+	"time"
 
 	kapi "k8s.io/kubernetes/pkg/api"
 	kclient "k8s.io/kubernetes/pkg/client/unversioned"
@@ -114,17 +115,17 @@ func TestTriggers_imageChange(t *testing.T) {
 
 	configWatch, err := openshiftProjectAdminClient.DeploymentConfigs(testutil.Namespace()).Watch(kapi.ListOptions{})
 	if err != nil {
-		t.Fatalf("Couldn't subscribe to Deployments %v", err)
+		t.Fatalf("Couldn't subscribe to deploymentconfigs %v", err)
 	}
 	defer configWatch.Stop()
 
 	if imageStream, err = openshiftProjectAdminClient.ImageStreams(testutil.Namespace()).Create(imageStream); err != nil {
-		t.Fatalf("Couldn't create ImageStream: %v", err)
+		t.Fatalf("Couldn't create imagestream: %v", err)
 	}
 
 	imageWatch, err := openshiftProjectAdminClient.ImageStreams(testutil.Namespace()).Watch(kapi.ListOptions{})
 	if err != nil {
-		t.Fatalf("Couldn't subscribe to ImageStreams: %s", err)
+		t.Fatalf("Couldn't subscribe to imagestreams: %v", err)
 	}
 	defer imageWatch.Stop()
 
@@ -147,29 +148,29 @@ func TestTriggers_imageChange(t *testing.T) {
 			t.Fatalf("unexpected error: %v", err)
 		}
 
-		t.Log("Waiting for image stream mapping to be reflected in the IS status...")
+		t.Log("Waiting for image stream mapping to be reflected in the image stream status...")
 	statusLoop:
 		for {
 			select {
 			case event := <-imageWatch.ResultChan():
 				stream := event.Object.(*imageapi.ImageStream)
 				if _, ok := stream.Status.Tags[imageapi.DefaultImageTag]; ok {
-					t.Logf("ImageStream %s now has Status with tags: %#v", stream.Name, stream.Status.Tags)
+					t.Logf("imagestream %q now has status with tags: %#v", stream.Name, stream.Status.Tags)
 					break statusLoop
 				}
-				t.Logf("Still waiting for latest tag status on ImageStream %s", stream.Name)
+				t.Logf("Still waiting for latest tag status on imagestream %q", stream.Name)
 			}
 		}
 	}
 
 	if config, err = openshiftProjectAdminClient.DeploymentConfigs(testutil.Namespace()).Create(config); err != nil {
-		t.Fatalf("Couldn't create DeploymentConfig: %v", err)
+		t.Fatalf("Couldn't create deploymentconfig: %v", err)
 	}
 
 	createTagEvent()
 
 	var newConfig *deployapi.DeploymentConfig
-	t.Log("Waiting for a new deployment config in response to ImageStream update")
+	t.Log("Waiting for a new deployment config in response to imagestream update")
 waitForNewConfig:
 	for {
 		select {
@@ -184,12 +185,155 @@ waitForNewConfig:
 					}
 					break waitForNewConfig
 				}
-				t.Log("Still waiting for a new deployment config in response to ImageStream update")
+				t.Log("Still waiting for a new deployment config in response to imagestream update")
 			}
 		}
 	}
 }
 
+func TestTriggers_imageChange_nonAutomatic(t *testing.T) {
+	testutil.RequireEtcd(t)
+	_, clusterAdminKubeConfig, err := testserver.StartTestMaster()
+	if err != nil {
+		t.Fatalf("error starting master: %v", err)
+	}
+	openshiftClusterAdminClient, err := testutil.GetClusterAdminClient(clusterAdminKubeConfig)
+	if err != nil {
+		t.Fatalf("error getting cluster admin client: %v", err)
+	}
+	openshiftClusterAdminClientConfig, err := testutil.GetClusterAdminClientConfig(clusterAdminKubeConfig)
+	if err != nil {
+		t.Fatalf("error getting cluster admin client config: %v", err)
+	}
+	openshiftProjectAdminClient, err := testserver.CreateNewProject(openshiftClusterAdminClient, *openshiftClusterAdminClientConfig, testutil.Namespace(), "bob")
+	if err != nil {
+		t.Fatalf("error creating project: %v", err)
+	}
+
+	imageStream := &imageapi.ImageStream{ObjectMeta: kapi.ObjectMeta{Name: deploytest.ImageStreamName}}
+
+	if imageStream, err = openshiftProjectAdminClient.ImageStreams(testutil.Namespace()).Create(imageStream); err != nil {
+		t.Fatalf("Couldn't create imagestream: %v", err)
+	}
+
+	imageWatch, err := openshiftProjectAdminClient.ImageStreams(testutil.Namespace()).Watch(kapi.ListOptions{})
+	if err != nil {
+		t.Fatalf("Couldn't subscribe to imagestreams: %v", err)
+	}
+	defer imageWatch.Stop()
+
+	image := fmt.Sprintf("sha256:%s", deploytest.ImageID)
+	pullSpec := fmt.Sprintf("registry:8080/%s/%s@%s", testutil.Namespace(), deploytest.ImageStreamName, image)
+	// Make a function which can create a new tag event for the image stream and
+	// then wait for the stream status to be asynchronously updated.
+	mapping := &imageapi.ImageStreamMapping{
+		ObjectMeta: kapi.ObjectMeta{Name: imageStream.Name},
+		Tag:        imageapi.DefaultImageTag,
+		Image: imageapi.Image{
+			ObjectMeta: kapi.ObjectMeta{
+				Name: image,
+			},
+			DockerImageReference: pullSpec,
+		},
+	}
+	updated := ""
+
+	createTagEvent := func(mapping *imageapi.ImageStreamMapping) {
+		if err := openshiftProjectAdminClient.ImageStreamMappings(testutil.Namespace()).Create(mapping); err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+
+		t.Log("Waiting for image stream mapping to be reflected in the image stream status...")
+
+		for {
+			select {
+			case event := <-imageWatch.ResultChan():
+				stream := event.Object.(*imageapi.ImageStream)
+				tagEventList, ok := stream.Status.Tags[imageapi.DefaultImageTag]
+				if ok {
+					if updated != tagEventList.Items[0].DockerImageReference {
+						updated = tagEventList.Items[0].DockerImageReference
+						return
+					}
+				}
+				t.Logf("Still waiting for latest tag status update on imagestream %q", stream.Name)
+			}
+		}
+	}
+
+	configWatch, err := openshiftProjectAdminClient.DeploymentConfigs(testutil.Namespace()).Watch(kapi.ListOptions{})
+	if err != nil {
+		t.Fatalf("Couldn't subscribe to deploymentconfigs: %v", err)
+	}
+	defer configWatch.Stop()
+
+	config := deploytest.OkDeploymentConfig(0)
+	config.Namespace = testutil.Namespace()
+	config.Spec.Triggers = []deployapi.DeploymentTriggerPolicy{deploytest.OkImageChangeTrigger()}
+	config.Spec.Triggers[0].ImageChangeParams.Automatic = false
+	if config, err = openshiftProjectAdminClient.DeploymentConfigs(testutil.Namespace()).Create(config); err != nil {
+		t.Fatalf("Couldn't create deploymentconfig: %v", err)
+	}
+
+	createTagEvent(mapping)
+
+	var newConfig *deployapi.DeploymentConfig
+	t.Log("Waiting for the initial deploymentconfig update in response to the imagestream update")
+
+	timeout := time.After(30 * time.Second)
+
+	// This is the initial deployment with automatic=false in its ICT - it should be updated to pullSpec
+out:
+	for {
+		select {
+		case event := <-configWatch.ResultChan():
+			if event.Type != watchapi.Modified {
+				continue
+			}
+
+			newConfig = event.Object.(*deployapi.DeploymentConfig)
+
+			if newConfig.Status.LatestVersion > 0 {
+				t.Fatalf("unexpected latestVersion update - the config has no config change trigger")
+			}
+
+			if e, a := updated, newConfig.Spec.Template.Spec.Containers[0].Image; e == a {
+				break out
+			}
+		case <-timeout:
+			t.Fatalf("timed out waiting for the image update to happen")
+		}
+	}
+
+	t.Log("Waiting for the second imagestream update - it shouldn't update the deploymentconfig")
+
+	// Subsequent updates to the image shouldn't update the pod template image
+	mapping.Image.Name = "sha256:thisupdatedimageshouldneverlandinthepodtemplate"
+	mapping.Image.DockerImageReference = fmt.Sprintf("registry:8080/%s/%s@%s", testutil.Namespace(), deploytest.ImageStreamName, mapping.Image.Name)
+	createTagEvent(mapping)
+
+	for {
+		select {
+		case event := <-configWatch.ResultChan():
+			if event.Type != watchapi.Modified {
+				continue
+			}
+
+			newConfig = event.Object.(*deployapi.DeploymentConfig)
+
+			if newConfig.Status.LatestVersion > 0 {
+				t.Fatalf("unexpected latestVersion update - the config has no config change trigger")
+			}
+
+			if e, a := updated, newConfig.Spec.Template.Spec.Containers[0].Image; e == a {
+				t.Fatalf("unexpected image update, expected initial image to be the same")
+			}
+		case <-timeout:
+			return
+		}
+	}
+}
+
 func TestTriggers_configChange(t *testing.T) {
 	const namespace = "test-triggers-configchange"