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

fix scheduler crash when Prioritize Map function failed #68563

Merged
merged 1 commit into from Sep 27, 2018

Conversation

@DylanBLE
Contributor

DylanBLE commented Sep 12, 2018

What this PR does / why we need it:
Fix a bug causing scheduler crash

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #68561

Special notes for your reviewer:

Release note:

Fix scheduler crashes when Prioritize Map function returns error.
@DylanBLE

This comment has been minimized.

Show comment
Hide comment
@DylanBLE

DylanBLE Sep 12, 2018

Contributor

/assign sig-scheduling-maintainers

Contributor

DylanBLE commented Sep 12, 2018

/assign sig-scheduling-maintainers

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Sep 12, 2018

Contributor

@DylanBLE: GitHub didn't allow me to assign the following users: sig-scheduling-maintainers.

Note that only kubernetes members and repo collaborators can be assigned.
For more information please see the contributor guide

In response to this:

/assign sig-scheduling-maintainers

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.

Contributor

k8s-ci-robot commented Sep 12, 2018

@DylanBLE: GitHub didn't allow me to assign the following users: sig-scheduling-maintainers.

Note that only kubernetes members and repo collaborators can be assigned.
For more information please see the contributor guide

In response to this:

/assign sig-scheduling-maintainers

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.

@DylanBLE

This comment has been minimized.

Show comment
Hide comment
@DylanBLE
Contributor

DylanBLE commented Sep 12, 2018

@neolit123

This comment has been minimized.

Show comment
Hide comment
@neolit123

neolit123 Sep 12, 2018

Member

/kind bug
/ok-to-test

Member

neolit123 commented Sep 12, 2018

/kind bug
/ok-to-test

@neolit123

This comment has been minimized.

Show comment
Hide comment
@neolit123

neolit123 Sep 12, 2018

Member

@DylanBLE could you please add a release note explaining your bug fix.

Member

neolit123 commented Sep 12, 2018

@DylanBLE could you please add a release note explaining your bug fix.

@k82cn

This comment has been minimized.

Show comment
Hide comment
Member

k82cn commented Sep 13, 2018

@DylanBLE

This comment has been minimized.

Show comment
Hide comment
@DylanBLE

DylanBLE Sep 13, 2018

Contributor

/retest

Contributor

DylanBLE commented Sep 13, 2018

/retest

@DylanBLE

This comment has been minimized.

Show comment
Hide comment
@DylanBLE

DylanBLE Sep 13, 2018

Contributor

/retest

Contributor

DylanBLE commented Sep 13, 2018

/retest

@DylanBLE

This comment has been minimized.

Show comment
Hide comment
@DylanBLE

DylanBLE Sep 13, 2018

Contributor

/retest

Contributor

DylanBLE commented Sep 13, 2018

/retest

@DylanBLE

This comment has been minimized.

Show comment
Hide comment
@DylanBLE

DylanBLE Sep 15, 2018

Contributor

/test pull-kubernetes-e2e-kops-aws

Contributor

DylanBLE commented Sep 15, 2018

/test pull-kubernetes-e2e-kops-aws

@DylanBLE

This comment has been minimized.

Show comment
Hide comment
@DylanBLE

DylanBLE Sep 18, 2018

Contributor

/milestone v1.12

Contributor

DylanBLE commented Sep 18, 2018

/milestone v1.12

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Sep 18, 2018

Contributor

@DylanBLE: You must be a member of the kubernetes/kubernetes-milestone-maintainers github team to set the milestone.

In response to this:

/milestone v1.12

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.

Contributor

k8s-ci-robot commented Sep 18, 2018

@DylanBLE: You must be a member of the kubernetes/kubernetes-milestone-maintainers github team to set the milestone.

In response to this:

/milestone v1.12

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.

@yastij

yastij approved these changes Sep 18, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot removed the lgtm label Sep 22, 2018

if priorityConfigs[i].Function != nil {
continue
}
results[i][index], err = priorityConfigs[i].Map(pod, meta, nodeInfo)
if err != nil {
appendError(err)
return

This comment has been minimized.

@Huang-Wei

Huang-Wei Sep 24, 2018

Member

please keep the return, as it's in the closure, so is as design.

@Huang-Wei

Huang-Wei Sep 24, 2018

Member

please keep the return, as it's in the closure, so is as design.

This comment has been minimized.

@DylanBLE

DylanBLE Sep 24, 2018

Contributor

If results[i] returns with error, return will cause results[i+1] to be empty.

@DylanBLE

DylanBLE Sep 24, 2018

Contributor

If results[i] returns with error, return will cause results[i+1] to be empty.

This comment has been minimized.

@Huang-Wei

Huang-Wei Sep 24, 2018

Member

I see.

@Huang-Wei
@Huang-Wei

/lgtm

cc @bsalamat for another look.

@k8s-ci-robot k8s-ci-robot removed the lgtm label Sep 26, 2018

@DylanBLE

This comment has been minimized.

Show comment
Hide comment
@DylanBLE

DylanBLE Sep 26, 2018

Contributor

/retest

Contributor

DylanBLE commented Sep 26, 2018

/retest

@bsalamat

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Sep 26, 2018

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Sep 26, 2018

Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, DylanBLE, Huang-Wei, ravisantoshgudimetla, yastij

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

Contributor

k8s-ci-robot commented Sep 26, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, DylanBLE, Huang-Wei, ravisantoshgudimetla, yastij

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

@DylanBLE

This comment has been minimized.

Show comment
Hide comment
@DylanBLE

DylanBLE Sep 27, 2018

Contributor

@bsalamat Could you please remove the /hold label and add a milestone for v1.12? Thanks.

Contributor

DylanBLE commented Sep 27, 2018

@bsalamat Could you please remove the /hold label and add a milestone for v1.12? Thanks.

@k82cn

This comment has been minimized.

Show comment
Hide comment
@k82cn

k82cn Sep 27, 2018

Member

/hold cancel

Member

k82cn commented Sep 27, 2018

/hold cancel

@k82cn

This comment has been minimized.

Show comment
Hide comment
@k82cn

k82cn Sep 27, 2018

Member

add a milestone for v1.12

Please create another cherry pick PR for 1.12; it will be included in 1.12.1. IMO, we should cherry pick this to avoid crash.

Member

k82cn commented Sep 27, 2018

add a milestone for v1.12

Please create another cherry pick PR for 1.12; it will be included in 1.12.1. IMO, we should cherry pick this to avoid crash.

@DylanBLE

This comment has been minimized.

Show comment
Hide comment
@DylanBLE

DylanBLE Sep 27, 2018

Contributor

/test pull-kubernetes-integration

Contributor

DylanBLE commented Sep 27, 2018

/test pull-kubernetes-integration

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Sep 27, 2018

Contributor

@DylanBLE: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-kops-aws f33c2c1 link /test pull-kubernetes-e2e-kops-aws
pull-kubernetes-integration f33c2c1 link /test pull-kubernetes-integration

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

Contributor

k8s-ci-robot commented Sep 27, 2018

@DylanBLE: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-kops-aws f33c2c1 link /test pull-kubernetes-e2e-kops-aws
pull-kubernetes-integration f33c2c1 link /test pull-kubernetes-integration

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot merged commit a6bc5aa into kubernetes:master Sep 27, 2018

18 checks passed

cla/linuxfoundation DylanBLE authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
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-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details

k8s-ci-robot added a commit that referenced this pull request Sep 30, 2018

Merge pull request #69135 from DylanBLE/dev
Automated cherry pick of #68563: fix scheduler crash when Prioritize Map function failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment