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

kep-sidecar-containers #2148

Merged
merged 1 commit into from
Nov 29, 2018
Merged

Conversation

Joseph-Irving
Copy link
Member

@Joseph-Irving Joseph-Irving commented May 14, 2018

This is my initial thoughts on the concept of the sidecar container to address kubernetes/kubernetes#25908
Hopefully this can prompt some discussion around the issue as I think it's something that a fair amount of people would like to be addressed.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 14, 2018
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
  • If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: [email protected]

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.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label May 14, 2018
@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. labels May 14, 2018
@rabbitfang
Copy link

IMO, sidecar: true is a better choice than sidecarContainers:

sidecarContainers

  • All tools that iterate over pod containers would need to be updated to recognize that sidecarContainers contains containers that are run in the pod (e.g. dashboard has a list of containers for exec and logs, monitoring tools would need to be updated to also watch sidecarContainers.
  • I think sidecarContainers deviates from the API styling. initContainers are used because the containers have an entirely different lifecycle from regular containers (initContainers are run one at a time, in order, before any containers are run). Sidecars have basically the same lifecycle as regular containers; they just don't contribute to the pod's terminated state (i.e. the pod terminates when non-sidecar containers terminate).

sidecar: true

I think a daemon might be a better field name, analogous to daemon threads that don't prevent process termination, but this is a weak preference

  • The only tooling change that is required would be to core kubernetes components (API server and kubelet). Third party tools that do not care about the distinction won't need to be changed.
  • More inline with current API schema styling.
  • The schema would not prevent setting sidecar: true on init containers or setting making all containers sidecars, but this could be solved with validation.

@Joseph-Irving
Copy link
Member Author

@rabbitfang Yeah I'm totally open to that idea, I merely presented this as a StrawMan based of some discussion I had with some sig-apps people at kubecon.
My only concern would be if you had a lot sidecars it could be quite hard to read compared to the having a separate section in the Pod Spec, however this is a minor gripe and it sounds like it would be easier to implement.

I'll be interested if anyone else has any strong opinions on this

@jmillikin-stripe
Copy link

I have a pretty strong preference for having sidecar and non-sidecar containers listed in the same place, because aside from end-of-lifecycle behavior they are identical (e.g. consume resources, expose ports). Having a separate container list for sidecars means we'd need to update lots of tooling that needs to inspect Container values but (1) doesn't care if they're sidecars or not (authorizers) or (2) can gracefully degrade if unaware of the sidecar field (dashboards, reports).

@stp-ip
Copy link
Member

stp-ip commented May 15, 2018

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 15, 2018
@Joseph-Irving
Copy link
Member Author

ok thanks for the feedback, sounds like sidecar: true is more popular and easier to implement so I've update the proposal to use that.

@patrickf55places
Copy link

Should sig/node be brought in on this (since a significant part of the changes will be with kubelet)?

Copy link

@mhuxtable mhuxtable left a comment

Choose a reason for hiding this comment

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

I'm really keen on this proposal as we are currently using the common hacks to properly manage sidecars in jobs. Would be happy to help with implementation when we get to that point.

keps/sig-apps/0008-sidecarcontainer.md Outdated Show resolved Hide resolved
keps/sig-apps/0008-sidecarcontainer.md Outdated Show resolved Hide resolved
keps/sig-apps/0008-sidecarcontainer.md Outdated Show resolved Hide resolved
@Joseph-Irving Joseph-Irving force-pushed the master branch 2 times, most recently from 0f6ab63 to f61cbb5 Compare May 16, 2018 09:34
@Joseph-Irving
Copy link
Member Author

/sig node

@k8s-ci-robot k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels May 16, 2018
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 27, 2018
@Joseph-Irving
Copy link
Member Author

@kubernetes/sig-node-feature-requests, @dchen1107 , @derekwaynecarr, can you please take a look and let us know if you think the overall KEP is reasonable enough for merging, there are still a few outstanding issues/questions/details that need addressing but these could be addressed in follow up PRs.

@kow3ns has approved this so we just need a LGTM and we can have this merged.

@k8s-ci-robot
Copy link
Contributor

@Joseph-Irving: Reiterating the mentions to trigger a notification:
@kubernetes/sig-node-feature-requests

In response to this:

@kubernetes/sig-node-feature-requests, @dchen1107 , @derekwaynecarr, can you please take a look and let us know if you think the overall KEP is reasonable enough for merging, there are still a few outstanding issues/questions/details that need addressing but these could be addressed in follow up PRs.

@kow3ns has approved this so we just need a LGTM and we can have this merged.

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
Copy link
Member Author

It's starting to look unlikely that this KEP will be merged before the 30th. If so I will open a new PR in https://github.com/kubernetes/enhancements and hopefully progress can be made over there.

@thockin
Copy link
Member

thockin commented Nov 29, 2018

I think we can merge this. If we're mainly arguing about naming, just get it merged and follow-up.

Is that the only major outstanding issue?

@Joseph-Irving
Copy link
Member Author

Joseph-Irving commented Nov 29, 2018

@thockin to quote @kow3ns

I think we should merge the KEP as provisional provided SIG Node agrees that we SHOULD do this work.

We haven't had any feedback from sig-node on this since the 21st August unless somebody here is from sig-node and I didn't realise. It would be good for them to at least give it a thumbs up before merging as this will require kubelet changes.

I personally would just love to get this all merged, but it's not my call.

@thockin
Copy link
Member

thockin commented Nov 29, 2018

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 29, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kow3ns, thockin

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 bb6f763 into kubernetes:master Nov 29, 2018
@derekwaynecarr
Copy link
Member

I would like to discuss this in a future sig-node meeting , can we get this on the agenda?

Copy link
Member

@derekwaynecarr derekwaynecarr left a comment

Choose a reason for hiding this comment

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

I have no objection to solving the problem and understand the need. I feel like we may want to enable the pattern with more basic primitives, but maybe I am just the outlier.

```
Sidecars will be started before normal containers but after init, so that they are ready before your main processes start.

This will change the Pod startup to look like this:
Copy link
Member

Choose a reason for hiding this comment

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

I feel like we are wanting to enforce a specific startup and shutdown sequence for containers in a pod spec, and rather than invent new types of containers with special meaning, we may as well have a graph that lets a container establish a Wants=, Requires=, Requisite=, Before=, After= style graph where a container can reference another named container in the pod spec. Validation can ensure no loops. I feel like we are slowly walking a path that will eventually just demand that, and we may as well explore it. New types of containers are very hard to reason about in kubelet when doing support.

Copy link
Contributor

Choose a reason for hiding this comment

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

Before=, After= may mean that container should runs before some container, and runs alter some other container. and what does Wants=, Requires=, Requisite= refer to?

@thockin
Copy link
Member

thockin commented Nov 30, 2018 via email

justaugustus pushed a commit to justaugustus/community that referenced this pull request Dec 1, 2018
@Joseph-Irving
Copy link
Member Author

@derekwaynecarr, yeah it would be good to talk about this at a sig-node meeting again. @enisoc, who's the sig-apps sponsor, is away on leave until January so it's probably worth waiting until he's back.

@enisoc
Copy link
Member

enisoc commented Jan 14, 2019

FYI I'm back as of today. Was there any further discussion outside this thread that I need to catch up on?

@Joseph-Irving
Copy link
Member Author

Joseph-Irving commented Jan 15, 2019

No nothing else to catch up on.

We currently have this KEP on the sig-node agenda for the 29th Jan if anyone is interested.

Also this kep has now been moved over to kubernetes/enhancements and can be found here https://github.com/kubernetes/enhancements/blob/master/keps/sig-apps/sidecarcontainers.md .
Any further updates will be raised as PRs over there.

@soltysh
Copy link
Contributor

soltysh commented Jan 18, 2019

I must admit I have some concerns wrt to both proposal itself as well as the API. I'm feeling like the alternatives were not fully experimented. I'll try to show up on the sig-node meeting on Jan 29th to discuss this in depth.

@thockin
Copy link
Member

thockin commented Jan 28, 2019

I will not make the sig-node call, but if the feature wants to be more than what is written here, I care and want to weigh in.

@Joseph-Irving
Copy link
Member Author

@thockin I've opened up a new tracking issue for further discussion kubernetes/enhancements#753 if people want to follow along.
Current plan is to discuss this topic again at next week's sig-node meeting (5th feb) and hopefully get to the point where work can begin soon.


### Risks and Mitigations

You could set all containers to be `sidecar: true`, this seems wrong, so maybe the api should do a validation check that at least one container is not a sidecar.

Choose a reason for hiding this comment

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

why does this seem wrong ? Semantically yes, sidecar to what ? But practically it doesnt limit anything i think

rata added a commit to kinvolk/kubernetes-enhancements that referenced this pull request Sep 9, 2020
This was added due to a comment from Istio long ago[1], but they don't
need this anymore[2]. Furthermore, our use cases at Kinvolk also work
just fine without this.

[1]: kubernetes/community#2148 (comment)
[2]: kubernetes#1913 (comment)

Signed-off-by: Rodrigo Campos <[email protected]>
rata added a commit to kinvolk/kubernetes-enhancements that referenced this pull request Sep 10, 2020
This was added due to a comment from Istio long ago[1], but they don't
need this anymore[2]. Furthermore, our use cases at Kinvolk also work
just fine without this.

[1]: kubernetes/community#2148 (comment)
[2]: kubernetes#1913 (comment)

Signed-off-by: Rodrigo Campos <[email protected]>
SergeyKanzhelev pushed a commit to SergeyKanzhelev/enhancements that referenced this pull request Jan 8, 2021
This was added due to a comment from Istio long ago[1], but they don't
need this anymore[2]. Furthermore, our use cases at Kinvolk also work
just fine without this.

[1]: kubernetes/community#2148 (comment)
[2]: kubernetes#1913 (comment)

Signed-off-by: Rodrigo Campos <[email protected]>
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/design Categorizes issue or PR as related to design. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.