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

PodPreset: Add same functionality for init containers as standard containers #71479

Merged
merged 3 commits into from Mar 1, 2019

Conversation

@soggiest
Copy link
Contributor

soggiest commented Nov 27, 2018

What type of PR is this?
/kind feature

What this PR does / why we need it:
Adds the same functionality for init containers as standard containers in the PodPreset admission controller.

Which issue(s) this PR fixes:
Fixes #55410

Does this PR introduce a user-facing change?:

Adds the same information to an init container as a standard container in a pod when using PodPresets.
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 27, 2018

Hi @soggiest. Thanks for your PR.

I'm waiting for a kubernetes 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.

@k8s-ci-robot k8s-ci-robot requested review from deads2k and derekwaynecarr Nov 27, 2018

@dims

This comment has been minimized.

Copy link
Member

dims commented Nov 27, 2018

/ok-to-test

@soggiest soggiest referenced this pull request Nov 27, 2018

Closed

New membership request: soggiest #272

6 of 6 tasks complete
@ncdc

This comment has been minimized.

Copy link
Member

ncdc commented Nov 28, 2018

@soggiest I would recommend updating test/e2e/servicecatalog/podpreset.go to include verifying the init containers were modified as expected.

@soggiest

This comment has been minimized.

Copy link
Contributor Author

soggiest commented Nov 29, 2018

/retest

@soggiest

This comment has been minimized.

Copy link
Contributor Author

soggiest commented Nov 29, 2018

/sig service-catalog

@soggiest

This comment has been minimized.

Copy link
Contributor Author

soggiest commented Nov 29, 2018

/test all

@ncdc

This comment has been minimized.

Copy link
Member

ncdc commented Nov 29, 2018

FWIW, since the PodPreset e2es don't seem to run in CI that I can find, I built a kube-apiserver with the changes from this PR, finagled it into a Minikube environment, and I was able to manually run the PodPreset e2e tests (including removing the skip that it should only run on gce), and everything passed.

@oomichi
Copy link
Member

oomichi left a comment

just one comment

Show resolved Hide resolved test/e2e/servicecatalog/podpreset.go Outdated
@soggiest

This comment has been minimized.

Copy link
Contributor Author

soggiest commented Dec 4, 2018

/retest

{
Name: "init1",
Image: imageutils.GetE2EImage(imageutils.BusyBox),
Env: []v1.EnvVar{{Name: "abc", Value: "value2"}, {Name: "ABC", Value: "value"}},

This comment has been minimized.

@oomichi

oomichi Dec 5, 2018

Member

Sorry for missing this point on my previous review.
After reading the original issue(#55410) carefully, the reporter wanted to set the init container specific environment values on podPresets.
Current ones are the same as the container's one of line 218. So it is better to set different environment value as the e2e test to verify the behavior.

This comment has been minimized.

@soggiest

soggiest Dec 5, 2018

Author Contributor

I don't see where the original user wanted there to be different values for the init container vs the main container. The way that I've read it is that they want podpresets to function with init containers at all.

@soggiest

This comment has been minimized.

Copy link
Contributor Author

soggiest commented Dec 10, 2018

@kubernetes/sig-service-catalog-feature-requests
Can someone review this PR, please?

@mbarthelemy-grab

This comment has been minimized.

Copy link

mbarthelemy-grab commented Dec 18, 2018

Hi,
Will this get merged? Would be super useful for us (we need to be able to access PodPreset env vars from initContainers).

@ncdc

This comment has been minimized.

Copy link
Member

ncdc commented Feb 6, 2019

@jessfraz @dims @smarterclayton @timothysc @pmorie - anyone object to this going in?

@derekwaynecarr

This comment has been minimized.

Copy link
Member

derekwaynecarr commented Feb 6, 2019

@ncdc I thought we had decided against further evolution of pod preset in favor of webhooks out of tree, but it’s possible I missed something that changed.

@dims

This comment has been minimized.

Copy link
Member

dims commented Feb 6, 2019

/milestone v1.14
/sig node
/assign @derekwaynecarr @deads2k

Let's mark this as v1.14 so folks look at it and we can yank it out if it does not make it (as @derekwaynecarr mentions that a decision was made at some point)

@smarterclayton

This comment has been minimized.

Copy link
Contributor

smarterclayton commented Feb 6, 2019

We really should move forward with pod preset as a beta CRD and webhook in a kubernetes sig namespace. It’s a great feature, one that I think a lot of people want, and we’ve come so close. But according to the last state of this discussion, we did agree to move this out before we do much more work on it.

@ncdc

This comment has been minimized.

Copy link
Member

ncdc commented Feb 6, 2019

@derekwaynecarr @smarterclayton @pmorie I completely understand the motivation for moving this out of tree. I agree with it. However, the current in-tree feature only applies the presets to normal containers, and not init containers. It's only doing half the job it should be doing. I'm advocating that we complete the in-tree feature with this PR. Is that ok for now, and going forward, someone can look at externalizing this?

@ncdc

This comment has been minimized.

Copy link
Member

ncdc commented Feb 14, 2019

@soggiest

This comment has been minimized.

Copy link
Contributor Author

soggiest commented Feb 28, 2019

@smarterclayton @derekwaynecarr @pmorie with code freeze coming up can we get another look at this PR, please?

@dims

This comment has been minimized.

Copy link
Member

dims commented Feb 28, 2019

@ncdc i can buy that argument! so

/lgtm

@ncdc

This comment has been minimized.

Copy link
Member

ncdc commented Feb 28, 2019

@dims thank you thank you thank you!

@ncdc

This comment has been minimized.

Copy link
Member

ncdc commented Feb 28, 2019

/approve
(for the test bit)

@dims

This comment has been minimized.

Copy link
Member

dims commented Feb 28, 2019

need the approvers though! @ncdc :)

@smarterclayton

This comment has been minimized.

Copy link
Contributor

smarterclayton commented Feb 28, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 28, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ncdc, smarterclayton, soggiest

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 55a6576 into kubernetes:master Mar 1, 2019

18 checks passed

cla/linuxfoundation soggiest authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Context retired without replacement.
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Context retired without replacement.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.