Support maxSurge in rolling update #98

kerthcet · 2024-04-12T09:07:57Z

What type of PR is this?

/kind feature

What this PR does / why we need it

Which issue(s) this PR fixes

Fixes #3

Special notes for your reviewer

It works like:

When rolling update, lws will burst out maxSurge replicas
When rolling update enter into the final stage, we'll update a new replica and reclaim a bursted replica
Finally the rolling update succeeded.

Does this PR introduce a user-facing change?

Support maxSurge in rolling update.

kerthcet · 2024-04-12T09:08:28Z

This is the general prototype, I need more tests.

kerthcet · 2024-04-12T09:12:01Z

So when rolling update, lws will request for hardware as well, there's no difference with lws without maxSurge, this can not help with your case, right? @liurupeng But when reclaiming the bursted replicas, it can somehow help here ...

kerthcet · 2024-04-16T11:37:12Z

You can take a look if you want, only webhook validation and e2e tests not added. But seems @ahg-g has question about this feature. We can't achieve the similar behavior as deployment does.

kerthcet · 2024-04-17T10:50:21Z

All set now. PTAL.

kerthcet · 2024-04-18T04:16:38Z

Would you like to take a look of this PR? Because it changes a lot and once other PR (I mean related ones) merges, lots of conflicts have to be solve. @ahg-g @liurupeng

ahg-g · 2024-04-18T04:52:42Z

I will try to find sometime this Friday

pkg/controllers/leaderworkerset_controller.go

test/e2e/e2e_test.go

test/integration/controllers/leaderworkerset_test.go

liurupeng · 2024-04-18T20:51:13Z

Sorry for the delay, added two comments about the semantics of the MaxSurge support, once that's finalized, will check more detailed, thanks Kante!

kerthcet · 2024-04-30T06:57:10Z

How rolling update looks like right now with maxSurge, let's say we have a lws with replicas=4, maxUnavailable=2, maxSurge=2, rollinStep = 2+2 = 4:

✅ replica updated successfully
❎ replica hasn't updated yet
⏳ rolling update in progress

	Partition	Replicas	R-0	R-1	R-2	R-3	R-4	R-5	Note
Stage1	0	4	✅	✅	✅	✅			Before rolling update
Stage2	4	6	❎	❎	❎	❎	⏳	⏳	Rolling update started, bursted 2 replicas, Partition=lws.replicas
Stage3	2	6	❎	❎	⏳	⏳	⏳	⏳	Partition changes to 2 (6-4)
Stage4	2	6	❎	❎	⏳	⏳	✅	⏳	Since the last replica is not ready, Partition will not change
Stage5	0	6	⏳	⏳	⏳	⏳	✅	✅	Partition = 6-4-2=0
Stage6	0	6	⏳	⏳	⏳	✅	✅	✅
Stage7	0	5	⏳	✅	⏳	✅	✅		Release a replica when unavailableReplicas == maxSurge
Stage8	0	4	✅	⏳	✅	✅			Release another replica for accommodation
Stage9	0	4	✅	✅	✅	✅			Rolling update finished

kerthcet · 2024-04-30T06:59:03Z

/retest

kerthcet · 2024-04-30T07:03:00Z

/test pull-lws-test-integration-main

kerthcet · 2024-04-30T07:03:38Z

Have no idea why https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_lws/98/pull-lws-test-integration-main/1785199425849659392 is failed, maybe the controller is stuck.

ahg-g · 2024-04-30T07:15:40Z

How rolling update looks like right now with maxSurge, let's say we have a lws with replicas=4, maxUnavailable=2, maxSurge=2, rollinStep = 2+2 = 4:

✅ replica updated successfully

❎ replica hasn't updated yet

⏳ rolling update in progress

Partition Replicas R-0 R-1 R-2 R-3 R-4 R-5 Note
Stage1 0 4 ✅ ✅ ✅ ✅ Before rolling update
Stage2 4 6 ❎ ❎ ❎ ❎ ⏳ ⏳ Rolling update started, bursted 2 replicas, Partition=lws.replicas
Stage3 2 6 ❎ ❎ ⏳ ⏳ ⏳ ⏳ Partition changes to 2 (6-4)
Stage4 2 6 ❎ ❎ ⏳ ⏳ ✅ ⏳ Since the last replica is not ready, Partition will not change
Stage5 0 6 ⏳ ⏳ ⏳ ⏳ ✅ ✅ Partition = 6-4-2=0
Stage6 0 6 ⏳ ⏳ ⏳ ✅ ✅ ✅
Stage7 0 5 ⏳ ✅ ⏳ ✅ ✅ Release a replica when unavailableReplicas == maxSurge
Stage8 0 4 ✅ ⏳ ✅ ✅ Release another replica for accommodation
Stage9 0 4 ✅ ✅ ✅ ✅ Rolling update finished

This is fantastics and makes a lot of sense, we need to make sure this is well documented!

liurupeng · 2024-05-02T05:27:05Z

pkg/controllers/leaderworkerset_controller.go

@@ -94,13 +94,13 @@ func (r *LeaderWorkerSetReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 	log := ctrl.LoggerFrom(ctx).WithValues("leaderworkerset", klog.KObj(lws))
 	ctx = ctrl.LoggerInto(ctx, log)

-	partition, err := r.rollingUpdatePartition(ctx, lws)
+	partition, replicas, err := r.rollingUpdateParameters(ctx, lws)


Suggested change

partition, replicas, err := r.rollingUpdateParameters(ctx, lws)

partition, desiredReplicasCount, err := r.rollingUpdateParameters(ctx, lws)

How do you feel like renaming it as "desiredReplicasCount" so that we could know that this will be set as the real replica count

Outside the rollingupdateParameters, there's only one replicas, I guess it's ok.

test/integration/controllers/leaderworkerset_test.go

liurupeng · 2024-05-02T06:28:59Z

overall lgtm! Thanks Kante!

ahg-g

This is great, really impressive!

ahg-g · 2024-05-01T20:44:07Z

pkg/controllers/leaderworkerset_controller.go

+	}
+	// No need to burst more than the replicas.
+	if maxSurge > int(lwsReplicas) {
+		maxSurge = int(lwsReplicas)


Should this just be part of the api validation?

Replicas is elastic, same to maxUnavailable, we can't force the maxSurge <= Replicas.

pkg/controllers/leaderworkerset_controller.go

ahg-g · 2024-05-01T20:48:11Z

pkg/controllers/leaderworkerset_controller.go

 	sts := &appsv1.StatefulSet{}
-	err := r.Get(ctx, types.NamespacedName{Name: lws.Name, Namespace: lws.Namespace}, sts)
+	err = r.Get(ctx, types.NamespacedName{Name: lws.Name, Namespace: lws.Namespace}, sts)


I think getting the sts should be done outside this function, and the sts is passed as a parameter. It doesn't seem relevant here.

Move this out we can't tell whether it's because of sts not found or request error, unless we have the logics outside, prefer to leave here.

pkg/controllers/leaderworkerset_controller.go

ahg-g · 2024-05-05T03:29:43Z

pkg/controllers/leaderworkerset_controller.go

+
+	defer func() {
+		// Reclaim the bursted replicas gradually.
+		if lwsUnreadyReplicas <= int32(maxSurge) {


I find it hard to reason about when using a defer function and named return variables, I suggest to directly put the value in the return statements below and remove the named return variable and defer func.

Also, I am not sure the calculation here is correct, shouldn't it always be the following (without the if statement above):

replicas = lwsReplicas + min(utils.NonZeroValue(lwsUnreadyReplicas-maxUnavailable), maxSurge)

The general idea is like when we have 2 unready replicas, and 2 burst replicas, what can happen is the 2 unready replicas may stuck in pending as stockout, so what I want to achieve here is at this moment, let us reclaim one replica from the burst ones and make room for the unready ones.

We can also reclaim replicas >= 1 as well, actually I think this has nothing related to maxUnavailable. It's about how many replicas we should reclaim each time.

api/leaderworkerset/v1/leaderworkerset_types.go

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

kerthcet · 2024-05-07T02:28:18Z

/retest

kerthcet · 2024-05-08T08:45:08Z

kindly ping @ahg-g @liurupeng can we push this forward. Hope not to rebase again. 😅

liurupeng · 2024-05-08T16:10:55Z

/lgtm
/approve

k8s-ci-robot · 2024-05-08T16:11:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kerthcet, liurupeng

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,liurupeng]

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 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. labels Apr 12, 2024

k8s-ci-robot requested review from ahg-g and liurupeng April 12, 2024 09:08

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 12, 2024

kerthcet force-pushed the feat/support-maxSurge branch from 5dadf9e to 4f3a924 Compare April 12, 2024 09:09

kerthcet force-pushed the feat/support-maxSurge branch from 4f3a924 to 809fe16 Compare April 12, 2024 10:53

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

kerthcet force-pushed the feat/support-maxSurge branch from ec3d3bc to ad84cad Compare April 16, 2024 11:30

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 16, 2024

kerthcet changed the title ~~[WIP]Support maxSurge in rolling update~~ Support maxSurge in rolling update Apr 16, 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 Apr 16, 2024

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

kerthcet mentioned this pull request Apr 17, 2024

Mimic LeaderWorkerSet status as StatefulSet or Deployment #96

Closed

kerthcet force-pushed the feat/support-maxSurge branch from ad84cad to ecd806e Compare April 17, 2024 07:43

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

liurupeng reviewed Apr 18, 2024

View reviewed changes

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

liurupeng reviewed Apr 18, 2024

View reviewed changes

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

liurupeng reviewed Apr 18, 2024

View reviewed changes

test/e2e/e2e_test.go Show resolved Hide resolved

liurupeng reviewed Apr 18, 2024

View reviewed changes

test/integration/controllers/leaderworkerset_test.go Show resolved Hide resolved

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

kerthcet force-pushed the feat/support-maxSurge branch from ecd806e to 229a677 Compare April 30, 2024 06:47

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 30, 2024

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

liurupeng reviewed May 2, 2024

View reviewed changes

ahg-g reviewed May 5, 2024

View reviewed changes

Support maxSurge in rolling update

3a7fbc0

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

kerthcet force-pushed the feat/support-maxSurge branch from 229a677 to 8592a8a Compare May 6, 2024 10:30

Allow maxUnavailable to 0 when maxSurge is not 0

2635627

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

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

fix comments

0dcf3ce

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

kerthcet force-pushed the feat/support-maxSurge branch from 8592a8a to 0dcf3ce Compare May 6, 2024 15:38

k8s-ci-robot assigned liurupeng May 8, 2024

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

k8s-ci-robot merged commit 70ca1c7 into kubernetes-sigs:main May 8, 2024
7 checks passed

liurupeng mentioned this pull request Jun 4, 2024

Release v0.3.0 #159

Closed

20 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support maxSurge in rolling update #98

Support maxSurge in rolling update #98

kerthcet commented Apr 12, 2024

kerthcet commented Apr 12, 2024

kerthcet commented Apr 12, 2024

kerthcet commented Apr 16, 2024

kerthcet commented Apr 17, 2024

kerthcet commented Apr 18, 2024 •

edited

Loading

ahg-g commented Apr 18, 2024

liurupeng commented Apr 18, 2024

kerthcet commented Apr 30, 2024

kerthcet commented Apr 30, 2024

kerthcet commented Apr 30, 2024

kerthcet commented Apr 30, 2024

ahg-g commented Apr 30, 2024 •

edited

Loading

liurupeng May 2, 2024

kerthcet May 6, 2024

liurupeng commented May 2, 2024

ahg-g left a comment

ahg-g May 1, 2024

kerthcet May 6, 2024

ahg-g May 1, 2024

kerthcet May 6, 2024

ahg-g May 5, 2024

kerthcet May 6, 2024

kerthcet commented May 7, 2024

kerthcet commented May 8, 2024

liurupeng commented May 8, 2024

k8s-ci-robot commented May 8, 2024

	partition, replicas, err := r.rollingUpdateParameters(ctx, lws)
	partition, desiredReplicasCount, err := r.rollingUpdateParameters(ctx, lws)

Support maxSurge in rolling update #98

Support maxSurge in rolling update #98

Conversation

kerthcet commented Apr 12, 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?

kerthcet commented Apr 12, 2024

kerthcet commented Apr 12, 2024

kerthcet commented Apr 16, 2024

kerthcet commented Apr 17, 2024

kerthcet commented Apr 18, 2024 • edited Loading

ahg-g commented Apr 18, 2024

liurupeng commented Apr 18, 2024

kerthcet commented Apr 30, 2024

kerthcet commented Apr 30, 2024

kerthcet commented Apr 30, 2024

kerthcet commented Apr 30, 2024

ahg-g commented Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liurupeng commented May 2, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kerthcet commented May 7, 2024

kerthcet commented May 8, 2024

liurupeng commented May 8, 2024

k8s-ci-robot commented May 8, 2024

kerthcet commented Apr 18, 2024 •

edited

Loading

ahg-g commented Apr 30, 2024 •

edited

Loading