Skip to content

Commit

Permalink
fix(trafficrouting): apply stable selectors on canary service on roll…
Browse files Browse the repository at this point in the history 
…out abort argoproj#2781 (argoproj#2818)

* Re-apply stable to simple canary service on abort when no traffic routing is used
fixes argoproj#2781

* remove redundant block of code

Signed-off-by: mitchell amihod <[email protected]>

* Add an e2e covering the fix.

Basically a copy/pasta of test that was checking for this when using traffic routing

Signed-off-by: mitchell amihod <[email protected]>

---------

Signed-off-by: mitchell amihod <[email protected]>
  • Loading branch information
@meeech
meeech authored Jul 11, 2023
1 parent f1de890 commit 3c5ac36
Show file tree
Hide file tree
Showing 5 changed files with 247 additions and 18 deletions.
134 changes: 125 additions & 9 deletions rollout/canary_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -427,7 +427,6 @@ func TestResetCurrentStepIndexOnStepChange(t *testing.T) {
newConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "", false)
expectedPatch := fmt.Sprintf(expectedPatchWithoutPodHash, expectedCurrentPodHash, expectedCurrentStepHash, newConditions)
assert.Equal(t, calculatePatch(r2, expectedPatch), patch)

}

func TestResetCurrentStepIndexOnPodSpecChange(t *testing.T) {
Expand Down Expand Up @@ -469,7 +468,6 @@ func TestResetCurrentStepIndexOnPodSpecChange(t *testing.T) {

expectedPatch := fmt.Sprintf(expectedPatchWithoutPodHash, expectedCurrentPodHash, newConditions)
assert.Equal(t, calculatePatch(r2, expectedPatch), patch)

}

func TestCanaryRolloutCreateFirstReplicasetNoSteps(t *testing.T) {
Expand Down Expand Up @@ -708,7 +706,6 @@ 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
Expand Down Expand Up @@ -1019,9 +1016,8 @@ 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) {
Expand All @@ -1034,7 +1030,7 @@ func TestSyncRolloutIgnoreWaitOutsideOfReconciliationPeriod(t *testing.T) {
},
{
Pause: &v1alpha1.RolloutPause{
Duration: v1alpha1.DurationFromInt(3600), //1 hour
Duration: v1alpha1.DurationFromInt(3600), // 1 hour
},
},
}
Expand Down Expand Up @@ -1068,9 +1064,8 @@ 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) {
Expand All @@ -1084,7 +1079,8 @@ func TestSyncRolloutWaitIncrementStepIndex(t *testing.T) {
Pause: &v1alpha1.RolloutPause{
Duration: v1alpha1.DurationFromInt(5),
},
}, {
},
{
Pause: &v1alpha1.RolloutPause{},
},
}
Expand Down Expand Up @@ -1236,6 +1232,7 @@ func TestCanarySVCSelectors(t *testing.T) {
},
},
}

rc := rolloutContext{
log: logutil.WithRollout(rollout),
reconcilerBase: reconcilerBase{
Expand Down Expand Up @@ -1266,6 +1263,7 @@ func TestCanarySVCSelectors(t *testing.T) {
},
},
}

stopchan := make(chan struct{})
defer close(stopchan)
informers.Start(stopchan)
Expand All @@ -1286,6 +1284,124 @@ 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()
Expand Down
9 changes: 9 additions & 0 deletions rollout/service.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,15 @@ func (c *rolloutContext) reconcileStableAndCanaryService() error {
if err != nil {
return err
}

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
}
return nil
}

err = c.ensureSVCTargets(c.rollout.Spec.Strategy.Canary.CanaryService, c.newRS, true)
if err != nil {
return err
Expand Down
6 changes: 1 addition & 5 deletions rollout/trafficrouting.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,11 +143,7 @@ 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 {
Expand Down
25 changes: 21 additions & 4 deletions test/e2e/canary_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -443,7 +443,7 @@ spec:
port: 80
periodSeconds: 30
strategy:
canary:
canary:
steps:
- setWeight: 20
- pause: {}
Expand Down Expand Up @@ -539,7 +539,24 @@ func (s *CanarySuite) TestCanaryScaleDownOnAbort() {
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).
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
ExpectServiceSelector("canary-scaledowndelay-canary", map[string]string{"app": "canary-scaledowndelay", "rollouts-pod-template-hash": "66597877b7"}, false).
When().
Sleep(3*time.Second).
Expand Down Expand Up @@ -590,7 +607,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"])
Expand All @@ -599,7 +616,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)
}
91 changes: 91 additions & 0 deletions test/e2e/functional/canary-scaledownonabortnotrafficrouting.yaml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
apiVersion: v1
kind: Service
metadata:
name: canary-scaledowndelay-root
spec:
type: NodePort
ports:
- port: 80
targetPort: http
protocol: TCP
name: http
selector:
app: canary-scaledowndelay
---
apiVersion: v1
kind: Service
metadata:
name: canary-scaledowndelay-canary
spec:
type: NodePort
ports:
- port: 80
targetPort: http
protocol: TCP
name: http
selector:
app: canary-scaledowndelay
---
apiVersion: v1
kind: Service
metadata:
name: canary-scaledowndelay-stable
spec:
type: NodePort
ports:
- port: 80
targetPort: http
protocol: TCP
name: http
selector:
app: canary-scaledowndelay
---
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
name: canary-scaledowndelay-ingress
annotations:
kubernetes.io/ingress.class: alb
spec:
rules:
- http:
paths:
- path: /*
pathType: Prefix
backend:
service:
name: canary-scaledowndelay-root
port:
name: use-annotation
---
apiVersion: argoproj.io/v1alpha1
kind: Rollout
metadata:
name: canary-scaledownd-on-abort
spec:
selector:
matchLabels:
app: canary-scaledowndelay
template:
metadata:
labels:
app: canary-scaledowndelay
spec:
containers:
- name: canary-scaledowndelay
image: nginx:1.19-alpine
ports:
- name: http
containerPort: 80
protocol: TCP
resources:
requests:
memory: 16Mi
cpu: 5m
strategy:
canary:
canaryService: canary-scaledowndelay-canary
stableService: canary-scaledowndelay-stable
steps:
- setWeight: 50
- pause: {}

0 comments on commit 3c5ac36

Please sign in to comment.