Skip to content

Conversation

@pohly
Copy link
Contributor

@pohly pohly commented Sep 19, 2022

What type of PR is this?

/kind documentation

What this PR does / why we need it:

A pod with Spec.TerminationGracePeriodSeconds will get force-deleted also when the client doing the delete doesn't explicitly ask for it. That follows from

// user has specified a value
if options.GracePeriodSeconds != nil {
period = *options.GracePeriodSeconds
} else {
// use the default value if set, or deletes the pod immediately (0)
if pod.Spec.TerminationGracePeriodSeconds != nil {
period = *pod.Spec.TerminationGracePeriodSeconds
}
}
// if the pod is not scheduled, delete immediately
if len(pod.Spec.NodeName) == 0 {
period = 0
}
// if the pod is already terminated, delete immediately
if pod.Status.Phase == api.PodFailed || pod.Status.Phase == api.PodSucceeded {
period = 0
}
if period < 0 {
period = 1
}
choosing TerminationGracePeriodSeconds as the value for GracePeriodSeconds when nothing is chosen explicitly.

This effect was not obvious from the documentation of the field and might be something that users should avoid.

Does this PR introduce a user-facing change?

NONE

A pod with Spec.TerminationGracePeriodSeconds will get force-deleted also when
the client doing the delete doesn't explicitly ask for it. That follows from
https://github.com/kubernetes/kubernetes/blob/0f582f7c3f504e807550310d00f130cb5c18c0c3/pkg/registry/core/pod/strategy.go#L151-L171
choosing TerminationGracePeriodSeconds as the value for GracePeriodSeconds when
nothing is chosen explicitly.

This effect was not obvious from the documentation of the field and might be
something that users should avoid.
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/documentation Categorizes issue or PR as related to documentation. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 19, 2022
// signal (no opportunity to shut down).
// signal (no opportunity to shut down) and also turns all Pod deletions for the pod into
// force-deletes (apiserver removes the Pod object immediately). Forced deletions can be potentially
// disruptive for some workloads and their Pods.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"Forced deletions ..." is a copy of the advice from https://kubernetes.io/docs/concepts/workloads/pods/pod-lifecycle/#pod-termination-forced.

It's debatable whether it should get repeated here.

Copy link
Member

Choose a reason for hiding this comment

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

Even the comment above your change is not exactly true:

The value zero indicates stop immediately via the kill signal (no opportunity to shut down).

The doc reads:

Setting the grace period to 0 forcibly and immediately deletes the Pod from the API server. If the pod was still running on a node, that forcible deletion triggers the kubelet to begin immediate cleanup.
[...]
When a force deletion is performed, the API server does not wait for confirmation from the kubelet that the Pod has been terminated on the node it was running on. It removes the Pod in the API immediately so a new Pod can be created with the same name. On the node, Pods that are set to terminate immediately will still be given a small grace period before being force killed.

So here's my suggestion:

The value zero indicates that the pod should be forced-deleted immediately without waiting for confirmation
that it has been terminated. On the node, pods that are set to terminate immediately will still be given a
small grace period before being force killed. Forced deletions can be potentially disruptive for some workloads
and their Pods.

I think that explains both what happens on the apiserver and the node, and also gives the warning you're looking for, wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So "The value zero indicates stop immediately via the kill signal (no opportunity to shut down)." (from the original documentation) is really just plain wrong?

On the node, pods that are set to terminate immediately will still be given a small grace period before being force killed.

Where does this small grace period come from? Can it be configured?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know anything but what I've seen in the doc you linked :-) I doubt it can be configured. But I think that's almost irrelevant, what matters most is that it's removed from the apiserver without confirmation.

What problem are you trying to solve? It sounds like you need some policy to prevent terminationGracePeriod from being set to 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know anything but what I've seen in the doc you linked :-)

Those docs also might be wrong... If we want the API documentation to be correct, we probably need to determine what the implementation in kubelet actually does.

What problem are you trying to solve?

I wanted test pods to be killed immediately by kubelet but without also enabling a force delete because I wanted to go through the normal pod deletion process. From the documentation it sounded like TerminationGracePeriodSeconds: 0 would do that ("The value zero indicates stop immediately via the kill signal"), but than I discovered that a Delete(..., metav1.DeleteOptions{}) (i.e. a normal delete) acted like a force-delete (Delete(..., metav1.DeleteOptions{GracePeriodSeconds: &zero})).

This was surprising. I want to avoid that surprise for others, either:

  • by fixing the documentation to describe accurately what TerminationGracePeriodSeconds does or
  • by making the implementation behave as implied by the documentation (control kill period in kubelet, without the force-delete side effect).

If someone wants it to be one, why wouldn't they set it to one?

They might want it to be zero, without realizing the full implications. Fixing the documentation would help here because in practice, a delay of one second is close enough - users just need to know it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@klueska do you know how quickly kubelet kills pods when TerminationGracePeriodSeconds: 0? Is it immediately (because the Pod object is gone) or after a certain grace period?

It would be counter-intuitive if pods got killed more slowly for TerminationGracePeriodSeconds: 0 than for TerminationGracePeriodSeconds: 1.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pohly
Once this PR has been reviewed and has the lgtm label, please assign liggitt for approval by writing /assign @liggitt in a comment. For more information see:The Kubernetes Code Review Process.

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

Details Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot added 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. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 19, 2022
@k8s-triage-robot
Copy link

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@leilajal
Copy link
Contributor

/assign @apelisse
/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Sep 20, 2022
@pohly
Copy link
Contributor Author

pohly commented Sep 20, 2022

FWIW, I also wouldn't mind changing the behavior so that if Spec.TerminationGracePeriodSeconds is used as default and happens to be zero, then one would be used instead of zero. I'd find that less surprising, but it probably has to be considered an API change.

@apelisse
Copy link
Member

FWIW, I also wouldn't mind changing the behavior so that if Spec.TerminationGracePeriodSeconds is used as default and happens to be zero, then one would be used instead of zero. I'd find that less surprising, but it probably has to be considered an API change.

If someone wants it to be one, why wouldn't they set it to one?

@aojea
Copy link
Member

aojea commented Sep 22, 2022

/assign @smarterclayton

he was trying to clarify this too #102025

it seems the bot closed it

@pohly
Copy link
Contributor Author

pohly commented Sep 22, 2022

Thanks @aojea. That other PR shows that the current behavior is unintentionally and should be changed.

Let's put this doc update on hold and instead see whether we can finish the implementation update.

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 22, 2022
@k8s-triage-robot
Copy link

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-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 21, 2022
@pohly
Copy link
Contributor Author

pohly commented Dec 21, 2022

No progress on #102025 but as I think that that's the right solution I'll close this one here.

/close

@k8s-ci-robot
Copy link
Contributor

@pohly: Closed this PR.

Details

In response to this:

No progress on #102025 but as I think that that's the right solution I'll close this one here.

/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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/code-generation cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/documentation Categorizes issue or PR as related to documentation. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. 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. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants