Updated taskspec with nodeselector to allow for different nodeselector for each task. #2297

NikeNano · 2020-03-26T15:24:45Z

The goal with this PR is to allow for different task in a Pipeline to have different node selectors.

Currently it is only possible(correct my if I am wrong) to set the nodeslector on the podTemplate spec, which will result in that all task have the same node selector. This limites the node selection to one choice and also limits the workflow usage.

My first contribution to Tekton, super happy for feedback and any suggestions.

Changes

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

tekton-robot · 2020-03-26T15:24:49Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign afrittoli
You can assign the PR to them by writing /assign @afrittoli in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

tekton-robot · 2020-03-26T15:25:00Z

Hi @NikeNano. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

NikeNano · 2020-03-27T07:26:33Z

/assign @afrittoli

dibyom · 2020-03-30T17:42:11Z

/ok-to-test

dibyom · 2020-03-30T17:46:52Z

I think of the ideas with the PodTemplate is that it is part of the Run object types and not the Task/Pipeline. This makes the Task/Pipeline types reusable across clusters. The use case is interesting though so I'll put this on the Working Group agenda to see what others think of this

tekton-robot · 2020-03-30T18:32:19Z

@NikeNano: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-tekton-pipeline-build-tests	`dbca76b`	link	`/test pull-tekton-pipeline-build-tests`
pull-tekton-pipeline-integration-tests	`dbca76b`	link	`/test pull-tekton-pipeline-integration-tests`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

jlpettersson · 2020-03-30T19:54:48Z

pkg/apis/pipeline/v1alpha1/pipeline_types.go

+
+	// Set the NodeSelector on PipelineTask level to allow
+	// for different NodeSelectors for different steps.
+	NodeSelector map[string]string `json:"nodeSelector,omitempty"`
 }


I think there need to be a validation so that NodeSelector and Workspace is not used together. Tasks using Workspace need to be scheduled to where the e.g. PVC (volume) is bound.

Thx, I will check it out :). New to tekton so need to read up on workspaces!

NikeNano · 2020-03-30T20:38:41Z

I think of the ideas with the PodTemplate is that it is part of the Run object types and not the Task/Pipeline. This makes the Task/Pipeline types reusable across clusters. The use case is interesting though so I'll put this on the Working Group agenda to see what others think of this

Would it be better suited to move the functionality to the run objects using the names/tags to set nodeselectors to specific tasks during run? This PR is related to working on a PR for kubeflow pipelines related to tekton https://github.com/kubeflow/kfp-tekton.

vdemeester · 2020-03-31T07:43:05Z

I think of the ideas with the PodTemplate is that it is part of the Run object types and not the Task/Pipeline. This makes the Task/Pipeline types reusable across clusters. The use case is interesting though so I'll put this on the Working Group agenda to see what others think of this

Would it be better suited to move the functionality to the run objects using the names/tags to set nodeselectors to specific tasks during run? This PR is related to working on a PR for kubeflow pipelines related to tekton https://github.com/kubeflow/kfp-tekton.

@NikeNano You can already specficy NodeSelectors on the *Run object using podTemplate : see podTemplate

vdemeester · 2020-04-01T11:55:23Z

Ah I see, what you need is a "by pipelinetask" podtemplate, a little bit like what we have for serviceAccount and serviceAccountNames 🤔

NikeNano · 2020-04-01T12:03:14Z

Ah I see, what you need is a "by pipelinetask" podtemplate, a little bit like what we have for serviceAccount and serviceAccountNames 🤔

Yeah, I would guess this is a use case that could be relevant for several uses cases outside the efforts of Kubeflow pipeline + Tekton.

vdemeester · 2020-04-01T12:12:40Z

Actually this is a bit trickier than I initially thought. Putting a hold to discuss this more.

The general needs is : being able to override runtime info (serviceAccountName, podTemplate) for a task or a set of task in the Pipeline execution (PipelineRun).

Right now, we do this only for serviceAccountName with a serviceAccounNames field :

spec:
  serviceAccountName: sa-1
  serviceAccountNames:
    - taskName: build-task
      serviceAccountName: sa-for-build

Adding a podTemplates on the PipelineRun spec would make it weird, I feel we may want a better way to do this.

spec:
  serviceAccountName: sa-1
  podTemplate: 
    securityContext:
      runAsNonRoot: true
    # […]
  taskOverride: # <- need a better name
  - name: build-task
    serviceAccountName: sa-for-build
    podTemplate:    
      securityContext:
        runAsNonRoot: true
      # […]

@bobcatfish @afrittoli @sbwsg @NikeNano wdyt ?

I would really love to keep those k8s specific and runtime field in PipelineRun and in a podTemplate as much as possible — mainly to reduce our implicit dependency to k8s structs.

/hold

NikeNano · 2020-04-01T14:41:03Z

I don't follow do you mean that the following:

spec:
  serviceAccountName: sa-1
  podTemplate: 
    securityContext:
      runAsNonRoot: true
    # […]
  taskOverride: # <- need a better name
  - name: build-task
    serviceAccountName: sa-for-build
    podTemplate:    
      securityContext:
        runAsNonRoot: true
      # […]
```'

is good or bad? I am new to Tekton so don't have that much of an opinion, but the above looks good to me. Have use argo a little bit and some times get the feeling that Tekton is more verbose and even simpler pipelines becomes quite verbose.

I would be happy to contribute if changes are needed in the controller or similar to handle the changes!

NikeNano · 2020-04-04T15:11:16Z

Any progress on this @dibyom ? :)

dibyom · 2020-04-06T20:18:11Z

I like the TaskOverride field in the PipelineRun that @vdemeester suggested. I'll add it to this week's WG meeting agenda as well. Assuming we agree on that, we can progress with this!

Working Group details and notes

bobcatfish · 2020-04-06T21:25:19Z

I would really love to keep those k8s specific and runtime field in PipelineRun and in a podTemplate as much as possible — mainly to reduce our implicit dependency to k8s structs.

I completely agree @vdemeester

Before we go ahead with anything @NikeNano could you describe in a bit more detail the use case where you need this? I think it will help us to figure out the best solution

NikeNano · 2020-04-07T15:26:33Z

@bobcatfish I made this initial PR as parts of the efforts for Kubeflow Pipelines to support both argo and Tekton as the underlaying infrastructure for execution. For Kubeflow Pipelines node selectors can have several use cases one of which is to allow for the usage of GPU resources example from the docs. I guess some users also want to have the controller of which node which workload should go to and thus utalize node selector. For the use case with GPU:s not all steps of a pipeline usually have the possibility to utilise GPU resources and thus the current setup where one PodTemplateSpec is used is not sufficient.

bobcatfish · 2020-04-08T16:59:48Z

Thanks for the explanation @NikeNano !

skaegi · 2020-04-08T17:20:54Z

Sorry... a little late to the party here. We "kustomize" our Tasks and Pipelines on-the-fly just prior to execution to support things like serviceName, namespace, runtimeclass, add bonus annotations etc. to support these kinds of flows. Maybe one option to consider is to somehow provide first-class support for kustomize in our "run" CRDs instead of adding our own override syntax.

NikeNano · 2020-04-08T17:47:53Z

Could you point me to where in the code base this is done @skaegi? Would like to take a look to see how it is done :) To me it sounds like this "kustomize" is very similar, how would this be specified?

skaegi · 2020-04-08T18:08:21Z

I'm embarrassed to say our agent is proprietary right now but kustomize is a technology built into kubectl and described here -- https://github.com/kubernetes-sigs/kustomize

NikeNano · 2020-04-09T06:40:46Z

I'm embarrassed to say our agent is proprietary right now but kustomize is a technology built into kubectl and described here -- https://github.com/kubernetes-sigs/kustomize

Ahh .... thanks for the link!

skaegi · 2020-04-09T16:06:25Z

I opened #2362 to look specifically at adding per task override that @vdemeester first mentioned as I think it might be the cleanest approach. The kustomization we do in our agent is different in that we are also altering non-runtime definitions. It's interesting but I think more flexible than what is needed.

NikeNano · 2020-04-14T09:39:41Z

Based upon your comment @skaegi, should we close this PR? I will start to work on #2362 in the mean time :)

vdemeester · 2020-04-14T15:57:39Z

@NikeNano it would make sense yes 😉
/close

tekton-robot · 2020-04-14T15:57:41Z

@vdemeester: Closed this PR.

In response to this:

@NikeNano it would make sense yes 😉
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

nodeselector for each task

dbca76b

tekton-robot requested review from afrittoli and imjasonh March 26, 2020 15:24

tekton-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 26, 2020

tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 26, 2020

tekton-robot assigned afrittoli Mar 27, 2020

NikeNano mentioned this pull request Mar 28, 2020

Identify initial set of KFP sample pipelines to test kubeflow/kfp-tekton#29

Closed

9 tasks

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 30, 2020

jlpettersson reviewed Mar 30, 2020

View reviewed changes

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 1, 2020

NikeNano mentioned this pull request Apr 14, 2020

Fix for #2362, adding support per TaskRun runtime settings. #2389

Merged

3 tasks

tekton-robot closed this Apr 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated taskspec with nodeselector to allow for different nodeselector for each task. #2297

Updated taskspec with nodeselector to allow for different nodeselector for each task. #2297

NikeNano commented Mar 26, 2020 •

edited

Loading

tekton-robot commented Mar 26, 2020

tekton-robot commented Mar 26, 2020

NikeNano commented Mar 27, 2020

dibyom commented Mar 30, 2020

dibyom commented Mar 30, 2020

tekton-robot commented Mar 30, 2020

jlpettersson Mar 30, 2020

NikeNano Mar 30, 2020

NikeNano commented Mar 30, 2020

vdemeester commented Mar 31, 2020

vdemeester commented Apr 1, 2020

NikeNano commented Apr 1, 2020

vdemeester commented Apr 1, 2020 •

edited

Loading

NikeNano commented Apr 1, 2020

NikeNano commented Apr 4, 2020

dibyom commented Apr 6, 2020

bobcatfish commented Apr 6, 2020

NikeNano commented Apr 7, 2020

bobcatfish commented Apr 8, 2020

skaegi commented Apr 8, 2020 •

edited

Loading

NikeNano commented Apr 8, 2020

skaegi commented Apr 8, 2020

NikeNano commented Apr 9, 2020

skaegi commented Apr 9, 2020

NikeNano commented Apr 14, 2020

vdemeester commented Apr 14, 2020

tekton-robot commented Apr 14, 2020

Updated taskspec with nodeselector to allow for different nodeselector for each task. #2297

Updated taskspec with nodeselector to allow for different nodeselector for each task. #2297

Conversation

NikeNano commented Mar 26, 2020 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

tekton-robot commented Mar 26, 2020

tekton-robot commented Mar 26, 2020

NikeNano commented Mar 27, 2020

dibyom commented Mar 30, 2020

dibyom commented Mar 30, 2020

tekton-robot commented Mar 30, 2020

jlpettersson Mar 30, 2020

Choose a reason for hiding this comment

NikeNano Mar 30, 2020

Choose a reason for hiding this comment

NikeNano commented Mar 30, 2020

vdemeester commented Mar 31, 2020

vdemeester commented Apr 1, 2020

NikeNano commented Apr 1, 2020

vdemeester commented Apr 1, 2020 • edited Loading

NikeNano commented Apr 1, 2020

NikeNano commented Apr 4, 2020

dibyom commented Apr 6, 2020

bobcatfish commented Apr 6, 2020

NikeNano commented Apr 7, 2020

bobcatfish commented Apr 8, 2020

skaegi commented Apr 8, 2020 • edited Loading

NikeNano commented Apr 8, 2020

skaegi commented Apr 8, 2020

NikeNano commented Apr 9, 2020

skaegi commented Apr 9, 2020

NikeNano commented Apr 14, 2020

vdemeester commented Apr 14, 2020

tekton-robot commented Apr 14, 2020

NikeNano commented Mar 26, 2020 •

edited

Loading

vdemeester commented Apr 1, 2020 •

edited

Loading

skaegi commented Apr 8, 2020 •

edited

Loading