Use a helper for setting the Succeeded condition on PipelineRun.

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
afrittoli · Jun 4, 2020 · 8ff3169 · 8ff3169
1 parent ae69af2
commit 8ff3169
Show file tree

Hide file tree

Showing 2 changed files with 86 additions and 147 deletions.
diff --git a/pkg/apis/pipeline/v1beta1/pipelinerun_types.go b/pkg/apis/pipeline/v1beta1/pipelinerun_types.go
@@ -227,6 +227,25 @@ 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) {

diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go
@@ -279,26 +279,19 @@ 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.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),
- })
+ pr.Status.MarkFailed(ReasonCouldntGetPipeline,
+ "Error retrieving pipeline for pipelinerun %s/%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.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),
- })
+ 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)
  return
  }
  pipelineState, err := resources.ResolvePipelineRun(ctx,
@@ -321,29 +314,17 @@ 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.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),
- })
+ pr.Status.MarkFailed(ReasonCouldntGetTask,
+ "Pipeline %s/%s can't be Run; it contains Tasks that don't exist: %s",
+ pipelineMeta.Namespace, pipelineMeta.Name, err)
  case *resources.ConditionNotFoundError:
- 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),
- })
+ pr.Status.MarkFailed(ReasonCouldntGetCondition,
+ "PipelineRun %s/%s can't be Run; it contains Conditions that don't exist: %s",
+ pipelineMeta.Namespace, pr.Name, err)
  default:
- 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),
- })
+ pr.Status.MarkFailed(ReasonFailedValidation,
+ "PipelineRun %s/%s can't be Run; couldn't resolve all references: %s",
+ pipelineMeta.Namespace, pr.Name, err)
  }
  }
  resolvedResultRefs := resources.ResolvePipelineResultRefs(pipelineState, pipelineSpec.Results)
@@ -367,13 +348,9 @@ 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.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),
- })
+ pr.Status.MarkFailed(ReasonCouldntGetPipeline,
+ "Error retrieving pipeline for pipelinerun %s/%s: %s",
+ pr.Namespace, pr.Name, err)
  return nil
  }
 
@@ -402,49 +379,33 @@ 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.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),
- })
+ pr.Status.MarkFailed(ReasonInvalidGraph,
+ "PipelineRun %s/%s's Pipeline DAG is invalid: %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.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),
- })
+ pr.Status.MarkFailed(ReasonFailedValidation,
+ "Pipeline %s/%s can't be Run; it has an invalid spec: %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.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),
- })
+ 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)
  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.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),
- })
+ 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)
  return nil
  }
 
@@ -453,37 +414,25 @@ 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.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),
- })
+ 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)
  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.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),
- })
+ 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)
  return nil
  }
 
  // Ensure that the ServiceAccountNames defined correct.
  if err := resources.ValidateServiceaccountMapping(pipelineSpec, pr); err != nil {
- 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),
- })
+ pr.Status.MarkFailed(ReasonInvalidServiceAccountMapping,
+ "PipelineRun %s/%s doesn't define ServiceAccountNames correctly: %s",
+ pr.Namespace, pr.Name, err)
  return nil
  }
 
@@ -511,29 +460,17 @@ 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.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),
- })
+ pr.Status.MarkFailed(ReasonCouldntGetTask,
+ "Pipeline %s/%s can't be Run; it contains Tasks that don't exist: %s",
+ pipelineMeta.Namespace, pipelineMeta.Name, err)
  case *resources.ConditionNotFoundError:
- 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),
- })
+ pr.Status.MarkFailed(ReasonCouldntGetCondition,
+ "PipelineRun %s/%s can't be Run; it contains Conditions that don't exist: %s",
+ pipelineMeta.Namespace, pr.Name, err)
  default:
- 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),
- })
+ pr.Status.MarkFailed(ReasonFailedValidation,
+ "PipelineRun %s/%s can't be Run; couldn't resolve all references: %s",
+ pipelineMeta.Namespace, pr.Name, err)
  }
  return nil
  }
@@ -548,13 +485,7 @@ 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.SetCondition(&apis.Condition{
- Type: apis.ConditionSucceeded,
- Status: corev1.ConditionFalse,
- Reason: ReasonFailedValidation,
- Message: err.Error(),
- })
- // update pr completed time
+ pr.Status.MarkFailed(ReasonFailedValidation, err.Error())
  return nil
  }
  }
@@ -564,13 +495,9 @@ 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.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),
- })
+ pr.Status.MarkFailed(volumeclaim.ReasonCouldntCreateWorkspacePVC,
+ "Failed to create PVC for PipelineRun %s/%s Workspaces correctly: %s",
+ pr.Namespace, pr.Name, err)
  return nil
  }
  }
@@ -579,13 +506,9 @@ 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.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),
- })
+ pr.Status.MarkFailed(ReasonCouldntCreateAffinityAssistantStatefulSet,
+ "Failed to create StatefulSet for PipelineRun %s/%s correctly: %s",
+ pr.Namespace, pr.Name, err)
  return nil
  }
  }
@@ -600,12 +523,7 @@ 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.SetCondition(&apis.Condition{
- Type: apis.ConditionSucceeded,
- Status: corev1.ConditionFalse,
- Reason: ReasonFailedValidation,
- Message: err.Error(),
- })
+ pr.Status.MarkFailed(ReasonFailedValidation, err.Error())
  return nil
  }
  resources.ApplyTaskResults(nextRprts, resolvedResultRefs)
@@ -639,11 +557,18 @@ 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)
- pr.Status.SetCondition(after)
+ 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)
+ }
  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, pr.Status.GetCondition(apis.ConditionSucceeded))
+ c.Logger.Infof("PipelineRun %s status is being set to %s", pr.Name, after)
  return nil
 }
 
@@ -908,11 +833,6 @@ 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