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 <Joel.speed@hotmail.co.uk>

added test

.

fix

fix test

fixed a test

gofmt

lint

fix

function name

validation fix

.

godocs added

.

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)
 	}

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)
 	}

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)
 	}

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)
 	}

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
 	}

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
 			}

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")
+}

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")
 	}

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
 	}

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.