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

@@ -0,0 +1,3 @@
+```release-note:improvement
+core: constraint operands are now compared numerically if operands are numbers
+```

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,35 +851,73 @@ 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
 	}
 }
 
 // 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
 	}
 }

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))
+		})
 	}
 }
 

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
   =

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