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 default pod template support #1541

Closed
wants to merge 3 commits into from
Closed

Add default pod template support #1541

wants to merge 3 commits into from

Conversation

eddycharly
Copy link
Member

@eddycharly eddycharly commented Nov 8, 2019

This PR adds support for a default pod template through the default ConfigMap.

This is useful when one wants to apply a pod template to all task and pipeline runs, for example specifying a node selector, a security policy, etc...

When a pod template is specified for a PipelineRun or TaskRun it takes precedence over the default pod template.
Therefore, the default pod template only applies to PipelineRun or TaskRun that don't have a pod template.

Submitter Checklist

@tekton-robot 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 Nov 8, 2019
@googlebot googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Nov 8, 2019
@tekton-robot tekton-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 8, 2019
@tekton-robot
Copy link
Collaborator

Hi @eddycharly. 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.

@vdemeester
Copy link
Member

Yet to be defined : what behavior should we implement when a pod template and a default pod template are defined ? Merge both ? Pod template overrides completely the default one ? How can we bypass the default pod template if wanted (explicitly specifying null clears out the default pod template) ?

We should stick to what we do for the rest, aka, a Task step containerspec takes over the same field the stepTemplate field. In that case, a user using podTemplate in his TaskRun would take over the field from the "default" config podTemplate. And as for stepTemplate we would merge the fields.

@eddycharly
Copy link
Member Author

eddycharly commented Nov 8, 2019

@vdemeester thanks for the feedback. i agree that sticking to what is already done is good for consistency.

Trying to build right now, my changes introduce a cyclic dependency as config needs v1alpha1 (for PodTemplate struct), and v1alpha1 needs config (in pipelinerun_defaults.go for example).

How would you recommend solving this dependency issue ?

Should i create an interface ? A package to hold structs like PodTemplate ?

@vdemeester
Copy link
Member

Trying to build right now, my changes introduce a cyclic dependency as config needs v1alpha1 (for PodTemplate struct), and v1alpha1 needs config (in pipelinerun_defaults.go for example).

How would you recommend solving this dependency issue ?

Should i create an interface ? A package to hold structs like PodTemplate ?

I think we have a little bit time to think about that. I would want to wait for other feedback on this feature before tackling this issue.

That said, on top of my head, we can extract the sturct into a package and use type aliasing to not have the cyclic dependency. I might take this approach for #1526 (more or less).

@eddycharly
Copy link
Member Author

Just trying to figure out what would have to be done to implement it... in case it is accepted ;)

Still waiting for feedback/discussion of course.

@ghost
Copy link

ghost commented Dec 18, 2019

what behavior should we implement when a pod template and a default pod template are defined ? Merge both ?

I would suggest not trying to perform a merge. My preference would be that the pod gets either the one specified on the TaskRun or the default if not specified, not an amalgamation of both.

@eddycharly
Copy link
Member Author

@sbwsg @vdemeester should i go ahead with this or do you think it's useless ?

@vdemeester
Copy link
Member

@eddycharly I think it make sense 👼 and I think, as a start I do like @sbwsg suggestion. If there is no PodTemplate on a TaskRun, let's use the one from the default (if any), and if there is a PodTemplate, it overrides them all 👼

@vdemeester
Copy link
Member

/ok-to-test

@tekton-robot tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 9, 2020
@tekton-robot
Copy link
Collaborator

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/config/default.go 81.8% 62.5% -19.3

@eddycharly
Copy link
Member Author

@vdemeester to resolve the cyclic reference issue i had with pod.go, i moved it in pkg/apis/pipeline instead of pkg/apis/pipeline/v1alpha1.
Do you have a recommandation for the package where it should be or pipeline is fine ?

@tekton-robot tekton-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 9, 2020
@tekton-robot
Copy link
Collaborator

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/config/default.go 81.8% 62.5% -19.3

@vdemeester
Copy link
Member

@vdemeester to resolve the cyclic reference issue i had with pod.go, i moved it in pkg/apis/pipeline instead of pkg/apis/pipeline/v1alpha1.
Do you have a recommandation for the package where it should be or pipeline is fine ?

hum 🤔 why is there a cyclic reference issue ?

@eddycharly
Copy link
Member Author

It introduces a dependency from config to v1alpha1 (to deserialize the pod template struct from the config map), but v1alpha1 already depends on config...

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It introduces a dependency from config to v1alpha1 (to deserialize the pod template struct from the config map), but v1alpha1 already depends on config...

ah dang… then we need to at least use type aliasing to make sure we don't break downstream go user of tektoncd/pipeline (like dashboard, cli, …).

I would prefer a package like pod for moving the pod.go code, so something like pkg/apis/pipeline/pod/ with a template.go file in there ?

And in v1alpha1/pod.go having,

// […]

package v1alpha1

import (
	// […]
	"github.com/tektoncd/pipeline/pkg/apis/pipeline/pod"
	// […]
)

// […]

type PodTemplate = pod.PodTemplate

wdyt ?

@tekton-robot tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 9, 2020
@tekton-robot
Copy link
Collaborator

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/config/default.go 81.8% 62.5% -19.3

@tekton-robot
Copy link
Collaborator

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/config/default.go 81.8% 100.0% 18.2
test/builder/pipeline.go 87.9% 87.0% -0.9
test/builder/task.go 82.3% 81.1% -1.1

fixed review comments


use ghodss/yaml


go mod tidy
@tekton-robot
Copy link
Collaborator

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/config/default.go 81.8% 90.0% 8.2
test/builder/pipeline.go 87.9% 87.0% -0.9
test/builder/task.go 82.3% 81.1% -1.1

@eddycharly
Copy link
Member Author

Outch.. conficts :(
Can i resolve conflicts and someone merge the PR please ? @vdemeester @bobcatfish

@bobcatfish
Copy link
Collaborator

Thanks for keeping at this @eddycharly !

To resolve the conflicts you'll need to rebase your branch onto the latest version of master and then force push, then we can lgtm and merge. Let us know if you want us to point you in the right direction to do that!

@tekton-robot
Copy link
Collaborator

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/config/default.go 84.6% 90.9% 6.3
test/builder/pipeline.go 88.2% 87.3% -0.9
test/builder/task.go 82.3% 81.1% -1.1

@tekton-robot
Copy link
Collaborator

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/config/default.go 84.6% 90.9% 6.3
test/builder/pipeline.go 88.2% 87.3% -0.9
test/builder/task.go 82.3% 81.1% -1.1

@eddycharly
Copy link
Member Author

/test pull-tekton-pipeline-integration-tests

@eddycharly
Copy link
Member Author

@bobcatfish i resolved the conflicts.

@bobcatfish
Copy link
Collaborator

Something went wrong: failed to prepare test environment: --provider=gke boskos failed to acquire project: resources not found

This is happening over in #1888 as well, re-opened tektoncd/plumbing#29

@eddycharly
Copy link
Member Author

boskos seems to be out of resources :(

@eddycharly
Copy link
Member Author

/test pull-tekton-pipeline-integration-tests

@bobcatfish
Copy link
Collaborator

Sorry about this @eddycharly trying to figure out what's going on 😭

@eddycharly
Copy link
Member Author

Thanks for your support @bobcatfish !
Good luck, i will retry later, better let boskos alone for now ;)

@eddycharly
Copy link
Member Author

/test pull-tekton-pipeline-integration-tests

1 similar comment
@bobcatfish
Copy link
Collaborator

/test pull-tekton-pipeline-integration-tests

@eddycharly
Copy link
Member Author

/test pull-tekton-pipeline-integration-tests

1 similar comment
@eddycharly
Copy link
Member Author

/test pull-tekton-pipeline-integration-tests

@eddycharly
Copy link
Member Author

@bobcatfish all tests are passing now. Can you give a look please ?

@vdemeester
Copy link
Member

@eddycharly can you squash your commits ?

fix taskrun tests
@tekton-robot
Copy link
Collaborator

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/config/default.go 84.6% 90.9% 6.3

@eddycharly
Copy link
Member Author

/test tekton-pipeline-unit-tests

@tekton-robot
Copy link
Collaborator

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

Test name Commit Details Rerun command
pull-tekton-pipeline-unit-tests 38a0894 link /test tekton-pipeline-unit-tests
pull-tekton-pipeline-build-tests 38a0894 link /test pull-tekton-pipeline-build-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.

@eddycharly
Copy link
Member Author

@vdemeester any idea why unit tests are failing ?
I0120 11:06:38.619] 2020-01-20T11:06:37.193Z ERROR TestReconcile/success-with-cluster-task taskrun/controller.go:59 Failed to create taskrun metrics recorder running_taskruns_count: cannot register view "running_taskruns_count"; a different view with the same name is already registered

@vdemeester
Copy link
Member

@eddycharly interesting… I thought we fixed that test… There might be a slight race in there still… 🤔

@eddycharly eddycharly closed this Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants