Return all scheduler predicate failures instead of the first one #86022

Huang-Wei · 2019-12-07T04:58:49Z

What type of PR is this?

/kind bug
/sig scheduling

What this PR does / why we need it:

Scheduled used to report all failure reasons upon a predicate failure. For example, if a Pod requests excessive cpu and memory, running kubectl describe pod <pod name> will get message:

Events:
  Type     Reason            Age              From               Message
  ----     ------            ----             ----               -------
  Warning  FailedScheduling  8s (x2 over 8s)  default-scheduler  0/1 nodes are available: 1 Insufficient cpu, 1 Insufficient memory.

However, in 1.17, we introduced changes which only returns the first failure reason. For the above example, it reports:

Events:
  Type     Reason            Age        From               Message
  ----     ------            ----       ----               -------
  Warning  FailedScheduling  <unknown>  default-scheduler  0/1 nodes are available: 1 Insufficient cpu.

It's not appropriate since:

users get less error info and may have to resolve the failure several rounds to get it resolved eventually; however, they can get a full failure picture in one glance in the before
internally, scheduler still keeps the logic to calculate all failure reasons; only returns the first failure doesn't help to reduce the memory footprint.

Which issue(s) this PR fixes:

Part of #85918.

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Fixed an issue that the scheduler only returns the first failure reason.

/cc @ahg-g

k8s-ci-robot · 2019-12-07T05:01:20Z

[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:

~~pkg/scheduler/OWNERS~~ [Huang-Wei]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ahg-g · 2019-12-07T12:44:34Z

pkg/scheduler/framework/v1alpha1/interface.go

-	code    Code
-	message string
+	code     Code
+	failures []error


I honestly think this is not proper and confusing. I don't really think we need it either, why not just concatenate the reasons in PredicateResultToFrameworkStatus into a single string that we store in message? This is basically what utilerrors.NewAggregate returns.

For each predicate/plugin, it may have multiple internal (atomic) failure reasons. Suppose the number is n, in theory, there could be 2^n possible aggregated failure. Take PodFitsResources for example, there could be insufficient resource on cpu, memory, ephemeral storage, or extended resource, if we just concatenate the aggregated failures into a string, that string wouldn't be merged properly well with the failures in other nodes into the final histogram message:

For an aggregated string case:

node1: <insufficient cpu, memory, ephemeral stroage>

node2: <insufficient cpu>

node3: <insufficient memory>

The message users can see will be:

0/3 nodes are available: 1 Insufficient cpu, memory, ephemeral stroage, 1 Insufficient cpu, 1 Insufficient memory

NOTE: the message length showed here becomes O(2ⁿ) instead of O(n)..

While with this PR, and also which is consistent with the old behavior:

node1: <insufficient cpu>, <insufficient memory>, <insufficient ephemeral stroage>

node2: <insufficient cpu>

node3: <insufficient memory>

The message users can see will be:

0/3 nodes are available: 2 Insufficient cpu, 2 Insufficient memory, 1 Insufficient ephemeral stroage

ok, so the concern is the number of combinations in the histogram.

Since "error" is not the right type to explain all Status codes (e.g., Unschedulable is not an error), I suggest we have a list of strings instead, we can name it for example reasons []strings?

ahg-g · 2019-12-07T22:55:42Z

pkg/scheduler/framework/v1alpha1/interface.go

-	code    Code
-	message string
+	code     Code
+	failures []error


ok, so the concern is the number of combinations in the histogram.

Since "error" is not the right type to explain all Status codes (e.g., Unschedulable is not an error), I suggest we have a list of strings instead, we can name it for example reasons []strings?

ahg-g · 2019-12-08T03:44:02Z

pkg/scheduler/framework/v1alpha1/interface.go

 		return ""
 	}
-	return s.message
+	return s.AsError().Error()


I believe we need to return the list of reasons/errors so that we can iterate over them when generating the histogram: https://github.com/kubernetes/kubernetes/blob/a73e0f2112d285e4872037428dff8dda55229039/pkg/scheduler/core/generic_scheduler.go#L99

I think we should have two functions, one is Message which concatenates all reasons, and one named Reasons which return the slice. Reasons is used when generating the histogram, and ```Message`` used for logs and all other purposes.

ahg-g · 2019-12-08T03:46:45Z

pkg/scheduler/framework/v1alpha1/interface.go

 }

 // NewStatus makes a Status out of the given arguments and returns its pointer.
 func NewStatus(code Code, msg string) *Status {
 	return &Status{
-		code:    code,
-		message: msg,
+		code:     code,


you can update the function like this: func NewStatus(code Code, reasons ...string) *Status, and so we don't need NewStatusWithFailures

Huang-Wei · 2019-12-08T08:21:02Z

/hold
will squash the commits.

ahg-g

few nits, please also squash.

ahg-g · 2019-12-08T12:53:31Z

pkg/scheduler/algorithm/predicates/error.go

@@ -105,13 +105,14 @@ var unresolvablePredicateFailureErrors = map[PredicateFailureReason]struct{}{

 // UnresolvablePredicateExists checks if there is at least one unresolvable predicate failure reason, if true
 // returns the first one in the list.
-func UnresolvablePredicateExists(reasons []PredicateFailureReason) PredicateFailureReason {
+func UnresolvablePredicateExists(reasons []PredicateFailureReason) []string {


fix the comment

also, how about we just return boolean?

ahg-g · 2019-12-08T12:56:32Z

pkg/scheduler/framework/plugins/migration/utils.go

-		return framework.NewStatus(framework.UnschedulableAndUnresolvable, r.GetReason())
+	var failureReasons []string
+	if failureReasons = predicates.UnresolvablePredicateExists(reasons); len(failureReasons) != 0 {
+		return framework.NewStatus(framework.UnschedulableAndUnresolvable, failureReasons...)


if UnresolvablePredicateExists returns boolean, then we can do this:

code := framework.Unschedulable if predicates.UnresolvablePredicateExists(reasons) { code := framework.UnschedulableAndUnresolvable }

SG. The only side effect is that for some predicate/fitler plugin, the chances are some reasons are UnschedulableAndUnresolvable and some are just Unschedulable, then here we're returning all reasons instead of only UnschedulableAndUnresolvable reasons. But it should be good overall.

I mean: https://github.com/kubernetes/kubernetes/pull/86022/files#diff-fbf6c2939ce03e3e449cf69766ae18aaR57

ahg-g · 2019-12-08T12:56:55Z

pkg/scheduler/framework/plugins/migration/utils.go

+	for _, reason := range reasons {
+		failureReasons = append(failureReasons, reason.GetReason())
+	}
+	return framework.NewStatus(framework.Unschedulable, failureReasons...)


and here: return framework.NewStatus(code, failureReasons...)

ahg-g · 2019-12-08T12:58:02Z

pkg/scheduler/framework/v1alpha1/interface.go

 func (s *Status) Message() string {
-	if s == nil {
+	if s == nil || s.IsSuccess() {


we can keep it as is, we don't need to check for IsSuccess

ahg-g · 2019-12-08T12:58:39Z

pkg/scheduler/framework/v1alpha1/interface.go

 func (s *Status) AsError() error {
-	if s.IsSuccess() {
+	msg := s.Message()
+	if msg == "" {


ditt, we can keep this as is

Huang-Wei · 2019-12-09T06:56:25Z

/retest

ahg-g · 2019-12-09T13:32:41Z

/lgtm

Thanks Wei!

Huang-Wei · 2019-12-09T17:36:54Z

Thanks @ahg-g for reviewing!

Huang-Wei · 2019-12-09T19:32:43Z

/hold cancel

k8s-ci-robot requested a review from ahg-g December 7, 2019 04:58

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 7, 2019

Huang-Wei force-pushed the sched-reserve-multi-errs branch from 6578038 to a73e0f2 Compare December 7, 2019 06:38

ahg-g reviewed Dec 7, 2019

View reviewed changes

ahg-g reviewed Dec 8, 2019

View reviewed changes

Huang-Wei force-pushed the sched-reserve-multi-errs branch from a73e0f2 to 4003f4c Compare December 8, 2019 07:43

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 8, 2019

ahg-g reviewed Dec 8, 2019

View reviewed changes

Huang-Wei force-pushed the sched-reserve-multi-errs branch from 4003f4c to 3dfdab6 Compare December 9, 2019 05:08

Return all predicate failures instead of the first one

a136108

Huang-Wei force-pushed the sched-reserve-multi-errs branch from 3dfdab6 to a136108 Compare December 9, 2019 05:11

k8s-ci-robot assigned ahg-g Dec 9, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 9, 2019

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 9, 2019

k8s-ci-robot merged commit d842c19 into kubernetes:master Dec 9, 2019

k8s-ci-robot added this to the v1.18 milestone Dec 9, 2019

Huang-Wei deleted the sched-reserve-multi-errs branch December 9, 2019 23:29

Huang-Wei mentioned this pull request May 21, 2020

FailedScheduling doesn't report all reasons for nodes failing #91340

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return all scheduler predicate failures instead of the first one #86022

Return all scheduler predicate failures instead of the first one #86022

Huang-Wei commented Dec 7, 2019

k8s-ci-robot commented Dec 7, 2019

ahg-g Dec 7, 2019

Huang-Wei Dec 7, 2019

ahg-g Dec 7, 2019 •

edited

Huang-Wei Dec 8, 2019

ahg-g Dec 7, 2019 •

edited

ahg-g Dec 8, 2019

ahg-g Dec 8, 2019

Huang-Wei commented Dec 8, 2019

ahg-g left a comment

ahg-g Dec 8, 2019

ahg-g Dec 8, 2019

ahg-g Dec 8, 2019

Huang-Wei Dec 9, 2019

Huang-Wei Dec 9, 2019

ahg-g Dec 8, 2019

ahg-g Dec 8, 2019

ahg-g Dec 8, 2019

Huang-Wei commented Dec 9, 2019

ahg-g commented Dec 9, 2019

Huang-Wei commented Dec 9, 2019

Huang-Wei commented Dec 9, 2019

Return all scheduler predicate failures instead of the first one #86022

Return all scheduler predicate failures instead of the first one #86022

Conversation

Huang-Wei commented Dec 7, 2019

k8s-ci-robot commented Dec 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahg-g Dec 7, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahg-g Dec 7, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Huang-Wei commented Dec 8, 2019

ahg-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Huang-Wei commented Dec 9, 2019

ahg-g commented Dec 9, 2019

Huang-Wei commented Dec 9, 2019

Huang-Wei commented Dec 9, 2019

ahg-g Dec 7, 2019 •

edited

ahg-g Dec 7, 2019 •

edited