fix: wait for full availability before switching selector

Signed-off-by: Jesse Suen <[email protected]>

rollout/canary.go

@@ -216,7 +216,7 @@ func (c *rolloutContext) scaleDownOldReplicaSetsForCanary(oldRSs []*appsv1.Repli
 				// and doesn't yet have scale down deadline. This happens when a user changes their
 				// mind in the middle of an V1 -> V2 update, and then applies a V3. We are deciding
 				// what to do with the defunct, intermediate V2 ReplicaSet right now.
-				if replicasetutil.IsReplicaSetReady(c.newRS) && replicasetutil.IsReplicaSetReady(c.stableRS) {
+				if replicasetutil.IsReplicaSetAvailable(c.newRS) && replicasetutil.IsReplicaSetAvailable(c.stableRS) {
 					// If the both new and old RS are available, we can infer that it is safe to
 					// scale down this ReplicaSet, since traffic should have shifted away from this RS.
 					// TODO: instead of checking availability of canary/stable, a better way to determine

rollout/canary_test.go

@@ -1186,7 +1186,7 @@ func TestCanarySVCSelectors(t *testing.T) {
 	}{
 		{0, 0, false},
 		{2, 0, false},
-		{2, 1, true},
+		{2, 1, false},
 		{2, 2, true},
 	} {
 		namespace := "namespace"

rollout/service.go

@@ -248,7 +248,7 @@ func (c *rolloutContext) reconcileStableAndCanaryService() error {
 }
 
 // ensureSVCTargets updates the service with the given name to point to the given ReplicaSet,
-// but only if that ReplicaSet has availability.
+// but only if that ReplicaSet has full availability.
 func (c *rolloutContext) ensureSVCTargets(svcName string, rs *appsv1.ReplicaSet) error {
 	if rs == nil || svcName == "" {
 		return nil
@@ -260,10 +260,10 @@ func (c *rolloutContext) ensureSVCTargets(svcName string, rs *appsv1.ReplicaSet)
 	currSelector := svc.Spec.Selector[v1alpha1.DefaultRolloutUniqueLabelKey]
 	desiredSelector := rs.Labels[v1alpha1.DefaultRolloutUniqueLabelKey]
 	if currSelector != desiredSelector {
-		// ensure we have available replicas, otherwise we will point the service to nothing
-		if rs.Status.AvailableReplicas == 0 {
+		// ensure ReplicaSet is available, otherwise we will point the service to nothing or an underprovisioned ReplicaSet
+		if !replicasetutil.IsReplicaSetAvailable(rs) {
 			logCtx := c.log.WithField(logutil.ServiceKey, svc.Name)
-			logCtx.Infof("delaying service switch from %s to %s: ReplicaSet not yet available", currSelector, desiredSelector)
+			logCtx.Infof("delaying service switch from %s to %s: ReplicaSet not fully available", currSelector, desiredSelector)
 			return nil
 		}
 		err = c.switchServiceSelector(svc, desiredSelector, c.rollout)

utils/replicaset/replicaset.go

@@ -623,12 +623,12 @@ func GetPodsOwnedByReplicaSet(ctx context.Context, client kubernetes.Interface,
 	return podOwnedByRS, nil
 }
 
-// IsReplicaSetReady returns if a ReplicaSet is scaled up and its ready count is >= desired count
-func IsReplicaSetReady(rs *appsv1.ReplicaSet) bool {
+// IsReplicaSetAvailable returns if a ReplicaSet is scaled up and its ready count is >= desired count
+func IsReplicaSetAvailable(rs *appsv1.ReplicaSet) bool {
 	if rs == nil {
 		return false
 	}
 	replicas := rs.Spec.Replicas
-	readyReplicas := rs.Status.ReadyReplicas
-	return replicas != nil && *replicas != 0 && readyReplicas != 0 && *replicas <= readyReplicas
+	availableReplicas := rs.Status.AvailableReplicas
+	return replicas != nil && *replicas != 0 && availableReplicas != 0 && *replicas <= availableReplicas
 }

utils/replicaset/replicaset_test.go

@@ -1249,53 +1249,69 @@ spec:
 	assert.True(t, PodTemplateEqualIgnoreHash(&live, &desired))
 }
 
-func TestIsReplicaSetReady(t *testing.T) {
+func TestIsReplicaSetAvailable(t *testing.T) {
 	{
-		assert.False(t, IsReplicaSetReady(nil))
+		assert.False(t, IsReplicaSetAvailable(nil))
 	}
 	{
 		rs := appsv1.ReplicaSet{
 			Spec: appsv1.ReplicaSetSpec{
 				Replicas: pointer.Int32Ptr(1),
 			},
 			Status: appsv1.ReplicaSetStatus{
-				ReadyReplicas: 0,
+				ReadyReplicas:     0,
+				AvailableReplicas: 0,
 			},
 		}
-		assert.False(t, IsReplicaSetReady(&rs))
+		assert.False(t, IsReplicaSetAvailable(&rs))
 	}
 	{
 		rs := appsv1.ReplicaSet{
 			Spec: appsv1.ReplicaSetSpec{
 				Replicas: pointer.Int32Ptr(1),
 			},
 			Status: appsv1.ReplicaSetStatus{
-				ReadyReplicas: 1,
+				ReadyReplicas:     1,
+				AvailableReplicas: 1,
 			},
 		}
-		assert.True(t, IsReplicaSetReady(&rs))
+		assert.True(t, IsReplicaSetAvailable(&rs))
 	}
 	{
 		rs := appsv1.ReplicaSet{
 			Spec: appsv1.ReplicaSetSpec{
 				Replicas: pointer.Int32Ptr(1),
 			},
 			Status: appsv1.ReplicaSetStatus{
-				ReadyReplicas: 2,
+				ReadyReplicas:     2,
+				AvailableReplicas: 2,
 			},
 		}
-		assert.True(t, IsReplicaSetReady(&rs))
+		assert.True(t, IsReplicaSetAvailable(&rs))
 	}
 	{
 		rs := appsv1.ReplicaSet{
 			Spec: appsv1.ReplicaSetSpec{
 				Replicas: pointer.Int32Ptr(0),
 			},
 			Status: appsv1.ReplicaSetStatus{
-				ReadyReplicas: 0,
+				ReadyReplicas:     0,
+				AvailableReplicas: 0,
 			},
 		}
-		// NOTE: currently consider scaled down replicas as not ready
-		assert.False(t, IsReplicaSetReady(&rs))
+		// NOTE: currently consider scaled down replicas as not available
+		assert.False(t, IsReplicaSetAvailable(&rs))
+	}
+	{
+		rs := appsv1.ReplicaSet{
+			Spec: appsv1.ReplicaSetSpec{
+				Replicas: pointer.Int32Ptr(0),
+			},
+			Status: appsv1.ReplicaSetStatus{
+				ReadyReplicas:     1,
+				AvailableReplicas: 0,
+			},
+		}
+		assert.False(t, IsReplicaSetAvailable(&rs))
 	}
 }