-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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 container template configuration to Task #767
Conversation
/assign @dwnusbaum |
@abayer linking @bobcatfish comment here too #714 (comment)
This applies too 👼 |
@vdemeester Yeah, this was just me resurrecting my old PR that did |
@bobcatfish @vdemeester re that - I'm thinking of maybe adding a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR is looking great!! 🎉 Just a few minor comments
@bobcatfish @vdemeester re that - I'm thinking of maybe adding a BaseContainer field to TaskSpec, with the field just being a *corev1.Container, which is then used as an initial container that steps are merged onto. I've got similar logic in Jenkins X that's working great for us.
I'm leaning toward this solution as well but I want to get some more opinions before we go ahead with it maybe @dlorenc @imjasonh @vdemeester I'm interested in what you think about this.
@abayer do you have other corev1.Container fields specifically that you'd find it handy to be able to declare at the Task level?
And I'm happy to either:
- Put this PR on hold while we discuss adding a container spec OR
- Merge this PR and iterate (would mean adding backwards incompatible changes if we decide to add the container spec tho since we'd remove
env
in that case)
- name: "FOO" | ||
value: "baz" | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wooooot great docs 🙇♀️ :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Albeit ones I now have to rewrite...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and the new ones are great too!!
@@ -524,6 +533,38 @@ func createRedirectedTaskSpec(kubeclient kubernetes.Interface, ts *v1alpha1.Task | |||
return ts, nil | |||
} | |||
|
|||
// combineEnvVars takes a base slice of environment variables and a slice of environment | |||
// variables to overlay on top of the base slice, returning the result. | |||
func combineEnvVars(base []corev1.EnvVar, override []corev1.EnvVar) []corev1.EnvVar { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like a great interface! i think this is a good candidate for moving into its own package with its own unit tests 😜 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welp, I switched to a new function. Which I will move etc. =)
envVars = append(envVars, k) | ||
} | ||
|
||
// Avoid nondeterministic results by sorting the keys and appending vars in that order. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
step.Env = combineEnvVars(ts.Env, step.Env) | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this code is solid, but I'm not sure createRedirectedTaskSpec
is the place to put it because I think that createRedirectedTaskSpec
is specifically about adding our magic entrypoint override business, and manipulating the environment variables doesn't seem like part of that to me - maybe we could move this up into createPod
? (open to other ideas too!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which is exactly what I did in the latest version. =)
@@ -82,6 +82,13 @@ var ( | |||
|
|||
saTask = tb.Task("test-with-sa", "foo", tb.TaskSpec(tb.Step("sa-step", "foo", tb.Command("/mycmd")))) | |||
|
|||
taskEnvTask = tb.Task("test-task-env", "foo", tb.TaskSpec( | |||
tb.TaskEnvVar("FRUIT", "APPLE"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe include one we don't override as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest iteration shows this working right - it shows up in the init containers and the nop container.
/hold @bobcatfish @vdemeester Modified to add |
3e1f910
to
2389eb0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good (except the docs of course which I can totally understand your hesitance to update XD)! And I think we should tackle the pod spec separately with maybe a bit more deliberation.
I'd like to hear from a couple of @vdemeester @imjasonh @dlorenc on their thoughts on this before we go ahead with the container spec but I'd say 👍 from me.
|
||
// PodSpec can be used to specify advanced configuration for the pod used | ||
// by the Task. | ||
PodSpec *corev1.PodSpec `json:"podSpec,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
id prefer we start with just the BaseContainer
for now and do the pod spec separately - specifically b/c if we have the pod spec we'll probably want to remove some attributes (e.g. affinity
etc.) at the same time (and it seems like we want the pod spec to be a runtime thing... AND i wonder if we actually want it in the run, or referred to by the run... AND @chmouel has some interesting thoughts on that #714 (comment) long story short i think we should do a bit of design re. the pod spec before going for it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good!
|
||
// BaseContainer can be used as the basis for all step containers within the | ||
// Task, so that the steps inherit settings on the base container. | ||
BaseContainer *corev1.Container `json:"baseContainer,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vdemeester pointed out the similarity between what we're doing here and deployment.template
and job.template
so I think it could be good to try to stick to a similar naming scheme, maybe something like containerTemplate
or stepTemplate
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing.
@@ -229,7 +244,29 @@ func MakePod(taskRun *v1alpha1.TaskRun, taskSpec v1alpha1.TaskSpec, kubeclient k | |||
|
|||
nopContainer := &corev1.Container{Name: "nop", Image: *nopImage, Command: []string{"/ko-app/nop"}} | |||
entrypoint.RedirectStep(cache, len(podContainers), nopContainer, kubeclient, taskRun, logger) | |||
podContainers = append(podContainers, *nopContainer) | |||
mergedStep, err := mergeStepWithBaseContainer(baseContainer, nopContainer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, instead of calling mergeStepWithBaseContainer
several times, is there a way we can call it with all the steps together once we have them? (maybe on podContainers
below?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, can do
return nil, err | ||
} | ||
|
||
mergedData, err := strategicpatch.StrategicMergePatchUsingLookupPatchMeta(baseData, patch, schema) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would you mind adding some comments before each of the strategicpatch
calls elaborating on what's going on (from my ignorant-of-strategic-merge point of view it seems weird that there are 3 different calls required here and im not sure about details like "why do we need 'using lookup patch meta'"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...yeah, I’ll be honest, it’s a bit black magic to me - I just kept hitting things until I got the result I needed. =) But I got there right before wrapping up for the day, so I’ll add explanatory comments tomorrow.
}, nil | ||
} | ||
|
||
|
||
func mergeStepWithBaseContainer(base, override *corev1.Container) (*corev1.Container, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could have this in it's own package with unit tests :D
(No worries if you want to wait to till we decide if we want to go forward with this impl vs the previous one)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
=)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I'm thinking of it, a question - what's the motivation for putting functions like this in their own packages, rather than just unit testing them in place? Just trying to understand. =)
/hold cancel Ok, I think we're good now, so I'm removing the hold. |
04b8078
to
d433124
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few comments — the merging piece is interesting… is it how deployment
& co do it ?
@@ -23,6 +23,7 @@ import ( | |||
"encoding/hex" | |||
"flag" | |||
"fmt" | |||
"github.com/tektoncd/pipeline/pkg/merge" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: misplaced import 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stupid IDE!
@@ -170,17 +171,19 @@ func MakePod(taskRun *v1alpha1.TaskRun, taskSpec v1alpha1.TaskSpec, kubeclient k | |||
} | |||
annotations["sidecar.istio.io/inject"] = "false" | |||
|
|||
baseContainer := taskSpec.ContainerTemplate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: s/baseContainer/containerTemplate/g
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixing!
@vdemeester This is basically how |
Container configuration defined on the `Task` will be propagated to all steps within the `Task`, and can be overridden on individual steps.
Derived from what I've done over at tektoncd/pipeline#767 - this is just cleaner, simpler, better.
Derived from what I've done over at tektoncd/pipeline#767 - this is just cleaner, simpler, better.
Derived from what I've done over at tektoncd/pipeline#767 - this is just cleaner, simpler, better.
Derived from what I've done over at tektoncd/pipeline#767 - this is just cleaner, simpler, better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks awesome @abayer ! For once in my life I have nothing to nit, it all looks great :D
p.s. I also spoke briefly with @dlorenc and @imjasonh offline and they're on board with this approach.
Thanks again for this (so quickly!) and also thanks for your patience re. my original reluctance to add this feature 🙇♀️
/approve
/lgtm
/meow space
@@ -72,7 +73,9 @@ following fields: | |||
- [`outputs`](#outputs) - Specifies [`PipelineResources`](resources.md) needed | |||
by your `Task` | |||
- [`volumes`](#volumes) - Specifies one or more volumes that you want to make | |||
available to your build. | |||
available to your `Task`'s steps. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops, thanks for fixing! ❤️
- name: "FOO" | ||
value: "baz" | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and the new ones are great too!!
return nil, err | ||
} | ||
|
||
// Actually apply the merge patch to the template JSON. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks so much for these detailed comments, I think it really clears up what's going on! 🙏 🎉
In response to this:
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. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abayer, bobcatfish 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:
Approvers can indicate their approval by writing |
/test pull-tekton-pipeline-integration-tests |
@abayer has contributed features such as step templating (tektoncd#767) and Pipeline level params (tektoncd#471), as well as many general tweaks and fixes, and 40+ reviews! Not to mention that since he works on Jenkins X (built on Tekton) he has invaluable feedback from the perspective of someone actively building on top of Tekton! Let it be noted forever that @bobcatfish should have gotten around to doing this ages ago and she very much appreciates @abayer's patience 🙏
@abayer has contributed features such as step templating (#767) and Pipeline level params (#471), as well as many general tweaks and fixes, and 40+ reviews! Not to mention that since he works on Jenkins X (built on Tekton) he has invaluable feedback from the perspective of someone actively building on top of Tekton! Let it be noted forever that @bobcatfish should have gotten around to doing this ages ago and she very much appreciates @abayer's patience 🙏
Changes
The container template defined on the
Task
will be propagated to allsteps within the
Task
, and can be overridden on individual steps.fixes #254
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide
for more details.
Release Notes