add preference binpack replicas plugin #295
Conversation
mrlihanbo
force-pushed
the
feat/PreferenceBinpack-2
branch
from
b4172cf to
2aa1c64
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #295 +/- ##
==========================================
+ Coverage 30.42% 31.15% +0.73%
==========================================
Files 120 122 +2
Lines 13944 14266 +322
==========================================
+ Hits 4242 4445 +203
- Misses 9296 9406 +110
- Partials 406 415 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
mrlihanbo
force-pushed
the
feat/PreferenceBinpack-2
branch
4 times, most recently
from
4f4b26c to
61f2525
Compare
| @@ -21,7 +21,7 @@ import ( | |||
| "github.com/kubewharf/kubeadmiral/pkg/controllers/scheduler/framework/plugins/names" | |||
| ) | |||
|
|
|||
| func GetDefaultEnabledPlugins() *fedcore.EnabledPlugins { | |||
| func GetDefaultEnabledPlugins(replicasPlugin string) *fedcore.EnabledPlugins { | |||
There was a problem hiding this comment.
Any chance to mutate the returned EnabledPlugins in the scheduler instead? The default list is the baseline, and should not vary depending on configuration.
| for _, pod := range podList { | ||
| if pod.GetDeletionTimestamp() != nil { | ||
| continue | ||
| } | ||
|
|
||
| scheduledCondition, isUnschedulable := getPodScheduledCondition(pod) | ||
| if !isUnschedulable { | ||
| if scheduledCondition != nil { |
There was a problem hiding this comment.
Should we be more conservative and additionally check that scheduledCondition.Status == True?
There was a problem hiding this comment.
lgtm, I will add the additionally check
| return scheduledReplicas, nil, nil | ||
| } | ||
|
|
||
| // If we don't desiredPlan to avoid migration, just return the plan computed from preferences |
There was a problem hiding this comment.
| // If we don't desiredPlan to avoid migration, just return the plan computed from preferences | |
| // If we don't need to avoid migration, just return the plan computed from preferences |
| if !keepUnschedulableReplicas { | ||
| newOverflow := make(map[string]int64) | ||
| for key, value := range desiredOverflow { | ||
| value = framework.MinInt64(value, totalReplicas-currentTotalScheduledReplicas) |
There was a problem hiding this comment.
Let's add a comment -
// Cap the overflow at currently unscheduled replicas.
| return currentReplicaCount, nil | ||
| } | ||
|
|
||
| func preCheck( |
There was a problem hiding this comment.
| func preCheck( | |
| func shouldPlan( |
| limitedCapacity map[string]int64, | ||
| avoidDisruption bool, | ||
| keepUnschedulableReplicas bool, | ||
| scheduledReplicas map[string]int64, |
There was a problem hiding this comment.
Let's add a comment for this param - Replicas that have been scheduled in the member clusters to prevent the confusion that scheduled here may refer to fed scheduling.
| }() | ||
|
|
||
| for cluster, replicas := range scheduledReplicas { | ||
| if capacity, exist := estimatedCapacity[cluster]; exist && capacity != replicas { |
There was a problem hiding this comment.
Let's add a comment here to explain why we skip planning here
If `capacity != replicas`, some replicas are still being scheduled. We defer planning until the status of all replicas are known.
|
Related to #289 The new PropagationPolicy field Scheduling profile was designed to configure plugins based on the scheduling requirements. Adding This is bad design imo. |
mrlihanbo
force-pushed
the
feat/PreferenceBinpack-2
branch
from
61f2525 to
2ffd8d3
Compare
|
Adding on to @gary-lgy 's point, the scheduling profile used to dictate which fields in This PR introduces the inverse of this, creating a two-way relation between profiles and policies where they can affect one another in elaborate and unexpected ways. This is bad design and a slippery slope in my opinion as well. |
|
I propose an alternative solution. @mrlihanbo @gary-lgy Instead of introducing a new
|
|
Agreed all. But there is one thing, schedulingProfile is indeed more complicated than adding replicaStrategy to policy. I think if preferenceBinpack is a basic feature, it would be better to add replicaStrategy. Otherwise, if it's a extended feature, it would be better to add it in schedulingProfile. During the former discussion, seems we hope it to be a basic feature? |
mrlihanbo
force-pushed
the
feat/PreferenceBinpack-2
branch
from
2ffd8d3 to
863b6ef
Compare
|
To those without context, there was an offline discussion among @manmanhappy @gary-lgy @mrlihanbo @JackZxj and @Poor12 on this topic. @gary-lgy proposed the 2 built-in profiles solution mentioned by @limhawjia above. However, @manmanhappy rejected it and proposed the current solution as implemented in #289 and this PR (and did not leave much room for further discussion). Replying to @Poor12's comment:
From whose perspective? The profiles are built-in (either in code or created by the platform). From the users' perspective, they need to configure a single field in the policy spec, be it
Basic or extended is subjective, and I don't think it's the right yardstick to determine whether a configuration option should become a policy field. There are in fact many considerations for preferring the profile approach:
To be absolutely clear, I maintain my position (which I made clear during the initial offline discussion) that we shouldn't go down this slippery slope of continuously adding new fields. |
add preference binpack replicas plugin
No description provided.