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 KEP API implementation #919

Merged
merged 1 commit into from
Jun 19, 2019
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 32 additions & 5 deletions keps/sig-apps/sidecarcontainers.md
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ Allowing multiple containers to run at once during the init phase - this could b

## Proposal

Create a way to define containers as sidecars, this will be an additional field to the Container Spec: `sidecar: true`. //TODO Decide on the API (see [Alternatives](#alternatives))
Create a way to define containers as sidecars, this will be an additional field to the `container.lifecycle` spec: `Type` which can be either `Standard` (default) or `Sidecar`.

e.g:
```yaml
Expand All @@ -120,7 +120,8 @@ spec:
command: ['do something']
- name: sidecar
image: sidecar-image
sidecar: true
lifecycle:
type: Sidecar
command: ["do something to help my app"]

```
Expand Down Expand Up @@ -155,8 +156,34 @@ The proposal can broken down into four key pieces of implementation that all rel
* Sidecars are terminated after normal containers
* Sidecars start before normal containers

#### API Changes:
As this is a change to the Container spec we will be using feature gating, you will be required to explicitly enable this feature on the api server as recommended [here](https://github.com/kubernetes/community/blob/master/contributors/devel/api_changes.md#adding-unstable-features-to-stable-versions).

New field `Type` will be added to the lifecycle struct:

```go
type Lifecycle struct {
// Type
// One of Standard, Sidecar.
// Defaults to Standard
// +optional
Type LifecycleType `json:"type,omitempty" protobuf:"bytes,3,opt,name=type,casttype=LifecycleType"`
}
```
New type `LifecycleType` will be added with two constants:
```go
// LifecycleType describes the lifecycle behaviour of the container
type LifecycleType string

const (
// LifecycleTypeStandard is the default container lifecycle behaviour
LifecycleTypeStandard LifecycleType = "Standard"
// LifecycleTypeSidecar means that the container will start up before standard containers and be terminated after
LifecycleTypeSidecar LifecycleType = "Sidecar"
)
```
Note that currently the `lifecycle` struct is only used for `preStop` and `postStop` so we will need to change its description to reflect the expansion of its uses.

#### Kubelet Changes:
Joseph-Irving marked this conversation as resolved.
Show resolved Hide resolved
Broad outline of what places could be modified to implement desired behaviour:

Expand Down Expand Up @@ -188,9 +215,9 @@ Please view this [video](https://youtu.be/4hC8t6_8bTs) if you want to see what t

### Risks and Mitigations

You could set all containers to be `sidecar: true`, this would cause strange behaviour in regards to shutting down the sidecars when all the non-sidecars have exited. To solve this the api could do a validation check that at least one container is not a sidecar.
You could set all containers to have `lifecycle.type: Sidecar`, this would cause strange behaviour in regards to shutting down the sidecars when all the non-sidecars have exited. To solve this the api could do a validation check that at least one container is not a sidecar.

Init containers would be able to have `sidecar: true` applied to them as it's an additional field to the container spec, this doesn't currently make sense as init containers are ran sequentially. We could get around this by having the api throw a validation error if you try to use this field on an init container or just ignore the field.
Init containers would be able to have `lifecycle.type: Sidecar` applied to them as it's an additional field to the container spec, this doesn't currently make sense as init containers are ran sequentially. We could get around this by having the api throw a validation error if you try to use this field on an init container or just ignore the field.

Older Kubelets that don't implement the sidecar logic could have a pod scheduled on them that has the sidecar field. As this field is just an addition to the Container Spec the Kubelet would still be able to schedule the pod, treating the sidecars as if they were just a normal container. This could potentially cause confusion to a user as their pod would not behave in the way they expect, but would avoid pods being unable to schedule.

Expand Down Expand Up @@ -239,4 +266,4 @@ One alternative would be to have a new field in the Pod Spec of `sidecarContaine

Another alternative would be to change the Job Spec to have a `primaryContainer` field to tell it which containers are important. However I feel this is perhaps too specific to job when this Sidecar concept could be useful in other scenarios.

Having it as a boolean could cause problems later down the line if more lifecycle related flags were added, perhaps it makes more sense to have something like `containerLifecycle: Sidecar` to make it more future proof.
A boolean flag of `sidecar: true` could be used to indicate which pods are sidecars, however this prevents additional ContainerLifecycles from being added in the future.