Skip to content
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

scheduler: Make != constraints more flexible #4875

Merged
merged 4 commits into from
Nov 15, 2018
Merged

scheduler: Make != constraints more flexible #4875

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0925bfe
scheduler: Allow comparisons of nil values
endocrimes Nov 13, 2018
3d0a45f
scheduler: Add is_set/is_not_set constraints
endocrimes Nov 13, 2018
cf8bc32
docs: Add is_set/is_not_set
endocrimes Nov 14, 2018
5aac304
Add changelog entry for != operator
endocrimes Nov 15, 2018
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 CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

__BACKWARDS INCOMPATIBILITIES:__
* core: Switch to structured logging using [go-hclog](https://github.com/hashicorp/go-hclog)
* core: Allow the != constraint to match against keys that do not exist [[GH-4875](https://github.com/hashicorp/nomad/pull/4875)]

IMPROVEMENTS:
* core: Added advertise address to client node meta data [[GH-4390](https://github.com/hashicorp/nomad/issues/4390)]
Expand Down
20 changes: 13 additions & 7 deletions nomad/structs/structs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6324,13 +6324,15 @@ func (ta *TaskArtifact) validateChecksum() error {
}

const (
ConstraintDistinctProperty = "distinct_property"
ConstraintDistinctHosts = "distinct_hosts"
ConstraintRegex = "regexp"
ConstraintVersion = "version"
ConstraintSetContains = "set_contains"
ConstraintSetContainsAll = "set_contains_all"
ConstraintSetContainsAny = "set_contains_any"
ConstraintDistinctProperty = "distinct_property"
ConstraintDistinctHosts = "distinct_hosts"
ConstraintRegex = "regexp"
ConstraintVersion = "version"
ConstraintSetContains = "set_contains"
ConstraintSetContainsAll = "set_contains_all"
ConstraintSetContainsAny = "set_contains_any"
ConstraintAttributeIsSet = "is_set"
ConstraintAttributeIsNotSet = "is_not_set"
)

// Constraints are used to restrict placement options.
Expand Down Expand Up @@ -6401,6 +6403,10 @@ func (c *Constraint) Validate() error {
mErr.Errors = append(mErr.Errors, fmt.Errorf("Distinct Property must have an allowed count of 1 or greater: %d < 1", count))
}
}
case ConstraintAttributeIsSet, ConstraintAttributeIsNotSet:
if c.RTarget != "" {
mErr.Errors = append(mErr.Errors, fmt.Errorf("Operator %q does not support an RTarget", c.Operand))
}
case "=", "==", "is", "!=", "not", "<", "<=", ">", ">=":
if c.RTarget == "" {
mErr.Errors = append(mErr.Errors, fmt.Errorf("Operator %q requires an RTarget", c.Operand))
Expand Down
12 changes: 3 additions & 9 deletions scheduler/device.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,19 +74,13 @@ func (d *deviceAllocator) AssignDevice(ask *structs.RequestedDevice) (out *struc
totalWeight := 0.0
for _, a := range ask.Affinities {
// Resolve the targets
lVal, ok := resolveDeviceTarget(a.LTarget, devInst.Device)
if !ok {
continue
}
rVal, ok := resolveDeviceTarget(a.RTarget, devInst.Device)
if !ok {
continue
}
lVal, lOk := resolveDeviceTarget(a.LTarget, devInst.Device)
rVal, rOk := resolveDeviceTarget(a.RTarget, devInst.Device)

totalWeight += math.Abs(a.Weight)

// Check if satisfied
if !checkAttributeAffinity(d.ctx, a.Operand, lVal, rVal) {
if !checkAttributeAffinity(d.ctx, a.Operand, lVal, rVal, lOk, rOk) {
continue
}
choiceScore += a.Weight
Expand Down
110 changes: 73 additions & 37 deletions scheduler/feasible.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,21 +398,16 @@ func (c *ConstraintChecker) Feasible(option *structs.Node) bool {
}

func (c *ConstraintChecker) meetsConstraint(constraint *structs.Constraint, option *structs.Node) bool {
// Resolve the targets
lVal, ok := resolveTarget(constraint.LTarget, option)
if !ok {
return false
}
rVal, ok := resolveTarget(constraint.RTarget, option)
if !ok {
return false
}
// Resolve the targets. Targets that are not present are treated as `nil`.
// This is to allow for matching constraints where a target is not present.
lVal, lOk := resolveTarget(constraint.LTarget, option)
rVal, rOk := resolveTarget(constraint.RTarget, option)

// Check if satisfied
return checkConstraint(c.ctx, constraint.Operand, lVal, rVal)
return checkConstraint(c.ctx, constraint.Operand, lVal, rVal, lOk, rOk)
}

// resolveTarget is used to resolve the LTarget and RTarget of a Constraint
// resolveTarget is used to resolve the LTarget and RTarget of a Constraint.
func resolveTarget(target string, node *structs.Node) (interface{}, bool) {
// If no prefix, this must be a literal value
if !strings.HasPrefix(target, "${") {
Expand Down Expand Up @@ -448,8 +443,9 @@ func resolveTarget(target string, node *structs.Node) (interface{}, bool) {
}
}

// checkConstraint checks if a constraint is satisfied
func checkConstraint(ctx Context, operand string, lVal, rVal interface{}) bool {
// checkConstraint checks if a constraint is satisfied. The lVal and rVal
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to do the same here too: https://github.com/hashicorp/nomad/blob/136dace45c34223bb0a63ba7a0019792d6e7d797/scheduler/feasible.go#L984

// interfaces may be nil.
func checkConstraint(ctx Context, operand string, lVal, rVal interface{}, lFound, rFound bool) bool {
// Check for constraints not handled by this checker.
switch operand {
case structs.ConstraintDistinctHosts, structs.ConstraintDistinctProperty:
Expand All @@ -460,32 +456,36 @@ func checkConstraint(ctx Context, operand string, lVal, rVal interface{}) bool {

switch operand {
case "=", "==", "is":
return reflect.DeepEqual(lVal, rVal)
return lFound && rFound && reflect.DeepEqual(lVal, rVal)
case "!=", "not":
return !reflect.DeepEqual(lVal, rVal)
case "<", "<=", ">", ">=":
return checkLexicalOrder(operand, lVal, rVal)
return lFound && rFound && checkLexicalOrder(operand, lVal, rVal)
case structs.ConstraintAttributeIsSet:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't all the existing except != and not require lFound && rFound to be true

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise this is true and that is a change in behavior and seemingly undesirable:

constraint {
  attribute = "${meta.my-non-existing}"
  operator  = "=="
  value     = "${meta.my-other-non-existing}"
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch.. first I thought the existing methods for other operators already handled nils inside by returning false after casting fails. But reflect.deepequal(nil, nil) will return true, so we should capture that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch 😬 🙈

return lFound
case structs.ConstraintAttributeIsNotSet:
return !lFound
case structs.ConstraintVersion:
return checkVersionMatch(ctx, lVal, rVal)
return lFound && rFound && checkVersionMatch(ctx, lVal, rVal)
case structs.ConstraintRegex:
return checkRegexpMatch(ctx, lVal, rVal)
return lFound && rFound && checkRegexpMatch(ctx, lVal, rVal)
case structs.ConstraintSetContains, structs.ConstraintSetContainsAll:
return checkSetContainsAll(ctx, lVal, rVal)
return lFound && rFound && checkSetContainsAll(ctx, lVal, rVal)
case structs.ConstraintSetContainsAny:
return checkSetContainsAny(lVal, rVal)
return lFound && rFound && checkSetContainsAny(lVal, rVal)
default:
return false
}
}

// checkAffinity checks if a specific affinity is satisfied
func checkAffinity(ctx Context, operand string, lVal, rVal interface{}) bool {
return checkConstraint(ctx, operand, lVal, rVal)
func checkAffinity(ctx Context, operand string, lVal, rVal interface{}, lFound, rFound bool) bool {
return checkConstraint(ctx, operand, lVal, rVal, lFound, rFound)
}

// checkAttributeAffinity checks if an affinity is satisfied
func checkAttributeAffinity(ctx Context, operand string, lVal, rVal *psstructs.Attribute) bool {
return checkAttributeConstraint(ctx, operand, lVal, rVal)
func checkAttributeAffinity(ctx Context, operand string, lVal, rVal *psstructs.Attribute, lFound, rFound bool) bool {
return checkAttributeConstraint(ctx, operand, lVal, rVal, lFound, rFound)
}

// checkLexicalOrder is used to check for lexical ordering
Expand Down Expand Up @@ -931,17 +931,11 @@ func nodeDeviceMatches(ctx Context, d *structs.NodeDeviceResource, req *structs.

for _, c := range req.Constraints {
// Resolve the targets
lVal, ok := resolveDeviceTarget(c.LTarget, d)
if !ok {
return false
}
rVal, ok := resolveDeviceTarget(c.RTarget, d)
if !ok {
return false
}
lVal, lOk := resolveDeviceTarget(c.LTarget, d)
rVal, rOk := resolveDeviceTarget(c.RTarget, d)

// Check if satisfied
if !checkAttributeConstraint(ctx, c.Operand, lVal, rVal) {
if !checkAttributeConstraint(ctx, c.Operand, lVal, rVal, lOk, rOk) {
return false
}
}
Expand Down Expand Up @@ -979,8 +973,9 @@ func resolveDeviceTarget(target string, d *structs.NodeDeviceResource) (*psstruc
}
}

// checkAttributeConstraint checks if a constraint is satisfied
func checkAttributeConstraint(ctx Context, operand string, lVal, rVal *psstructs.Attribute) bool {
// checkAttributeConstraint checks if a constraint is satisfied. nil equality
// comparisons are considered to be false.
func checkAttributeConstraint(ctx Context, operand string, lVal, rVal *psstructs.Attribute, lFound, rFound bool) bool {
// Check for constraints not handled by this checker.
switch operand {
case structs.ConstraintDistinctHosts, structs.ConstraintDistinctProperty:
Expand All @@ -990,15 +985,36 @@ func checkAttributeConstraint(ctx Context, operand string, lVal, rVal *psstructs
}

switch operand {
case "<", "<=", ">", ">=", "=", "==", "is", "!=", "not":
case "!=", "not":
// Neither value was provided, nil != nil == false
if !(lFound || rFound) {
return false
}

// Only 1 value was provided, therefore nil != some == true
if lFound != rFound {
return true
}

// Both values were provided, so actually compare them
v, ok := lVal.Compare(rVal)
if !ok {
return false
}

return v != 0

case "<", "<=", ">", ">=", "=", "==", "is":
if !(lFound && rFound) {
return false
}

v, ok := lVal.Compare(rVal)
if !ok {
return false
}

switch operand {
case "not", "!=":
return v != 0
case "is", "==", "=":
return v == 0
case "<":
Expand All @@ -1014,15 +1030,27 @@ func checkAttributeConstraint(ctx Context, operand string, lVal, rVal *psstructs
}

case structs.ConstraintVersion:
if !(lFound && rFound) {
return false
}

return checkAttributeVersionMatch(ctx, lVal, rVal)
case structs.ConstraintRegex:
if !(lFound && rFound) {
return false
}

ls, ok := lVal.GetString()
rs, ok2 := rVal.GetString()
if !ok || !ok2 {
return false
}
return checkRegexpMatch(ctx, ls, rs)
case structs.ConstraintSetContains, structs.ConstraintSetContainsAll:
if !(lFound && rFound) {
return false
}

ls, ok := lVal.GetString()
rs, ok2 := rVal.GetString()
if !ok || !ok2 {
Expand All @@ -1031,13 +1059,21 @@ func checkAttributeConstraint(ctx Context, operand string, lVal, rVal *psstructs

return checkSetContainsAll(ctx, ls, rs)
case structs.ConstraintSetContainsAny:
if !(lFound && rFound) {
return false
}

ls, ok := lVal.GetString()
rs, ok2 := rVal.GetString()
if !ok || !ok2 {
return false
}

return checkSetContainsAny(ls, rs)
case structs.ConstraintAttributeIsSet:
return lFound
case structs.ConstraintAttributeIsNotSet:
return !lFound
default:
return false
}
Expand Down
Loading