Added do-not-evict option for pods #499
Conversation
| @@ -55,7 +55,7 @@ func NewController(kubeClient client.Client, coreV1Client corev1.CoreV1Interface | |||
| func (c *Controller) Reconcile(ctx context.Context, object client.Object) (reconcile.Result, error) { | |||
| node := object.(*v1.Node) | |||
| // 1. Check if node is terminable | |||
| if node.DeletionTimestamp == nil || !functional.ContainsString(node.Finalizers, provisioning.KarpenterFinalizer) { | |||
| if node.DeletionTimestamp.IsZero() || !functional.ContainsString(node.Finalizers, provisioning.KarpenterFinalizer) { | |||
There was a problem hiding this comment.
How does IsZero work if timestamp is nil? Does the fn handle the case?
There was a problem hiding this comment.
From the function documentation
// IsZero returns true if the value is nil or time is zero.
| @@ -89,18 +88,83 @@ var _ = Describe("Termination", func() { | |||
| v1alpha2.ProvisionerNamespaceLabelKey: "default", | |||
| }, | |||
| }) | |||
| ExpectCreated(env.Client, node) | |||
| ExpectCreated(env.Client, test.Pod(test.PodOptions{ | |||
| podEvict := test.Pod(test.PodOptions{ | |||
There was a problem hiding this comment.
consider inlining {podoptions{nodename}}
| })) | ||
| }) | ||
| ExpectCreated(env.Client, node) | ||
| ExpectCreated(env.Client, podEvict, podSkip) |
There was a problem hiding this comment.
This variadic is fancy and can take (env.Client, node, podevict, podskip)
|
|
||
| // Expect podToEvict to be evicting, and delete it | ||
| podEvict = ExpectPodExists(env.Client, podEvict.Name, podEvict.Namespace) | ||
| Expect(podEvict.GetObjectMeta().GetDeletionTimestamp().IsZero()).To(Equal(false)) |
| ExpectNotFound(env.Client, node) | ||
| }) | ||
| It("should not terminate nodes that have a do-not-evict pod", func() { | ||
| node := test.NodeWith(test.NodeOptions{ |
There was a problem hiding this comment.
Can you share this node between tests?
| It("should not terminate nodes that have a do-not-evict pod", func() { | ||
| node := test.NodeWith(test.NodeOptions{ | ||
| Finalizers: []string{v1alpha2.KarpenterFinalizer}, | ||
| Labels: map[string]string{ |
There was a problem hiding this comment.
Remind me why we care about these labels?
There was a problem hiding this comment.
We don't, that was from a previous PR that I was copying. Good point.
|
|
||
| ExpectCreated(env.Client, node) | ||
| for _, pod := range pods { | ||
| ExpectCreated(env.Client, pod) |
There was a problem hiding this comment.
Does go type interference not allow ExpectCreated(env.Client, pods...)?
| }, | ||
| }) | ||
| pods := []*v1.Pod{ | ||
| test.Pod(test.PodOptions{ |
There was a problem hiding this comment.
consider naming these pods instead of a slice similar to the previous test, I think it will clean things up.
| ExpectDeleted(env.Client, pods[1]) | ||
|
|
||
| // Reconcile node to evict pod | ||
| node = ExpectNodeExists(env.Client, node.Name) |
There was a problem hiding this comment.
Do you need to check existence here?
There was a problem hiding this comment.
If termination unexpectedly terminates even with pods still existing, this test could pass. This is to make sure the node doesn't terminate prematurely.
| critical = append(critical, pod) | ||
| for _, p := range pods { | ||
| if val, ok := p.Annotations[provisioning.KarpenterDoNotEvictPodAnnotation]; ok && val == "true" { | ||
| zap.S().Debugf("Node %s has a do-not-evict pod %s. Watiing to drain until all do-not-evict pods are terminated.", node.Name, p.Name) |
There was a problem hiding this comment.
Remind me, we won't try to reconcile again until we get a pod event, right?
There was a problem hiding this comment.
We will reconcile again. We will exponentially back off though.
| if pod.Spec.PriorityClassName == "system-cluster-critical" || pod.Spec.PriorityClassName == "system-node-critical" { | ||
| critical = append(critical, pod) | ||
| for _, p := range pods { | ||
| if val, ok := p.Annotations[provisioning.KarpenterDoNotEvictPodAnnotation]; ok && val == "true" { |
There was a problem hiding this comment.
You should be able to do p.Annotations[provisioning.KarpenterDoNotEvictPodAnnotation] == "true", since if the key doesn't exist, the string will be ""
| critical = append(critical, pod) | ||
| for _, p := range pods { | ||
| if val, ok := p.Annotations[provisioning.KarpenterDoNotEvictPodAnnotation]; ok && val == "true" { | ||
| zap.S().Debugf("Node %s has a do-not-evict pod %s. Watiing to drain until all do-not-evict pods are terminated.", node.Name, p.Name) |
There was a problem hiding this comment.
Watiing.
Perhaps
Unable to drain node %s, pod %s has 'do-not-evict' annotation"
| @@ -101,18 +109,13 @@ func (t *Terminator) terminate(ctx context.Context, node *v1.Node) error { | |||
|
|
|||
| // evictPods returns true if there are no evictable pods | |||
| func (t *Terminator) evictPods(ctx context.Context, pods []*v1.Pod) bool { | |||
| empty := true | |||
There was a problem hiding this comment.
Does this function even need to return true anymore? Shouldn't it return err? We already know if the node is empty before we even call this function, since we can check len(pods) before we call this.
There was a problem hiding this comment.
I think it's a matter of doing a len(check) before, or doing it as the return value. Either way, we still need to check if there was a pod that needs evicting. I added the return len(pods)==0 so that if we call evictPods, it'll just return that immediately because the for loop would have nothing to iterate through.
With regards to erroring vs bool, I think we decided on an early comment to punt on this to another PR.
Issue, if available:
Description of changes:
karpenter.sh/do-not-evictwhere requesting to terminate that node will cordon it, but wait to drain until those pods are terminated.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.