luxolus
/
open-nomad
2
0
Fork 0
Code Issues 1 Pull Requests Packages Releases Wiki Activity

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
This commit is contained in:
Seth Hoenig 2022-09-27 11:07:07 -05:00 committed by GitHub
parent 24eea2d4d4
commit 5df5e70542
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 118 additions and 48 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
.changelog/14722.txt Normal file
View File

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

101
scheduler/feasible.go
View File

@ -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
}
}

51
scheduler/feasible_test.go
View File

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

5
website/content/docs/job-specification/constraint.mdx
View File

@ -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
=

6
website/content/docs/upgrade/upgrade-specific.mdx
View File

@ -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