deployer controller: ensure phase direction

pkg/deploy/controller/deployerpod/controller.go

@@ -97,9 +97,8 @@ func (c *DeployerPodController) Handle(pod *kapi.Pod) error {
 
 	switch pod.Status.Phase {
 	case kapi.PodRunning:
-		if !deployutil.IsTerminatedDeployment(deployment) {
-			nextStatus = deployapi.DeploymentStatusRunning
-		}
+		nextStatus = deployapi.DeploymentStatusRunning
+
 	case kapi.PodSucceeded:
 		nextStatus = deployapi.DeploymentStatusComplete
 
@@ -137,7 +136,7 @@ func (c *DeployerPodController) Handle(pod *kapi.Pod) error {
 		}
 	}
 
-	if currentStatus != nextStatus {
+	if deployutil.CanTransitionPhase(currentStatus, nextStatus) {
 		deployment.Annotations[deployapi.DeploymentStatusAnnotation] = string(nextStatus)
 		if _, err := c.kClient.ReplicationControllers(deployment.Namespace).Update(deployment); err != nil {
 			if kerrors.IsNotFound(err) {

pkg/deploy/controller/deployerpod/controller_test.go

@@ -352,7 +352,8 @@ func TestHandle_podTerminatedFailNoContainerStatusTest(t *testing.T) {
 // TestHandle_cleanupDesiredReplicasAnnotation ensures that the desired replicas annotation
 // will be cleaned up in a complete deployment and stay around in a failed deployment
 func TestHandle_cleanupDesiredReplicasAnnotation(t *testing.T) {
-	deployment, _ := deployutil.MakeDeployment(deploytest.OkDeploymentConfig(1), kapi.Codecs.LegacyCodec(deployapi.SchemeGroupVersion))
+	// shared fixtures shouldn't be used in unit tests
+	shared, _ := deployutil.MakeDeployment(deploytest.OkDeploymentConfig(1), kapi.Codecs.LegacyCodec(deployapi.SchemeGroupVersion))
 
 	tests := []struct {
 		name     string
@@ -361,17 +362,18 @@ func TestHandle_cleanupDesiredReplicasAnnotation(t *testing.T) {
 	}{
 		{
 			name:     "complete deployment - cleaned up annotation",
-			pod:      succeededPod(deployment),
+			pod:      succeededPod(shared),
 			expected: false,
 		},
 		{
 			name:     "failed deployment - annotation stays",
-			pod:      terminatedPod(deployment),
+			pod:      terminatedPod(shared),
 			expected: true,
 		},
 	}
 
 	for _, test := range tests {
+		deployment, _ := deployutil.MakeDeployment(deploytest.OkDeploymentConfig(1), kapi.Codecs.LegacyCodec(deployapi.SchemeGroupVersion))
 		var updatedDeployment *kapi.ReplicationController
 		deployment.Annotations[deployapi.DesiredReplicasAnnotation] = "1"
 

pkg/deploy/controller/deployment/controller.go

@@ -221,7 +221,7 @@ func (c *DeploymentController) Handle(deployment *kapi.ReplicationController) er
 		}
 	}
 
-	if currentStatus != nextStatus || deploymentScaled {
+	if deployutil.CanTransitionPhase(currentStatus, nextStatus) || deploymentScaled {
 		deployment.Annotations[deployapi.DeploymentStatusAnnotation] = string(nextStatus)
 		if _, err := c.deploymentClient.updateDeployment(deployment.Namespace, deployment); err != nil {
 			if config, decodeErr := c.decodeConfig(deployment); decodeErr == nil {

pkg/deploy/util/util.go

@@ -279,6 +279,33 @@ func IsTerminatedDeployment(deployment *api.ReplicationController) bool {
 	return current == deployapi.DeploymentStatusComplete || current == deployapi.DeploymentStatusFailed
 }
 
+// CanTransitionPhase returns whether it is allowed to go from the current to the next phase.
+func CanTransitionPhase(current, next deployapi.DeploymentStatus) bool {
+	switch current {
+	case deployapi.DeploymentStatusNew:
+		switch next {
+		case deployapi.DeploymentStatusPending,
+			deployapi.DeploymentStatusRunning,
+			deployapi.DeploymentStatusFailed,
+			deployapi.DeploymentStatusComplete:
+			return true
+		}
+	case deployapi.DeploymentStatusPending:
+		switch next {
+		case deployapi.DeploymentStatusRunning,
+			deployapi.DeploymentStatusFailed,
+			deployapi.DeploymentStatusComplete:
+			return true
+		}
+	case deployapi.DeploymentStatusRunning:
+		switch next {
+		case deployapi.DeploymentStatusFailed, deployapi.DeploymentStatusComplete:
+			return true
+		}
+	}
+	return false
+}
+
 // annotationFor returns the annotation with key for obj.
 func annotationFor(obj runtime.Object, key string) string {
 	meta, err := api.ObjectMetaFor(obj)

pkg/deploy/util/util_test.go

@@ -184,3 +184,169 @@ func TestSort(t *testing.T) {
 		t.Errorf("Unexpected sort order")
 	}
 }
+
+func TestCanTransitionPhase(t *testing.T) {
+	tests := []struct {
+		name          string
+		current, next deployapi.DeploymentStatus
+		expected      bool
+	}{
+		{
+			name:     "New->New",
+			current:  deployapi.DeploymentStatusNew,
+			next:     deployapi.DeploymentStatusNew,
+			expected: false,
+		},
+		{
+			name:     "New->Pending",
+			current:  deployapi.DeploymentStatusNew,
+			next:     deployapi.DeploymentStatusPending,
+			expected: true,
+		},
+		{
+			name:     "New->Running",
+			current:  deployapi.DeploymentStatusNew,
+			next:     deployapi.DeploymentStatusRunning,
+			expected: true,
+		},
+		{
+			name:     "New->Complete",
+			current:  deployapi.DeploymentStatusNew,
+			next:     deployapi.DeploymentStatusComplete,
+			expected: true,
+		},
+		{
+			name:     "New->Failed",
+			current:  deployapi.DeploymentStatusNew,
+			next:     deployapi.DeploymentStatusFailed,
+			expected: true,
+		},
+		{
+			name:     "Pending->New",
+			current:  deployapi.DeploymentStatusPending,
+			next:     deployapi.DeploymentStatusNew,
+			expected: false,
+		},
+		{
+			name:     "Pending->Pending",
+			current:  deployapi.DeploymentStatusPending,
+			next:     deployapi.DeploymentStatusPending,
+			expected: false,
+		},
+		{
+			name:     "Pending->Running",
+			current:  deployapi.DeploymentStatusPending,
+			next:     deployapi.DeploymentStatusRunning,
+			expected: true,
+		},
+		{
+			name:     "Pending->Failed",
+			current:  deployapi.DeploymentStatusPending,
+			next:     deployapi.DeploymentStatusFailed,
+			expected: true,
+		},
+		{
+			name:     "Pending->Complete",
+			current:  deployapi.DeploymentStatusPending,
+			next:     deployapi.DeploymentStatusComplete,
+			expected: true,
+		},
+		{
+			name:     "Running->New",
+			current:  deployapi.DeploymentStatusRunning,
+			next:     deployapi.DeploymentStatusNew,
+			expected: false,
+		},
+		{
+			name:     "Running->Pending",
+			current:  deployapi.DeploymentStatusRunning,
+			next:     deployapi.DeploymentStatusPending,
+			expected: false,
+		},
+		{
+			name:     "Running->Running",
+			current:  deployapi.DeploymentStatusRunning,
+			next:     deployapi.DeploymentStatusRunning,
+			expected: false,
+		},
+		{
+			name:     "Running->Failed",
+			current:  deployapi.DeploymentStatusRunning,
+			next:     deployapi.DeploymentStatusFailed,
+			expected: true,
+		},
+		{
+			name:     "Running->Complete",
+			current:  deployapi.DeploymentStatusRunning,
+			next:     deployapi.DeploymentStatusComplete,
+			expected: true,
+		},
+		{
+			name:     "Complete->New",
+			current:  deployapi.DeploymentStatusComplete,
+			next:     deployapi.DeploymentStatusNew,
+			expected: false,
+		},
+		{
+			name:     "Complete->Pending",
+			current:  deployapi.DeploymentStatusComplete,
+			next:     deployapi.DeploymentStatusPending,
+			expected: false,
+		},
+		{
+			name:     "Complete->Running",
+			current:  deployapi.DeploymentStatusComplete,
+			next:     deployapi.DeploymentStatusRunning,
+			expected: false,
+		},
+		{
+			name:     "Complete->Failed",
+			current:  deployapi.DeploymentStatusComplete,
+			next:     deployapi.DeploymentStatusFailed,
+			expected: false,
+		},
+		{
+			name:     "Complete->Complete",
+			current:  deployapi.DeploymentStatusComplete,
+			next:     deployapi.DeploymentStatusComplete,
+			expected: false,
+		},
+		{
+			name:     "Failed->New",
+			current:  deployapi.DeploymentStatusFailed,
+			next:     deployapi.DeploymentStatusNew,
+			expected: false,
+		},
+		{
+			name:     "Failed->Pending",
+			current:  deployapi.DeploymentStatusFailed,
+			next:     deployapi.DeploymentStatusPending,
+			expected: false,
+		},
+		{
+			name:     "Failed->Running",
+			current:  deployapi.DeploymentStatusFailed,
+			next:     deployapi.DeploymentStatusRunning,
+			expected: false,
+		},
+		{
+			name:     "Failed->Complete",
+			current:  deployapi.DeploymentStatusFailed,
+			next:     deployapi.DeploymentStatusComplete,
+			expected: false,
+		},
+		{
+			name:     "Failed->Failed",
+			current:  deployapi.DeploymentStatusFailed,
+			next:     deployapi.DeploymentStatusFailed,
+			expected: false,
+		},
+	}
+
+	for _, test := range tests {
+		got := CanTransitionPhase(test.current, test.next)
+		if got != test.expected {
+			t.Errorf("%s: expected %t, got %t", test.name, test.expected, got)
+		}
+	}
+}