✨ Add rollout strategy support for KCP by jan-est · Pull Request #4073 · kubernetes-sigs/cluster-api

jan-est · 2021-01-14T07:46:24Z

What this PR does / why we need it:
This PR implements proposal 3857 by adding rollout strategy support for KCP. PR will enables use of maxSurge and maxUnavailable fields during KCP upgrade.

k8s-ci-robot · 2021-01-14T07:46:32Z

Hi @jan-est. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

vincepri · 2021-01-14T16:11:11Z

/ok-to-test
/milestone v0.4.0

vincepri · 2021-02-03T21:11:16Z

@jan-est Can you squash commits?

jan-est · 2021-02-04T08:04:32Z

@jan-est Can you squash commits?

@vincepri Done

vincepri · 2021-02-04T18:13:18Z

+	// We can ignore MaxUnavailable because we are enforcing health checks before we get here.
+	maxNodes := *kcp.Spec.Replicas + int32(kcp.Spec.RolloutStrategy.RollingUpdate.MaxSurge.IntValue())
+	if status.Nodes < maxNodes {


Is MaxUnavailable not used anywhere in the code?

It is not used anywhere in kubeadmControlPlane controllers code.

Is it used anywhere else?

If not, we should probably remove it

We are using MaxUnavailable in webhook code

I guess in this implementation we don't need both MaxSurge and MaxUnavailable since one is the opposite of the other. The benefit of having MaxUnavailbale is that it makes it explicit for the user that setting maxSurge to 0 means that you'll have 1 control plane at a time go down during upgrade.

Makes sense to remove it and add it in the future if we actually need it to implement a more flexible rollout.

If we do remove it we also need to update any docs/proposal that reference it

If we do want to keep it for UX purposes, it should probably be a calculated field, under Status

I am +1 for removing it complete. I agree with @CecileRobertMichon that we can add it back in the future if necessary. I remove MaxUnavailable and update the proposal when this is approved and merged?

We should possibly remove it in this same PR, so the changes go in together. +1 from me as well.

vincepri · 2021-02-04T18:15:03Z

-	if status.Nodes <= *kcp.Spec.Replicas {
+	// RolloutStrategy is currently defaulted and validated to be RollingUpdate
+	// We can ignore MaxUnavailable because we are enforcing health checks before we get here.
+	maxNodes := *kcp.Spec.Replicas + int32(kcp.Spec.RolloutStrategy.RollingUpdate.MaxSurge.IntValue())


@detiber @sedefsavas @CecileRobertMichon @fabriziopandini Please double check these changes, given that you all have worked more or less on this codebase.

If we were to backport this to v1alpha3, would you foresee any issues?

CecileRobertMichon · 2021-02-09T17:14:03Z

+		return err
+	}
+
+	dest.Spec.RolloutStrategy = restored.Spec.RolloutStrategy


if we do backport to v1alpha3 wouldn't this be a problem?

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 ...

fabriziopandini · 2021-02-22T09:31:28Z

 		allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "version"), in.Spec.Version, "must be a valid semantic version"))
 	}

+	if in.Spec.RolloutStrategy != nil {


Should we error out in case in.Spec.RolloutStrategy == nil?

Can if in.Spec.RolloutStrategy == nil ever be true? I assumed that defaulting on line 68 makes sure that the value is never nil. Or have I misunderstood?

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

Thanks for the clarification @fabriziopandini.

fabriziopandini · 2021-02-22T09:32:26Z

+		ios1 := intstr.FromInt(1)
+		ios0 := intstr.FromInt(0)
+
+		if *in.Spec.RolloutStrategy.RollingUpdate.MaxSurge == ios0 && *in.Spec.Replicas < int32(3) {


Should we check for RollingUpdate != nil and error out in case this is not true?

@fabriziopandini do you see any cases where RollingUpdate != nil is not true? I assumed that defaulting on line 68 makes sure that the value is never nil. Or have I misunderstood?

vincepri · 2021-03-03T15:27:46Z

LGTM

Let's squash commits before merging

vincepri · 2021-03-03T18:36:37Z

/test pull-cluster-api-test-main

vincepri · 2021-03-03T18:43:35Z

/retest

vincepri · 2021-03-03T18:52:15Z

/retest

jan-est · 2021-03-04T06:17:01Z

/retest

fabriziopandini · 2021-03-04T08:53:58Z

/lgtm

jan-est · 2021-03-09T06:51:08Z

/retest

fabriziopandini · 2021-03-09T10:27:11Z

/lgtm

vincepri · 2021-03-09T15:24:15Z

/approve

k8s-ci-robot · 2021-03-09T15:24:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [vincepri]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 14, 2021

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 14, 2021

k8s-ci-robot requested review from CecileRobertMichon and detiber January 14, 2021 07:46

jan-est force-pushed the kcp-rollout-strategy branch from 4fa622d to 915623b Compare January 14, 2021 08:32

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 14, 2021

k8s-ci-robot added this to the v0.4.0 milestone Jan 14, 2021

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 14, 2021

jan-est force-pushed the kcp-rollout-strategy branch from 915623b to 3c7d3f4 Compare January 14, 2021 17:59

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 14, 2021

jan-est force-pushed the kcp-rollout-strategy branch from 3c7d3f4 to 2983b86 Compare January 14, 2021 18:57

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 14, 2021

jan-est force-pushed the kcp-rollout-strategy branch 2 times, most recently from f0d3469 to 3a918f3 Compare January 15, 2021 09:10

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 15, 2021

jan-est force-pushed the kcp-rollout-strategy branch 4 times, most recently from fb76378 to 3dae187 Compare January 22, 2021 12:00

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 22, 2021

jan-est force-pushed the kcp-rollout-strategy branch from 3dae187 to 0728012 Compare January 22, 2021 12:46

jan-est force-pushed the kcp-rollout-strategy branch 4 times, most recently from abf4836 to 620a288 Compare February 2, 2021 10:54

jan-est force-pushed the kcp-rollout-strategy branch 3 times, most recently from df2290e to 7183044 Compare February 4, 2021 07:23

vincepri requested changes Feb 4, 2021

View reviewed changes

vincepri reviewed Feb 4, 2021

View reviewed changes

jan-est force-pushed the kcp-rollout-strategy branch from 7183044 to 00f3c8e Compare February 5, 2021 07:24

CecileRobertMichon reviewed Feb 9, 2021

View reviewed changes

devigned mentioned this pull request Feb 14, 2021

machine pool surge, max unavailable and instance delete kubernetes-sigs/cluster-api-provider-azure#1105

Closed

3 tasks

fabriziopandini reviewed Feb 22, 2021

View reviewed changes

devigned mentioned this pull request Mar 3, 2021

add azure machine pool machine proposal kubernetes-sigs/cluster-api-provider-azure#1191

Merged

3 tasks

Add rollout strategy support for KCP

74a70a6

fabriziopandini mentioned this pull request Mar 10, 2021

Implement E2E for scale in rollout #4282

Closed

jan-est mentioned this pull request Mar 11, 2021

✨ Backport KCP Rollout Strategy to v1alpha3 api #4293

Merged

jan-est mentioned this pull request Mar 22, 2021

✨ Add E2E for scale in rollout #4347

Merged

Conversation

jan-est commented Jan 14, 2021

Uh oh!

k8s-ci-robot commented Jan 14, 2021

Uh oh!

vincepri commented Jan 14, 2021

Uh oh!

vincepri commented Feb 3, 2021

Uh oh!

jan-est commented Feb 4, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CecileRobertMichon Feb 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jan-est Feb 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vincepri commented Mar 3, 2021

Uh oh!

vincepri commented Mar 3, 2021

Uh oh!

vincepri commented Mar 3, 2021

Uh oh!

vincepri commented Mar 3, 2021

Uh oh!

jan-est commented Mar 4, 2021

Uh oh!

fabriziopandini commented Mar 4, 2021

CecileRobertMichon Feb 9, 2021 •

edited

Loading

jan-est Feb 10, 2021 •

edited

Loading