Use a helper for setting the Succeeded condition on PipelineRun. #2749
Conversation
These helpers reduce a lot of the boilerplate and give us hooks where we can eagerly set the CompletionTime field rather than waiting for `updateStatus`. Fixes: tektoncd#2741
|
The following is the coverage report on the affected files.
|
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
1 similar comment
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
|
/kind bug |
| // MarkSucceeded changes the Succeeded condition to True with the provided reason and message. | ||
| func (pr *PipelineRunStatus) MarkSucceeded(reason, messageFormat string, messageA ...interface{}) { | ||
| pipelineRunCondSet.Manage(pr).MarkTrueWithReason(apis.ConditionSucceeded, reason, messageFormat, messageA...) | ||
| succeeded := pr.GetCondition(apis.ConditionSucceeded) | ||
| pr.CompletionTime = &succeeded.LastTransitionTime.Inner | ||
| } | ||
|
|
||
| // MarkFailed changes the Succeeded condition to False with the provided reason and message. | ||
| func (pr *PipelineRunStatus) MarkFailed(reason, messageFormat string, messageA ...interface{}) { | ||
| pipelineRunCondSet.Manage(pr).MarkFalse(apis.ConditionSucceeded, reason, messageFormat, messageA...) | ||
| succeeded := pr.GetCondition(apis.ConditionSucceeded) | ||
| pr.CompletionTime = &succeeded.LastTransitionTime.Inner | ||
| } | ||
|
|
||
| // MarkRunning changes the Succeeded condition to Unknown with the provided reason and message. | ||
| func (pr *PipelineRunStatus) MarkRunning(reason, messageFormat string, messageA ...interface{}) { | ||
| pipelineRunCondSet.Manage(pr).MarkUnknown(apis.ConditionSucceeded, reason, messageFormat, messageA...) | ||
| } | ||
|
|
There was a problem hiding this comment.
Thanks for this!
I was under the impression that in some cases we set the status to "ConditionFalse" even in case of transient errors, but I cannot find an example anymore - perhaps it was in the taskrun controller - so I think this is good!
If we find a case of transient error in the future we'll need to make sure we don't use the MarkFailed helper because we would not want to set the completion time in that case.
The only concern left that I have is that we have an issue with setting the completion time today. We want to mark the pipeline failed as early as possible, however taskruns may still be running when that happens, and we need to keep them running until they finish, which means that the completion time does not really reflect the time when the last sub-resource completed.
To fix that issue with this change in place, it means we won't be able to set the pipeline as failed until all TaskRuns completed. Is this an acceptable approach?
@vdemeester @bobcatfish @pritidesai
/approve
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: afrittoli 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:
Approvers can indicate their approval by writing |
yes its a valid concern. I havent looked into this yet (will look into it ASAP) but are we now setting pipeline competition time when a first taskRun failure is noted 🤔 In that case, we need a separate helper to not set the competition time on failure. The issue is Let's say we have three tasks executing in parallel, A, B, and C. Task A fails but Task B and C are still running. Now, Task B finishes execution successfully but C is still running. While we are waiting for Task C to finish, Reconciler should not schedule (Task Y) subsequent node of Task B for execution otherwise we might break backward compatibility of failing pipeline on first task failure. Trying to address this particular scenario in issue #1680 |
|
TBH I'm more inclined now towards changing the logic to setting both status and completion time of the |
|
I tested master with a pipeline having three tasks: PipelineRun completion time is set to the task A competition time while task B and task C are still running 😢 :
|
|
Yeah, the completion time is not set along with setting success or failure, which is a change in behaviour. I think the next step on this should be to do so only once all resources spawned by the pipelinerun have completed their work. @bobcatfish @vdemeester @pritidesai @mattmoor @imjasonh thoughts? |
|
I'm a bit puzzled by why my change affected this unless the |
|
e.g. are we expecting this line to undo a |
|
TBH I would prefer having stop setting the status to failed with no completion time, wait until the DAG is completed and then report how many failed / passed / skipped / cancelled. |
…un." PR tektoncd#2749 introduces helpers to set the completion time along with setting the Succeeded condition to Unknown, True or False. This is fine, however in combination with a previous issue, whereby we update the Succeeded condition to False in case of failure as soon as the first failure is encountered, this results in having the completion time set as soon as the first failure is encountered, which may not match the actual completion time of the pipeline run, in case other tasks were already running when the initial failure occurred. For v0.13.x we shall keep completion time and condition update separated. Next release will include this plus a fix to the original issue. This reverts commit 8ff3169.
…un." PR #2749 introduces helpers to set the completion time along with setting the Succeeded condition to Unknown, True or False. This is fine, however in combination with a previous issue, whereby we update the Succeeded condition to False in case of failure as soon as the first failure is encountered, this results in having the completion time set as soon as the first failure is encountered, which may not match the actual completion time of the pipeline run, in case other tasks were already running when the initial failure occurred. For v0.13.x we shall keep completion time and condition update separated. Next release will include this plus a fix to the original issue. This reverts commit 8ff3169.
These helpers reduce a lot of the boilerplate and give us hooks
where we can eagerly set the CompletionTime field rather than waiting
for
updateStatus.Fixes: #2741