add feature gate and test both case in ut

Signed-off-by: Abner-1 <[email protected]>
openkruise · Apr 26, 2024 · d5e6703 · d5e6703
1 parent c67ba6e
commit d5e6703
Show file tree

Hide file tree

Showing 4 changed files with 81 additions and 88 deletions.
diff --git a/pkg/controller/cloneset/cloneset_controller_test.go b/pkg/controller/cloneset/cloneset_controller_test.go
@@ -154,12 +154,19 @@ func TestReconcile(t *testing.T) {
 	// Test for pods update
 	testUpdate(g, instance)
 
-	// Test for pods update with volume claim changed
-	testUpdateVolumeClaimTemplates(t, g, instance, nil)
+	testVCTWhenFG := func(enable bool) {
+		defer utilfeature.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.RecreatePodWhenChangeVCTInCloneSetGate, enable)()
 
-	// Clean vct hash before each case => lack vct hash -> keep in-place update
-	testUpdateVolumeClaimTemplates(t, g, instance, cleanVCTHashInRevisions)
+		// Test for pods update with volume claim changed
+		testUpdateVolumeClaimTemplates(t, g, instance, nil)
 
+		// Clean vct hash before each case => lack vct hash -> keep in-place update
+		testUpdateVolumeClaimTemplates(t, g, instance, cleanVCTHashInRevisions)
+	}
+	testVCTWhenFG(true)
+	testVCTWhenFG(false)
+
+	defer utilfeature.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.RecreatePodWhenChangeVCTInCloneSetGate, true)()
 	// Test case with history revision
 	testUpdateVolumeClaimTemplates2(t, g, instance)
 }
@@ -497,7 +504,7 @@ func testUpdateVolumeClaimTemplates(t *testing.T, g *gomega.GomegaWithT, instanc
 	missingVCTHash, volumeSizeCheck := false, true
 	replicas := 4
 	var postHook testCloneSetHookFn = nil
-	if preHook != nil {
+	if preHook != nil || !utilfeature.DefaultFeatureGate.Enabled(features.RecreatePodWhenChangeVCTInCloneSetGate) {
 		// when vct hash missing, recreate pod => in-place update pod
 		missingVCTHash = true
 		volumeSizeCheck = false

diff --git a/pkg/features/kruise_features.go b/pkg/features/kruise_features.go
@@ -116,6 +116,9 @@ const (
 
 	// Enables a enhanced livenessProbe solution
 	EnhancedLivenessProbeGate featuregate.Feature = "EnhancedLivenessProbe"
+
+	// RecreatePodWhenChangeVCTInCloneSetGate recreate the pod upon changing volume claim templates in a clone set to ensure PVC consistency.
+	RecreatePodWhenChangeVCTInCloneSetGate featuregate.Feature = "RecreatePodWhenChangeVCTInCloneSetGate"
 )
 
 var defaultFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{
@@ -145,7 +148,8 @@ var defaultFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{
 	ResourceDistributionGate:              {Default: false, PreRelease: featuregate.Alpha},
 	DeletionProtectionForCRDCascadingGate: {Default: false, PreRelease: featuregate.Alpha},
 
-	EnhancedLivenessProbeGate: {Default: false, PreRelease: featuregate.Alpha},
+	EnhancedLivenessProbeGate:              {Default: false, PreRelease: featuregate.Alpha},
+	RecreatePodWhenChangeVCTInCloneSetGate: {Default: false, PreRelease: featuregate.Alpha},
 }
 
 func init() {

diff --git a/pkg/util/inplaceupdate/inplace_update_defaults.go b/pkg/util/inplaceupdate/inplace_update_defaults.go
@@ -23,6 +23,7 @@ import (
 	"strings"
 
 	"github.com/appscode/jsonpatch"
+
 	appspub "github.com/openkruise/kruise/apis/apps/pub"
 	"github.com/openkruise/kruise/pkg/features"
 	"github.com/openkruise/kruise/pkg/util"
@@ -236,10 +237,13 @@ func defaultCalculateInPlaceUpdateSpec(oldRevision, newRevision *apps.Controller
 		return nil
 	}
 
-	if !opts.IgnoreVolumeClaimTemplatesHashDiff {
-		canInPlace := volumeclaimtemplate.CanVCTemplateInplaceUpdate(oldRevision, newRevision)
-		if !canInPlace {
-			return nil
+	// RecreatePodWhenChangeVCTInCloneSetGate enabled
+	if utilfeature.DefaultFeatureGate.Enabled(features.RecreatePodWhenChangeVCTInCloneSetGate) {
+		if !opts.IgnoreVolumeClaimTemplatesHashDiff {
+			canInPlace := volumeclaimtemplate.CanVCTemplateInplaceUpdate(oldRevision, newRevision)
+			if !canInPlace {
+				return nil
+			}
 		}
 	}
 

diff --git a/pkg/util/inplaceupdate/inplace_update_defaults_test.go b/pkg/util/inplaceupdate/inplace_update_defaults_test.go
@@ -18,12 +18,15 @@ package inplaceupdate
 
 import (
 	"encoding/json"
+	"fmt"
 	"reflect"
 	"testing"
 	"time"
 
 	appspub "github.com/openkruise/kruise/apis/apps/pub"
+	"github.com/openkruise/kruise/pkg/features"
 	"github.com/openkruise/kruise/pkg/util"
+	utilfeature "github.com/openkruise/kruise/pkg/util/feature"
 	"github.com/openkruise/kruise/pkg/util/volumeclaimtemplate"
 
 	apps "k8s.io/api/apps/v1"
@@ -563,11 +566,19 @@ func Test_defaultCalculateInPlaceUpdateSpec_VCTHash(t *testing.T) {
   }
 }`
 
+	desiredWhenDisableFG := &UpdateSpec{
+		Revision: "new-revision",
+		ContainerImages: map[string]string{
+			"nginx": "nginx:stable-alpine",
+		},
+	}
 	ignoreVCTHashUpdateOpts := &UpdateOptions{IgnoreVolumeClaimTemplatesHashDiff: true}
 	tests := []struct {
 		name string
 		args args
 		want *UpdateSpec
+
+		wantWhenDisable *UpdateSpec
 	}{
 		{
 			name: "both revision annotation is nil=> ignore",
@@ -592,12 +603,8 @@ func Test_defaultCalculateInPlaceUpdateSpec_VCTHash(t *testing.T) {
 				},
 				opts: &UpdateOptions{},
 			},
-			want: &UpdateSpec{
-				Revision: "new-revision",
-				ContainerImages: map[string]string{
-					"nginx": "nginx:stable-alpine",
-				},
-			},
+			want:            desiredWhenDisableFG,
+			wantWhenDisable: desiredWhenDisableFG,
 		},
 		{
 			name: "old revision annotation is nil=> ignore",
@@ -622,12 +629,8 @@ func Test_defaultCalculateInPlaceUpdateSpec_VCTHash(t *testing.T) {
 				},
 				opts: &UpdateOptions{},
 			},
-			want: &UpdateSpec{
-				Revision: "new-revision",
-				ContainerImages: map[string]string{
-					"nginx": "nginx:stable-alpine",
-				},
-			},
+			want:            desiredWhenDisableFG,
+			wantWhenDisable: desiredWhenDisableFG,
 		},
 		{
 			name: "new revision annotation is nil => ignore",
@@ -652,12 +655,8 @@ func Test_defaultCalculateInPlaceUpdateSpec_VCTHash(t *testing.T) {
 				},
 				opts: &UpdateOptions{},
 			},
-			want: &UpdateSpec{
-				Revision: "new-revision",
-				ContainerImages: map[string]string{
-					"nginx": "nginx:stable-alpine",
-				},
-			},
+			want:            desiredWhenDisableFG,
+			wantWhenDisable: desiredWhenDisableFG,
 		},
 		{
 			name: "revision annotation changes => recreate",
@@ -682,7 +681,8 @@ func Test_defaultCalculateInPlaceUpdateSpec_VCTHash(t *testing.T) {
 				},
 				opts: &UpdateOptions{},
 			},
-			want: nil,
+			want:            nil,
+			wantWhenDisable: desiredWhenDisableFG,
 		},
 		{
 			name: "the same revision annotation => in-place update",
@@ -707,12 +707,8 @@ func Test_defaultCalculateInPlaceUpdateSpec_VCTHash(t *testing.T) {
 				},
 				opts: &UpdateOptions{},
 			},
-			want: &UpdateSpec{
-				Revision: "new-revision",
-				ContainerImages: map[string]string{
-					"nginx": "nginx:stable-alpine",
-				},
-			},
+			want:            desiredWhenDisableFG,
+			wantWhenDisable: desiredWhenDisableFG,
 		},
 		{
 			name: "both empty revision annotation => in-place update",
@@ -737,12 +733,8 @@ func Test_defaultCalculateInPlaceUpdateSpec_VCTHash(t *testing.T) {
 				},
 				opts: &UpdateOptions{},
 			},
-			want: &UpdateSpec{
-				Revision: "new-revision",
-				ContainerImages: map[string]string{
-					"nginx": "nginx:stable-alpine",
-				},
-			},
+			want:            desiredWhenDisableFG,
+			wantWhenDisable: desiredWhenDisableFG,
 		},
 
 		// IgnoreVolumeClaimTemplatesHashDiff is true
@@ -769,12 +761,8 @@ func Test_defaultCalculateInPlaceUpdateSpec_VCTHash(t *testing.T) {
 				},
 				opts: ignoreVCTHashUpdateOpts,
 			},
-			want: &UpdateSpec{
-				Revision: "new-revision",
-				ContainerImages: map[string]string{
-					"nginx": "nginx:stable-alpine",
-				},
-			},
+			want:            desiredWhenDisableFG,
+			wantWhenDisable: desiredWhenDisableFG,
 		},
 		{
 			name: "IgnoreVolumeClaimTemplatesHashDiff&old revision annotation is nil=> ignore",
@@ -799,12 +787,8 @@ func Test_defaultCalculateInPlaceUpdateSpec_VCTHash(t *testing.T) {
 				},
 				opts: ignoreVCTHashUpdateOpts,
 			},
-			want: &UpdateSpec{
-				Revision: "new-revision",
-				ContainerImages: map[string]string{
-					"nginx": "nginx:stable-alpine",
-				},
-			},
+			want:            desiredWhenDisableFG,
+			wantWhenDisable: desiredWhenDisableFG,
 		},
 		{
 			name: "IgnoreVolumeClaimTemplatesHashDiff&new revision annotation is nil => ignore",
@@ -829,12 +813,8 @@ func Test_defaultCalculateInPlaceUpdateSpec_VCTHash(t *testing.T) {
 				},
 				opts: ignoreVCTHashUpdateOpts,
 			},
-			want: &UpdateSpec{
-				Revision: "new-revision",
-				ContainerImages: map[string]string{
-					"nginx": "nginx:stable-alpine",
-				},
-			},
+			want:            desiredWhenDisableFG,
+			wantWhenDisable: desiredWhenDisableFG,
 		},
 		{
 			name: "IgnoreVolumeClaimTemplatesHashDiff&revision annotation changes => in-place update",
@@ -859,12 +839,8 @@ func Test_defaultCalculateInPlaceUpdateSpec_VCTHash(t *testing.T) {
 				},
 				opts: ignoreVCTHashUpdateOpts,
 			},
-			want: &UpdateSpec{
-				Revision: "new-revision",
-				ContainerImages: map[string]string{
-					"nginx": "nginx:stable-alpine",
-				},
-			},
+			want:            desiredWhenDisableFG,
+			wantWhenDisable: desiredWhenDisableFG,
 		},
 		{
 			name: "IgnoreVolumeClaimTemplatesHashDiff&the same revision annotation => in-place update",
@@ -889,12 +865,8 @@ func Test_defaultCalculateInPlaceUpdateSpec_VCTHash(t *testing.T) {
 				},
 				opts: ignoreVCTHashUpdateOpts,
 			},
-			want: &UpdateSpec{
-				Revision: "new-revision",
-				ContainerImages: map[string]string{
-					"nginx": "nginx:stable-alpine",
-				},
-			},
+			want:            desiredWhenDisableFG,
+			wantWhenDisable: desiredWhenDisableFG,
 		},
 		{
 			name: "IgnoreVolumeClaimTemplatesHashDiff&both empty revision annotation => in-place update",
@@ -919,26 +891,32 @@ func Test_defaultCalculateInPlaceUpdateSpec_VCTHash(t *testing.T) {
 				},
 				opts: ignoreVCTHashUpdateOpts,
 			},
-			want: &UpdateSpec{
-				Revision: "new-revision",
-				ContainerImages: map[string]string{
-					"nginx": "nginx:stable-alpine",
-				},
-			},
+			want:            desiredWhenDisableFG,
+			wantWhenDisable: desiredWhenDisableFG,
 		},
 	}
 
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			got := defaultCalculateInPlaceUpdateSpec(tt.args.oldRevision, tt.args.newRevision, tt.args.opts)
-			if got != nil && tt.want != nil {
-				if !reflect.DeepEqual(got.ContainerImages, tt.want.ContainerImages) {
-					t.Errorf("defaultCalculateInPlaceUpdateSpec() = %v, want %v", got, tt.want)
+	testWhenEnable := func(enable bool) {
+		defer utilfeature.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.RecreatePodWhenChangeVCTInCloneSetGate, enable)()
+		for _, tt := range tests {
+			t.Run(fmt.Sprintf("%v-%v", tt.name, enable), func(t *testing.T) {
+				got := defaultCalculateInPlaceUpdateSpec(tt.args.oldRevision, tt.args.newRevision, tt.args.opts)
+				wanted := tt.wantWhenDisable
+				if utilfeature.DefaultFeatureGate.Enabled(features.RecreatePodWhenChangeVCTInCloneSetGate) {
+					wanted = tt.want
 				}
-			} else if !(got == nil && tt.want == nil) {
-				t.Errorf("defaultCalculateInPlaceUpdateSpec() = %v, want %v", got, tt.want)
-			}
-			// got == nil && tt.want == nil => pass
-		})
+				if got != nil && wanted != nil {
+					if !reflect.DeepEqual(got.ContainerImages, wanted.ContainerImages) {
+						t.Errorf("defaultCalculateInPlaceUpdateSpec() = %v, want %v", got, wanted)
+					}
+				} else if !(got == nil && wanted == nil) {
+					t.Errorf("defaultCalculateInPlaceUpdateSpec() = %v, want %v", got, wanted)
+				}
+				// got == nil && tt.want == nil => pass
+			})
+		}
 	}
+	testWhenEnable(true)
+	testWhenEnable(false)
+
 }