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

Sidecar kubelet implementation #80744

Open
wants to merge 13 commits into
base: master
from

Conversation

@Joseph-Irving
Copy link
Contributor

Joseph-Irving commented Jul 30, 2019

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR adds the functionality for the Sidecars KEP kubernetes/enhancements#753

NOTE: This PR contains two commits from #79649 which implements the API making it seem a lot larger, please ignore the first two commits for this PR.

I've separated the commits to each implement a different piece of functionality which I hope makes it easier to review, I've added some explanation in the commit messages to help explain some of my thought process.

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Containers can now be marked as having `lifecycle.type = "Sidecar"`, these containers are started before regular containers and terminated after. When `RestartPolicy!=Always` sidecars are terminated when all non-sidecars have exited. 

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP] https://github.com/kubernetes/enhancements/blob/master/keps/sig-apps/sidecarcontainers.md
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jul 30, 2019

Hi @Joseph-Irving. 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.

@Joseph-Irving

This comment has been minimized.

Copy link
Contributor Author

Joseph-Irving commented Jul 30, 2019

/kind feature

@Joseph-Irving

This comment has been minimized.

Copy link
Contributor Author

Joseph-Irving commented Jul 30, 2019

/hold

Copy link
Contributor

mattjmcnaughton left a comment

Hi @Joseph-Irving - cool stuff!

Does the "/ hold" mean that you'd like reviewers to wait on reviewing this?

If so, do you mind updating the title of the diff to start w/ [WIP]? Just makes it easier for organizing sig-node prs :)

Thanks!

@Joseph-Irving

This comment has been minimized.

Copy link
Contributor Author

Joseph-Irving commented Jul 30, 2019

@mattjmcnaughton no the hold is due to the fact it has a dependent PR so this should not be merged before that, api PR (#79649) I would like people to start reviewing this.

Copy link
Member

draveness left a comment

/ok-to-test
/priority important-soon
/cc @draveness

@Joseph-Irving Joseph-Irving force-pushed the Joseph-Irving:sidecar_implementation branch from b79101c to 8a71aa9 Aug 1, 2019
- Rename sidecarStatus.Containers waiting to NonSidecarContainersWaiting
- Rename getSidecarIDs to getSidecarIndexes
- isSidecar takes pointer to container
- Add comment clafirfying how sidecars block non sidecars starting
during startup
- Log sidecar name when triggering shutdown
By omitting them counting towards running the kubelet would not update 
their container status when they shutdown as it thought the pod had 
finished
@Joseph-Irving Joseph-Irving force-pushed the Joseph-Irving:sidecar_implementation branch from 48d208f to 9c42039 Mar 5, 2020
@Joseph-Irving

This comment has been minimized.

Copy link
Contributor Author

Joseph-Irving commented Mar 5, 2020

big thanks to @dims, @thockin, @kow3ns for helping out. I have rebased now

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Mar 5, 2020

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@dims

This comment has been minimized.

Copy link
Member

dims commented Mar 5, 2020

/test pull-kubernetes-e2e-gce-alpha-features

@dims

This comment has been minimized.

Copy link
Member

dims commented Mar 5, 2020

/assign @sjenning

@mattjmcnaughton

This comment has been minimized.

Copy link
Contributor

mattjmcnaughton commented Mar 5, 2020

For my own book keeping, tracking the different components to review here. I'll try and keep this up to date.

* [x]  Shutdown triggering for sidecars when RestartPolicy != Always

* [x]  Pre-stop hooks sent to sidecars before non sidecar containers

* [x]  Sidecars terminated after normal containers

* [x]  Sidecars start before normal containers

* [x]  Sidecars not included in pod phase calculations

* [x]  General design, etc...

@Joseph-Irving any big components I'm missing?

@Joseph-Irving ok, I consider this diff reviewed on my end. I'm convinced the overall design fulfills what you outlined in the KEP. I'm also convinced that this code has been feature flagged in a way that it won't have adverse impacts when the feature is turned off. We should resolve some of the code duplication when we promote to beta/GA, but I agree that its acceptable (or maybe even preferable) during the alpha period. I defer to the other folks commenting around the specifics of some of the edge cases, although again, my humble suggestiong would be that we are comfortable resolving some of these smaller edge cases in the alpha in a series of smaller diffs after the larger diff is merged (again, provided the edge cases won't cause negative impacts when the sidecar feature is turned off).

@Joseph-Irving for your own bookkeeping, what would you like me to mark this diff? I can mark lgtm and approve? I'm not sure if that will give the others the mistaken impression that this diff is totally wrapped up... I'm just speaking as a sig-node reviewer :)

Please let me know if there's anything else I can do to help and thank you for your patience :)

Translating my review into marking lgtm re request from @dims

Confirmed with @Joseph-Irving that nothing has changed since I reviewed outside of adding new test cases

Thanks again for @dims and @Joseph-Irving

/lgtm

@derekwaynecarr

This comment has been minimized.

Copy link
Member

derekwaynecarr commented Mar 5, 2020

Changes to the pod life-cycle are hard to review. While I understand the motivations for the feature (as well as the concerns), please remember that a number of reviewers have been trying to ensure the existing life-cycle works as described.

See:
#88592

That resulted in a number of us working for hours to chase down root causes that resulted in merges for the following to ensure our existing lifecycle patterns work:
#88440
#88591

For this particular PR, I am having to find every reference to init containers or containers and ask "what about a side car container"?. That includes code that is not present in this PR. As the author, I trust you have already done this, but keep in mind that as the reviewer, it takes time as well and we can get distracted chasing other challenges.

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Mar 5, 2020

Given we have existing issues with container lifecycle in the kubelet (e.g. #88766), is adding new modes to the same code area before understanding/resolving them a good idea? I think the existing container lifecycle support needs to be rock solid before expanding the surface area. That's more of a statement about the current state of the kubelet than about this PR or feature.

@smarterclayton

This comment has been minimized.

Copy link
Contributor

smarterclayton commented Mar 6, 2020

I just spent a couple of weeks in the Kubelet, and it’s dark and scary in there. As valuable as a feature like this can be, I share Jordan’s concern. We’re undercovered in a lot of significant ways in review time, test coverage, and complexity. We definitely will want a long alpha period for this and ephemeral containers, but I would prefer to start that in 1.19 with a commitment to more upfront work on review than post facto digging out.

@dims

This comment has been minimized.

Copy link
Member

dims commented Mar 6, 2020

@smarterclayton @liggitt @derekwaynecarr fair enough! i wanted to make sure we give this a fair shot for 1.18 and looks like we have to move this out. Thanks for taking time, i know this was a crazy day like all code freeze days.

/milestone v1.19

@k8s-ci-robot k8s-ci-robot modified the milestones: v1.18, v1.19 Mar 6, 2020
@dims

This comment has been minimized.

Copy link
Member

dims commented Mar 6, 2020

@derekwaynecarr @liggitt @sjenning all of you had mentioned variations of "existing container lifecycle support needs to be rock solid", can we please log an issue or something to figure out what we need to do there? So we know what to shoot for in 1.19 in addition to supporting this use case.

thanks,
Dims

@irvifa

This comment has been minimized.

Copy link
Member

irvifa commented Mar 6, 2020

@Joseph-Irving seems like this PR needs another rebase I saw there's still many conflict for this branch..

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Mar 6, 2020

@Joseph-Irving: PR needs rebase.

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.

@dims

This comment has been minimized.

Copy link
Member

dims commented Mar 6, 2020

@irvifa let's revisit this after code freeze, this is now marked for 1.19

@thockin

This comment has been minimized.

Copy link
Member

thockin commented Mar 10, 2020

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.