docs: RFC for disruption.terminationGracePeriod feature #834

wmgroot · 2023-12-01T08:05:15Z

Design for #743
Implementation PR: #916

Description
RFC for support for disruption.forceDrainAfter (nodeDrainTimeout)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

linux-foundation-easycla · 2023-12-01T08:05:19Z

The committers listed above are authorized under a signed CLA.

✅ login: wmgroot (6a4ac94)

k8s-ci-robot · 2023-12-01T08:05:21Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: wmgroot
Once this PR has been reviewed and has the lgtm label, please assign bwagner5 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:

OWNERS

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 · 2023-12-01T08:05:24Z

Welcome @wmgroot!

It looks like this is your first PR to kubernetes-sigs/karpenter 🎉. 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-sigs/karpenter 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 · 2023-12-01T08:05:25Z

Hi @wmgroot. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

designs/disruption-controls-drain-timeout.md

jackfrancis · 2023-12-01T21:26:20Z

/test ls

k8s-ci-robot · 2023-12-01T21:26:23Z

@jackfrancis: The specified target(s) for /test were not found.
The following commands are available to trigger optional jobs:

/test pull-karpenter-test-1-26
/test pull-karpenter-test-1-27
/test pull-karpenter-test-1-28
/test pull-karpenter-test-1-29

Use /test all to run all jobs.

In response to this:

/test ls

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.

jackfrancis · 2023-12-01T21:27:56Z

/test pull-karpenter-test-1-26

jackfrancis · 2023-12-01T21:29:17Z

/test pull-karpenter-test-1-29

k8s-ci-robot · 2023-12-01T21:29:36Z

@wmgroot: 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-karpenter-test-1-26	`0ab6f41`	link	false	`/test pull-karpenter-test-1-26`
pull-karpenter-test-1-29	`0ab6f41`	link	false	`/test pull-karpenter-test-1-29`

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.

jackfrancis · 2023-12-01T21:30:10Z

@wmgroot thank you for letting me pollute your PR thread w/ some prow tests, just testing out some recent additions, nothing for you to worry about here :)

designs/disruption-controls-drain-timeout.md

ellistarn · 2023-12-05T05:19:01Z

designs/disruption-controls-drain-timeout.md

+DESCRIPTION:
+     NodeDrainTimeout is the total amount of time that the controller will spend
+     on draining a node. The default value is 0, meaning that the node can be
+     drained without any time limitations. NOTE: NodeDrainTimeout is different


This NOTE suggests that there has been some confusion with this semantic. Any learnings here @jackfrancis?

njtran

Nice design! Thanks for writing this out. Eager to talk more about this.

designs/disruption-controls-drain-timeout.md

njtran · 2023-12-05T22:19:53Z

designs/disruption-controls-drain-timeout.md

+	with caution.
+```
+
+It might sufficient to skip straight to termination of the node at the cloud provider rather than waiting for a successful force drain to complete.


The problem with this is similar to #655. You'll get leaked pods, which is a hefty consideration if we decide to go this way.

I'm not sure how best to address this problem for daemonset pods which often tolerate all taints. I think we can attempt a force drain for non-daemonset pods as described in the spec, but would need to rely on full termination in the cloud provider for the remaining edge cases.

Daemonsets tolerating all taints I see as a separate issue. Many daemonsets do this because it's the easiest way to ensure that the pods are running as long as possible, without having to think about all the different addons that may add taints. Whichever ones terminate all taints will either need to be okay with being leaked or we may need to respect their terminationGracePeriodSeconds as well.

garvinp-stripe · 2023-12-06T18:15:11Z

designs/disruption-termination-grace-period.md

+    consolidationPolicy: WhenUnderutilized || WhenEmpty
+    consolidateAfter: 10m || Never # metav1.Duration
+    expireAfter: 10m || Never # Equivalent to v1alpha5 TTLSecondsUntilExpired
+    terminationGracePeriodSeconds: 86400 || 24h || nil


if we are specifying seconds we shouldn't allow 24h?

Yeah, you're correct. I'm going to update it to support standard duration formats for convenience so we don't have to google how many seconds are in a day all the time.

What about terminationGracePeriodDuration? Would make it easier to do the thing you're trying to do.

I think I slightly prefer terminationGracePeriod just for brevity, but I don't have very strong opinions.

wmgroot · 2024-01-26T17:32:24Z

Sounds good to me. We should be able to work on the implementation details in parallel since switching between NodePool vs NodeClaim spec is a fairly trivial part of the actual implementation.

github-actions · 2024-02-14T12:01:31Z