Skip to content
This repository has been archived by the owner on Oct 22, 2021. It is now read-only.

Add support for multi-containers aka sidecar per Pod #340

Closed
qu1queee opened this issue Mar 22, 2018 · 6 comments
Closed

Add support for multi-containers aka sidecar per Pod #340

qu1queee opened this issue Mar 22, 2018 · 6 comments

Comments

@qu1queee
Copy link
Contributor

Hi,

We currently have a strong use case for supporting multiple containers per Pod(aka sidecars). By default in Kube, a pod configuration file allows to have multiple entries under spec.containers.

However, the current setup of fissile(e.g. running a fissile build helm), does not support this by default. We can always patch the YAML templates generated by fissile, but ideally we want to support this sidecar feature as part of the fissile natural behavior(e.g. throught fissile build helm).

We would like to open a discussion on how to achieve this, and then PRing this to you. The main concern is that we will need to modify/introduce some of the YAML syntax for the role-manifest .

Our current idea would be to introduce two new keys in the role-manifest.yml, as follows:

roles:
- name: nginx
  jobs:
  - name: nginx
    release_name: nginx
  tags:
  - clustered
  sidecars:                 # new list to reference sidecar roles
  - log-forwarder
  run:
    memory: 256
    virtual-cpus: 4
    exposed-ports: [...]
    shared-volumes:
      - path: /mnt/share
        tag: common-share
        empty-dir: true    # new reference for a "volumes" entry in the template YAML
- name: log-forwarder
  type: sidecar            # new type called sidecar
  jobs:
  - name: elk
    release_name: elk
  tags:
  - clustered
  run:
    memory: 256
    virtual-cpus: 4
    exposed-ports: [...]
    shared-volumes:
      - path: /mnt/share
        tag: common-share
        empty-dir: true    # new reference for a "volumes" entry in the template YAML

As illustrated above, the roles.nginx.sidecars will ideally be the trigger to colocate in the nginx pod template, a second item under spec.containers with the properties of the log-forwarder sidecar.

Inside the log-forwarder role, the roles.component.type will indicate that this Pod YAML template should not be generated, whilst we expect the docker image to be generated.

With this sidecar, we would also require a simple volume sharing solution. For example, as defined in the Creating a Pod that runs two Containers scenario, we just want to share one directory between the containers. For that, an additional role-manifest.yml modification would be required to enable support for emptyDir: {}.

What do you think?

Thanks in advance for your feedback.
Regards,
@HeavyWombat @qu1queee

@mook-as
Copy link
Contributor

mook-as commented Mar 22, 2018

This is off the top of my head, and I think bringing in @viovanov would be useful too…

  • It might be better to actually bake multiple-container support into fissile, rather than doing it as sidecar containers, to be more obvious what's going on. Essentially, only name, description, and tags (maybe type?) would apply to the whole role… Not sure yet, though.
    • If you're worried about sharing containers between roles, YAML has anchors and things that should make that easier to deal with (but internally fissile would just see them as copies).
    • The type of resource to create (statefulset vs deployment) would be the same for all containers… and we currently based that on volumes and tags. I think this would be the thing that determines which path we go down: what happens if the sidecar looks like it needs to be a statefulset, but the main role doesn't?
  • Would it make sense to have a new volume type (where we currently have persistent-volumes and shared-volumes)?

@HeavyWombat
Copy link
Contributor

Thanks for your feedback. We went through your points and compared it to our thoughts on the topic. We see four main topics at the moment:

  • Name/class of the issue
  • YAML structure/configuration
  • Resource type (StatefulSet vs Deployment)
  • Volumes

Name/class of the issue

You are right, just naming it sidecar does not fully reflect the container co-location. It is more like that sidecar could be one of the flavours of that co-location of containers in pods. Maybe we start to call it either multi-container support, or container co-location in a pod.

YAML structure

We played around with different ideas on where to place it and to find reasons for and against different approaches. We strongly see the container co-location configuration on the same level as the current roles (bosh, bosh-task, docker?). A second (or third, ...) container in a pod is on the same level as the first one in the eyes of Kube. So putting the configuration nested into an existing role feels not right from a model kind of view. We played with the idea of configuring it in a role and using YAML anchors, but it looked kind of wrong at the end, especially since the co-located container configuration does include more than just a name, and description. It actually is pretty much the same amount like a current role. One other idea was to introduce a new root level key for the "roles" that only serve as co-location containers, but that felt like overkill.

Resource Type

Here we thought about that if one container of the pod is stateful and therefore requires a StatefulSet as its Kube form, it will become a StatefulSet even if the other container does not require that.

Volumes

There are multi-container in pod scenarios where you just want to share a directory between containers without the need to have a volumes claim, e.g. the emptyDir example. However, the current state of fissile with shared-volumes and persistent-volumes always create the volume claim section and that's why we suggested the emptyDir key as an addition. It would be also fine to introduce a whole new <some-suitable-name>-volumes key for the role.

Like suggested in the Slack channel, a meeting would be nice to talk about this.

@mook-as
Copy link
Contributor

mook-as commented Mar 23, 2018

Hi! Thanks for the (much more detailed than what I wrote) response!

The reason I was thinking of putting the container stuff inside the role is because I was mentally thinking of the role as the pod level construct (so having more containers in there is actually closer to the kube view); I can totally understand that if the role maps instead to a container the sidecar style matches better.

In case it wasn't clear: I'm totally not against the proposal, I'm just bouncing idea around to help me understand things 😄 I see that a meeting has been scheduled, and I'm confident enough in the people involved (yes I might have done some GitHub stalking…) 😀 And the bits of the response I didn't mention should be assumed to be things I'm happy with.

Thanks again!

@HeavyWombat
Copy link
Contributor

@mook-as I admit, our comment became a bit bigger than initially intended. Your feedback was very helpful for us to revisit everything we thought to have thought through. And that's why we figured it might be helpful to just write everything down in a hopefully organized way in preparation for the meeting this week.

@qu1queee
Copy link
Contributor Author

Some notes after the call with @viovanov, @HeavyWombat.

YAML Structure

The role block should support two features(aka keys):

  • existing type should support the collocated-container type. This will indicate that the role will never generate a helm template/pod, but its configuration will be used to inject a second container inside another role.
  • new key extra-pod-containers. A list of existing roles of they type collocated-container, that are intended to be place in this role as secondary containers.

Resource type

Master node have priority and collocated containers cannot be stateful. This should be checked through a sanity check.

Volumes

We need a new feature to specify that we want to share directories between containers that not necessarily be persistent volumes. In other words: We need a way to specify we want Kube's emptyDir feature.

@HeavyWombat
Copy link
Contributor

HeavyWombat commented May 7, 2018

@viovanov @mook-as We finished a first draft of the proposal including all the sanity-checks we discussed so far. This does not include the volumes yet. We wanted to touch base with you about feedback at the current stage and if that matches your assumptions.

Update: We added the emptyDir volume support as well as the promised sanity check that a role of type colocated-container does not have tags that would make it stateful.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants