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.

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

scheduler: handle and log preemption return error #75369

Open
wants to merge 1 commit into
base: master
from

Conversation

@wgliang
Copy link
Member

wgliang commented Mar 14, 2019

What type of PR is this?
/kind cleanup
/sig scheduling

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Mar 14, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: wgliang
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: k82cn

If they are not already assigned, you can assign the PR to them by writing /assign @k82cn in a comment when ready.

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

@wgliang wgliang force-pushed the wgliang:cleanup/scheduler-cleanup-2 branch from bf3e602 to fc9c702 Mar 14, 2019

@wgliang

This comment has been minimized.

Copy link
Member Author

wgliang commented Mar 14, 2019

/retest

@@ -466,7 +466,9 @@ func (sched *Scheduler) scheduleOne() {
" No preemption is performed.")
} else {
preemptionStartTime := time.Now()
sched.preempt(pod, fitError)
if err := sched.preempt(pod, fitError); err != nil {
klog.Warningf("%v/%v preempting lower priority pods fail,%v", pod.Namespace, pod.Name, err)

This comment has been minimized.

@neolit123

neolit123 Mar 17, 2019

Member

please change to ... pods fail: %v

This comment has been minimized.

@neolit123

neolit123 Mar 17, 2019

Member

what would be the report rate of this warning?
does it make sense to only show it in verbose mode - e.g. V(2)?

This comment has been minimized.

@ravisantoshgudimetla

ravisantoshgudimetla Mar 17, 2019

Contributor

Agreed, increasing log level will ensure we are not aggressively filling logs.

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Mar 17, 2019

/priority backlog

@@ -466,7 +466,9 @@ func (sched *Scheduler) scheduleOne() {
" No preemption is performed.")
} else {
preemptionStartTime := time.Now()
sched.preempt(pod, fitError)
if err := sched.preempt(pod, fitError); err != nil {
klog.Warningf("%v/%v preempting lower priority pods fail,%v", pod.Namespace, pod.Name, err)

This comment has been minimized.

@ravisantoshgudimetla

This comment has been minimized.

@wgliang

wgliang Mar 19, 2019

Author Member

Yes, it will always run metrics.PodScheduleFailures.Inc().

@wgliang wgliang force-pushed the wgliang:cleanup/scheduler-cleanup-2 branch from fc9c702 to 180a3bc Mar 19, 2019

@@ -287,17 +287,17 @@ func (sched *Scheduler) schedule(pod *v1.Pod) (core.ScheduleResult, error) {
// preempt tries to create room for a pod that has failed to schedule, by preempting lower priority pods if possible.
// If it succeeds, it adds the name of the node where preemption has happened to the pod annotations.
// It returns the node name and an error if any.

This comment has been minimized.

@hex108

hex108 Mar 22, 2019

Member

This line also needs change: It returns an error if any.

This comment has been minimized.

@Huang-Wei

Huang-Wei Mar 22, 2019

Member

+1. Please address.

This comment has been minimized.

@wgliang

wgliang Mar 23, 2019

Author Member

DONE

@@ -466,7 +466,9 @@ func (sched *Scheduler) scheduleOne() {
" No preemption is performed.")
} else {
preemptionStartTime := time.Now()
sched.preempt(pod, fitError)
if err := sched.preempt(pod, fitError); err != nil {

This comment has been minimized.

@hex108

hex108 Mar 22, 2019

Member

How about keeping return (node, err) same as before, and printing it if err == nil? It might be useful for debugging.

@Huang-Wei
Copy link
Member

Huang-Wei left a comment

LGTM.

It seems the only caller of preempt() is scheduleOne() and it doesn't rely on the return values.

Will leave /approval to @bsalamat.

@wgliang wgliang force-pushed the wgliang:cleanup/scheduler-cleanup-2 branch from 180a3bc to da284f3 Mar 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.