-
Notifications
You must be signed in to change notification settings - Fork 38.6k
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: wrap errors for framework/runtime #98266
Conversation
@gavinfish: 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. |
/assign @alculquicondor |
/retest |
@@ -538,7 +538,7 @@ func (f *frameworkImpl) RunFilterPlugins( | |||
if !pluginStatus.IsUnschedulable() { | |||
// Filter plugins are not supposed to return any status other than | |||
// Success or Unschedulable. | |||
errStatus := framework.NewStatus(framework.Error, fmt.Sprintf("running %q filter plugin for pod %q: %v", pl.Name(), pod.Name, pluginStatus.Message())) | |||
errStatus := framework.AsStatus(fmt.Errorf("running %q filter plugin for pod %q: %w", pl.Name(), pod.Name, pluginStatus.AsError())) |
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.
can you trace what happens with the error in higher function calls?
Maybe the name of the Pod is added again at some point.
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.
no, we use the function in both schedule and preempt and they are all clean.
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 found where we log these errors:
kubernetes/pkg/scheduler/scheduler.go
Line 467 in 7315c1f
klog.ErrorS(nil, "Status after running PostFilter plugins for pod", klog.KObj(pod), "status", status) |
kubernetes/pkg/scheduler/scheduler.go
Line 483 in 7315c1f
klog.ErrorS(err, "Error selecting node for pod", "pod", klog.KObj(pod)) |
Both have the Pod, so no need to add it here
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.
Get your point now. Nice catch!
/lgtm |
/retest |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor, gavinfish 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 |
5f93a1e
to
9f8f3b2
Compare
please squash |
9f8f3b2
to
b79c2eb
Compare
squashed |
/lgtm |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Wrap all errors under framework/runtime.
Which issue(s) this PR fixes:
Part of #94695
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: