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
merge MakeDefaultErrorFunc into handleSchedulingFailure #111036
merge MakeDefaultErrorFunc into handleSchedulingFailure #111036
Conversation
@Huang-Wei: 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. |
if err == nil { | ||
// Only tests can reach here. | ||
return | ||
} |
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 section will be removed in the follow-up PR that reuses error string.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
@@ -105,7 +105,7 @@ func TestCoreResourceEnqueue(t *testing.T) { | |||
if fitError == nil { | |||
t.Fatalf("Expect Pod %v to fail at scheduling.", podInfo.Pod.Name) | |||
} | |||
testCtx.Scheduler.Error(podInfo, fitError) | |||
testCtx.Scheduler.FailureHandler(ctx, fwk, podInfo, fitError, v1.PodReasonUnschedulable, nil) |
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 old Error()
is now part of FailureHandler()
and must be called from a Scheduler instance.
pkg/scheduler/scheduler.go
Outdated
if failureHandler != nil { | ||
sched.FailureHandler = failureHandler |
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.
non-nil case is mostly called in UTs where may compose a fake FailureHandler function.
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 think it is better to do it the same way as sched.SchedulePod
:
- remove the parameter completely (it is confusing to pass a nil in the production code)
- do
sched.FailureHandler = failureHandler
- The UT that needs to set the handler to something different can just do that directly on the instance this function returns.
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.
Good point. Updated.
/retest |
50ac4cf
to
0280c3e
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.
looks good to me, squash please?
@@ -810,7 +811,49 @@ func getAttemptsLabel(p *framework.QueuedPodInfo) string { | |||
// handleSchedulingFailure records an event for the pod that indicates the | |||
// pod has failed to schedule. Also, update the pod condition and nominated node name if set. | |||
func (sched *Scheduler) handleSchedulingFailure(ctx context.Context, fwk framework.Framework, podInfo *framework.QueuedPodInfo, err error, reason string, nominatingInfo *framework.NominatingInfo) { | |||
sched.Error(podInfo, err) | |||
pod := podInfo.Pod | |||
if err == ErrNoNodesAvailable { |
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 moved from the other function, but lots of indentation in this segment of the code, perhaps we can try to improve it as a followup.
0280c3e
to
4f77732
Compare
@ahg-g squashed the commits. Thanks for reviewing. |
/retest |
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass. This bot retests PRs for certain kubernetes repos according to the following rules:
You can:
/retest |
What type of PR is this?
/kind cleanup
/sig scheduling
What this PR does / why we need it:
FailureHandler
, and update UTs accordingly.Which issue(s) this PR fixes:
Related with #110871 and #103853.
Special notes for your reviewer:
Does this PR introduce a user-facing change?