feat: Add karpenter_pods_disrupted_total metric #2253
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: omerap12 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/L
Signed-off-by: Omer Aplatony <[email protected]>
omerap12
force-pushed
the
karpenter_pods_disrupted_total
branch
from
ea3549d to
201d456
Compare
Pull Request Test Coverage Report for Build 15783145450Details
💛 - Coveralls |
k8s-ci-robot
added
the
needs-rebase
Signed-off-by: Omer Aplatony <[email protected]>
k8s-ci-robot
removed
the
needs-rebase
|
Friendly ping @jonathan-innis @engedaam |
k8s-ci-robot
added
the
needs-rebase
Signed-off-by: Omer Aplatony <[email protected]>
k8s-ci-robot
added
needs-rebase
Signed-off-by: Omer Aplatony <[email protected]>
k8s-ci-robot
removed
the
needs-rebase
| @@ -99,6 +99,11 @@ func (c *Controller) Reconcile(ctx context.Context) (reconcile.Result, error) { | |||
| errs[i] = client.IgnoreNotFound(err) | |||
| return | |||
| } | |||
| pods := &corev1.PodList{} | |||
| var podCount int | |||
| if err := c.kubeClient.List(ctx, pods, client.MatchingFields{"spec.nodeName": node.Name}); err == nil { | |||
There was a problem hiding this comment.
Should we include DS pods in this calculation or should we just stick to reschedulable pods (running pods)
There was a problem hiding this comment.
I think we should only include rescheduled pods.DS pods are automatically handled and are meant to run on every node (or specific ones using nodeSelector or affinity). So, if we include DaemonSet pods in the disruption metrics, it might give a wrong idea about real workload disruptions.
| @@ -131,6 +131,10 @@ func (c *Controller) deleteNodeClaim(ctx context.Context, nodeClaim *v1.NodeClai | |||
| if err := c.kubeClient.Delete(ctx, nodeClaim); err != nil { | |||
| return reconcile.Result{}, client.IgnoreNotFound(err) | |||
| } | |||
| pods := &corev1.PodList{} | |||
| if err := c.kubeClient.List(ctx, pods, client.MatchingFields{"spec.nodeName": node.Name}); err != nil { | |||
There was a problem hiding this comment.
Since pods are quite big, I think we should either look at creating a helper that returns a count of reschedulable pods that are currently bound to a node and employs the client.UnsafeDisableDeepCopy method OR we should use the PartialMetadataList -- I'm personally more in favor of the first one
| @@ -28,6 +28,7 @@ import ( | |||
| "github.com/awslabs/operatorpkg/singleton" | |||
There was a problem hiding this comment.
Is there some way that we could avoid duplicating the pod list logic and rather handle the pod disruption metric inside of the termination controller? One thought that I had when I was thinking about the implementation of this is that we could make sure that we add the Disrupted status condition on the NodeClaim always (expiration and health included) and then when we go to look-up the disruption reason for pods, we can just look-up the status condition reason to determine which type of resaon we should fire for that metric
There was a problem hiding this comment.
Yeah I agree this is a better idea.
k8s-ci-robot
added
the
needs-rebase
|
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-sigs/prow repository. |
Fixes #2020
Description
Added a new metric
karpenter_pods_disrupted_totalto track the number of pods disrupted during node termination events.How was this change tested?
Added unit tests for the new metric
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.