Support maxUnavailable in rollout #65

kerthcet · 2024-03-19T21:42:57Z

What type of PR is this?

/kind feature

What this PR does / why we need it

Which issue(s) this PR fixes

part of #3

Special notes for your reviewer

Does this PR introduce a user-facing change?

Support maxUnavailable in rollout

k8s-ci-robot · 2024-03-19T21:43:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kerthcet

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~~ [kerthcet]

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

kerthcet · 2024-03-19T21:43:19Z

/hold

kerthcet · 2024-03-25T22:48:10Z

Main changes:

The general mechanism is we'll have a templateRevisionHash label for leaderStatefulst, leaderPod, workerStatefulset, workerPod, it will be used to verify that this is the new revision, so when leaderTemplate or workerTemplate is updated, we'll trigger a rolling update of the leader statefulset, which means a group of pods(leader+worker) will be updated the same time.
The rolling update number of replicas is defined by maxUnavailable, which means when we set maxUnavailable to 2, and we have a lws with 5 replicas, the rolling update index will looks like: 4,3 -> 2,1, -> 0.

Other updates:

the maxUnavailable is default to 1 because 0 is not allowed in Statefulset
the maxSurge(implementation not included in this PR) is default to 0, then the default behavior will rolling update a replica each time, I think this is more cautious.

Another concern is about the leaderWorkerTemplate.size, I have an implementation locally, it behaves as when we increate or decrease the size, it will not lead to the statefulset rollingupdate, only scalilng up/down the worker pods. But I may need your inputs for this, and this is now conflict with several things:

we have a size annotation in the leaderPod, which will lead to the statefulset rolling update each time we change the size
the restart policy RecreateGroupOnPodRestart is somehow conflict with decreasing the size because when a pod is deleted, the leaderPod will also be deleted.

so I remove the rolling update for size update right now, and make it immutable as original.

Please take a look and sorry for the slow response because I'm right at kubecon last week. cc @ahg-g @liurupeng

kerthcet · 2024-03-25T22:49:04Z

/hold cancel
we may need e2e test as well. I'll add the e2e test framework later.

liurupeng · 2024-03-26T04:45:49Z

@kerthcet we have someone working on the e2e test already, that work is in progress, thanks!

Makefile

config/manager/kustomization.yaml

pkg/controllers/leaderworkerset_controller.go

liurupeng · 2024-03-28T06:43:59Z

pkg/controllers/leaderworkerset_controller.go

+
+	return nil
+}
+
 // updates the condition of the leaderworkerset to either Progressing or Available.
 func (r *LeaderWorkerSetReconciler) updateConditions(ctx context.Context, lws *leaderworkerset.LeaderWorkerSet) (bool, error) {


qq, will you add the lws.status.updatedReplica in the next PR?

For functionality, seems not that necessary, but for visibility, seems useful, maybe I'll add it later, but on my original design, I didn't have the plan.

TL;DR: I'll add the field.

pkg/controllers/leaderworkerset_controller.go

liurupeng · 2024-03-28T06:55:12Z

pkg/controllers/leaderworkerset_controller.go

+	templateHash := utils.LeaderWorkerTemplateHash(lws)
+stop:
+	for i := replicas - 1; i >= 0; i-- {
+		for j := len(stsList.Items) - 1; j >= 0; j-- {


qq, why we traverse the stsList from the last to the first one as well?

Because at first I though the items will start from the last index to 0, which will accelerate the calculation, but seems not that case, but I didn't change the implementation for lazy reason.
Anyway I think it doesn't make any difference? 😅

liurupeng · 2024-03-28T07:10:30Z

Thanks @kerthcet , this is a great implementation! Agree we should allow updating size in the next change, (this change is already large enough). Regarding updating the size will trigger the StatefulSet upgrade, could you elaborate more on this? When upgrade happens, both the template hash label and size annotation will change on the leader statefulset at the same time right? Will the leader statefulSet start a rolling update again and what is the behavior you saw?

ahg-g

This is really good, thanks Kante!

Makefile

api/leaderworkerset/v1/leaderworkerset_types.go

pkg/utils/utils.go

test/integration/controllers/leaderworkerset_test.go

pkg/webhooks/leaderworkerset_webhook.go

test/integration/webhooks/leaderworkerset_test.go

kerthcet · 2024-04-02T07:59:10Z

/test pull-lws-test-integration-main

kerthcet · 2024-04-02T08:10:25Z

/test pull-lws-test-integration-main

ahg-g · 2024-04-03T00:02:23Z

so I remove the rolling update for size update right now, and make it immutable as original.

I think an immutable size is fine and a reasonable tradeoff to simplify the rolling update logic

Signed-off-by: kerthcet <[email protected]>

kerthcet · 2024-04-03T08:49:42Z

Last 2 commit:

Add one e2e test
Solve the performance issue when calculating the partition field

ahg-g

A couple of nits, otherwise this is good from my end!

pkg/controllers/leaderworkerset_controller.go

ahg-g · 2024-04-04T02:31:45Z

test/e2e/e2e_test.go

+		testing.UpdateWorkerTemplate(ctx, k8sClient, lws)
+
+		// Wait for leaderWorkerSet ready again.
+		testing.ExpectLeaderWorkerSetAvailable(ctx, k8sClient, lws, "All replicas are ready")


how do we ensure that this is checking the availability status after the update? isn't it possible that we are still checking the previous status before the lws got a chance to pick the update and change the availability status to false?

Move this down then we'll check the hash first to make sure we're validating the new revision.

I am not sure this will make a difference, it would be nice if we can check that the template hash changed, and then check that the lws is available.

I think we already do as you said, so the process is:

update the lws

ExpectValidLeaderStatefulSet will check that the template hash is already the updated one, see

lws/test/testutils/validators.go

Lines 121 to 124 in 59f5a76

hash := utils.LeaderWorkerTemplateHash(&lws)

if sts.Labels[leaderworkerset.TemplateRevisionHashKey] != hash {

return fmt.Errorf("mismatch template revision hash for leader statefulset, got: %s, want: %s", sts.Spec.Template.Labels[leaderworkerset.TemplateRevisionHashKey], hash)

}

ExpectLeaderWorkerSetAvailable validate the lws is available.

test/integration/controllers/leaderworkerset_test.go

Signed-off-by: kerthcet <[email protected]>

kerthcet · 2024-04-04T04:55:48Z

All set @ahg-g

liurupeng · 2024-04-04T05:41:59Z

lgtm as well, I will wait for @ahg-g to approve

ahg-g · 2024-04-04T06:59:52Z

/label tide/merge-method-squash
/lgtm
/hold cancel

Great work!

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 19, 2024

k8s-ci-robot requested review from ahg-g and liurupeng March 19, 2024 21:43

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 19, 2024

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 19, 2024

kerthcet force-pushed the feat/support-roll-2 branch 2 times, most recently from 31dd616 to 15ed653 Compare March 25, 2024 22:23

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 Mar 25, 2024

kerthcet changed the title ~~[WIP] Support maxUnavailable in rollout~~ Support maxUnavailable in rollout Mar 25, 2024

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 25, 2024

kerthcet force-pushed the feat/support-roll-2 branch 2 times, most recently from 317dc4b to f82172a Compare March 25, 2024 22:35

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 25, 2024

kerthcet force-pushed the feat/support-roll-2 branch from a7e6bfc to a7db8d3 Compare March 26, 2024 06:25

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 Mar 26, 2024

liurupeng reviewed Mar 28, 2024

View reviewed changes

pkg/controllers/leaderworkerset_controller.go Outdated Show resolved Hide resolved

liurupeng reviewed Mar 28, 2024

View reviewed changes

pkg/controllers/leaderworkerset_controller.go Outdated Show resolved Hide resolved

liurupeng reviewed Mar 28, 2024

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 1, 2024

ahg-g reviewed Apr 2, 2024

View reviewed changes

kerthcet force-pushed the feat/support-roll-2 branch from e4473ca to f25bd10 Compare April 2, 2024 07:54

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 2, 2024

kerthcet added 7 commits April 3, 2024 10:32

Fix the latest setup-envtest requires the go1.22

f0c7c4d

Signed-off-by: kerthcet <[email protected]>

Support maxUnavailable in rollout

c226ec8

Signed-off-by: kerthcet <[email protected]>

fix comments

9a99d5c

Signed-off-by: kerthcet <[email protected]>

fix flaky integration test

52ff64e

Signed-off-by: kerthcet <[email protected]>

fix comments

47698f7

Signed-off-by: kerthcet <[email protected]>

Fix flaky test

7fad4ea

Signed-off-by: kerthcet <[email protected]>

Add e2e test

b906059

Signed-off-by: kerthcet <[email protected]>

kerthcet force-pushed the feat/support-roll-2 branch from c6408ba to b906059 Compare April 3, 2024 03:51

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 3, 2024

solve rolling update partition performance issue

7ff79a2

Signed-off-by: kerthcet <[email protected]>

kerthcet force-pushed the feat/support-roll-2 branch from 311c0f4 to 7ff79a2 Compare April 3, 2024 08:49

ahg-g reviewed Apr 4, 2024

View reviewed changes

fix comments

2b9da9c

Signed-off-by: kerthcet <[email protected]>

kerthcet force-pushed the feat/support-roll-2 branch from 75bc593 to 2b9da9c Compare April 4, 2024 04:46

k8s-ci-robot added tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Apr 4, 2024

k8s-ci-robot assigned ahg-g Apr 4, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 4, 2024

k8s-ci-robot merged commit 59f5a76 into kubernetes-sigs:main Apr 4, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support maxUnavailable in rollout #65

Support maxUnavailable in rollout #65

kerthcet commented Mar 19, 2024

k8s-ci-robot commented Mar 19, 2024

kerthcet commented Mar 19, 2024

kerthcet commented Mar 25, 2024

kerthcet commented Mar 25, 2024

liurupeng commented Mar 26, 2024

liurupeng Mar 28, 2024 •

edited

Loading

kerthcet Apr 1, 2024

kerthcet Apr 1, 2024

liurupeng Mar 28, 2024

kerthcet Apr 1, 2024

liurupeng commented Mar 28, 2024 •

edited

Loading

ahg-g left a comment

kerthcet commented Apr 2, 2024

kerthcet commented Apr 2, 2024

ahg-g commented Apr 3, 2024

kerthcet commented Apr 3, 2024 •

edited

Loading

ahg-g left a comment

ahg-g Apr 4, 2024

kerthcet Apr 4, 2024

ahg-g Apr 4, 2024

kerthcet Apr 4, 2024

kerthcet commented Apr 4, 2024

liurupeng commented Apr 4, 2024

ahg-g commented Apr 4, 2024

	hash := utils.LeaderWorkerTemplateHash(&lws)
	if sts.Labels[leaderworkerset.TemplateRevisionHashKey] != hash {
	return fmt.Errorf("mismatch template revision hash for leader statefulset, got: %s, want: %s", sts.Spec.Template.Labels[leaderworkerset.TemplateRevisionHashKey], hash)
	}

Support maxUnavailable in rollout #65

Support maxUnavailable in rollout #65

Conversation

kerthcet commented Mar 19, 2024

What type of PR is this?

What this PR does / why we need it

Which issue(s) this PR fixes

Special notes for your reviewer

Does this PR introduce a user-facing change?

k8s-ci-robot commented Mar 19, 2024

kerthcet commented Mar 19, 2024

kerthcet commented Mar 25, 2024

kerthcet commented Mar 25, 2024

liurupeng commented Mar 26, 2024

liurupeng Mar 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liurupeng commented Mar 28, 2024 • edited Loading

ahg-g left a comment

Choose a reason for hiding this comment

kerthcet commented Apr 2, 2024

kerthcet commented Apr 2, 2024

ahg-g commented Apr 3, 2024

kerthcet commented Apr 3, 2024 • edited Loading

ahg-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kerthcet commented Apr 4, 2024

liurupeng commented Apr 4, 2024

ahg-g commented Apr 4, 2024

liurupeng Mar 28, 2024 •

edited

Loading

liurupeng commented Mar 28, 2024 •

edited

Loading

kerthcet commented Apr 3, 2024 •

edited

Loading