-
Notifications
You must be signed in to change notification settings - Fork 668
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add new podEvictor statistics #648
Conversation
Hi @pravarag. 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 Once the patch is verified, the new status will be reflected by the 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. |
24f7920
to
8d513b5
Compare
🥇 It would be nice to have more insights around these metrics. What are your (and others) thoughts around including the following info?
|
Thanks @a7i for sharing above comments. I've few doubts though and again for this change we don't have anything definite on paper so I'll post these here for now,
Wouldn't this be more user specific? Like, not everyone will be having same custom labels. My understanding could be wrong here but if you have any examples for the same would be helpful 🙂 or does the custom labels refer to labels here? |
Sorry if I was not clear. This proposal will be done outside of this PR. Once this PR is merged, I will create a Feature with my proposal/ideas around it. |
@a7i thanks for the information shared above. Please feel free to suggest any changes I can implement as part of this PR as well. |
I would really like to see |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/ok-to-test
@pravarag could you please move the PR out of draft if it's ready for review and remove the WIP
from the title?
StabilityLevel: metrics.ALPHA, | ||
}, []string{"result"}) | ||
|
||
TotalPodsSkipped = metrics.NewCounterVec( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there anywhere you're updating TotalPodsSkipped
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
@@ -99,6 +99,7 @@ func (pe *PodEvictor) TotalEvicted() int { | |||
for _, count := range pe.nodepodCount { | |||
total += count | |||
} | |||
metrics.TotalPodsEvicted.With(map[string]string{"result": "total pods evicted so far"}).Inc() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the metric being incremented here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for incrementing the metrics here is to capture the total pods evicted as per the count goes up in line 100. I thought of keeping it align with the evicted pod count going up in evictions.go
so that it might be easier to record the total count. Let me know your thoughts otherwise I'll make some changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. It seems odd that it's being incremented in a method that is only supposed to get total evicted count.
// TotalEvicted gives a number of pods evicted through all nodes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated this as well to this line and have included namespace
as a field under all the metrics.
@@ -112,14 +113,16 @@ func (pe *PodEvictor) EvictPod(ctx context.Context, pod *v1.Pod, node *v1.Node, | |||
} | |||
if pe.maxPodsToEvictPerNode > 0 && pe.nodepodCount[node]+1 > pe.maxPodsToEvictPerNode { | |||
metrics.PodsEvicted.With(map[string]string{"result": "maximum number reached", "strategy": strategy, "namespace": pod.Namespace}).Inc() | |||
return false, fmt.Errorf("Maximum number %v of evicted pods per %q node reached", pe.maxPodsToEvictPerNode, node.Name) | |||
metrics.PodsEvictedSuccess.With(map[string]string{"strategy": strategy}).Inc() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is PodsEvictedSuccess
being incremented when maxPodsToEvictPerNode has been reached? Should this be TotalPodsSkipped
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
8d513b5
to
b67619d
Compare
55ef3c4
to
8b5bb6a
Compare
@a7i @damemi I've one doubt regarding the utilization of metric
Any suggestions on where else I can utilize TotalPodsSkipped metrics would be helpful. |
@pravarag I think this looks good, just needs a rebase |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: damemi, pravarag The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
62f27d9
to
afc9334
Compare
afc9334
to
ff0e422
Compare
Thanks @damemi , I've rebased the branch as suggested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder what is benefit of the new TotalPodsSkipped
, PodsEvictedSuccess
and PodsEvictedFailed
metrics besides simpler expressions when querying the metrics? Currently they are subset of what is already provided by PodsEvicted
.
When running the descheduler with metrics enabled the PodsEvicted
metric gets continuously incremented over time with pods getting regularly evicted. In order to provide a number of pods evicted/skipped/... in a single run the pod evictor needs to be signaled a new run/descheduling cycle started.
if pe.metricsEnabled { | ||
metrics.PodsEvicted.With(map[string]string{"result": "maximum number of pods per node reached", "strategy": strategy, "namespace": pod.Namespace, "node": node.Name}).Inc() | ||
} | ||
metrics.PodsEvicted.With(map[string]string{"result": "maximum number of pods per node reached", "strategy": strategy, "namespace": pod.Namespace, "node": node.Name}).Inc() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind putting all the places where your new metrics are populated under the same condition? The PodEvictor
was recently extended with pe.metricsEnabled
condition to populate the metrics only when the metrics server is running.
&metrics.CounterOpts{ | ||
Subsystem: DeschedulerSubsystem, | ||
Name: "total_pods_skipped", | ||
Help: "Total pods skipped for a single run", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder what a single run means in this context? Is it meant as a single descheduling cycle? TotalPodsSkipped
metric is used only inside PodEvictor
where there's currently no way to distinguish when a single run finished and a new one started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could introduce a copy of these metrics named something like PodsSkippedLastRun
, for example. It would require some refactoring to the PodEvictor, maybe pass it a timestamp at the start of a new run, and if that timestamp differs from the last one it remembers then reset the counters for those metrics. But I think knowing the totals vs individual runs would be helpful.
I suppose you could get the total by just summing the individual runs, in which case we don't need the Total..
metrics anymore. Either way, I think this is a good start and the refactors could come as a follow up. Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With single run
I meant a single descheduling cycle (somewhat mentioned here in this comment: #503 (comment)). So just wanted to check will it still be a good idea to introduce this metric or not @damemi @ingvagabund
@@ -184,6 +184,7 @@ func evictPod(ctx context.Context, client clientset.Interface, pod *v1.Pod, poli | |||
err := client.PolicyV1beta1().Evictions(eviction.Namespace).Evict(ctx, eviction) | |||
|
|||
if apierrors.IsTooManyRequests(err) { | |||
metrics.TotalPodsSkipped.With(map[string]string{"result": "total pods skipped so far", "namespace": pod.Namespace}).Inc() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting the result
to total pods skipped so far
is redundant here given the metric's name is TotalPodsSkipped
. I wonder if it would make more sense to set the result
to pod skipped due to TooManyRequests error
?
return false, nil | ||
} | ||
|
||
metrics.PodsEvictedSuccess.With(map[string]string{"strategy": strategy, "namespace": pod.Namespace}).Inc() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PodsEvictedSuccess
is a special case of PodsEvicted
(with result
label set to success
and node
label ignored).
metrics.PodsEvicted.With(map[string]string{"result": "error", "strategy": strategy, "namespace": pod.Namespace, "node": node.Name}).Inc() | ||
} | ||
metrics.PodsEvicted.With(map[string]string{"result": "error", "strategy": strategy, "namespace": pod.Namespace, "node": node.Name}).Inc() | ||
metrics.PodsEvictedFailed.With(map[string]string{"strategy": strategy, "namespace": pod.Namespace}).Inc() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PodsEvictedFailed
is a special case of PodsEvicted
(with result
label set to error
and node
label ignored). I wonder what is benefit of creating this metric compared to PodsEvictedFailed
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this was again inclined towards capturing Failed metrics for a single run. Are you suggesting PodsEvictedFailed
as compared to PodsEvicted
metric alone?
metrics.PodsEvicted.With(map[string]string{"result": "maximum number of pods per namespace reached", "strategy": strategy, "namespace": pod.Namespace, "node": node.Name}).Inc() | ||
} | ||
metrics.PodsEvicted.With(map[string]string{"result": "maximum number of pods per namespace reached", "strategy": strategy, "namespace": pod.Namespace, "node": node.Name}).Inc() | ||
metrics.TotalPodsSkipped.With(map[string]string{"result": "total pods skipped so far", "namespace": pod.Namespace}).Inc() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TotalPodsSkipped
called here is a special case of PodsEvicted
(with result
label set to maximum number of pods per namespace reached
and node
label ignored).
return false, fmt.Errorf("Maximum number %v of evicted pods per %q namespace reached", *pe.maxPodsToEvictPerNamespace, pod.Namespace) | ||
} | ||
|
||
err := evictPod(ctx, pe.client, pod, pe.policyGroupVersion) | ||
// increment TotalPodsEvicted | ||
metrics.TotalPodsEvicted.With(map[string]string{"result": "total pods evicted so far", "namespace": pod.Namespace}).Inc() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point it is unknown if a pod was evicted or not. Thus, incrementing the TotalPodsEvicted
does not reflect the actual pod eviction. The metric currently captures how many times the evictPod
function was invoked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, I'll update this part. Thanks!
Within each descheduling cycle a new pod evictor instance is created. Which eliminates any way of telling the pod evictor "a run has ended". The |
@pravarag hey, i was looking into similar metrics around evictions. are you still planning on finishing this pr up or do you need any help? |
Hey @jklaw90 , I'm working on addressing the latest review comments for this PR. If you have any suggestions/comments, please feel free to share on this PR and I'll definitely check those 🙂 . |
@ingvagabund @damemi thanks for the detailed review. So few questions which I have now:
|
@damemi @ingvagabund are we planning to merge these changes as part of release 1.24. If so, I would like you suggestions in moving this forward with all the changes required :) |
@pravarag I would like to merge this, and sorry it's taken so long. I think the only thing still being discussed was the fact that this is reporting cumulative pod evictions rather than single-run as implied. @ingvagabund is that all that was left to sort out? I wonder if maybe a different metric type, like a histogram, could solve this and provide easier access to single-run metrics |
I can give a try by implementing histogram type metric for single-run of pod. |
I wonder if we still need to capture the per single-run metrics given we can use @pravarag I am sorry. I don't think there's anything else left to do for the moment wrt. capturing metrics for a single run. Rather going back to the drawing board and identifying new metrics which we might add to the code. |
Thanks @ingvagabund @damemi for your reviews on this. And like mentioned above, that we may not need to capture metrics for a single run I guess, I can close this PR then? and since I've already put some effort around it, I still don't want to leave it unfinished as this has come out to be a good learning for me as well. Kindly let me know in ways we can rethink on implementing newer metrics as part of this issue? I'm definitely open for a discussion around it be it on this PR itself or maybe going back to the original issue :) |
@pravarag sorry about this. it sounds like we've come back around to not needing single-run metrics. I think per-strategy metrics could be a good option though. what do you think? |
I think that's a good idea @damemi @ingvagabund and I can work on implementing those. But wanted to check how to approach the same? Do I need to open a |
On the other hand, I was thinking if I could just close this PR and open a fresh PR with new (per strategy) changes 🤔 and we can continue in that same issue. |
The new descheduling framework will have more options for introducing new metrics. |
@ingvagabund by |
Opening a new PR sounds reasonable so we can start fresh. The issue can stay open though. |
Closing this PR based on above discussions, will open a fresh one based on newer changes. |
Fixes #503
As part of this change, we are adding newer metrics for improving podEvictor statistics. Currently there is only one metric which is being calculated under: evictions.go