Adjust PipelineRun's StartTime based on TaskRun state.

[mattmoor]

Changes

Occasionally, it is possible for us to be reconciling a PipelineRun and have the
status we intend to report reflect an inaccurate StartTime (see issue for
details). This corrects for those circumstances by ensuring that the StartTime
we report for a PipelineRun is never later than the smallest CreationTimestamp
of a child TaskRun.

Fixes: #3460

/kind bug

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

• Includes tests (if functionality changed/added)
• Includes docs (if user facing)
• Commit messages follow commit message best practices
• Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

• If you are adding a new binary/image to the cmd dir, please update
  the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

Fixes a bug where PipelineRun may report as Failed when it really timed out.

[imjasonh]

Does this need a test?

[imjasonh]

In addition to stating why we do this, can you add a sentence describing how we adjust, to make this easier to read?

[mattmoor]

tweaked wording, PTAL 🙏

[pritidesai]

Does this need a test?

yes please, a test would be great. Does the same bug apply to taskRun start time?

[mattmoor]

I pushed unit tests for AdjustStartTime.

Does the same bug apply to taskRun start time

@pritidesai I haven't looked at the TaskRun StartTime logic. In principle it could similarly benefit from rationalizing its StartTime with the metadata.creationTimestamp of its child resources (e.g. Pods), but AFAIK it shouldn't manifest in failures the way it does in the linked bug for PipelineRun -> TaskRun because it is a "leaf" of the timeout evaluation chain. So it's at least more tolerant of minor drift.

[pritidesai]

wow that was quick @mattmoor 🙏

So taskRun has initializeConditions() if it hasn't started and updating startTime in case of any such gap:

pipeline/pkg/apis/pipeline/v1beta1/taskrun_types.go

Lines 245 to 259 in 51a02c8

	func (trs *TaskRunStatus) InitializeConditions() {
	started := false
	if trs.StartTime.IsZero() {
	trs.StartTime = &metav1.Time{Time: time.Now()}
	started = true
	}
	conditionManager := taskRunCondSet.Manage(trs)
	conditionManager.InitializeConditions()
	// Ensure the started reason is set for the "Succeeded" condition
	if started {
	initialCondition := conditionManager.GetCondition(apis.ConditionSucceeded)
	initialCondition.Reason = TaskRunReasonStarted.String()
	conditionManager.SetCondition(*initialCondition)
	}
	}

pipeline/pkg/reconciler/taskrun/taskrun.go

Lines 92 to 99 in adc4cfa

	if !tr.HasStarted() {
	tr.Status.InitializeConditions()
	// In case node time was not synchronized, when controller has been scheduled to other nodes.
	if tr.Status.StartTime.Sub(tr.CreationTimestamp.Time) < 0 {
	logger.Warnf("TaskRun %s createTimestamp %s is after the taskRun started %s", tr.GetNamespacedName().String(), tr.CreationTimestamp, tr.Status.StartTime)
	tr.Status.StartTime = &tr.CreationTimestamp
	}
	// Emit events. During the first reconcile the status of the TaskRun may change twice

which means, we might not see this bug in taskRun 🤔

[mattmoor]

@pritidesai that same logic is in PipelineRun (the initialization and clamping to its own creation timestamp), but the question is what outside resources we'd want to clamp the StartTime on. Maybe the Pod?

[pritidesai]

@pritidesai that same logic is in PipelineRun (the initialization and clamping to its own creation timestamp), but the question is what outside resources we'd want to clamp the StartTime on. Maybe the Pod?

yup I noticed pipelineRun having the same logic 😜 yup podCreation time sounds right.

[pritidesai]

thanks @mattmoor

I was thinking of moving updating pr.Status to a separate function for readability, we have many different status fields being updated here, will create a refactoring PR.

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pritidesai

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [pritidesai]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[vdemeester]

/lgtm

[pritidesai]

@pritidesai that same logic is in PipelineRun (the initialization and clamping to its own creation timestamp), but the question is what outside resources we'd want to clamp the StartTime on. Maybe the Pod?

yup I noticed pipelineRun having the same logic 😜 yup podCreation time sounds right.

Rather, step start time 🤔