Merge pull request #1360 from fluxcd/fix-1359

Fix history truncation logic for RetryOnFailure

Author: matheuscscp

api/v2/snapshot_types.go

@@ -107,6 +107,15 @@ func (in *Snapshots) Truncate(ignoreTests bool) {
 	}
 }
 
+// TruncateIgnoringPreviousSnapshots sorts the Snapshots by version
+// in descending order and retains only the most recent 5 Snapshots.
+func (in *Snapshots) TruncateIgnoringPreviousSnapshots() {
+	in.SortByVersion()
+	if in.Len() > defaultMaxHistory {
+		*in = (*in)[:defaultMaxHistory]
+	}
+}
+
 // Snapshot captures a point-in-time copy of the status information for a Helm release,
 // as managed by the controller.
 type Snapshot struct {

api/v2/snapshot_types_test.go

@@ -296,3 +296,63 @@ func TestSnapshots_Truncate(t *testing.T) {
 		})
 	}
 }
+
+func TestSnapshots_TruncateIgnoringPreviousSnapshots(t *testing.T) {
+	tests := []struct {
+		name string
+		in   Snapshots
+		want Snapshots
+	}{
+		{
+			name: "keeps previous snapshot",
+			in: Snapshots{
+				{Version: 1, Status: "superseded"},
+				{Version: 3, Status: "failed"},
+				{Version: 2, Status: "superseded"},
+				{Version: 4, Status: "deployed"},
+			},
+			want: Snapshots{
+				{Version: 4, Status: "deployed"},
+				{Version: 3, Status: "failed"},
+				{Version: 2, Status: "superseded"},
+				{Version: 1, Status: "superseded"},
+			},
+		},
+		{
+			name: "retains most recent snapshots when all have failed",
+			in: Snapshots{
+				{Version: 6, Status: "deployed"},
+				{Version: 5, Status: "failed"},
+				{Version: 4, Status: "failed"},
+				{Version: 3, Status: "failed"},
+				{Version: 2, Status: "failed"},
+				{Version: 1, Status: "failed"},
+			},
+			want: Snapshots{
+				{Version: 6, Status: "deployed"},
+				{Version: 5, Status: "failed"},
+				{Version: 4, Status: "failed"},
+				{Version: 3, Status: "failed"},
+				{Version: 2, Status: "failed"},
+			},
+		},
+		{
+			name: "without previous snapshot",
+			in: Snapshots{
+				{Version: 1, Status: "deployed"},
+			},
+			want: Snapshots{
+				{Version: 1, Status: "deployed"},
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			tt.in.TruncateIgnoringPreviousSnapshots()
+
+			if !reflect.DeepEqual(tt.in, tt.want) {
+				t.Errorf("TruncateIgnoringPreviousSnapshots() got %v, want %v", tt.in, tt.want)
+			}
+		})
+	}
+}

internal/reconcile/atomic_release.go

@@ -344,15 +344,19 @@ func (r *AtomicRelease) actionForState(ctx context.Context, req *Request, state
 	case ReleaseStatusInSync:
 		log.Info("release in-sync with desired state")
 
-		// Remove all history up to the previous release action.
-		// We need to continue to hold on to the previous release result
-		// to ensure we can e.g. roll back when tests are enabled without
-		// any further changes to the release.
-		ignoreFailures := req.Object.GetTest().IgnoreFailures
-		if remediation := req.Object.GetActiveRemediation(); remediation != nil {
-			ignoreFailures = remediation.MustIgnoreTestFailures(req.Object.GetTest().IgnoreFailures)
+		if retry := req.Object.GetActiveRetry(); retry != nil {
+			req.Object.Status.History.TruncateIgnoringPreviousSnapshots()
+		} else {
+			// Remove all history up to the previous release action.
+			// We need to continue to hold on to the previous release result
+			// to ensure we can e.g. roll back when tests are enabled without
+			// any further changes to the release.
+			ignoreFailures := req.Object.GetTest().IgnoreFailures
+			if remediation := req.Object.GetActiveRemediation(); remediation != nil {
+				ignoreFailures = remediation.MustIgnoreTestFailures(req.Object.GetTest().IgnoreFailures)
+			}
+			req.Object.Status.History.Truncate(ignoreFailures)
 		}
-		req.Object.Status.History.Truncate(ignoreFailures)
 
 		if forceRequested {
 			log.Info(msgWithReason("forcing upgrade for in-sync release", "force requested through annotation"))
@@ -460,6 +464,8 @@ func (r *AtomicRelease) actionForState(ctx context.Context, req *Request, state
 		// If the action strategy is to retry (and not remediate), we behave just like
 		// "flux reconcile hr --force" and .spec.<action>.remediation.retries set to 0.
 		if req.Object.GetActiveRetry() != nil {
+			req.Object.Status.History.TruncateIgnoringPreviousSnapshots()
+
 			log.V(logger.DebugLevel).Info("retrying upgrade for failed release")
 			return NewUpgrade(r.configFactory, r.eventRecorder), nil
 		}