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
Make some scheduler metrics stable #105941
Conversation
Welcome @rezakrimi! |
Hi @rezakrimi. 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 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. |
/ok-to-test This needs a release note |
/retitle Make some scheduler metrics stable |
e842783
to
94cfb0a
Compare
/retest |
94cfb0a
to
b616db7
Compare
/retest |
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.
/approve
/lgtm
for scheduling
@logicalhan Can you please review this? |
@@ -44,5 +44,6 @@ func PodScheduleError(profile string, duration float64) { | |||
|
|||
func observeScheduleAttemptAndLatency(result, profile string, duration float64) { | |||
e2eSchedulingLatency.WithLabelValues(result, profile).Observe(duration) | |||
schedulingLatency.WithLabelValues(result, profile).Observe(duration) |
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'm not super comfortable with setting a metric to stable immediately in the same PR that it is introduced.. can we bake this a little?
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.
Fine with me. FYI @vantuvt
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.
@logicalhan, in case it wasn't clear, this is just a "rename" by duplication+deprecation of old metric.
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.
@logicalhan Also wanted to point out that this kind of renaming+making stable has been done before #99785
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 also don't think we should go through another round of Alpha stability when adjusting a metric before it graduates to stable. As a matter of fact, Kubernetes resources can also be modified during the graduation process to GA.
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 ok...
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.
It would be useful to write some release-note for this change.
d0ebc40
to
197283d
Compare
197283d
to
bb15f02
Compare
Please add a release note |
ahh, you have to put it in between ticks:
|
Done |
/lgtm |
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.
/lgtm
/approve
@@ -44,5 +44,6 @@ func PodScheduleError(profile string, duration float64) { | |||
|
|||
func observeScheduleAttemptAndLatency(result, profile string, duration float64) { | |||
e2eSchedulingLatency.WithLabelValues(result, profile).Observe(duration) | |||
schedulingLatency.WithLabelValues(result, profile).Observe(duration) |
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 ok...
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor, logicalhan, rezakrimi 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 |
/triage accepted |
@wojtek-t is the renamed metric used somehow in the performance dashboards? I couldn't find any usage using github search https://github.com/search?q=org%3Akubernetes+scheduler_e2e_scheduling_duration_seconds&type=code |
What type of PR is this?
/kind cleanup
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #105861
Special notes for your reviewer:
NONE
Does this PR introduce a user-facing change?