TEP-0061 Allow custom task to be embedded in pipeline

[ScrapCodes]

See issue tektoncd/pipeline#3682, for initial discussion regarding this TEP.

/cc @Tomcli, @litong01

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ litong01 (38fce90)

[Tomcli]

/cc @afrittoli @pritidesai

[Tomcli]

@animeshsingh

[pritidesai]

Thank you @ScrapCodes for this proposal 🙏

Some house keeping - please squash the commits into one.

[pritidesai]

This can go in the alternative section if needed. Combination of taskRef and taskSpec is generally avoided so far, it's either one or the other allowed.

This is not needed in the proposal. Just taskSpec is sufficient to inline custom specifications. Like @imjasonh suggested in one of the comments in the issue, taskSpec can hold apiVersion, kind, and spec.

This would require to change the EmbeddedTask definition to:

type EmbeddedTask struct {
	// +optional
	Metadata PipelineTaskMetadata `json:"metadata,omitempty"`

	// TaskSpec is a specification of a task
	TaskSpec `json:",inline,omitempty"`

        // APIVersion and Kind can be specified here.
	runtime.TypeMeta `json:",inline,omitempty"`

        // inline custom task specification here
	Spec             runtime.RawExtension `json:"spec,omitempty"`
}

[pritidesai]

With this change to the EmbeddedTask struct, inlining task-loop specifications can be done with:

spec:
  tasks:
    - name: run-my-tests
      taskSpec:
        apiVersion: custom.tekton.dev/v1alpha1
        kind: TaskLoop
        spec:
          taskRef:
            name: simpletask
          iterateParam: word
          timeout: 60s
          retries: 2

[pritidesai]

Please drop this for now, this can be added back with the proposal.

[imjasonh]

Thanks for writing this up! I'm excited to see this work moving forward. 👍

[imjasonh]

Can you elaborate on this point with an example Run YAML that the above Task would produce? I think that would help illustrate what the proposal is suggesting. It could also be helpful to show what code changes you're proposing to the Run struct, along with @pritidesai 's Go and YAML examples in her comments.

Adding taskSpec to the Run type adds a requirement for Run controller authors, which we'll need to describe in documentation. Some controllers might only want to support taskRef and not taskSpec (or vice versa), and we should give guidance about how they can signal that to users.

[imjasonh]

I don't think we should prescribe that a Run controller should use webhook validation necessarily (even though I think it's a good idea). Webhooks in k8s are unfortunately still very annoying to set up. Async validation in reconciling might be better for some Custom Tasks, so we should allow that.

Maybe loosen the wording to "custom controllers should consider having a webhook to validate..." ?

[Tomcli]

Thanks, the webhook could be optional and let the custom task owner decide whether to implement it.

[ScrapCodes]

The committers are authorized under a signed CLA.

• ✅ litong01 (38fce90)
• ✅ Prashant Sharma (8700292)

[ScrapCodes]

Thanks a lot @pritidesai, @imjasonh and @Tomcli for the quickest review. 🚀

I have dropped Proposal section for now, we can add that later. Please Take a look again !

[afrittoli]

+100

Having Tekton resources generated through a DSL or an external platform is a very common use case, and this very much reduces the orchestration complexity for those use cases.

[afrittoli]

Would you then add support for embedded spec to the test controller?

[Tomcli]

We will update the custom task test over here. Looks like there no test controller running in the e2e test?
https://github.com/tektoncd/pipeline/blob/main/test/custom_task_test.go

[afrittoli]

Thank you for this! This addition makes very much sense.
The embedded spec should be an optional part of the Run API.
I believe that's the intention anyways, and something we can specify along with the proposal details.

[dibyom]

/assign @vdemeester
/assign @sbwsg

/cc @imjasonh @afrittoli

[ghost]

Could you clarify this a bit? It sounds in the beginning like custom tasks are submitted to k8s along with a pipeline but then the second part of the sentence says that, no, custom tasks are actually submitted separately. I don't totally understand what we mean here.

[Tomcli]

Sorry, there are some mistakes on the wording. The formal custom task is referring to the pipelineTask type. The latter custom task is referring to the custom resources created for the custom task. I will update the summary to make it more clear.

[ghost]

This jumps directly into a general assertion that there are natural or unnatural ways for users to work with Kubernetes. This is useful context but I think it would be really helpful if we could nail down a specific motivation too.

From your experiences using custom tasks can you include here a description of the exact problem you and your users face? Ideally not generalized over an entire problem area but very concretely: User X does A, User Z does B, these actions collide because C, we are then motivated to add Custom Task Specs to fix it by ...

[Tomcli]

We will move our Kubeflow pipeline use case over here since that's the exact problem we are facing

[ghost]

This is, I think, exactly what I was looking for in the motivation section. Could you move this in to the motivation section and elaborate on it a little bit: "we first have to call the api to get X, then we have to call to get Y, this could be much simpler if we just had the whole thing available in a single place".

[Tomcli]

Will do.

[ghost]

I've read this and I've read the associated issue but I still don't quite grok what we mean by "managing the custom task CR". What is being managed? Also, which users are we talking about here? The KFP end-users or KFP devs?

[Tomcli]

Here we are just referring users that run pipeline directly on Kubernetes using Tekton. I can update the use cases here to fit more in terms of the KFP scenario.

[ghost]

This likely cannot be known until the implementation is proposed. Suggest simply omitting this for the time being.

[Tomcli]

will do

[ghost]

I believe you're referring here to the fact that deployments and such allow the user to define a podTemplate or similar, is that right? Could you call that out specifically as part of this? Otherwise it's left up to interpretation and prior knowledge/understanding of those resource types.

[Tomcli]

will do

[ghost]

Could we soften the wording of this one a bit? I think there are approaches here where the custom task controller could specify the validation (e.g. as openapi) and pipelines controller enforces that validation.

We might not want to make this required, or the default expectation, but I am a tad nervous about "only" treating this field as a generic blob which the pipelines controller has no insight into. It could make future work around conformance and Custom Tasks (for example) considerably more complicated if we've blanket prohibited the pipelines controller from performing any verification / validation.

We could remove this goal and simply leave it as a non-goal for now?

[Tomcli]

Yes, we can move this to non-goal

[ScrapCodes]

@sbwsg, @Tomcli, Hi Scot and Tommy, I am still improving the document as per the feedback, will let you guys know once I have an update.

[ScrapCodes]

@sbwsg Please take another look !

[ghost]

Thanks a lot for tackling all those changes! I'm very excited to see this move ahead.

/approve

[ghost]

👍

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ImJasonH, sbwsg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• teps/OWNERS [ImJasonH,sbwsg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[pritidesai]

thank you @ScrapCodes @Tomcli 🙏
/lgtm

[Tomcli]

Thanks everyone for reviewing this TEP!!