fix: rollback to stable with dynamicStableScale could overwhelm stabl…

…e pods (#3077) * fix: rollback to stable with dynamicStableScale could go under maxUnavailable Signed-off-by: Jesse Suen <[email protected]> * test: add unit tests Signed-off-by: Jesse Suen <[email protected]> * test: add e2e tests Signed-off-by: Jesse Suen <[email protected]> * refactor: move isReplicaSetReferenced to replicaset.go Signed-off-by: Jesse Suen <[email protected]> --------- Signed-off-by: Jesse Suen <[email protected]>
argoproj · Oct 23, 2023 · 7f25955 · 7f25955
1 parent 047f2a2
commit 7f25955
Show file tree

Hide file tree

Showing 15 changed files with 631 additions and 124 deletions.
diff --git a/rollout/bluegreen.go b/rollout/bluegreen.go
@@ -220,10 +220,9 @@ func (c *rolloutContext) scaleDownOldReplicaSetsForBlueGreen(oldRSs []*appsv1.Re
 	annotationedRSs := int32(0)
 	rolloutReplicas := defaults.GetReplicasOrDefault(c.rollout.Spec.Replicas)
 	for _, targetRS := range oldRSs {
-		if replicasetutil.IsStillReferenced(c.rollout.Status, targetRS) {
-			// We should technically never get here because we shouldn't be passing a replicaset list
-			// which includes referenced ReplicaSets. But we check just in case
-			c.log.Warnf("Prevented inadvertent scaleDown of RS '%s'", targetRS.Name)
+		if c.isReplicaSetReferenced(targetRS) {
+			// We might get here if user interrupted an an update in order to move back to stable.
+			c.log.Infof("Skip scale down of older RS '%s': still referenced", targetRS.Name)
 			continue
 		}
 		if *targetRS.Spec.Replicas == 0 {

diff --git a/rollout/canary.go b/rollout/canary.go
@@ -180,10 +180,9 @@ func (c *rolloutContext) scaleDownOldReplicaSetsForCanary(oldRSs []*appsv1.Repli
 
 	annotationedRSs := int32(0)
 	for _, targetRS := range oldRSs {
-		if replicasetutil.IsStillReferenced(c.rollout.Status, targetRS) {
-			// We should technically never get here because we shouldn't be passing a replicaset list
-			// which includes referenced ReplicaSets. But we check just in case
-			c.log.Warnf("Prevented inadvertent scaleDown of RS '%s'", targetRS.Name)
+		if c.isReplicaSetReferenced(targetRS) {
+			// We might get here if user interrupted an an update in order to move back to stable.
+			c.log.Infof("Skip scale down of older RS '%s': still referenced", targetRS.Name)
 			continue
 		}
 		if maxScaleDown <= 0 {
@@ -220,15 +219,8 @@ 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 !c.replicaSetReferencedByCanaryTraffic(targetRS) {
-					// It is safe to scale the intermediate RS down, if no traffic is directed to it.
-					c.log.Infof("scaling down intermediate RS '%s'", targetRS.Name)
-				} else {
-					c.log.Infof("Skip scaling down intermediate RS '%s': still referenced by service", targetRS.Name)
-					// This ReplicaSet is still referenced by the service. It is not safe to scale
-					// this down.
-					continue
-				}
+				// It is safe to scale the intermediate RS down, since no traffic is directed to it.
+				c.log.Infof("scaling down intermediate RS '%s'", targetRS.Name)
 			}
 		}
 		if *targetRS.Spec.Replicas == desiredReplicaCount {
@@ -248,19 +240,26 @@ func (c *rolloutContext) scaleDownOldReplicaSetsForCanary(oldRSs []*appsv1.Repli
 	return totalScaledDown, nil
 }
 
-func (c *rolloutContext) replicaSetReferencedByCanaryTraffic(rs *appsv1.ReplicaSet) bool {
-	rsPodHash := replicasetutil.GetPodTemplateHash(rs)
-	ro := c.rollout
-
-	if ro.Status.Canary.Weights == nil {
-		return false
-	}
-
-	if ro.Status.Canary.Weights.Canary.PodTemplateHash == rsPodHash || ro.Status.Canary.Weights.Stable.PodTemplateHash == rsPodHash {
-		return true
+// isDynamicallyRollingBackToStable returns true if we were in the middle of an canary update with
+// dynamic stable scaling, but was interrupted and are now rolling back to stable RS. This is similar
+// to, but different than aborting. With abort, desired hash != stable hash and so we know the
+// two hashes to balance traffic against. But with dynamically rolling back to stable, the
+// desired hash == stable hash, and so we must use the *previous* desired hash and balance traffic
+// between previous desired vs. stable hash, in order to safely shift traffic back to stable.
+// This function also returns the previous desired hash (where we are weighted to)
+func isDynamicallyRollingBackToStable(ro *v1alpha1.Rollout, desiredRS *appsv1.ReplicaSet) (bool, string) {
+	if rolloututil.IsFullyPromoted(ro) && ro.Spec.Strategy.Canary.TrafficRouting != nil && ro.Spec.Strategy.Canary.DynamicStableScale {
+		if ro.Status.Canary.Weights != nil {
+			currSelector := ro.Status.Canary.Weights.Canary.PodTemplateHash
+			desiredSelector := replicasetutil.GetPodTemplateHash(desiredRS)
+			if currSelector != desiredSelector {
+				if desiredRS.Status.AvailableReplicas < *ro.Spec.Replicas {
+					return true, currSelector
+				}
+			}
+		}
 	}
-
-	return false
+	return false, ""
 }
 
 // canProceedWithScaleDownAnnotation returns whether or not it is safe to proceed with annotating

diff --git a/rollout/canary_test.go b/rollout/canary_test.go
@@ -8,6 +8,7 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	appsv1 "k8s.io/api/apps/v1"
 	v1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -1890,3 +1891,120 @@ func TestHandleCanaryAbort(t *testing.T) {
 		assert.JSONEq(t, calculatePatch(r1, fmt.Sprintf(expectedPatch, newConditions)), patch)
 	})
 }
+
+func TestIsDynamicallyRollingBackToStable(t *testing.T) {
+	newRSWithHashAndReplicas := func(hash string, available int32) *appsv1.ReplicaSet {
+		return &appsv1.ReplicaSet{
+			ObjectMeta: metav1.ObjectMeta{
+				Labels: map[string]string{
+					v1alpha1.DefaultRolloutUniqueLabelKey: hash,
+				},
+			},
+			Status: v1.ReplicaSetStatus{
+				AvailableReplicas: available,
+			},
+		}
+	}
+
+	testCases := []struct {
+		name                         string
+		status                       v1alpha1.RolloutStatus
+		trafficRoutingDisabled       bool
+		dynamicStableScalingDisabled bool
+		rsHash                       string
+		rsAvailableReplicas          *int32 // if nil, will set to rollout replicas
+		trafficWeights               *v1alpha1.TrafficWeights
+		expectedResult               bool
+	}{
+		{
+			name:           "desired RS != stable RS",
+			status:         v1alpha1.RolloutStatus{CurrentPodHash: "abc123", StableRS: "def456"},
+			rsHash:         "",
+			expectedResult: false,
+		},
+		{
+			name:                   "not using traffic routing",
+			trafficRoutingDisabled: true,
+			status:                 v1alpha1.RolloutStatus{CurrentPodHash: "abc123", StableRS: "abc123"},
+			rsHash:                 "",
+			expectedResult:         false,
+		},
+		{
+			name:                         "not using dynamicStableScaling",
+			dynamicStableScalingDisabled: true,
+			status:                       v1alpha1.RolloutStatus{CurrentPodHash: "abc123", StableRS: "abc123"},
+			rsHash:                       "",
+			expectedResult:               false,
+		},
+		{
+			name: "weighted selector == desired RS",
+			status: v1alpha1.RolloutStatus{
+				CurrentPodHash: "abc123",
+				StableRS:       "abc123",
+				Canary: v1alpha1.CanaryStatus{
+					Weights: &v1alpha1.TrafficWeights{
+						Canary: v1alpha1.WeightDestination{
+							PodTemplateHash: "abc123",
+						},
+					},
+				},
+			},
+			rsHash:         "abc123",
+			expectedResult: false,
+		},
+		{
+			name: "weighted selector != desired RS, desired not fully available",
+			status: v1alpha1.RolloutStatus{
+				CurrentPodHash: "abc123",
+				StableRS:       "abc123",
+				Canary: v1alpha1.CanaryStatus{
+					Weights: &v1alpha1.TrafficWeights{
+						Canary: v1alpha1.WeightDestination{
+							PodTemplateHash: "def456",
+						},
+					},
+				},
+			},
+			rsHash:              "abc123",
+			rsAvailableReplicas: pointer.Int32(1),
+			expectedResult:      true,
+		},
+		{
+			name: "weighted selector != desired RS, desired RS is fully available",
+			status: v1alpha1.RolloutStatus{
+				CurrentPodHash: "abc123",
+				StableRS:       "abc123",
+				Canary: v1alpha1.CanaryStatus{
+					Weights: &v1alpha1.TrafficWeights{
+						Canary: v1alpha1.WeightDestination{
+							PodTemplateHash: "def456",
+						},
+					},
+				},
+			},
+			rsHash:         "abc123",
+			expectedResult: true,
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			ro := newCanaryRollout("test", 10, nil, nil, nil, intstr.FromInt(0), intstr.FromInt(1))
+			if !tc.trafficRoutingDisabled {
+				ro.Spec.Strategy.Canary.TrafficRouting = &v1alpha1.RolloutTrafficRouting{}
+			}
+			if !tc.dynamicStableScalingDisabled {
+				ro.Spec.Strategy.Canary.DynamicStableScale = true
+			}
+			ro.Status = tc.status
+
+			desiredRS := newRSWithHashAndReplicas(tc.rsHash, 1)
+			if tc.rsAvailableReplicas != nil {
+				desiredRS.Status.AvailableReplicas = *tc.rsAvailableReplicas
+			}
+
+			rbToStable, _ := isDynamicallyRollingBackToStable(ro, desiredRS)
+
+			assert.Equal(t, tc.expectedResult, rbToStable)
+		})
+	}
+}
diff --git a/rollout/replicaset.go b/rollout/replicaset.go
@@ -7,6 +7,7 @@ import (
 	"time"
 
 	appsv1 "k8s.io/api/apps/v1"
+	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	patchtypes "k8s.io/apimachinery/pkg/types"
@@ -15,6 +16,7 @@ import (
 	"github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1"
 	"github.com/argoproj/argo-rollouts/utils/defaults"
 	replicasetutil "github.com/argoproj/argo-rollouts/utils/replicaset"
+	serviceutil "github.com/argoproj/argo-rollouts/utils/service"
 	timeutil "github.com/argoproj/argo-rollouts/utils/time"
 )
 
@@ -296,3 +298,56 @@ func (c *rolloutContext) scaleDownDelayHelper(rs *appsv1.ReplicaSet, annotatione
 
 	return annotationedRSs, desiredReplicaCount, nil
 }
+
+// isReplicaSetReferenced returns if the given ReplicaSet is still being referenced by any of
+// the current, stable, blue-green services. Used to determine if the ReplicaSet can
+// safely be scaled to zero, or deleted.
+func (c *rolloutContext) isReplicaSetReferenced(rs *appsv1.ReplicaSet) bool {
+	rsPodHash := replicasetutil.GetPodTemplateHash(rs)
+	if rsPodHash == "" {
+		return false
+	}
+	ro := c.rollout
+	referencesToCheck := []string{
+		ro.Status.StableRS,
+		ro.Status.CurrentPodHash,
+		ro.Status.BlueGreen.ActiveSelector,
+		ro.Status.BlueGreen.PreviewSelector,
+	}
+	if ro.Status.Canary.Weights != nil {
+		referencesToCheck = append(referencesToCheck, ro.Status.Canary.Weights.Canary.PodTemplateHash, ro.Status.Canary.Weights.Stable.PodTemplateHash)
+	}
+	for _, ref := range referencesToCheck {
+		if ref == rsPodHash {
+			return true
+		}
+	}
+
+	// The above are static, lightweight checks to see if the selectors we record in our status are
+	// still referencing the ReplicaSet in question. Those checks aren't always enough. Next, we do
+	// a deeper check to look up the actual service objects, and see if they are still referencing
+	// the ReplicaSet. If so, we cannot scale it down.
+	var servicesToCheck []string
+	if ro.Spec.Strategy.Canary != nil {
+		servicesToCheck = []string{ro.Spec.Strategy.Canary.CanaryService, ro.Spec.Strategy.Canary.StableService}
+	} else {
+		servicesToCheck = []string{ro.Spec.Strategy.BlueGreen.ActiveService, ro.Spec.Strategy.BlueGreen.PreviewService}
+	}
+	for _, svcName := range servicesToCheck {
+		if svcName == "" {
+			continue
+		}
+		svc, err := c.servicesLister.Services(c.rollout.Namespace).Get(svcName)
+		if err != nil {
+			if k8serrors.IsNotFound(err) {
+				// service doesn't exist
+				continue
+			}
+			return true
+		}
+		if serviceutil.GetRolloutSelectorLabel(svc) == rsPodHash {
+			return true
+		}
+	}
+	return false
+}