-
Notifications
You must be signed in to change notification settings - Fork 500
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
new algorithm for scheduler HA predicate #260
Conversation
/run-e2e-tests |
/run-e2e-tests |
pkg/scheduler/predicates/ha_test.go
Outdated
}, | ||
}, | ||
{ | ||
name: "one pod, podName is wrong", | ||
ordinal: 0, | ||
name: "one node, return zero node", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name: "one node, return zero node", | |
name: "zero node, return zero node", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Co-Authored-By: weekface <weekface@gmail.com>
if !minInitialized { | ||
minInitialized = true | ||
min = count | ||
min := -1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it better to initialize the min variable to a large enough value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Large enough, how big is it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default number of pods that can be run on a kubelet is 110, so I think it's okay to be larger than this number, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This number may be modified by the user. -1 is good enough i think.
…perator into new-scheduler-ha
/run-e2e-tests |
pkg/scheduler/predicates/ha_test.go
Outdated
@@ -39,229 +40,327 @@ func TestMapNil(t *testing.T) { | |||
|
|||
g.Expect(m["c"] == nil).To(Equal(true)) | |||
g.Expect(m["a"] == nil).To(Equal(false)) | |||
|
|||
var i *int | |||
g.Expect(i).To(BeNil()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this?
pkg/scheduler/predicates/ha_test.go
Outdated
g.Expect(i).To(BeNil()) | ||
} | ||
|
||
func TestSortEmptyArr(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of this test?
/run-e2e-tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
fixes #235
@tennix @onlymellb @xiaojingchen PTAL