diff --git a/pkg/apis/apps/validation/BUILD b/pkg/apis/apps/validation/BUILD index b6967f8b796..b2d9ec1e36d 100644 --- a/pkg/apis/apps/validation/BUILD +++ b/pkg/apis/apps/validation/BUILD @@ -14,6 +14,7 @@ go_library( "//pkg/apis/apps:go_default_library", "//pkg/apis/core:go_default_library", "//pkg/apis/core/validation:go_default_library", + "//pkg/features:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/api/equality:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/api/validation:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", @@ -22,6 +23,7 @@ go_library( "//staging/src/k8s.io/apimachinery/pkg/util/intstr:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/validation:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library", + "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library", ], ) diff --git a/pkg/apis/apps/validation/validation.go b/pkg/apis/apps/validation/validation.go index e297c8a75b9..e1705bb75f8 100644 --- a/pkg/apis/apps/validation/validation.go +++ b/pkg/apis/apps/validation/validation.go @@ -28,9 +28,11 @@ import ( "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/validation/field" + utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/kubernetes/pkg/apis/apps" api "k8s.io/kubernetes/pkg/apis/core" apivalidation "k8s.io/kubernetes/pkg/apis/core/validation" + "k8s.io/kubernetes/pkg/features" ) // ValidateStatefulSetName can be used to check whether the given StatefulSet name is valid. @@ -343,14 +345,35 @@ func ValidateDaemonSetSpec(spec *apps.DaemonSetSpec, fldPath *field.Path, opts a // ValidateRollingUpdateDaemonSet validates a given RollingUpdateDaemonSet. func ValidateRollingUpdateDaemonSet(rollingUpdate *apps.RollingUpdateDaemonSet, fldPath *field.Path) field.ErrorList { - allErrs := field.ErrorList{} - allErrs = append(allErrs, ValidatePositiveIntOrPercent(rollingUpdate.MaxUnavailable, fldPath.Child("maxUnavailable"))...) - if getIntOrPercentValue(rollingUpdate.MaxUnavailable) == 0 { - // MaxUnavailable cannot be 0. - allErrs = append(allErrs, field.Invalid(fldPath.Child("maxUnavailable"), rollingUpdate.MaxUnavailable, "cannot be 0")) + var allErrs field.ErrorList + if utilfeature.DefaultFeatureGate.Enabled(features.DaemonSetUpdateSurge) { + // Validate both fields are positive ints or have a percentage value + allErrs = append(allErrs, ValidatePositiveIntOrPercent(rollingUpdate.MaxUnavailable, fldPath.Child("maxUnavailable"))...) + allErrs = append(allErrs, ValidatePositiveIntOrPercent(rollingUpdate.MaxSurge, fldPath.Child("maxSurge"))...) + + // Validate that MaxUnavailable and MaxSurge are not more than 100%. + allErrs = append(allErrs, IsNotMoreThan100Percent(rollingUpdate.MaxUnavailable, fldPath.Child("maxUnavailable"))...) + allErrs = append(allErrs, IsNotMoreThan100Percent(rollingUpdate.MaxSurge, fldPath.Child("maxSurge"))...) + + // Validate exactly one of MaxSurge or MaxUnavailable is non-zero + hasUnavailable := getIntOrPercentValue(rollingUpdate.MaxUnavailable) != 0 + hasSurge := getIntOrPercentValue(rollingUpdate.MaxSurge) != 0 + switch { + case hasUnavailable && hasSurge: + allErrs = append(allErrs, field.Invalid(fldPath.Child("maxSurge"), rollingUpdate.MaxSurge, "may not be set when maxUnavailable is non-zero")) + case !hasUnavailable && !hasSurge: + allErrs = append(allErrs, field.Required(fldPath.Child("maxUnavailable"), "cannot be 0 when maxSurge is 0")) + } + + } else { + allErrs = append(allErrs, ValidatePositiveIntOrPercent(rollingUpdate.MaxUnavailable, fldPath.Child("maxUnavailable"))...) + if getIntOrPercentValue(rollingUpdate.MaxUnavailable) == 0 { + // MaxUnavailable cannot be 0. + allErrs = append(allErrs, field.Invalid(fldPath.Child("maxUnavailable"), rollingUpdate.MaxUnavailable, "cannot be 0")) + } + // Validate that MaxUnavailable is not more than 100%. + allErrs = append(allErrs, IsNotMoreThan100Percent(rollingUpdate.MaxUnavailable, fldPath.Child("maxUnavailable"))...) } - // Validate that MaxUnavailable is not more than 100%. - allErrs = append(allErrs, IsNotMoreThan100Percent(rollingUpdate.MaxUnavailable, fldPath.Child("maxUnavailable"))...) return allErrs } diff --git a/pkg/apis/apps/validation/validation_test.go b/pkg/apis/apps/validation/validation_test.go index 10a5d23b83f..ee6f650917b 100644 --- a/pkg/apis/apps/validation/validation_test.go +++ b/pkg/apis/apps/validation/validation_test.go @@ -2971,3 +2971,182 @@ func TestValidateReplicaSet(t *testing.T) { } } } + +func TestDaemonSetUpdateMaxSurge(t *testing.T) { + testCases := map[string]struct { + ds *apps.RollingUpdateDaemonSet + enableSurge bool + expectError bool + }{ + "invalid: unset": { + ds: &apps.RollingUpdateDaemonSet{}, + expectError: true, + }, + "invalid: zero percent": { + ds: &apps.RollingUpdateDaemonSet{ + MaxUnavailable: intstr.FromString("0%"), + }, + expectError: true, + }, + "invalid: zero": { + ds: &apps.RollingUpdateDaemonSet{ + MaxUnavailable: intstr.FromInt(0), + }, + expectError: true, + }, + "valid: one": { + ds: &apps.RollingUpdateDaemonSet{ + MaxUnavailable: intstr.FromInt(1), + }, + }, + "valid: one percent": { + ds: &apps.RollingUpdateDaemonSet{ + MaxUnavailable: intstr.FromString("1%"), + }, + }, + "valid: 100%": { + ds: &apps.RollingUpdateDaemonSet{ + MaxUnavailable: intstr.FromString("100%"), + }, + }, + "invalid: greater than 100%": { + ds: &apps.RollingUpdateDaemonSet{ + MaxUnavailable: intstr.FromString("101%"), + }, + expectError: true, + }, + + "valid: surge and unavailable set": { + ds: &apps.RollingUpdateDaemonSet{ + MaxUnavailable: intstr.FromString("1%"), + MaxSurge: intstr.FromString("1%"), + }, + }, + + "invalid: surge enabled, unavailable zero percent": { + ds: &apps.RollingUpdateDaemonSet{ + MaxUnavailable: intstr.FromString("0%"), + }, + enableSurge: true, + expectError: true, + }, + "invalid: surge enabled, unavailable zero": { + ds: &apps.RollingUpdateDaemonSet{ + MaxUnavailable: intstr.FromInt(0), + }, + enableSurge: true, + expectError: true, + }, + "valid: surge enabled, unavailable one": { + ds: &apps.RollingUpdateDaemonSet{ + MaxUnavailable: intstr.FromInt(1), + }, + enableSurge: true, + }, + "valid: surge enabled, unavailable one percent": { + ds: &apps.RollingUpdateDaemonSet{ + MaxUnavailable: intstr.FromString("1%"), + }, + enableSurge: true, + }, + "valid: surge enabled, unavailable 100%": { + ds: &apps.RollingUpdateDaemonSet{ + MaxUnavailable: intstr.FromString("100%"), + }, + enableSurge: true, + }, + "invalid: surge enabled, unavailable greater than 100%": { + ds: &apps.RollingUpdateDaemonSet{ + MaxUnavailable: intstr.FromString("101%"), + }, + enableSurge: true, + expectError: true, + }, + + "invalid: surge enabled, surge zero percent": { + ds: &apps.RollingUpdateDaemonSet{ + MaxSurge: intstr.FromString("0%"), + }, + enableSurge: true, + expectError: true, + }, + "invalid: surge enabled, surge zero": { + ds: &apps.RollingUpdateDaemonSet{ + MaxSurge: intstr.FromInt(0), + }, + enableSurge: true, + expectError: true, + }, + "valid: surge enabled, surge one": { + ds: &apps.RollingUpdateDaemonSet{ + MaxSurge: intstr.FromInt(1), + }, + enableSurge: true, + }, + "valid: surge enabled, surge one percent": { + ds: &apps.RollingUpdateDaemonSet{ + MaxSurge: intstr.FromString("1%"), + }, + enableSurge: true, + }, + "valid: surge enabled, surge 100%": { + ds: &apps.RollingUpdateDaemonSet{ + MaxSurge: intstr.FromString("100%"), + }, + enableSurge: true, + }, + "invalid: surge enabled, surge greater than 100%": { + ds: &apps.RollingUpdateDaemonSet{ + MaxSurge: intstr.FromString("101%"), + }, + enableSurge: true, + expectError: true, + }, + + "invalid: surge enabled, surge and unavailable set": { + ds: &apps.RollingUpdateDaemonSet{ + MaxUnavailable: intstr.FromString("1%"), + MaxSurge: intstr.FromString("1%"), + }, + enableSurge: true, + expectError: true, + }, + + "invalid: surge enabled, surge and unavailable zero percent": { + ds: &apps.RollingUpdateDaemonSet{ + MaxUnavailable: intstr.FromString("0%"), + MaxSurge: intstr.FromString("0%"), + }, + enableSurge: true, + expectError: true, + }, + "invalid: surge enabled, surge and unavailable zero": { + ds: &apps.RollingUpdateDaemonSet{ + MaxUnavailable: intstr.FromInt(0), + MaxSurge: intstr.FromInt(0), + }, + enableSurge: true, + expectError: true, + }, + "invalid: surge enabled, surge and unavailable mixed zero": { + ds: &apps.RollingUpdateDaemonSet{ + MaxUnavailable: intstr.FromInt(0), + MaxSurge: intstr.FromString("0%"), + }, + enableSurge: true, + expectError: true, + }, + } + for tcName, tc := range testCases { + t.Run(tcName, func(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DaemonSetUpdateSurge, tc.enableSurge)() + errs := ValidateRollingUpdateDaemonSet(tc.ds, field.NewPath("spec", "updateStrategy", "rollingUpdate")) + if tc.expectError && len(errs) == 0 { + t.Errorf("Unexpected success") + } + if !tc.expectError && len(errs) != 0 { + t.Errorf("Unexpected error(s): %v", errs) + } + }) + } +} diff --git a/pkg/registry/apps/daemonset/BUILD b/pkg/registry/apps/daemonset/BUILD index 68ae8aefac1..42bf43c9353 100644 --- a/pkg/registry/apps/daemonset/BUILD +++ b/pkg/registry/apps/daemonset/BUILD @@ -18,16 +18,19 @@ go_library( "//pkg/api/pod:go_default_library", "//pkg/apis/apps:go_default_library", "//pkg/apis/apps/validation:go_default_library", + "//pkg/features:go_default_library", "//staging/src/k8s.io/api/apps/v1beta2:go_default_library", "//staging/src/k8s.io/api/extensions/v1beta1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/api/equality:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/api/validation:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/util/intstr:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library", "//staging/src/k8s.io/apiserver/pkg/endpoints/request:go_default_library", "//staging/src/k8s.io/apiserver/pkg/registry/rest:go_default_library", "//staging/src/k8s.io/apiserver/pkg/storage/names:go_default_library", + "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library", ], ) @@ -38,10 +41,15 @@ go_test( deps = [ "//pkg/apis/apps:go_default_library", "//pkg/apis/core:go_default_library", + "//pkg/features:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/util/diff:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/util/intstr:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library", "//staging/src/k8s.io/apiserver/pkg/endpoints/request:go_default_library", "//staging/src/k8s.io/apiserver/pkg/registry/rest:go_default_library", + "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library", + "//staging/src/k8s.io/component-base/featuregate/testing:go_default_library", ], ) diff --git a/pkg/registry/apps/daemonset/strategy.go b/pkg/registry/apps/daemonset/strategy.go index 03bdaa89583..af934471bae 100644 --- a/pkg/registry/apps/daemonset/strategy.go +++ b/pkg/registry/apps/daemonset/strategy.go @@ -25,14 +25,17 @@ 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" ) // daemonSetStrategy implements verification logic for daemon sets. @@ -75,6 +78,7 @@ func (daemonSetStrategy) PrepareForCreate(ctx context.Context, obj runtime.Objec daemonSet.Spec.TemplateGeneration = 1 } + dropDaemonSetDisabledFields(daemonSet, nil) pod.DropDisabledTemplateFields(&daemonSet.Spec.Template, nil) } @@ -83,6 +87,7 @@ 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 @@ -112,6 +117,35 @@ 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) diff --git a/pkg/registry/apps/daemonset/strategy_test.go b/pkg/registry/apps/daemonset/strategy_test.go index f48b3fd7e26..d5e30ec230c 100644 --- a/pkg/registry/apps/daemonset/strategy_test.go +++ b/pkg/registry/apps/daemonset/strategy_test.go @@ -21,11 +21,16 @@ 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" "k8s.io/apiserver/pkg/registry/rest" + 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 ( @@ -189,3 +194,207 @@ 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)) + } + }) + } + +}