Skip to content

Fix incorrect call to 'bind' in scheduler #50028

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

Merged
k8s-github-robot merged 1 commit into from
Aug 4, 2017
+24 −9
Merged

Fix incorrect call to 'bind' in scheduler #50028

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions plugin/pkg/scheduler/scheduler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,14 +185,14 @@ func (sched *Scheduler) schedule(pod *v1.Pod) (string, error) {
}

// assume signals to the cache that a pod is already in the cache, so that binding can be asnychronous.
func (sched *Scheduler) assume(pod *v1.Pod, host string) error {
// assume modifies `assumed`.
func (sched *Scheduler) assume(assumed *v1.Pod, host string) error {
// Optimistically assume that the binding will succeed and send it to apiserver
// in the background.
// If the binding fails, scheduler will release resources allocated to assumed pod
// immediately.
assumed := *pod
assumed.Spec.NodeName = host
if err := sched.config.SchedulerCache.AssumePod(&assumed); err != nil {
if err := sched.config.SchedulerCache.AssumePod(assumed); err != nil {
glog.Errorf("scheduler cache AssumePod failed: %v", err)
// TODO: This means that a given pod is already in cache (which means it
// is either assumed or already added). This is most probably result of a
Expand All @@ -207,7 +207,7 @@ func (sched *Scheduler) assume(pod *v1.Pod, host string) error {
// predicates in equivalence cache.
// If the binding fails, these invalidated item will not break anything.
if sched.config.Ecache != nil {
sched.config.Ecache.InvalidateCachedPredicateItemForPodAdd(pod, host)
sched.config.Ecache.InvalidateCachedPredicateItemForPodAdd(assumed, host)
}
return nil
}
Expand Down Expand Up @@ -264,15 +264,17 @@ func (sched *Scheduler) scheduleOne() {

// Tell the cache to assume that a pod now is running on a given node, even though it hasn't been bound yet.
// This allows us to keep scheduling without waiting on binding to occur.
err = sched.assume(pod, suggestedHost)
assumedPod := *pod
Copy link
Contributor

@wanghaoran1988 wanghaoran1988 Aug 3, 2017

Choose a reason for hiding this comment

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

Why you make a copy here? seems you moved the copy from inside the assume func here.

Copy link
Contributor

@k82cn k82cn Aug 3, 2017

Choose a reason for hiding this comment

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

we should not use obj from cache directly.

Copy link
Contributor Author

@julia-stripe julia-stripe Aug 3, 2017

Choose a reason for hiding this comment

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

Basically we need to make a copy somewhere and @k82cn suggested it would be more clear if we made the copy outside of the assume function. (which I agree with)

// assume modifies `assumedPod` by setting NodeName=suggestedHost
err = sched.assume(&assumedPod, suggestedHost)
if err != nil {
return
}

// bind the pod to its host asynchronously (we can do this b/c of the assumption step above).
go func() {
err := sched.bind(pod, &v1.Binding{
ObjectMeta: metav1.ObjectMeta{Namespace: pod.Namespace, Name: pod.Name, UID: pod.UID},
err := sched.bind(&assumedPod, &v1.Binding{
Copy link
Member

@bsalamat bsalamat Aug 3, 2017

Choose a reason for hiding this comment

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

Isn't this the same as before.

Copy link
Contributor

@k82cn k82cn Aug 3, 2017

Choose a reason for hiding this comment

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

we used pod directly before the fix; and it was assumedPod before the refactor.

Copy link
Member

@wojtek-t wojtek-t Aug 3, 2017

Choose a reason for hiding this comment

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

It's not the same - assume() method above is modifying the given pod.

Copy link
Member

@bsalamat bsalamat Aug 3, 2017

Choose a reason for hiding this comment

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

I meant this piece of code is logically the same in this PR.

Copy link
Member

@bsalamat bsalamat Aug 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Is sched.assume() going to modify assumedPod?

Copy link
Member

@wojtek-t wojtek-t Aug 3, 2017

Choose a reason for hiding this comment

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

@bsalamat I don't understand this question. Yes it is modifying. Yes it is what we are trying to achieve.

Copy link
Member

@bsalamat bsalamat Aug 3, 2017

Choose a reason for hiding this comment

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

Thanks, @wojtek-t! I don't know why I missed it. I guess github UI still confuses me.

ObjectMeta: metav1.ObjectMeta{Namespace: assumedPod.Namespace, Name: assumedPod.Name, UID: assumedPod.UID},
Target: v1.ObjectReference{
Kind: "Node",
Name: suggestedHost,
Expand Down
11 changes: 10 additions & 1 deletion plugin/pkg/scheduler/scheduler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ func TestScheduler(t *testing.T) {
sendPod *v1.Pod
algo algorithm.ScheduleAlgorithm
expectErrorPod *v1.Pod
expectForgetPod *v1.Pod
expectAssumedPod *v1.Pod
expectError error
expectBind *v1.Binding
Expand All @@ -139,7 +140,8 @@ func TestScheduler(t *testing.T) {
expectAssumedPod: podWithID("foo", testNode.Name),
injectBindError: errB,
expectError: errB,
expectErrorPod: podWithID("foo", ""),
expectErrorPod: podWithID("foo", testNode.Name),
expectForgetPod: podWithID("foo", testNode.Name),
eventReason: "FailedScheduling",
}, {
sendPod: deletingPod("foo"),
Expand All @@ -151,11 +153,15 @@ func TestScheduler(t *testing.T) {
for i, item := range table {
var gotError error
var gotPod *v1.Pod
var gotForgetPod *v1.Pod
var gotAssumedPod *v1.Pod
var gotBinding *v1.Binding
configurator := &FakeConfigurator{
Config: &Config{
SchedulerCache: &schedulertesting.FakeCache{
ForgetFunc: func(pod *v1.Pod) {
gotForgetPod = pod
},
AssumeFunc: func(pod *v1.Pod) {
gotAssumedPod = pod
},
Expand Down Expand Up @@ -196,6 +202,9 @@ func TestScheduler(t *testing.T) {
if e, a := item.expectErrorPod, gotPod; !reflect.DeepEqual(e, a) {
t.Errorf("%v: error pod: wanted %v, got %v", i, e, a)
}
if e, a := item.expectForgetPod, gotForgetPod; !reflect.DeepEqual(e, a) {
t.Errorf("%v: forget pod: wanted %v, got %v", i, e, a)
}
if e, a := item.expectError, gotError; !reflect.DeepEqual(e, a) {
t.Errorf("%v: error: wanted %v, got %v", i, e, a)
}
Expand Down
6 changes: 5 additions & 1 deletion plugin/pkg/scheduler/testing/fake_cache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
// FakeCache is used for testing
type FakeCache struct {
AssumeFunc func(*v1.Pod)
ForgetFunc func(*v1.Pod)
}

func (f *FakeCache) AssumePod(pod *v1.Pod) error {
Expand All @@ -34,7 +35,10 @@ func (f *FakeCache) AssumePod(pod *v1.Pod) error {

func (f *FakeCache) FinishBinding(pod *v1.Pod) error { return nil }

func (f *FakeCache) ForgetPod(pod *v1.Pod) error { return nil }
func (f *FakeCache) ForgetPod(pod *v1.Pod) error {
f.ForgetFunc(pod)
return nil
}

func (f *FakeCache) AddPod(pod *v1.Pod) error { return nil }

Expand Down