From 5df5e705426dd0a2e09da6c1a4d7c90a863b3360 Mon Sep 17 00:00:00 2001 From: Seth Hoenig Date: Tue, 27 Sep 2022 11:07:07 -0500 Subject: [PATCH] core: numeric operands comparisons in constraints (#14722) * cleanup: fixup linter warnings in schedular/feasible.go * 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 * fix: always parse as int64 --- .changelog/14722.txt | 3 + scheduler/feasible.go | 101 ++++++++++++------ scheduler/feasible_test.go | 51 ++++++--- .../docs/job-specification/constraint.mdx | 5 +- .../content/docs/upgrade/upgrade-specific.mdx | 6 ++ 5 files changed, 118 insertions(+), 48 deletions(-) create mode 100644 .changelog/14722.txt diff --git a/.changelog/14722.txt b/.changelog/14722.txt new file mode 100644 index 000000000..5786c9fee --- /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 152aea7f6..8da3e6869 100644 --- a/scheduler/feasible.go +++ b/scheduler/feasible.go @@ -7,11 +7,12 @@ import ( "strconv" "strings" - memdb "github.com/hashicorp/go-memdb" - version "github.com/hashicorp/go-version" + "github.com/hashicorp/go-memdb" + "github.com/hashicorp/go-version" "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 ( @@ -57,7 +58,7 @@ type FeasibleIterator interface { Reset() } -// JobContextualIterator is an iterator that can have the job and task group set +// ContextualIterator is an iterator that can have the job and task group set // on it. type ContextualIterator interface { SetJob(*structs.Job) @@ -818,7 +819,7 @@ 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) + return lFound && rFound && checkOrder(operand, lVal, rVal) case structs.ConstraintAttributeIsSet: return lFound case structs.ConstraintAttributeIsNotSet: @@ -850,27 +851,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.ParseInt(lVal, 10, 64) + if lErr != nil { + return false, false + } + right, rErr := strconv.ParseInt(rVal, 10, 64) + 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 } @@ -878,7 +917,7 @@ func checkLexicalOrder(op string, lVal, rVal interface{}) bool { // checkVersionMatch is used to compare a version on the // left hand side with a set of constraints on the right hand side -func checkVersionMatch(ctx Context, parse verConstraintParser, lVal, rVal interface{}) bool { +func checkVersionMatch(_ Context, parse verConstraintParser, lVal, rVal interface{}) bool { // Parse the version var versionStr string switch v := lVal.(type) { @@ -903,18 +942,18 @@ func checkVersionMatch(ctx Context, parse verConstraintParser, lVal, rVal interf } // 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 // left hand side with a set of constraints on the right hand side -func checkAttributeVersionMatch(ctx Context, parse verConstraintParser, lVal, rVal *psstructs.Attribute) bool { +func checkAttributeVersionMatch(_ Context, parse verConstraintParser, lVal, rVal *psstructs.Attribute) bool { // Parse the version var versionStr string if s, ok := lVal.GetString(); ok { @@ -938,13 +977,13 @@ func checkAttributeVersionMatch(ctx Context, parse verConstraintParser, lVal, rV } // 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 @@ -982,7 +1021,7 @@ func checkRegexpMatch(ctx Context, lVal, rVal interface{}) bool { // checkSetContainsAll is used to see if the left hand side contains the // string on the right hand side -func checkSetContainsAll(ctx Context, lVal, rVal interface{}) bool { +func checkSetContainsAll(_ Context, lVal, rVal interface{}) bool { // Ensure left-hand is string lStr, ok := lVal.(string) if !ok { @@ -1485,13 +1524,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 +1542,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 2a5d804a2..d93ab37a5 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 5ddc7d680..b959c3fee 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 9a98d2d20..5215318e0 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