From e8efdab8c1cfcab5516404e6f2c01eee12cb44b9 Mon Sep 17 00:00:00 2001 From: Andrew Bayer Date: Mon, 20 Jul 2020 11:17:39 -0400 Subject: [PATCH] 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 --- pkg/cmd/gc/gc_activities.go | 4 +-- pkg/cmd/gc/gc_activities_test.go | 52 +++++++++++++++++++++++++++++--- 2 files changed, 50 insertions(+), 6 deletions(-) diff --git a/pkg/cmd/gc/gc_activities.go b/pkg/cmd/gc/gc_activities.go index f813743ec7..d2cf415282 100644 --- a/pkg/cmd/gc/gc_activities.go +++ b/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 diff --git a/pkg/cmd/gc/gc_activities_test.go b/pkg/cmd/gc/gc_activities_test.go index 81200d4dbe..df8151534d 100644 --- a/pkg/cmd/gc/gc_activities_test.go +++ b/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)