Repository: kubernetes-sigs/prow

Issue:
Config to automatically Re-trigger failed periodics
OpenShift has certain infra related periodics that run on a daily (or similar) frequency. This is only because the jobs are sometimes flaky, and the subsequent run will pass. The frequency could be reduced to weekly if there was a guarantee that the job would be retried a number of times if it fails.

A new config could be added to support automatically re-triggering a periodic ProwJob only in the case that it fails. It would accept the number of times to retry, and the interval at which to trigger the re-run. Something like the following to retrigger a failed job `3` times, `6` hours apart:
```yaml
retrigger-failed-run:
  attempts: 3
  interval: 6h
```

Implementation details:
I think that it may be possible for `plank` to handle the retriggers

Discussion:
@smg247: /cc @stbenjam@stbenjam: Thanks, this is great! 

The only question I have is if the job fails, if we should be able to unequivocally run it 3 more times, or only run it until it succeeds.  The former would help offset the bad signal and give us more confidence in the job's reliability.

Maybe configurable? 

```yaml
retrigger-failed-run:
  strategy: until_success | run_all
  attempts: 3
  interval: 6h
```

/cc @deads2k

@deads2k: Configurable would be good.
1. Sometimes we want definitely three more times
2. Sometimes we want run up to three more times for a success.@petr-muller: Retest until success seems universally useful but I'm not a fan of having logic where a single passing job is good but a single failing job results in multiple passing jobs later.

> I think that it may be possible for plank to handle the retriggers

Almost certainly not plank. Plank consumes Prowjobs, should not create them (unless you'd do the re-runs as additional Pods for a single Prowjob, for which we would need to rethink big parts of e.g. artifact reporting). I believe this belongs to horologium, especially with the `interval: 6h` config. We'd probably need some horologium-specific annotations on Prowjobs to recognize their position in a retest series and prevent each subsequent failure to cause a new round of retests.

There are more fun interactions to resolve, like how do the retests interact with standard interval-triggered periodics? Would they delay them?@stbenjam: > Retest until success seems universally useful but I'm not a fan of having logic where a single passing job is good but a single failing job results in multiple passing jobs later.

For infrequently run jobs, we want to be able to still detect subtler regressions. If a developer makes an existing test go from 99% to 50%, we'll eventually get a failure on weekly runs -- that's our first hint there's something wrong, but we need more than 1 additional attempt to confirm.  We'd get it eventually but it could take a month+.  The unconditional attempts is a signal booster.@petr-muller: Okay, that makes sense, I see the value now :+1: It helps to amplify subtle decreases in reliability while saving resources because jobs that we think are solid may not need to run as often.@k8s-triage-robot: The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:
- After 90d of inactivity, `lifecycle/stale` is applied
- After 30d of inactivity since `lifecycle/stale` was applied, `lifecycle/rotten` is applied
- After 30d of inactivity since `lifecycle/rotten` was applied, the issue is closed

You can:
- Mark this issue as fresh with `/remove-lifecycle stale`
- Close this issue with `/close`
- Offer to help out with [Issue Triage][1]

Please send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community).

/lifecycle stale

[1]: https://www.kubernetes.dev/docs/guide/issue-triage/@smg247: /remove-lifecycle stale@jmguzik: /assign @jmguzik @k8s-triage-robot: The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:
- After 90d of inactivity, `lifecycle/stale` is applied
- After 30d of inactivity since `lifecycle/stale` was applied, `lifecycle/rotten` is applied
- After 30d of inactivity since `lifecycle/rotten` was applied, the issue is closed

You can:
- Mark this issue as fresh with `/remove-lifecycle stale`
- Close this issue with `/close`
- Offer to help out with [Issue Triage][1]

Please send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community).

/lifecycle stale

[1]: https://www.kubernetes.dev/docs/guide/issue-triage/@petr-muller: This was implemented in #358 

/close@k8s-ci-robot: @petr-muller: Closing this issue.

<details>

In response to [this](https://github.com/kubernetes-sigs/prow/issues/268#issuecomment-2806734192):

>This was implemented in #358 
>
>/close


Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
</details>

Solution patch:
diff --git a/cmd/horologium/main.go b/cmd/horologium/main.go
index 3ce9e9df8..52532c503 100644
--- a/cmd/horologium/main.go
+++ b/cmd/horologium/main.go
@@ -21,6 +21,7 @@ import (
 	"flag"
 	"fmt"
 	"os"
+	"strconv"
 	"time"
 
 	"github.com/sirupsen/logrus"
@@ -30,12 +31,14 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/cluster"
 
 	prowapi "sigs.k8s.io/prow/pkg/apis/prowjobs/v1"
+	v1 "sigs.k8s.io/prow/pkg/apis/prowjobs/v1"
 	"sigs.k8s.io/prow/pkg/config"
 	"sigs.k8s.io/prow/pkg/cron"
 	pkgflagutil "sigs.k8s.io/prow/pkg/flagutil"
 	prowflagutil "sigs.k8s.io/prow/pkg/flagutil"
 	configflagutil "sigs.k8s.io/prow/pkg/flagutil/config"
 	"sigs.k8s.io/prow/pkg/interrupts"
+	"sigs.k8s.io/prow/pkg/kube"
 	"sigs.k8s.io/prow/pkg/logrusutil"
 	"sigs.k8s.io/prow/pkg/metrics"
 	"sigs.k8s.io/prow/pkg/pjutil"
@@ -44,6 +47,7 @@ import (
 
 const (
 	defaultTickInterval = time.Minute
+	maxRetries          = 10
 )
 
 type options struct {
@@ -206,8 +210,17 @@ func sync(prowJobClient ctrlruntimeclient.Client, cfg *config.Config, cr cronCli
 				"job":            p.JobBase.Name,
 			}).Debug("Trigger time has not yet been reached.")
 		}
-		if !previousFound || shouldTrigger {
-			prowJob := pjutil.NewProwJob(pjutil.PeriodicSpec(p), p.Labels, p.Annotations,
+
+		var labels map[string]string
+		if p.Labels != nil {
+			labels = make(map[string]string)
+			for k, v := range p.Labels {
+				labels[k] = v
+			}
+		}
+
+		if !previousFound || shouldTrigger || shouldTriggerFailedRun(j, p, now, logger, &labels) {
+			prowJob := pjutil.NewProwJob(pjutil.PeriodicSpec(p), labels, p.Annotations,
 				pjutil.RequireScheduling(cfg.Scheduler.Enabled))
 			prowJob.Namespace = cfg.ProwJobNamespace
 			logger.WithFields(logrus.Fields{
@@ -227,3 +240,54 @@ func sync(prowJobClient ctrlruntimeclient.Client, cfg *config.Config, cr cronCli
 	}
 	return nil
 }
+
+func shouldTriggerFailedRun(j v1.ProwJob, p config.Periodic, now time.Time, logger *logrus.Entry, labels *map[string]string) bool {
+	if p.Retry == nil {
+		return false
+	}
+	if !j.Complete() {
+		return false
+	}
+	runCount := 1
+
+	countLabel, exists := j.Labels[kube.ReRunLabel]
+	if exists {
+		count, err := strconv.Atoi(countLabel)
+		if err != nil {
+			logger.WithFields(logrus.Fields{}).WithFields(
+				pjutil.ProwJobFields(&j),
+			).Warn("Failed to convert label value.")
+			return false
+		}
+		runCount = count + 1
+	}
+
+	maxForJob := p.Retry.Attempts
+	if maxForJob > maxRetries {
+		maxForJob = maxRetries
+	}
+	if runCount > maxForJob {
+		return false
+	}
+
+	lastRunTime := j.Status.StartTime.Time
+	if now.Sub(lastRunTime) <= p.Retry.GetInterval() {
+		return false
+	}
+
+	if !p.Retry.RunAll && j.Status.State == v1.SuccessState {
+		return false
+	}
+
+	if *labels == nil {
+		*labels = make(map[string]string)
+	}
+	(*labels)[kube.ReRunLabel] = strconv.Itoa(runCount)
+
+	logger.WithFields(logrus.Fields{
+		"attempt": strconv.Itoa(runCount),
+		"run_all": p.Retry.RunAll,
+	}).Debug("Job marked to be retried")
+
+	return true
+}
diff --git a/pkg/config/config.go b/pkg/config/config.go
index e3d68c023..41750bd60 100644
--- a/pkg/config/config.go
+++ b/pkg/config/config.go
@@ -2435,6 +2435,14 @@ func (c Config) validatePeriodics(periodics []Periodic) error {
 			periodics[j].interval = d
 		}
 
+		if p.Retry != nil {
+			d, err := time.ParseDuration(periodics[j].Retry.Interval)
+			if err != nil {
+				errs = append(errs, fmt.Errorf("cannot parse Retry duration for %s: %w", periodics[j].Name, err))
+			}
+			periodics[j].interval = d
+		}
+
 		if p.MinimumInterval != "" {
 			d, err := time.ParseDuration(periodics[j].MinimumInterval)
 			if err != nil {
diff --git a/pkg/config/jobs.go b/pkg/config/jobs.go
index 2ad6c2d99..c29f41bfa 100644
--- a/pkg/config/jobs.go
+++ b/pkg/config/jobs.go
@@ -266,6 +266,19 @@ type Postsubmit struct {
 	JenkinsSpec *JenkinsSpec `json:"jenkins_spec,omitempty"`
 }
 
+// Retry defines the configuration for retrying failed prowjobs.
+type Retry struct {
+	// RunAll retries will not stop on first successful run
+	// (failed job will always cause Attempts retries to be executed)
+	RunAll bool `json:"run_all,omitempty"`
+	// Attempts specifies the maximum number of retry attempts allowed.
+	Attempts int `json:"attempts,omitempty"`
+	// Interval defines the wait duration between consecutive retry attempts.
+	Interval string `json:"interval,omitempty"`
+
+	interval time.Duration
+}
+
 // Periodic runs on a timer.
 type Periodic struct {
 	JobBase
@@ -282,6 +295,8 @@ type Periodic struct {
 	// Tags for config entries
 	Tags []string `json:"tags,omitempty"`
 
+	Retry *Retry `json:"retry,omitempty"`
+
 	interval         time.Duration
 	minimum_interval time.Duration
 }
@@ -293,6 +308,16 @@ type JenkinsSpec struct {
 	GitHubBranchSourceJob bool `json:"github_branch_source_job,omitempty"`
 }
 
+// SetInterval updates interval, the frequency duration it runs.
+func (r *Retry) SetInterval(d time.Duration) {
+	r.interval = d
+}
+
+// GetInterval returns interval, the frequency duration it runs.
+func (r *Retry) GetInterval() time.Duration {
+	return r.interval
+}
+
 // SetInterval updates interval, the frequency duration it runs.
 func (p *Periodic) SetInterval(d time.Duration) {
 	p.interval = d
diff --git a/pkg/kube/prowjob.go b/pkg/kube/prowjob.go
index 5e8c2fc5c..de4f90ca2 100644
--- a/pkg/kube/prowjob.go
+++ b/pkg/kube/prowjob.go
@@ -69,6 +69,9 @@ const (
 	// IsOptionalLabel is added in resources created by prow and
 	// carries the Optional from a Presubmit job.
 	IsOptionalLabel = "prow.k8s.io/is-optional"
+	// ReRunLabel is added in periodics that are configured to be
+	// re-runned several times, value starts from 0 and it's increased on re-run
+	ReRunLabel = "prow.k8s.io/re-run"
 
 	// Gerrit related labels that are used by Prow
 



Test patch:
diff --git a/cmd/horologium/main_test.go b/cmd/horologium/main_test.go
index 62916a852..468904a90 100644
--- a/cmd/horologium/main_test.go
+++ b/cmd/horologium/main_test.go
@@ -20,6 +20,7 @@ import (
 	"context"
 	"flag"
 	"reflect"
+	"strconv"
 	"testing"
 	"time"
 
@@ -30,9 +31,11 @@ import (
 	fakectrlruntimeclient "sigs.k8s.io/controller-runtime/pkg/client/fake"
 
 	prowapi "sigs.k8s.io/prow/pkg/apis/prowjobs/v1"
+	v1 "sigs.k8s.io/prow/pkg/apis/prowjobs/v1"
 	"sigs.k8s.io/prow/pkg/config"
 	"sigs.k8s.io/prow/pkg/flagutil"
 	configflagutil "sigs.k8s.io/prow/pkg/flagutil/config"
+	"sigs.k8s.io/prow/pkg/kube"
 )
 
 type fakeCron struct {
@@ -60,9 +63,14 @@ func TestSync(t *testing.T) {
 	testcases := []struct {
 		testName string
 
-		jobName         string
-		jobComplete     bool
-		jobStartTimeAgo time.Duration
+		jobName                  string
+		jobComplete              bool
+		jobStartTimeAgo          time.Duration
+		retry                    *config.Retry
+		state                    prowapi.ProwJobState
+		interval                 time.Duration
+		retryLabelNumber         int
+		expectedRetryLabelNumber int
 
 		shouldStart bool
 	}{
@@ -105,50 +113,171 @@ func TestSync(t *testing.T) {
 			jobStartTimeAgo: time.Second,
 			shouldStart:     false,
 		},
+		{
+			testName:        "old, complete job",
+			jobName:         "j",
+			jobComplete:     true,
+			jobStartTimeAgo: time.Hour,
+			shouldStart:     true,
+		},
+		{
+			testName:                 "complete job meant to be re-run",
+			jobName:                  "j",
+			jobComplete:              true,
+			jobStartTimeAgo:          time.Minute,
+			shouldStart:              true,
+			state:                    v1.FailureState,
+			retry:                    &config.Retry{Attempts: 3},
+			interval:                 time.Minute,
+			retryLabelNumber:         1,
+			expectedRetryLabelNumber: 2,
+		},
+		{
+			testName:         "failed job but horologium limit is reached",
+			jobName:          "j",
+			jobComplete:      true,
+			jobStartTimeAgo:  time.Minute,
+			shouldStart:      false,
+			state:            v1.FailureState,
+			retry:            &config.Retry{Attempts: 13},
+			interval:         time.Minute,
+			retryLabelNumber: 10,
+		},
+		{
+			testName:         "running job not meant to be re-run",
+			jobName:          "j",
+			jobComplete:      false,
+			jobStartTimeAgo:  time.Minute,
+			shouldStart:      false,
+			state:            v1.PendingState,
+			retry:            &config.Retry{RunAll: true, Attempts: 3},
+			interval:         time.Minute,
+			retryLabelNumber: 1,
+		},
+		{
+			testName:                 "complete job meant to be re-run even if success state",
+			jobName:                  "j",
+			jobComplete:              true,
+			jobStartTimeAgo:          time.Minute,
+			shouldStart:              true,
+			state:                    v1.SuccessState,
+			retry:                    &config.Retry{RunAll: true, Attempts: 3},
+			interval:                 time.Minute,
+			retryLabelNumber:         1,
+			expectedRetryLabelNumber: 2,
+		},
+		{
+			testName:                 "complete job meant to be re-run after 2 attempts",
+			jobName:                  "j",
+			jobComplete:              true,
+			jobStartTimeAgo:          time.Minute,
+			shouldStart:              true,
+			state:                    v1.SuccessState,
+			retry:                    &config.Retry{RunAll: true, Attempts: 3},
+			interval:                 time.Minute,
+			retryLabelNumber:         1,
+			expectedRetryLabelNumber: 2,
+		},
+		{
+			testName:         "complete job not meant to be re-run after 3 attempts",
+			jobName:          "j",
+			jobComplete:      true,
+			jobStartTimeAgo:  time.Minute,
+			shouldStart:      false,
+			state:            v1.SuccessState,
+			retry:            &config.Retry{RunAll: true, Attempts: 3},
+			interval:         time.Minute,
+			retryLabelNumber: 3,
+		},
+		{
+			testName:         "complete job not meant to be re-run with until_success after success state",
+			jobName:          "j",
+			jobComplete:      true,
+			jobStartTimeAgo:  time.Minute,
+			shouldStart:      false,
+			state:            v1.SuccessState,
+			retry:            &config.Retry{Attempts: 3},
+			interval:         time.Minute,
+			retryLabelNumber: 2,
+		},
 	}
 	for _, tc := range testcases {
-		cfg := config.Config{
-			ProwConfig: config.ProwConfig{
-				ProwJobNamespace: "prowjobs",
-			},
-			JobConfig: config.JobConfig{
-				Periodics: []config.Periodic{{JobBase: config.JobBase{Name: "j"}}},
-			},
-		}
-		cfg.Periodics[0].SetInterval(time.Minute)
-
-		var jobs []client.Object
-		now := time.Now()
-		if tc.jobName != "" {
-			job := &prowapi.ProwJob{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "with-interval",
-					Namespace: "prowjobs",
-				},
-				Spec: prowapi.ProwJobSpec{
-					Type: prowapi.PeriodicJob,
-					Job:  tc.jobName,
+		t.Run(tc.testName, func(t *testing.T) {
+			cfg := config.Config{
+				ProwConfig: config.ProwConfig{
+					ProwJobNamespace: "prowjobs",
 				},
-				Status: prowapi.ProwJobStatus{
-					StartTime: metav1.NewTime(now.Add(-tc.jobStartTimeAgo)),
+				JobConfig: config.JobConfig{
+					Periodics: []config.Periodic{{JobBase: config.JobBase{Name: "j"}, Retry: tc.retry}},
 				},
 			}
-			complete := metav1.NewTime(now.Add(-time.Millisecond))
-			if tc.jobComplete {
-				job.Status.CompletionTime = &complete
+			cfg.Periodics[0].SetInterval(time.Minute * 30)
+			if cfg.JobConfig.Periodics[0].Retry != nil {
+				tc.retry.SetInterval(tc.interval)
 			}
-			jobs = append(jobs, job)
-		}
-		fakeProwJobClient := newCreateTrackingClient(jobs)
-		fc := &fakeCron{}
-		if err := sync(fakeProwJobClient, &cfg, fc, now); err != nil {
-			t.Fatalf("For case %s, didn't expect error: %v", tc.testName, err)
-		}
 
-		sawCreation := fakeProwJobClient.sawCreate
-		if tc.shouldStart != sawCreation {
-			t.Errorf("For case %s, did the wrong thing.", tc.testName)
-		}
+			var jobs []client.Object
+			now := time.Now()
+			if tc.jobName != "" {
+				job := &prowapi.ProwJob{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "with-interval",
+						Namespace: "prowjobs",
+					},
+					Spec: prowapi.ProwJobSpec{
+						Type: prowapi.PeriodicJob,
+						Job:  tc.jobName,
+					},
+					Status: prowapi.ProwJobStatus{
+						StartTime: metav1.NewTime(now.Add(-tc.jobStartTimeAgo)),
+						State:     tc.state,
+					},
+				}
+				complete := metav1.NewTime(now.Add(-time.Millisecond))
+				if tc.jobComplete {
+					job.Status.CompletionTime = &complete
+				}
+				jobs = append(jobs, job)
+				if tc.retryLabelNumber != 0 {
+					if job.Labels == nil {
+						job.Labels = make(map[string]string)
+					}
+					job.Labels[kube.ReRunLabel] = strconv.Itoa(tc.retryLabelNumber)
+				}
+			}
+
+			fakeProwJobClient := newCreateTrackingClient(jobs)
+			fc := &fakeCron{}
+			if err := sync(fakeProwJobClient, &cfg, fc, now); err != nil {
+				t.Fatalf("Didn't expect error: %v", err)
+			}
+
+			sawCreation := fakeProwJobClient.sawCreate
+			if tc.shouldStart != sawCreation {
+				t.Errorf("Did the wrong thing. Expected sawCreation: %v, got: %v", tc.shouldStart, sawCreation)
+			}
+			if sawCreation {
+				pj, ok := fakeProwJobClient.created[0].(*v1.ProwJob)
+				if !ok {
+					t.Error("Failed to convert created object to *v1.ProwJob")
+					return
+				}
+
+				if tc.retryLabelNumber > 0 {
+					countLabel, exists := pj.Labels[kube.ReRunLabel]
+					if !exists {
+						t.Errorf("Expected label %s to exist", kube.ReRunLabel)
+					} else {
+						count, err := strconv.Atoi(countLabel)
+						if err != nil {
+							t.Errorf("Failed to convert label value: %v", err)
+						} else if count != tc.expectedRetryLabelNumber {
+							t.Errorf("Expected retry label value %d, but got %d", tc.expectedRetryLabelNumber, count)
+						}
+					}
+				}
+			}
+		})
 	}
 }
 
diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go
index aafa6e9fc..90006d577 100644
--- a/pkg/config/config_test.go
+++ b/pkg/config/config_test.go
@@ -8205,6 +8205,13 @@ func TestValidatePeriodics(t *testing.T) {
 			},
 			expectedError: "cannot parse duration for a: time: invalid duration \"hello\"",
 		},
+		{
+			name: "Invalid Retry interval",
+			periodics: []Periodic{
+				{JobBase: JobBase{Name: "a"}, Retry: &Retry{Interval: "hello"}, Interval: "6h"},
+			},
+			expectedError: "cannot parse Retry duration for a: time: invalid duration \"hello\"",
+		},
 		{
 			name: "Invalid minimum_interval",
 			periodics: []Periodic{
@@ -8221,6 +8228,15 @@ func TestValidatePeriodics(t *testing.T) {
 				{JobBase: JobBase{Name: "a"}, Interval: "10ns", interval: time.Duration(10)},
 			},
 		},
+		{
+			name: "Sets Retrigger Failed interval",
+			periodics: []Periodic{
+				{JobBase: JobBase{Name: "a"}, Retry: &Retry{Interval: "10ns"}, Interval: "6h"},
+			},
+			expected: []Periodic{
+				{JobBase: JobBase{Name: "a"}, Retry: &Retry{Interval: "10ns"}, interval: time.Duration(10), Interval: "6h"},
+			},
+		},
 		{
 			name: "Sets minimum_interval",
 			periodics: []Periodic{