feat: wait for first batch of pods to be ready before adding canary s…

…elector label to SVC

Signed-off-by: Aaron Weisberg <[email protected]>

USERS.md

@@ -13,3 +13,4 @@ Organizations below are **officially** using Argo Rollouts. Please send a PR wit
 1. [Quipper](https://www.quipper.com/)
 1. [Devtron Labs](https://github.com/devtron-labs/devtron)
 1. [Quizlet](https://quizlet.com)
+1. [Skillz](https://www.skillz.com)

rollout/canary_test.go

@@ -8,6 +8,7 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	v1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -18,6 +19,8 @@ import (
 
 	"github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1"
 	"github.com/argoproj/argo-rollouts/pkg/client/clientset/versioned/fake"
+	k8sinformers "k8s.io/client-go/informers"
+
 	"github.com/argoproj/argo-rollouts/utils/annotations"
 	"github.com/argoproj/argo-rollouts/utils/conditions"
 	logutil "github.com/argoproj/argo-rollouts/utils/log"
@@ -1033,9 +1036,9 @@ func TestCanaryRolloutWithCanaryService(t *testing.T) {
 	f := newFixture(t)
 	defer f.Close()
 
-	rollout := newCanaryRollout("foo", 0, nil, nil, nil, intstr.FromInt(1), intstr.FromInt(0))
+	rollout := newCanaryRollout("foo", 1, nil, nil, nil, intstr.FromInt(1), intstr.FromInt(0))
 	canarySvc := newService("canary", 80, nil, rollout)
-	rs := newReplicaSetWithStatus(rollout, 0, 0)
+	rs := newReplicaSetWithStatus(rollout, 1, 1)
 	rollout.Spec.Strategy.Canary.CanaryService = canarySvc.Name
 
 	f.rolloutLister = append(f.rolloutLister, rollout)
@@ -1048,6 +1051,98 @@ func TestCanaryRolloutWithCanaryService(t *testing.T) {
 	f.run(getKey(rollout, t))
 }
 
+func TestCanarySVCSelectors(t *testing.T) {
+	testTables := []struct {
+		canaryReplicas      int32
+		canaryReadyReplicas int32
+
+		shouldTargetNewRS bool
+	}{
+		{0, 0, false},
+		{2, 0, false},
+		{2, 1, false},
+		{2, 2, true},
+	}
+
+	for _, tt := range testTables {
+		namespace := "namespace"
+		selectorNewRSVal := "new-rs-xxx"
+		stableService := &corev1.Service{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "stable",
+				Namespace: namespace,
+			},
+		}
+		canaryService := &corev1.Service{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "canary",
+				Namespace: namespace,
+			},
+		}
+		kubeclient := k8sfake.NewSimpleClientset(stableService, canaryService)
+		informers := k8sinformers.NewSharedInformerFactory(kubeclient, 0)
+		servicesLister := informers.Core().V1().Services().Lister()
+
+		rollout := &v1alpha1.Rollout{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "selector-labels-test",
+				Namespace: namespace,
+			},
+			Spec: v1alpha1.RolloutSpec{
+				Strategy: v1alpha1.RolloutStrategy{
+					Canary: &v1alpha1.CanaryStrategy{
+						StableService: stableService.Name,
+						CanaryService: canaryService.Name,
+					},
+				},
+			},
+		}
+		rc := rolloutContext{
+			log: logutil.WithRollout(rollout),
+			reconcilerBase: reconcilerBase{
+				servicesLister: servicesLister,
+				kubeclientset:  kubeclient,
+				recorder:       &FakeEventRecorder{},
+			},
+			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(tt.canaryReplicas),
+				},
+				Status: v1.ReplicaSetStatus{
+					ReadyReplicas: tt.canaryReadyReplicas,
+				},
+			},
+			stableRS: &v1.ReplicaSet{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "stable",
+					Namespace: namespace,
+				},
+			},
+		}
+		stopchan := make(chan struct{})
+		defer close(stopchan)
+		informers.Start(stopchan)
+		informers.WaitForCacheSync(stopchan)
+		err := rc.reconcileStableAndCanaryService()
+		if err != nil {
+			t.Error(err.Error())
+		}
+		updatedCanarySVC, err := servicesLister.Services(rc.rollout.Namespace).Get(canaryService.Name)
+		assert.NoError(t, err, "unable to get updated canary service")
+		if tt.shouldTargetNewRS {
+			assert.Equal(t, selectorNewRSVal, updatedCanarySVC.Spec.Selector[v1alpha1.DefaultRolloutUniqueLabelKey], "canary SVC is missing newRS selector label")
+		} else {
+			assert.Equal(t, "", updatedCanarySVC.Spec.Selector[v1alpha1.DefaultRolloutUniqueLabelKey], "canary SVC should not have newRS selector label")
+		}
+	}
+}
+
 func TestCanaryRolloutWithInvalidCanaryServiceName(t *testing.T) {
 	f := newFixture(t)
 	defer f.Close()
@@ -1084,9 +1179,9 @@ func TestCanaryRolloutWithStableService(t *testing.T) {
 	f := newFixture(t)
 	defer f.Close()
 
-	rollout := newCanaryRollout("foo", 0, nil, nil, nil, intstr.FromInt(1), intstr.FromInt(0))
+	rollout := newCanaryRollout("foo", 1, nil, nil, nil, intstr.FromInt(1), intstr.FromInt(0))
 	stableSvc := newService("stable", 80, nil, rollout)
-	rs := newReplicaSetWithStatus(rollout, 0, 0)
+	rs := newReplicaSetWithStatus(rollout, 1, 1)
 	rollout.Spec.Strategy.Canary.StableService = stableSvc.Name
 	rollout.Status.StableRS = rs.Labels[v1alpha1.DefaultRolloutUniqueLabelKey]
 

rollout/service.go

@@ -126,6 +126,12 @@ func (c *rolloutContext) reconcileStableAndCanaryService() error {
 	if c.rollout.Spec.Strategy.Canary == nil {
 		return nil
 	}
+	if c.newRS == nil || c.newRS.Status.ReadyReplicas == 0 {
+		return nil
+	}
+	if c.newRS.Spec.Replicas == nil || *c.newRS.Spec.Replicas > c.newRS.Status.ReadyReplicas {
+		return nil
+	}
 	if c.rollout.Spec.Strategy.Canary.StableService != "" && c.stableRS != nil {
 		svc, err := c.servicesLister.Services(c.rollout.Namespace).Get(c.rollout.Spec.Strategy.Canary.StableService)
 		if err != nil {