From 316eff8dee1bb7a5929a28bc07bace910bb0e126 Mon Sep 17 00:00:00 2001 From: Renato Viana Date: Sun, 22 Mar 2020 23:41:45 +0000 Subject: [PATCH] Fixed percentage behavior in instr fixed syntax, wrote a test fixed a test . 1 Update staging/src/k8s.io/apimachinery/pkg/util/intstr/intstr_test.go Co-Authored-By: Joel Speed added test . fix fix test fixed a test gofmt lint fix function name validation fix . godocs added . --- pkg/apis/apps/v1/defaults_test.go | 2 +- pkg/apis/apps/v1beta1/defaults_test.go | 2 +- pkg/apis/apps/v1beta2/defaults_test.go | 2 +- pkg/controller/daemon/update.go | 2 +- .../deployment/util/deployment_util.go | 6 +- pkg/controller/disruption/disruption.go | 4 +- .../apimachinery/pkg/util/intstr/intstr.go | 51 ++++++++++++ .../pkg/util/intstr/intstr_test.go | 82 ++++++++++++++++++- .../kubectl/pkg/util/deployment/deployment.go | 4 +- test/e2e/apps/deployment.go | 4 +- 10 files changed, 143 insertions(+), 16 deletions(-) diff --git a/pkg/apis/apps/v1/defaults_test.go b/pkg/apis/apps/v1/defaults_test.go index 88648238e86..0d44bcef56a 100644 --- a/pkg/apis/apps/v1/defaults_test.go +++ b/pkg/apis/apps/v1/defaults_test.go @@ -465,7 +465,7 @@ func TestSetDefaultDeployment(t *testing.T) { func TestDefaultDeploymentAvailability(t *testing.T) { d := roundTrip(t, runtime.Object(&appsv1.Deployment{})).(*appsv1.Deployment) - maxUnavailable, err := intstr.GetValueFromIntOrPercent(d.Spec.Strategy.RollingUpdate.MaxUnavailable, int(*(d.Spec.Replicas)), false) + maxUnavailable, err := intstr.GetScaledValueFromIntOrPercent(d.Spec.Strategy.RollingUpdate.MaxUnavailable, int(*(d.Spec.Replicas)), false) if err != nil { t.Fatalf("unexpected error: %v", err) } diff --git a/pkg/apis/apps/v1beta1/defaults_test.go b/pkg/apis/apps/v1beta1/defaults_test.go index be10a078f5b..a36ce7aee40 100644 --- a/pkg/apis/apps/v1beta1/defaults_test.go +++ b/pkg/apis/apps/v1beta1/defaults_test.go @@ -187,7 +187,7 @@ func TestSetDefaultDeployment(t *testing.T) { func TestDefaultDeploymentAvailability(t *testing.T) { d := roundTrip(t, runtime.Object(&appsv1beta1.Deployment{})).(*appsv1beta1.Deployment) - maxUnavailable, err := intstr.GetValueFromIntOrPercent(d.Spec.Strategy.RollingUpdate.MaxUnavailable, int(*(d.Spec.Replicas)), false) + maxUnavailable, err := intstr.GetScaledValueFromIntOrPercent(d.Spec.Strategy.RollingUpdate.MaxUnavailable, int(*(d.Spec.Replicas)), false) if err != nil { t.Fatalf("unexpected error: %v", err) } diff --git a/pkg/apis/apps/v1beta2/defaults_test.go b/pkg/apis/apps/v1beta2/defaults_test.go index 9edf217ea41..7c9f6c3d6a6 100644 --- a/pkg/apis/apps/v1beta2/defaults_test.go +++ b/pkg/apis/apps/v1beta2/defaults_test.go @@ -435,7 +435,7 @@ func TestSetDefaultDeployment(t *testing.T) { func TestDefaultDeploymentAvailability(t *testing.T) { d := roundTrip(t, runtime.Object(&appsv1beta2.Deployment{})).(*appsv1beta2.Deployment) - maxUnavailable, err := intstr.GetValueFromIntOrPercent(d.Spec.Strategy.RollingUpdate.MaxUnavailable, int(*(d.Spec.Replicas)), false) + maxUnavailable, err := intstr.GetScaledValueFromIntOrPercent(d.Spec.Strategy.RollingUpdate.MaxUnavailable, int(*(d.Spec.Replicas)), false) if err != nil { t.Fatalf("unexpected error: %v", err) } diff --git a/pkg/controller/daemon/update.go b/pkg/controller/daemon/update.go index 1b7620fb355..c8a67c35bca 100644 --- a/pkg/controller/daemon/update.go +++ b/pkg/controller/daemon/update.go @@ -415,7 +415,7 @@ func (dsc *DaemonSetsController) getUnavailableNumbers(ds *apps.DaemonSet, nodeL numUnavailable++ } } - maxUnavailable, err := intstrutil.GetValueFromIntOrPercent(ds.Spec.UpdateStrategy.RollingUpdate.MaxUnavailable, desiredNumberScheduled, true) + maxUnavailable, err := intstrutil.GetScaledValueFromIntOrPercent(ds.Spec.UpdateStrategy.RollingUpdate.MaxUnavailable, desiredNumberScheduled, true) if err != nil { return -1, -1, fmt.Errorf("invalid value for MaxUnavailable: %v", err) } diff --git a/pkg/controller/deployment/util/deployment_util.go b/pkg/controller/deployment/util/deployment_util.go index 4312ef03040..fb1af676d4c 100644 --- a/pkg/controller/deployment/util/deployment_util.go +++ b/pkg/controller/deployment/util/deployment_util.go @@ -817,7 +817,7 @@ func NewRSNewReplicas(deployment *apps.Deployment, allRSs []*apps.ReplicaSet, ne switch deployment.Spec.Strategy.Type { case apps.RollingUpdateDeploymentStrategyType: // Check if we can scale up. - maxSurge, err := intstrutil.GetValueFromIntOrPercent(deployment.Spec.Strategy.RollingUpdate.MaxSurge, int(*(deployment.Spec.Replicas)), true) + maxSurge, err := intstrutil.GetScaledValueFromIntOrPercent(deployment.Spec.Strategy.RollingUpdate.MaxSurge, int(*(deployment.Spec.Replicas)), true) if err != nil { return 0, err } @@ -881,11 +881,11 @@ func WaitForObservedDeployment(getDeploymentFunc func() (*apps.Deployment, error // 2 desired, max unavailable 0%, surge 1% - should scale new(+1), then old(-1), then new(+1), then old(-1) // 1 desired, max unavailable 0%, surge 1% - should scale new(+1), then old(-1) func ResolveFenceposts(maxSurge, maxUnavailable *intstrutil.IntOrString, desired int32) (int32, int32, error) { - surge, err := intstrutil.GetValueFromIntOrPercent(intstrutil.ValueOrDefault(maxSurge, intstrutil.FromInt(0)), int(desired), true) + surge, err := intstrutil.GetScaledValueFromIntOrPercent(intstrutil.ValueOrDefault(maxSurge, intstrutil.FromInt(0)), int(desired), true) if err != nil { return 0, 0, err } - unavailable, err := intstrutil.GetValueFromIntOrPercent(intstrutil.ValueOrDefault(maxUnavailable, intstrutil.FromInt(0)), int(desired), false) + unavailable, err := intstrutil.GetScaledValueFromIntOrPercent(intstrutil.ValueOrDefault(maxUnavailable, intstrutil.FromInt(0)), int(desired), false) if err != nil { return 0, 0, err } diff --git a/pkg/controller/disruption/disruption.go b/pkg/controller/disruption/disruption.go index c0a02c2b50a..78f274e7427 100644 --- a/pkg/controller/disruption/disruption.go +++ b/pkg/controller/disruption/disruption.go @@ -599,7 +599,7 @@ func (dc *DisruptionController) getExpectedPodCount(pdb *policy.PodDisruptionBud return } var maxUnavailable int - maxUnavailable, err = intstr.GetValueFromIntOrPercent(pdb.Spec.MaxUnavailable, int(expectedCount), true) + maxUnavailable, err = intstr.GetScaledValueFromIntOrPercent(pdb.Spec.MaxUnavailable, int(expectedCount), true) if err != nil { return } @@ -618,7 +618,7 @@ func (dc *DisruptionController) getExpectedPodCount(pdb *policy.PodDisruptionBud } var minAvailable int - minAvailable, err = intstr.GetValueFromIntOrPercent(pdb.Spec.MinAvailable, int(expectedCount), true) + minAvailable, err = intstr.GetScaledValueFromIntOrPercent(pdb.Spec.MinAvailable, int(expectedCount), true) if err != nil { return } diff --git a/staging/src/k8s.io/apimachinery/pkg/util/intstr/intstr.go b/staging/src/k8s.io/apimachinery/pkg/util/intstr/intstr.go index 6576def82e7..6ef2c557ab4 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/intstr/intstr.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/intstr/intstr.go @@ -151,6 +151,33 @@ func ValueOrDefault(intOrPercent *IntOrString, defaultValue IntOrString) *IntOrS return intOrPercent } +// GetScaledValueFromIntOrPercent is meant to replace GetValueFromIntOrPercent. +// This method returns a scaled value from an IntOrString type. If the IntOrString +// is a percentage string value it's treated as a percentage and scaled appropriately +// in accordance to the total, if it's an int value it's treated as a a simple value and +// if it is a string value which is either non-numeric or numeric but lacking a trailing '%' it returns an error. +func GetScaledValueFromIntOrPercent(intOrPercent *IntOrString, total int, roundUp bool) (int, error) { + if intOrPercent == nil { + return 0, errors.New("nil value for IntOrString") + } + value, isPercent, err := getIntOrPercentValueSafely(intOrPercent) + if err != nil { + return 0, fmt.Errorf("invalid value for IntOrString: %v", err) + } + if isPercent { + if roundUp { + value = int(math.Ceil(float64(value) * (float64(total)) / 100)) + } else { + value = int(math.Floor(float64(value) * (float64(total)) / 100)) + } + } + return value, nil +} + +// GetValueFromIntOrPercent was deprecated in favor of +// GetScaledValueFromIntOrPercent. This method was treating all int as a numeric value and all +// strings with or without a percent symbol as a percentage value. +// Deprecated func GetValueFromIntOrPercent(intOrPercent *IntOrString, total int, roundUp bool) (int, error) { if intOrPercent == nil { return 0, errors.New("nil value for IntOrString") @@ -169,6 +196,8 @@ func GetValueFromIntOrPercent(intOrPercent *IntOrString, total int, roundUp bool return value, nil } +// getIntOrPercentValue is a legacy function and only meant to be called by GetValueFromIntOrPercent +// For a more correct implementation call getIntOrPercentSafely func getIntOrPercentValue(intOrStr *IntOrString) (int, bool, error) { switch intOrStr.Type { case Int: @@ -183,3 +212,25 @@ func getIntOrPercentValue(intOrStr *IntOrString) (int, bool, error) { } return 0, false, fmt.Errorf("invalid type: neither int nor percentage") } + +func getIntOrPercentValueSafely(intOrStr *IntOrString) (int, bool, error) { + switch intOrStr.Type { + case Int: + return intOrStr.IntValue(), false, nil + case String: + isPercent := false + s := intOrStr.StrVal + if strings.HasSuffix(s, "%") { + isPercent = true + s = strings.TrimSuffix(intOrStr.StrVal, "%") + } else { + return 0, false, fmt.Errorf("invalid type: string is not a percentage") + } + v, err := strconv.Atoi(s) + if err != nil { + return 0, false, fmt.Errorf("invalid value %q: %v", intOrStr.StrVal, err) + } + return int(v), isPercent, nil + } + return 0, false, fmt.Errorf("invalid type: neither int nor percentage") +} diff --git a/staging/src/k8s.io/apimachinery/pkg/util/intstr/intstr_test.go b/staging/src/k8s.io/apimachinery/pkg/util/intstr/intstr_test.go index f6e082987d8..a1ad9cb36d8 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/intstr/intstr_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/intstr/intstr_test.go @@ -110,7 +110,79 @@ func TestIntOrStringMarshalJSONUnmarshalYAML(t *testing.T) { } } -func TestGetValueFromIntOrPercent(t *testing.T) { +func TestGetIntFromIntOrString(t *testing.T) { + tests := []struct { + input IntOrString + expectErr bool + expectVal int + expectPerc bool + }{ + { + input: FromInt(200), + expectErr: false, + expectVal: 200, + expectPerc: false, + }, + { + input: FromString("200"), + expectErr: true, + expectPerc: false, + }, + { + input: FromString("30%0"), + expectErr: true, + expectPerc: false, + }, + { + input: FromString("40%"), + expectErr: false, + expectVal: 40, + expectPerc: true, + }, + { + input: FromString("%"), + expectErr: true, + expectPerc: false, + }, + { + input: FromString("a%"), + expectErr: true, + expectPerc: false, + }, + { + input: FromString("a"), + expectErr: true, + expectPerc: false, + }, + { + input: FromString("40#"), + expectErr: true, + expectPerc: false, + }, + { + input: FromString("40%%"), + expectErr: true, + expectPerc: false, + }, + } + for _, test := range tests { + t.Run("", func(t *testing.T) { + value, isPercent, err := getIntOrPercentValueSafely(&test.input) + if test.expectVal != value { + t.Fatalf("expected value does not match, expected: %d, got: %d", test.expectVal, value) + } + if test.expectPerc != isPercent { + t.Fatalf("expected percent does not match, expected: %t, got: %t", test.expectPerc, isPercent) + } + if test.expectErr != (err != nil) { + t.Fatalf("expected error does not match, expected error: %v, got: %v", test.expectErr, err) + } + }) + } + +} + +func TestGetIntFromIntOrPercent(t *testing.T) { tests := []struct { input IntOrString total int @@ -156,11 +228,15 @@ func TestGetValueFromIntOrPercent(t *testing.T) { input: FromString("#%"), expectErr: true, }, + { + input: FromString("90"), + expectErr: true, + }, } for i, test := range tests { t.Logf("test case %d", i) - value, err := GetValueFromIntOrPercent(&test.input, test.total, test.roundUp) + value, err := GetScaledValueFromIntOrPercent(&test.input, test.total, test.roundUp) if test.expectErr && err == nil { t.Errorf("expected error, but got none") continue @@ -176,7 +252,7 @@ func TestGetValueFromIntOrPercent(t *testing.T) { } func TestGetValueFromIntOrPercentNil(t *testing.T) { - _, err := GetValueFromIntOrPercent(nil, 0, false) + _, err := GetScaledValueFromIntOrPercent(nil, 0, false) if err == nil { t.Errorf("expected error got none") } diff --git a/staging/src/k8s.io/kubectl/pkg/util/deployment/deployment.go b/staging/src/k8s.io/kubectl/pkg/util/deployment/deployment.go index d44bf3b3463..af956f4b4b7 100644 --- a/staging/src/k8s.io/kubectl/pkg/util/deployment/deployment.go +++ b/staging/src/k8s.io/kubectl/pkg/util/deployment/deployment.go @@ -208,11 +208,11 @@ func findOldReplicaSets(deployment *appsv1.Deployment, rsList []*appsv1.ReplicaS // 2 desired, max unavailable 0%, surge 1% - should scale new(+1), then old(-1), then new(+1), then old(-1) // 1 desired, max unavailable 0%, surge 1% - should scale new(+1), then old(-1) func ResolveFenceposts(maxSurge, maxUnavailable *intstrutil.IntOrString, desired int32) (int32, int32, error) { - surge, err := intstrutil.GetValueFromIntOrPercent(intstrutil.ValueOrDefault(maxSurge, intstrutil.FromInt(0)), int(desired), true) + surge, err := intstrutil.GetScaledValueFromIntOrPercent(intstrutil.ValueOrDefault(maxSurge, intstrutil.FromInt(0)), int(desired), true) if err != nil { return 0, 0, err } - unavailable, err := intstrutil.GetValueFromIntOrPercent(intstrutil.ValueOrDefault(maxUnavailable, intstrutil.FromInt(0)), int(desired), false) + unavailable, err := intstrutil.GetScaledValueFromIntOrPercent(intstrutil.ValueOrDefault(maxUnavailable, intstrutil.FromInt(0)), int(desired), false) if err != nil { return 0, 0, err } diff --git a/test/e2e/apps/deployment.go b/test/e2e/apps/deployment.go index 915ba28c069..ebfecb7dffe 100644 --- a/test/e2e/apps/deployment.go +++ b/test/e2e/apps/deployment.go @@ -755,7 +755,7 @@ func testProportionalScalingDeployment(f *framework.Framework) { framework.ExpectNoError(err) // Checking state of first rollout's replicaset. - maxUnavailable, err := intstr.GetValueFromIntOrPercent(deployment.Spec.Strategy.RollingUpdate.MaxUnavailable, int(*(deployment.Spec.Replicas)), false) + maxUnavailable, err := intstr.GetScaledValueFromIntOrPercent(deployment.Spec.Strategy.RollingUpdate.MaxUnavailable, int(*(deployment.Spec.Replicas)), false) framework.ExpectNoError(err) // First rollout's replicaset should have Deployment's (replicas - maxUnavailable) = 10 - 2 = 8 available replicas. @@ -780,7 +780,7 @@ func testProportionalScalingDeployment(f *framework.Framework) { secondRS, err := deploymentutil.GetNewReplicaSet(deployment, c.AppsV1()) framework.ExpectNoError(err) - maxSurge, err := intstr.GetValueFromIntOrPercent(deployment.Spec.Strategy.RollingUpdate.MaxSurge, int(*(deployment.Spec.Replicas)), false) + maxSurge, err := intstr.GetScaledValueFromIntOrPercent(deployment.Spec.Strategy.RollingUpdate.MaxSurge, int(*(deployment.Spec.Replicas)), false) framework.ExpectNoError(err) // Second rollout's replicaset should have 0 available replicas.