Track node removal latency for empty and non-empty nodes#9377
Track node removal latency for empty and non-empty nodes#9377tetianakh wants to merge 1 commit intokubernetes:masterfrom
Conversation
k8s-ci-robot
added
the
do-not-merge/work-in-progress
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsInstructions 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. |
k8s-ci-robot
added
kind/feature
|
Hi @tetianakh. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tetianakh The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
k8s-ci-robot
added
size/M
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
k8s-ci-robot
added
size/L
tetianakh
changed the title
tetianakh
force-pushed
the
was_empty
branch
4 times, most recently
from
7f80e68 to
5d251ee
Compare
|
/assign Choraden |
Choraden
left a comment
Choraden
left a comment
There was a problem hiding this comment.
Thanks @tetianakh! Left some minor comments.
| Help: "Latency from when an unneeded node is eligible for scale down until it is removed (deleted=true) or it became needed again (deleted=false).", | ||
| Buckets: k8smetrics.ExponentialBuckets(1, 1.5, 19), // ~1s → ~24min | ||
| }, []string{"deleted"}, | ||
| }, []string{"deleted", "type"}, |
There was a problem hiding this comment.
"type" is common and very generic. How about node_type or unneeded_type to avoid ambiguity?
| nodeType := metrics.NonEmptyUnneededNode | ||
| if len(v.ntbr.PodsToReschedule) == 0 { | ||
| nodeType = metrics.EmptyUnneededNode | ||
| } |
There was a problem hiding this comment.
How about extracting it to a helper function?
That would make the intent clearer and centralize the definition of emptiness. It would also simplify testing this particular behavior.
There was a problem hiding this comment.
That helper function could be also reused in the candidatesFromNames (from node_latency_tracker_test.go) to be sure that we are testing the right logic.
3 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
This change adds
typelabel to thenode_removal_latency_secondsmetric. This allows to track the scaledown latency of empty and non-empty nodes separately.