Option to increase concurrency of rolling update within instancegroup #8271

johngmyers · 2020-01-04T20:37:16Z

Adds a RollingUpdate field to instancegroup and cluster for configuring the rolling update strategy. Starts with MaxUnavailable, mostly cribbed from machine-api's MachineDeployment. Unlike MachineDeployment, permits disabling rolling updates for an instancegroup.

/kind feature
/area rolling-update
Fixes #1718, #7685

k8s-ci-robot · 2020-01-04T20:37:24Z

Hi @johngmyers. Thanks for your PR.

I'm waiting for a kubernetes 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.

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.

rifelpet · 2020-01-05T14:24:07Z

/ok-to-test

johngmyers · 2020-01-05T20:43:29Z

/retest

pkg/instancegroups/rollingupdate_test.go

pkg/instancegroups/instancegroups.go

johngmyers · 2020-01-12T05:02:39Z

Looks like my tests are a little flaky. Perhaps a GC is getting in and throwing off the timings. I will investigate.
/test pull-kops-bazel-test

johngmyers · 2020-01-13T05:32:45Z

/test pull-kops-verify-boilerplate

johngmyers · 2020-01-13T06:02:54Z

/retest

pkg/instancegroups/instancegroups.go

pkg/instancegroups/settings.go

justinsb · 2020-01-26T15:01:21Z

pkg/instancegroups/settings.go

+		unavailable, err := intstr.GetValueFromIntOrPercent(rollingUpdate.MaxUnavailable, numInstances, false)
+		if err != nil {
+			// If unparseable use the default value
+			unavailable = 1


Nit: we might want to log a warning here. Ideally we would have already caught this in validation though.

justinsb · 2020-01-26T15:03:58Z

pkg/instancegroups/settings_test.go

+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			defaultCluster := &kops.RollingUpdate{}
+			setFieldValue(defaultCluster, tc.name, tc.defaultValue)


I think I know where this is going... it is usually easier and clearer just to write it out, but ... let's see once the chain lands!

pkg/instancegroups/instancegroups.go

justinsb · 2020-01-26T15:06:12Z

pkg/instancegroups/instancegroups.go

-		} else if rollingUpdateData.CloudOnly {
+	if maxConcurrency == 0 {
+		klog.Infof("Rolling updates for InstanceGroup %s are disabled", r.CloudGroup.InstanceGroup.Name)
+		return nil


Should we return an error here (as we did not update the IG)?

We want to continue to update the subsequent instancegroups. The admin specifically wanted to have kops skip this instancegroup, so it isn't an error.

One issue here is the placement. Where it is currently it will validate and taint before returning. Should this be moved up before the initial validation?

I may be misunderstanding, but I think you're saying that maxUnavailable: 0 means "skip this IG for rolling updates". I'm not sure I follow that implication, but I also don't know what maxUnavailable: 0 would mean!

Is maxUnavailable: 0 something you're actually using? Because we could just treat it as a validation error until we have maxSurge support (I guess that maxUnavailable=0 and maxSurge=0 is a validation error on a Deployment?)

I'm not overly worry about the tainting, TBH, given that!

pkg/instancegroups/instancegroups.go

justinsb · 2020-01-26T15:18:08Z

LGTM in general, a few nits if you agree with them, but the only two I'd like to reach resolution on now is whether we should pass the channel in to drainTerminateAndWait, and having a comment on the sweep block about how it avoids validate calls.

pkg/apis/kops/validation/instancegroup.go

johngmyers · 2020-01-27T07:45:31Z

/test pull-kops-e2e-kubernetes-aws

justinsb · 2020-01-28T03:35:21Z

Thanks for the tweaks @johngmyers - this LGTM. I do wonder if we should just make maxUnavailable: 0 into a validation error for now - are you using it? But anyway we can do that in a separate PR - I don't want to drag this one out any more - thanks for your patience!

/approve
/lgtm

k8s-ci-robot · 2020-01-28T03:35:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johngmyers, justinsb

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:

~~OWNERS~~ [justinsb]

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

johngmyers · 2020-01-28T03:43:01Z

@justinsb maxUnavailable: 0 is intended to address #7685. Do you have an issue with providing that feature, or just this PR's implementation of it? Perhaps we should reopen #7685 and discuss there?

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. area/rolling-update 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 4, 2020

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

k8s-ci-robot requested review from joshbranham and robinpercy January 4, 2020 20:37

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 5, 2020

johngmyers added 7 commits January 5, 2020 12:09

Add MaxUnavailable setting to cluster and instancegroup

ce241bc

make apimachinery

855cd1f

make crds

d6ceffe

Create resolveSettings

adaf903

Extract drainTerminateAndWait()

91f4920

Extract maybeValidate

0952374

Implement MaxUnavailable

0c3651c

johngmyers force-pushed the max-unavailable branch from f9ea0cf to 0c3651c Compare January 5, 2020 20:10

johngmyers mentioned this pull request Jan 11, 2020

Option to surge during rolling update #8313

Merged

johngmyers commented Jan 11, 2020

View reviewed changes

pkg/instancegroups/rollingupdate_test.go Outdated Show resolved Hide resolved

johngmyers commented Jan 11, 2020

View reviewed changes

pkg/instancegroups/instancegroups.go Outdated Show resolved Hide resolved

Allow MaxConcurrency for masters and bastions

10d6416

Reduce test flakiness

5f72d12

justinsb reviewed Jan 26, 2020

View reviewed changes

pkg/instancegroups/instancegroups.go Outdated Show resolved Hide resolved

justinsb reviewed Jan 26, 2020

View reviewed changes

johngmyers force-pushed the max-unavailable branch from bfa3bea to ec66fea Compare January 26, 2020 20:43

johngmyers added 2 commits January 26, 2020 14:18

Address review comments

d56ad41

Add validation of RollingUpdate field

dcdf08c

johngmyers force-pushed the max-unavailable branch from ec66fea to dcdf08c Compare January 26, 2020 22:19

johngmyers commented Jan 27, 2020

View reviewed changes

pkg/apis/kops/validation/instancegroup.go Show resolved Hide resolved

k8s-ci-robot assigned justinsb Jan 28, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 28, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 28, 2020

k8s-ci-robot merged commit e56c507 into kubernetes:master Jan 28, 2020

k8s-ci-robot added this to the v1.18 milestone Jan 28, 2020

johngmyers deleted the max-unavailable branch January 28, 2020 04:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Option to increase concurrency of rolling update within instancegroup #8271

Option to increase concurrency of rolling update within instancegroup #8271

johngmyers commented Jan 4, 2020

k8s-ci-robot commented Jan 4, 2020

rifelpet commented Jan 5, 2020

johngmyers commented Jan 5, 2020

johngmyers commented Jan 12, 2020

johngmyers commented Jan 13, 2020

johngmyers commented Jan 13, 2020

justinsb Jan 26, 2020

justinsb Jan 26, 2020

justinsb Jan 26, 2020

johngmyers Jan 26, 2020

johngmyers Jan 26, 2020

justinsb Jan 28, 2020

justinsb commented Jan 26, 2020

johngmyers commented Jan 27, 2020

justinsb commented Jan 28, 2020

k8s-ci-robot commented Jan 28, 2020

johngmyers commented Jan 28, 2020

Option to increase concurrency of rolling update within instancegroup #8271

Option to increase concurrency of rolling update within instancegroup #8271

Conversation

johngmyers commented Jan 4, 2020

k8s-ci-robot commented Jan 4, 2020

rifelpet commented Jan 5, 2020

johngmyers commented Jan 5, 2020

johngmyers commented Jan 12, 2020

johngmyers commented Jan 13, 2020

johngmyers commented Jan 13, 2020

justinsb Jan 26, 2020

Choose a reason for hiding this comment

justinsb Jan 26, 2020

Choose a reason for hiding this comment

justinsb Jan 26, 2020

Choose a reason for hiding this comment

johngmyers Jan 26, 2020

Choose a reason for hiding this comment

johngmyers Jan 26, 2020

Choose a reason for hiding this comment

justinsb Jan 28, 2020

Choose a reason for hiding this comment

justinsb commented Jan 26, 2020

johngmyers commented Jan 27, 2020

justinsb commented Jan 28, 2020

k8s-ci-robot commented Jan 28, 2020

johngmyers commented Jan 28, 2020