From 3d0a45f6e57b9019de45f9ee5e2d9bdf9a66d759 Mon Sep 17 00:00:00 2001 From: Danielle Tomlinson Date: Tue, 13 Nov 2018 15:57:59 -0800 Subject: [PATCH] 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 3ee542475..bf3747352 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 8fbc79c63..197064716 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 20f8e2243..ce0f6d1ca 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 0174d9c18..98a2f6b5e 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 7d2b4ad4e..1ffd028e8 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