Skip to content
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.

Sign up for GitHub

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

Jump to bottom

KEP-4247: Update criteria targeting to enable the QueueingHints by default #4877

Merged
merged 1 commit into from
Sep 30, 2024
Merged

KEP-4247: Update criteria targeting to enable the QueueingHints by default #4877

merged 1 commit into from
Sep 30, 2024

Conversation

macsko
Copy link
Member

@macsko macsko commented Sep 26, 2024

  • One-line PR description: Update the metrics and beta criteria as we are targeting to enable the feature by default in v1.32.
  • Other comments:

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 26, 2024
@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. labels Sep 26, 2024
@macsko
Copy link
Member Author

macsko commented Sep 26, 2024

/cc @alculquicondor @sanposhiho

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 26, 2024
@macsko
Copy link
Member Author

macsko commented Sep 26, 2024

Do we want to define more SLOs, e.g. for hints or event handling performance? If yes, I think we will need to come back to them later, when we get some numbers from scheduler_perf test cases that will be added.

Right now, we have only SLOs related directly to scheduling process:

enhancements/keps/sig-scheduling/4247-queueinghint/README.md

Lines 856 to 860 in 4b9728d

In the default scheduler, we should see the throughput around 100-150 pods/s ([ref](https://perf-dash.k8s.io/#/?jobname=gce-5000Nodes&metriccategoryname=Scheduler&metricname=LoadSchedulingThroughput&TestName=load)), and this feature shouldn't bring any regression there.
Based on that:
- `schedule_attempts_total` shouldn't be less than 100 in a second.
- the average of `scheduling_algorithm_duration_seconds` shouldn't be above 10 ms.

alculquicondor
alculquicondor reviewed Sep 27, 2024
View reviewed changes
Copy link
Member

@alculquicondor alculquicondor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make sure we are not missing any questions for the Production Readiness Questionnaire from the latest https://github.com/kubernetes/enhancements/blob/master/keps/NNNN-kep-template/README.md?

keps/sig-scheduling/4247-queueinghint/README.md Outdated Show resolved Hide resolved
keps/sig-scheduling/4247-queueinghint/README.md Outdated Show resolved Hide resolved
keps/sig-scheduling/4247-queueinghint/README.md Outdated Show resolved Hide resolved
keps/sig-scheduling/4247-queueinghint/README.md Outdated Show resolved Hide resolved
keps/sig-scheduling/4247-queueinghint/README.md
- No performance degradation is confirmed via scheduler_perf.
- scheduler_perf covers the performance of most QueueingHintFn for in-tree plugins.
- scheduler_perf runs with QueueingHint both enabled and disabled for all test cases.
- Event handling duration is monitored using scheduler_perf.
- The feature gate is enabled by default.
- No bug report for a while after enabling it by default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have we mentioned any additional criteria that we might consider when going GA?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember any that is not covered in Beta part.

keps/sig-scheduling/4247-queueinghint/README.md Outdated Show resolved Hide resolved
@alculquicondor
Copy link
Member

After addressing all the issues, please assign to @wojtek-t for PRR.

@tjons tjons mentioned this pull request Sep 29, 2024
Open
14 tasks
@wojtek-t wojtek-t self-assigned this Sep 30, 2024
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 30, 2024
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 30, 2024
This was referenced Sep 30, 2024
Open
Open
alculquicondor
alculquicondor reviewed Sep 30, 2024
View reviewed changes
Copy link
Member

@alculquicondor alculquicondor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 30, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, macsko

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 30, 2024
@k8s-ci-robot k8s-ci-robot merged commit 8b162d3 into kubernetes:master Sep 30, 2024
4 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.32 milestone Sep 30, 2024
sanposhiho
sanposhiho reviewed Oct 1, 2024
View reviewed changes
Copy link
Member

@sanposhiho sanposhiho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @macsko!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sanposhiho sanposhiho sanposhiho left review comments

@alculquicondor alculquicondor alculquicondor left review comments

@ahg-g ahg-g Awaiting requested review from ahg-g

@Huang-Wei Huang-Wei Awaiting requested review from Huang-Wei

Assignees

@alculquicondor alculquicondor

@wojtek-t wojtek-t

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
SIG Scheduling
Archived in project
Milestone
v1.32
Development

Successfully merging this pull request may close these issues.
5 participants
@macsko @alculquicondor @k8s-ci-robot @sanposhiho @wojtek-t