Pipelinerun hangs with condition validation issue

[afrittoli]

Expected Behavior

If the condition validation fails, the error should be reported back

Actual Behavior

The pipelinerun remains in status "running", no error is reported, only the following error could be found in the controller logs:

{"level":"info","logger":"controller.pipeline-controller.event-broadcaster","caller":"record/event.go:221","msg":"Event(v1.ObjectReference{Kind:\"PipelineRun\", Namespace:\"default\", Name:\"flip-coin-condition-demo-run-8hzc6\", UID:\"5076f9f6-ef5a-11e9-9898-e67dbd3091a9\", APIVersion:\"tekton.dev/v1alpha1\", ResourceVersion:\"40144313\", FieldPath:\"\"}): type: 'Warning' reason: 'ConditionCheckCreationFailed' Failed to create TaskRun \"flip-coin-condition-demo-run-8hzc6-random-for-head-688v9--lhg94\": Internal error occurred: admission webhook \"webhook.tekton.dev\" denied the request: mutation failed: invalid value: : taskspec.inputs.params.face.type","knative.dev/controller":"pipeline-controller"}

Steps to Reproduce the Problem

1. I'm not sure what the issue is yet, I suspect the type "string" must be specified on condition parameters

Additional Info

The validation error is reported on timeout:

flip-coin-condition-demo-run-8hzc6   19 minutes ago   ---          Running
flip-coin-condition-demo-run-fg4x9   1 hour ago       15 seconds   Failed
flip-coin-condition-demo-run-cb47k   1 hour ago       0 seconds    Failed(PipelineValidationFailed)

[dibyom]

I thought the type "string" should be added by default if one is not specified.

[dibyom]

Wonder if this is related to #1407

[afrittoli]

Yeah, it does for task parameters but it doesn't seem to be happening for conditions?
I'm not running on openshift, and the webhook process has been running for days just fine.

[dibyom]

/assign

[dibyom]

So two issues here:

1. We do not set the default parameter type if we are embedding the taskSpec inside a taskrun (which is what conditional uses internally). Looking at the example taskruns, all of them explicitly set the param type to string, so we have not run into this before

2. If we return an error from the reconcile function, we never call the bit that updates the Status. This is usually fine since returning an error will add it back in the queue for another reconcile cycle. Unfortunately the embedded error here is a validation error so rerunning reconcile makes no difference.
   

   pipeline/pkg/reconciler/pipelinerun/pipelinerun.go

   Line 170 in ef8a6b2

   return err

[afrittoli]

So two issues here:

1. We do not set the default parameter type if we are embedding the taskSpec inside a taskrun (which is what conditional uses internally). Looking at the example taskruns, all of them explicitly set the param type to string, so we have not run into this before

D'oh, that was me updating the taskruns a couple of days ago. Because CI was broken we did not run the taskruns in CI for a while, so we didn't realize that the embedded task did not get the default until I found the issue while fixing CI. But I didn't manage yet to dig into the root cause, which you now did, thanks!

2. If we return an error from the `reconcile` function, we never call the bit that updates the `Status`. This is usually fine since returning an error will add it back in the queue for another reconcile cycle. Unfortunately the embedded error here is a validation error so rerunning reconcile makes no difference.
   https://github.com/tektoncd/pipeline/blob/ef8a6b2259724f0e29f2bfb87591564289fbf7b5/pkg/reconciler/pipelinerun/pipelinerun.go#L170

I believe this is #1204, which has been around for a while now.

[dibyom]

I believe this is #1204, which has been around for a while now.

Nice! I'm going to review that!!