Skip to content

Commit 1a66eb7

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes#89482 from renatoviana12/master
fixed percentage behaviour in instr
2 parents bbbab14 + 316eff8 commit 1a66eb7

File tree

10 files changed

+143
-16
lines changed

10 files changed

+143
-16
lines changed

‎pkg/apis/apps/v1/defaults_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -465,7 +465,7 @@ func TestSetDefaultDeployment(t *testing.T) {
465465
func TestDefaultDeploymentAvailability(t *testing.T) {
466466
d := roundTrip(t, runtime.Object(&appsv1.Deployment{})).(*appsv1.Deployment)
467467

468-
maxUnavailable, err := intstr.GetValueFromIntOrPercent(d.Spec.Strategy.RollingUpdate.MaxUnavailable, int(*(d.Spec.Replicas)), false)
468+
maxUnavailable, err := intstr.GetScaledValueFromIntOrPercent(d.Spec.Strategy.RollingUpdate.MaxUnavailable, int(*(d.Spec.Replicas)), false)
469469
if err != nil {
470470
t.Fatalf("unexpected error: %v", err)
471471
}

‎pkg/apis/apps/v1beta1/defaults_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -187,7 +187,7 @@ func TestSetDefaultDeployment(t *testing.T) {
187187
func TestDefaultDeploymentAvailability(t *testing.T) {
188188
d := roundTrip(t, runtime.Object(&appsv1beta1.Deployment{})).(*appsv1beta1.Deployment)
189189

190-
maxUnavailable, err := intstr.GetValueFromIntOrPercent(d.Spec.Strategy.RollingUpdate.MaxUnavailable, int(*(d.Spec.Replicas)), false)
190+
maxUnavailable, err := intstr.GetScaledValueFromIntOrPercent(d.Spec.Strategy.RollingUpdate.MaxUnavailable, int(*(d.Spec.Replicas)), false)
191191
if err != nil {
192192
t.Fatalf("unexpected error: %v", err)
193193
}

‎pkg/apis/apps/v1beta2/defaults_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -435,7 +435,7 @@ func TestSetDefaultDeployment(t *testing.T) {
435435
func TestDefaultDeploymentAvailability(t *testing.T) {
436436
d := roundTrip(t, runtime.Object(&appsv1beta2.Deployment{})).(*appsv1beta2.Deployment)
437437

438-
maxUnavailable, err := intstr.GetValueFromIntOrPercent(d.Spec.Strategy.RollingUpdate.MaxUnavailable, int(*(d.Spec.Replicas)), false)
438+
maxUnavailable, err := intstr.GetScaledValueFromIntOrPercent(d.Spec.Strategy.RollingUpdate.MaxUnavailable, int(*(d.Spec.Replicas)), false)
439439
if err != nil {
440440
t.Fatalf("unexpected error: %v", err)
441441
}

‎pkg/controller/daemon/update.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -415,7 +415,7 @@ func (dsc *DaemonSetsController) getUnavailableNumbers(ds *apps.DaemonSet, nodeL
415415
numUnavailable++
416416
}
417417
}
418-
maxUnavailable, err := intstrutil.GetValueFromIntOrPercent(ds.Spec.UpdateStrategy.RollingUpdate.MaxUnavailable, desiredNumberScheduled, true)
418+
maxUnavailable, err := intstrutil.GetScaledValueFromIntOrPercent(ds.Spec.UpdateStrategy.RollingUpdate.MaxUnavailable, desiredNumberScheduled, true)
419419
if err != nil {
420420
return -1, -1, fmt.Errorf("invalid value for MaxUnavailable: %v", err)
421421
}

‎pkg/controller/deployment/util/deployment_util.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -817,7 +817,7 @@ func NewRSNewReplicas(deployment *apps.Deployment, allRSs []*apps.ReplicaSet, ne
817817
switch deployment.Spec.Strategy.Type {
818818
case apps.RollingUpdateDeploymentStrategyType:
819819
// Check if we can scale up.
820-
maxSurge, err := intstrutil.GetValueFromIntOrPercent(deployment.Spec.Strategy.RollingUpdate.MaxSurge, int(*(deployment.Spec.Replicas)), true)
820+
maxSurge, err := intstrutil.GetScaledValueFromIntOrPercent(deployment.Spec.Strategy.RollingUpdate.MaxSurge, int(*(deployment.Spec.Replicas)), true)
821821
if err != nil {
822822
return 0, err
823823
}
@@ -881,11 +881,11 @@ func WaitForObservedDeployment(getDeploymentFunc func() (*apps.Deployment, error
881881
// 2 desired, max unavailable 0%, surge 1% - should scale new(+1), then old(-1), then new(+1), then old(-1)
882882
// 1 desired, max unavailable 0%, surge 1% - should scale new(+1), then old(-1)
883883
func ResolveFenceposts(maxSurge, maxUnavailable *intstrutil.IntOrString, desired int32) (int32, int32, error) {
884-
surge, err := intstrutil.GetValueFromIntOrPercent(intstrutil.ValueOrDefault(maxSurge, intstrutil.FromInt(0)), int(desired), true)
884+
surge, err := intstrutil.GetScaledValueFromIntOrPercent(intstrutil.ValueOrDefault(maxSurge, intstrutil.FromInt(0)), int(desired), true)
885885
if err != nil {
886886
return 0, 0, err
887887
}
888-
unavailable, err := intstrutil.GetValueFromIntOrPercent(intstrutil.ValueOrDefault(maxUnavailable, intstrutil.FromInt(0)), int(desired), false)
888+
unavailable, err := intstrutil.GetScaledValueFromIntOrPercent(intstrutil.ValueOrDefault(maxUnavailable, intstrutil.FromInt(0)), int(desired), false)
889889
if err != nil {
890890
return 0, 0, err
891891
}

‎pkg/controller/disruption/disruption.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -599,7 +599,7 @@ func (dc *DisruptionController) getExpectedPodCount(pdb *policy.PodDisruptionBud
599599
return
600600
}
601601
var maxUnavailable int
602-
maxUnavailable, err = intstr.GetValueFromIntOrPercent(pdb.Spec.MaxUnavailable, int(expectedCount), true)
602+
maxUnavailable, err = intstr.GetScaledValueFromIntOrPercent(pdb.Spec.MaxUnavailable, int(expectedCount), true)
603603
if err != nil {
604604
return
605605
}
@@ -618,7 +618,7 @@ func (dc *DisruptionController) getExpectedPodCount(pdb *policy.PodDisruptionBud
618618
}
619619

620620
var minAvailable int
621-
minAvailable, err = intstr.GetValueFromIntOrPercent(pdb.Spec.MinAvailable, int(expectedCount), true)
621+
minAvailable, err = intstr.GetScaledValueFromIntOrPercent(pdb.Spec.MinAvailable, int(expectedCount), true)
622622
if err != nil {
623623
return
624624
}

‎staging/src/k8s.io/apimachinery/pkg/util/intstr/intstr.go‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,33 @@ func ValueOrDefault(intOrPercent *IntOrString, defaultValue IntOrString) *IntOrS
135135
return intOrPercent
136136
}
137137

138+
// GetScaledValueFromIntOrPercent is meant to replace GetValueFromIntOrPercent.
139+
// This method returns a scaled value from an IntOrString type. If the IntOrString
140+
// is a percentage string value it's treated as a percentage and scaled appropriately
141+
// in accordance to the total, if it's an int value it's treated as a a simple value and
142+
// if it is a string value which is either non-numeric or numeric but lacking a trailing '%' it returns an error.
143+
func GetScaledValueFromIntOrPercent(intOrPercent *IntOrString, total int, roundUp bool) (int, error) {
144+
if intOrPercent == nil {
145+
return 0, errors.New("nil value for IntOrString")
146+
}
147+
value, isPercent, err := getIntOrPercentValueSafely(intOrPercent)
148+
if err != nil {
149+
return 0, fmt.Errorf("invalid value for IntOrString: %v", err)
150+
}
151+
if isPercent {
152+
if roundUp {
153+
value = int(math.Ceil(float64(value) * (float64(total)) / 100))
154+
} else {
155+
value = int(math.Floor(float64(value) * (float64(total)) / 100))
156+
}
157+
}
158+
return value, nil
159+
}
160+
161+
// GetValueFromIntOrPercent was deprecated in favor of
162+
// GetScaledValueFromIntOrPercent. This method was treating all int as a numeric value and all
163+
// strings with or without a percent symbol as a percentage value.
164+
// Deprecated
138165
func GetValueFromIntOrPercent(intOrPercent *IntOrString, total int, roundUp bool) (int, error) {
139166
if intOrPercent == nil {
140167
return 0, errors.New("nil value for IntOrString")
@@ -153,6 +180,8 @@ func GetValueFromIntOrPercent(intOrPercent *IntOrString, total int, roundUp bool
153180
return value, nil
154181
}
155182

183+
// getIntOrPercentValue is a legacy function and only meant to be called by GetValueFromIntOrPercent
184+
// For a more correct implementation call getIntOrPercentSafely
156185
func getIntOrPercentValue(intOrStr *IntOrString) (int, bool, error) {
157186
switch intOrStr.Type {
158187
case Int:
@@ -167,3 +196,25 @@ func getIntOrPercentValue(intOrStr *IntOrString) (int, bool, error) {
167196
}
168197
return 0, false, fmt.Errorf("invalid type: neither int nor percentage")
169198
}
199+
200+
func getIntOrPercentValueSafely(intOrStr *IntOrString) (int, bool, error) {
201+
switch intOrStr.Type {
202+
case Int:
203+
return intOrStr.IntValue(), false, nil
204+
case String:
205+
isPercent := false
206+
s := intOrStr.StrVal
207+
if strings.HasSuffix(s, "%") {
208+
isPercent = true
209+
s = strings.TrimSuffix(intOrStr.StrVal, "%")
210+
} else {
211+
return 0, false, fmt.Errorf("invalid type: string is not a percentage")
212+
}
213+
v, err := strconv.Atoi(s)
214+
if err != nil {
215+
return 0, false, fmt.Errorf("invalid value %q: %v", intOrStr.StrVal, err)
216+
}
217+
return int(v), isPercent, nil
218+
}
219+
return 0, false, fmt.Errorf("invalid type: neither int nor percentage")
220+
}

‎staging/src/k8s.io/apimachinery/pkg/util/intstr/intstr_test.go‎

Lines changed: 79 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,79 @@ func TestIntOrStringMarshalJSONUnmarshalYAML(t *testing.T) {
110110
}
111111
}
112112

113-
func TestGetValueFromIntOrPercent(t *testing.T) {
113+
func TestGetIntFromIntOrString(t *testing.T) {
114+
tests := []struct {
115+
input IntOrString
116+
expectErr bool
117+
expectVal int
118+
expectPerc bool
119+
}{
120+
{
121+
input: FromInt(200),
122+
expectErr: false,
123+
expectVal: 200,
124+
expectPerc: false,
125+
},
126+
{
127+
input: FromString("200"),
128+
expectErr: true,
129+
expectPerc: false,
130+
},
131+
{
132+
input: FromString("30%0"),
133+
expectErr: true,
134+
expectPerc: false,
135+
},
136+
{
137+
input: FromString("40%"),
138+
expectErr: false,
139+
expectVal: 40,
140+
expectPerc: true,
141+
},
142+
{
143+
input: FromString("%"),
144+
expectErr: true,
145+
expectPerc: false,
146+
},
147+
{
148+
input: FromString("a%"),
149+
expectErr: true,
150+
expectPerc: false,
151+
},
152+
{
153+
input: FromString("a"),
154+
expectErr: true,
155+
expectPerc: false,
156+
},
157+
{
158+
input: FromString("40#"),
159+
expectErr: true,
160+
expectPerc: false,
161+
},
162+
{
163+
input: FromString("40%%"),
164+
expectErr: true,
165+
expectPerc: false,
166+
},
167+
}
168+
for _, test := range tests {
169+
t.Run("", func(t *testing.T) {
170+
value, isPercent, err := getIntOrPercentValueSafely(&test.input)
171+
if test.expectVal != value {
172+
t.Fatalf("expected value does not match, expected: %d, got: %d", test.expectVal, value)
173+
}
174+
if test.expectPerc != isPercent {
175+
t.Fatalf("expected percent does not match, expected: %t, got: %t", test.expectPerc, isPercent)
176+
}
177+
if test.expectErr != (err != nil) {
178+
t.Fatalf("expected error does not match, expected error: %v, got: %v", test.expectErr, err)
179+
}
180+
})
181+
}
182+
183+
}
184+
185+
func TestGetIntFromIntOrPercent(t *testing.T) {
114186
tests := []struct {
115187
input IntOrString
116188
total int
@@ -156,11 +228,15 @@ func TestGetValueFromIntOrPercent(t *testing.T) {
156228
input: FromString("#%"),
157229
expectErr: true,
158230
},
231+
{
232+
input: FromString("90"),
233+
expectErr: true,
234+
},
159235
}
160236

161237
for i, test := range tests {
162238
t.Logf("test case %d", i)
163-
value, err := GetValueFromIntOrPercent(&test.input, test.total, test.roundUp)
239+
value, err := GetScaledValueFromIntOrPercent(&test.input, test.total, test.roundUp)
164240
if test.expectErr && err == nil {
165241
t.Errorf("expected error, but got none")
166242
continue
@@ -176,7 +252,7 @@ func TestGetValueFromIntOrPercent(t *testing.T) {
176252
}
177253

178254
func TestGetValueFromIntOrPercentNil(t *testing.T) {
179-
_, err := GetValueFromIntOrPercent(nil, 0, false)
255+
_, err := GetScaledValueFromIntOrPercent(nil, 0, false)
180256
if err == nil {
181257
t.Errorf("expected error got none")
182258
}

‎staging/src/k8s.io/kubectl/pkg/util/deployment/deployment.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -208,11 +208,11 @@ func findOldReplicaSets(deployment *appsv1.Deployment, rsList []*appsv1.ReplicaS
208208
// 2 desired, max unavailable 0%, surge 1% - should scale new(+1), then old(-1), then new(+1), then old(-1)
209209
// 1 desired, max unavailable 0%, surge 1% - should scale new(+1), then old(-1)
210210
func ResolveFenceposts(maxSurge, maxUnavailable *intstrutil.IntOrString, desired int32) (int32, int32, error) {
211-
surge, err := intstrutil.GetValueFromIntOrPercent(intstrutil.ValueOrDefault(maxSurge, intstrutil.FromInt(0)), int(desired), true)
211+
surge, err := intstrutil.GetScaledValueFromIntOrPercent(intstrutil.ValueOrDefault(maxSurge, intstrutil.FromInt(0)), int(desired), true)
212212
if err != nil {
213213
return 0, 0, err
214214
}
215-
unavailable, err := intstrutil.GetValueFromIntOrPercent(intstrutil.ValueOrDefault(maxUnavailable, intstrutil.FromInt(0)), int(desired), false)
215+
unavailable, err := intstrutil.GetScaledValueFromIntOrPercent(intstrutil.ValueOrDefault(maxUnavailable, intstrutil.FromInt(0)), int(desired), false)
216216
if err != nil {
217217
return 0, 0, err
218218
}

‎test/e2e/apps/deployment.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -755,7 +755,7 @@ func testProportionalScalingDeployment(f *framework.Framework) {
755755
framework.ExpectNoError(err)
756756

757757
// Checking state of first rollout's replicaset.
758-
maxUnavailable, err := intstr.GetValueFromIntOrPercent(deployment.Spec.Strategy.RollingUpdate.MaxUnavailable, int(*(deployment.Spec.Replicas)), false)
758+
maxUnavailable, err := intstr.GetScaledValueFromIntOrPercent(deployment.Spec.Strategy.RollingUpdate.MaxUnavailable, int(*(deployment.Spec.Replicas)), false)
759759
framework.ExpectNoError(err)
760760

761761
// First rollout's replicaset should have Deployment's (replicas - maxUnavailable) = 10 - 2 = 8 available replicas.
@@ -780,7 +780,7 @@ func testProportionalScalingDeployment(f *framework.Framework) {
780780
secondRS, err := deploymentutil.GetNewReplicaSet(deployment, c.AppsV1())
781781
framework.ExpectNoError(err)
782782

783-
maxSurge, err := intstr.GetValueFromIntOrPercent(deployment.Spec.Strategy.RollingUpdate.MaxSurge, int(*(deployment.Spec.Replicas)), false)
783+
maxSurge, err := intstr.GetScaledValueFromIntOrPercent(deployment.Spec.Strategy.RollingUpdate.MaxSurge, int(*(deployment.Spec.Replicas)), false)
784784
framework.ExpectNoError(err)
785785

786786
// Second rollout's replicaset should have 0 available replicas.

0 commit comments

Comments
 (0)