-
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 common RolloutStrategy API #244
Add common RolloutStrategy API #244
Conversation
Add @haoqing0110 |
39d5981
to
e748ae4
Compare
} | ||
|
||
// MandatoryDecisionGroup set the decision group name or group index. | ||
type MandatoryDecisionGroup 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.
How to handle the case when both GroupName and GroupIndex are defined in each MandatoryDecisionGroup? Can intorstring
make it easier?
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.
When user set GroupName group selection should be based on GroupName even if user set GroupIndex.
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.
Sounds good, is that possible to add this explanation in the API comments?
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.
sure, done.
// 2) Progressive means apply the workload to the selected clusters progressively per cluster. The workload will not be applied to the next cluster unless one of the current applied clusters reach the successful state or timeout. | ||
// 3) ProgressivePerGroup means apply the workload to decisionGroup clusters progressively per group. The workload will not be applied to the next decisionGroup unless all clusters in the current group reach the successful state or timeout. | ||
// +kubebuilder:default:=All | ||
// +optional |
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 need to an enum validation
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, done
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.
Still need some const string like after adding enum validation.
const (
//All means apply the workload to all clusters in the decision groups at once.
All string = "All"
//Progressive means apply the workload to the selected clusters progressively per cluster.
Progressive string = "Progressive"
//ProgressivePerGroup means apply the workload to the selected clusters progressively per group.
ProgressivePerGroup string = "ProgressivePerGroup"
)
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.
ops, removed by mistake. done
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object | ||
// +k8s:deepcopy-gen=true | ||
|
||
// RolloutStrategy API used by consumer workload applier APIs such as ManifestWorkReplicaSet, Policy, ManagedClusterAddon ...etc |
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 only indicate that this is for rolling out workload or configuration across clusters. We do not need to mention ManifestWorkReplicaSet, Policy, ManagedClusterAddon. Otherwise, the doc will be added into each crd definition.
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, done
} | ||
|
||
// MandatoryDecisionGroup set the decision group name or group index. | ||
type MandatoryDecisionGroup 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.
can we allow setting GroupName and GroupIndex at the same time?
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.
no, We should consider GroupName first then GroupIndex.
// MaxConcurrency is the max number of clusters to deploy workload concurrently. The default value for MaxConcurrency is determined from the clustersPerDecisionGroup defined in the placement->DecisionStrategy. | ||
// +kubebuilder:validation:Pattern="^((100|[0-9]{1,2})%|[0-9]+)$" | ||
// +optional | ||
MaxConcurrency string `json:"maxConcurrency,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.
Have the below question for maxconcurrency
, might need a clearer explanation.
- Does the
maxconcurrency
also work on MandatoryDecisionGroup? - If a placement strategy is defined as below:
DecisionStrategy:
numberOfClustersPerDecisionGroup: 150
DecisionGroups:
- label: prod-canary-west
clusterSelector:
matchExpressions:
- key: prod-canary-west
operator: Exist
….
status:
numberOfSelectedClusters: 300
decisionGroups:
- decisionGroupIndex: 0
decisionGroupLabel: prod-canary-west
placementDecisions:
- ztp-placement-decision-0 // cluster1 to cluster 100
- ztp-placement-decision-1 // cluster101 to cluster120
clusterCount: 120
- decisionGroupIndex: 1
placementDecisions:
- ztp-placement-decision-2 // cluster121 to cluster 220
- ztp-placement-decision-3 // cluster221 to cluster 270
clusterCount: 150 //
- decisionGroupIndex: 2
placementDecisions:
- ztp-placement-decision-4 // cluster271 to cluster 300
clusterCount: 30
When type is rolloutProgressive
and not define any mandatoryDecisionGroups
, what’s the expected upgrade behavior, will decision groups be ignored and upgrade cluster1 to cluster 150 first actually?
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, based on the example above if the MandatoryDecisionGroup and Maxconcurrency not defined the rolloutProgressive should follow numberOfClustersPerDecisionGroup. Maxconcurrency to give the option override the numberOfClustersPerDecisionGroup.
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 this filed also be the type intorstring
to be consistent with clustersPerDecisionGroup?
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, done.
e748ae4
to
ead3cf4
Compare
|
||
// Timeout to consider while applying the workload. | ||
type Timeout struct { | ||
// Timeout define how long workload applier controller will wait till workload reach successful state in the cluster. Only considered for Rollout Type Progressive and ProgressivePerGroup. |
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 comment is inconsistent with the above type RolloutAll
which has timeout field.
// RolloutAll is a RolloutStrategy Type
type RolloutAll struct {
// +optional
Timeout Timeout `json:",inline"`
}
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.
ops, removed.
ead3cf4
to
70312e5
Compare
LGTM |
"k8s.io/apimachinery/pkg/util/intstr" | ||
) | ||
|
||
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object |
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 not a runtime object so line 7 is not needed. see here https://pkg.go.dev/k8s.io/gengo/examples/deepcopy-gen
/approve @haoqing0110 are we fine if there is no DeepCopy? |
[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 |
DeepCopy() for the common rollout strategy is needed I think, since we treat it as a duck type and will add library or helper function only for this part. |
|
Signed-off-by: melserngawy <melserng@redhat.com>
70312e5
to
a2203e5
Compare
/lgtm |
f64f0af
into
open-cluster-management-io:main
// All define required fields for RolloutStrategy type All | ||
// +optional | ||
All *RolloutAll `json:"all,omitempty"` | ||
|
||
// Progressive define required fields for RolloutStrategy type Progressive | ||
// +optional | ||
Progressive *RolloutProgressive `json:"progressive,omitempty"` | ||
|
||
// ProgressivePerGroup define required fields for RolloutStrategy type ProgressivePerGroup | ||
// +optional | ||
ProgressivePerGroup *RolloutProgressivePerGroup `json:"progressivePerGroup,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.
Is there a reason behind having these as separate keys? Could it not be streamlined as providing the Type
and inlining all of the options? If that's an option, I can work on a PR. At minimum, I think it might be nice to have Timeout
at the same level as Type
?
type RolloutStrategy struct {
// +kubebuilder:validation:Enum=All;Progressive;ProgressivePerGroup
// +kubebuilder:default:=All
// +optional
Type string `json:"type,omitempty"`
// +optional
Timeout `json:",inline"`
// +optional
MandatoryDecisionGroups `json:",inline"`
// +kubebuilder:validation:Pattern="^((100|[0-9]{1,2})%|[0-9]+)$"
// +kubebuilder:validation:XIntOrString
// +optional
MaxConcurrency intstr.IntOrString `json:"maxConcurrency,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.
this is designed as an union type, a certain type is point to the a certain field. see here https://github.com/kubernetes/enhancements/blob/master/keps/sig-api-machinery/1027-api-unions/README.md. This could avoid unexpected wrong configurations. For instance, you would not want user set Type to Progressive and still set the MandatoryDecisionGroups.
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 has been done with union type to avoid miss leading the endUser as for ex; when the Type is ALL there is no need to define the MandatoryDecisionGroups and MaxConcurrency. As well as when the Type is ProgressivePerGroup no need to define MaxConcurrency.
So make sense to let each Type has its own 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.
Thanks for the comments--it'd be nice if the type could be inferred from the key rather than having an additional type
to avoid the stutter, which I think is what feels awkward about this to me, but it seems that's not possible. 🙁
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.
Actually, I don't see the union type even implemented yet in Kubernetes?
Kubebuilder doesn't support OneOf
(yet?), but some Kustomize post-processing on the CRD could let consumers use the OneOf
in the CRD rather than having a Type
field. This would also avoid confusion since it'd be impossible to set the Type
to one thing and then provide a set of other options (as I think can be done as it is now)?
oneOf:
- required: [ "all" ]
- required: [ "progressive" ]
- required: [ "progressivePerGroup" ]
Though without Kubebuilder support, it'd have to be implemented by the consumers of the library, unfortunately.
No description provided.