Revert "fix(trafficrouting): apply stable selectors on canary service…

… on rollout abort argoproj#2781 (argoproj#2818)"

This reverts commit 3c5ac36.

Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>

rollout/canary_test.go

@@ -708,6 +708,7 @@ func TestCanaryRolloutScaleDownOldRsDontScaleDownTooMuch(t *testing.T) {
 	assert.Equal(t, int32(0), *updatedRS1.Spec.Replicas)
 	updatedRS2 := f.getUpdatedReplicaSet(updatedRS2Index)
 	assert.Equal(t, int32(4), *updatedRS2.Spec.Replicas)
+
 }
 
 // TestCanaryDontScaleDownOldRsDuringInterruptedUpdate tests when we need to prevent scale down an
@@ -1068,8 +1069,9 @@ func TestSyncRolloutWaitAddToQueue(t *testing.T) {
 	c, i, k8sI := f.newController(func() time.Duration { return 30 * time.Minute })
 	f.runController(key, true, false, c, i, k8sI)
 
-	// When the controller starts, it will enqueue the rollout while syncing the informer and during the reconciliation step
+	//When the controller starts, it will enqueue the rollout while syncing the informer and during the reconciliation step
 	assert.Equal(t, 2, f.enqueuedObjects[key])
+
 }
 
 func TestSyncRolloutIgnoreWaitOutsideOfReconciliationPeriod(t *testing.T) {
@@ -1082,7 +1084,7 @@ func TestSyncRolloutIgnoreWaitOutsideOfReconciliationPeriod(t *testing.T) {
 		},
 		{
 			Pause: &v1alpha1.RolloutPause{
-				Duration: v1alpha1.DurationFromInt(3600), // 1 hour
+				Duration: v1alpha1.DurationFromInt(3600), //1 hour
 			},
 		},
 	}
@@ -1116,8 +1118,9 @@ func TestSyncRolloutIgnoreWaitOutsideOfReconciliationPeriod(t *testing.T) {
 	key := fmt.Sprintf("%s/%s", r2.Namespace, r2.Name)
 	c, i, k8sI := f.newController(func() time.Duration { return 30 * time.Minute })
 	f.runController(key, true, false, c, i, k8sI)
-	// When the controller starts, it will enqueue the rollout so we expect the rollout to enqueue at least once.
+	//When the controller starts, it will enqueue the rollout so we expect the rollout to enqueue at least once.
 	assert.Equal(t, 1, f.enqueuedObjects[key])
+
 }
 
 func TestSyncRolloutWaitIncrementStepIndex(t *testing.T) {
@@ -1131,8 +1134,7 @@ func TestSyncRolloutWaitIncrementStepIndex(t *testing.T) {
 			Pause: &v1alpha1.RolloutPause{
 				Duration: v1alpha1.DurationFromInt(5),
 			},
-		},
-		{
+		}, {
 			Pause: &v1alpha1.RolloutPause{},
 		},
 	}
@@ -1284,7 +1286,6 @@ func TestCanarySVCSelectors(t *testing.T) {
 				},
 			},
 		}
-
 		rc := rolloutContext{
 			log: logutil.WithRollout(rollout),
 			reconcilerBase: reconcilerBase{
@@ -1315,7 +1316,6 @@ func TestCanarySVCSelectors(t *testing.T) {
 				},
 			},
 		}
-
 		stopchan := make(chan struct{})
 		defer close(stopchan)
 		informers.Start(stopchan)
@@ -1336,124 +1336,6 @@ func TestCanarySVCSelectors(t *testing.T) {
 	}
 }
 
-func TestCanarySVCSelectorsBasicCanaryAbortServiceSwitchBack(t *testing.T) {
-	for _, tc := range []struct {
-		canaryReplicas      int32
-		canaryAvailReplicas int32
-		shouldAbortRollout  bool
-		shouldTargetNewRS   bool
-	}{
-		{2, 2, false, true}, // Rollout, canaryService should point at the canary RS
-		{2, 2, true, false}, // Rollout aborted, canaryService should point at the stable RS
-	} {
-		namespace := "namespace"
-		selectorLabel := "selector-labels-test"
-		selectorNewRSVal := "new-rs-xxx"
-		selectorStableRSVal := "stable-rs-xxx"
-		stableService := &corev1.Service{
-			ObjectMeta: metav1.ObjectMeta{
-				Name:        "stable",
-				Namespace:   namespace,
-				Annotations: map[string]string{v1alpha1.ManagedByRolloutsKey: selectorLabel},
-				Labels: map[string]string{
-					v1alpha1.DefaultRolloutUniqueLabelKey: selectorStableRSVal,
-				},
-			},
-		}
-		canaryService := &corev1.Service{
-			ObjectMeta: metav1.ObjectMeta{
-				Name:        "canary",
-				Namespace:   namespace,
-				Annotations: map[string]string{v1alpha1.ManagedByRolloutsKey: selectorLabel},
-			},
-		}
-		kubeclient := k8sfake.NewSimpleClientset(stableService, canaryService)
-		informers := k8sinformers.NewSharedInformerFactory(kubeclient, 0)
-		servicesLister := informers.Core().V1().Services().Lister()
-
-		rollout := &v1alpha1.Rollout{
-			ObjectMeta: metav1.ObjectMeta{
-				Name:      selectorLabel,
-				Namespace: namespace,
-			},
-			Spec: v1alpha1.RolloutSpec{
-				Strategy: v1alpha1.RolloutStrategy{
-					Canary: &v1alpha1.CanaryStrategy{
-						StableService: stableService.Name,
-						CanaryService: canaryService.Name,
-					},
-				},
-			},
-		}
-
-		pc := pauseContext{
-			rollout: rollout,
-		}
-		if tc.shouldAbortRollout {
-			pc.AddAbort("Add Abort")
-		}
-
-		rc := rolloutContext{
-			log:          logutil.WithRollout(rollout),
-			pauseContext: &pc,
-			reconcilerBase: reconcilerBase{
-				servicesLister: servicesLister,
-				kubeclientset:  kubeclient,
-				recorder:       record.NewFakeEventRecorder(),
-			},
-			rollout: rollout,
-			newRS: &v1.ReplicaSet{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "canary",
-					Namespace: namespace,
-					Labels: map[string]string{
-						v1alpha1.DefaultRolloutUniqueLabelKey: selectorNewRSVal,
-					},
-				},
-				Spec: v1.ReplicaSetSpec{
-					Replicas: pointer.Int32Ptr(tc.canaryReplicas),
-				},
-				Status: v1.ReplicaSetStatus{
-					AvailableReplicas: tc.canaryAvailReplicas,
-				},
-			},
-			stableRS: &v1.ReplicaSet{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "stable",
-					Namespace: namespace,
-					Labels: map[string]string{
-						v1alpha1.DefaultRolloutUniqueLabelKey: selectorStableRSVal,
-					},
-				},
-				Spec: v1.ReplicaSetSpec{
-					Replicas: pointer.Int32Ptr(tc.canaryReplicas),
-				},
-				Status: v1.ReplicaSetStatus{
-					AvailableReplicas: tc.canaryAvailReplicas,
-				},
-			},
-		}
-
-		stopchan := make(chan struct{})
-		defer close(stopchan)
-		informers.Start(stopchan)
-		informers.WaitForCacheSync(stopchan)
-		err := rc.reconcileStableAndCanaryService()
-		assert.NoError(t, err, "unable to reconcileStableAndCanaryService")
-		updatedCanarySVC, err := servicesLister.Services(rc.rollout.Namespace).Get(canaryService.Name)
-		assert.NoError(t, err, "unable to get updated canary service")
-		if tc.shouldTargetNewRS {
-			assert.Equal(t, selectorNewRSVal, updatedCanarySVC.Spec.Selector[v1alpha1.DefaultRolloutUniqueLabelKey],
-				"canary SVC should have newRS selector label when newRS has %d replicas and %d AvailableReplicas",
-				tc.canaryReplicas, tc.canaryAvailReplicas)
-		} else {
-			assert.Equal(t, selectorStableRSVal, updatedCanarySVC.Spec.Selector[v1alpha1.DefaultRolloutUniqueLabelKey],
-				"canary SVC should have stableRS selector label when newRS has %d replicas and %d AvailableReplicas",
-				tc.canaryReplicas, tc.canaryAvailReplicas)
-		}
-	}
-}
-
 func TestCanaryRolloutWithInvalidCanaryServiceName(t *testing.T) {
 	f := newFixture(t)
 	defer f.Close()

rollout/service.go

@@ -259,10 +259,11 @@ func (c *rolloutContext) reconcileStableAndCanaryService() error {
 	}
 
 	if c.pauseContext != nil && c.pauseContext.IsAborted() && c.rollout.Spec.Strategy.Canary.TrafficRouting == nil {
-		err = c.ensureSVCTargets(c.rollout.Spec.Strategy.Canary.CanaryService, c.stableRS, true)
-		if err != nil {
-			return err
-		}
+		// Problem with the ALB readinessGates
+		// err = c.ensureSVCTargets(c.rollout.Spec.Strategy.Canary.CanaryService, c.stableRS, true)
+		// if err != nil {
+		// 	return err
+		// }
 		return nil
 	}
 

rollout/trafficrouting.go

@@ -145,7 +145,11 @@ func (c *rolloutContext) reconcileTrafficRouting() error {
 		c.newStatus.Canary.Weights = nil
 		return nil
 	}
-
+	if reconcilers == nil {
+		// Not using traffic routing
+		c.newStatus.Canary.Weights = nil
+		return nil
+	}
 	c.log.Infof("Found %d TrafficRouting Reconcilers", len(reconcilers))
 	// iterate over the list of trafficReconcilers
 	for _, reconciler := range reconcilers {

test/e2e/canary_test.go

@@ -444,7 +444,7 @@ spec:
             port: 80
           periodSeconds: 30
   strategy:
-    canary:
+    canary: 
       steps:
       - setWeight: 20
       - pause: {}
@@ -540,24 +540,7 @@ func (s *CanarySuite) TestCanaryScaleDownOnAbort() {
 		AbortRollout().
 		WaitForRolloutStatus("Degraded").
 		Then().
-		// Expect that the canary service selector has been moved back to stable
-		ExpectServiceSelector("canary-scaledowndelay-canary", map[string]string{"app": "canary-scaledowndelay", "rollouts-pod-template-hash": "66597877b7"}, false).
-		When().
-		Sleep(3*time.Second).
-		Then().
-		ExpectRevisionPodCount("2", 0)
-}
-
-func (s *CanarySuite) TestCanaryScaleDownOnAbortNoTrafficRouting() {
-	s.Given().
-		HealthyRollout(`@functional/canary-scaledownonabortnotrafficrouting.yaml`).
-		When().
-		UpdateSpec(). // update to revision 2
-		WaitForRolloutStatus("Paused").
-		AbortRollout().
-		WaitForRolloutStatus("Degraded").
-		Then().
-		// Expect that the canary service selector has been moved back to stable
+		//Expect that the canary service selector has been moved back to stable
 		ExpectServiceSelector("canary-scaledowndelay-canary", map[string]string{"app": "canary-scaledowndelay", "rollouts-pod-template-hash": "66597877b7"}, false).
 		When().
 		Sleep(3*time.Second).
@@ -625,7 +608,7 @@ func (s *CanarySuite) TestCanaryDynamicStableScale() {
 		WaitForRevisionPodCount("2", 1).
 		Then().
 		ExpectRevisionPodCount("1", 4).
-		// Assert that the canary service selector is still not set to stable rs because of dynamic stable scale still in progress
+		//Assert that the canary service selector is still not set to stable rs because of dynamic stable scale still in progress
 		Assert(func(t *fixtures.Then) {
 			canarySvc, stableSvc := t.GetServices()
 			assert.NotEqual(s.T(), canarySvc.Spec.Selector["rollouts-pod-template-hash"], stableSvc.Spec.Selector["rollouts-pod-template-hash"])
@@ -634,7 +617,7 @@ func (s *CanarySuite) TestCanaryDynamicStableScale() {
 		MarkPodsReady("1", 1). // mark last remaining stable pod as ready (4/4 stable are ready)
 		WaitForRevisionPodCount("2", 0).
 		Then().
-		// Expect that the canary service selector is now set to stable because of dynamic stable scale is over and we have all pods up on stable rs
+		//Expect that the canary service selector is now set to stable because of dynamic stable scale is over and we have all pods up on stable rs
 		ExpectServiceSelector("dynamic-stable-scale-canary", map[string]string{"app": "dynamic-stable-scale", "rollouts-pod-template-hash": "868d98995b"}, false).
 		ExpectRevisionPodCount("1", 4)
 }