Skip to content
This repository has been archived by the owner on May 25, 2023. It is now read-only.

RFC: Add a new option --scheduler-name for ignoring pods which should be h… #170

Merged
merged 1 commit into from
Mar 8, 2018

Conversation

mitake
Copy link
Contributor

@mitake mitake commented Feb 21, 2018

…andled by the default scheduler

This commit adds a new option --scheduler-name to
kube-batchd. kube-batchd handles pods which have the name specified
with the option in its v1.Pod.Spec.SchedulerName. The motivation is
separating pods which should be controlled by kube-batchd from other
pods which can be handled by the default scheduler.

I'm still not fully sure this change is valuable for kube-arbitrator's design. If I can have comments, I'm really glad.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 21, 2018
@k82cn
Copy link
Contributor

k82cn commented Feb 28, 2018

xref #164

@@ -88,6 +89,9 @@ func newSchedulerCache(config *rest.Config) *SchedulerCache {
FilterFunc: func(obj interface{}) bool {
switch t := obj.(type) {
case *v1.Pod:
if strings.Compare(obj.(*v1.Pod).Spec.SchedulerName, schedulerName) != 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Only filter Pods with the same scheduler name may be not enough. It may cause kube-batchd schedule pods to a node overload.

Such as some pods from another scheduler are running on a node, however, the node is idle for kube-batchd due to kube-batchd doesn't get those running pods info. When kube-batchd try to schedule pods on that node, it may fail due to lack of resources.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

We should watch 'pending & schedulerName' or 'running' pods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jinzhejz @k82cn thanks for your review. I'll update the condition. How about 'pending & schedulerName' or 'nonTerminatedPod()'?

@mitake
Copy link
Contributor Author

mitake commented Mar 1, 2018

@jinzhejz @k82cn updated the condition, could you take a look?

@@ -88,6 +89,10 @@ func newSchedulerCache(config *rest.Config) *SchedulerCache {
FilterFunc: func(obj interface{}) bool {
switch t := obj.(type) {
case *v1.Pod:
pod := obj.(*v1.Pod)
if strings.Compare(pod.Spec.SchedulerName, schedulerName) == 0 && pod.Status.Phase == v1.PodPending {
Copy link
Contributor

Choose a reason for hiding this comment

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

it also adds pending pods from other scheduler(nonTerminatedPod() will return true)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah I see, will fix it soon, thanks

@mitake
Copy link
Contributor Author

mitake commented Mar 7, 2018

@jinzhejz updated the condition, could you take a look?

@jinzhejz
Copy link
Contributor

jinzhejz commented Mar 8, 2018

@mitake there is build error, could you take a look

# github.com/kubernetes-incubator/kube-arbitrator/pkg/batchd/cache
pkg/batchd/cache/cache.go:90:17: t declared and not used
make: *** [kube-arbitrator] Error 2

And other looks good to me.

@mitake
Copy link
Contributor Author

mitake commented Mar 8, 2018

@jinzhejz oops... really sorry for that, I'll fix it soon.

…andled by the default scheduler

This commit adds a new option --scheduler-name to
kube-batchd. kube-batchd handles pods which have the name specified
with the option in its v1.Pod.Spec.SchedulerName. The motivation is
separating pods which should be controlled by kube-batchd from other
pods which can be handled by the default scheduler.
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 8, 2018
@mitake
Copy link
Contributor Author

mitake commented Mar 8, 2018

@jinzhejz fixed the error, could you take a look?

Copy link
Contributor

@jinzhejz jinzhejz left a comment

Choose a reason for hiding this comment

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

lgtm

@jinzhejz
Copy link
Contributor

jinzhejz commented Mar 8, 2018

@k82cn

@k82cn k82cn merged commit 6434191 into kubernetes-retired:master Mar 8, 2018
@k82cn
Copy link
Contributor

k82cn commented Mar 8, 2018

thanks, team; merged :)

@mitake mitake deleted the sched-name branch March 8, 2018 02:34
kevin-wangzefeng pushed a commit to kevin-wangzefeng/scheduler that referenced this pull request Jun 28, 2019
RFC: Add a new option --scheduler-name for ignoring pods which should be h…
kevin-wangzefeng pushed a commit to kevin-wangzefeng/scheduler that referenced this pull request Jun 28, 2019
RFC: Add a new option --scheduler-name for ignoring pods which should be h…
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants