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
Expose node status so that external preemption plugins can use it #98129
Conversation
@chendave: This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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. |
/release-note none |
/release-note-none |
/sig scheduling |
@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. |
pkg/scheduler/framework/plugins/defaultpreemption/default_preemption.go
Outdated
Show resolved
Hide resolved
008d8ac
to
eee8f21
Compare
@adtac thanks for the review! |
/cc @ahg-g |
pkg/scheduler/framework/plugins/defaultpreemption/default_preemption.go
Outdated
Show resolved
Hide resolved
return "", err | ||
} | ||
if len(candidates) == 0 { | ||
return "", &framework.FitError{ |
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's not clear to me that how this returned obj is utilized by subsequent PostFilter plugins.
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 PR intends to change the public method of FindCandidates
to return node status
, so that the calling of this method could get the node status as well.
Quote from @ahg-g 's initial comments,
This is just so external preemption plugins that use this interface can benefit from this info, but this is less critical for this PR, so I guess we can leave this here for now.
As to the return FitError
here, it's used to populate the log message on the preemption failure. I did some refactoring to move this obj from line 224 to here, the reason is I need to add the unschedulableNodeStatus
into the status or else user might confuse that they have for example they have 3 nodes in the cluster, but only give hint on one node that doesn't have enough resource but no idea on what's happening on other two nodes (please see my update on the testcases).
In short, it is sill used by log population.
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.
so that the calling of this method could get the node status as well.
Well, the current version is not stateless so arguably I doubt if users would like to use it... But I see your point.
pkg/scheduler/framework/plugins/defaultpreemption/default_preemption.go
Outdated
Show resolved
Hide resolved
pkg/scheduler/framework/plugins/defaultpreemption/default_preemption.go
Outdated
Show resolved
Hide resolved
pkg/scheduler/framework/plugins/defaultpreemption/default_preemption.go
Outdated
Show resolved
Hide resolved
pkg/scheduler/framework/plugins/defaultpreemption/default_preemption_test.go
Show resolved
Hide resolved
Signed-off-by: Dave Chen <dave.chen@arm.com>
88d3f81
to
58eb969
Compare
rebased and comments has been addressed. |
@chendave LGTM. Could you squash the commits? |
58eb969
to
ffe3f41
Compare
Signed-off-by: Dave Chen <dave.chen@arm.com>
ffe3f41
to
fe5a266
Compare
Squashed commits into 2. |
/retest |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chendave, 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 |
/retest |
What type of PR is this?
Add one of the following kinds:
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
This is following up the comments from here: #97184 (comment)
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: