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-1819: notes on the order of extenders #2132

Merged
merged 1 commit into from
Jan 7, 2021
Merged

KEP-1819: notes on the order of extenders #2132

merged 1 commit into from
Jan 7, 2021

Conversation

cofyc
Copy link
Member

@cofyc cofyc commented Nov 6, 2020

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 6, 2020
@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 Nov 6, 2020
@cofyc cofyc changed the title notes on the order of extenders KEP-1819: notes on the order of extenders Nov 6, 2020
@cofyc cofyc force-pushed the notes-on-order-of-extenders branch from 7b7dafd to 95d7aac Compare November 6, 2020 10:26
@cofyc
Copy link
Member Author

cofyc commented Nov 6, 2020

/assign @ahg-g

@kikisdeliveryservice kikisdeliveryservice mentioned this pull request Dec 3, 2020
Closed
Huang-Wei
Huang-Wei reviewed Jan 6, 2021
View reviewed changes
keps/sig-scheduling/1819-scheduler-extender/README.md Outdated
Comment on lines 188 to 193
Multiple extenders can be configured and will be called sequentially in the
scheduler framework. Like the scheduler framework running filter plugins, which
will stop on the first filter that returns `Unschedulable` status,
unschedulable nodes will be not be passed to subsequent extenders. Users are
encouraged to carefully order extenders to have the extender which may report
`UnschedulableAndUnresolvable` first. This can improve preemption latency.
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit vague to me, I think we should separate the explanation of "extenders are running sequentially" and "semantics of failedAndUnresolvable" - the previous one was already known; while the latter one is new and should be emphasized. See kubernetes/kubernetes#92866 (comment).

Copy link
Member Author

Choose a reason for hiding this comment

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

updated

I add a note about the order of multiple extenders in the "Configuration" section. The explanation of "semantics of failedAndUnresolvable" is moved to the "Interface > Filter" section, right after the spec of the ExtenderFilterResult struct.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds better. Some comments below.

@cofyc cofyc force-pushed the notes-on-order-of-extenders branch from 95d7aac to a7ce976 Compare January 6, 2021 07:26
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 6, 2021
Huang-Wei
Huang-Wei reviewed Jan 6, 2021
View reviewed changes
Copy link
Member

@Huang-Wei Huang-Wei left a comment

Choose a reason for hiding this comment

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

Some nits.

keps/sig-scheduling/1819-scheduler-extender/README.md Outdated
Comment on lines 243 to 245
When multiple extenders are configured, unschedulable nodes will not be passed
to subsequent extenders. This is like the scheduler framework running filter
plugins, which stops on the first filter that reports unschedulable status.
Copy link
Member

Choose a reason for hiding this comment

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

These lines can be removed as extender used to work the same way.

keps/sig-scheduling/1819-scheduler-extender/README.md Outdated
When multiple extenders are configured, unschedulable nodes will not be passed
to subsequent extenders. This is like the scheduler framework running filter
plugins, which stops on the first filter that reports unschedulable status.
Users are encouraged to carefully order extenders to have the extender which
Copy link
Member

Choose a reason for hiding this comment

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

It's recommended to order the extenders which may report UnschedulableAndUnresolvable ahead of others. This can improve the preemption efficiency.

@cofyc cofyc force-pushed the notes-on-order-of-extenders branch from a7ce976 to 8f43f44 Compare January 7, 2021 01:55
@cofyc
Copy link
Member Author

cofyc commented Jan 7, 2021 •
edited
Loading

comments addressed, diff for quick review

diff --git a/keps/sig-scheduling/1819-scheduler-extender/README.md b/keps/sig-scheduling/1819-scheduler-extender/README.md
index c8a85526..239081f3 100644
--- a/keps/sig-scheduling/1819-scheduler-extender/README.md
+++ b/keps/sig-scheduling/1819-scheduler-extender/README.md
@@ -241,11 +241,9 @@ unschedulable, except the nodes in the latter will be skipped in preemption
 phase.
 
 When multiple extenders are configured, unschedulable nodes will not be passed
-to subsequent extenders. This is like the scheduler framework running filter
-plugins, which stops on the first filter that reports unschedulable status.
-Users are encouraged to carefully order extenders to have the extender which
-may report `UnschedulableAndUnresolvable` first. This can improve preemption
-latency.
+to subsequent extenders. It's recommended to order the extenders which may
+report `UnschedulableAndUnresolvable` ahead of others. This can improve the
+preemption efficiency.
 
 #### Prioritize

ahg-g
ahg-g reviewed Jan 7, 2021
View reviewed changes
Copy link
Member

@ahg-g ahg-g left a comment

Choose a reason for hiding this comment

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

a couple of nits

keps/sig-scheduling/1819-scheduler-extender/README.md Outdated Show resolved Hide resolved
keps/sig-scheduling/1819-scheduler-extender/README.md Outdated Show resolved Hide resolved
keps/sig-scheduling/1819-scheduler-extender/README.md Outdated Show resolved Hide resolved
keps/sig-scheduling/1819-scheduler-extender/README.md Outdated Show resolved Hide resolved
@cofyc cofyc force-pushed the notes-on-order-of-extenders branch from 430661a to 78078f0 Compare January 7, 2021 15:25
@cofyc
Copy link
Member Author

cofyc commented Jan 7, 2021

suggestions are applied, thanks!

@ahg-g
Copy link
Member

ahg-g commented Jan 7, 2021

/lgtm

leaving it to Wei to approve.

@ahg-g
Copy link
Member

ahg-g commented Jan 7, 2021

/hold

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jan 7, 2021
@Huang-Wei
Copy link
Member

/approve
/hold cancel

Thanks @cofyc .

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 7, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cofyc, Huang-Wei

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 Jan 7, 2021
@k8s-ci-robot k8s-ci-robot merged commit 35069fd into kubernetes:master Jan 7, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.21 milestone Jan 7, 2021
@cofyc cofyc deleted the notes-on-order-of-extenders branch February 4, 2021 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahg-g ahg-g ahg-g left review comments

@Huang-Wei Huang-Wei Huang-Wei left review comments

Assignees

@ahg-g ahg-g

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
None yet
Milestone
v1.21
Development

Successfully merging this pull request may close these issues.
4 participants
@cofyc @ahg-g @Huang-Wei @k8s-ci-robot