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