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

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

Merged
merged 1 commit into from Dec 9, 2019

Conversation

@Huang-Wei
Copy link
Member

Huang-Wei commented Dec 7, 2019

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:

  1. 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
  2. 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

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 7, 2019

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Huang-Wei Huang-Wei force-pushed the Huang-Wei:sched-reserve-multi-errs branch from 6578038 to a73e0f2 Dec 7, 2019
code Code
message string
code Code
failures []error

This comment has been minimized.

Copy link
@ahg-g

ahg-g Dec 7, 2019

Member

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.

This comment has been minimized.

Copy link
@Huang-Wei

Huang-Wei Dec 7, 2019

Author Member

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(2n) 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

This comment has been minimized.

Copy link
@ahg-g

ahg-g Dec 8, 2019

Member

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?

This comment has been minimized.

Copy link
@Huang-Wei

Huang-Wei Dec 8, 2019

Author Member

SG.

code Code
message string
code Code
failures []error

This comment has been minimized.

Copy link
@ahg-g

ahg-g Dec 8, 2019

Member

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?

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

This comment has been minimized.

Copy link
@ahg-g

ahg-g Dec 8, 2019

Member

I believe we need to return the list of reasons/errors so that we can iterate over them when generating the histogram:

reasons[status.Message()]++

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.

}

// 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,

This comment has been minimized.

Copy link
@ahg-g

ahg-g Dec 8, 2019

Member

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

@Huang-Wei Huang-Wei force-pushed the Huang-Wei:sched-reserve-multi-errs branch from a73e0f2 to 4003f4c Dec 8, 2019
@Huang-Wei

This comment has been minimized.

Copy link
Member Author

Huang-Wei commented Dec 8, 2019

/hold
will squash the commits.

Copy link
Member

ahg-g left a comment

few nits, please also squash.

@@ -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 {

This comment has been minimized.

Copy link
@ahg-g

ahg-g Dec 8, 2019

Member

fix the comment

This comment has been minimized.

Copy link
@ahg-g

ahg-g Dec 8, 2019

Member

also, how about we just return boolean?

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

This comment has been minimized.

Copy link
@ahg-g

ahg-g Dec 8, 2019

Member

if UnresolvablePredicateExists returns boolean, then we can do this:

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

This comment has been minimized.

Copy link
@Huang-Wei

Huang-Wei Dec 9, 2019

Author Member

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.

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

This comment has been minimized.

Copy link
@ahg-g

ahg-g Dec 8, 2019

Member

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

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

This comment has been minimized.

Copy link
@ahg-g

ahg-g Dec 8, 2019

Member

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

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

This comment has been minimized.

Copy link
@ahg-g

ahg-g Dec 8, 2019

Member

ditt, we can keep this as is

@Huang-Wei Huang-Wei force-pushed the Huang-Wei:sched-reserve-multi-errs branch from 4003f4c to 3dfdab6 Dec 9, 2019
@Huang-Wei Huang-Wei force-pushed the Huang-Wei:sched-reserve-multi-errs branch from 3dfdab6 to a136108 Dec 9, 2019
@Huang-Wei

This comment has been minimized.

Copy link
Member Author

Huang-Wei commented Dec 9, 2019

/retest

@ahg-g

This comment has been minimized.

Copy link
Member

ahg-g commented Dec 9, 2019

/lgtm

Thanks Wei!

@Huang-Wei

This comment has been minimized.

Copy link
Member Author

Huang-Wei commented Dec 9, 2019

Thanks @ahg-g for reviewing!

@Huang-Wei

This comment has been minimized.

Copy link
Member Author

Huang-Wei commented Dec 9, 2019

/hold cancel

@k8s-ci-robot k8s-ci-robot merged commit d842c19 into kubernetes:master Dec 9, 2019
15 checks passed
15 checks passed
cla/linuxfoundation Huang-Wei authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-kind Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Dec 9, 2019
@Huang-Wei Huang-Wei deleted the Huang-Wei:sched-reserve-multi-errs branch Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.