From bcf43538a2614e40744306683da615a45df98480 Mon Sep 17 00:00:00 2001 From: Seth Hoenig Date: Tue, 27 Sep 2022 10:22:03 -0500 Subject: [PATCH] core: numeric operands comparisons in constraints This PR changes constraint comparisons to be numeric rather than lexical if both operands are integers or floats. Inspiration #4856 Closes #4729 Closes #14719 --- .changelog/14722.txt | 3 + scheduler/feasible.go | 92 ++++++++++++++----- scheduler/feasible_test.go | 51 +++++++--- .../docs/job-specification/constraint.mdx | 5 +- .../content/docs/upgrade/upgrade-specific.mdx | 6 ++ 5 files changed, 115 insertions(+), 42 deletions(-) create mode 100644 .changelog/14722.txt diff --git a/.changelog/14722.txt b/.changelog/14722.txt new file mode 100644 index 000000000000..5786c9feefa3 --- /dev/null +++ b/.changelog/14722.txt @@ -0,0 +1,3 @@ +```release-note:improvement +core: constraint operands are now compared numerically if operands are numbers +``` diff --git a/scheduler/feasible.go b/scheduler/feasible.go index 157d6702f638..adf5b3cd4693 100644 --- a/scheduler/feasible.go +++ b/scheduler/feasible.go @@ -12,6 +12,7 @@ import ( "github.com/hashicorp/nomad/helper/constraints/semver" "github.com/hashicorp/nomad/nomad/structs" psstructs "github.com/hashicorp/nomad/plugins/shared/structs" + "golang.org/x/exp/constraints" ) const ( @@ -818,7 +819,10 @@ func checkConstraint(ctx Context, operand string, lVal, rVal interface{}, lFound case "!=", "not": return !reflect.DeepEqual(lVal, rVal) case "<", "<=", ">", ">=": - return lFound && rFound && checkLexicalOrder(operand, lVal, rVal) + if !lFound || !rFound { + return false + } + return checkOrder(operand, lVal, rVal) case structs.ConstraintAttributeIsSet: return lFound case structs.ConstraintAttributeIsNotSet: @@ -850,27 +854,65 @@ func checkAttributeAffinity(ctx Context, operand string, lVal, rVal *psstructs.A return checkAttributeConstraint(ctx, operand, lVal, rVal, lFound, rFound) } -// checkLexicalOrder is used to check for lexical ordering -func checkLexicalOrder(op string, lVal, rVal interface{}) bool { - // Ensure the values are strings - lStr, ok := lVal.(string) - if !ok { +// checkOrder returns the result of (lVal operand rVal). The comparison is +// done as integers if possible, or floats if possible, and lexically otherwise. +func checkOrder(operand string, lVal, rVal any) bool { + left, leftOK := lVal.(string) + right, rightOK := rVal.(string) + if !leftOK || !rightOK { return false } - rStr, ok := rVal.(string) - if !ok { - return false + if result, ok := checkIntegralOrder(operand, left, right); ok { + return result + } + if result, ok := checkFloatOrder(operand, left, right); ok { + return result } + return checkLexicalOrder(operand, left, right) +} + +// checkIntegralOrder compares lVal and rVal as integers if possible, or false otherwise. +func checkIntegralOrder(op, lVal, rVal string) (bool, bool) { + left, lErr := strconv.Atoi(lVal) + if lErr != nil { + return false, false + } + right, rErr := strconv.Atoi(rVal) + if rErr != nil { + return false, false + } + return compareOrder(op, left, right), true +} + +// checkFloatOrder compares lVal and rVal as floats if possible, or false otherwise. +func checkFloatOrder(op, lVal, rVal string) (bool, bool) { + left, lErr := strconv.ParseFloat(lVal, 64) + if lErr != nil { + return false, false + } + right, rErr := strconv.ParseFloat(rVal, 64) + if rErr != nil { + return false, false + } + return compareOrder(op, left, right), true +} + +// checkLexicalOrder compares lVal and rVal lexically. +func checkLexicalOrder(op string, lVal, rVal string) bool { + return compareOrder[string](op, lVal, rVal) +} +// compareOrder returns the result of the expression (left op right) +func compareOrder[T constraints.Ordered](op string, left, right T) bool { switch op { case "<": - return lStr < rStr + return left < right case "<=": - return lStr <= rStr + return left <= right case ">": - return lStr > rStr + return left > right case ">=": - return lStr >= rStr + return left >= right default: return false } @@ -903,13 +945,13 @@ func checkVersionMatch(_ Context, parse verConstraintParser, lVal, rVal interfac } // Parse the constraints - constraints := parse(constraintStr) - if constraints == nil { + c := parse(constraintStr) + if c == nil { return false } // Check the constraints against the version - return constraints.Check(vers) + return c.Check(vers) } // checkAttributeVersionMatch is used to compare a version on the @@ -938,13 +980,13 @@ func checkAttributeVersionMatch(_ Context, parse verConstraintParser, lVal, rVal } // Parse the constraints - constraints := parse(constraintStr) - if constraints == nil { + c := parse(constraintStr) + if c == nil { return false } // Check the constraints against the version - return constraints.Check(vers) + return c.Check(vers) } // checkRegexpMatch is used to compare a value on the @@ -1485,13 +1527,13 @@ func newVersionConstraintParser(ctx Context) verConstraintParser { return c } - constraints, err := version.NewConstraint(cstr) + constraint, err := version.NewConstraint(cstr) if err != nil { return nil } - cache[cstr] = constraints + cache[cstr] = constraint - return constraints + return constraint } } @@ -1503,12 +1545,12 @@ func newSemverConstraintParser(ctx Context) verConstraintParser { return c } - constraints, err := semver.NewConstraint(cstr) + constraint, err := semver.NewConstraint(cstr) if err != nil { return nil } - cache[cstr] = constraints + cache[cstr] = constraint - return constraints + return constraint } } diff --git a/scheduler/feasible_test.go b/scheduler/feasible_test.go index 2a5d804a2104..d93ab37a5b06 100644 --- a/scheduler/feasible_test.go +++ b/scheduler/feasible_test.go @@ -11,6 +11,7 @@ import ( "github.com/hashicorp/nomad/nomad/mock" "github.com/hashicorp/nomad/nomad/structs" psstructs "github.com/hashicorp/nomad/plugins/shared/structs" + "github.com/shoenig/test/must" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -1128,45 +1129,65 @@ func TestCheckConstraint(t *testing.T) { } } -func TestCheckLexicalOrder(t *testing.T) { +func TestCheckOrder(t *testing.T) { ci.Parallel(t) - type tcase struct { + cases := []struct { op string - lVal, rVal interface{} - result bool - } - cases := []tcase{ + lVal, rVal any + exp bool + }{ { op: "<", lVal: "bar", rVal: "foo", - result: true, + exp: true, }, { op: "<=", lVal: "foo", rVal: "foo", - result: true, + exp: true, }, { op: ">", lVal: "bar", rVal: "foo", - result: false, + exp: false, }, { op: ">=", lVal: "bar", rVal: "bar", - result: true, + exp: true, }, { op: ">", - lVal: 1, rVal: "foo", - result: false, + lVal: "1", rVal: "foo", + exp: false, + }, + { + op: "<", + lVal: "10", rVal: "1", + exp: false, + }, + { + op: "<", + lVal: "1", rVal: "10", + exp: true, + }, + { + op: "<", + lVal: "10.5", rVal: "1.5", + exp: false, + }, + { + op: "<", + lVal: "1.5", rVal: "10.5", + exp: true, }, } for _, tc := range cases { - if res := checkLexicalOrder(tc.op, tc.lVal, tc.rVal); res != tc.result { - t.Fatalf("TC: %#v, Result: %v", tc, res) - } + name := fmt.Sprintf("%v %s %v", tc.lVal, tc.op, tc.rVal) + t.Run(name, func(t *testing.T) { + must.Eq(t, tc.exp, checkOrder(tc.op, tc.lVal, tc.rVal)) + }) } } diff --git a/website/content/docs/job-specification/constraint.mdx b/website/content/docs/job-specification/constraint.mdx index 5ddc7d680565..b959c3fee513 100644 --- a/website/content/docs/job-specification/constraint.mdx +++ b/website/content/docs/job-specification/constraint.mdx @@ -64,8 +64,9 @@ all groups (and tasks) in the job. to examine for the constraint. This can be any of the [Nomad interpolated values](/docs/runtime/interpolation#interpreted_node_vars). -- `operator` `(string: "=")` - Specifies the comparison operator. The ordering is - compared lexically. Possible values include: +- `operator` `(string: "=")` - Specifies the comparison operator. If the operator + is one of `>, >=, <, <=`, the ordering is compared numerically if the operands + are both integers or both floats, and lexically otherwise. Possible values include: ```text = diff --git a/website/content/docs/upgrade/upgrade-specific.mdx b/website/content/docs/upgrade/upgrade-specific.mdx index 9a98d2d20fb6..5215318e04b9 100644 --- a/website/content/docs/upgrade/upgrade-specific.mdx +++ b/website/content/docs/upgrade/upgrade-specific.mdx @@ -64,6 +64,12 @@ Nomad clients no longer have their Consul and Vault fingerprints cleared when connectivity is lost with Consul and Vault. To intentionally remove Consul and Vault from a client node, you will need to restart the Nomad client agent. +#### Numeric Operand Comparisons in Constraints + +Prior to Nomad 1.4.0 the `<, <=, >, >=` operators in a constraint would always +compare the operands lexically. This behavior has been changed so that the comparison +is done numerically if both operands are integers or floats. + ## Nomad 1.3.3 Environments that don't support the use of [`uid`][template_uid] and