TaskRuns may be orphaned by a PipelineRun

[afrittoli]

Expected Behavior

The PipelineRun controller should always be aware of TaskRuns owned by a PipelineRun even when the PipelineRun status is stale (because of API issues).
Before using the PipelineRun status as a source of truth, we should check it against the list ot TaskRuns owned by the PipelineRun.

Actual Behavior

When TaskRuns are created during the reconcile cycle of a PipelineRun, they are added to the PipelineRun status which is then sync'ed back to etcd. The sync may fail whatever error condition - if that happens, the TaskRuns are partially "orphaned": the PipelineRun will be queued for reconcile when they change and they will be deleted when the PipelineRun is deleted, however the PipelineRun controller will not be aware that they exist at all, and it will create new duplicate TaskRuns.

This happens because ResolvePipelineRun relies on the PipelineRun status alone, via the getTaskRunName function.

Steps to Reproduce the Problem

1. I would like to be able to reproduce this. It might be worth building a test case to reproduce this situation but it might be difficult. If we can disable specific API calls from functioning we could allow creating a taskrun while prevent updating a pipelinerun.

Additional Info

• Tekton Pipeline version:

v0.12.0

[afrittoli]

/kind bug

[n3wscott]

my advice would be to not use the trackers for this but use a informer for TaskRuns inside of PipelineRuns. We do this all over Knative, here is an example: https://github.com/knative/eventing/blob/master/pkg/reconciler/parallel/controller.go#L62-L65

The Parallel resource makes Subscriptions and manages their lifecycles.

The issue you are going to run into is that the generate name on the TaskRun is random.

I think you should setup the watch on TaskRuns and propagate the real name of the TaskRun as they are generated and use that list to be able to reconcile their existence. You might want to link this up some how with the generation of the Pipeline or PipelineRun

[afrittoli]

my advice would be to not use the trackers for this but use a informer for TaskRuns inside of PipelineRuns. We do this all over Knative, here is an example: https://github.com/knative/eventing/blob/master/pkg/reconciler/parallel/controller.go#L62-L65

Today we do both, not sure why:

• use the informer:

  pipeline/pkg/reconciler/pipelinerun/controller.go

  Lines 99 to 101 in fe5981e

  	taskRunInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
  	UpdateFunc: controller.PassNew(impl.EnqueueControllerOf),
  	})

• setup a tracker:

  pipeline/pkg/reconciler/pipelinerun/pipelinerun.go

  Lines 185 to 186 in d5a91fd

  	if err := c.tracker.Track(pr.GetTaskRunRef(), pr); err != nil {
  	c.Logger.Errorf("Failed to create tracker for TaskRuns for PipelineRun %s: %v", pr.Name, err)

So we do run the reconciler whenever a TaskRun is updated.

The Parallel resource makes Subscriptions and manages their lifecycles.

The issue you are going to run into is that the generate name on the TaskRun is random.

I think you should setup the watch on TaskRuns and propagate the real name of the TaskRun as they are generated and use that list to be able to reconcile their existence. You might want to link this up some how with the generation of the Pipeline or PipelineRun

When we generate the name for a TaskRun we store it in the PipelineRun status.
However if sync back to etcd fails during the reconcile cycle where the name was generated, we lose any track of that name. I've not seen happening in the wild tbh, but it seems to be a possibility - which is what this issue is about.