Fix negative unavailableReplicas dc field

Signed-off-by: Michail Kargakis <[email protected]>

pkg/deploy/api/validation/validation.go

@@ -133,13 +133,12 @@ func handleEmptyImageReferences(template *kapi.PodTemplateSpec, triggers []deplo
 func ValidateDeploymentConfigStatus(status deployapi.DeploymentConfigStatus) field.ErrorList {
 	allErrs := field.ErrorList{}
 	statusPath := field.NewPath("status")
-	if status.LatestVersion < 0 {
-		allErrs = append(allErrs, field.Invalid(statusPath.Child("latestVersion"), status.LatestVersion, "latestVersion cannot be negative"))
-	}
-	if status.ObservedGeneration < int64(0) {
-		allErrs = append(allErrs, field.Invalid(statusPath.Child("observedGeneration"), status.ObservedGeneration, "observedGeneration cannot be negative"))
-	}
+	allErrs = append(allErrs, kapivalidation.ValidateNonnegativeField(int64(status.LatestVersion), statusPath.Child("latestVersion"))...)
+	allErrs = append(allErrs, kapivalidation.ValidateNonnegativeField(int64(status.ObservedGeneration), statusPath.Child("observedGeneration"))...)
+	allErrs = append(allErrs, kapivalidation.ValidateNonnegativeField(int64(status.Replicas), statusPath.Child("replicas"))...)
 	allErrs = append(allErrs, kapivalidation.ValidateNonnegativeField(int64(status.ReadyReplicas), statusPath.Child("readyReplicas"))...)
+	allErrs = append(allErrs, kapivalidation.ValidateNonnegativeField(int64(status.AvailableReplicas), statusPath.Child("availableReplicas"))...)
+	allErrs = append(allErrs, kapivalidation.ValidateNonnegativeField(int64(status.UnavailableReplicas), statusPath.Child("unavailableReplicas"))...)
 	return allErrs
 }
 

pkg/deploy/controller/deploymentconfig/deploymentconfig_controller.go

@@ -7,7 +7,6 @@ import (
 	"github.com/golang/glog"
 
 	kapierrors "k8s.io/apimachinery/pkg/api/errors"
-	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
 	kutilerrors "k8s.io/apimachinery/pkg/util/errors"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
@@ -275,10 +274,7 @@ func (c *DeploymentConfigController) reconcileDeployments(existingDeployments []
 // Update the status of the provided deployment config. Additional conditions will override any other condition in the
 // deployment config status.
 func (c *DeploymentConfigController) updateStatus(config *deployapi.DeploymentConfig, deployments []*kapi.ReplicationController, additional ...deployapi.DeploymentCondition) error {
-	newStatus, err := c.calculateStatus(*config, deployments, additional...)
-	if err != nil {
-		return err
-	}
+	newStatus := calculateStatus(config, deployments, additional...)
 
 	// NOTE: We should update the status of the deployment config only if we need to, otherwise
 	// we hotloop between updates.
@@ -305,36 +301,33 @@ func (c *DeploymentConfigController) updateStatus(config *deployapi.DeploymentCo
 	return nil
 }
 
-func (c *DeploymentConfigController) calculateStatus(config deployapi.DeploymentConfig, deployments []*kapi.ReplicationController, additional ...deployapi.DeploymentCondition) (deployapi.DeploymentConfigStatus, error) {
-	selector := labels.Set(config.Spec.Selector).AsSelector()
-	// TODO: Replace with using rc.status.availableReplicas that comes with the next rebase.
-	pods, err := c.podLister.Pods(config.Namespace).List(selector)
-	if err != nil {
-		return config.Status, err
-	}
-	available := deployutil.GetAvailablePods(pods, config.Spec.MinReadySeconds)
-
-	// UpdatedReplicas represents the replicas that use the deployment config template which means
+func calculateStatus(config *deployapi.DeploymentConfig, rcs []*kapi.ReplicationController, additional ...deployapi.DeploymentCondition) deployapi.DeploymentConfigStatus {
+	// UpdatedReplicas represents the replicas that use the current deployment config template which means
 	// we should inform about the replicas of the latest deployment and not the active.
 	latestReplicas := int32(0)
-	latestExists, latestRC := deployutil.LatestDeploymentInfo(&config, deployments)
+	latestExists, latestRC := deployutil.LatestDeploymentInfo(config, rcs)
 	if !latestExists {
 		latestRC = nil
 	} else {
 		latestReplicas = deployutil.GetStatusReplicaCountForDeployments([]*kapi.ReplicationController{latestRC})
 	}
 
-	total := deployutil.GetReplicaCountForDeployments(deployments)
+	available := deployutil.GetAvailableReplicaCountForReplicationControllers(rcs)
+	total := deployutil.GetReplicaCountForDeployments(rcs)
+	unavailableReplicas := total - available
+	if unavailableReplicas < 0 {
+		unavailableReplicas = 0
+	}
 
 	status := deployapi.DeploymentConfigStatus{
 		LatestVersion:       config.Status.LatestVersion,
 		Details:             config.Status.Details,
 		ObservedGeneration:  config.Generation,
-		Replicas:            deployutil.GetStatusReplicaCountForDeployments(deployments),
+		Replicas:            deployutil.GetStatusReplicaCountForDeployments(rcs),
 		UpdatedReplicas:     latestReplicas,
 		AvailableReplicas:   available,
-		ReadyReplicas:       deployutil.GetReadyReplicaCountForReplicationControllers(deployments),
-		UnavailableReplicas: total - available,
+		ReadyReplicas:       deployutil.GetReadyReplicaCountForReplicationControllers(rcs),
+		UnavailableReplicas: unavailableReplicas,
 		Conditions:          config.Status.Conditions,
 	}
 
@@ -343,10 +336,10 @@ func (c *DeploymentConfigController) calculateStatus(config deployapi.Deployment
 		deployutil.SetDeploymentCondition(&status, cond)
 	}
 
-	return status, nil
+	return status
 }
 
-func updateConditions(config deployapi.DeploymentConfig, newStatus *deployapi.DeploymentConfigStatus, latestRC *kapi.ReplicationController) {
+func updateConditions(config *deployapi.DeploymentConfig, newStatus *deployapi.DeploymentConfigStatus, latestRC *kapi.ReplicationController) {
 	// Availability condition.
 	if newStatus.AvailableReplicas >= config.Spec.Replicas-deployutil.MaxUnavailable(config) && newStatus.AvailableReplicas > 0 {
 		minAvailability := deployutil.NewDeploymentCondition(deployapi.DeploymentAvailable, kapi.ConditionTrue, "", "Deployment config has minimum availability.")
@@ -364,7 +357,7 @@ func updateConditions(config deployapi.DeploymentConfig, newStatus *deployapi.De
 			condition := deployutil.NewDeploymentCondition(deployapi.DeploymentProgressing, kapi.ConditionUnknown, "", msg)
 			deployutil.SetDeploymentCondition(newStatus, *condition)
 		case deployapi.DeploymentStatusRunning:
-			if deployutil.IsProgressing(config, *newStatus) {
+			if deployutil.IsProgressing(config, newStatus) {
 				deployutil.RemoveDeploymentCondition(newStatus, deployapi.DeploymentProgressing)
 				msg := fmt.Sprintf("replication controller %q is progressing", latestRC.Name)
 				condition := deployutil.NewDeploymentCondition(deployapi.DeploymentProgressing, kapi.ConditionTrue, deployapi.ReplicationControllerUpdatedReason, msg)

pkg/deploy/controller/deploymentconfig/deploymentconfig_controller_test.go

@@ -1,6 +1,7 @@
 package deploymentconfig
 
 import (
+	"reflect"
 	"sort"
 	"strconv"
 	"testing"
@@ -9,6 +10,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/diff"
+	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/apimachinery/pkg/watch"
 	clientgotesting "k8s.io/client-go/testing"
 	"k8s.io/client-go/tools/cache"
@@ -432,3 +434,112 @@ func TestHandleScenarios(t *testing.T) {
 func newInt32(i int32) *int32 {
 	return &i
 }
+
+func newDC(version, replicas, maxUnavailable int, cond deployapi.DeploymentCondition) *deployapi.DeploymentConfig {
+	return &deployapi.DeploymentConfig{
+		Spec: deployapi.DeploymentConfigSpec{
+			Replicas: int32(replicas),
+			Strategy: deployapi.DeploymentStrategy{
+				Type: deployapi.DeploymentStrategyTypeRolling,
+				RollingParams: &deployapi.RollingDeploymentStrategyParams{
+					MaxUnavailable: intstr.FromInt(maxUnavailable),
+					MaxSurge:       intstr.FromInt(1),
+				},
+			},
+		},
+		Status: deployapi.DeploymentConfigStatus{
+			LatestVersion: int64(version),
+			Conditions: []deployapi.DeploymentCondition{
+				cond,
+			},
+		},
+	}
+}
+
+var (
+	availableCond = deployapi.DeploymentCondition{
+		Type:   deployapi.DeploymentAvailable,
+		Status: kapi.ConditionTrue,
+	}
+	unavailableCond = deployapi.DeploymentCondition{
+		Type:   deployapi.DeploymentAvailable,
+		Status: kapi.ConditionFalse,
+	}
+)
+
+func newRC(version, desired, current, ready, available int32) *kapi.ReplicationController {
+	return &kapi.ReplicationController{
+		ObjectMeta: metav1.ObjectMeta{
+			Annotations: map[string]string{deployapi.DeploymentVersionAnnotation: strconv.Itoa(int(version))},
+		},
+		Spec: kapi.ReplicationControllerSpec{
+			Replicas: desired,
+		},
+		Status: kapi.ReplicationControllerStatus{
+			Replicas:          current,
+			ReadyReplicas:     ready,
+			AvailableReplicas: available,
+		},
+	}
+}
+
+func TestCalculateStatus(t *testing.T) {
+	tests := []struct {
+		name string
+
+		dc  *deployapi.DeploymentConfig
+		rcs []*kapi.ReplicationController
+
+		expected deployapi.DeploymentConfigStatus
+	}{
+		{
+			name: "available deployment",
+
+			dc: newDC(3, 3, 1, availableCond),
+			rcs: []*kapi.ReplicationController{
+				newRC(3, 2, 2, 1, 1),
+				newRC(2, 0, 0, 0, 0),
+				newRC(1, 0, 1, 1, 1),
+			},
+
+			expected: deployapi.DeploymentConfigStatus{
+				LatestVersion:     int64(3),
+				Replicas:          int32(3),
+				ReadyReplicas:     int32(2),
+				AvailableReplicas: int32(2),
+				UpdatedReplicas:   int32(2),
+				Conditions: []deployapi.DeploymentCondition{
+					availableCond,
+				},
+			},
+		},
+		{
+			name: "unavailable deployment",
+
+			dc: newDC(2, 2, 0, unavailableCond),
+			rcs: []*kapi.ReplicationController{
+				newRC(2, 2, 0, 0, 0),
+				newRC(1, 0, 1, 1, 1),
+			},
+
+			expected: deployapi.DeploymentConfigStatus{
+				LatestVersion:       int64(2),
+				Replicas:            int32(1),
+				ReadyReplicas:       int32(1),
+				AvailableReplicas:   int32(1),
+				UpdatedReplicas:     int32(0),
+				UnavailableReplicas: int32(1),
+				Conditions: []deployapi.DeploymentCondition{
+					unavailableCond,
+				},
+			},
+		},
+	}
+
+	for _, test := range tests {
+		status := calculateStatus(test.dc, test.rcs)
+		if !reflect.DeepEqual(status, test.expected) {
+			t.Errorf("%s: expected status:\n%+v\ngot status:\n%+v", test.name, test.expected, status)
+		}
+	}
+}

pkg/deploy/util/util.go

@@ -337,18 +337,6 @@ func GetStatusReplicaCountForDeployments(deployments []*api.ReplicationControlle
 	return totalReplicaCount
 }
 
-// GetAvailablePods returns all the available pods from the provided pod list.
-func GetAvailablePods(pods []*api.Pod, minReadySeconds int32) int32 {
-	available := int32(0)
-	for i := range pods {
-		pod := pods[i]
-		if api.IsPodAvailable(pod, minReadySeconds, metav1.NewTime(time.Now())) {
-			available++
-		}
-	}
-	return available
-}
-
 // GetReadyReplicaCountForReplicationControllers returns the number of ready pods corresponding to
 // the given replication controller.
 func GetReadyReplicaCountForReplicationControllers(replicationControllers []*api.ReplicationController) int32 {
@@ -361,6 +349,18 @@ func GetReadyReplicaCountForReplicationControllers(replicationControllers []*api
 	return totalReadyReplicas
 }
 
+// GetAvailableReplicaCountForReplicationControllers returns the number of available pods corresponding to
+// the given replication controller.
+func GetAvailableReplicaCountForReplicationControllers(replicationControllers []*api.ReplicationController) int32 {
+	totalAvailableReplicas := int32(0)
+	for _, rc := range replicationControllers {
+		if rc != nil {
+			totalAvailableReplicas += rc.Status.AvailableReplicas
+		}
+	}
+	return totalAvailableReplicas
+}
+
 func DeploymentConfigNameFor(obj runtime.Object) string {
 	return annotationFor(obj, deployapi.DeploymentConfigAnnotation)
 }
@@ -472,16 +472,16 @@ func IsRollingConfig(config *deployapi.DeploymentConfig) bool {
 
 // IsProgressing expects a state deployment config and its updated status in order to
 // determine if there is any progress.
-func IsProgressing(config deployapi.DeploymentConfig, newStatus deployapi.DeploymentConfigStatus) bool {
+func IsProgressing(config *deployapi.DeploymentConfig, newStatus *deployapi.DeploymentConfigStatus) bool {
 	oldStatusOldReplicas := config.Status.Replicas - config.Status.UpdatedReplicas
 	newStatusOldReplicas := newStatus.Replicas - newStatus.UpdatedReplicas
 
 	return (newStatus.UpdatedReplicas > config.Status.UpdatedReplicas) || (newStatusOldReplicas < oldStatusOldReplicas)
 }
 
 // MaxUnavailable returns the maximum unavailable pods a rolling deployment config can take.
-func MaxUnavailable(config deployapi.DeploymentConfig) int32 {
-	if !IsRollingConfig(&config) {
+func MaxUnavailable(config *deployapi.DeploymentConfig) int32 {
+	if !IsRollingConfig(config) {
 		return int32(0)
 	}
 	// Error caught by validation