From 0925bfe6184af5eea425d0c5dce63405a369c1b3 Mon Sep 17 00:00:00 2001 From: Danielle Tomlinson Date: Tue, 13 Nov 2018 13:28:10 -0800 Subject: [PATCH 1/4] scheduler: Allow comparisons of nil values This commit allows the ConstraintChecker to test values that do not exist. This is useful when wanting to _exclude_ given nodes from executing a job, for example, if you wanted to give canary nodes an attribute, and not run critical services on them, you may specify something like the below, but not want to tag all other nodes with the inverse. ```hcl constraint { attribute = "${node.attr.canary} operator = "!=" value = "1" } ``` This also requires all constraint checkers to allow for nil target values, as they will no longer be short circuited by resolving a target. --- scheduler/feasible.go | 20 ++++++++-------- scheduler/feasible_test.go | 47 +++++++++++++++++++++++++++++++++++++- 2 files changed, 56 insertions(+), 11 deletions(-) diff --git a/scheduler/feasible.go b/scheduler/feasible.go index 4db5ae1276e1..20f8e2243f13 100644 --- a/scheduler/feasible.go +++ b/scheduler/feasible.go @@ -398,15 +398,14 @@ 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. + // + // TODO: There may be a case where there is a distinction between not + // present and nil, but I'm not aware of it. Perhaps we should plum this to + // checkConstraint and add an extra level of validation? + lVal, _ := resolveTarget(constraint.LTarget, option) + rVal, _ := resolveTarget(constraint.RTarget, option) // Check if satisfied return checkConstraint(c.ctx, constraint.Operand, lVal, rVal) @@ -448,7 +447,8 @@ func resolveTarget(target string, node *structs.Node) (interface{}, bool) { } } -// checkConstraint checks if a constraint is satisfied +// checkConstraint checks if a constraint is satisfied. The lVal and rVal +// interfaces may be nil. func checkConstraint(ctx Context, operand string, lVal, rVal interface{}) bool { // Check for constraints not handled by this checker. switch operand { diff --git a/scheduler/feasible_test.go b/scheduler/feasible_test.go index abbdcb484d08..0174d9c18e98 100644 --- a/scheduler/feasible_test.go +++ b/scheduler/feasible_test.go @@ -202,7 +202,8 @@ func TestConstraintChecker(t *testing.T) { nodes[0].Attributes["kernel.name"] = "freebsd" nodes[1].Datacenter = "dc2" - nodes[2].NodeClass = "large" + nodes[2].Attributes["not_present"] = "true" + nodes[3].NodeClass = "large" constraints := []*structs.Constraint{ { @@ -215,6 +216,11 @@ func TestConstraintChecker(t *testing.T) { LTarget: "${attr.kernel.name}", RTarget: "linux", }, + { + Operand: "!=", + LTarget: "${attr.not_present}", + RTarget: "true", + }, { Operand: "is", LTarget: "${node.class}", @@ -236,6 +242,10 @@ func TestConstraintChecker(t *testing.T) { }, { Node: nodes[2], + Result: false, + }, + { + Node: nodes[3], Result: true, }, } @@ -342,6 +352,16 @@ func TestCheckConstraint(t *testing.T) { lVal: "foo", rVal: "foo", result: true, }, + { + op: "==", + lVal: "foo", rVal: nil, + result: false, + }, + { + op: "==", + lVal: nil, rVal: "foo", + result: false, + }, { op: "!=", lVal: "foo", rVal: "foo", @@ -352,6 +372,16 @@ func TestCheckConstraint(t *testing.T) { lVal: "foo", rVal: "bar", result: true, }, + { + op: "!=", + lVal: nil, rVal: "foo", + result: true, + }, + { + op: "!=", + lVal: "foo", rVal: nil, + result: true, + }, { op: "not", lVal: "foo", rVal: "bar", @@ -362,16 +392,31 @@ func TestCheckConstraint(t *testing.T) { lVal: "1.2.3", rVal: "~> 1.0", result: true, }, + { + op: structs.ConstraintVersion, + lVal: nil, rVal: "~> 1.0", + result: false, + }, { op: structs.ConstraintRegex, lVal: "foobarbaz", rVal: "[\\w]+", result: true, }, + { + op: structs.ConstraintRegex, + lVal: nil, rVal: "[\\w]+", + result: false, + }, { op: "<", lVal: "foo", rVal: "bar", result: false, }, + { + op: "<", + lVal: nil, rVal: "bar", + result: false, + }, { op: structs.ConstraintSetContains, lVal: "foo,bar,baz", rVal: "foo, bar ", From 3d0a45f6e57b9019de45f9ee5e2d9bdf9a66d759 Mon Sep 17 00:00:00 2001 From: Danielle Tomlinson Date: Tue, 13 Nov 2018 15:57:59 -0800 Subject: [PATCH 2/4] scheduler: Add is_set/is_not_set constraints This adds constraints for asserting that a given attribute or value exists, or does not exist. This acts as a companion to =, or != operators, e.g: ```hcl constraint { attribute = "${attrs.type}" operator = "!=" value = "database" } constraint { attribute = "${attrs.type}" operator = "is_set" } ``` --- nomad/structs/structs.go | 20 +++++--- scheduler/device.go | 12 ++--- scheduler/feasible.go | 102 +++++++++++++++++++++++++------------ scheduler/feasible_test.go | 92 ++++++++++++++++++++++++++++----- scheduler/rank.go | 12 ++--- 5 files changed, 166 insertions(+), 72 deletions(-) diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 3ee54247569d..bf3747352374 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -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. @@ -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)) diff --git a/scheduler/device.go b/scheduler/device.go index 8fbc79c63a55..1970647163da 100644 --- a/scheduler/device.go +++ b/scheduler/device.go @@ -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 diff --git a/scheduler/feasible.go b/scheduler/feasible.go index 20f8e2243f13..ce0f6d1ca970 100644 --- a/scheduler/feasible.go +++ b/scheduler/feasible.go @@ -400,18 +400,14 @@ func (c *ConstraintChecker) Feasible(option *structs.Node) bool { func (c *ConstraintChecker) meetsConstraint(constraint *structs.Constraint, option *structs.Node) bool { // 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. - // - // TODO: There may be a case where there is a distinction between not - // present and nil, but I'm not aware of it. Perhaps we should plum this to - // checkConstraint and add an extra level of validation? - lVal, _ := resolveTarget(constraint.LTarget, option) - rVal, _ := resolveTarget(constraint.RTarget, option) + 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, "${") { @@ -449,7 +445,7 @@ func resolveTarget(target string, node *structs.Node) (interface{}, bool) { // checkConstraint checks if a constraint is satisfied. The lVal and rVal // interfaces may be nil. -func checkConstraint(ctx Context, operand string, lVal, rVal interface{}) bool { +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: @@ -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: + 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 @@ -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 } } @@ -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: @@ -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 "<": @@ -1014,8 +1030,16 @@ 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 { @@ -1023,6 +1047,10 @@ func checkAttributeConstraint(ctx Context, operand string, lVal, rVal *psstructs } 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 { @@ -1031,6 +1059,10 @@ 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 { @@ -1038,6 +1070,10 @@ func checkAttributeConstraint(ctx Context, operand string, lVal, rVal *psstructs } return checkSetContainsAny(ls, rs) + case structs.ConstraintAttributeIsSet: + return lFound + case structs.ConstraintAttributeIsNotSet: + return !lFound default: return false } diff --git a/scheduler/feasible_test.go b/scheduler/feasible_test.go index 0174d9c18e98..98a2f6b5e6db 100644 --- a/scheduler/feasible_test.go +++ b/scheduler/feasible_test.go @@ -197,13 +197,12 @@ func TestConstraintChecker(t *testing.T) { mock.Node(), mock.Node(), mock.Node(), - mock.Node(), } nodes[0].Attributes["kernel.name"] = "freebsd" nodes[1].Datacenter = "dc2" - nodes[2].Attributes["not_present"] = "true" - nodes[3].NodeClass = "large" + nodes[2].NodeClass = "large" + nodes[2].Attributes["foo"] = "bar" constraints := []*structs.Constraint{ { @@ -218,13 +217,12 @@ func TestConstraintChecker(t *testing.T) { }, { Operand: "!=", - LTarget: "${attr.not_present}", - RTarget: "true", + LTarget: "${node.class}", + RTarget: "linux-medium-pci", }, { - Operand: "is", - LTarget: "${node.class}", - RTarget: "large", + Operand: "is_set", + LTarget: "${attr.foo}", }, } checker := NewConstraintChecker(ctx, constraints) @@ -242,10 +240,6 @@ func TestConstraintChecker(t *testing.T) { }, { Node: nodes[2], - Result: false, - }, - { - Node: nodes[3], Result: true, }, } @@ -304,6 +298,7 @@ func TestResolveConstraintTarget(t *testing.T) { { target: "${attr.rand}", node: node, + val: "", result: false, }, { @@ -315,6 +310,7 @@ func TestResolveConstraintTarget(t *testing.T) { { target: "${meta.rand}", node: node, + val: "", result: false, }, } @@ -362,6 +358,11 @@ func TestCheckConstraint(t *testing.T) { lVal: nil, rVal: "foo", result: false, }, + { + op: "==", + lVal: nil, rVal: nil, + result: false, + }, { op: "!=", lVal: "foo", rVal: "foo", @@ -382,6 +383,11 @@ func TestCheckConstraint(t *testing.T) { lVal: "foo", rVal: nil, result: true, }, + { + op: "!=", + lVal: nil, rVal: nil, + result: false, + }, { op: "not", lVal: "foo", rVal: "bar", @@ -427,11 +433,31 @@ func TestCheckConstraint(t *testing.T) { lVal: "foo,bar,baz", rVal: "foo,bam", result: false, }, + { + op: structs.ConstraintAttributeIsSet, + lVal: "foo", + result: true, + }, + { + op: structs.ConstraintAttributeIsSet, + lVal: nil, + result: false, + }, + { + op: structs.ConstraintAttributeIsNotSet, + lVal: nil, + result: true, + }, + { + op: structs.ConstraintAttributeIsNotSet, + lVal: "foo", + result: false, + }, } for _, tc := range cases { _, ctx := testContext(t) - if res := checkConstraint(ctx, tc.op, tc.lVal, tc.rVal); res != tc.result { + if res := checkConstraint(ctx, tc.op, tc.lVal, tc.rVal, tc.lVal != nil, tc.rVal != nil); res != tc.result { t.Fatalf("TC: %#v, Result: %v", tc, res) } } @@ -2017,6 +2043,12 @@ func TestCheckAttributeConstraint(t *testing.T) { rVal: psstructs.NewStringAttribute("foo"), result: true, }, + { + op: "=", + lVal: nil, + rVal: nil, + result: false, + }, { op: "is", lVal: psstructs.NewStringAttribute("foo"), @@ -2035,6 +2067,18 @@ func TestCheckAttributeConstraint(t *testing.T) { rVal: psstructs.NewStringAttribute("foo"), result: false, }, + { + op: "!=", + lVal: nil, + rVal: psstructs.NewStringAttribute("foo"), + result: true, + }, + { + op: "!=", + lVal: psstructs.NewStringAttribute("foo"), + rVal: nil, + result: true, + }, { op: "!=", lVal: psstructs.NewStringAttribute("foo"), @@ -2089,11 +2133,31 @@ func TestCheckAttributeConstraint(t *testing.T) { rVal: psstructs.NewStringAttribute("foo,bam"), result: true, }, + { + op: structs.ConstraintAttributeIsSet, + lVal: psstructs.NewStringAttribute("foo,bar,baz"), + result: true, + }, + { + op: structs.ConstraintAttributeIsSet, + lVal: nil, + result: false, + }, + { + op: structs.ConstraintAttributeIsNotSet, + lVal: psstructs.NewStringAttribute("foo,bar,baz"), + result: false, + }, + { + op: structs.ConstraintAttributeIsNotSet, + lVal: nil, + result: true, + }, } for _, tc := range cases { _, ctx := testContext(t) - if res := checkAttributeConstraint(ctx, tc.op, tc.lVal, tc.rVal); res != tc.result { + if res := checkAttributeConstraint(ctx, tc.op, tc.lVal, tc.rVal, tc.lVal != nil, tc.rVal != nil); res != tc.result { t.Fatalf("TC: %#v, Result: %v", tc, res) } } diff --git a/scheduler/rank.go b/scheduler/rank.go index 7d2b4ad4e931..1ffd028e8dcf 100644 --- a/scheduler/rank.go +++ b/scheduler/rank.go @@ -564,17 +564,11 @@ func (iter *NodeAffinityIterator) Next() *RankedNode { func matchesAffinity(ctx Context, affinity *structs.Affinity, option *structs.Node) bool { //TODO(preetha): Add a step here that filters based on computed node class for potential speedup // Resolve the targets - lVal, ok := resolveTarget(affinity.LTarget, option) - if !ok { - return false - } - rVal, ok := resolveTarget(affinity.RTarget, option) - if !ok { - return false - } + lVal, lOk := resolveTarget(affinity.LTarget, option) + rVal, rOk := resolveTarget(affinity.RTarget, option) // Check if satisfied - return checkAffinity(ctx, affinity.Operand, lVal, rVal) + return checkAffinity(ctx, affinity.Operand, lVal, rVal, lOk, rOk) } // ScoreNormalizationIterator is used to combine scores from various prior From cf8bc3224d5faa50fc67c4301773aeb86906b918 Mon Sep 17 00:00:00 2001 From: Danielle Tomlinson Date: Tue, 13 Nov 2018 19:07:13 -0800 Subject: [PATCH 3/4] docs: Add is_set/is_not_set --- website/source/docs/job-specification/constraint.html.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/website/source/docs/job-specification/constraint.html.md b/website/source/docs/job-specification/constraint.html.md index 6f355799dbc8..0425cff3a3ca 100644 --- a/website/source/docs/job-specification/constraint.html.md +++ b/website/source/docs/job-specification/constraint.html.md @@ -85,6 +85,8 @@ all groups (and tasks) in the job. regexp set_contains version + is_set + is_not_set ``` For a detailed explanation of these values and their behavior, please see @@ -196,6 +198,13 @@ constraint { } ``` +- `"is_set"` - Specifies that a given attribute must be present. This can be + combined with the `"!="` operator to require that an attribute has been set + before checking for equality. The default behavior for `"!="` is to include + nodes that don't have that attribute set. + +- `"is_not_set"` - Specifies that a given attribute must not be present. + ## `constraint` Examples The following examples only show the `constraint` stanzas. Remember that the From 5aac3047e0ec08f8168dfe882186659b977e3d4e Mon Sep 17 00:00:00 2001 From: Danielle Tomlinson Date: Wed, 14 Nov 2018 17:10:12 -0800 Subject: [PATCH 4/4] Add changelog entry for != operator --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a4e8c605580a..7fa7539f21c2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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)]