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

Add SidecarContainers feature #116429

Merged
merged 7 commits into from
Jul 8, 2023
Merged

Conversation

gjkim42
Copy link
Member

@gjkim42 gjkim42 commented Mar 9, 2023

What type of PR is this?

/kind feature
/kind api-change

What this PR does / why we need it:

This adds the Sidecar Containers feature to kubernetes.

  • API change - add restartPolicy to Init containers
  • API validation - only allow restartPolicy for Init containers and allow startupProbe for the sidecar containers
  • Startup: wait for Started, not for completion.
  • Reconciling status of Sidecar container with the runtime
  • Termination: terminate the pod even if sidecar is still running
  • Resources calculation update
  • Restart sidecar on sidecar failure/completion while pod is still running
  • Topology/CPU managers update

Which issue(s) this PR fixes:

Fixes #

ref: #115934

Special notes for your reviewer:

Does this PR introduce a user-facing change?

The new feature gate "SidecarContainers" is now available. This feature introduces sidecar containers, a new type of init container that starts before other containers but remains running for the full duration of the pod's lifecycle and will not block pod termination.

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

- [KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/753-sidecar-containers

/cc @SergeyKanzhelev @matthyx

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Mar 9, 2023
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/code-generation area/kubelet kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 9, 2023
@cici37
Copy link
Contributor

cici37 commented Mar 9, 2023

/cc @jpbetz

@matthyx
Copy link
Contributor

matthyx commented Mar 9, 2023

/triage accepted
/priority important-soon

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Mar 9, 2023
Copy link
Contributor

@jpbetz jpbetz left a comment

Choose a reason for hiding this comment

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

Here's some API review related feedback, I'll do a pass on the implementation shortly

staging/src/k8s.io/api/core/v1/types.go Outdated Show resolved Hide resolved
pkg/apis/core/validation/validation.go Outdated Show resolved Hide resolved
@SergeyKanzhelev
Copy link
Member

I updated the state of this PR in this issue description: #115934

There are still some things to be done, but those are tracked and close to be implemented

@olitomlinson
Copy link

The new feature gate "SidecarContainers" is now available. This feature introduces sidecar containers, a new type of init container that starts before other containers but remains running for the full duration of the pod's lifecycle and will not block pod termination.

Quick one, just to be more explicit, would the sidecar init container start before other non-sidecar init containers? or would it start in parallel to non-sidecar init containers? TIA

Apologies if this is the wrong place to ask the question, I'm new here :)

@matthyx
Copy link
Contributor

matthyx commented Jul 10, 2023

The new feature gate "SidecarContainers" is now available. This feature introduces sidecar containers, a new type of init container that starts before other containers but remains running for the full duration of the pod's lifecycle and will not block pod termination.

Quick one, just to be more explicit, would the sidecar init container start before other non-sidecar init containers? or would it start in parallel to non-sidecar init containers? TIA

Apologies if this is the wrong place to ask the question, I'm new here :)

It depends on which order you put them inside initContainers... there is only one ordered list of init containers, some of which are executed to completion (non-sidecars) and the new ones launched and restarted if needed (sidecars).

@tzneal
Copy link
Contributor

tzneal commented Jul 10, 2023

Quick one, just to be more explicit, would the sidecar init container start before other non-sidecar init containers? or would it start in parallel to non-sidecar init containers? TIA

The sidecar init containers follow the standard startup order logic (i.e. in the order specified).

@matthyx
Copy link
Contributor

matthyx commented Jul 10, 2023

The new feature gate "SidecarContainers" is now available. This feature introduces sidecar containers, a new type of init container that starts before other containers but remains running for the full duration of the pod's lifecycle and will not block pod termination.

Quick one, just to be more explicit, would the sidecar init container start before other non-sidecar init containers? or would it start in parallel to non-sidecar init containers? TIA
Apologies if this is the wrong place to ask the question, I'm new here :)

It depends on which order you put them inside initContainers... there is only one ordered list of init containers, some of which are executed to completion (non-sidecars) and the new ones launched and restarted if needed (sidecars).

I need to finish the docs PR and make it clear there before we release...

@gjkim42 gjkim42 deleted the sidecar branch July 16, 2023 07:03
victortoso added a commit to victortoso/kubevirt that referenced this pull request Oct 27, 2023
There is a but in sidecar lifecycle that might keep the sidecar
running even when its Pod has terminated, leaving the Pod dangling
with "NotReady" state. This is being tracked in:

    kubernetes/kubernetes#116429

This bug shows itself in KubeVirt on migration. After the first
migration job, the VM can't be migrated again as previous migration
does not finish.

In the hooks' v1alpha3 we introduce the Shutdown() method to allow
notify the sidecar that it should exit(). Follow up patches will
implement this method in the cmd/example-hook-sidecar.

Fixes: kubevirt#8395
Signed-off-by: Victor Toso <victortoso@redhat.com>
victortoso added a commit to victortoso/kubevirt that referenced this pull request Oct 27, 2023
There is a but in sidecar lifecycle that might keep the sidecar
running even when its Pod has terminated, leaving the Pod dangling
with "NotReady" state. This is being tracked in:

    kubernetes/kubernetes#116429

This bug shows itself in KubeVirt on migration. After the first
migration job, the VM can't be migrated again as previous migration
does not finish.

In the hooks' v1alpha3 we introduce the Shutdown() method to allow
notify the sidecar that it should exit(). Follow up patches will
implement this method in the cmd/example-hook-sidecar.

Fixes: kubevirt#8395
Signed-off-by: Victor Toso <victortoso@redhat.com>
victortoso added a commit to victortoso/kubevirt that referenced this pull request Nov 16, 2023
There is a but in sidecar lifecycle that might keep the sidecar
running even when its Pod has terminated, leaving the Pod dangling
with "NotReady" state. This is being tracked in:

    kubernetes/kubernetes#116429

This bug shows itself in KubeVirt on migration. After the first
migration job, the VM can't be migrated again as previous migration
does not finish.

In the hooks' v1alpha3 we introduce the Shutdown() method to allow
notify the sidecar that it should exit(). Follow up patches will
implement this method in the cmd/example-hook-sidecar.

Fixes: kubevirt#8395
Signed-off-by: Victor Toso <victortoso@redhat.com>
victortoso added a commit to victortoso/kubevirt that referenced this pull request Nov 16, 2023
There is a but in sidecar lifecycle that might keep the sidecar
running even when its Pod has terminated, leaving the Pod dangling
with "NotReady" state. This is being tracked in:

    kubernetes/kubernetes#116429

This bug shows itself in KubeVirt on migration. After the first
migration job, the VM can't be migrated again as previous migration
does not finish.

In the hooks' v1alpha3 we introduce the Shutdown() method to allow
notify the sidecar that it should exit(). Follow up patches will
implement this method in the cmd/example-hook-sidecar.

Fixes: kubevirt#8395
Signed-off-by: Victor Toso <victortoso@redhat.com>
victortoso added a commit to victortoso/kubevirt that referenced this pull request Nov 17, 2023
There is a but in sidecar lifecycle that might keep the sidecar
running even when its Pod has terminated, leaving the Pod dangling
with "NotReady" state. This is being tracked in:

    kubernetes/kubernetes#116429

This bug shows itself in KubeVirt on migration. After the first
migration job, the VM can't be migrated again as previous migration
does not finish.

In the hooks' v1alpha3 we introduce the Shutdown() method to allow
notify the sidecar that it should exit(). Follow up patches will
implement this method in the cmd/example-hook-sidecar.

Fixes: kubevirt#8395
Signed-off-by: Victor Toso <victortoso@redhat.com>
victortoso added a commit to victortoso/kubevirt that referenced this pull request Nov 23, 2023
There is a but in sidecar lifecycle that might keep the sidecar
running even when its Pod has terminated, leaving the Pod dangling
with "NotReady" state. This is being tracked in:

    kubernetes/kubernetes#116429

This bug shows itself in KubeVirt on migration. After the first
migration job, the VM can't be migrated again as previous migration
does not finish.

In the hooks' v1alpha3 we introduce the Shutdown() method to allow
notify the sidecar that it should exit(). Follow up patches will
implement this method in the cmd/example-hook-sidecar.

Fixes: kubevirt#8395
Signed-off-by: Victor Toso <victortoso@redhat.com>
victortoso added a commit to victortoso/kubevirt that referenced this pull request Nov 24, 2023
There is a but in sidecar lifecycle that might keep the sidecar
running even when its Pod has terminated, leaving the Pod dangling
with "NotReady" state. This is being tracked in:

    kubernetes/kubernetes#116429

This bug shows itself in KubeVirt on migration. After the first
migration job, the VM can't be migrated again as previous migration
does not finish.

In the hooks' v1alpha3 we introduce the Shutdown() method to allow
notify the sidecar that it should exit(). Follow up patches will
implement this method in the cmd/example-hook-sidecar.

Fixes: kubevirt#8395
Signed-off-by: Victor Toso <victortoso@redhat.com>
victortoso added a commit to victortoso/kubevirt that referenced this pull request Nov 27, 2023
There is a but in sidecar lifecycle that might keep the sidecar
running even when its Pod has terminated, leaving the Pod dangling
with "NotReady" state. This is being tracked in:

    kubernetes/kubernetes#116429

This bug shows itself in KubeVirt on migration. After the first
migration job, the VM can't be migrated again as previous migration
does not finish.

In the hooks' v1alpha3 we introduce the Shutdown() method to allow
notify the sidecar that it should exit(). Follow up patches will
implement this method in the cmd/example-hook-sidecar.

Fixes: kubevirt#8395
Signed-off-by: Victor Toso <victortoso@redhat.com>
enp0s3 pushed a commit to enp0s3/kubevirt that referenced this pull request Dec 3, 2023
There is a but in sidecar lifecycle that might keep the sidecar
running even when its Pod has terminated, leaving the Pod dangling
with "NotReady" state. This is being tracked in:

    kubernetes/kubernetes#116429

This bug shows itself in KubeVirt on migration. After the first
migration job, the VM can't be migrated again as previous migration
does not finish.

In the hooks' v1alpha3 we introduce the Shutdown() method to allow
notify the sidecar that it should exit(). Follow up patches will
implement this method in the cmd/example-hook-sidecar.

Fixes: kubevirt#8395
Signed-off-by: Victor Toso <victortoso@redhat.com>
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. area/code-generation area/kubelet area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Archived in project
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet