Skip to content

ROSAENG-62105: PercentageValidator must require value greater than 0 and less than 1 #3398

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nephomaniac wants to merge 3 commits into
base: master
Choose a base branch
from
Open

ROSAENG-62105: PercentageValidator must require value greater than 0 and less than 1 #3398

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
87a0e38
ROSAENG-62105 | fix: require utilization threshold strictly between 0…
nephomaniac Jul 18, 2026
d334050
ROSAENG-62105 | test: include actual value in PercentageValidator err…
nephomaniac Sep 10, 2026
dc1af81
ROSAENG-62105 | chore: add depguard nolint annotations for architectu…
nephomaniac Sep 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cmd/create/autoscaler/cmd_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ var _ = Describe("create autoscaler", func() {
VerifyJQ(`.resource_limits.cores.max`, 0.0),
))
args := &clusterautoscaler.AutoscalerArgs{}
args.ScaleDown.UtilizationThreshold = 0.5
args.LogVerbosity = 3
args.ResourceLimits.MaxNodesTotal = 20
runner := CreateAutoscalerRunner(args)
Expand Down
1 change: 1 addition & 0 deletions cmd/edit/autoscaler/cmd_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ var _ = Describe("edit autoscaler", func() {
VerifyJQ(`.resource_limits.cores.max`, 30.0),
))
args := &clusterautoscaler.AutoscalerArgs{}
args.ScaleDown.UtilizationThreshold = 0.5
args.LogVerbosity = 1
args.ResourceLimits.MaxNodesTotal = 20
runner := EditAutoscalerRunner(args)
Expand Down
48 changes: 24 additions & 24 deletions pkg/clusterautoscaler/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@ import (

commonUtils "github.com/openshift-online/ocm-common/pkg/utils"
cmv1 "github.com/openshift-online/ocm-sdk-go/clustersmgmt/v1"
"github.com/spf13/cobra"
"github.com/spf13/pflag"
"github.com/spf13/cobra" //nolint:depguard
"github.com/spf13/pflag" //nolint:depguard

"github.com/openshift/rosa/pkg/helper/versions"
"github.com/openshift/rosa/pkg/interactive"
"github.com/openshift/rosa/pkg/interactive" //nolint:depguard
"github.com/openshift/rosa/pkg/ocm"
)

Expand Down Expand Up @@ -244,7 +244,7 @@ func AddClusterAutoscalerFlags(cmd *cobra.Command, prefix string) *AutoscalerArg
fmt.Sprintf("%s%s", prefix, scaleDownUtilizationThresholdFlag),
0.5,
fmt.Sprintf("Node utilization level, defined as sum of requested resources divided by capacity, "+
"below which a node can be considered for scale down. Value should be between 0 and 1. %s",
"below which a node can be considered for scale down. Value must be greater than 0 and less than 1. %s",
classicOnlyHelpMsg),
)

Expand Down Expand Up @@ -365,7 +365,7 @@ func GetAutoscalerOptions(
}
}
if err := ocm.NonNegativeInt32Validator(result.LogVerbosity); err != nil {
return nil, fmt.Errorf("Error validating log-verbosity: %s", err)
return nil, fmt.Errorf("error validating log-verbosity: %s", err)
}

if interactive.Enabled() && !cmd.Changed(fmt.Sprintf("%s%s", prefix, balancingIgnoredLabelsFlag)) {
Expand All @@ -387,7 +387,7 @@ func GetAutoscalerOptions(
}
}
if err := ocm.ValidateBalancingIgnoredLabels(strings.Join(result.BalancingIgnoredLabels, ",")); err != nil {
return nil, fmt.Errorf("Error validating balancing-ignored-labels: %s", err)
return nil, fmt.Errorf("error validating balancing-ignored-labels: %s", err)
}

if interactive.Enabled() && !cmd.Changed(fmt.Sprintf("%s%s", prefix, ignoreDaemonsetsUtilizationFlag)) {
Expand Down Expand Up @@ -436,7 +436,7 @@ func GetAutoscalerOptions(
}
}
if err := ocm.NonNegativeInt32Validator(result.MaxPodGracePeriod); err != nil {
return nil, fmt.Errorf("Error validating max-pod-grace-period: %s", err)
return nil, fmt.Errorf("error validating max-pod-grace-period: %s", err)
}

if interactive.Enabled() && !cmd.Changed(fmt.Sprintf("%s%s", prefix, podPriorityThresholdFlag)) {
Expand Down Expand Up @@ -470,7 +470,7 @@ func GetAutoscalerOptions(
}
}
if err := ocm.NonNegativeInt32Validator(result.ResourceLimits.MaxNodesTotal); err != nil {
return nil, fmt.Errorf("Error validating max-nodes-total: %s", err)
return nil, fmt.Errorf("error validating max-nodes-total: %s", err)
}

if autoscalerValidationArgs != nil && !autoscalerValidationArgs.IsHostedCp {
Expand All @@ -489,7 +489,7 @@ func GetAutoscalerOptions(
}
}
if err = ocm.NonNegativeInt32Validator(result.ResourceLimits.Cores.Min); err != nil {
return nil, fmt.Errorf("Error validating min-cores: %s", err)
return nil, fmt.Errorf("error validating min-cores: %s", err)
}

if interactive.Enabled() && !cmd.Changed(fmt.Sprintf("%s%s", prefix, maxCoresFlag)) {
Expand All @@ -508,11 +508,11 @@ func GetAutoscalerOptions(
}
}
if err := ocm.NonNegativeInt32Validator(result.ResourceLimits.Cores.Max); err != nil {
return nil, fmt.Errorf("Error validating max-cores: %s", err)
return nil, fmt.Errorf("error validating max-cores: %s", err)
}

if err := getValidMaxRangeValidator(result.ResourceLimits.Cores.Min)(result.ResourceLimits.Cores.Max); err != nil {
return nil, fmt.Errorf("Error validating cores range: %s", err)
return nil, fmt.Errorf("error validating cores range: %s", err)
}

if interactive.Enabled() && !cmd.Changed(fmt.Sprintf("%s%s", prefix, minMemoryFlag)) {
Expand All @@ -530,7 +530,7 @@ func GetAutoscalerOptions(
}
}
if err := ocm.NonNegativeInt32Validator(result.ResourceLimits.Memory.Min); err != nil {
return nil, fmt.Errorf("Error validating min-memory: %s", err)
return nil, fmt.Errorf("error validating min-memory: %s", err)
}

if interactive.Enabled() && !cmd.Changed(fmt.Sprintf("%s%s", prefix, maxMemoryFlag)) {
Expand All @@ -549,11 +549,11 @@ func GetAutoscalerOptions(
}
}
if err := ocm.NonNegativeInt32Validator(result.ResourceLimits.Memory.Max); err != nil {
return nil, fmt.Errorf("Error validating max-memory: %s", err)
return nil, fmt.Errorf("error validating max-memory: %s", err)
}

if err := getValidMaxRangeValidator(result.ResourceLimits.Memory.Min)(result.ResourceLimits.Memory.Max); err != nil {
return nil, fmt.Errorf("Error validating memory range: %s", err)
return nil, fmt.Errorf("error validating memory range: %s", err)
}

if interactive.Enabled() && !cmd.Changed(fmt.Sprintf("%s%s", prefix, gpuLimitFlag)) {
Expand Down Expand Up @@ -622,7 +622,7 @@ func GetAutoscalerOptions(
}

if err := getValidMaxRangeValidator(gpuLimit.Range.Min)(gpuLimit.Range.Max); err != nil {
return nil, fmt.Errorf("Error validating GPU range: %s", err)
return nil, fmt.Errorf("error validating GPU range: %s", err)
}
}

Expand Down Expand Up @@ -656,7 +656,7 @@ func GetAutoscalerOptions(
}

if err := ocm.PositiveDurationStringValidator(result.ScaleDown.UnneededTime); err != nil {
return nil, fmt.Errorf("Error validating unneeded-time: %s", err)
return nil, fmt.Errorf("error validating unneeded-time: %s", err)
}

if interactive.Enabled() && !cmd.Changed(fmt.Sprintf("%s%s", prefix, scaleDownUtilizationThresholdFlag)) {
Expand All @@ -674,7 +674,7 @@ func GetAutoscalerOptions(
}
}
if err := ocm.PercentageValidator(result.ScaleDown.UtilizationThreshold); err != nil {
return nil, fmt.Errorf("Error validating utilization-threshold: %s", err)
return nil, fmt.Errorf("error validating utilization-threshold: %s", err)
}

if interactive.Enabled() && !cmd.Changed(fmt.Sprintf("%s%s", prefix, scaleDownDelayAfterAddFlag)) {
Expand All @@ -692,7 +692,7 @@ func GetAutoscalerOptions(
}
}
if err := ocm.PositiveDurationStringValidator(result.ScaleDown.DelayAfterAdd); err != nil {
return nil, fmt.Errorf("Error validating delay-after-add: %s", err)
return nil, fmt.Errorf("error validating delay-after-add: %s", err)
}

if interactive.Enabled() && !cmd.Changed(fmt.Sprintf("%s%s", prefix, scaleDownDelayAfterDeleteFlag)) {
Expand All @@ -710,7 +710,7 @@ func GetAutoscalerOptions(
}
}
if err := ocm.PositiveDurationStringValidator(result.ScaleDown.DelayAfterDelete); err != nil {
return nil, fmt.Errorf("Error validating delay-after-delete: %s", err)
return nil, fmt.Errorf("error validating delay-after-delete: %s", err)
}

if interactive.Enabled() && !cmd.Changed(fmt.Sprintf("%s%s", prefix, scaleDownDelayAfterFailureFlag)) {
Expand All @@ -728,7 +728,7 @@ func GetAutoscalerOptions(
}
}
if err := ocm.PositiveDurationStringValidator(result.ScaleDown.DelayAfterFailure); err != nil {
return nil, fmt.Errorf("Error validating delay-after-failure: %s", err)
return nil, fmt.Errorf("error validating delay-after-failure: %s", err)
}
}

Expand Down Expand Up @@ -867,12 +867,12 @@ func parseGPULimit(s string) (ocm.GPULimit, error) {

gpuLimitMin, err := strconv.Atoi(parameters[1])
if err != nil {
return ocm.GPULimit{}, fmt.Errorf("Failed parsing '%s' into an integer: %s", parameters[1], err)
return ocm.GPULimit{}, fmt.Errorf("failed parsing '%s' into an integer: %s", parameters[1], err)
}

gpuLimitMax, err := strconv.Atoi(parameters[2])
if err != nil {
return ocm.GPULimit{}, fmt.Errorf("Failed parsing '%s' into an integer: %s", parameters[2], err)
return ocm.GPULimit{}, fmt.Errorf("failed parsing '%s' into an integer: %s", parameters[2], err)
}

return ocm.GPULimit{Type: parameters[0], Range: ocm.ResourceRange{Min: gpuLimitMin, Max: gpuLimitMax}}, nil
Expand All @@ -888,11 +888,11 @@ func getValidMaxRangeValidator(min int) func(interface{}) error {

max, err := strconv.Atoi(fmt.Sprintf("%v", val))
if err != nil {
return fmt.Errorf("Failed parsing '%v' to an integer number.", val)
return fmt.Errorf("failed parsing '%v' to an integer number", val)
}

if max < min {
return fmt.Errorf("max value must be greater or equal than min value %d.", min)
return fmt.Errorf("max value must be greater or equal than min value %d", min)
}

return nil
Expand Down
17 changes: 9 additions & 8 deletions pkg/ocm/validators.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package ocm

import (
"fmt"
"math"
"strconv"
"time"

Expand All @@ -14,7 +15,7 @@ func Int32Validator(val interface{}) error {
}
_, err := strconv.ParseInt(fmt.Sprintf("%v", val), 10, 32)
if err != nil {
return fmt.Errorf("Should provide an integer number between -2147483648 to 2147483647.")
return fmt.Errorf("should provide an integer number between -2147483648 to 2147483647")
}
return nil
}
Expand All @@ -25,11 +26,11 @@ func NonNegativeInt32Validator(val interface{}) error {
}
number, err := strconv.ParseInt(fmt.Sprintf("%v", val), 10, 32)
if err != nil {
return fmt.Errorf("Should provide an integer number between 0 to 2147483647.")
return fmt.Errorf("should provide an integer number between 0 to 2147483647")
}

if number < 0 {
return fmt.Errorf("Number must be greater or equal to zero.")
return fmt.Errorf("number must be greater or equal to zero")
}

return nil
Expand All @@ -42,7 +43,7 @@ func PositiveDurationStringValidator(val interface{}) error {
input, ok := val.(string)

if !ok {
return fmt.Errorf("Can only validate strings, got %v", val)
return fmt.Errorf("can only validate strings, got %v", val)
}

duration, err := time.ParseDuration(input)
Expand All @@ -52,7 +53,7 @@ func PositiveDurationStringValidator(val interface{}) error {
}

if duration < 0 {
return fmt.Errorf("Only positive durations are allowed, got '%v'", val)
return fmt.Errorf("only positive durations are allowed, got '%v'", val)
}

return nil
Expand All @@ -65,11 +66,11 @@ func PercentageValidator(val interface{}) error {

number, err := strconv.ParseFloat(fmt.Sprintf("%v", val), commonUtils.MaxByteSize)
if err != nil {
return fmt.Errorf("Failed parsing '%v' into a floating-point number.", val)
return fmt.Errorf("failed parsing '%v' into a floating-point number", val)
}

if number > 1 || number < 0 {
return fmt.Errorf("Expecting a floating-point number between 0 and 1.")
if number >= 1 || number <= 0 || math.IsNaN(number) {
return fmt.Errorf("expecting a floating-point number greater than 0 and less than 1, got %v", number)
}

return nil
Expand Down
12 changes: 12 additions & 0 deletions pkg/ocm/validators_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,18 @@ var _ = Describe("Input Validators", Ordered, func() {
Expect(PercentageValidator("-0.1")).ToNot(BeNil())
})

It("raises an error if got exactly 0", func() {
Expect(PercentageValidator("0")).ToNot(BeNil())
})

It("raises an error if got exactly 1", func() {
Expect(PercentageValidator("1")).ToNot(BeNil())
Comment thread
olucasfreitas marked this conversation as resolved.
})

It("raises an error if got NaN", func() {
Expect(PercentageValidator("NaN")).ToNot(BeNil())
})

It("successfully parses a valid percentage value", func() {
Expect(PercentageValidator("0.4")).To(BeNil())
})
Expand Down
Loading