TEP-0114: Custom Tasks Beta - Required Work

[jerop]

In TEP-0114: Custom Tasks Beta, we proposed migrating Custom Tasks and Runs to beta. This is an overarching issue to track the work required to promote Custom Tasks to beta.

The work scoped here are blockers for promotion.

• TEP-0114: Custom Tasks Beta - Testing #5156
• TEP-0114: Custom Tasks Beta - References and Specifications #5154
• TEP-0114: Custom Tasks Beta - Support v1beta1.CustomRun #5153
• TEP-0114: Custom Tasks Beta - Conversion from v1alpha1.Run to v1beta1.CustomRun #5443
• TEP-0114: Updating pipelineTask spec to accomodate for CustomRun and Run #5532
• TEP-0114: Custom Tasks Beta - default enable-custom-tasks to "true" #5155
• TEP-0114: Custom Tasks Beta - Documentation #5158

The pull requests completing the above work are here.

[pritidesai]

/assign

[lbernick]

Experimenting with Custom Tasks for the ColocatedPipelineRun POC, wanted to leave a few notes on things we might want to improve for beta:

• Codegen is really difficult. It seems like the best solution right now is to add the new Go types, not import them anywhere yet, copy go.mod and codegen scripts from another working example, run go mod vendor, run the codegen, then add the reconciler/controller.
  • go structs also need to have the correct annotations e.g. // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object which doesn't seem to be documented
• Using a Custom Task as a runtime type that executes an authoring time type is awkward (here, a ColocatedPipelineRun executes a Pipeline)-- the following issues can probably be addressed with better docs
  • I wrote code which copies the status of the ColocatedPipelineRun to the status of the Run and vice versa (now that I think about it maybe ColocatedPipelineRun doesn't need to have its own status-- but this probably highlights the need for better examples)
  • I want to have multiple timeouts fields for ColocatedPipelineRun (similar to the multiple timeouts on PipelineRun) but it's unclear how this should interact with the Run timeout
  • getting the embedded spec of the custom object via Run.Spec.Spec.Spec is quite confusing
• You have to write your own serialization/deserialization for Run.Status.ExtraFields and Run.Spec.Spec.Spec, which can silently go wrong and is awkward to test (in my case I realized that the JSON annotations were incorrect and JSON.Unmarshal by default ignores unknown fields instead of returning an error)

[vdemeester]

• Codegen is really difficult. It seems like the best solution right now is to add the new Go types, not import them anywhere yet, copy go.mod and codegen scripts from another working example, run go mod vendor, run the codegen, then add the reconciler/controller.

  • go structs also need to have the correct annotations e.g. // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object which doesn't seem to be documented

I tend to agree, but this is "only true" for custom task reconciler that are written in Go 👼🏼 , aka it's the same problem you have when writing any CRD and reconciler with Go more or less.

• Using a Custom Task as a runtime type that executes an authoring time type is awkward (here, a ColocatedPipelineRun executes a Pipeline)-- the following issues can probably be addressed with better docs
  • I wrote code which copies the status of the ColocatedPipelineRun to the status of the Run and vice versa (now that I think about it maybe ColocatedPipelineRun doesn't need to have its own status-- but this probably highlights the need for better examples)
  • I want to have multiple timeouts fields for ColocatedPipelineRun (similar to the multiple timeouts on PipelineRun) but it's unclear how this should interact with the Run timeout
  • getting the embedded spec of the custom object via Run.Spec.Spec.Spec is quite confusing

+:100:

[lbernick]

#4686 is relevant here-- there's some discussion of adding a custom task controller for testing; could be generally useful for developing on custom tasks

[lbernick]

Created #5120 to track implementing a custom task controller for testing.

[jerop]

@tektoncd/core-maintainers updated this issue reflect what we agreed to do to promote custom tasks to beta in TEP-0114

[lbernick]

One more idea I'd like to bring up for beta here after pairing on this with @JeromeJu: I'm wondering if it would make sense to have the PipelineRun controller responsible for retries of custom runs, as opposed to the current behavior of a custom task controller being responsible for implementing retry behavior. This would make custom tasks easier to author, and make them more consistent with how we handle TaskRuns. It would involve removing spec.retries from customRun, and the PR controller would be fully responsible for updating retriesstatus when a run has failed. This solution was discussed in this thread, and it looks like it was rejected to avoid having the PipelineRun controller be responsible for updating Run statuses, but that's a pattern we already use with TaskRuns. @dibyom @jerop @ScrapCodes what do you think about this?

[vdemeester]

This solution was discussed in this thread, and it looks like it was rejected to avoid having the PipelineRun controller be responsible for updating Run statuses, but that's a pattern we already use with TaskRuns. @dibyom @jerop @ScrapCodes what do you think about this?

How is it a pattern we use for TaskRuns ? The PipelineRun controller should not mutate the TaskRun status at all, only the TaskRun controller does (which happens to be in the same binary).

[lbernick]

How is it a pattern we use for TaskRuns ? The PipelineRun controller should not mutate the TaskRun status at all, only the TaskRun controller does (which happens to be in the same binary).

happens here

[vdemeester]

How is it a pattern we use for TaskRuns ? The PipelineRun controller should not mutate the TaskRun status at all, only the TaskRun controller does (which happens to be in the same binary).

happens here

😱 🙀

[dibyom]

This would make custom tasks easier to author, and make them more consistent with how we handle TaskRuns. It would involve removing spec.retries from customRun, and the PR controller would be fully responsible for updating retriesstatus when a run has failed.

How would retries work if the Run was created by itself without being in a Pipeline? Can easy retry behavior be something we add as part of an SDK for making authoring custom tasks easier?

[lbernick]

How would retries work if the Run was created by itself without being in a Pipeline?

They wouldn't, just as TaskRuns can't retry on their own -- retries are a Pipeline feature.

Can easy retry behavior be something we add as part of an SDK for making authoring custom tasks easier?

potentially, although it's still harder than having it implemented in the PR controller.

Sounds like this needs more discussion, so I created #5218.