Add a PodTemplate field for PipelineRun and TaskRun #1004

vdemeester · 2019-06-24T15:18:03Z

Changes

This allows to group and add PodSpec customizations to PipelineRun
and TaskRun and share the field with both of them.

We are still putting field by hand compared to PodSpec as PodSpec
has a large number of fields, including InitContainers and
Containers and we don't want people to be able to add random
containers to the Task pod.
This adds SecurityContext in addition to the current PodSpec
specific fields.

This is done in a backward compatibility way. The old field are still
there, but PodTemplate takes precedence over it (depending on which
field are present). These fields will be remove when we bump the API
to v1beta1.

Putting this as wip because :

is this the right approach ?
need to add more tests (current tests passes but do not exercice the "combining")
need to update docs

Closes #714

/cc @bobcatfish @dicarlo2 @chmouel

Signed-off-by: Vincent Demeester vdemeest@redhat.com

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.

Release Notes

Adds a PodTemplate field to PipelineRun and TaskRun and deprecates Affinity, Tolerations and NodeSelector on those.

tekton-robot · 2019-06-24T15:18:05Z

@vdemeester: GitHub didn't allow me to request PR reviews from the following users: dicarlo2.

Note that only tektoncd members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Changes

This allows to group and add PodSpec customizations to PipelineRun
and TaskRun and share the field with both of them.

We are still putting field by hand compared to PodSpec as PodSpec
has a large number of fields, including InitContainers and
Containers and we don't want people to be able to add random
containers to the Task pod.

This adds SecurityContext in addition to the current PodSpec
specific fields.

This is done in a backward compatibility way. The old field are still
there, but PodTemplate takes precedence over it (depending on which
field are present). These fields will be remove when we bump the API
to v1beta1.

Putting this as wip because :

is this the right approach ?

need to add more tests (current tests passes but do not exercice the "combining")

need to update docs

/cc @bobcatfish @dicarlo2

Signed-off-by: Vincent Demeester vdemeest@redhat.com

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.

Release Notes
Adds a PodTemplate field to PipelineRun and TaskRun and deprecates Affinity, Tolerations and NodeSelector on those.

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.

tekton-robot · 2019-06-24T15:18:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

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:

~~OWNERS~~ [vdemeester]

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

vdemeester · 2019-06-25T14:01:39Z

/test pull-tekton-pipeline-go-coverage

a-roberts · 2019-06-26T16:30:39Z

Vincent mentioned this on the working group call today looking for feedback, I think this is a great idea, we've used pod templates here before and I'm a fan.

Node affinity and PSPs are useful to be able to support at this layer (e.g. for supporting other platforms and running in more locked down environments).

I'd expect to see docs and potentially an example as the next logical step and I'll be happy to take a look. Are there any really specific use cases we're thinking of as well e.g. running with and without PSPs to cover? How about a runAsNonRoot example?

When you mention a test for combining, that's just to make sure this podTemplate gets precedence right? And that we're OK for being backwards compatible?

pkg/apis/pipeline/v1alpha1/pod.go

bobcatfish

Your approach looks good to me! I agree with how you have only exposed a subset of the Pod fields - especially since it would be easier to add more fields later v.s. remove fields we discover are problematic :)

Before we merge this I hope we can add some docs and tests 😇 🙏 😅 (which I think you were planning to do anyway!!)

This is done in a backward compatibility way.

could you create an issue in the backlog to make sure we make the backwards incompatible change in a later release? (unless you did already! ❤️ )

bobcatfish · 2019-07-08T14:11:00Z

pkg/apis/pipeline/v1alpha1/pod.go

@@ -0,0 +1,56 @@
+/*
+Copyright 2018 The Tekton Authors.


googlebot · 2019-07-09T13:09:53Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-07-09T14:22:04Z

☹️ Sorry, but only Googlers may change the label cla: yes.

googlebot · 2019-07-10T09:04:16Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

vdemeester · 2019-07-10T09:04:50Z

@bobcatfish added some documentation and on test (I need to add some e2e tests also). And the CLA is all good now 🎉

vdemeester · 2019-07-10T15:21:38Z

/hold

bobcatfish

Docs are looking great! I'm a bit confused about one of the unit tests, but assuming I'm just missing something (tho in that case maybe a comment would help? 😇 ):

/lgtm
/meow space

examples/taskruns/task-volume.yaml

bobcatfish · 2019-07-11T12:19:26Z

pkg/apis/pipeline/v1alpha1/pod_test.go

+			PodAffinity: &corev1.PodAffinity{},
+		},
+	}
+	// Same as template above


i'm a bit confused! wouldn't we want "banana": "chocolat", etc. to show up in the expected (got) template?

I should change the names of the variable to make it clearer, good point. So affinity, nodeSelector and tolerations are coming from the deprecated fields, so PodTemplate takes precedence over them. I choose to not merge NodSelector and Tolerations to make it simpler (especially as those deprecated field are going away rather shortly anyway) ; that's why "banana": "chocolat" is not in the got template 👼

OOOOOOOOH okay i get it, the point is to override them - maybe we should include a deprecated field we don't override as well in one of the tests? (to make sure they can be combined - unless it's supposed to be all or nothing, all depcreated or all new)

Ah right, I see what you mean, yeah I'll update the tests

bobcatfish · 2019-07-11T12:20:03Z

pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go

-			Affinity:       pr.Spec.Affinity,
-		},
-	}
+			PodTemplate:    podTemplate,


tekton-robot · 2019-07-11T12:20:42Z

@bobcatfish:

In response to this:

Docs are looking great! I'm a bit confused about one of the unit tests, but assuming I'm just missing something (tho in that case maybe a comment would help? 😇 ):

/lgtm
/meow space

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.

This allows to group and add `PodSpec` customizations to PipelineRun and TaskRun and share the field with both of them. - We are still putting field by hand compared to PodSpec as PodSpec has a large number of fields, including `InitContainers` and `Containers` and we don't want people to be able to add random containers to the `Task` pod. - This adds SecurityContext in addition to the current `PodSpec` specific fields. - This adds Volumes in addition to the current `PodSpec` specific fields. This is done in a backward compatibility way. The old field are still there, but `PodTemplate` takes precedence over it (depending on which field are present). These fields will be remove when we bump the API to `v1beta1`. Signed-off-by: Vincent Demeester <vdemeest@redhat.com>

bobcatfish

awesome possum!

🎉

/lgtm

bobcatfish · 2019-07-11T21:02:29Z

pkg/apis/pipeline/v1alpha1/pod_test.go

+	// Same as template above
+	want := v1alpha1.PodTemplate{
+		NodeSelector: map[string]string{
+			"banana": "chocolat",


bobcatfish · 2019-07-11T21:02:44Z

pkg/apis/pipeline/v1alpha1/pod_test.go

+	corev1 "k8s.io/api/core/v1"
+)
+
+func TestCombinedPodTemplateTakesPrecedence(t *testing.T) {


vdemeester · 2019-07-12T06:51:22Z

/hold cancel

tekton-robot requested a review from bobcatfish June 24, 2019 15:18

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 24, 2019

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Jun 24, 2019

tekton-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 24, 2019

vdemeester force-pushed the 714-carry-podtemplate branch from 5766738 to 0bd16ce Compare June 24, 2019 15:42

vdemeester mentioned this pull request Jun 25, 2019

Rename containerTemplate to stepTemplate 👠 #931

Merged

3 tasks

vdemeester added this to the Pipelines 0.5 🐱 milestone Jun 27, 2019

vdemeester force-pushed the 714-carry-podtemplate branch 2 times, most recently from ed47f87 to 6b5c228 Compare June 27, 2019 14:58

bobcatfish changed the title ~~wip: Add a PodTemplate field for PipelineRun and TaskRun~~ Add a PodTemplate field for PipelineRun and TaskRun Jul 3, 2019

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 3, 2019

vdemeester mentioned this pull request Jul 4, 2019

Update buildpacks-v3 template for latest lifecycle tektoncd/catalog#50

Merged

2 tasks

vdemeester commented Jul 4, 2019

View reviewed changes

pkg/apis/pipeline/v1alpha1/pod.go Show resolved Hide resolved

bobcatfish reviewed Jul 8, 2019

View reviewed changes

vdemeester force-pushed the 714-carry-podtemplate branch 3 times, most recently from c50040e to 9fb8275 Compare July 9, 2019 13:09

googlebot added cla: no and removed cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit labels Jul 9, 2019

vdemeester force-pushed the 714-carry-podtemplate branch from 9fb8275 to 00bff40 Compare July 9, 2019 13:51

vdemeester added cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit and removed cla: no labels Jul 9, 2019

googlebot removed the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Jul 9, 2019

vdemeester force-pushed the 714-carry-podtemplate branch from 00bff40 to 0a834d3 Compare July 10, 2019 09:04

googlebot added cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit and removed cla: no labels Jul 10, 2019

vdemeester modified the milestones: Pipelines 0.5 🐱, Pipelines 0.6 🐱 Jul 10, 2019

vdemeester force-pushed the 714-carry-podtemplate branch from 0a834d3 to 4a7cb8a Compare July 10, 2019 12:21

bobcatfish mentioned this pull request Jul 10, 2019

Add SecurityContext to TaskSpec #714

Closed

3 tasks

vdemeester force-pushed the 714-carry-podtemplate branch 3 times, most recently from 9e49bc0 to 5b20131 Compare July 10, 2019 14:19

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 10, 2019

tekton-robot assigned bobcatfish Jul 11, 2019

bobcatfish reviewed Jul 11, 2019

View reviewed changes

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 11, 2019

vdemeester force-pushed the 714-carry-podtemplate branch from 5b20131 to 2eb4a63 Compare July 11, 2019 13:07

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 11, 2019

vdemeester force-pushed the 714-carry-podtemplate branch from 2eb4a63 to 69b3e7e Compare July 11, 2019 13:16

bobcatfish reviewed Jul 11, 2019

View reviewed changes

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 11, 2019

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 12, 2019

tekton-robot merged commit 4873422 into tektoncd:master Jul 12, 2019

vdemeester deleted the 714-carry-podtemplate branch July 12, 2019 07:15

vdemeester mentioned this pull request Jul 12, 2019

Remove PodSpec deprecated field in favor of PodTemplate #1070

Closed

bobcatfish mentioned this pull request Aug 20, 2019

Create docs on how to make backward compatible changes #1221

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a PodTemplate field for PipelineRun and TaskRun #1004

Add a PodTemplate field for PipelineRun and TaskRun #1004

vdemeester commented Jun 24, 2019 •

edited

Loading

tekton-robot commented Jun 24, 2019

Changes

Submitter Checklist

Release Notes

tekton-robot commented Jun 24, 2019

vdemeester commented Jun 25, 2019

a-roberts commented Jun 26, 2019 •

edited

Loading

bobcatfish left a comment

bobcatfish Jul 8, 2019

vdemeester Jul 8, 2019

googlebot commented Jul 9, 2019

googlebot commented Jul 9, 2019

googlebot commented Jul 10, 2019

vdemeester commented Jul 10, 2019

vdemeester commented Jul 10, 2019

bobcatfish left a comment

bobcatfish Jul 11, 2019

vdemeester Jul 11, 2019

bobcatfish Jul 11, 2019

vdemeester Jul 11, 2019

bobcatfish Jul 11, 2019

tekton-robot commented Jul 11, 2019

bobcatfish left a comment

bobcatfish Jul 11, 2019

bobcatfish Jul 11, 2019

vdemeester commented Jul 12, 2019

Add a PodTemplate field for PipelineRun and TaskRun #1004

Add a PodTemplate field for PipelineRun and TaskRun #1004

Conversation

vdemeester commented Jun 24, 2019 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Jun 24, 2019

Changes

Submitter Checklist

Release Notes

tekton-robot commented Jun 24, 2019

vdemeester commented Jun 25, 2019

a-roberts commented Jun 26, 2019 • edited Loading

bobcatfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Jul 9, 2019

googlebot commented Jul 9, 2019

googlebot commented Jul 10, 2019

vdemeester commented Jul 10, 2019

vdemeester commented Jul 10, 2019

bobcatfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Jul 11, 2019

bobcatfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdemeester commented Jul 12, 2019

vdemeester commented Jun 24, 2019 •

edited

Loading

a-roberts commented Jun 26, 2019 •

edited

Loading