fix(analysis): prevent promotion on inconclusive background run. Fixes: #3850

rollout/analysis.go

@@ -169,8 +169,14 @@ func needsNewAnalysisRun(currentAr *v1alpha1.AnalysisRun, rollout *v1alpha1.Roll
 	// is set and then seeing if the last status was inconclusive.
 	// There is an additional check for the BlueGreen Pause because the prepromotion analysis always has the BlueGreen
 	// Pause and that causes controllerPause to be set. The extra check for the BlueGreen Pause ensures that a new Analysis
-	// Run is created only when the previous AnalysisRun is inconclusive
-	if rollout.Status.ControllerPause && getPauseCondition(rollout, v1alpha1.PauseReasonBlueGreenPause) == nil {
+	// Run is created only when the previous AnalysisRun is inconclusive.
+	// Additional check for the Canary Pause prevents Canary promotion when AnalysisRun is inconclusive and reached
+	// inconclusiveLimit. Otherwise, another AnalysisRun will be spawned and can cause Success status,
+	// because of termination when the AnalysisRun is still in-flight.
+	if rollout.Status.ControllerPause &&
+		getPauseCondition(rollout, v1alpha1.PauseReasonCanaryPauseStep) == nil &&
+		getPauseCondition(rollout, v1alpha1.PauseReasonBlueGreenPause) == nil {
+
 		return currentAr.Status.Phase == v1alpha1.AnalysisPhaseInconclusive
 	}
 	return rollout.Status.AbortedAt != nil

test/e2e/analysis_test.go

@@ -27,6 +27,7 @@ func (s *AnalysisSuite) SetupSuite() {
 	s.E2ESuite.SetupSuite()
 	// shared analysis templates for suite
 	s.ApplyManifests("@functional/analysistemplate-web-background.yaml")
+	s.ApplyManifests("@functional/analysistemplate-web-background-inconclusive.yaml")
 	s.ApplyManifests("@functional/analysistemplate-sleep-job.yaml")
 	s.ApplyManifests("@functional/analysistemplate-multiple-job.yaml")
 	s.ApplyManifests("@functional/analysistemplate-fail-multiple-job.yaml")
@@ -68,6 +69,28 @@ func (s *AnalysisSuite) TestCanaryBackgroundAnalysis() {
 		WaitForBackgroundAnalysisRunPhase("Successful")
 }
 
+func (s *AnalysisSuite) TestCanaryInconclusiveBackgroundAnalysis() {
+	s.Given().
+		RolloutObjects("@functional/rollout-background-analysis-inconclusive.yaml").
+		When().
+		ApplyManifests().
+		WaitForRolloutStatus("Healthy").
+		Then().
+		ExpectAnalysisRunCount(0).
+		When().
+		UpdateSpec().
+		WaitForRolloutStatus("Paused").
+		WaitForBackgroundAnalysisRunPhase("Running").
+		Then().
+		ExpectAnalysisRunCount(1).
+		When().
+		WaitForBackgroundAnalysisRunPhase("Inconclusive").
+		WaitForRolloutMessage("InconclusiveAnalysisRun").
+		Then().
+		ExpectRolloutStatus("Paused").
+		ExpectRolloutMessage("InconclusiveAnalysisRun")
+}
+
 func (s *AnalysisSuite) TestCanaryInlineAnalysis() {
 	s.Given().
 		RolloutObjects("@functional/rollout-inline-analysis.yaml").

test/e2e/functional/analysistemplate-web-background-inconclusive.yaml

@@ -0,0 +1,22 @@
+# A web metric which uses the httpbin service as a metric provider
+apiVersion: argoproj.io/v1alpha1
+kind: AnalysisTemplate
+metadata:
+  name: web-background-inconclusive
+spec:
+  args:
+    - name: url-val
+      value: "https://httpbin.org/anything"
+  metrics:
+    - name: web
+      interval: 7s
+      inconclusiveLimit: 3
+      successCondition: result.status == "success"
+      failureCondition: result.status == "failure"
+      provider:
+        web:
+          url: "{{args.url-val}}"
+          method: POST
+          jsonBody:
+            status: "inconclusive"
+          jsonPath: "{$.json}"

test/e2e/functional/rollout-background-analysis-inconclusive.yaml

@@ -0,0 +1,30 @@
+apiVersion: argoproj.io/v1alpha1
+kind: Rollout
+metadata:
+  name: rollout-background-analysis-inconclusive
+spec:
+  replicas: 2
+  strategy:
+    canary:
+      steps:
+      - setWeight: 10
+      - pause: { duration: 30s }
+      analysis:
+        templates:
+        - templateName: web-background-inconclusive
+        startingStep: 1
+  selector:
+    matchLabels:
+      app: rollout-background-analysis-inconclusive
+  template:
+    metadata:
+      labels:
+        app: rollout-background-analysis-inconclusive
+    spec:
+      containers:
+      - name: rollouts-demo
+        image: nginx:1.19-alpine
+        resources:
+          requests:
+            memory: 16Mi
+            cpu: 5m

test/fixtures/then.go

@@ -55,6 +55,18 @@ func (t *Then) ExpectRolloutStatus(expectedStatus string) *Then {
 	return t
 }
 
+func (t *Then) ExpectRolloutMessage(expectedMessage string) *Then {
+	ro, err := t.rolloutClient.ArgoprojV1alpha1().Rollouts(t.namespace).Get(t.Context, t.rollout.GetName(), metav1.GetOptions{})
+	t.CheckError(err)
+	_, message := rolloututil.GetRolloutPhase(ro)
+	if message != expectedMessage {
+		t.log.Errorf("Rollout message expected to be '%s'. actual: %s", expectedMessage, message)
+		t.t.FailNow()
+	}
+	t.log.Infof("Rollout expectation status=%s met", expectedMessage)
+	return t
+}
+
 func (t *Then) ExpectReplicaCounts(desired, current, updated, ready, available any) *Then {
 	ro, err := t.rolloutClient.ArgoprojV1alpha1().Rollouts(t.namespace).Get(t.Context, t.rollout.GetName(), metav1.GetOptions{})
 	t.CheckError(err)

test/fixtures/when.go

@@ -278,6 +278,14 @@ func (w *When) WaitForRolloutStatus(status string, timeout ...time.Duration) *Wh
 	return w.WaitForRolloutCondition(checkStatus, fmt.Sprintf("status=%s", status), timeout...)
 }
 
+func (w *When) WaitForRolloutMessage(message string, timeout ...time.Duration) *When {
+	checkStatus := func(ro *rov1.Rollout) bool {
+		_, m := rolloututil.GetRolloutPhase(ro)
+		return m == message
+	}
+	return w.WaitForRolloutCondition(checkStatus, fmt.Sprintf("message=%s", message), timeout...)
+}
+
 func (w *When) MarkPodsReady(revision string, count int, timeouts ...time.Duration) *When {
 	ticker := time.NewTicker(time.Second)
 	defer ticker.Stop()