Enhancements to scheduler priority functions #2906
Conversation
abhgupta
commented
Dec 12, 2014
abhgupta
commented
- Modified the existing spreading priority to consider service pods explicitly
- Added a new priority function to spread pods across zones
|
@bgrant0607 @davidopp @mikedanese please review This will need to be reconciled with #2825 |
abhgupta
force-pushed
the
abhgupta-dev
branch
from
9dec46d
to
d0f789c
Compare
|
This can be refactored into an algorithm provider to reconcile with #2825. It should be pretty straight forward. |
| // spreads pods belonging to the same service across minions | ||
| factory.AddPriority("ServiceSpreadPriority", algorithm.NewServiceSpreadPriority(factory.ServiceLister), 1) | ||
| // spreads pods belonging to the same service across minions in different zones | ||
| factory.AddPriority("ZoneSpreadPriority", algorithm.NewZoneSpreadPriority(factory.ServiceLister, "zone"), 1) |
There was a problem hiding this comment.
Is there precedence for reserving a specific label key? If we do start treating certain labels specially we might want to reference them from a const where the special behavior is documented (possibly like we do for ports in pkg/master/ports/ports.go) as well as documenting this behavior somewhere like the labels doc.
There was a problem hiding this comment.
I didn't really give much thought on how this was supposed to be done, so I just left it for the review. Important thing is that this label is applicable to minions only and should not be user modifiable (ones who create pods/replication controllers/services). This is something that the admin/installer is supposed to deal with and hence reserving a label for this purpose on the minions should be reasonable.
The label key should be configurable but does not need to be documented in a generic manner, since this does not apply to labels generically.
There was a problem hiding this comment.
Another thing is, we wouldn't want this "zone" label to be hardcoded (even as a const) inside factory.go. After the plugin refactoring, this should be made a const inside the algorithm provider that defines the predicates/priorities.
If the above is acceptable, I'd say we let this be for now and change once the plugin refactoring PR merges.
abhgupta
force-pushed
the
abhgupta-dev
branch
2 times, most recently
from
4946746
to
49d49d5
Compare
|
@davidopp Made changes based on initial review feedback - please take another look when you get a chance. |
abhgupta
force-pushed
the
abhgupta-dev
branch
5 times, most recently
from
8363041
to
4f86075
Compare
|
@davidopp @bgrant0607 @mikedanese PTAL |
abhgupta
force-pushed
the
abhgupta-dev
branch
5 times, most recently
from
d61b909
to
0d028ae
Compare
|
This PR is now ready for review. |
|
|
||
| podCounts := map[string]int{} | ||
| numServicePods := len(pods) | ||
| if numServicePods > 0 { |
There was a problem hiding this comment.
Check is not necessary. Iteration over an empty slice is a no-op.
| // Fit is defined based on whether the minion has the specified label values as the pod being scheduled | ||
| // Alternately, if the pod does not specify any/all labels, the other pods in the service are looked at | ||
| factory.RegisterFitPredicate("ServiceAffinity", algorithm.NewServiceAffinityPredicate(factory.PodLister, factory.ServiceLister, factory.MinionLister, []string{"region"})), | ||
| // Fit is defined based on the presence/absence of a label on a minion, regardless of value. |
There was a problem hiding this comment.
Since this is specifically using the "region" label, the comment should probably mention that.
|
@davidopp PTAL. I have made changes to incorporate all the feedback. |
abhgupta
force-pushed
the
abhgupta-dev
branch
2 times, most recently
from
4c5c4a5
to
3da3229
Compare
|
@davidopp This PR is now ready for a final review. |
|
Very sorry for the delay. Will take a look this evening. |
| return antiAffinity.CalculateAntiAffinityPriority | ||
| } | ||
|
|
||
| func (s *ServiceAntiAffinity) CalculateAntiAffinityPriority(pod api.Pod, podLister PodLister, minionLister MinionLister) (HostPriorityList, error) { |
There was a problem hiding this comment.
Add a comment explaining how this priority function works.
|
Other than the comments I made, LGTM! |
- Modified the existing spreading priority to consider service pods explicitly - Added a new priority function to spread pods across zones
abhgupta
force-pushed
the
abhgupta-dev
branch
from
0dd14bb
to
c20d062
Compare
abhgupta
force-pushed
the
abhgupta-dev
branch
from
5240cad
to
dbac18a
Compare
|
@davidopp @bgrant0607 This PR should now be ready for merge. |
|
@davidopp Please merge this if you're happy with it. |
Enhancements to scheduler priority functions
