-
Notifications
You must be signed in to change notification settings - Fork 74
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
Add placement DecisionStrategy API #242
Add placement DecisionStrategy API #242
Conversation
cluster/v1beta1/types_placement.go
Outdated
// Label to be added to the created placement Decisions | ||
// +kubebuilder:validation:Required | ||
// +required | ||
Label string `json:"labels,omitempty"` |
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 does the value of this field look like
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 is label value so the placementDecision will have label key as "cluster.open-cluster-management.io/decisiongrouplabel" and the user define here the 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.
I see, label may cause confusion. May be better to change the field to name or groupName as we don't want the user to add multiple labels.
cluster/v1beta1/types_placement.go
Outdated
|
||
// Decision group label that is defined in the DecisionStrategy's DecisionGroup. | ||
// +optional | ||
DecisionGroupLabel string `json:"decisionGroupLabel"` |
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.
shouldn't this contain key/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.
the key is API constant "cluster.open-cluster-management.io/decisiongrouplabel"
cluster/v1beta1/types_placement.go
Outdated
// +kubebuilder:validation:Enum=None;"5%";"10%";"15%";"20%";"25%";"30%";"50%";"100%" | ||
// +kubebuilder:default:=None | ||
// +optional | ||
ClusterPercentagePerDecisionGroup string `json:"clusterPercentagePerDecisionGroup,omitempty"` |
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.
It is actually MaxCountPerGroup right? I still think we should only have one field that can be set as IntOrString
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.
okay, I wasn't sure that we can define enum and int for type IntOrString, will push a fix for 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.
e911c19
to
02c9225
Compare
PlacementLabel string = "cluster.open-cluster-management.io/placement" | ||
// decision group index. | ||
DecisionGroupIndexLabel string = "cluster.open-cluster-management.io/decisiongroupindex" |
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.
DecisionGroupIndexLabel string = "cluster.open-cluster-management.io/decisiongroupindex" | |
DecisionGroupIndexLabel string = "cluster.open-cluster-management.io/decision-group-index" |
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.
done
// decision group index. | ||
DecisionGroupIndexLabel string = "cluster.open-cluster-management.io/decisiongroupindex" | ||
// decision group label. | ||
DecisionGroupName string = "cluster.open-cluster-management.io/decisiongroupname" |
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.
DecisionGroupName string = "cluster.open-cluster-management.io/decisiongroupname" | |
DecisionGroupName string = "cluster.open-cluster-management.io/decision-group-name" |
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.
kind of confusing the label key is decision-group-name not decision-group-labelvalue ? if yes, better to keep the fields up as group Name then ?
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 label key is decision-group-name, while what you want user to set is the label value of this key. Group name is not clear since it may imply this is a resource name or a certain field, but it is actually a label value of the certain key
cluster/v1beta1/types_placement.go
Outdated
// +kubebuilder:validation:Enum=None;"5%";"10%";"15%";"20%";"25%";"30%";"50%";"100%" | ||
// +kubebuilder:default:=None | ||
// +optional | ||
ClusterPercentagePerDecisionGroup string `json:"clusterPercentagePerDecisionGroup,omitempty"` |
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.
cluster/v1beta1/types_placement.go
Outdated
} | ||
|
||
// Group the created placementDecision into decision groups based on the number of clusters per decision group. | ||
type DecisionStrategy struct { |
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 would think this like
type DecisionStrategy struct {
GroupStrategy GroupStrategy
}
type GroupStrategy struct {
DecisionGroups []DecisionGroup `json:"decisionGroups,omitempty"`
ClustersPerDecisionGroup string `json:"clustersPerDecisionGroup,omitempty"`
}
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.
Do we need to have nested struct ? I believe decisionStrategy as single struct suppose to be good unless we will extended with something related.
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.
nested will help us to extend in the future
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.
done
cluster/v1beta1/types_placement.go
Outdated
DecisionStrategy DecisionStrategy `json:"decisionStrategy,omitempty"` | ||
} | ||
|
||
// Subset of the created placementDecision with label. |
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.
we should add this the clusters will be put into the decisions with a decision group label .....
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.
done
cluster/v1beta1/types_placement.go
Outdated
@@ -94,6 +95,40 @@ type PlacementSpec struct { | |||
// certain taints to be selected by placements with matching tolerations. | |||
// +optional | |||
Tolerations []Toleration `json:"tolerations,omitempty"` | |||
|
|||
// Decision Strategy divide the created placement decision to groups and define number of clusters per decision 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.
// Decision Strategy divide the created placement decision to groups and define number of clusters per decision group. | |
// Decision Strategy divide the created placement decision to groups and define number of clusters per decision group. Decision result will be sorted by name and filled into decisions if it is not set. |
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.
done
cluster/v1beta1/types_placement.go
Outdated
|
||
// Group the created placementDecision into decision groups based on the number of clusters per decision group. | ||
type DecisionStrategy struct { | ||
// Define subsets of the created placement decisions and add labels to the placementDecision. |
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.
// Define subsets of the created placement decisions and add labels to the placementDecision. | |
// DecisionGroups represents a list of predefined groups to put decision results. |
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.
done
// ClustersPerDecisionGroup is a specific number or percentage of the total selected clusters. | ||
// The specific number will divide the placementDecisions to decisionGroups each group has max number of clusters equal to that specific number. | ||
// The percentage will divide the placementDecisions to decisionGroups each group has max number of clusters based on the total num of selected clusters and percentage. | ||
// ex; for a total 100 clusters selected, ClustersPerDecisionGroup equal to 20% will divide the placement decision to 5 groups each group should have 20 clusters. |
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.
should clarify that If DecisionGroups is set, the matched decision result will be put into them at first. The left decision results will be put into the decision groups based on this field
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.
done
|
||
// Decision group name that is defined in the DecisionStrategy's DecisionGroup. | ||
// +optional | ||
DecisionGroupName string `json:"decisionGroupName"` |
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.
DecisionGroupName string `json:"decisionGroupName"` | |
GroupLabelValue string `json:"groupLableValue"` |
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 change it above to groupName instead of name/labelValue and mention that in the CRD definition L106
cluster/v1beta1/types_placement.go
Outdated
|
||
// List of placement decisions names associated with the decision group | ||
// +optional | ||
PlacementDecisions []string `json:"placementDecisions"` |
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.
Decisions
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.
done
03ac89d
to
5d71971
Compare
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 added the rolloutStrategy struct to the helper class as per discussion with ACM teams. The rolloutStrategy struct will be part and consumed by other APIs like manifestWorkReplicatSet, policy, Addon,...etc
cluster/v1beta1/types_placement.go
Outdated
} | ||
|
||
// Group the created placementDecision into decision groups based on the number of clusters per decision group. | ||
type DecisionStrategy struct { |
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.
Do we need to have nested struct ? I believe decisionStrategy as single struct suppose to be good unless we will extended with something related.
|
||
// Decision group name that is defined in the DecisionStrategy's DecisionGroup. | ||
// +optional | ||
DecisionGroupName string `json:"decisionGroupName"` |
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 change it above to groupName instead of name/labelValue and mention that in the CRD definition L106
cluster/v1beta1/types_placement.go
Outdated
// ex; for a total 100 clusters selected, ClustersPerDecisionGroup equal to 20% will divide the placement decision to 5 groups each group should have 20 clusters. | ||
// Default is having all clusters in a single group. | ||
// If the DecisionGroups field defined, it will be considered first to create the decisionGroups then the ClustersPerDecisionGroup will be used to determine the rest of decisionGroups. | ||
// +kubebuilder:validation:Enum="5%";"10%";"15%";"20%";"25%";"30%";"40%";"50%";"100%" |
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.
do not use enum here. you can have a regex to validate
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.
done
// Group name to be added to the created placement Decisions labels with label key cluster.open-cluster-management.io/decision-group-name | ||
// +kubebuilder:validation:Required | ||
// +required | ||
GroupName string `json:"groupName,omitempty"` |
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.
add regex validation rule here, since it is a label 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.
done
cluster/v1beta1/types_placement.go
Outdated
|
||
// DecisionGroup define a subset of clusters that will be added to placementDecisions with groupName label. | ||
type DecisionGroup struct { | ||
// Group name to be added to the created placement Decisions labels with label key cluster.open-cluster-management.io/decision-group-name |
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.
Group name to be added as the value of the created PlacementDecisions labels with key ...
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.
done
e15d8b3
to
bed4557
Compare
/approve I am good with this change @haoqing0110 @zhiweiyin318 please take a final look and I think we can tag it. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qiujian16, serngawy The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
cluster/v1beta1/types_placement.go
Outdated
// +kubebuilder:validation:Pattern="^((100|[0-9]{1,2})%|[0-9]+)$" | ||
// +kubebuilder:default:="100%" | ||
// +optional | ||
ClustersPerDecisionGroup string `json:"clustersPerDecisionGroup,omitempty"` |
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.
Using intstr.IntOrString
here?
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 believe the validation pattern is good enough to do the same check.
actually I tried to set the type as intOrString but it gives me error while trying to build the API (error about having the validation pattern 100|[0-9]% with intOrString type) it did not build successful.
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.
Using string should be fine from the perspective of functional implementation.
One benefit of using IntOrString
I can think is that it's in line with user habits, like k8s deployment maxSurge
and maxUnavailable
, user don't need to warp "" for an integer in yaml.
The Placement "demo1" is invalid: spec.decisionStrategy.groupStrategy.clustersPerDecisionGroup: Invalid value: "integer": spec.decisionStrategy.groupStrategy.clustersPerDecisionGroup in body must be of type string: "integer"
For the format check, another method is that we can check it in code like https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/util/intstr/intstr.go#LL212C6-L212C32 and report error in placement "misconfigured" condition. Hope this can help.
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.
Nice, wasn't aware that intOrString type has function to validate the percentage. okay, changing it to intOrString
cluster/v1beta1/helpers.go
Outdated
// Timeout defined in minutes for how long workload applier controller will wait till workload reach successful state in the cluster. Only considered for Rollout Type Progressive and ProgressivePerGroup. Default is None meaning the workload applier will not proceed apply workload to other clusters if did not reach the successful state. | ||
// +kubebuilder:default:=None | ||
// +optional | ||
Timeout string `json:"timeout,omitempty"` |
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 the unit of the Timeout field? minute? it could be "1h", "30m"? or "1hour", "30minutes"?
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.
good point, it should be second with int32 as type commonly
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.
good point thanks, I added a validation pattern for [0-9]+[h|m|s ] |None
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.
should be *int32
? my understanding is that None and 0 have different meanings, right?
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.
yes, its string type to be able to set None
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.
FYI https://github.com/open-cluster-management-io/api/blob/main/cluster/v1beta1/types_placement.go#L320 which is nil by default.
Are there user cases that would be waiting for minutes or even hours?
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.
yes, upgrade OCP takes at least 50min. other examples; install Subscription/operator (ACM), images download (preCache) take around 5min-10min. AND the timeout string can be validated using the ParseDuration https://pkg.go.dev/time#ParseDuration
3aa6838
to
7f121af
Compare
LGTM |
Signed-off-by: melserngawy <melserng@redhat.com>
7f121af
to
7418172
Compare
As we discussed the rolloutStrategy struct is separated from the helper and moved to another PR [0]. I believe the DecisionStrategy is good to merge. [0] #244 |
/lgtm |
8f11dd3
into
open-cluster-management-io:main
No description provided.