diff --git a/pkg/controller/scheduler.go b/pkg/controller/scheduler.go index 5b3de8ed..a3acba6c 100644 --- a/pkg/controller/scheduler.go +++ b/pkg/controller/scheduler.go @@ -301,7 +301,7 @@ func (c *Controller) advanceCanary(name string, namespace string) { // instead of rolling back traffic to the unhealthy primary if cd.Status.Phase == flaggerv1.CanaryPhasePromoting || cd.Status.Phase == flaggerv1.CanaryPhaseFinalising { - c.handleFailedPromotion(cd, canaryController, err) + c.handleFailedPromotion(cd, canaryController, meshRouter, err) } else { c.rollback(cd, canaryController, meshRouter, scalerReconciler) } @@ -997,14 +997,28 @@ func (c *Controller) rollback(canary *flaggerv1.Canary, canaryController canary. } // handleFailedPromotion marks the rollout as failed when the primary is unhealthy -// during promotion, without scaling the canary down or routing to the primary. -func (c *Controller) handleFailedPromotion(canary *flaggerv1.Canary, canaryController canary.Controller, err error) { +// during promotion and routes all traffic back to the canary, the only healthy +// copy of the new revision. Traffic may already have been shifted to the primary +// by runPromotionTrafficShift, so it must be moved back explicitly. +func (c *Controller) handleFailedPromotion(canary *flaggerv1.Canary, canaryController canary.Controller, + meshRouter router.Interface, err error) { c.recordEventWarningf(canary, "Promotion of %s.%s failed, primary not ready: %v", canary.Spec.TargetRef.Name, canary.Namespace, err) c.alert(canary, fmt.Sprintf("Promotion failed, primary not ready: %v", err), false, flaggerv1.SeverityError) - if err := canaryController.SetStatusPhase(canary, flaggerv1.CanaryPhaseFailed); err != nil { + // route all traffic to the canary, off the unhealthy primary + primaryWeight := 0 + canaryWeight := c.totalWeight(canary) + if err := meshRouter.SetRoutes(canary, primaryWeight, canaryWeight, false); err != nil { + c.recordEventWarningf(canary, "%v", err) + return + } + c.recorder.SetWeight(canary, primaryWeight, canaryWeight) + + // mark as failed while reporting the weight that matches the routing + if err := canaryController.SyncStatus(canary, flaggerv1.CanaryStatus{ + Phase: flaggerv1.CanaryPhaseFailed, CanaryWeight: canaryWeight}); err != nil { c.logger.With("canary", fmt.Sprintf("%s.%s", canary.Name, canary.Namespace)).Errorf("%v", err) return } diff --git a/pkg/controller/scheduler_deployment_test.go b/pkg/controller/scheduler_deployment_test.go index 9e7c5704..47bce0c5 100644 --- a/pkg/controller/scheduler_deployment_test.go +++ b/pkg/controller/scheduler_deployment_test.go @@ -165,16 +165,76 @@ func TestScheduler_DeploymentPromotionPrimaryNotReady(t *testing.T) { require.NotNil(t, canaryDep.Spec.Replicas) assert.Equal(t, canaryReplicas, *canaryDep.Spec.Replicas, "canary must not be scaled to zero when promotion fails") - // traffic must NOT be shifted entirely onto the broken primary + // traffic must be routed to the healthy canary, off the broken primary primaryWeight, canaryWeight, _, err := mocks.router.GetRoutes(mocks.canary) require.NoError(t, err) - assert.NotEqual(t, 100, primaryWeight, "traffic must not be routed entirely to the unhealthy primary") - assert.Greater(t, canaryWeight, 0, "canary must keep serving traffic when promotion fails") + assert.Equal(t, 0, primaryWeight, "no traffic must remain on the unhealthy primary") + assert.Equal(t, 100, canaryWeight, "all traffic must be routed to the healthy canary") // the rollout is reported as failed so it stops advancing and alerts c, err := mocks.flaggerClient.FlaggerV1beta1().Canaries("default").Get(context.TODO(), "podinfo", metav1.GetOptions{}) require.NoError(t, err) assert.Equal(t, flaggerv1.CanaryPhaseFailed, c.Status.Phase) + assert.Equal(t, 100, c.Status.CanaryWeight, "reported canary weight must match the traffic on the canary") +} + +// when the primary fails after promotion traffic has already been shifted to it +// (Finalising phase), traffic must be routed back to the healthy canary (#1898) +func TestScheduler_DeploymentPromotionFailedAfterTrafficShift(t *testing.T) { + mocks := newDeploymentFixture(nil) + + // initializing + mocks.ctrl.advanceCanary("podinfo", "default") + mocks.makePrimaryReady(t) + + // initialized + mocks.ctrl.advanceCanary("podinfo", "default") + + // update + dep2 := newDeploymentTestDeploymentV2() + _, err := mocks.kubeClient.AppsV1().Deployments("default").Update(context.TODO(), dep2, metav1.UpdateOptions{}) + require.NoError(t, err) + + // detect changes -> progressing, canary scaled up + mocks.ctrl.advanceCanary("podinfo", "default") + mocks.makeCanaryReady(t) + + canaryDep, err := mocks.kubeClient.AppsV1().Deployments("default").Get(context.TODO(), "podinfo", metav1.GetOptions{}) + require.NoError(t, err) + require.NotNil(t, canaryDep.Spec.Replicas) + canaryReplicas := *canaryDep.Spec.Replicas + require.Greater(t, canaryReplicas, int32(0)) + + // simulate: promotion already shifted all traffic to the primary (Finalising) + cd, err := mocks.flaggerClient.FlaggerV1beta1().Canaries("default").Get(context.TODO(), "podinfo", metav1.GetOptions{}) + require.NoError(t, err) + err = mocks.deployer.SetStatusPhase(cd, flaggerv1.CanaryPhaseFinalising) + require.NoError(t, err) + require.NoError(t, mocks.router.SetRoutes(mocks.canary, 100, 0, false)) + + // the promoted primary then fails to stay ready + mocks.makePrimaryNotReady(t) + + // advance: Flagger observes the primary is stuck + mocks.ctrl.advanceCanary("podinfo", "default") + + // traffic must be routed back to the healthy canary, off the broken primary + primaryWeight, canaryWeight, _, err := mocks.router.GetRoutes(mocks.canary) + require.NoError(t, err) + assert.Equal(t, 0, primaryWeight, "no traffic must remain on the unhealthy primary") + assert.Equal(t, 100, canaryWeight, "all traffic must be routed to the healthy canary") + + // the canary must not be scaled to zero + canaryDep, err = mocks.kubeClient.AppsV1().Deployments("default").Get(context.TODO(), "podinfo", metav1.GetOptions{}) + require.NoError(t, err) + require.NotNil(t, canaryDep.Spec.Replicas) + assert.Equal(t, canaryReplicas, *canaryDep.Spec.Replicas, "canary must not be scaled to zero when promotion fails") + + // reported canary weight must match the routing (not zeroed) + c, err := mocks.flaggerClient.FlaggerV1beta1().Canaries("default").Get(context.TODO(), "podinfo", metav1.GetOptions{}) + require.NoError(t, err) + assert.Equal(t, flaggerv1.CanaryPhaseFailed, c.Status.Phase) + assert.Equal(t, 100, c.Status.CanaryWeight, "reported canary weight must match the traffic on the canary") } func TestScheduler_DeploymentSkipAnalysis(t *testing.T) {