Fix grace period override used for immediate evictions in eviction manager

[claassen]

What type of PR is this?

/kind bug

What this PR does / why we need it:

Currently when evicting pods due to disk or memory pressure, or when a pod's storage use has exceeded the configured ephemeral-storage resource limit we use a grace period override of 0 during eviction which actually ends up allowing the pod's full configured grace period rather than performing an immediate eviction due to the logic here:

https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/pod_workers.go#L1000-L1002

In order to actually perform an immediate eviction we should instead use a grace period override of 1 in these cases.

Which issue(s) this PR fixes:

Fixes #115819

Special notes for your reviewer:

Note that the linked issue also mentions a similar problem when force deleting pods via --grace-period=0 --force. This PR makes no attempt to address that issue. There are some notes around the --force option using a grace period of 0 for backwards compatibility which I am not sure the reason for, but pod deletion using --now or the equivalent --grace-period=1 behaves as expected and serves to illustrate how the correct grace period override for immediate eviction should be 1 rather than 0.

Does this PR introduce a user-facing change?

NONE

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

NONE

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: claassen / name: Michael Claassen (1d7921d, 12a8836)

[k8s-ci-robot]

Welcome @claassen!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @claassen. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: claassen
Once this PR has been reviewed and has the lgtm label, please assign random-liu for approval. For more information see the Kubernetes Code Review Process.

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

[claassen]

The messages here changed because previously in the test helper we were not setting the container name on the ContainerStats objects so it wasn't picking up the resource usage

[bart0sh]

/triage accepted
/priority important-soon
/assign @bobbypage @SergeyKanzhelev

[rphillips]

/test pull-kubernetes-cos-cgroupv2-containerd-node-e2e-eviction
/test pull-crio-cgroupv1-node-e2e-eviction

[k8s-ci-robot]

@claassen: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-cos-cgroupv2-containerd-node-e2e-eviction	12a8836	link	false	/test pull-kubernetes-cos-cgroupv2-containerd-node-e2e-eviction
pull-crio-cgroupv1-node-e2e-eviction	12a8836	link	false	/test pull-crio-cgroupv1-node-e2e-eviction

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.

[tuibeovince]

Can't we fix this for all cases if we handle it in pod_workers.go? Something like:

diff --git a/pkg/kubelet/pod_workers.go b/pkg/kubelet/pod_workers.go
index 20e8b493a8f..b90ccad3e5a 100644
--- a/pkg/kubelet/pod_workers.go
+++ b/pkg/kubelet/pod_workers.go
@@ -981,10 +981,12 @@ func calculateEffectiveGracePeriod(status *podSyncStatus, pod *v1.Pod, options *
        // enforce the restriction that a grace period can only decrease and track whatever our value is,
        // then ensure a calculated value is passed down to lower levels
        gracePeriod := status.gracePeriod
+       overriden := false
        // this value is bedrock truth - the apiserver owns telling us this value calculated by apiserver
        if override := pod.DeletionGracePeriodSeconds; override != nil {
                if gracePeriod == 0 || *override < gracePeriod {
                        gracePeriod = *override
+                       overriden = true
                }
        }
        // we allow other parts of the kubelet (namely eviction) to request this pod be terminated faster
@@ -992,12 +994,13 @@ func calculateEffectiveGracePeriod(status *podSyncStatus, pod *v1.Pod, options *
                if override := options.PodTerminationGracePeriodSecondsOverride; override != nil {
                        if gracePeriod == 0 || *override < gracePeriod {
                                gracePeriod = *override
+                               overriden = true
                        }
                }
        }
        // make a best effort to default this value to the pod's desired intent, in the event
        // the kubelet provided no requested value (graceful termination?)
-       if gracePeriod == 0 && pod.Spec.TerminationGracePeriodSeconds != nil {
+       if !overriden && gracePeriod == 0 && pod.Spec.TerminationGracePeriodSeconds != nil {
                gracePeriod = *pod.Spec.TerminationGracePeriodSeconds
        }
        // no matter what, we always supply a grace period of 1

I agree with this perspective that fixing the problem on pod_workers.go side may be enough to cover all the cases.
But since, there is a code block that checks for all grace period values < 1 and sets it all to 1:

    // no matter what, we always supply a grace period of 1

leaving this PR's fix for assigning the grace period to 1 during evictions will skip this one condition check and be a less complicated implementation. In my opinion, there might be merit in applying the fix in both eviction_manager.go and pod_workers.go. What do you think?

[k8s-ci-robot]

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.

[zcahana]

In my opinion, there might be merit in applying the fix in both eviction_manager.go and pod_workers.go. What do you think?

Applying the fix to pod_workers.go will have the added benefit of solving this for force deleted pods, which too are stopped with their full terminationGracePeriodSeconds instead of "immediately" (see #108741, as well as this comment in the original issue solved by this PR). While not explicitly in-scope for this PR, this could catch 2 (related) birds.

[tuibeovince]

Applying the fix to pod_workers.go will have the added benefit of solving this for force deleted pods, which too are stopped with their full terminationGracePeriodSeconds instead of "immediately" (see #108741, as well as this comment in the original issue solved by this PR). While not explicitly in-scope for this PR, this could catch 2 (related) birds.

I share the same sentiments. A fix in pod_workers.go may just be sufficient. Still, as stated in the issue being solved by this PR, the convention of "immediate", the documentation, and such must also be addressed with equal priority.

Sorry but I am still unfamiliar with the process of redefining conventions yet (I would love to know).

[tuibeovince]

There is a PR that I am also looking into that addresses the problem entirely on the pod_workers.go end (#120451) This might be a point of interest or a place for further discussion about this fix.

[tuibeovince]

@Seaiii Thank you. I will look into your PR as well.

[Seaiii]

@Seaiii Thank you. I will look into your PR as well.

Oh, sorry, I misunderstood, you were talking about forcing the first deletion to be 0. I mentioned issue before and now it doesn't allow first time deletion with a value of 0 so I turned it off

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and 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:

• Reopen this PR with /reopen
• Mark this PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

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

/close

[k8s-ci-robot]

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and 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:

• Reopen this PR with /reopen
• Mark this PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

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

/close

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.

[rphillips]

/reopen

[k8s-ci-robot]

@rphillips: Reopened this PR.

In response to this:

/reopen

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]

/remove-lifecycle rotten

@claassen please, rebase the PR, thanks.

[olyazavr]

👋 We've encountered this bug in our setup, and would be interested in seeing this merged in

[olyazavr]

I've rebased this PR here: #124063

[bart0sh]

/close
in favor of #124063

[k8s-ci-robot]

@bart0sh: Closed this PR.

In response to this:

/close
in favor of #124063

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.