Keep canary alive when primary promotion fails

When the canary analysis succeeds, Flagger copies the canary pod spec
to the primary and waits for the primary rollout to finish. If the
primary fails to become ready, the non-retriable readiness error
triggered the standard analysis rollback, which routes all traffic to
the primary and scales the canary to zero.

During promotion the primary already runs the new (failing) spec while
the canary is the only healthy copy of the new revision still serving
traffic. Rolling back therefore sends all traffic to the broken primary
and deletes the working canary, taking the application down.

Halt the promotion instead: when the primary is not ready and the canary
is in the Promoting or Finalising phase, mark the rollout as failed and
alert, but keep the canary running and leave routing untouched until the
primary recovers or a corrected revision is applied.

Fixes #1898

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

Author: pedrampdd

pkg/controller/scheduler.go

@@ -297,7 +297,17 @@ func (c *Controller) advanceCanary(name string, namespace string) {
 		if err != nil {
 			c.recordEventWarningf(cd, "%v", err)
 			if !retriable {
-				c.rollback(cd, canaryController, meshRouter, scalerReconciler)
+				// Once the canary spec has been promoted to the primary, the canary
+				// is the only healthy copy of the new revision still serving traffic.
+				// The standard rollback would route all traffic to the unhealthy
+				// primary and scale the canary to zero, taking the application down.
+				// Halt the promotion instead and keep the canary running.
+				if cd.Status.Phase == flaggerv1.CanaryPhasePromoting ||
+					cd.Status.Phase == flaggerv1.CanaryPhaseFinalising {
+					c.promotionFailed(cd, canaryController, err)
+				} else {
+					c.rollback(cd, canaryController, meshRouter, scalerReconciler)
+				}
 			}
 			return
 		}
@@ -989,6 +999,33 @@ func (c *Controller) rollback(canary *flaggerv1.Canary, canaryController canary.
 	c.runPostRolloutHooks(canary, flaggerv1.CanaryPhaseFailed)
 }
 
+// promotionFailed marks the canary as failed when the primary does not become
+// ready after the canary spec has been promoted to it. Unlike rollback, it does
+// not route traffic to the primary nor scale the canary to zero: during promotion
+// the canary is the only healthy copy of the new revision serving traffic, so
+// rolling back to the unhealthy primary would cause an outage. The canary keeps
+// running until the primary recovers or a corrected revision is applied.
+func (c *Controller) promotionFailed(canary *flaggerv1.Canary, canaryController canary.Controller, 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 {
+		c.logger.With("canary", fmt.Sprintf("%s.%s", canary.Name, canary.Namespace)).Errorf("%v", err)
+		return
+	}
+
+	c.recorder.SetStatus(canary, flaggerv1.CanaryPhaseFailed)
+	c.recorder.IncFailures(metrics.CanaryMetricLabels{
+		Name:               canary.Spec.TargetRef.Name,
+		Namespace:          canary.Namespace,
+		DeploymentStrategy: canary.DeploymentStrategy(),
+		AnalysisStatus:     metrics.AnalysisStatusCompleted,
+	})
+	c.runPostRolloutHooks(canary, flaggerv1.CanaryPhaseFailed)
+}
+
 func (c *Controller) setPhaseInitialized(cd *flaggerv1.Canary, canaryController canary.Controller) error {
 	if cd.Status.Phase == "" || cd.Status.Phase == flaggerv1.CanaryPhaseInitializing {
 		cd.Status.Phase = flaggerv1.CanaryPhaseInitialized

pkg/controller/scheduler_deployment_fixture_test.go

@@ -82,6 +82,40 @@ func (f fixture) makeReady(t *testing.T, name string) {
 	require.NoError(t, err)
 }
 
+// makePrimaryNotReady puts the primary deployment into a stuck rollout state
+// (progress deadline exceeded) so that IsPrimaryReady returns a non-retriable error.
+func (f fixture) makePrimaryNotReady(t *testing.T) {
+	primaryName := fmt.Sprintf("%s-primary", f.canary.Spec.TargetRef.Name)
+	p, err := f.kubeClient.AppsV1().
+		Deployments("default").
+		Get(context.TODO(), primaryName, metav1.GetOptions{})
+	require.NoError(t, err)
+
+	p.Status = appsv1.DeploymentStatus{
+		ObservedGeneration: p.Generation,
+		Replicas:           1,
+		UpdatedReplicas:    1,
+		ReadyReplicas:      0,
+		AvailableReplicas:  0,
+		Conditions: []appsv1.DeploymentCondition{
+			{
+				Type:   appsv1.DeploymentProgressing,
+				Status: corev1.ConditionFalse,
+				Reason: "ProgressDeadlineExceeded",
+			},
+			{
+				Type:           appsv1.DeploymentAvailable,
+				Status:         corev1.ConditionFalse,
+				Reason:         "MinimumReplicasUnavailable",
+				LastUpdateTime: metav1.Now(),
+			},
+		},
+	}
+
+	_, err = f.kubeClient.AppsV1().Deployments("default").Update(context.TODO(), p, metav1.UpdateOptions{})
+	require.NoError(t, err)
+}
+
 func newDeploymentFixture(c *flaggerv1.Canary) fixture {
 	if c == nil {
 		c = newDeploymentTestCanary()

pkg/controller/scheduler_deployment_test.go

@@ -116,6 +116,72 @@ func TestScheduler_DeploymentRollback(t *testing.T) {
 	assert.Equal(t, flaggerv1.CanaryPhaseFailed, c.Status.Phase)
 }
 
+// TestScheduler_DeploymentPromotionPrimaryNotReady verifies that when the canary
+// analysis has succeeded and the new spec has been promoted to the primary, but
+// the primary fails to become ready, Flagger does NOT perform the destructive
+// analysis-failure rollback. Routing all traffic to the unhealthy primary and
+// scaling the (healthy) canary to zero would take the whole application down,
+// because during promotion the canary is the only running copy of the new
+// revision. See https://github.com/fluxcd/flagger/issues/1898.
+func TestScheduler_DeploymentPromotionPrimaryNotReady(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)
+	require.NoError(t, assertPhase(mocks.flaggerClient, "podinfo", flaggerv1.CanaryPhaseProgressing))
+
+	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: analysis succeeded, spec promoted to primary, now finishing promotion
+	cd, err := mocks.flaggerClient.FlaggerV1beta1().Canaries("default").Get(context.TODO(), "podinfo", metav1.GetOptions{})
+	require.NoError(t, err)
+	err = mocks.deployer.SetStatusPhase(cd, flaggerv1.CanaryPhasePromoting)
+	require.NoError(t, err)
+
+	// known routing state at promotion time (split between primary and canary)
+	require.NoError(t, mocks.router.SetRoutes(mocks.canary, 50, 50, false))
+
+	// the promoted primary fails to roll out
+	mocks.makePrimaryNotReady(t)
+
+	// advance: Flagger observes the primary is stuck
+	mocks.ctrl.advanceCanary("podinfo", "default")
+
+	// the canary must NOT be scaled to zero - it is the only healthy copy serving traffic
+	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")
+
+	// traffic must NOT be shifted entirely onto 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")
+
+	// 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)
+}
+
 func TestScheduler_DeploymentSkipAnalysis(t *testing.T) {
 	mocks := newDeploymentFixture(nil)
 	// initializing