Merge pull request #4875 from hashicorp/f-constraints

scheduler: Make != constraints more flexible

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)]

nomad/structs/structs.go

@@ -6334,13 +6334,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.
@@ -6411,6 +6413,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))

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

scheduler/feasible.go

@@ -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, "${") {
@@ -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
+// 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:
@@ -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
 	}

scheduler/feasible_test.go

@@ -197,12 +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].NodeClass = "large"
+	nodes[2].Attributes["foo"] = "bar"
 
 	constraints := []*structs.Constraint{
 		{
@@ -216,9 +216,13 @@ func TestConstraintChecker(t *testing.T) {
 			RTarget: "linux",
 		},
 		{
-			Operand: "is",
+			Operand: "!=",
 			LTarget: "${node.class}",
-			RTarget: "large",
+			RTarget: "linux-medium-pci",
+		},
+		{
+			Operand: "is_set",
+			LTarget: "${attr.foo}",
 		},
 	}
 	checker := NewConstraintChecker(ctx, constraints)
@@ -294,6 +298,7 @@ func TestResolveConstraintTarget(t *testing.T) {
 		{
 			target: "${attr.rand}",
 			node:   node,
+			val:    "",
 			result: false,
 		},
 		{
@@ -305,6 +310,7 @@ func TestResolveConstraintTarget(t *testing.T) {
 		{
 			target: "${meta.rand}",
 			node:   node,
+			val:    "",
 			result: false,
 		},
 	}
@@ -342,6 +348,21 @@ 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: nil, rVal: nil,
+			result: false,
+		},
 		{
 			op:   "!=",
 			lVal: "foo", rVal: "foo",
@@ -352,6 +373,21 @@ 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:   "!=",
+			lVal: nil, rVal: nil,
+			result: false,
+		},
 		{
 			op:   "not",
 			lVal: "foo", rVal: "bar",
@@ -362,16 +398,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  ",
@@ -382,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)
 		}
 	}
@@ -1972,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"),
@@ -1990,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"),
@@ -2044,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)
 		}
 	}

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

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