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: fix panic when preempting and evicting allocs #6792

Merged
merged 4 commits into from Dec 3, 2019

Conversation

@schmichael
Copy link
Member

schmichael commented Dec 3, 2019

Sorry about the noisy review. I search/replaced all the noErr helpers to require.NoError. Commit
6112ad9 contains just the fix.

Fixes #6787

In ProposedAllocs the proposed alloc slice was being copied while its
contents were not. Since RemoveAllocs nils elements of the proposed
alloc slice and is called twice, it could panic on the second call when
erroneously accessing a nil'd alloc.

The fix is to not copy the proposed alloc slice and pass the slice
returned by the 1st RemoveAllocs call to the 2nd call, thus maintaining
the trimmed length.

schmichael added 2 commits Dec 3, 2019
Fixes #6787

In ProposedAllocs the proposed alloc slice was being copied while its
contents were not. Since RemoveAllocs nils elements of the proposed
alloc slice and is called twice, it could panic on the second call when
erroneously accessing a nil'd alloc.

The fix is to not copy the proposed alloc slice and pass the slice
returned by the 1st RemoveAllocs call to the 2nd call, thus maintaining
the trimmed length.
@schmichael schmichael added this to the 0.10.2 milestone Dec 3, 2019
@schmichael schmichael requested review from langmartin and preetapan Dec 3, 2019
Copy link
Contributor

langmartin left a comment

👍

Copy link
Member

preetapan left a comment

LGTM

@notnoop

This comment has been minimized.

Copy link
Member

notnoop commented Dec 3, 2019

Code LGTM.

This seems like a serious regression. It would be useful to elaborate in the issue (and upgrade guides) explanation the triggering conditions and affected as well as workarounds (e.g. disabling system preemption for folks that don't need it?).

@schmichael

This comment has been minimized.

Copy link
Member Author

schmichael commented Dec 3, 2019

This seems like a serious regression. It would be useful to elaborate in the issue (and upgrade guides) explanation the triggering conditions and affected as well as workarounds (e.g. disabling system preemption for folks that don't need it?).

The bug has existed since 0.9.0-beta1, so it's somewhat rare to hit thankfully.

I pushed a docs update to document it. It's a bit of a funky place to mention it, but I can't think of anywhere else more appropriate.

Rendered version:

image

@schmichael schmichael merged commit b2dd21a into master Dec 3, 2019
17 of 18 checks passed
17 of 18 checks passed
ci/circleci: test-client Your tests failed on CircleCI
Details
ci/circleci: build-binaries Your tests passed on CircleCI!
Details
ci/circleci: lint-go Your tests passed on CircleCI!
Details
ci/circleci: test-32bit Your tests passed on CircleCI!
Details
ci/circleci: test-api Your tests passed on CircleCI!
Details
ci/circleci: test-devices Your tests passed on CircleCI!
Details
ci/circleci: test-docker Your tests passed on CircleCI!
Details
ci/circleci: test-e2e Your tests passed on CircleCI!
Details
ci/circleci: test-exec Your tests passed on CircleCI!
Details
ci/circleci: test-nomad Your tests passed on CircleCI!
Details
ci/circleci: test-other Your tests passed on CircleCI!
Details
ci/circleci: test-shared-exec Your tests passed on CircleCI!
Details
ci/circleci: test-ui Your tests passed on CircleCI!
Details
ci/circleci: test-website Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
netlify/nomad-ui/deploy-preview UI deployment failed
Details
netlify/nomad-website/deploy-preview website deployment succeeded
Details
@schmichael schmichael deleted the b-propose-panic branch Dec 3, 2019
schmichael added a commit that referenced this pull request Dec 3, 2019
scheduler: fix panic when preempting and evicting allocs
schmichael added a commit that referenced this pull request Dec 4, 2019
schmichael added a commit that referenced this pull request Dec 4, 2019
schmichael added a commit that referenced this pull request Dec 4, 2019
docs: add #6792 backport to 0.9.7 changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.