should not override KillPodOptions.PodTerminationGracePeriodSecondsOverride by pod spec.TerminationGracePeriodSeconds #109412

249043822 · 2022-04-11T02:44:53Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Since the gracePeriodOverride in KillContainer is only used for killing container, not contains the preStop time, so we should not always set this param in podworker, because we may also lose the probe.TerminationGracePeriodSeconds.

kubernetes/pkg/kubelet/kuberuntime/kuberuntime_container.go

Line 716 in 7380fc7

if gracePeriodOverride != nil {

pod deletion, TerminationGracePeriodSeconds is not correct

apiVersion: v1
kind: ReplicationController
metadata:
  name: busytest
  namespace: default
  labels:
    app: rctest
spec:
  replicas: 1
  selector:
    app: busytest
  template:
    metadata:
      labels:
        app: busytest
    spec:
      containers:
      - name: busy01
        image: docker.artsz.zte.com.cn/cci/usee/test/busybox:v1.0
        command:
        - sh
        - -c
        - sleep 3600s
        lifecycle:
            preStop:
              exec:
                command: [ "/bin/sleep", "10" ]
      terminationGracePeriodSeconds: 30

I0410 23:13:46.482969   25118 kuberuntime_container.go:614] "PreStop hook completed" pod="default/busytest-b6cbn" podUID=d9f1f5e4-5b3e-4661-8702-2f2f94cd70a5 containerName="busy01" containerID="containerd://87db50c7e467afff05744465b5f7fc036d7c1d6b0861c2480160eff053be5a8a"
I0410 23:13:46.483034   25118 kuberuntime_container.go:724] "Killing container with a grace period override" pod="default/busytest-b6cbn" podUID=d9f1f5e4-5b3e-4661-8702-2f2f94cd70a5 containerName="busy01" containerID="containerd://87db50c7e467afff05744465b5f7fc036d7c1d6b0861c2480160eff053be5a8a" gracePeriod=30
I0410 23:13:46.483060   25118 kuberuntime_container.go:728] "Killing container with a grace period" pod="default/busytest-b6cbn" podUID=d9f1f5e4-5b3e-4661-8702-2f2f94cd70a5 containerName="busy01" containerID="containerd://87db50c7e467afff05744465b5f7fc036d7c1d6b0861c2480160eff053be5a8a" gracePeriod=30

terminationGracePeriodSeconds is 30, and the hook takes 10 seconds to complete, and the Container should take 20 seconds to stop, but the log shows 30

pod eviction, gracePeriodOverride is not correct

kind: Pod
apiVersion: v1
metadata:
  name: evictme
spec:
  restartPolicy: Never
  terminationGracePeriodSeconds: 10
  containers:
  - name: busybox
    image: k8s.gcr.io/e2e-test-images/nginx:1.14-2
    command: ["sh", "-c", "fallocate -l 4G file; sleep 100000"]

I0411 17:47:31.238192   23726 eviction_manager.go:350] "Eviction manager: must evict pod(s) to reclaim" resourceName="ephemeral-storage"
I0411 17:47:31.238827   23726 eviction_manager.go:368] "Eviction manager: pods ranked for eviction" pods=[default/evictme default/csi-hostpathplugin-0 default/nfs-server-674bcc7996-hsq7z kube-system/kube-controller-manager-10.235.30.82 kube-system/kube-apiserver-10.235.30.82 kube-system/kube-proxy-10.235.30.82 kube-system/kube-scheduler-10.235.30.82 default/my-csi-app kube-system/calico-node-7sdl5 kube-system/calico-kube-controllers-6cdf7cf687-89q4d kube-system/coredns-86c46b7bb8-9jv7x]
I0411 17:47:31.239343   23726 kuberuntime_container.go:728] "Killing container with a grace period override" pod="default/evictme" podUID=486791a9-9d06-4c08-8930-3583b5f34192 containerName="busybox" containerID="containerd://934833bdd381ad34f6a3bf6f25360c0888649cb1abef1496dd39db40b7e7edca" gracePeriod=10
I0411 17:47:31.239359   23726 kuberuntime_container.go:728] "Killing container with a grace period" pod="default/evictme" podUID=486791a9-9d06-4c08-8930-3583b5f34192 containerName="busybox" containerID="containerd://934833bdd381ad34f6a3bf6f25360c0888649cb1abef1496dd39db40b7e7edca" gracePeriod=10

pod is evicted, it should be killed with PodTerminationGracePeriodSecondsOverride 0, but the log shows 10

Which issue(s) this PR fixes:

Fixes ##109352

Special notes for your reviewer:

/cc @smarterclayton @rphillips

Does this PR introduce a user-facing change?

NONE

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

…erride by pod spec.TerminationGracePeriodSeconds

249043822 · 2022-04-11T03:39:43Z

/retest

yangjunmyfm192085 · 2022-04-11T12:12:33Z

pkg/kubelet/pod_workers.go

+			return gracePeriod, status.gracePeriod != 0 && status.gracePeriod != gracePeriod
+		}
+	}
+	// this value is bedrock truth - the apiserver owns telling us this value calculated by apiserver


the apiserver owners tell us?

yangjunmyfm192085

/lgtm

ehashman · 2022-04-12T20:02:23Z

/hold

There have been a bunch of issues in flight in this area over the past few releases... wanted to link #98507 #107893 #102025

/cc @smarterclayton

k8s-ci-robot · 2022-04-13T00:56:01Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: 249043822, yanghesong
To complete the pull request process, please assign derekwaynecarr after the PR has been reviewed.
You can assign the PR to them by writing /assign @derekwaynecarr in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

pkg/kubelet/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

endocrimes · 2022-05-04T17:09:38Z

/cc @rphillips

k8s-triage-robot · 2022-08-02T17:50:06Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

249043822 · 2022-08-03T00:46:54Z

/remove-lifecycle stale

k8s-triage-robot · 2022-11-01T00:53:03Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2022-12-01T01:22:31Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

tgoodsell-tempus · 2022-12-15T06:04:24Z

/remove-lifecycle rotten

tgoodsell-tempus · 2022-12-15T17:25:57Z

/cc @249043822

Do you have any insight on where this is sitting with the sig-node team?

k8s-ci-robot · 2022-12-15T17:25:59Z

@tgoodsell-tempus: GitHub didn't allow me to request PR reviews from the following users: 249043822.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @249043822

Do you have any insight on where this is sitting with the sig-node team?

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.

bart0sh · 2022-12-20T07:57:30Z

/priority important-longterm
/triage accepted
/assign @bobbypage

aoxn · 2023-03-03T07:41:55Z

@smarterclayton GracePeriod was override by default and executePreStopHook time was not respected. Thus preStopHook execution time is not included in the gracefulTerminationSeconds. It is unexpected.

bart0sh · 2023-03-10T07:50:03Z

/assign @smarterclayton

marquiz · 2023-05-31T09:03:12Z

pkg/kubelet/pod_workers.go

+			// we should keep options.PodTerminationGracePeriodSecondsOverride as it is
+			// since if options.PodTerminationGracePeriodSecondsOverride is nil, the killContainer interface
+			// of kuberuntime_container will still calculate a effective grace period for this container termination
+			return gracePeriod, status.gracePeriod != 0 && status.gracePeriod != gracePeriod


Do we need to check for gracePeriod < 1 here?

srl11 · 2023-06-13T09:35:52Z

pkg/kubelet/pod_workers.go

@@ -681,9 +681,6 @@ func (p *podWorkers) UpdatePod(options UpdatePodOptions) {

 		wasGracePeriodShortened = gracePeriodShortened
 		status.gracePeriod = gracePeriod
-		// always set the grace period for syncTerminatingPod so we don't have to recalculate,
-		// will never be zero.
-		options.KillPodOptions.PodTerminationGracePeriodSecondsOverride = &gracePeriod


Should we keep this ?
Correct me if I'm wrong: It seems that the gracePeriod in killContainer is valued by options.KillPodOptions.PodTerminationGracePeriodSecondsOverride.
Is that beeter to keep having options.KillPodOptions.PodTerminationGracePeriodSecondsOverride set by the effective gracePeriod here?

k8s-triage-robot · 2024-01-20T17:08:56Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

robert-heinzmann-logmein · 2024-02-08T16:40:06Z

Is this issue addressed somewhere / somehow else ?

should not override KillPodOptions.PodTerminationGracePeriodSecondsOv…

649331b

…erride by pod spec.TerminationGracePeriodSeconds

k8s-ci-robot requested review from rphillips and smarterclayton April 11, 2022 02:44

yangjunmyfm192085 reviewed Apr 11, 2022

View reviewed changes

yangjunmyfm192085 reviewed Apr 12, 2022

View reviewed changes

k8s-ci-robot assigned yangjunmyfm192085 Apr 12, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 12, 2022

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 12, 2022

ehashman added this to Triage in SIG Node PR Triage Apr 12, 2022

yanghesong approved these changes Apr 13, 2022

View reviewed changes

pacoxu moved this from Triage to Waiting on Author in SIG Node PR Triage Jun 20, 2022

249043822 moved this from Waiting on Author to Needs Reviewer in SIG Node PR Triage Jun 24, 2022

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 2, 2022

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 3, 2022

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 1, 2022

yanghesong mentioned this pull request Nov 8, 2022

REQUEST: New membership for <yanghesong> kubernetes/org#3826

Closed

9 tasks

David-Tamrazov mentioned this pull request Nov 14, 2022

Pods termination grace period seconds are not executed as expected. #109352

Open

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Dec 1, 2022

k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Dec 15, 2022

bart0sh moved this from Needs Approver to Triage in SIG Node PR Triage Dec 16, 2022

k8s-ci-robot assigned bobbypage Dec 20, 2022

bart0sh moved this from Triage to Needs Reviewer in SIG Node PR Triage Dec 20, 2022

k8s-ci-robot assigned smarterclayton Mar 10, 2023

SergeyKanzhelev moved this from Needs Reviewer to Needs Approver in SIG Node PR Triage Mar 10, 2023

marquiz reviewed May 31, 2023

View reviewed changes

srl11 reviewed Jul 26, 2023

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 20, 2024

249043822 closed this Feb 1, 2024

SIG Node PR Triage automation moved this from Needs Approver to Done Feb 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

should not override KillPodOptions.PodTerminationGracePeriodSecondsOverride by pod spec.TerminationGracePeriodSeconds #109412

should not override KillPodOptions.PodTerminationGracePeriodSecondsOverride by pod spec.TerminationGracePeriodSeconds #109412

249043822 commented Apr 11, 2022

249043822 commented Apr 11, 2022

yangjunmyfm192085 Apr 11, 2022

249043822 Apr 12, 2022

yangjunmyfm192085 left a comment

ehashman commented Apr 12, 2022

k8s-ci-robot commented Apr 13, 2022

endocrimes commented May 4, 2022

k8s-triage-robot commented Aug 2, 2022

249043822 commented Aug 3, 2022

k8s-triage-robot commented Nov 1, 2022

k8s-triage-robot commented Dec 1, 2022

tgoodsell-tempus commented Dec 15, 2022

tgoodsell-tempus commented Dec 15, 2022

k8s-ci-robot commented Dec 15, 2022

bart0sh commented Dec 20, 2022

aoxn commented Mar 3, 2023 •

edited

bart0sh commented Mar 10, 2023

marquiz May 31, 2023

srl11 Jun 13, 2023 •

edited

k8s-triage-robot commented Jan 20, 2024

robert-heinzmann-logmein commented Feb 8, 2024

should not override KillPodOptions.PodTerminationGracePeriodSecondsOverride by pod spec.TerminationGracePeriodSeconds #109412

should not override KillPodOptions.PodTerminationGracePeriodSecondsOverride by pod spec.TerminationGracePeriodSeconds #109412

Conversation

249043822 commented Apr 11, 2022

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR 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.:

249043822 commented Apr 11, 2022

yangjunmyfm192085 Apr 11, 2022

Choose a reason for hiding this comment

249043822 Apr 12, 2022

Choose a reason for hiding this comment

yangjunmyfm192085 left a comment

Choose a reason for hiding this comment

ehashman commented Apr 12, 2022

k8s-ci-robot commented Apr 13, 2022

endocrimes commented May 4, 2022

k8s-triage-robot commented Aug 2, 2022

249043822 commented Aug 3, 2022

k8s-triage-robot commented Nov 1, 2022

k8s-triage-robot commented Dec 1, 2022

tgoodsell-tempus commented Dec 15, 2022

tgoodsell-tempus commented Dec 15, 2022

k8s-ci-robot commented Dec 15, 2022

bart0sh commented Dec 20, 2022

aoxn commented Mar 3, 2023 • edited

bart0sh commented Mar 10, 2023

marquiz May 31, 2023

Choose a reason for hiding this comment

srl11 Jun 13, 2023 • edited

Choose a reason for hiding this comment

k8s-triage-robot commented Jan 20, 2024

robert-heinzmann-logmein commented Feb 8, 2024

aoxn commented Mar 3, 2023 •

edited

srl11 Jun 13, 2023 •

edited