Skip to content

Commit

Permalink
Revert "Use a helper for setting the Succeeded condition on PipelineR…
Browse files Browse the repository at this point in the history 
…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.
  • Loading branch information
@afrittoli @tekton-robot
afrittoli authored and tekton-robot committed Jun 8, 2020
1 parent 3c3c5e2 commit 0de671e
Show file tree
Hide file tree
Showing 2 changed files with 147 additions and 86 deletions.
19 changes: 0 additions & 19 deletions pkg/apis/pipeline/v1beta1/pipelinerun_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -227,25 +227,6 @@ func (pr *PipelineRunStatus) SetCondition(newCond *apis.Condition) {
}
}

// 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...)
}

// MarkResourceNotConvertible adds a Warning-severity condition to the resource noting
// that it cannot be converted to a higher version.
func (pr *PipelineRunStatus) MarkResourceNotConvertible(err *CannotConvertError) {
Expand Down
214 changes: 147 additions & 67 deletions pkg/reconciler/pipelinerun/pipelinerun.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -279,19 +279,26 @@ func (c *Reconciler) updatePipelineResults(ctx context.Context, pr *v1beta1.Pipe
return
}
c.Logger.Errorf("Failed to determine Pipeline spec to use for pipelinerun %s: %v", pr.Name, err)
pr.Status.MarkFailed(ReasonCouldntGetPipeline,
"Error retrieving pipeline for pipelinerun %s/%s: %s",
pr.Namespace, pr.Name, err)
pr.Status.SetCondition(&apis.Condition{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionFalse,
Reason: ReasonCouldntGetPipeline,
Message: fmt.Sprintf("Error retrieving pipeline for pipelinerun %s: %s",
fmt.Sprintf("%s/%s", pr.Namespace, pr.Name), err),
})
return
}
if pipelineSpec.Results != nil && len(pipelineSpec.Results) != 0 {
providedResources, err := resources.GetResourcesFromBindings(
pr, c.resourceLister.PipelineResources(pr.Namespace).Get)
providedResources, err := resources.GetResourcesFromBindings(pr, c.resourceLister.PipelineResources(pr.Namespace).Get)
if err != nil {
// This Run has failed, so we need to mark it as failed and stop reconciling it
pr.Status.MarkFailed(ReasonCouldntGetResource,
"PipelineRun %s/%s can't be Run; it tries to bind Resources that don't exist: %s",
pipelineMeta.Namespace, pr.Name, err)
pr.Status.SetCondition(&apis.Condition{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionFalse,
Reason: ReasonCouldntGetResource,
Message: fmt.Sprintf("PipelineRun %s can't be Run; it tries to bind Resources that don't exist: %s",
fmt.Sprintf("%s/%s", pipelineMeta.Namespace, pr.Name), err),
})
return
}
pipelineState, err := resources.ResolvePipelineRun(ctx,
Expand All @@ -314,17 +321,29 @@ func (c *Reconciler) updatePipelineResults(ctx context.Context, pr *v1beta1.Pipe
// This Run has failed, so we need to mark it as failed and stop reconciling it
switch err := err.(type) {
case *resources.TaskNotFoundError:
pr.Status.MarkFailed(ReasonCouldntGetTask,
"Pipeline %s/%s can't be Run; it contains Tasks that don't exist: %s",
pipelineMeta.Namespace, pipelineMeta.Name, err)
pr.Status.SetCondition(&apis.Condition{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionFalse,
Reason: ReasonCouldntGetTask,
Message: fmt.Sprintf("Pipeline %s can't be Run; it contains Tasks that don't exist: %s",
fmt.Sprintf("%s/%s", pipelineMeta.Namespace, pipelineMeta.Name), err),
})
case *resources.ConditionNotFoundError:
pr.Status.MarkFailed(ReasonCouldntGetCondition,
"PipelineRun %s/%s can't be Run; it contains Conditions that don't exist: %s",
pipelineMeta.Namespace, pr.Name, err)
pr.Status.SetCondition(&apis.Condition{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionFalse,
Reason: ReasonCouldntGetCondition,
Message: fmt.Sprintf("PipelineRun %s can't be Run; it contains Conditions that don't exist: %s",
fmt.Sprintf("%s/%s", pipelineMeta.Namespace, pr.Name), err),
})
default:
pr.Status.MarkFailed(ReasonFailedValidation,
"PipelineRun %s/%s can't be Run; couldn't resolve all references: %s",
pipelineMeta.Namespace, pr.Name, err)
pr.Status.SetCondition(&apis.Condition{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionFalse,
Reason: ReasonFailedValidation,
Message: fmt.Sprintf("PipelineRun %s can't be Run; couldn't resolve all references: %s",
fmt.Sprintf("%s/%s", pipelineMeta.Namespace, pr.Name), err),
})
}
}
resolvedResultRefs := resources.ResolvePipelineResultRefs(pipelineState, pipelineSpec.Results)
Expand All @@ -348,9 +367,13 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun) err
return nil
}
c.Logger.Errorf("Failed to determine Pipeline spec to use for pipelinerun %s: %v", pr.Name, err)
pr.Status.MarkFailed(ReasonCouldntGetPipeline,
"Error retrieving pipeline for pipelinerun %s/%s: %s",
pr.Namespace, pr.Name, err)
pr.Status.SetCondition(&apis.Condition{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionFalse,
Reason: ReasonCouldntGetPipeline,
Message: fmt.Sprintf("Error retrieving pipeline for pipelinerun %s: %s",
fmt.Sprintf("%s/%s", pr.Namespace, pr.Name), err),
})
return nil
}

Expand Down Expand Up @@ -379,33 +402,49 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun) err
d, err := dag.Build(v1beta1.PipelineTaskList(pipelineSpec.Tasks))
if err != nil {
// This Run has failed, so we need to mark it as failed and stop reconciling it
pr.Status.MarkFailed(ReasonInvalidGraph,
"PipelineRun %s/%s's Pipeline DAG is invalid: %s",
pr.Namespace, pr.Name, err)
pr.Status.SetCondition(&apis.Condition{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionFalse,
Reason: ReasonInvalidGraph,
Message: fmt.Sprintf("PipelineRun %s's Pipeline DAG is invalid: %s",
fmt.Sprintf("%s/%s", pr.Namespace, pr.Name), err),
})
return nil
}

if err := pipelineSpec.Validate(ctx); err != nil {
// This Run has failed, so we need to mark it as failed and stop reconciling it
pr.Status.MarkFailed(ReasonFailedValidation,
"Pipeline %s/%s can't be Run; it has an invalid spec: %s",
pipelineMeta.Namespace, pipelineMeta.Name, err)
pr.Status.SetCondition(&apis.Condition{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionFalse,
Reason: ReasonFailedValidation,
Message: fmt.Sprintf("Pipeline %s can't be Run; it has an invalid spec: %s",
fmt.Sprintf("%s/%s", pipelineMeta.Namespace, pipelineMeta.Name), err),
})
return nil
}

if err := resources.ValidateResourceBindings(pipelineSpec, pr); err != nil {
// This Run has failed, so we need to mark it as failed and stop reconciling it
pr.Status.MarkFailed(ReasonInvalidBindings,
"PipelineRun %s/%s doesn't bind Pipeline %s/%s's PipelineResources correctly: %s",
pr.Namespace, pr.Name, pr.Namespace, pipelineMeta.Name, err)
pr.Status.SetCondition(&apis.Condition{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionFalse,
Reason: ReasonInvalidBindings,
Message: fmt.Sprintf("PipelineRun %s doesn't bind Pipeline %s's PipelineResources correctly: %s",
fmt.Sprintf("%s/%s", pr.Namespace, pr.Name), fmt.Sprintf("%s/%s", pr.Namespace, pipelineMeta.Name), err),
})
return nil
}
providedResources, err := resources.GetResourcesFromBindings(pr, c.resourceLister.PipelineResources(pr.Namespace).Get)
if err != nil {
// This Run has failed, so we need to mark it as failed and stop reconciling it
pr.Status.MarkFailed(ReasonCouldntGetResource,
"PipelineRun %s/%s can't be Run; it tries to bind Resources that don't exist: %s",
pipelineMeta.Namespace, pr.Name, err)
pr.Status.SetCondition(&apis.Condition{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionFalse,
Reason: ReasonCouldntGetResource,
Message: fmt.Sprintf("PipelineRun %s can't be Run; it tries to bind Resources that don't exist: %s",
fmt.Sprintf("%s/%s", pipelineMeta.Namespace, pr.Name), err),
})
return nil
}

Expand All @@ -414,25 +453,37 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun) err
err = resources.ValidateParamTypesMatching(pipelineSpec, pr)
if err != nil {
// This Run has failed, so we need to mark it as failed and stop reconciling it
pr.Status.MarkFailed(ReasonParameterTypeMismatch,
"PipelineRun %s/%s parameters have mismatching types with Pipeline %s/%s's parameters: %s",
pr.Namespace, pr.Name, pr.Namespace, pipelineMeta.Name, err)
pr.Status.SetCondition(&apis.Condition{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionFalse,
Reason: ReasonParameterTypeMismatch,
Message: fmt.Sprintf("PipelineRun %s parameters have mismatching types with Pipeline %s's parameters: %s",
fmt.Sprintf("%s/%s", pr.Namespace, pr.Name), fmt.Sprintf("%s/%s", pr.Namespace, pipelineMeta.Name), err),
})
return nil
}

// Ensure that the workspaces expected by the Pipeline are provided by the PipelineRun.
if err := resources.ValidateWorkspaceBindings(pipelineSpec, pr); err != nil {
pr.Status.MarkFailed(ReasonInvalidWorkspaceBinding,
"PipelineRun %s/%s doesn't bind Pipeline %s/%s's Workspaces correctly: %s",
pr.Namespace, pr.Name, pr.Namespace, pipelineMeta.Name, err)
pr.Status.SetCondition(&apis.Condition{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionFalse,
Reason: ReasonInvalidWorkspaceBinding,
Message: fmt.Sprintf("PipelineRun %s doesn't bind Pipeline %s's Workspaces correctly: %s",
fmt.Sprintf("%s/%s", pr.Namespace, pr.Name), fmt.Sprintf("%s/%s", pr.Namespace, pipelineMeta.Name), err),
})
return nil
}

// Ensure that the ServiceAccountNames defined correct.
if err := resources.ValidateServiceaccountMapping(pipelineSpec, pr); err != nil {
pr.Status.MarkFailed(ReasonInvalidServiceAccountMapping,
"PipelineRun %s/%s doesn't define ServiceAccountNames correctly: %s",
pr.Namespace, pr.Name, err)
pr.Status.SetCondition(&apis.Condition{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionFalse,
Reason: ReasonInvalidServiceAccountMapping,
Message: fmt.Sprintf("PipelineRun %s doesn't define ServiceAccountNames correctly: %s",
fmt.Sprintf("%s/%s", pr.Namespace, pr.Name), err),
})
return nil
}

Expand Down Expand Up @@ -460,17 +511,29 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun) err
// This Run has failed, so we need to mark it as failed and stop reconciling it
switch err := err.(type) {
case *resources.TaskNotFoundError:
pr.Status.MarkFailed(ReasonCouldntGetTask,
"Pipeline %s/%s can't be Run; it contains Tasks that don't exist: %s",
pipelineMeta.Namespace, pipelineMeta.Name, err)
pr.Status.SetCondition(&apis.Condition{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionFalse,
Reason: ReasonCouldntGetTask,
Message: fmt.Sprintf("Pipeline %s can't be Run; it contains Tasks that don't exist: %s",
fmt.Sprintf("%s/%s", pipelineMeta.Namespace, pipelineMeta.Name), err),
})
case *resources.ConditionNotFoundError:
pr.Status.MarkFailed(ReasonCouldntGetCondition,
"PipelineRun %s/%s can't be Run; it contains Conditions that don't exist: %s",
pipelineMeta.Namespace, pr.Name, err)
pr.Status.SetCondition(&apis.Condition{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionFalse,
Reason: ReasonCouldntGetCondition,
Message: fmt.Sprintf("PipelineRun %s can't be Run; it contains Conditions that don't exist: %s",
fmt.Sprintf("%s/%s", pipelineMeta.Namespace, pr.Name), err),
})
default:
pr.Status.MarkFailed(ReasonFailedValidation,
"PipelineRun %s/%s can't be Run; couldn't resolve all references: %s",
pipelineMeta.Namespace, pr.Name, err)
pr.Status.SetCondition(&apis.Condition{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionFalse,
Reason: ReasonFailedValidation,
Message: fmt.Sprintf("PipelineRun %s can't be Run; couldn't resolve all references: %s",
fmt.Sprintf("%s/%s", pipelineMeta.Namespace, pr.Name), err),
})
}
return nil
}
Expand All @@ -485,7 +548,13 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun) err
err := taskrun.ValidateResolvedTaskResources(rprt.PipelineTask.Params, rprt.ResolvedTaskResources)
if err != nil {
c.Logger.Errorf("Failed to validate pipelinerun %q with error %v", pr.Name, err)
pr.Status.MarkFailed(ReasonFailedValidation, err.Error())
pr.Status.SetCondition(&apis.Condition{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionFalse,
Reason: ReasonFailedValidation,
Message: err.Error(),
})
// update pr completed time
return nil
}
}
Expand All @@ -495,9 +564,13 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun) err
// create workspace PVC from template
if err = c.pvcHandler.CreatePersistentVolumeClaimsForWorkspaces(pr.Spec.Workspaces, pr.GetOwnerReference(), pr.Namespace); err != nil {
c.Logger.Errorf("Failed to create PVC for PipelineRun %s: %v", pr.Name, err)
pr.Status.MarkFailed(volumeclaim.ReasonCouldntCreateWorkspacePVC,
"Failed to create PVC for PipelineRun %s/%s Workspaces correctly: %s",
pr.Namespace, pr.Name, err)
pr.Status.SetCondition(&apis.Condition{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionFalse,
Reason: volumeclaim.ReasonCouldntCreateWorkspacePVC,
Message: fmt.Sprintf("Failed to create PVC for PipelineRun %s Workspaces correctly: %s",
fmt.Sprintf("%s/%s", pr.Namespace, pr.Name), err),
})
return nil
}
}
Expand All @@ -506,9 +579,13 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun) err
// create Affinity Assistant (StatefulSet) so that taskRun pods that share workspace PVC achieve Node Affinity
if err = c.createAffinityAssistants(pr.Spec.Workspaces, pr, pr.Namespace); err != nil {
c.Logger.Errorf("Failed to create affinity assistant StatefulSet for PipelineRun %s: %v", pr.Name, err)
pr.Status.MarkFailed(ReasonCouldntCreateAffinityAssistantStatefulSet,
"Failed to create StatefulSet for PipelineRun %s/%s correctly: %s",
pr.Namespace, pr.Name, err)
pr.Status.SetCondition(&apis.Condition{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionFalse,
Reason: ReasonCouldntCreateAffinityAssistantStatefulSet,
Message: fmt.Sprintf("Failed to create StatefulSet for PipelineRun %s correctly: %s",
fmt.Sprintf("%s/%s", pr.Namespace, pr.Name), err),
})
return nil
}
}
Expand All @@ -523,7 +600,12 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun) err
resolvedResultRefs, err := resources.ResolveResultRefs(pipelineState, nextRprts)
if err != nil {
c.Logger.Infof("Failed to resolve all task params for %q with error %v", pr.Name, err)
pr.Status.MarkFailed(ReasonFailedValidation, err.Error())
pr.Status.SetCondition(&apis.Condition{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionFalse,
Reason: ReasonFailedValidation,
Message: err.Error(),
})
return nil
}
resources.ApplyTaskResults(nextRprts, resolvedResultRefs)
Expand Down Expand Up @@ -557,18 +639,11 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun) err
}
before := pr.Status.GetCondition(apis.ConditionSucceeded)
after := resources.GetPipelineConditionStatus(pr, pipelineState, c.Logger, d)
switch after.Status {
case corev1.ConditionTrue:
pr.Status.MarkSucceeded(after.Reason, after.Message)
case corev1.ConditionFalse:
pr.Status.MarkFailed(after.Reason, after.Message)
case corev1.ConditionUnknown:
pr.Status.MarkRunning(after.Reason, after.Message)
}
pr.Status.SetCondition(after)
events.Emit(c.Recorder, before, after, pr)

pr.Status.TaskRuns = getTaskRunsStatus(pr, pipelineState)
c.Logger.Infof("PipelineRun %s status is being set to %s", pr.Name, after)
c.Logger.Infof("PipelineRun %s status is being set to %s", pr.Name, pr.Status.GetCondition(apis.ConditionSucceeded))
return nil
}

Expand Down Expand Up @@ -833,6 +908,11 @@ func (c *Reconciler) updateStatus(pr *v1beta1.PipelineRun) (*v1beta1.PipelineRun
if err != nil {
return nil, fmt.Errorf("error getting PipelineRun %s when updating status: %w", pr.Name, err)
}
succeeded := pr.Status.GetCondition(apis.ConditionSucceeded)
if succeeded.Status == corev1.ConditionFalse || succeeded.Status == corev1.ConditionTrue {
// update pr completed time
pr.Status.CompletionTime = &metav1.Time{Time: time.Now()}
}
if !reflect.DeepEqual(pr.Status, newPr.Status) {
newPr = newPr.DeepCopy() // Don't modify the informer's copy
newPr.Status = pr.Status
Expand Down

0 comments on commit 0de671e

Please sign in to comment.