-
Notifications
You must be signed in to change notification settings - Fork 1k
feat: the scheduler support SpreadByLabel constraints #3203
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
base: master
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
52f2d5f to
c722d40
Compare
Signed-off-by: chaunceyjiang <[email protected]>
c722d40 to
a23ffdf
Compare
apiVersion: policy.karmada.io/v1alpha1
kind: PropagationPolicy
metadata:
name: nginx
spec:
resourceSelectors:
- apiVersion: apps/v1
kind: Deployment
name: nginx
placement:
clusterAffinity:
labelSelector:
matchExpressions:
- key: stage
operator: In
values:
- dev
- prod
replicaScheduling:
replicaSchedulingType: Divided
replicaDivisionPreference: Weighted
weightPreference:
dynamicWeight: AvailableReplicas
spreadConstraints:
- maxGroups: 2
minGroups: 2
spreadByLabel: stage
|
|
/cc @RainbowMango @Garrybest @jwcesign Please take a look. |
|
/assign |
|
Ping @XiShanYongYe-Chang |
| if _, exist := spreadConstraintMap[policyv1alpha1.SpreadByFieldRegion]; exist { | ||
| return selectBestClustersByRegion(spreadConstraintMap, groupClustersInfo) | ||
| } else if _, exist := spreadConstraintMap[policyv1alpha1.SpreadByFieldCluster]; exist { | ||
| return selectBestClustersByCluster(spreadConstraintMap[policyv1alpha1.SpreadByFieldCluster], groupClustersInfo, needReplicas) |
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.
I have a question about the previous logic.
Here bypass the logic according to spreadByField, what if we have two spread constraints? E.g:
spreadConstraints:
- spreadByField: region
maxGroups: 2
minGroups: 2
- spreadByField: cluster
maxGroups: 2
minGroups: 2This example has two constraints, one is spreadByField: region the other one is spreadByField: cluster. For this case, we are going to select the cluster only by region?
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.
Oh, I get it.
| regions := selectRegions(groupClustersInfo.Regions, spreadConstraintMap[policyv1alpha1.SpreadByFieldRegion], spreadConstraintMap[policyv1alpha1.SpreadByFieldCluster]) |
Here checked the cluster group.
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.
Sorry, I didn't understand the purpose of your question. 'spreadByField' and 'spreadByLabel' cannot co-exist.
| if selector.Matches(labels.Set(clusterInfos[j].Cluster.GetLabels())) { | ||
| selectedClusters = append(selectedClusters, clusterInfos[j]) | ||
| } |
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.
I have a question if two clusters have the same label(same key and value), should they be considered one group or two groups?
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.
one group.
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.
But two clusters that have the same label(same key but different value) should be considered two groups, right?
| } else if hasSpreadByLabel { | ||
| return selectBestClustersByClusterLabels(spreadConstraints, groupClustersInfo, needReplicas) |
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.
Grouping by label and field cannot take effect at the same time?
Such as the below config:
...
spreadConstraints:
- maxGroups: 2
minGroups: 2
spreadByLabel: stage
- maxGroups: 2
minGroups: 2
spreadByField: cluster
...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.
karmada/pkg/util/validation/validation.go
Lines 100 to 103 in 35f3e3f
| // SpreadByField and SpreadByLabel should not co-exist | |
| if len(constraint.SpreadByField) > 0 && len(constraint.SpreadByLabel) > 0 { | |
| allErrs = append(allErrs, field.Invalid(fldPath.Index(index), constraint, "spreadByLabel should not co-exist with spreadByField")) | |
| } |
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 only thing that's forbidden is the same constraint, they're in two constraints in the example.
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.
Thank you for the reminder. Currently, spreadByLabel is being ignored in this situation. Do you have any good suggestions?
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.
I'm sorry, I don't have a good idea yet. I feel that there are many factors to consider when implementing, and it may be complicated to implement.
|
I'm trying to figure out which PR/Issue should be included in the coming v1.7 release which is planned at the end of this month. I'll try to get back on the scheduler in v1.8, let's see if we can move this forward then. |
|
I was about to open the same issue and found that it already exists. Is there any progress on this? Are any additional PRs needed? |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #3187
Special notes for your reviewer:
the scheduler support SpreadByLabel constraints
Does this PR introduce a user-facing change?: