Merge pull request kubernetes#21872 from kargakis/maxUavailability-fix

kubectl: preserve availability when maxUnavailability is not 100%
332054781 · Feb 25, 2016 · 63feac0 · 63feac0
2 parents 3b10ce1 + 9cc1910
commit 63feac0
Show file tree

Hide file tree

Showing 2 changed files with 27 additions and 103 deletions.
diff --git a/pkg/kubectl/rolling_updater.go b/pkg/kubectl/rolling_updater.go
@@ -20,7 +20,6 @@ import (
 	goerrors "errors"
 	"fmt"
 	"io"
-	"math"
 	"strconv"
 	"strings"
 	"time"
@@ -198,12 +197,12 @@ func (r *RollingUpdater) Update(config *RollingUpdaterConfig) error {
 			oldRc.Name, originalReplicasAnnotation, oldRc.Annotations[originalReplicasAnnotation])
 	}
 	// The maximum pods which can go unavailable during the update.
-	maxUnavailable, err := extractMaxValue(config.MaxUnavailable, "maxUnavailable", desired)
+	maxUnavailable, err := intstr.GetValueFromIntOrPercent(&config.MaxUnavailable, desired, false)
 	if err != nil {
 		return err
 	}
 	// The maximum scaling increment.
-	maxSurge, err := extractMaxValue(config.MaxSurge, "maxSurge", desired)
+	maxSurge, err := intstr.GetValueFromIntOrPercent(&config.MaxSurge, desired, true)
 	if err != nil {
 		return err
 	}
@@ -486,29 +485,6 @@ func (r *RollingUpdater) cleanupWithClients(oldRc, newRc *api.ReplicationControl
 	}
 }
 
-// func extractMaxValue is a helper to extract config max values as either
-// absolute numbers or based on percentages of the given value.
-func extractMaxValue(field intstr.IntOrString, name string, value int) (int, error) {
-	switch field.Type {
-	case intstr.Int:
-		if field.IntVal < 0 {
-			return 0, fmt.Errorf("%s must be >= 0", name)
-		}
-		return field.IntValue(), nil
-	case intstr.String:
-		s := strings.Replace(field.StrVal, "%", "", -1)
-		v, err := strconv.Atoi(s)
-		if err != nil {
-			return 0, fmt.Errorf("invalid %s value %q: %v", name, field.StrVal, err)
-		}
-		if v < 0 {
-			return 0, fmt.Errorf("%s must be >= 0", name)
-		}
-		return int(math.Ceil(float64(value) * (float64(v)) / 100)), nil
-	}
-	return 0, fmt.Errorf("invalid kind %q for %s", field.Type, name)
-}
-
 func Rename(c client.ReplicationControllersNamespacer, rc *api.ReplicationController, newName string) error {
 	oldName := rc.Name
 	rc.Name = newName

diff --git a/pkg/kubectl/rolling_updater_test.go b/pkg/kubectl/rolling_updater_test.go
@@ -430,20 +430,21 @@ Scaling foo-v1 down to 0
 Scaling foo-v2 up to 10
 `,
 		}, {
-			name:        "1->1 10/0 fast readiness",
+			name:        "1->1 25/25 maintain minimum availability",
 			oldRc:       oldRc(1, 1),
 			newRc:       newRc(0, 1),
 			newRcExists: false,
-			maxUnavail:  intstr.FromString("10%"),
-			maxSurge:    intstr.FromString("0%"),
+			maxUnavail:  intstr.FromString("25%"),
+			maxSurge:    intstr.FromString("25%"),
 			expected: []interface{}{
-				down{oldReady: 1, newReady: 0, to: 0},
 				up{1},
+				down{oldReady: 1, newReady: 0, noop: true},
+				down{oldReady: 1, newReady: 1, to: 0},
 			},
 			output: `Created foo-v2
-Scaling up foo-v2 from 0 to 1, scaling down foo-v1 from 1 to 0 (keep 0 pods available, don't exceed 1 pods)
-Scaling foo-v1 down to 0
+Scaling up foo-v2 from 0 to 1, scaling down foo-v1 from 1 to 0 (keep 1 pods available, don't exceed 2 pods)
 Scaling foo-v2 up to 1
+Scaling foo-v1 down to 0
 `,
 		}, {
 			name:        "1->1 0/10 delayed readiness",
@@ -475,7 +476,7 @@ Scaling foo-v1 down to 0
 				down{oldReady: 1, newReady: 1, to: 0},
 			},
 			output: `Created foo-v2
-Scaling up foo-v2 from 0 to 1, scaling down foo-v1 from 1 to 0 (keep 0 pods available, don't exceed 2 pods)
+Scaling up foo-v2 from 0 to 1, scaling down foo-v1 from 1 to 0 (keep 1 pods available, don't exceed 2 pods)
 Scaling foo-v2 up to 1
 Scaling foo-v1 down to 0
 `,
@@ -655,6 +656,23 @@ Scaling foo-v1 down to 1
 Scaling foo-v2 up to 1
 Scaling foo-v1 down to 0
 Scaling foo-v2 up to 2
+`,
+		},
+		{
+			name:        "1->1 100%/0 allow maxUnavailability",
+			oldRc:       oldRc(1, 1),
+			newRc:       newRc(0, 1),
+			newRcExists: false,
+			maxUnavail:  intstr.FromString("100%"),
+			maxSurge:    intstr.FromInt(0),
+			expected: []interface{}{
+				down{oldReady: 1, newReady: 0, to: 0},
+				up{1},
+			},
+			output: `Created foo-v2
+Scaling up foo-v2 from 0 to 1, scaling down foo-v1 from 1 to 0 (keep 0 pods available, don't exceed 1 pods)
+Scaling foo-v1 down to 0
+Scaling foo-v2 up to 1
 `,
 		},
 	}
@@ -1571,73 +1589,3 @@ func TestRollingUpdater_readyPods(t *testing.T) {
 		}
 	}
 }
-
-func TestRollingUpdater_extractMaxValue(t *testing.T) {
-	tests := []struct {
-		field    intstr.IntOrString
-		original int
-		expected int
-		valid    bool
-	}{
-		{
-			field:    intstr.FromInt(1),
-			original: 100,
-			expected: 1,
-			valid:    true,
-		},
-		{
-			field:    intstr.FromInt(0),
-			original: 100,
-			expected: 0,
-			valid:    true,
-		},
-		{
-			field:    intstr.FromInt(-1),
-			original: 100,
-			valid:    false,
-		},
-		{
-			field:    intstr.FromString("10%"),
-			original: 100,
-			expected: 10,
-			valid:    true,
-		},
-		{
-			field:    intstr.FromString("100%"),
-			original: 100,
-			expected: 100,
-			valid:    true,
-		},
-		{
-			field:    intstr.FromString("200%"),
-			original: 100,
-			expected: 200,
-			valid:    true,
-		},
-		{
-			field:    intstr.FromString("0%"),
-			original: 100,
-			expected: 0,
-			valid:    true,
-		},
-		{
-			field:    intstr.FromString("-1%"),
-			original: 100,
-			valid:    false,
-		},
-	}
-
-	for i, test := range tests {
-		t.Logf("evaluating test %d", i)
-		max, err := extractMaxValue(test.field, "field", test.original)
-		if test.valid && err != nil {
-			t.Fatalf("unexpected error: %v", err)
-		}
-		if !test.valid && err == nil {
-			t.Fatalf("expected an error")
-		}
-		if e, a := test.expected, max; e != a {
-			t.Fatalf("expected max %d, got %d", e, a)
-		}
-	}
-}