-
Notifications
You must be signed in to change notification settings - Fork 1.5k
✨ Add rollout strategy support for KCP #4073
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,7 @@ import ( | |
| "github.com/pkg/errors" | ||
| apierrors "k8s.io/apimachinery/pkg/api/errors" | ||
| "k8s.io/apimachinery/pkg/runtime" | ||
| "k8s.io/apimachinery/pkg/util/intstr" | ||
| "k8s.io/apimachinery/pkg/util/validation/field" | ||
| kubeadmv1 "sigs.k8s.io/cluster-api/bootstrap/kubeadm/types/v1beta1" | ||
| "sigs.k8s.io/cluster-api/util/container" | ||
|
|
@@ -61,6 +62,25 @@ func (in *KubeadmControlPlane) Default() { | |
| if !strings.HasPrefix(in.Spec.Version, "v") { | ||
| in.Spec.Version = "v" + in.Spec.Version | ||
| } | ||
|
|
||
| ios1 := intstr.FromInt(1) | ||
|
|
||
| if in.Spec.RolloutStrategy == nil { | ||
| in.Spec.RolloutStrategy = &RolloutStrategy{} | ||
| } | ||
|
|
||
| // Enforce RollingUpdate strategy and default MaxSurge if not set. | ||
| if in.Spec.RolloutStrategy != nil { | ||
| if len(in.Spec.RolloutStrategy.Type) == 0 { | ||
| in.Spec.RolloutStrategy.Type = RollingUpdateStrategyType | ||
| } | ||
| if in.Spec.RolloutStrategy.Type == RollingUpdateStrategyType { | ||
| if in.Spec.RolloutStrategy.RollingUpdate == nil { | ||
| in.Spec.RolloutStrategy.RollingUpdate = &RollingUpdate{} | ||
| } | ||
| in.Spec.RolloutStrategy.RollingUpdate.MaxSurge = intstr.ValueOrDefault(in.Spec.RolloutStrategy.RollingUpdate.MaxSurge, ios1) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // ValidateCreate implements webhook.Validator so a webhook will be registered for the type | ||
|
|
@@ -116,6 +136,7 @@ func (in *KubeadmControlPlane) ValidateUpdate(old runtime.Object) error { | |
| {spec, "version"}, | ||
| {spec, "upgradeAfter"}, | ||
| {spec, "nodeDrainTimeout"}, | ||
| {spec, "rolloutStrategy"}, | ||
| } | ||
|
|
||
| allErrs := in.validateCommon() | ||
|
|
@@ -269,6 +290,42 @@ func (in *KubeadmControlPlane) validateCommon() (allErrs field.ErrorList) { | |
| allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "version"), in.Spec.Version, "must be a valid semantic version")) | ||
| } | ||
|
|
||
| if in.Spec.RolloutStrategy != nil { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we error out in case in.Spec.RolloutStrategy == nil?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that the defaulting function could change at some point, I personally prefer to have each func self consistent (or at least to explicitly document the assumption each function relies on, especially if those assumptions depends on something somewhere else in the codebase), but this is just a nit, feel free to ignore it
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the clarification @fabriziopandini. |
||
|
|
||
|
jan-est marked this conversation as resolved.
|
||
| if in.Spec.RolloutStrategy.Type != RollingUpdateStrategyType { | ||
| allErrs = append( | ||
| allErrs, | ||
| field.Required( | ||
| field.NewPath("spec", "rolloutStrategy", "type"), | ||
| "only RollingUpdateStrategyType is supported", | ||
| ), | ||
| ) | ||
| } | ||
|
|
||
| ios1 := intstr.FromInt(1) | ||
| ios0 := intstr.FromInt(0) | ||
|
|
||
| if *in.Spec.RolloutStrategy.RollingUpdate.MaxSurge == ios0 && *in.Spec.Replicas < int32(3) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we check for RollingUpdate != nil and error out in case this is not true?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @fabriziopandini do you see any cases where |
||
| allErrs = append( | ||
| allErrs, | ||
| field.Required( | ||
| field.NewPath("spec", "rolloutStrategy", "rollingUpdate"), | ||
| "when KubeadmControlPlane is configured to scale-in, replica count needs to be at least 3", | ||
| ), | ||
| ) | ||
| } | ||
|
|
||
| if *in.Spec.RolloutStrategy.RollingUpdate.MaxSurge != ios1 && *in.Spec.RolloutStrategy.RollingUpdate.MaxSurge != ios0 { | ||
| allErrs = append( | ||
| allErrs, | ||
| field.Required( | ||
| field.NewPath("spec", "rolloutStrategy", "rollingUpdate", "maxSurge"), | ||
| "value must be 1 or 0", | ||
| ), | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| allErrs = append(allErrs, in.validateCoreDNSImage()...) | ||
|
|
||
| return allErrs | ||
|
|
||
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.
if we do backport to v1alpha3 wouldn't this be a problem?
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.
If we do backport, we need to remove this part of the code and say something like
Required minimum v0.3.x version before upgrading is ...