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 Containers #753

Closed
Joseph-Irving opened this issue Jan 29, 2019 · 154 comments
Closed

Sidecar Containers #753

Joseph-Irving opened this issue Jan 29, 2019 · 154 comments

Comments

@Joseph-Irving
Copy link
Member

@Joseph-Irving Joseph-Irving commented Jan 29, 2019

Enhancement Description

/kind feature
/sig apps
/sig node

@Joseph-Irving
Copy link
Member Author

@Joseph-Irving Joseph-Irving commented Jan 29, 2019

@enisoc @dchen1107 @fejta @thockin @kow3ns @derekwaynecarr, opened this tracking issue so that we can discuss.

@kow3ns
Copy link
Member

@kow3ns kow3ns commented Jan 31, 2019

/assign

@Joseph-Irving
Copy link
Member Author

@Joseph-Irving Joseph-Irving commented Feb 1, 2019

@derekwaynecarr I've done some scoping out of the kubelet changes required for next week's sig-node meeting, I believe that changes are only needed in the kuberuntime package, specifically kuberuntime_manager.go in and kuberuntime_container.go.

In kuberuntime_manager.go you could modify computePodActions to implement the shutdown triggering (kill sidecars when all non-sidecars have permanently exited), and starting up the sidecars first.

In kuberuntime_container.go you could modify killContainersWithSyncResult for terminating the sidecars last and sending the preStop hooks (the preStop hooks bit was a bit debatable, it wasn't settled whether that should be done or not. @thockin had a good point about why you might not want to encourage that behaviour, see comment).

Let me know if you want me to investigate any further.

@resouer
Copy link
Member

@resouer resouer commented Feb 1, 2019

@kow3ns The discussion makes more sense to me if maybe we can define a full description of containers sequence in Pod spec (sig-app), and how to handle the sequence in kubelet for start, restart and cascading consideration (sig-node). Let's catch the Feb 5 sig-node meeting to give more inputs.

cc @Joseph-Irving

@luksa
Copy link
Member

@luksa luksa commented Feb 7, 2019

The proposal says that sidecars only run after the init containers run. But what if the use-case requires the sidecar to run while/before the init containers run. For example, if you'd like route the pod's traffic through a proxy running as a sidecar (as in Istio), you probably want that proxy to be in place while the init containers run in case the init container itself does network calls.

@Joseph-Irving
Copy link
Member Author

@Joseph-Irving Joseph-Irving commented Feb 14, 2019

@luksa I think there's the possibility of looking at having sidecars that run in init phase at some point but currently the proposal is not going to cover that use case. There is currently no way to have concurrent containers running in the init phase so that would be potentially a much larger/messier change than what is being suggested here.

@Joseph-Irving
Copy link
Member Author

@Joseph-Irving Joseph-Irving commented Feb 14, 2019

Update on this KEP:
I've spoken to both @derekwaynecarr and @dchen1107 from sig-node about this and they did not express any major concerns about the proposal. I will raise a PR to the KEP adding some initial notes around implementation details and clarifying a few points that came up during the discussion.

We still need to agree on the API, it seems there is consensus that a simple way of marking containers as sidecars is prefered over more in depth ordering flags. Having a bool is somewhat limiting though so perhaps something more along the lines of containerLifecycle: Sidecar would be preferable so that we have the option of expanding in the future.

@luksa
Copy link
Member

@luksa luksa commented Feb 14, 2019

@Joseph-Irving Actually, neither the boolean nor the containerLifecycle: Sidecar are appropriate for proper future extensibility. Instead, containerLifecycle should be an object, just like deployment.spec.strategy, with type: Sidecar. This would allow us to then introduce additional fields. For the "sidecar for the whole lifetime of the pod" solution, it would be expressed along these lines:

containerLifecycle: 
  type: Sidecar
  sidecar:
    scope: CompletePodLifetime

as opposed to

containerLifecycle: 
  type: Sidecar
  sidecar:
    scope: AfterInit

Please forgive my bad naming - I hope the names convey the idea.

But there is one problem with the approach where we introduce containerLifecycle to pod.spec.containers. Namely, it's wrong to have sidecars that run parallel to init containers specified under pod.spec.containers. So if you really want to be able to extend this to init containers eventually, you should find an alternative solution - one that would allow you to mark containers as sidecars at a higher level - i.e. not under pod.spec.containers or pod.spec.initContainers, but something like pod.spec.sidecarContainers, which I believe you already discussed, but dismissed. The init containers problem definitely calls for a solution along these lines.

@Joseph-Irving
Copy link
Member Author

@Joseph-Irving Joseph-Irving commented Feb 14, 2019

@luksa You could also solve the init problem by just allowing an init container to be marked as a sidecar and have that run alongside the init containers. As I understand it, the problem is that init containers sometimes need sidecars, which is different from needing a container that runs for the entire lifetime of the pod.

The problem with pod.spec.sidecarContainers is that it's a far more complex change, tooling would need to updated and the kubelet would require a lot of modifying to support another set of containers. The current proposal is far more modest, it's only building on what's already there.

@luksa
Copy link
Member

@luksa luksa commented Feb 14, 2019

@Joseph-Irving We could work with that yes. It's not ideal for the sidecar to shut down after the init containers run and then have the same sidecar start up again, but it's better than not having that option. The bigger problem is that older Kubelets wouldn't handle init-sidecar containers properly (as is the case with main-sidecar containers).

I'd just like you to keep init-sidecars in mind when finalizing the proposal. In essence, you're introducing the concept of "sidecar" into k8s (previously, we basically only had a set of containers that were all equal). Now you're introducing actual sidecars, so IMHO, you really should think this out thoroughly and not dismiss a very important sidecar use-case.

I'd be happy to help with implementing this. Without it, Istio can't provide its features to init containers (actually, in a properly secured Kubernetes cluster running Istio, init containers completely lose the ability to talk to any service).

@Joseph-Irving
Copy link
Member Author

@Joseph-Irving Joseph-Irving commented Mar 7, 2019

In relation to the implementation discussion in #841, I've opened a WIP PR containing a basic PoC for this proposal kubernetes/kubernetes#75099. It's just a first draft and obviously not perfect but the basic functionality works and gives you an idea of the amount of change required.

cc @enisoc

@Joseph-Irving
Copy link
Member Author

@Joseph-Irving Joseph-Irving commented Mar 18, 2019

I put together a short video just showing how the PoC currently behaves https://youtu.be/4hC8t6_8bTs. Seeing it in action can be better than reading about it.
Disclaimer: I'm not a pro youtuber.

@Joseph-Irving
Copy link
Member Author

@Joseph-Irving Joseph-Irving commented Mar 26, 2019

I've opened two new PRs:

  • #918 is about Graduation critieria, version skew, upgrade/downgrade
  • #919 is to decide on the API implementation

Any thoughts or suggestions will be much appreciated.

@kikisdeliveryservice
Copy link
Member

@kikisdeliveryservice kikisdeliveryservice commented Sep 12, 2020

Related PR: #1980

@kikisdeliveryservice
Copy link
Member

@kikisdeliveryservice kikisdeliveryservice commented Sep 13, 2020

Hi @Joseph-Irving @thockin and everyone else 😄

Enhancements Lead here. I see that there is still a ton of ongoing conversation, but as a reminder, please keep us updated of any plans to include this in 1.20 are decided so we can track the progress.

Thanks!
Kirsten

@rata
Copy link
Member

@rata rata commented Sep 14, 2020

@kikisdeliveryservice will keep you posted, thanks!

@Zachery2008
Copy link

@Zachery2008 Zachery2008 commented Sep 17, 2020

Does anyone have any reference to, or could be so kind to share, the current workaround for this issue, specifically for the case of the Istio Envoy sidecar?

@shaneqld for startup issues, the istio community came up with a quite clever workaround which basically injects envoy as the first container in the container list and adds a postStart hook that checks and wait for envoy to be ready. This is blocking and the other containers are not started making sure envoy is there and ready before starting the app container.

We had to port this to the version we're running but is quite straightforward and are happy with the results so far.

For shutdown we are also 'solving' with preStop hook but adding an arbitrary sleep which we hope the application would have gracefully shutdown before continue with SIGTERM.

Could you show some insights in detail how to do these? How to achieve adding 'pre-stop' to the Istio-proxy sidecar? It seems it needs some custom configuration or use custom sidecar. I face the same issue that when pods scales down, the main container is trying to finish the jobs but it loses connection to outside probably because the Istio-sidecar closed immediately after SIGTERM. Right now I just use the default sidecar injection. Thank you!

@tariq1890
Copy link
Member

@tariq1890 tariq1890 commented Sep 17, 2020

Ok this thread is getting hijacked. Let's stay on topic, please.

@kikisdeliveryservice
Copy link
Member

@kikisdeliveryservice kikisdeliveryservice commented Sep 29, 2020

Just a gentle reminder that Enhancements Freeze is next week, Tuesday, October 6th. By that time the KEP would need to be updated to be marked implementable.

Also the KEP is using an older format, so updating would be great (once you finish hammering out the details): https://github.com/kubernetes/enhancements/tree/master/keps/NNNN-kep-template

@rata
Copy link
Member

@rata rata commented Sep 30, 2020

@kikisdeliveryservice thanks for the remainder. Will do if it is decided to be included for 1.20. Thanks! :)

@rata
Copy link
Member

@rata rata commented Oct 5, 2020

This won't be part of 1.20. Thanks a lot for pinging! :)

@Atreus-Technologies
Copy link

@Atreus-Technologies Atreus-Technologies commented Oct 9, 2020

I have an interest in this issue, and wanted to thank both @Joseph-Irving and @howardjohn for their insights on this, which helped resolve some of my questions.

I don't want to hijack this proposal, but based on the conversations above, I wonder if this is maybe a slightly broader/larger issue than has been recognised so far.

I can imagine the following solutions to this issue -

  1. Define a new container entity "sidecar container" which starts after initContainers, before "main containers", and terminate after "main containers" terminate (per @Joseph-Irving original proposal)
  2. Define an additional field on (1) which sets whether the "sidecar container" starts before the initContainer(s) per @luksa suggestion).
  3. Go broader.

Personally, option (2) solves my immediate problem.

But I'm wondering if these questions don't speak to a more strategic issue in K8s around scheduling and how we define a pod. In my specific (Istio related) case, I suggested something like runlevels within pods.

Option (2) solves my problem too, but I can imagine even more complex dependency structures which might call for embedding a DAG of container dependencies within a pod/statefulSet/daemonSet/whatever - this is the option (3) I am thinking of.

Just wondering if this issue should really be re-focused on the pod definition itself, with a view to creating something more generic? I originally thought in terms of a runlevels analogy, but maybe an Airflow-like DAG structure would have the broadest applicability.

@michalzxc
Copy link

@michalzxc michalzxc commented Oct 9, 2020

What about adding envoy as init container as well? This way it will provide network for other init containers. When init will finish it would 'exit 0' as well, and then regular envoy (not init) would take over

@davinkevin
Copy link

@davinkevin davinkevin commented Oct 10, 2020

@michalzxc If I'm not wrong, init containers are executed one by one sequentially, so you currently can't have an envoy next to another container as an init-container.

@rata
Copy link
Member

@rata rata commented Oct 12, 2020

Hi!

The sidecar discussion continued on this places (I've updated sig-node slack, github PR that started this and several mailing lists):
https://groups.google.com/g/kubernetes-sig-node/c/w019G3R5VsQ/m/bbRDZTv5CAAJ
https://groups.google.com/g/kubernetes-sig-node/c/7kUaX-jBN3M/m/dhI3E8LOAAAJ

As you can see, we are collecting use cases now, after having some more use cases different small groups can create pre-proposals addressing them. Feel free to add your use case (if it's not there yet) or join later for the pre-proposals part :-)

Please, let's keep this enhacement issue on topic (and probably closed). You are welcome to join the conversation on those places :)

@Joseph-Irving
Copy link
Member Author

@Joseph-Irving Joseph-Irving commented Oct 21, 2020

This KEP will not be progressing, Sig-node and others feel that this is not an incremental step in the right direction so they've gone back to the drawing board and will be coming up with some new KEPs that can hopefully solve all the use cases stated in this KEP as well as others.

Please see @rata's previous comment #753 (comment)
For places where you can contribute to the discussion.

It's unfortunate that all the work done on this KEP will not be getting used, but a wider group of people are now thinking about these issues so hopefully the solution they come up with will be what's best for everyone.
I've already spent over two years trying to get this through so I think this a good time for me to move on, @rata and others will be leading these initiatives going forward.

/close

@k8s-ci-robot
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot commented Oct 21, 2020

@Joseph-Irving: Closing this issue.

In response to this:

This KEP will not be progressing, Sig-node and others feel that this is not an incremental step in the right direction so they've gone back to the drawing board and will be coming up with some new KEPs that can hopefully solve all the use cases stated in this KEP as well as others.

Please see @rata's previous comment #753 (comment)
For places where you can contribute to the discussion.

It's unfortunate that all the work done on this KEP will not be getting used, but a wider group of people are now thinking about these issues so hopefully the solution they come up with will be what's best for everyone.
I've already spent over two years trying to get this through so I think this a good time for me to move on, @rata and others will be leading these initiatives going forward.

/close

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.

dmckoske pushed a commit to RedHatInsights/rhsm-subscriptions that referenced this issue Feb 15, 2021
To test, observe in CI, you can adjust the schedules as desired like so:

```
oc process -f templates/rhsm-subscriptions-scheduler.yml --param=KAFKA_BOOTSTRAP_HOST=platform-mq-ci-kafka-bootstrap.platform-mq-ci.svc.cluster.local '--param=CAPTURE_SNAPSHOT_SCHEDULE=0 * * * *' '--param=PURGE_SNAPSHOT_SCHEDULE=5 * * * *' --param=IMAGE_TAG=05004c5 | oc apply -f -
```

By implementing in the scheduler template, we don't need app-interface
changes (I'm arguing that the cronjobs are just reimplementations of the
scheduler function).

I applied a deadline to the both the job/pod of 30 minutes.

Note that I did not apply splunk forwarder sidecar to these images as
it's not designed to be used with a terminating pod (the sidecar keeps
running, preventing the job from completing). Given that we're queueing
up work, but not performing it, and that the completed job pods' logs
are viewable in OpenShift, deferring sidecar integration for now
(future Kubernetes versions may give a clean, simple solution, see
kubernetes/enhancements#753).
hholst80 added a commit to morecontainers/reloader that referenced this issue Mar 24, 2021
The sidecar stuff was never added into K8s:
kubernetes/enhancements#753 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet