fix: Increase max age of PipelineRuns for GCing

In practice, this results in builds that take longer than the max age
sometimes (though not always) failing to get their status actually
recorded properly. So let's increase that age, and also add a check to
make sure the `PipelineRun` is in a terminal state before we even
consider deleting it.

fixes #7444

Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

pkg/cmd/gc/gc_activities.go

@@ -105,7 +105,7 @@ func NewCmdGCActivities(commonOpts *opts.CommonOptions) *cobra.Command {
 	cmd.Flags().IntVarP(&options.PullRequestHistoryLimit, "pr-history-limit", "", 2, "Minimum number of PipelineActivities to keep around per repository Pull Request")
 	cmd.Flags().DurationVarP(&options.PullRequestAgeLimit, "pull-request-age", "p", time.Hour*48, "Maximum age to keep PipelineActivities for Pull Requests")
 	cmd.Flags().DurationVarP(&options.ReleaseAgeLimit, "release-age", "r", time.Hour*24*30, "Maximum age to keep PipelineActivities for Releases")
-	cmd.Flags().DurationVarP(&options.PipelineRunAgeLimit, "pipelinerun-age", "", time.Hour*2, "Maximum age to keep completed PipelineRuns for all pipelines")
+	cmd.Flags().DurationVarP(&options.PipelineRunAgeLimit, "pipelinerun-age", "", time.Hour*12, "Maximum age to keep completed PipelineRuns for all pipelines")
 	cmd.Flags().DurationVarP(&options.ProwJobAgeLimit, "prowjob-age", "", time.Hour*24*7, "Maximum age to keep completed ProwJobs for all pipelines")
 	return cmd
 }
@@ -258,7 +258,7 @@ func (o *GCActivitiesOptions) gcPipelineRuns(ns string) error {
 	for _, pr := range runList.Items {
 		pipelineRun := pr
 		completionTime := pipelineRun.Status.CompletionTime
-		if completionTime != nil && completionTime.Add(o.PipelineRunAgeLimit).Before(now) {
+		if pipelineRun.IsDone() && completionTime != nil && completionTime.Add(o.PipelineRunAgeLimit).Before(now) {
 			err = o.deletePipelineRun(pipelineRunInterface, &pipelineRun)
 			if err != nil {
 				return err

pkg/cmd/gc/gc_activities_test.go

@@ -12,8 +12,10 @@ import (
 	"github.com/jenkins-x/jx/v2/pkg/cmd/testhelpers"
 	tektonv1alpha1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
 	tektonv1beta1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	prowjobv1 "k8s.io/test-infra/prow/apis/prowjobs/v1"
+	"knative.dev/pkg/apis"
 
 	"github.com/stretchr/testify/assert"
 
@@ -157,7 +159,7 @@ func TestGCPipelineActivitiesWithBatchAndPRBuilds(t *testing.T) {
 	})
 	assert.NoError(t, err)
 
-	_, err = tektonClient.TektonV1alpha1().PipelineRuns(ns).Create(&tektonv1alpha1.PipelineRun{
+	run1 := &tektonv1alpha1.PipelineRun{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: "run1",
 		},
@@ -166,9 +168,15 @@ func TestGCPipelineActivitiesWithBatchAndPRBuilds(t *testing.T) {
 				CompletionTime: &metav1.Time{Time: nowMinusThreeHours},
 			},
 		},
+	}
+	run1.Status.SetCondition(&apis.Condition{
+		Type:   apis.ConditionSucceeded,
+		Status: corev1.ConditionFalse,
 	})
+	_, err = tektonClient.TektonV1alpha1().PipelineRuns(ns).Create(run1)
 	assert.NoError(t, err)
-	_, err = tektonClient.TektonV1alpha1().PipelineRuns(ns).Create(&tektonv1alpha1.PipelineRun{
+
+	run2 := &tektonv1alpha1.PipelineRun{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: "run2",
 		},
@@ -177,7 +185,29 @@ func TestGCPipelineActivitiesWithBatchAndPRBuilds(t *testing.T) {
 				CompletionTime: &metav1.Time{Time: nowMinusOneHour},
 			},
 		},
+	}
+	run2.Status.SetCondition(&apis.Condition{
+		Type:   apis.ConditionSucceeded,
+		Status: corev1.ConditionFalse,
 	})
+	_, err = tektonClient.TektonV1alpha1().PipelineRuns(ns).Create(run2)
+	assert.NoError(t, err)
+
+	run3 := &tektonv1alpha1.PipelineRun{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "run3",
+		},
+		Status: tektonv1alpha1.PipelineRunStatus{
+			PipelineRunStatusFields: tektonv1beta1.PipelineRunStatusFields{
+				CompletionTime: &metav1.Time{Time: nowMinusThreeHours},
+			},
+		},
+	}
+	run1.Status.SetCondition(&apis.Condition{
+		Type:   apis.ConditionSucceeded,
+		Status: corev1.ConditionUnknown,
+	})
+	_, err = tektonClient.TektonV1alpha1().PipelineRuns(ns).Create(run3)
 	assert.NoError(t, err)
 
 	_, err = prowJobClient.ProwV1().ProwJobs(ns).Create(&prowjobv1.ProwJob{
@@ -225,12 +255,26 @@ func TestGCPipelineActivitiesWithBatchAndPRBuilds(t *testing.T) {
 	runs, err := tektonClient.TektonV1alpha1().PipelineRuns(ns).List(metav1.ListOptions{})
 	assert.NoError(t, err)
 
-	assert.Len(t, runs.Items, 1, "One PipelineRun should've been garbage collected")
+	assert.Len(t, runs.Items, 2, "One PipelineRun should've been garbage collected")
+
+	var remainingRun *tektonv1alpha1.PipelineRun
+	var incompleteRun *tektonv1alpha1.PipelineRun
 
-	remainingRun := runs.Items[0]
+	for _, r := range runs.Items {
+		if r.Name == "run2" {
+			remainingRun = r.DeepCopy()
+		}
+		if r.Name == "run3" {
+			incompleteRun = r.DeepCopy()
+		}
+	}
+	assert.NotNil(t, remainingRun)
+	assert.NotNil(t, incompleteRun)
 	assert.NotNil(t, remainingRun.Status.CompletionTime)
 	assert.Equal(t, nowMinusOneHour, remainingRun.Status.CompletionTime.Time, "Expected completion time for remaining PipelineRun of %s, but is %s", nowMinusOneHour, remainingRun.Status.CompletionTime.Time)
 
+	assert.False(t, incompleteRun.IsDone())
+
 	jobs, err := prowJobClient.ProwV1().ProwJobs(ns).List(metav1.ListOptions{})
 	assert.NoError(t, err)