From c37c93f47a231a53e2dd4fe2208008df2ae33fbf Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Mon, 9 Nov 2020 10:30:05 -0500 Subject: [PATCH] validation: Handle presence of MaxSurge on DaemonSet When the maxsurge daemonset gate is disabled, the registry and validation must properly handle stripping the field. In the special case where that would leave the MaxUnavailable field set to 0, we must set it to 1 which is the default value. --- pkg/apis/apps/validation/BUILD | 2 + pkg/apis/apps/validation/validation.go | 37 +++- pkg/apis/apps/validation/validation_test.go | 179 ++++++++++++++++ pkg/registry/apps/daemonset/BUILD | 8 + pkg/registry/apps/daemonset/strategy.go | 34 +++ pkg/registry/apps/daemonset/strategy_test.go | 209 +++++++++++++++++++ 6 files changed, 462 insertions(+), 7 deletions(-) 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)) + } + }) + } + +}