sidecars: terminate sidecars after main containers #120620
Conversation
|
/test |
|
@tzneal: The
The following commands are available to trigger optional jobs:
Use
In response to this:
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. |
|
/test pull-kubernetes-node-e2e-containerd-sidecar-containers |
|
/cc |
7933272
to
32ad959
Compare
|
/test pull-kubernetes-node-e2e-containerd-sidecar-containers |
|
/cc @matthyx |
32ad959
to
b9323bb
Compare
|
/test pull-kubernetes-node-e2e-containerd-sidecar-containers |
| // time remaining | ||
| if ordering != nil && gracePeriod > 0 { | ||
| // grace period is only in seconds, so the time we've waited gets truncated downward | ||
| gracePeriod -= int64(ordering.waitForTurn(containerName, gracePeriod)) |
There was a problem hiding this comment.
Note to self: Subtracting the waiting time from each container's termination grace period.
lgtm, as this way would not break the existing behavior.
Definitely open to other opinions, but I think that once we're in the termination phase, we don't restart containers. |
|
/assign @mrunalp |
| @@ -49,6 +49,7 @@ import ( | |||
| utilfeature "k8s.io/apiserver/pkg/util/feature" | |||
| runtimeapi "k8s.io/cri-api/pkg/apis/runtime/v1" | |||
| kubelettypes "k8s.io/kubelet/pkg/types" | |||
|
|
|||
There was a problem hiding this comment.
very minor nit: I don't think this newline is needed here. Those imports seems to be from the same block.
| } | ||
|
|
||
| // newTerminationOrdering constructs a terminationOrdering based on the pod spec and the currently running containers. | ||
| func newTerminationOrdering(pod *v1.Pod, runningContainerNames []string) *terminationOrdering { |
There was a problem hiding this comment.
does this assume that initialization has completed? (i.e. all Init containers are not in running state?)
There was a problem hiding this comment.
It should be ok, we only wait on main containers and sidecars. If a non-restartable init container is still running, we don't setup any waits for it and also pre-close the channel for any non-running main container.
|
|
||
| }) | ||
|
|
||
| ginkgo.It("should call sidecar container PreStop hook simultaneously", func() { |
There was a problem hiding this comment.
please add test for termination started while the Pod is still in initialization stage
There was a problem hiding this comment.
Also can you please test when some containers hasn't started yet (blocked by the first container's PostStart lifecycle hook)
There was a problem hiding this comment.
I added the first test. I wrote the second one, but it fails due to the pre-existing #116032 . If that is resolved, we can re-add the second test and it should pass.
Sidecars should terminate: - after all main containers have exited - serialized and in reverse order
eb6669b
to
7bcc98c
Compare
|
/test pull-kubernetes-node-e2e-containerd-sidecar-containers |
|
/milestone v1.29 |
|
Hello! Are we still targeting the 1.29 release? |
|
LGTM thanks. /assign @SergeyKanzhelev |
|
/hold for @SergeyKanzhelev |
|
/lgtm |
|
LGTM label has been added. Git tree hash: 7fab9c44e07d2d525572f1068b164d53eacb4ecc
|
| "k8s.io/kubernetes/pkg/kubelet/types" | ||
| ) | ||
|
|
||
| // terminationOrdering is used to enforce a termination ordering for sidecar containers. It sets up |
There was a problem hiding this comment.
nit:
Do we have to call this restartable init container instead of the sidecar containers to be consistent with other code?
There was a problem hiding this comment.
Yeah, lets do it in a follow up so we don't lose the approval.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: matthyx, mrunalp, SergeyKanzhelev, tzneal 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 |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Sidecars should terminate:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: