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.

Sign up for GitHub

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

Jump to bottom

Fix 'Schedulercache is corrupted' error #55262

Merged
merged 1 commit into from
Nov 8, 2017
Merged

Fix 'Schedulercache is corrupted' error #55262

merged 1 commit into from
Nov 8, 2017

Conversation

liggitt
Copy link
Member

@liggitt liggitt commented Nov 7, 2017 •
edited
Loading

Fixes #50916

If an Assume()ed pod is Add()ed with a different nodeName, the podStates view of the pod is not corrected to reflect the actual nodeName. On the next Update(), the scheduler observes the mismatch and process exits.

Fixed 'Schedulercache is corrupted' error in kube-scheduler

@k8s-ci-robot k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 7, 2017
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 7, 2017
@liggitt liggitt mentioned this pull request Nov 7, 2017
Closed
@liggitt
Copy link
Member Author

liggitt commented Nov 7, 2017

cc @kubernetes/sig-scheduling-bugs @kubernetes/sig-scheduling-pr-reviews

@k8s-ci-robot k8s-ci-robot added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. kind/bug Categorizes issue or PR as related to a bug. labels Nov 7, 2017
@liggitt
Copy link
Member Author

liggitt commented Nov 7, 2017

/retest

@timothysc
Copy link
Member

/assign @wojtek-t

@timothysc timothysc modified the milestone: v1.9 Nov 7, 2017
@timothysc timothysc added this to the v1.8 milestone Nov 7, 2017
timothysc
timothysc approved these changes Nov 7, 2017
View reviewed changes
Copy link
Member

@timothysc timothysc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm - minor comment.

plugin/pkg/scheduler/schedulercache/cache_test.go Outdated
t.Fatalf("AddPod failed: %v", err)
}
}
cache.cleanupAssumedPods(now.Add(2 * ttl))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you up the ttl?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is simulating passing of time to ensure the added pod doesn't expire. was copy/paste from previous testcase, though, and not really relevant to this one, so I can remove it

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 7, 2017
@k8s-cherrypick-bot
Copy link

Removing label cherrypick-candidate because no release milestone was set. This is an invalid state and thus this PR is not being considered for cherry-pick to any release branch. Please add an appropriate release milestone and then re-add the label.

@liggitt
Copy link
Member Author

liggitt commented Nov 7, 2017 •
edited
Loading

updated to remove unrelated ttl bit from unit test

This was referenced Nov 7, 2017
Merged
Merged
@jpbetz jpbetz added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Nov 7, 2017
@jpbetz jpbetz added cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. and removed cherrypick-candidate cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. labels Nov 7, 2017
@cblecker
Copy link
Member

cblecker commented Nov 7, 2017

CRD Flake
/test pull-kubernetes-unit

bsalamat
bsalamat reviewed Nov 7, 2017
View reviewed changes
Copy link
Member

@bsalamat bsalamat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your change looks good to me, I just wonder if Scheduler resource accounting remains valid in such scenario where a pod is assumed to a different node that it is bound to.

plugin/pkg/scheduler/schedulercache/cache_test.go Outdated
}
for _, podToUpdate := range tt.podsToUpdate {
if err := cache.UpdatePod(podToUpdate[0], podToUpdate[1]); err != nil {
t.Fatalf("AddPod failed: %v", err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/AddPod/UpdatePod/

@liggitt
Copy link
Member Author

liggitt commented Nov 7, 2017 •
edited
Loading

fixed gofmt error and test message, re-tagging

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2017
@liggitt
Copy link
Member Author

liggitt commented Nov 7, 2017 •
edited
Loading

I just wonder if Scheduler resource accounting remains valid in such scenario where a pod is assumed to a different node that it is bound to.

It was just the podStates' version of the pod that wasn't getting updated. The removePod(currState.pod)/addPod(pod) dance corrects the per-node accounting already (and the unit test I added demonstrates the per-node accounting is corrected):

kubernetes/plugin/pkg/scheduler/schedulercache/cache.go

Lines 234 to 241 in 454074d

case ok && cache.assumedPods[key]:
if currState.pod.Spec.NodeName != pod.Spec.NodeName {
// The pod was added to a different node than it was assumed to.
glog.Warningf("Pod %v assumed to a different node than added to.", key)
// Clean this up.
cache.removePod(currState.pod)
cache.addPod(pod)
}

@liggitt liggitt added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2017
@cblecker
Copy link
Member

cblecker commented Nov 7, 2017

@liggitt looks like there's still a gofmt issue on ./plugin/pkg/scheduler/schedulercache/cache_test.go

@bsalamat
Copy link
Member

bsalamat commented Nov 7, 2017

Thanks, @liggitt!

/lgtm

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2017
@liggitt
Copy link
Member Author

liggitt commented Nov 7, 2017

actually fixed gofmt error

@cblecker
Copy link
Member

cblecker commented Nov 7, 2017

reapplying LGTM
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, cblecker, liggitt, timothysc

Associated issue: 50916

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@cblecker
Copy link
Member

cblecker commented Nov 7, 2017

uhhh..

W1107 23:53:54.719] Run: ('bash', '-c', 'cd kubernetes && ./hack/jenkins/test-dockerized.sh')
W1107 23:53:54.722] bash: line 0: cd: kubernetes: No such file or directory

That's an odd unit test failure. Let's retry.
/test pull-kubernetes-unit

@cblecker
Copy link
Member

cblecker commented Nov 8, 2017

unit test failing: #55276

@cblecker
Copy link
Member

cblecker commented Nov 8, 2017

fingers crossed
/test pull-kubernetes-unit

@liggitt
Copy link
Member Author

liggitt commented Nov 8, 2017

TestCRD flake
/test pull-kubernetes-unit

jpbetz added a commit that referenced this pull request Nov 8, 2017
@jpbetz
Merge pull request #55266 from liggitt/automated-cherry-pick-of-#5526…
861dab5 
…2-upstream-release-1.8

Automated cherry pick of #55262
@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 33f873d into kubernetes:master Nov 8, 2017
@liggitt liggitt deleted the schedulercache branch November 9, 2017 13:14
k8s-github-robot pushed a commit that referenced this pull request Nov 14, 2017
Merge pull request #55267 from liggitt/automated-cherry-pick-of-#5526…
80ade7f 
…2-upstream-release-1.7

Automatic merge from submit-queue.

Automated cherry pick of #55262

Cherry pick of #55262 on release-1.7.

#55262: Fix 'Schedulercache is corrupted' error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bsalamat bsalamat bsalamat left review comments

@timothysc timothysc timothysc approved these changes

Assignees

@cblecker cblecker

@wojtek-t wojtek-t

@bsalamat bsalamat

@k82cn k82cn

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.8
Development

Successfully merging this pull request may close these issues.
10 participants
@liggitt @timothysc @k8s-cherrypick-bot @cblecker @bsalamat @k8s-github-robot @jpbetz @wojtek-t @k82cn @k8s-ci-robot