Merge pull request #111194 from ravisantoshgudimetla/promote-maxSurge-ga

Promote DS max surge to GA
2022-07-25 06:20:46 -07:00
parent a6afdf45dd ced1d2edbd
commit 7156c96e5d
18 changed files with 1363 additions and 1868 deletions
--- a/pkg/registry/apps/daemonset/strategy.go
+++ b/pkg/registry/apps/daemonset/strategy.go
@@ -24,17 +24,14 @@ import (
 	apivalidation "k8s.io/apimachinery/pkg/api/validation"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
-	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
 	"k8s.io/apiserver/pkg/registry/rest"
 	"k8s.io/apiserver/pkg/storage/names"
-	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	"k8s.io/kubernetes/pkg/api/legacyscheme"
 	"k8s.io/kubernetes/pkg/api/pod"
 	"k8s.io/kubernetes/pkg/apis/apps"
 	"k8s.io/kubernetes/pkg/apis/apps/validation"
-	"k8s.io/kubernetes/pkg/features"
 	"sigs.k8s.io/structured-merge-diff/v4/fieldpath"
 )

@@ -82,7 +79,6 @@ func (daemonSetStrategy) PrepareForCreate(ctx context.Context, obj runtime.Objec
 		daemonSet.Spec.TemplateGeneration = 1
 	}

-	dropDaemonSetDisabledFields(daemonSet, nil)
 	pod.DropDisabledTemplateFields(&daemonSet.Spec.Template, nil)
 }

@@ -91,7 +87,6 @@ func (daemonSetStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.
 	newDaemonSet := obj.(*apps.DaemonSet)
 	oldDaemonSet := old.(*apps.DaemonSet)

-	dropDaemonSetDisabledFields(newDaemonSet, oldDaemonSet)
 	pod.DropDisabledTemplateFields(&newDaemonSet.Spec.Template, &oldDaemonSet.Spec.Template)

 	// update is not allowed to set status
@@ -121,35 +116,6 @@ func (daemonSetStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.
 	}
 }

-// dropDaemonSetDisabledFields drops fields that are not used if their associated feature gates
-// are not enabled.  The typical pattern is:
-//     if !utilfeature.DefaultFeatureGate.Enabled(features.MyFeature) && !myFeatureInUse(oldSvc) {
-//         newSvc.Spec.MyFeature = nil
-//     }
-func dropDaemonSetDisabledFields(newDS *apps.DaemonSet, oldDS *apps.DaemonSet) {
-	if !utilfeature.DefaultFeatureGate.Enabled(features.DaemonSetUpdateSurge) {
-		if r := newDS.Spec.UpdateStrategy.RollingUpdate; r != nil {
-			if daemonSetSurgeFieldsInUse(oldDS) {
-				// we need to ensure that MaxUnavailable is non-zero to preserve previous behavior
-				if r.MaxUnavailable.IntVal == 0 && r.MaxUnavailable.StrVal == "0%" {
-					r.MaxUnavailable = intstr.FromInt(1)
-				}
-			} else {
-				// clear the MaxSurge field and let validation deal with MaxUnavailable
-				r.MaxSurge = intstr.IntOrString{}
-			}
-		}
-	}
-}
-
-// daemonSetSurgeFieldsInUse returns true if fields related to daemonset update surge are set
-func daemonSetSurgeFieldsInUse(ds *apps.DaemonSet) bool {
-	if ds == nil {
-		return false
-	}
-	return ds.Spec.UpdateStrategy.RollingUpdate != nil && (ds.Spec.UpdateStrategy.RollingUpdate.MaxSurge.IntVal != 0 || ds.Spec.UpdateStrategy.RollingUpdate.MaxSurge.StrVal != "")
-}
-
 // Validate validates a new daemon set.
 func (daemonSetStrategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList {
 	daemonSet := obj.(*apps.DaemonSet)
--- a/pkg/registry/apps/daemonset/strategy_test.go
+++ b/pkg/registry/apps/daemonset/strategy_test.go
@@ -21,15 +21,10 @@ import (
 	"testing"

 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/util/diff"
-	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
-	utilfeature "k8s.io/apiserver/pkg/util/feature"
-	featuregatetesting "k8s.io/component-base/featuregate/testing"
 	"k8s.io/kubernetes/pkg/apis/apps"
 	api "k8s.io/kubernetes/pkg/apis/core"
-	"k8s.io/kubernetes/pkg/features"
 )

 const (
@@ -139,207 +134,3 @@ func newDaemonSetWithSelectorLabels(selectorLabels map[string]string, templateGe
 		},
 	}
 }
-
-func makeDaemonSetWithSurge(unavailable intstr.IntOrString, surge intstr.IntOrString) *apps.DaemonSet {
-	return &apps.DaemonSet{
-		Spec: apps.DaemonSetSpec{
-			UpdateStrategy: apps.DaemonSetUpdateStrategy{
-				Type: apps.RollingUpdateDaemonSetStrategyType,
-				RollingUpdate: &apps.RollingUpdateDaemonSet{
-					MaxUnavailable: unavailable,
-					MaxSurge:       surge,
-				},
-			},
-		},
-	}
-}
-
-func TestDropDisabledField(t *testing.T) {
-	testCases := []struct {
-		name        string
-		enableSurge bool
-		ds          *apps.DaemonSet
-		old         *apps.DaemonSet
-		expect      *apps.DaemonSet
-	}{
-		{
-			name:        "not surge, no update",
-			enableSurge: false,
-			ds:          &apps.DaemonSet{},
-			old:         nil,
-			expect:      &apps.DaemonSet{},
-		},
-		{
-			name:        "not surge, field not used",
-			enableSurge: false,
-			ds:          makeDaemonSetWithSurge(intstr.FromInt(1), intstr.IntOrString{}),
-			old:         nil,
-			expect:      makeDaemonSetWithSurge(intstr.FromInt(1), intstr.IntOrString{}),
-		},
-		{
-			name:        "not surge, field not used in old and new",
-			enableSurge: false,
-			ds:          makeDaemonSetWithSurge(intstr.FromInt(1), intstr.IntOrString{}),
-			old:         makeDaemonSetWithSurge(intstr.FromInt(1), intstr.IntOrString{}),
-			expect:      makeDaemonSetWithSurge(intstr.FromInt(1), intstr.IntOrString{}),
-		},
-		{
-			name:        "not surge, field used",
-			enableSurge: false,
-			ds:          makeDaemonSetWithSurge(intstr.FromInt(2), intstr.FromInt(1)),
-			old:         makeDaemonSetWithSurge(intstr.FromInt(2), intstr.FromInt(1)),
-			expect:      makeDaemonSetWithSurge(intstr.FromInt(2), intstr.FromInt(1)),
-		},
-		{
-			name:        "not surge, field used, percent",
-			enableSurge: false,
-			ds:          makeDaemonSetWithSurge(intstr.FromInt(2), intstr.FromString("1%")),
-			old:         makeDaemonSetWithSurge(intstr.FromInt(2), intstr.FromString("1%")),
-			expect:      makeDaemonSetWithSurge(intstr.FromInt(2), intstr.FromString("1%")),
-		},
-		{
-			name:        "not surge, field used and cleared",
-			enableSurge: false,
-			ds:          makeDaemonSetWithSurge(intstr.FromInt(2), intstr.IntOrString{}),
-			old:         makeDaemonSetWithSurge(intstr.FromInt(2), intstr.FromInt(1)),
-			expect:      makeDaemonSetWithSurge(intstr.FromInt(2), intstr.IntOrString{}),
-		},
-		{
-			name:        "not surge, field used and cleared, percent",
-			enableSurge: false,
-			ds:          makeDaemonSetWithSurge(intstr.FromInt(2), intstr.IntOrString{}),
-			old:         makeDaemonSetWithSurge(intstr.FromInt(2), intstr.FromString("1%")),
-			expect:      makeDaemonSetWithSurge(intstr.FromInt(2), intstr.IntOrString{}),
-		},
-		{
-			name:        "surge, field not used",
-			enableSurge: true,
-			ds:          makeDaemonSetWithSurge(intstr.FromInt(1), intstr.IntOrString{}),
-			old:         nil,
-			expect:      makeDaemonSetWithSurge(intstr.FromInt(1), intstr.IntOrString{}),
-		},
-		{
-			name:        "surge, field not used in old and new",
-			enableSurge: true,
-			ds:          makeDaemonSetWithSurge(intstr.FromInt(1), intstr.IntOrString{}),
-			old:         makeDaemonSetWithSurge(intstr.FromInt(1), intstr.IntOrString{}),
-			expect:      makeDaemonSetWithSurge(intstr.FromInt(1), intstr.IntOrString{}),
-		},
-		{
-			name:        "surge, field used",
-			enableSurge: true,
-			ds:          makeDaemonSetWithSurge(intstr.IntOrString{}, intstr.FromInt(1)),
-			old:         nil,
-			expect:      makeDaemonSetWithSurge(intstr.IntOrString{}, intstr.FromInt(1)),
-		},
-		{
-			name:        "surge, field used, percent",
-			enableSurge: true,
-			ds:          makeDaemonSetWithSurge(intstr.FromInt(2), intstr.FromString("1%")),
-			old:         makeDaemonSetWithSurge(intstr.FromInt(2), intstr.FromString("1%")),
-			expect:      makeDaemonSetWithSurge(intstr.FromInt(2), intstr.FromString("1%")),
-		},
-		{
-			name:        "surge, field used in old and new",
-			enableSurge: true,
-			ds:          makeDaemonSetWithSurge(intstr.IntOrString{}, intstr.FromInt(1)),
-			old:         makeDaemonSetWithSurge(intstr.IntOrString{}, intstr.FromInt(1)),
-			expect:      makeDaemonSetWithSurge(intstr.IntOrString{}, intstr.FromInt(1)),
-		},
-		{
-			name:        "surge, allows both fields (validation must catch)",
-			enableSurge: true,
-			ds:          makeDaemonSetWithSurge(intstr.FromInt(2), intstr.FromInt(1)),
-			old:         makeDaemonSetWithSurge(intstr.FromInt(2), intstr.FromInt(1)),
-			expect:      makeDaemonSetWithSurge(intstr.FromInt(2), intstr.FromInt(1)),
-		},
-		{
-			name:        "surge, allows change from unavailable to surge",
-			enableSurge: true,
-			ds:          makeDaemonSetWithSurge(intstr.FromInt(2), intstr.IntOrString{}),
-			old:         makeDaemonSetWithSurge(intstr.IntOrString{}, intstr.FromInt(1)),
-			expect:      makeDaemonSetWithSurge(intstr.FromInt(2), intstr.IntOrString{}),
-		},
-		{
-			name:        "surge, allows change from surge to unvailable",
-			enableSurge: true,
-			ds:          makeDaemonSetWithSurge(intstr.IntOrString{}, intstr.FromInt(1)),
-			old:         makeDaemonSetWithSurge(intstr.FromInt(2), intstr.IntOrString{}),
-			expect:      makeDaemonSetWithSurge(intstr.IntOrString{}, intstr.FromInt(1)),
-		},
-		{
-			name:        "not surge, allows change from unavailable to surge",
-			enableSurge: false,
-			ds:          makeDaemonSetWithSurge(intstr.FromInt(2), intstr.IntOrString{}),
-			old:         makeDaemonSetWithSurge(intstr.IntOrString{}, intstr.FromInt(1)),
-			expect:      makeDaemonSetWithSurge(intstr.FromInt(2), intstr.IntOrString{}),
-		},
-		{
-			name:        "not surge, allows change from surge to unvailable",
-			enableSurge: false,
-			ds:          makeDaemonSetWithSurge(intstr.IntOrString{}, intstr.FromInt(1)),
-			old:         makeDaemonSetWithSurge(intstr.FromInt(2), intstr.IntOrString{}),
-			expect:      makeDaemonSetWithSurge(intstr.IntOrString{}, intstr.IntOrString{}),
-		},
-		{
-			name:        "not surge, allows change from unavailable to surge, percent",
-			enableSurge: false,
-			ds:          makeDaemonSetWithSurge(intstr.FromString("2%"), intstr.IntOrString{}),
-			old:         makeDaemonSetWithSurge(intstr.IntOrString{}, intstr.FromString("1%")),
-			expect:      makeDaemonSetWithSurge(intstr.FromString("2%"), intstr.IntOrString{}),
-		},
-		{
-			name:        "not surge, allows change from surge to unvailable, percent",
-			enableSurge: false,
-			ds:          makeDaemonSetWithSurge(intstr.IntOrString{}, intstr.FromString("1%")),
-			old:         makeDaemonSetWithSurge(intstr.FromString("2%"), intstr.IntOrString{}),
-			expect:      makeDaemonSetWithSurge(intstr.IntOrString{}, intstr.IntOrString{}),
-		},
-		{
-			name:        "not surge, resets zero percent, one percent",
-			enableSurge: false,
-			ds:          makeDaemonSetWithSurge(intstr.FromString("0%"), intstr.FromString("1%")),
-			old:         makeDaemonSetWithSurge(intstr.FromString("0%"), intstr.FromString("1%")),
-			expect:      makeDaemonSetWithSurge(intstr.FromInt(1), intstr.FromString("1%")),
-		},
-		{
-			name:        "not surge, resets and clears when zero percent",
-			enableSurge: false,
-			ds:          makeDaemonSetWithSurge(intstr.FromString("0%"), intstr.IntOrString{}),
-			old:         makeDaemonSetWithSurge(intstr.FromString("0%"), intstr.FromString("1%")),
-			expect:      makeDaemonSetWithSurge(intstr.FromInt(1), intstr.IntOrString{}),
-		},
-		{
-			name:        "not surge, sets zero percent, one percent",
-			enableSurge: false,
-			ds:          makeDaemonSetWithSurge(intstr.FromString("0%"), intstr.FromString("1%")),
-			old:         nil,
-			expect:      makeDaemonSetWithSurge(intstr.FromString("0%"), intstr.IntOrString{}),
-		},
-		{
-			name:        "not surge, sets and clears zero percent",
-			enableSurge: false,
-			ds:          makeDaemonSetWithSurge(intstr.FromString("0%"), intstr.IntOrString{}),
-			old:         nil,
-			expect:      makeDaemonSetWithSurge(intstr.FromString("0%"), intstr.IntOrString{}),
-		},
-	}
-	for _, tc := range testCases {
-		t.Run(tc.name, func(t *testing.T) {
-			defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DaemonSetUpdateSurge, tc.enableSurge)()
-			old := tc.old.DeepCopy()
-
-			dropDaemonSetDisabledFields(tc.ds, tc.old)
-
-			// old obj should never be changed
-			if !reflect.DeepEqual(tc.old, old) {
-				t.Fatalf("old ds changed: %v", diff.ObjectReflectDiff(tc.old, old))
-			}
-
-			if !reflect.DeepEqual(tc.ds, tc.expect) {
-				t.Fatalf("unexpected ds spec: %v", diff.ObjectReflectDiff(tc.expect, tc.ds))
-			}
-		})
-	}
-
-}