allow adding/removing VCTs for StatefulSet with OnPodRollingUpdate #2043

ninavdl · 2025-05-19T09:43:13Z

Ⅰ. Describe what this PR does

Currently, when OnPodRollingUpdate is enabled, as per https://openkruise.io/docs/user-manuals/advancedstatefulset#validation-rules-onpodrollingupdate-mode it is not supported to add/remove volumeClaimTemplates to an existing Advanced StatefulSet.

This PR adds a feature flag RecreatePodWhenChangeVCTInStatefulSetGate (similar to the already existing RecreatePodWhenChangeVCTInCloneSetGate), with which VCTs can be added to an existing StatefulSet with OnPodRollingUpdate. Adding/removing a VCT then causes the Pods to be recreated.

Ⅱ. Does this pull request fix one issue?

NONE

Ⅲ. Describe how to verify it

Create StatefulSet with OnPodRollingUpdate
After reconciliation, add or remove a VCT to the StatefulSet and mount it in a Pod template
Observe that the Pod is recreated with the new VCT.

Ⅳ. Special notes for reviews

kruise-bot · 2025-05-19T09:43:19Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign veophi for approval by writing /assign @veophi in a comment. For more information see:The Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

kruise-bot · 2025-05-19T09:43:29Z

Welcome @ninavdl! It looks like this is your first PR to openkruise/kruise 🎉

…tatefulSet with OnPodRollingUpdate changes Signed-off-by: Nina van der Linden <[email protected]>

codecov · 2025-05-19T10:08:39Z

Codecov Report

Attention: Patch coverage is 87.23404% with 6 lines in your changes missing coverage. Please review.

Project coverage is 43.90%. Comparing base (4025f61) to head (e85fa5b).
Report is 20 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/controller/statefulset/stateful_set_control.go	57.14%	2 Missing and 1 partial ⚠️
...oller/statefulset/statefulset_predownload_image.go	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2043      +/-   ##
==========================================
+ Coverage   43.77%   43.90%   +0.12%     
==========================================
  Files         316      316              
  Lines       31615    31670      +55     
==========================================
+ Hits        13841    13906      +65     
+ Misses      16373    16368       -5     
+ Partials     1401     1396       -5

Flag	Coverage Δ
unittests	`43.90% <87.23%> (+0.12%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: Nina van der Linden <[email protected]>

ABNER-1 · 2025-05-28T02:00:21Z

Hi, @ninavdl.
Thank you for contributing this feature.
I'll review it as soon as possible.

ABNER-1

Hi, @ninavdl .
Great job! Your contribution is highly appreciated.
Here are some suggestions to make it even better.

ABNER-1 · 2025-06-03T11:53:53Z

pkg/util/inplaceupdate/inplace_update_defaults.go

-			if !canInPlace {
-				return nil
-			}
+	if opts.RecreatePodWhenChangedVolumeClaimTemplate && !opts.IgnoreVolumeClaimTemplatesHashDiff {


Why is it necessary to make this change? It doesn't seem essential.

The hash is only recorded for the CloneSet.

When adding or removing VCTs in a StatefulSet, users may need to add or remove corresponding volume mounts. This implies that the Pod cannot be updated in place.

opts.RecreatePodWhenChangedVolumeClaimTemplate is true if the feature gate RecreatePodWhenChangeVCTInCloneSetGate or RecreatePodWhenChangeVCTInStatefulSetGate is enabled

pkg/webhook/statefulset/validating/statefulset_validation.go

ABNER-1 · 2025-06-03T12:10:19Z

test/e2e/apps/statefulset.go

@@ -918,6 +918,110 @@ var _ = SIGDescribe("AppStatefulSetStorage", func() {
 			sst.WaitForStatusPVCReadyReplicas(ss, 1)
 		})
 	})
+
+	ginkgo.Describe("Recreate Pod when modifying volumeClaimTemplate with OnPodRollingUpdate", func() {


Please add cases about sts withInPlaceUpdateReady.

Please add a case about rename volume name in vcts and volumeMounts.

I added a case for renaming VCTs.
What do you mean by sts withInPlaceUpdateReady?

sry.
By default, the StatefulSets created by NewStatefulSet use the Recreate update strategy.
We should add test cases to cover all scenarios (adding, removing, and renaming) for StatefulSets with InPlaceIfPossiblePodUpdateStrategyType update strategy.

You may refer to

kruise/test/e2e/apps/statefulset.go

Lines 85 to 91 in e85fa5b

injectSC := func(podUpdatePolicy appsv1beta1.PodUpdateStrategyType, ss *appsv1beta1.StatefulSet, volumeClaimUpdateStrategy appsv1beta1.VolumeClaimUpdateStrategyType, scNames ...string) {

if podUpdatePolicy == appsv1beta1.InPlaceIfPossiblePodUpdateStrategyType {

ss.Spec.UpdateStrategy.RollingUpdate = &appsv1beta1.RollingUpdateStatefulSetStrategy{

PodUpdatePolicy: podUpdatePolicy,

}

ss.Spec.Template.Spec.ReadinessGates = append(ss.Spec.Template.Spec.ReadinessGates, v1.PodReadinessGate{ConditionType: appspub.InPlaceUpdateReady})

}

ABNER-1 · 2025-06-03T12:33:33Z

Please add a TODO item near UpdatePodClaimForRetentionPolicy to address the removal of PVCs when users delete VCTs.

ABNER-1 · 2025-06-03T12:37:49Z

Please add a TODO item near UpdatePodClaimForRetentionPolicy to address the removal of PVCs when users delete VCTs.

Perhaps we can add a label to help users identify these PVCs.
If we do not recommend deletion during the update process due to the possibility of rollback, this label can allow users to manually clean up PVCs that are no longer referenced by any Pod after a certain period.

Signed-off-by: Nina van der Linden <[email protected]>

furykerry

@ninavdl this patch only ensure the pod is recreated if VCT of statefulset changes, but it does not address the problem of changing existing PVC. currently PVC is only deleted when statefulset scale down or statefulset itself get deleted. So we may have to add another PersistentVolumeClaimRetentionPolicyType to make the patch really work

furykerry · 2025-06-24T05:30:43Z

pkg/webhook/statefulset/validating/statefulset_validation.go

@@ -377,7 +379,8 @@ func ValidateVolumeClaimTemplateUpdate(c client.Client, sts, oldSts *appsv1beta1
 		sts.Spec.VolumeClaimUpdateStrategy.Type == appsv1beta1.OnPVCDeleteVolumeClaimUpdateStrategyType {
 		return nil
 	}
-	if len(sts.Spec.VolumeClaimTemplates) != len(oldSts.Spec.VolumeClaimTemplates) {
+	if len(sts.Spec.VolumeClaimTemplates) != len(oldSts.Spec.VolumeClaimTemplates) &&
+		!utilfeature.DefaultFeatureGate.Enabled(features.RecreatePodWhenChangeVCTInStatefulSetGate) {


maybe we can check against the feature gate features.RecreatePodWhenChangeVCTInStatefulSetGate and return nil early in L379

kruise-bot · 2025-06-27T13:27:25Z

@ninavdl: PR needs rebase.

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.

kruise-bot requested review from shiyan2016 and zmberg May 19, 2025 09:43

kruise-bot added the size/L size/L: 100-499 label May 19, 2025

ABNER-1 self-requested a review May 19, 2025 09:50

ninavdl force-pushed the feature/recreate-pod-statefulset-vct-change branch from 9f1b0df to 10ab9e8 Compare May 19, 2025 09:50

feat: add feature to recreate pod when the volumeClaimTemplate of a S…

04f73b9

…tatefulSet with OnPodRollingUpdate changes Signed-off-by: Nina van der Linden <[email protected]>

ninavdl force-pushed the feature/recreate-pod-statefulset-vct-change branch from 10ab9e8 to 04f73b9 Compare May 19, 2025 09:57

ninavdl force-pushed the feature/recreate-pod-statefulset-vct-change branch from e7fed35 to f2e9155 Compare May 19, 2025 11:46

fix e2e test flakiness by using WaitForRollingUpdate

ea2a91d

Signed-off-by: Nina van der Linden <[email protected]>

ninavdl force-pushed the feature/recreate-pod-statefulset-vct-change branch from f2e9155 to ea2a91d Compare May 19, 2025 11:54

ABNER-1 reviewed Jun 3, 2025

View reviewed changes

ABNER-1 self-assigned this Jun 4, 2025

Nina van der Linden added 2 commits June 16, 2025 13:43

simplify logic for vct update validation

974f736

Signed-off-by: Nina van der Linden <[email protected]>

add test for renaming vct

e85fa5b

Signed-off-by: Nina van der Linden <[email protected]>

ninavdl force-pushed the feature/recreate-pod-statefulset-vct-change branch from 45518a9 to e85fa5b Compare June 16, 2025 11:43

furykerry reviewed Jun 25, 2025

View reviewed changes

kruise-bot added the needs-rebase label Jun 27, 2025

	injectSC := func(podUpdatePolicy appsv1beta1.PodUpdateStrategyType, ss *appsv1beta1.StatefulSet, volumeClaimUpdateStrategy appsv1beta1.VolumeClaimUpdateStrategyType, scNames ...string) {
	if podUpdatePolicy == appsv1beta1.InPlaceIfPossiblePodUpdateStrategyType {
	ss.Spec.UpdateStrategy.RollingUpdate = &appsv1beta1.RollingUpdateStatefulSetStrategy{
	PodUpdatePolicy: podUpdatePolicy,
	}
	ss.Spec.Template.Spec.ReadinessGates = append(ss.Spec.Template.Spec.ReadinessGates, v1.PodReadinessGate{ConditionType: appspub.InPlaceUpdateReady})
	}

allow adding/removing VCTs for StatefulSet with OnPodRollingUpdate #2043

Are you sure you want to change the base?

allow adding/removing VCTs for StatefulSet with OnPodRollingUpdate #2043

Uh oh!

Conversation

ninavdl commented May 19, 2025

Ⅰ. Describe what this PR does

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how to verify it

Ⅳ. Special notes for reviews

Uh oh!

kruise-bot commented May 19, 2025

Uh oh!

kruise-bot commented May 19, 2025

Uh oh!

codecov bot commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ABNER-1 commented May 28, 2025

Uh oh!

ABNER-1 left a comment

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!

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!

ABNER-1 commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ABNER-1 commented Jun 3, 2025

Uh oh!

furykerry left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kruise-bot commented Jun 27, 2025

Uh oh!

Uh oh!

codecov bot commented May 19, 2025 •

edited

Loading

ABNER-1 commented Jun 3, 2025 •

edited

Loading