Route traffic to canary on promotion failure

On a failed promotion the canary keeps serving, but the traffic may
already have been shifted to the primary by runPromotionTrafficShift
before it started failing. Route all traffic back to the canary and
report the matching canary weight instead of zeroing it.

Addresses review feedback on #1931.

Signed-off-by: Pedram Pourmohammad <eragon.pedy@gmail.com>

Author: pedrampdd

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
 	}

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) {