From 531393d7937b727f2240aac38989fe3b64b3675f Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Wed, 18 Mar 2020 10:04:43 +0000 Subject: [PATCH 1/4] Test MaxUnhealthy IntInString case --- .../machinehealthcheck_controller_test.go | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/pkg/controller/machinehealthcheck/machinehealthcheck_controller_test.go b/pkg/controller/machinehealthcheck/machinehealthcheck_controller_test.go index 4204066b44..907730fc2b 100644 --- a/pkg/controller/machinehealthcheck/machinehealthcheck_controller_test.go +++ b/pkg/controller/machinehealthcheck/machinehealthcheck_controller_test.go @@ -2411,6 +2411,7 @@ func TestIsAllowedRemediation(t *testing.T) { // short circuit if ever more than 2 out of 5 go unhealthy maxUnhealthyInt := intstr.FromInt(2) maxUnhealthyString := intstr.FromString("40%") + maxUnhealthyIntInString := intstr.FromString("2") testCases := []struct { testCase string @@ -2501,6 +2502,48 @@ func TestIsAllowedRemediation(t *testing.T) { }, expected: false, }, + { + testCase: "not above maxUnhealthy (int in string)", + mhc: &mapiv1beta1.MachineHealthCheck{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: namespace, + }, + TypeMeta: metav1.TypeMeta{ + Kind: "MachineHealthCheck", + }, + Spec: mapiv1beta1.MachineHealthCheckSpec{ + Selector: metav1.LabelSelector{}, + MaxUnhealthy: &maxUnhealthyIntInString, + }, + Status: mapiv1beta1.MachineHealthCheckStatus{ + ExpectedMachines: IntPtr(5), + CurrentHealthy: IntPtr(3), + }, + }, + expected: true, + }, + { + testCase: "above maxUnhealthy (int in string)", + mhc: &mapiv1beta1.MachineHealthCheck{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: namespace, + }, + TypeMeta: metav1.TypeMeta{ + Kind: "MachineHealthCheck", + }, + Spec: mapiv1beta1.MachineHealthCheckSpec{ + Selector: metav1.LabelSelector{}, + MaxUnhealthy: &maxUnhealthyIntInString, + }, + Status: mapiv1beta1.MachineHealthCheckStatus{ + ExpectedMachines: IntPtr(5), + CurrentHealthy: IntPtr(2), + }, + }, + expected: false, + }, { testCase: "nil values", mhc: &mapiv1beta1.MachineHealthCheck{ From c9609837fe5796c39849cf6c93ffbd67a8ced48d Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Wed, 18 Mar 2020 10:05:19 +0000 Subject: [PATCH 2/4] Allow Int values in String for MaxUnhealthy --- .../machinehealthcheck_controller.go | 56 ++++++++++++++++++- 1 file changed, 55 insertions(+), 1 deletion(-) diff --git a/pkg/controller/machinehealthcheck/machinehealthcheck_controller.go b/pkg/controller/machinehealthcheck/machinehealthcheck_controller.go index d4e06c6f00..f2eaa527eb 100644 --- a/pkg/controller/machinehealthcheck/machinehealthcheck_controller.go +++ b/pkg/controller/machinehealthcheck/machinehealthcheck_controller.go @@ -2,7 +2,11 @@ package machinehealthcheck import ( "context" + "errors" "fmt" + "math" + "strconv" + "strings" "time" "k8s.io/apimachinery/pkg/util/intstr" @@ -228,7 +232,7 @@ func isAllowedRemediation(mhc *mapiv1.MachineHealthCheck) bool { if mhc.Spec.MaxUnhealthy == nil { return true } - maxUnhealthy, err := intstr.GetValueFromIntOrPercent(mhc.Spec.MaxUnhealthy, derefInt(mhc.Status.ExpectedMachines), false) + maxUnhealthy, err := getValueFromIntOrPercent(mhc.Spec.MaxUnhealthy, derefInt(mhc.Status.ExpectedMachines), false) if err != nil { glog.Errorf("%s: error decoding maxUnhealthy, remediation won't be allowed: %v", namespacedName(mhc), err) return false @@ -673,3 +677,53 @@ func hasMatchingLabels(machineHealthCheck *mapiv1.MachineHealthCheck, machine *m } return true } + +// getValueFromIntOrPercent returns the integer number value based on the +// percentage of the total or absolute number dependent on the IntOrString given +// +// The following code is copied from https://github.com/kubernetes/apimachinery/blob/1a505bc60c6dfb15cb18a8cdbfa01db042156fe2/pkg/util/intstr/intstr.go#L154-L185 +// But fixed so that string values aren't always assumed to be percentages +// See https://github.com/kubernetes/kubernetes/issues/89082 for details +func getValueFromIntOrPercent(intOrPercent *intstr.IntOrString, total int, roundUp bool) (int, error) { + if intOrPercent == nil { + return 0, errors.New("nil value for IntOrString") + } + value, isPercent, err := getIntOrPercentValue(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 +} + +// getIntOrPercentValue returns the integer value of the IntOrString and +// determines if this value is a percentage or absolute number +// +// The following code is copied from https://github.com/kubernetes/apimachinery/blob/1a505bc60c6dfb15cb18a8cdbfa01db042156fe2/pkg/util/intstr/intstr.go#L154-L185 +// But fixed so that string values aren't always assumed to be percentages +// See https://github.com/kubernetes/kubernetes/issues/89082 for details +func getIntOrPercentValue(intOrStr *intstr.IntOrString) (int, bool, error) { + switch intOrStr.Type { + case intstr.Int: + return intOrStr.IntValue(), false, nil + case intstr.String: + isPercent := false + s := intOrStr.StrVal + if strings.Contains(s, "%") { + isPercent = true + s = strings.Replace(intOrStr.StrVal, "%", "", -1) + } + v, err := strconv.Atoi(s) + if err != nil { + return 0, isPercent, 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") +} From 399bee172688007eefbd6565f2d0eeb22cbb96d7 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Thu, 19 Mar 2020 15:06:18 +0000 Subject: [PATCH 3/4] Add test case for invalid string value for isAllowedRemediation --- .../machinehealthcheck_controller_test.go | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/pkg/controller/machinehealthcheck/machinehealthcheck_controller_test.go b/pkg/controller/machinehealthcheck/machinehealthcheck_controller_test.go index 907730fc2b..47dd46f075 100644 --- a/pkg/controller/machinehealthcheck/machinehealthcheck_controller_test.go +++ b/pkg/controller/machinehealthcheck/machinehealthcheck_controller_test.go @@ -2412,6 +2412,7 @@ func TestIsAllowedRemediation(t *testing.T) { maxUnhealthyInt := intstr.FromInt(2) maxUnhealthyString := intstr.FromString("40%") maxUnhealthyIntInString := intstr.FromString("2") + maxUnhealthyMixedString := intstr.FromString("foo%50") testCases := []struct { testCase string @@ -2565,6 +2566,27 @@ func TestIsAllowedRemediation(t *testing.T) { }, expected: true, }, + { + testCase: "invalid string value", + mhc: &mapiv1beta1.MachineHealthCheck{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: namespace, + }, + TypeMeta: metav1.TypeMeta{ + Kind: "MachineHealthCheck", + }, + Spec: mapiv1beta1.MachineHealthCheckSpec{ + Selector: metav1.LabelSelector{}, + MaxUnhealthy: &maxUnhealthyMixedString, + }, + Status: mapiv1beta1.MachineHealthCheckStatus{ + ExpectedMachines: nil, + CurrentHealthy: nil, + }, + }, + expected: false, + }, } for _, tc := range testCases { From 7cd6de65708e2db8bc27e0a962198d922be54f46 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Fri, 20 Mar 2020 15:30:29 +0000 Subject: [PATCH 4/4] Add tests for getIntOrPercentValue --- .../machinehealthcheck_controller_test.go | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/pkg/controller/machinehealthcheck/machinehealthcheck_controller_test.go b/pkg/controller/machinehealthcheck/machinehealthcheck_controller_test.go index 47dd46f075..96c2dec30b 100644 --- a/pkg/controller/machinehealthcheck/machinehealthcheck_controller_test.go +++ b/pkg/controller/machinehealthcheck/machinehealthcheck_controller_test.go @@ -2598,6 +2598,74 @@ func TestIsAllowedRemediation(t *testing.T) { } } +func TestGetIntOrPercentValue(t *testing.T) { + int10 := intstr.FromInt(10) + percent20 := intstr.FromString("20%") + intInString30 := intstr.FromString("30") + invalidStringA := intstr.FromString("a") + invalidStringAPercent := intstr.FromString("a%") + + testCases := []struct { + name string + in *intstr.IntOrString + expectedValue int + expectedPercent bool + expectedError error + }{ + { + name: "with a integer", + in: &int10, + expectedValue: 10, + expectedPercent: false, + expectedError: nil, + }, + { + name: "with a percentage", + in: &percent20, + expectedValue: 20, + expectedPercent: true, + expectedError: nil, + }, + { + name: "with an int in string", + in: &intInString30, + expectedValue: 30, + expectedPercent: false, + expectedError: nil, + }, + { + name: "with an 'a' string", + in: &invalidStringA, + expectedValue: 0, + expectedPercent: false, + expectedError: fmt.Errorf("invalid value \"a\": strconv.Atoi: parsing \"a\": invalid syntax"), + }, + { + name: "with an 'a%' string", + in: &invalidStringAPercent, + expectedValue: 0, + expectedPercent: true, + expectedError: fmt.Errorf("invalid value \"a%%\": strconv.Atoi: parsing \"a\": invalid syntax"), + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + value, percent, err := getIntOrPercentValue(tc.in) + // Check first if one is nil, and the other isn't, otherwise if not nil, do the messages match + if (tc.expectedError != nil) != (err != nil) || err != nil && tc.expectedError.Error() != err.Error() { + t.Errorf("Case: %s. Got: %v, expected: %v", tc.name, err, tc.expectedError) + } + if tc.expectedPercent != percent { + t.Errorf("Case: %s. Got: %v, expected: %v", tc.name, percent, tc.expectedPercent) + } + if tc.expectedValue != value { + t.Errorf("Case: %s. Got: %v, expected: %v", tc.name, value, tc.expectedValue) + } + }) + } +} + func IntPtr(i int) *int { return &i }