Skip to content

Commit c99df33

Browse files
vdemeesterclaude
andcommitted
fix: Prevent excessive reconciliation when timeout disabled
- Stop immediate requeue loops when default-timeout-minutes is "0" - Remove redundant hasCondition checks (Knative already deduplicates) This adds an e2e tests that looks at pipeline logs to see how much reconciler loop there is. If you run it before the fix, it would count more than 1500 reconciler loop, whereas with the fix, only about 10. Signed-off-by: Vincent Demeester <vdemeest@redhat.com> Co-Authored-By: Claude <noreply@anthropic.com>
1 parent ad5317e commit c99df33

File tree

3 files changed

+377
-11
lines changed

3 files changed

+377
-11
lines changed

pkg/reconciler/pipelinerun/pipelinerun.go

Lines changed: 25 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@ import (
2525
"reflect"
2626
"regexp"
2727
"strings"
28-
"time"
2928

3029
"k8s.io/apimachinery/pkg/util/wait"
3130

@@ -275,22 +274,39 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, pr *v1.PipelineRun) pkgr
275274
}
276275

277276
if pr.Status.StartTime != nil {
278-
// Compute the time since the task started.
277+
// Compute the time since the pipeline started.
279278
elapsed := c.Clock.Since(pr.Status.StartTime.Time)
280279
// Snooze this resource until the appropriate timeout has elapsed.
281-
// but if the timeout has been disabled by setting timeout to 0, we
282-
// do not want to subtract from 0, because a negative wait time will
283-
// result in the requeue happening essentially immediately
284280
timeout := pr.PipelineTimeout(ctx)
285281
taskTimeout := pr.TasksTimeout()
286-
waitTime := timeout - elapsed
282+
283+
// If the main pipeline timeout is NoTimeoutDuration (0), it means no timeout is configured.
284+
// This can happen in two ways:
285+
// 1. User explicitly set pr.Spec.Timeouts.Pipeline to 0 (wants no timeout)
286+
// 2. User didn't set pr.Spec.Timeouts.Pipeline (nil) AND default-timeout-minutes config is "0"
287+
// In these cases, check if there are specific task or finally timeouts to enforce.
288+
// If not, don't requeue - the reconciler will be triggered by watch events.
287289
if timeout == config.NoTimeoutDuration {
288-
waitTime = time.Duration(config.FromContextOrDefaults(ctx).Defaults.DefaultTimeoutMinutes) * time.Minute
290+
// Check if we have a specific task timeout to wait for
291+
if pr.Status.FinallyStartTime == nil && taskTimeout != nil && taskTimeout.Duration != config.NoTimeoutDuration {
292+
waitTime := taskTimeout.Duration - elapsed
293+
return controller.NewRequeueAfter(waitTime)
294+
}
295+
// Check if we have a finally timeout to wait for
296+
if pr.Status.FinallyStartTime != nil && pr.FinallyTimeout() != nil && pr.FinallyTimeout().Duration != config.NoTimeoutDuration {
297+
waitTime := pr.FinallyTimeout().Duration - c.Clock.Since(pr.Status.FinallyStartTime.Time)
298+
return controller.NewRequeueAfter(waitTime)
299+
}
300+
// No timeout configured anywhere, don't requeue
301+
return nil
289302
}
303+
304+
// Pipeline timeout is set, calculate wait time based on the most restrictive timeout
305+
waitTime := timeout - elapsed
290306
if pr.Status.FinallyStartTime == nil && taskTimeout != nil {
291-
waitTime = pr.TasksTimeout().Duration - elapsed
307+
waitTime = taskTimeout.Duration - elapsed
292308
if taskTimeout.Duration == config.NoTimeoutDuration {
293-
waitTime = time.Duration(config.FromContextOrDefaults(ctx).Defaults.DefaultTimeoutMinutes) * time.Minute
309+
waitTime = timeout - elapsed
294310
}
295311
} else if pr.Status.FinallyStartTime != nil && pr.FinallyTimeout() != nil &&
296312
pr.FinallyTimeout().Duration != config.NoTimeoutDuration {

pkg/reconciler/taskrun/taskrun.go

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -237,10 +237,16 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, tr *v1.TaskRun) pkgrecon
237237
elapsed := c.Clock.Since(tr.Status.StartTime.Time)
238238
// Snooze this resource until the timeout has elapsed.
239239
timeout := tr.GetTimeout(ctx)
240-
waitTime := timeout - elapsed
240+
// If timeout is NoTimeoutDuration (0), it means no timeout is configured.
241+
// This can happen in two ways:
242+
// 1. User explicitly set tr.Spec.Timeout.Duration to 0 (wants no timeout)
243+
// 2. User didn't set tr.Spec.Timeout (nil) AND default-timeout-minutes config is "0"
244+
// In both cases, we should not requeue based on timeout. The reconciler will
245+
// still be triggered appropriately by pod watch events when the TaskRun changes.
241246
if timeout == config.NoTimeoutDuration {
242-
waitTime = time.Duration(config.FromContextOrDefaults(ctx).Defaults.DefaultTimeoutMinutes) * time.Minute
247+
return nil
243248
}
249+
waitTime := timeout - elapsed
244250
return controller.NewRequeueAfter(waitTime)
245251
}
246252
return nil

0 commit comments

Comments
 (0)