From 8efbdda868e9e6514bb39809d178d7015aee44b4 Mon Sep 17 00:00:00 2001 From: gabemontero Date: Wed, 24 Jan 2024 10:45:43 -0500 Subject: [PATCH] temp commit: alternative that uses global timeout config as requeue wait time when timeout disabled --- pkg/reconciler/pipelinerun/pipelinerun.go | 12 ++++-- pkg/reconciler/taskrun/taskrun.go | 49 ++++++++++++----------- 2 files changed, 34 insertions(+), 27 deletions(-) diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index 76fc46d240e..8aab3ccfdd0 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -25,6 +25,7 @@ import ( "reflect" "regexp" "strings" + "time" "github.com/hashicorp/go-multierror" "github.com/tektoncd/pipeline/pkg/apis/config" @@ -277,12 +278,17 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, pr *v1.PipelineRun) pkgr // do not want to subtract from 0, because a negative wait time will // result in the requeue happening essentially immediately timeout := pr.PipelineTimeout(ctx) + taskTimeout := pr.TasksTimeout() + waitTime := timeout - elapsed if timeout == config.NoTimeoutDuration { - timeout = taskrun.DeriveTimeoutForRequeueWaitTime(elapsed) + // timeout = taskrun.DeriveTimeoutForRequeueWaitTime(elapsed) + waitTime = time.Duration(config.FromContextOrDefaults(ctx).Defaults.DefaultTimeoutMinutes) * time.Minute } - waitTime := timeout - elapsed - if pr.Status.FinallyStartTime == nil && pr.TasksTimeout() != nil { + if pr.Status.FinallyStartTime == nil && taskTimeout != nil { waitTime = pr.TasksTimeout().Duration - elapsed + if taskTimeout.Duration == config.NoTimeoutDuration { + waitTime = time.Duration(config.FromContextOrDefaults(ctx).Defaults.DefaultTimeoutMinutes) * time.Minute + } } else if pr.Status.FinallyStartTime != nil && pr.FinallyTimeout() != nil { finallyWaitTime := pr.FinallyTimeout().Duration - c.Clock.Since(pr.Status.FinallyStartTime.Time) if finallyWaitTime < waitTime { diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go index 4ded7bde48d..2a3c70e21d8 100644 --- a/pkg/reconciler/taskrun/taskrun.go +++ b/pkg/reconciler/taskrun/taskrun.go @@ -20,7 +20,6 @@ import ( "context" "errors" "fmt" - "math" "reflect" "strings" "time" @@ -213,10 +212,12 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, tr *v1.TaskRun) pkgrecon elapsed := c.Clock.Since(tr.Status.StartTime.Time) // Snooze this resource until the timeout has elapsed. timeout := tr.GetTimeout(ctx) + waitTime := timeout - elapsed if timeout == config.NoTimeoutDuration { - timeout = DeriveTimeoutForRequeueWaitTime(elapsed) + // timeout = DeriveTimeoutForRequeueWaitTime(elapsed) + waitTime = time.Duration(config.FromContextOrDefaults(ctx).Defaults.DefaultTimeoutMinutes) * time.Minute } - return controller.NewRequeueAfter(timeout - elapsed) + return controller.NewRequeueAfter(waitTime) } return nil } @@ -1062,24 +1063,24 @@ func retryTaskRun(tr *v1.TaskRun, message string) { taskRunCondSet.Manage(&tr.Status).MarkUnknown(apis.ConditionSucceeded, v1.TaskRunReasonToBeRetried.String(), message) } -func DeriveTimeoutForRequeueWaitTime(elapsed time.Duration) time.Duration { - timeout := time.Duration(math.MaxInt64) - // an explicit choice has been made to not make these demarcation times configurable - // wrt wait times when timeout has been disabled; rationale: allowing - // disablement of timeout is problematic, but the feature has already been - // exposed. We'll get it to behave (i.e. no negative wait times) but we - // will not add configuration to encourage use of the feature. - switch { - case elapsed < 30*time.Minute: - timeout = 30 * time.Minute - case elapsed < time.Hour: - timeout = time.Hour - case elapsed < 6*time.Hour: - timeout = 6 * time.Hour - case elapsed < 24*time.Hour: - timeout = 24 * time.Hour - case elapsed < 48*time.Hour: - timeout = 48 * time.Hour - } - return timeout -} +// func DeriveTimeoutForRequeueWaitTime(elapsed time.Duration) time.Duration { +// timeout := time.Duration(math.MaxInt64) +// // an explicit choice has been made to not make these demarcation times configurable +// // wrt wait times when timeout has been disabled; rationale: allowing +// // disablement of timeout is problematic, but the feature has already been +// // exposed. We'll get it to behave (i.e. no negative wait times) but we +// // will not add configuration to encourage use of the feature. +// switch { +// case elapsed < 30*time.Minute: +// timeout = 30 * time.Minute +// case elapsed < time.Hour: +// timeout = time.Hour +// case elapsed < 6*time.Hour: +// timeout = 6 * time.Hour +// case elapsed < 24*time.Hour: +// timeout = 24 * time.Hour +// case elapsed < 48*time.Hour: +// timeout = 48 * time.Hour +// } +// return timeout +//}