From e99ee250a2317c5ac26f56afc6e238d2f2365352 Mon Sep 17 00:00:00 2001 From: Prashant Sharma Date: Fri, 23 Apr 2021 17:25:03 +0530 Subject: [PATCH] TEP-0061 Allow custom task to be embedded in pipeline, design. --- ...-custom-task-to-be-embedded-in-pipeline.md | 107 ++++++++++++++++-- teps/README.md | 2 +- 2 files changed, 96 insertions(+), 13 deletions(-) diff --git a/teps/0061-allow-custom-task-to-be-embedded-in-pipeline.md b/teps/0061-allow-custom-task-to-be-embedded-in-pipeline.md index 37115a646..c7e373615 100644 --- a/teps/0061-allow-custom-task-to-be-embedded-in-pipeline.md +++ b/teps/0061-allow-custom-task-to-be-embedded-in-pipeline.md @@ -1,8 +1,8 @@ --- -status: proposed +status: implementable title: Allow custom task to be embedded in pipeline creation-date: '2021-03-18' -last-updated: '2021-03-27' +last-updated: '2021-04-28' authors: - '@Tomcli' - '@litong01' @@ -80,6 +80,7 @@ same namespace create resource. Related issue 1. Custom task controllers are to be developed by other parties. Custom task specification validation by Tektoncd/Pipeline webhooks. +2. Handling `metadata` for embedded custom tasks i.e. `taskSpec`. ### Use Cases (optional) @@ -117,8 +118,8 @@ Add support for `Run.RunSpec.Spec`. Currently, `Run.RunSpec.Spec` is not supported and there are validations across the codebase to ensure, only `Run.RunSpec.Ref` is specified. As part of this TEP, in addition -to adding support for `Run.RunSpec.Spec`the validations will be changed to support -"One of `Run.RunSpec.Spec` or`Run.RunSpec.Ref`" only and not both as part of a single +to adding support for `Run.RunSpec.Spec` the validations will be changed to support +"One of `Run.RunSpec.Spec` or `Run.RunSpec.Ref`" only and not both as part of a single API request to kubernetes. Structure of `RunSpec` after adding the field `Spec` of type `*v1beta1.EmbeddedTask`, @@ -129,7 +130,7 @@ type RunSpec struct { // +optional Ref *TaskRef `json:"ref,omitempty"` - // TaskSpec is a specification of a custom task + // Spec is a specification of a custom task // +optional Spec *v1beta1.EmbeddedTask `json:"spec,omitempty"` @@ -343,6 +344,14 @@ Consider which use cases are impacted by this change and what are their performance requirements. --> +Performance improvement is a consequence of reduction in number of API request(s) to +create custom resource(s) accompanying a pipeline. In pipelines, where the number +of custom task resource objects are large, this can make a huge difference in +performance improvement. + +For the end users, trying to render the custom task resource details on the UI dashboard, +can be a much smoother experience if all the requests could be fetched in fewer API request(s). + ## Design Details +The actual code changes needed to implement this TEP, are very minimal. + +**Broad categories are:** +1. Add the relevant APIs. + Already covered in [Proposal section](#proposal). + +2. Change validation logic to accept the newly added API fields. + Currently `tecktoncd/pipeline` will reject any request for `Run`, + which does not include a `Run.RunSpec.Ref`. So this validation is now changed to + either one of `Ref` or `Spec` must be present, but not both. + + Next, whether it is a `Ref` or a `Spec`, validation logic will ensure, they have + non-empty values for, `APIVersion` and `Kind`. + + Lastly document advice for downstream custom controllers to implement their + own validation logic. This aspect is covered in full detail, in + [Upgrade & Migration Strategy section](#upgrade--migration-strategy-optional) + of this TEP. + +This TEP does not change the existing flow of creation of `Run` object, it updates +the Run object with the content of `RunSpec.Spec` by marshalling the field +`Spec runtime.RawExtension` to json and embed in the spec, before creating the +Run object. + +Code changes maybe implemented in three PRs. + +1. *Core changes*, i.e. Addition of APIs as per approved proposal, change validations and update +unit tests for testing new APIs and validation logic. + +2. *e2e tests* Add an e2e test for testing a pipelineRun with custom task embedded. + +3. *documentation* Update documentation on custom tasks, to include embedded spec. Also add the + guide on validation for downstream custom task developers. ## Test Plan @@ -374,20 +416,37 @@ All code is expected to have adequate tests (eventually with coverage expectations). --> -We can reuse the custom task e2e tests with the current test controller -to verify whether the controller can handle the custom task `taskSpec`. +We can reuse the current custom task e2e tests which simulates a custom task controller +updating `Run` status. Then, verify whether the controller can handle the custom task +`taskSpec` as well or not. ## Design Evaluation +Before the implementation of this TEP, i.e. without the support for embedding a +custom task spec in the `PipelineRun` resource, a user has to create multiple API +requests to the Apiserver. Next, he has to ensure unique names, to avoid conflict +in someone else's created custom task resource object. + +Embedding of custom task spec avoids the problems related to name collisions +and also improves performance by reducing the number of API requests to create custom +task resource objects. The performance benefit, of reducing the number of API requests, +is more evident when using web-ui based dashboard to display, pipeline details +(e.g. in Kubeflow Pipelines with tekton as backend). + +Lastly, it looks aesthetically nicer and coherent with existing regular task, +with all the custom task spec using fewer lines of yaml and all present in one place. ## Drawbacks - +One of the drawback can be, user may still use the `taskRef` for creating custom task +resource, based pipelines. As a result, may still run into "name collision issue" i.e. +when two or more users try to create a custom resource objects with same name. +Then it is difficult to say, which amongst them is actually run as part of the pipeline. + +This can be avoided by documenting the issue. ## Alternatives @@ -413,7 +472,31 @@ Use this section to detail wether this feature needs an upgrade or migration strategy. This is especially useful when we modify a behavior or add a feature that may replace and deprecate a current one. --> - +- **Existing custom controller need to upgrade their validation logic:** + + **Rationale:** Previously, there was only one possibility for the structure of `Run` objects, + i.e. they had the path as `Run.RunSpec.Ref`. A custom controller may do fine, + even without validating the input request(s) that misses a `Ref`. Because, + this was already validated by `tektoncd/pipeline`. After the implementation of + this TEP, this is no longer the case, a `Run.RunSpec` may either contain a `Ref` + or a `Spec`. So a request with a `Spec`, to a controller which does not have + proper validation for missing a `Ref`, and does not yet support a `Spec`, may + be rendered in an unstable state e.g. due to `nil dereference errors` or + fail due to timeout. + +- **Support `spec` or `taskSpec` in the existing custom controller:** + + With implementation of this tep, users can supply custom task spec embedded in a + `PipelineRun` or `Run`. The existing custom controller need to upgrade, to provide support. + +- **Unmarshalling the json of custom task object embedded as `Spec`:** + + `Run.RunSpec.Spec` objects are marshalled as binary by using `json.Marshal` where + `json` is imported from `encoding/json` library of golang. So the custom controller + may unmarshall these objects by using the corresponding `unmarshall` function as, + `json.Unmarshal(run.Spec.Spec.Spec.Raw, &customObjectSpec)`. + TODO: Add a reference to an example custom task controller e.g. `TaskLoop`. + ## References (optional) 1. [tektoncd/pipeline#3682](https://github.com/tektoncd/pipeline/issues/3682) -2. [Custom tasks](https://github.com/tektoncd/community/blob/main/teps/0002-custom-tasks.md) +2. [TEP-0002 Custom tasks](0002-custom-tasks.md) diff --git a/teps/README.md b/teps/README.md index aba2e27f6..40c6c4ba5 100644 --- a/teps/README.md +++ b/teps/README.md @@ -183,4 +183,4 @@ This is the complete list of Tekton teps: |[TEP-0056](0056-pipelines-in-pipelines.md) | Pipelines in Pipelines | proposed | 2021-03-08 | |[TEP-0058](0058-graceful-pipeline-run-termination.md) | Graceful Pipeline Run Termination | proposed | 2021-03-18 | |[TEP-0059](0059-skip-guarded-task-only.md) | Skip Guarded Task Only | proposed | 2021-03-24 | -|[TEP-0061](0061-allow-custom-task-to-be-embedded-in-pipeline.md) | Allow custom task to be embedded in pipeline | proposed | 2021-03-27 | +|[TEP-0061](0061-allow-custom-task-to-be-embedded-in-pipeline.md) | Allow custom task to be embedded in pipeline | implementable | 2021-04-28 |