Add termination grace period to pod sandbox config #88495
Conversation
k8s-ci-robot
added
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mrunalp The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@derekwaynecarr ptal |
There was a problem hiding this comment.
IMHO there's no sense to use negative values here, so maybe s/int64/uint64/?
There was a problem hiding this comment.
Yeah, agree, but my main reason was to match https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/api/core/v1/types.go#L2886.
There was a problem hiding this comment.
Got it.
From the top of your head, do you know if there's any special meaning for negative values? If not, I think we need to check and error out either here or in cri-o.
There was a problem hiding this comment.
I have amended the commit to follow the minimum grace period that is set for the container stop timeout.
Enable passing of sandbox's termination grace period down to OCI runtime, as an annotation for systemd. This is a glue between * kubernetes/kubernetes#88495 and * opencontainers/runc#2224 (or containers/crun#266) that is a part of the fix for * kubernetes/kubernetes#77873 Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
mrunalp
force-pushed
the
termination_grace_runpod
branch
from
1985047 to
ed1148b
Compare
|
cc @Random-Liu who I know has been involved in a lot of CRI stuff in the past. cc @Joseph-Irving as I know your sidecars implementation interacted w/ termination grace periods a fair amount, so just want to make sure you're aware. |
|
/assign @Random-Liu |
There was a problem hiding this comment.
I don't think this change is necessary
There was a problem hiding this comment.
This is a side effect of using gofmt (or goexports) and it's sort of inevitable as golang changes the formatting rules from time to time. Personally, I consider this is a price we have to pay for no coding style wars.
There was a problem hiding this comment.
ah, didn't realize it was linter driven 😄
There was a problem hiding this comment.
Yeah, came from there indeed but I have reverted it :)
This allows us to pass this down to runc and systemd so systemd can respect this timeout at shutdown. Signed-off-by: Mrunal Patel <mrunalp@gmail.com>
mrunalp
force-pushed
the
termination_grace_runpod
branch
from
ed1148b to
28a80ee
Compare
|
@mrunalp: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
|
@mrunalp Perhaps we can do without this PR, since My only concern is label is not standardized and its name can change (it's defined in pkg/kubelet/kuberuntime/labels.go -- maybe we can make it public to avoid copy-paste I do in cri-o/cri-o#3328) |
|
@kolyshkin Good find :) We can use that to implement this in CRI-O with the updated runc 👍. However, I still want to keep this open for feedback, because I feel that having this as a first class field in the CRI over an annotation is better. |
|
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
k8s-ci-robot
added
lifecycle/stale
|
@mrunalp: PR needs rebase. 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. |
|
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
k8s-ci-robot
added
lifecycle/rotten
|
Rotten issues close after 30d of inactivity. Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
|
@fejta-bot: Closed this PR. 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. |
7 participants
This allows us to pass this down to runc and systemd
so systemd can respect this timeout at shutdown.
Signed-off-by: Mrunal Patel mrunalp@gmail.com
What type of PR is this?
/kind bug
What this PR does / why we need it:
This PR allows us to handle graceful termination of pods at shutdown time. It passes the shutdown
grace period for pods as part of the pod sandbox configuration so it can be passed down to runc and systemd to respect the grace period.
runc PR for this has been merged opencontainers/runc#2224
Which issue(s) this PR fixes:
Fixes #77873
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
No.
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
-->