From d5e6703ed1f8b10e8c24bc9b00c2bbdd7b49f445 Mon Sep 17 00:00:00 2001 From: Abner-1 Date: Fri, 26 Apr 2024 17:37:57 +0800 Subject: [PATCH] add feature gate and test both case in ut Signed-off-by: Abner-1 --- .../cloneset/cloneset_controller_test.go | 17 ++- pkg/features/kruise_features.go | 6 +- .../inplaceupdate/inplace_update_defaults.go | 12 +- .../inplace_update_defaults_test.go | 134 ++++++++---------- 4 files changed, 81 insertions(+), 88 deletions(-) diff --git a/pkg/controller/cloneset/cloneset_controller_test.go b/pkg/controller/cloneset/cloneset_controller_test.go index 32ae55ca16..3a3696a9f5 100644 --- 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 index ff8992e982..a8f442a42f 100644 --- 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 index 65b9849c05..8ab9ce2e38 100644 --- 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 index 5bd833356e..dc1b12f84d 100644 --- 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) + }