Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add metadata to PipelineTask to specific additional labels and annotations #2767

Closed
ckadner opened this issue Jun 5, 2020 · 23 comments · Fixed by #2826
Closed

Add metadata to PipelineTask to specific additional labels and annotations #2767

ckadner opened this issue Jun 5, 2020 · 23 comments · Fixed by #2826
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature.

Comments

@ckadner
Copy link

ckadner commented Jun 5, 2020

Update issue title and description on 6/9/2020:


Metadata is not part of the taskspec, tasks contain both a taskspec to define the behavior and metadata to identify the task k8s resource. When a taskspec is embedded in a pipelinespec instead of referencing a task, their is no separate task k8s resource so we don't need task specific metadata since it wouldn't make sense.

We need to have support for setting different annotations or metadata on the TaskRun created from embedded TaskSpec define in Pipelines (from the Pipeline Label and Annotation), am I right ?

I could see having the following:

apiVersion: tekton.dev/v1beta1
kind: PipelineRun
metadata:
  name: pipelinerun-with-taskspec-to-echo-greetings
spec:
  pipelineSpec:
    params:
    - […]
    tasks:
    - name: echo-greetings
      medata:
        labels: [ …]  - name: GREETINGS
        description: "Greetings, default is Hello World!"
        type: string
        default: "Hello World!"
        annotations: […]
      taskSpec:
        […]

It should be added on all embedded spec types. It wouldn't break backward compatibility.


Original Issue Question:

Unsupported features for embedded pipelineSpec, taskSpec?

Can any Tekton YAML that contains separate "documents" for Tasks, Pipeline, PipelineRun be converted to a YAML file containing only one PipelineRun "document" with embedded pipelineSpec which embeds multiple taskSpecs?

One thing that seems to get lost are Task metadata like pod label annotations.

Related issues:
#872
#1333
#1554

Answer from @vdemeester:

AFAIK there should be no lost capabilities from embedding resource specs instead of referencing separate resources. It changes which k8s resources the data is stored in, but all the same specs are provided.

@tekton-robot tekton-robot added the kind/question Issues or PRs that are questions around the project or a particular feature label Jun 5, 2020
@animeshsingh
Copy link

@cjwagner @pritidesai

cc @afrittoli

@pritidesai
Copy link
Member

Here are examples of how tasks and pipeline can be embedded:

Yes, it is possible to embed multiple taskSpecs under pipelineSpec which in turn can be embedded under pipelineRun.

@pritidesai
Copy link
Member

pritidesai commented Jun 6, 2020

Metadata for first example:

 kubectl get pr pipelinerun-echo-greetings -o json | jq .metadata
{
  "annotations": {
    "kubectl.kubernetes.io/last-applied-configuration": "{\"apiVersion\":\"tekton.dev/v1beta1\",\"kind\":\"PipelineRun\",\"metadata\":{\"annotations\":{},\"name\":\"pipelinerun-echo-greetings\",\"namespace\":\"default\"},\"spec\":{\"params\":[{\"name\":\"MORNING_GREETINGS\",\"value\":\"Good Morning, Bob!\"},{\"name\":\"NIGHT_GREETINGS\",\"value\":\"Good Night, Bob!\"}],\"pipelineSpec\":{\"params\":[{\"default\":\"Good Morning!\",\"description\":\"morning greetings, default is Good Morning!\",\"name\":\"MORNING_GREETINGS\",\"type\":\"string\"},{\"default\":\"Good Night!\",\"description\":\"Night greetings, default is Good Night!\",\"name\":\"NIGHT_GREETINGS\",\"type\":\"string\"}],\"tasks\":[{\"name\":\"echo-good-morning\",\"params\":[{\"name\":\"MESSAGE\",\"value\":\"$(params.MORNING_GREETINGS)\"}],\"taskRef\":{\"name\":\"task-echo-message\"}},{\"name\":\"echo-good-night\",\"params\":[{\"name\":\"MESSAGE\",\"value\":\"$(params.NIGHT_GREETINGS)\"}],\"taskRef\":{\"name\":\"task-echo-message\"}}]}}}\n"
  },
  "creationTimestamp": "2020-06-05T23:54:14Z",
  "generation": 1,
  "labels": {
    "tekton.dev/pipeline": "pipelinerun-echo-greetings"
  },
  "name": "pipelinerun-echo-greetings",
  "namespace": "default",
  "resourceVersion": "4195854",
  "selfLink": "/apis/tekton.dev/v1beta1/namespaces/default/pipelineruns/pipelinerun-echo-greetings",
  "uid": "cef3aeef-0d40-4e96-9759-5e89079b93fb"
}

Anything missing here?

@ckadner
Copy link
Author

ckadner commented Jun 6, 2020

@pritidesai -- what we would want is metadata.labels on the embedded taskSpec

apiVersion: tekton.dev/v1beta1
kind: PipelineRun
metadata:
  name: pipelinerun-with-taskspec-to-echo-greetings
spec:
  pipelineSpec:
    params:
      - name: GREETINGS
        description: "Greetings, default is Hello World!"
        type: string
        default: "Hello World!"
    tasks:
      - name: echo-greetings
        taskSpec:
          metadata:  # <--- ERROR: cannot decode incoming new object: json: unknown field "metadata"
            labels:
              app: containerOp
          params:
          - name: MESSAGE
            type: string
            default: "Hello World!"
          steps:
            - name: echo
              image: ubuntu
              script: |
                #!/usr/bin/env bash
                echo "$(params.MESSAGE)"

$ kubectl apply -f temp/embeded_pipelinespec_embedded_taskspec.yaml

Error from server (BadRequest): error when creating "temp/embeded_pipelinespec_embedded_taskspec.yaml": admission webhook "webhook.pipeline.tekton.dev" denied the request: mutation failed: cannot decode incoming new object: json: unknown field "metadata"

But that is just one example. Really this question is about any know capabilities that would get lost when transforming from multiple separate Task documents to embedded taskSpecs.

@cjwagner
Copy link

cjwagner commented Jun 8, 2020

Here is my understanding:
Metadata is not part of the taskspec, tasks contain both a taskspec to define the behavior and metadata to identify the task k8s resource. When a taskspec is embedded in a pipelinespec instead of referencing a task, their is no separate task k8s resource so we don't need task specific metadata since it wouldn't make sense.
AFAIK there should be no lost capabilities from embedding resource specs instead of referencing separate resources. It changes which k8s resources the data is stored in, but all the same specs are provided.

@pritidesai
Copy link
Member

pritidesai commented Jun 8, 2020

Right, metadata is associated with CRD and can not be specified under specification, like following task metadata, pipeline metadata, etc:

apiVersion: tekton.dev/v1beta1
kind: Task
metadata:
  name: task-echo-good-morning
spec:

Pipeline metadata:

apiVersion: tekton.dev/v1beta1
kind: Pipeline
metadata:
  name: pipeline-to-echo-good-morning
spec:

CRD metadata is stored inside the resources themselves, in case of embedding resources, no resources are created and hence no metadata.

What is your use case? may be we can brainstorm and figure out how to support it

@Tomcli
Copy link
Contributor

Tomcli commented Jun 8, 2020

Is it possible to update the podTemplate so that we can add labels/annotations to taskRun without creating the task CRD?
https://github.com/tektoncd/pipeline/blob/master/pkg/apis/pipeline/pod/template.go

The custom label or annotations are not implemented in podTemplate, but we want to see can we add that support to the podTemplate similar to how ImagePullSecrets was done
550e3b4

@animeshsingh
Copy link

Thanks @Tomcli @ckadner

@pritidesai @afrittoli see if this can be prioritized - parts of our implementation definitely dependent on being able to pass lables/annotations at a taskRun level in addition to higher pipelineRun level

@Tomcli
Copy link
Contributor

Tomcli commented Jun 9, 2020

i.e. We want to append the extra pod labels or annotations to here https://github.com/tektoncd/pipeline/blob/master/pkg/pod/pod.go#L259-L260

@animeshsingh
Copy link

And ideally want it to be part of 0.13 bug-fix or patch, or 0.14 release latest for us to be able to deliver timely.

@Tomcli
Copy link
Contributor

Tomcli commented Jun 9, 2020

@ckadner maybe we can update the issue description to be a little bit more concise for metadata.labels and annotations? The other metadata are irrelevant in Kubeflow and our use case.

@vdemeester
Copy link
Member

@pritidesai @afrittoli see if this can be prioritized - parts of our implementation definitely dependent on being able to pass lables/annotations at a taskRun level in addition to higher pipelineRun level

@animeshsingh if I understand correctly, you need to have support for setting different annotations or metadata on the TaskRun created from embedded TaskSpec define in Pipelines (from the Pipeline Label and Annotation), am I right ?

I could see having the following:

apiVersion: tekton.dev/v1beta1
kind: PipelineRun
metadata:
  name: pipelinerun-with-taskspec-to-echo-greetings
spec:
  pipelineSpec:
    params:
    - […]
    tasks:
    - name: echo-greetings
      medata:
        labels: [ …]  - name: GREETINGS
        description: "Greetings, default is Hello World!"
        type: string
        default: "Hello World!"
        annotations: […]
      taskSpec:
        […]

It should be added on all embedded spec types. It wouldn't break backward compatibility.

And ideally want it to be part of 0.13 bug-fix or patch, or 0.14 release latest for us to be able to deliver timely.

A 0.13 bug-fix would definitely be not possible, it's not a bug, it's a new feature to add.

/kind feature
/unkind question

We should retitle the issue "Add metadata to PipelineTask to specific additional labels and annotations" 👼 🙏

@tekton-robot tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jun 9, 2020
@vdemeester vdemeester removed the kind/question Issues or PRs that are questions around the project or a particular feature label Jun 9, 2020
@animeshsingh
Copy link

Thanks @vdemeester. A 0.14 can land just in time for us - what's the timeframe for cutting it?

@ckadner please rename the issue.

@vdemeester
Copy link
Member

Thanks @vdemeester. A 0.14 can land just in time for us - what's the timeframe for cutting it?

I think the current deadline for 0.14 is on 1st July of 2020 for 0.14 🙃

@Tomcli
Copy link
Contributor

Tomcli commented Jun 9, 2020

@animeshsingh if I understand correctly, you need to have support for setting different annotations or metadata on the TaskRun created from embedded TaskSpec define in Pipelines (from the Pipeline Label and Annotation), am I right ?

Thanks @vdemeester. That's what we want.

@vdemeester
Copy link
Member

@animeshsingh if I understand correctly, you need to have support for setting different annotations or metadata on the TaskRun created from embedded TaskSpec define in Pipelines (from the Pipeline Label and Annotation), am I right ?

Thanks @vdemeester. That's what we want.

I think that's a fair ask and doable. cc @bobcatfish @pritidesai @afrittoli

@ckadner ckadner changed the title Unsupported features for embedded pipelineSpec, taskSpec? Add metadata to PipelineTask to specific additional labels and annotations Jun 9, 2020
@tekton-robot tekton-robot added the kind/question Issues or PRs that are questions around the project or a particular feature label Jun 9, 2020
@ckadner
Copy link
Author

ckadner commented Jun 9, 2020

/remove-kind question

@tekton-robot tekton-robot removed the kind/question Issues or PRs that are questions around the project or a particular feature label Jun 9, 2020
@pritidesai
Copy link
Member

@vdemeester does it mean introducing metadata in PipelineTask? It has to be TaskMetadata and can not be PipelineMetadata.

type PipelineTask struct {
	Name string `json:"name,omitempty"`
	Metadata metav1.ObjectMeta `json:"metadata"`
...

Do we introduce the same metadata at the pipelineSpec level?

type PipelineRunSpec struct {
	Metadata metav1.ObjectMeta `json:"metadata"`
...

@vdemeester
Copy link
Member

@pritidesai That might be a way yes 🙃 We don't necessary need to use metav1.ObjectMeta and just define our own with the map[string]string for labels and annotations.

It has to be TaskMetadata and can not be PipelineMetadata.

Not sure I follow 😝 both are the same type 🙃

@pritidesai
Copy link
Member

/assign

@animeshsingh
Copy link

@pritidesai thanks for tackling this - would love for this to be in master before v0.14 release, which i am hoping comes out some timeframe in July, and is adopted in ibm cloud for us to be able to leverage functionality finally on product side.

cc @Tomcli @skaegi

@pritidesai
Copy link
Member

yup, working on a PR as we speak 👍

@pritidesai
Copy link
Member

PR #2826 is created to support metadata with taskSpec in a PipelineTask to allow specifying labels and annotations when a task is embedded using taskSpec. These labels and annotations are propagated to taskRun and then to the pods.

These labels are commonly used to identify and filter pods for further actions (such as collecting pod metrics, and cleaning up completed pod with certain labels, etc) even being part of one single Pipeline.

  pipelineSpec:
    tasks:
    - name: echo
      metadata:
        labels:
          pipeline-sdk-type: kfp
      taskSpec:
       ...
    - name: echo2
      metadata:
        labels:
          pipeline-sdk-type: tfx
      taskSpec:
       ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants