Skip to content

Commit

Permalink
Revert "fix: switch service selector back to stable on canary service…
Browse files Browse the repository at this point in the history
… when aborted (argoproj#2540)"

This reverts commit 81b015d.

Signed-off-by: Zach Aller <[email protected]>
  • Loading branch information
zachaller committed Dec 10, 2024
1 parent 5a1fa16 commit a840781
Show file tree
Hide file tree
Showing 3 changed files with 10 additions and 14 deletions.
12 changes: 0 additions & 12 deletions rollout/trafficrouting.go
Original file line number Diff line number Diff line change
Expand Up @@ -179,22 +179,10 @@ func (c *rolloutContext) reconcileTrafficRouting() error {
}
} else if c.pauseContext.IsAborted() {
desiredWeight = c.calculateDesiredWeightOnAbortOrStableRollback()
if (c.rollout.Spec.Strategy.Canary.DynamicStableScale && desiredWeight == 0) || !c.rollout.Spec.Strategy.Canary.DynamicStableScale {
// If we are using dynamic stable scale we need to also make sure that desiredWeight=0 aka we are completely
// done with aborting before resetting the canary service selectors back to stable. For non-dynamic scale we do not check for availability because we are
// fully aborted and stable pods will be there, if we check for availability it causes issues with ALB readiness gates if all stable pods
// have the desired readiness gate on them during an abort we get stuck in a loop because all the stable go unready and rollouts won't be able
// to switch the desired services because there is no ready pods which causes pods to get stuck progressing forever waiting for readiness.
err = c.ensureSVCTargets(c.rollout.Spec.Strategy.Canary.CanaryService, c.stableRS, false)
if err != nil {
return err
}
}
err := reconciler.RemoveManagedRoutes()
if err != nil {
return err
}

} else if c.newRS == nil || c.newRS.Status.AvailableReplicas == 0 {
// when newRS is not available or replicas num is 0. never weight to canary
weightDestinations = append(weightDestinations, c.calculateWeightDestinationsFromExperiment()...)
Expand Down
3 changes: 1 addition & 2 deletions rollout/trafficrouting_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -952,7 +952,6 @@ func TestDynamicScalingDontIncreaseWeightWhenAborted(t *testing.T) {
f.objects = append(f.objects, r2)

f.expectPatchRolloutAction(r2)
f.expectPatchServiceAction(canarySvc, rs1PodHash)

f.fakeTrafficRouting = newUnmockedFakeTrafficRoutingReconciler()
f.fakeTrafficRouting.On("UpdateHash", mock.Anything, mock.Anything, mock.Anything).Return(nil)
Expand Down Expand Up @@ -1096,7 +1095,7 @@ func TestDynamicScalingDecreaseWeightAccordingToStableAvailabilityWhenAbortedAnd
f.objects = append(f.objects, r2)

f.expectPatchRolloutAction(r2)
f.expectPatchServiceAction(canarySvc, rs1PodHash)
//f.expectPatchServiceAction(canarySvc, rs1PodHash)

f.fakeTrafficRouting = newUnmockedFakeTrafficRoutingReconciler()
f.fakeTrafficRouting.On("UpdateHash", mock.Anything, mock.Anything, mock.Anything).Return(nil)
Expand Down
9 changes: 9 additions & 0 deletions test/e2e/canary_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -567,10 +567,13 @@ func (s *CanarySuite) TestCanaryScaleDownOnAbort() {
WaitForRolloutStatus("Paused").
AbortRollout().
WaitForRolloutStatus("Degraded").
<<<<<<< HEAD

Check failure on line 570 in test/e2e/canary_test.go

View workflow job for this annotation

GitHub Actions / Run end-to-end tests (1.28, false)

expected selector or type assertion, found '<<'

Check failure on line 570 in test/e2e/canary_test.go

View workflow job for this annotation

GitHub Actions / Run end-to-end tests (1.28, false)

expected selector or type assertion, found '<<'

Check failure on line 570 in test/e2e/canary_test.go

View workflow job for this annotation

GitHub Actions / Run end-to-end tests (1.29, false)

expected selector or type assertion, found '<<'

Check failure on line 570 in test/e2e/canary_test.go

View workflow job for this annotation

GitHub Actions / Run end-to-end tests (1.29, false)

expected selector or type assertion, found '<<'

Check failure on line 570 in test/e2e/canary_test.go

View workflow job for this annotation

GitHub Actions / Run end-to-end tests (1.30, false)

expected selector or type assertion, found '<<'

Check failure on line 570 in test/e2e/canary_test.go

View workflow job for this annotation

GitHub Actions / Run end-to-end tests (1.30, false)

expected selector or type assertion, found '<<'

Check failure on line 570 in test/e2e/canary_test.go

View workflow job for this annotation

GitHub Actions / Run end-to-end tests (1.31, true)

expected selector or type assertion, found '<<'

Check failure on line 570 in test/e2e/canary_test.go

View workflow job for this annotation

GitHub Actions / Run end-to-end tests (1.31, true)

expected selector or type assertion, found '<<'
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().
=======
>>>>>>> parent of 81b015d15 (fix: switch service selector back to stable on canary service when aborted (#2540))
Sleep(3*time.Second).
Then().
ExpectRevisionPodCount("2", 0)
Expand Down Expand Up @@ -654,18 +657,24 @@ func (s *CanarySuite) TestCanaryDynamicStableScale() {
WaitForRevisionPodCount("2", 1).
Then().
ExpectRevisionPodCount("1", 4).
<<<<<<< HEAD
// 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"])
}).
=======
>>>>>>> parent of 81b015d15 (fix: switch service selector back to stable on canary service when aborted (#2540))
When().
MarkPodsReady("1", 1). // mark last remaining stable pod as ready (4/4 stable are ready)
WaitForRevisionPodCount("2", 0).
Sleep(2*time.Second). //WaitForRevisionPodCount does not wait for terminating pods and so ExpectServiceSelector fails sleep a bit for the terminating pods to be deleted
Then().
<<<<<<< HEAD
// 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).
=======
>>>>>>> parent of 81b015d15 (fix: switch service selector back to stable on canary service when aborted (#2540))
ExpectRevisionPodCount("1", 4)
}

Expand Down

0 comments on commit a840781

Please sign in to comment.