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
prefer nominated node - IMPL #93179
prefer nominated node - IMPL #93179
Conversation
/cc @Huang-Wei @alculquicondor @ahg-g will try to build a benchmark for this later. |
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
/retest |
@alculquicondor thanks for your review, let's see whether @Huang-Wei and @ahg-g has anything to say about this. |
2559161
to
8c4c9ce
Compare
@chendave: The label(s) In response to this:
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. |
/sig scheduling |
/retest |
2 similar comments
/retest |
/retest |
kubernetes/test/integration/scheduler_perf/config/performance-config.yaml Lines 230 to 235 in 43fbe17
|
@alculquicondor Suppose we go with this new option, should the SchedulerConfig version be bumped to v1beta2 in 1.20? |
@ahg-g @alculquicondor thanks for the review and comments! |
/retest |
/approve approving the feature gate and the core logic, will leave the lgtm to Aldo. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahg-g, chendave 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 |
3648be6
to
f8454b7
Compare
commit squashed, comments addressed! |
/retest |
Signed-off-by: Dave Chen <dave.chen@arm.com>
f8454b7
to
6d800ff
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.
/lgtm
/hold cancel
/retest Review the full test history for this PR. Silence the bot with an |
1 similar comment
/retest Review the full test history for this PR. Silence the bot with an |
/retest |
per the requirement, link this to the enhancement issue: kubernetes/enhancements#1923 |
klog.ErrorS(err, "Evaluation failed on nominated node", "pod", klog.KObj(pod), "node", pod.Status.NominatedNodeName) | ||
} | ||
// Nominated node passes all the filters, scheduler is good to assign this node to the pod. | ||
if len(feasibleNodes) != 0 { |
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.
@chendave Just curious. Could we remove nominatedNode from allNodes
quickly if this result is empty? After that, this node should not go Filters/Extenders twice.
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 original version was removing this node from the node list to avoid the evaluation again later, but this require some slice manipulation, which might be cost more than just evaluate it again.
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.
this is true today, as we don't have too much default filtering list, and cost of the filtering is not dramatic.
Performance evaluation based on the metric of
scheduler_framework_extension_point_duration_seconds/Filter
in the case ofpreemption
when the feature gate is enabled, workloads configuration is 500Nodes, 2000InitPods and 500PodsToSchedule.xref: #96258
legacy metric name is
scheduling_algorithm_predicate_evaluation_seconds
, workloads configuration is 500Nodes, 2000InitPods and 500PodsToSchedule.Signed-off-by: Dave Chen dave.chen@arm.com
What type of PR is this?
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #93013
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
Please use the following format for linking documentation: