Skip to content

Commit

Permalink
Re-apply stable to simple canary service on abort when no traffic rou…
Browse files Browse the repository at this point in the history 
…ting is used

fixes argoproj#2781

* remove redundant block of code

Signed-off-by: mitchell amihod <4623+meeech@users.noreply.github.com>
  • Loading branch information
@meeech
meeech committed Jun 29, 2023
1 parent bf6fb11 commit 3aa470d
Show file tree
Hide file tree
Showing 3 changed files with 135 additions and 14 deletions.
134 changes: 125 additions & 9 deletions rollout/canary_test.go
View file
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
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
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

0 comments on commit 3aa470d

Please sign in to comment.