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
Refactor schedulingCycle and bindingCycle in scheduler #112025
Refactor schedulingCycle and bindingCycle in scheduler #112025
Conversation
Skipping CI for Draft Pull Request. |
@kerthcet: 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. |
/kind feature |
6f4af4d
to
1e3d18d
Compare
1e3d18d
to
c7cda01
Compare
/retest |
Flaky test #112465 |
This is ready to review. cc @ahg-g @alculquicondor @Huang-Wei |
The flaky test is fixed. |
f3c2ee4
to
d52d36e
Compare
Sorry about the squashed commit for solving code conflicts. |
d52d36e
to
b4b459d
Compare
b4b459d
to
78cfdc3
Compare
pkg/scheduler/schedule_one.go
Outdated
} | ||
sched.FailureHandler(ctx, fwk, assumedPodInfo, sts.AsError(), v1.PodReasonSchedulerError, clearNominatedNode) | ||
return ScheduleResult{}, nil | ||
return ScheduleResult{nominatingInfo: clearNominatedNode, reason: v1.PodReasonSchedulerError, unreserve: true}, |
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.
There is only 2 instances where we call Unreserve and Forget during scheduling cycle. Here and after Permit.
It sounds pretty clean to me to just keep the calls here.
bindingCycle handles it differently, but that's not relevant here. In other words, I think we can keep handleBindingCycleError
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
for nit
pkg/scheduler/schedule_one.go
Outdated
podsToActivate *framework.PodsToActivate) ( | ||
ScheduleResult, *framework.QueuedPodInfo, error) { |
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.
podsToActivate *framework.PodsToActivate) ( | |
ScheduleResult, *framework.QueuedPodInfo, error) { | |
podsToActivate *framework.PodsToActivate, | |
) (ScheduleResult, *framework.QueuedPodInfo, error) { |
maybe this is a bit more readable? But up to you
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.
updated and squashed again.
reason = v1.PodReasonUnschedulable | ||
} | ||
|
||
sched.FailureHandler(ctx, fwk, podInfo, status.AsError(), reason, clearNominatedNode, start) |
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.
Food for thought:
kind of weird that we have to pass an error and a reason.
Perhaps we should be passing a status (although I know it's a bit more tricky for scheduling cycle), or the error should contain information about whether it's unschedulable or not (by implementing Is
)
But this can be left for follow up, if you have extra time.
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 something we can optimize later. I'll follow this when I have time.
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor, kerthcet 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 |
Signed-off-by: kerthcet <kerthcet@gmail.com>
a076be2
to
f7f8578
Compare
/retest |
/hold cancel |
What type of PR is this?
/kind cleanup
/sig scheduling
What this PR does / why we need it:
Which issue(s) this PR fixes:
Part of #103853
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: