scheduler: Make != constraints more flexible #4875

endocrimes · 2018-11-14T03:12:11Z

There are two main changes in this pull request, one of them is technically a change in existing behaviour:

The `"!="` operator

The != operator will be allowed to evaluate nil or empty values. 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.

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.

The `"is_(not_)set"` operators

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 to restore the previous behaviour of the != operator:

constraint {
      attribute = "${attrs.type}"
      operator  = "!="
      value     = "database"
}

constraint {
      attribute = "${attrs.type}"
      operator  = "is_set"
}

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.

endocrimes · 2018-11-14T19:56:03Z

scheduler/feasible.go

@@ -480,7 +480,8 @@ func checkConstraint(ctx Context, operand string, lVal, rVal interface{}) bool {

 // checkAffinity checks if a specific affinity is satisfied
 func checkAffinity(ctx Context, operand string, lVal, rVal interface{}) bool {
-	return checkConstraint(ctx, operand, lVal, rVal)
+	// We pass ok here for both values, because matchesAffinity prevalidates these


@preetapan I'm not too familiar with the affinity system yet, but do we want to support similar is_set mechanics there too? - Right now, affinities are probably inheriting the new != matching behaviour?

~~yup, they share the same set of operators (except for distincthost/distinctproperty) and the underlying matching code so this should just work for affinities too~~
EDIT - Affinities already inherit the new != behavior like you pointed out.

@preetapan Doesn't this need to change to pass through the lOk, rOk values: https://github.com/hashicorp/nomad/blob/master/scheduler/rank.go#L577

since affinities are optional/best effort with weights it didn't make sense to me to support lok/rok values. ie creating two affinities one for the value being set and another for the value matching a value isn't meaningful when calculating a final score. So I wanted to preserve the existing implementation (only match if both the values are set)

preetapan · 2018-11-14T20:32:53Z

website/source/docs/job-specification/constraint.html.md

@@ -196,6 +198,12 @@ constraint {
    }
    ```

+- `"is_set"` - Specifies that a given attribute must be present. This can be
+  used with features like `!=` to require that an attribute has been configured,


Suggested rewrite : 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.

preetapan · 2018-11-14T20:36:06Z

scheduler/feasible_test.go

@@ -202,7 +202,8 @@ func TestConstraintChecker(t *testing.T) {

 	nodes[0].Attributes["kernel.name"] = "freebsd"


can you add test cases with constraints using the is_set/is_not_set operators?

Oh yep, sorry - thought I'd already added those.

preetapan

left comments

endocrimes · 2018-11-14T20:54:13Z

@preetapan Updated - PTAL then I'll squash the fixups into the respective commits.

dadgar · 2018-11-14T22:24:25Z

scheduler/feasible.go

@@ -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


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

dadgar · 2018-11-14T22:29:21Z

scheduler/feasible.go

@@ -465,6 +461,10 @@ func checkConstraint(ctx Context, operand string, lVal, rVal interface{}) bool {
 		return !reflect.DeepEqual(lVal, rVal)
 	case "<", "<=", ">", ">=":
 		return checkLexicalOrder(operand, lVal, rVal)
+	case structs.ConstraintAttributeIsSet:


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

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}" }

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.

Good catch 😬 🙈

preetapan

edited comment

endocrimes · 2018-11-15T00:04:09Z

@dadgar Updated 👍

dadgar · 2018-11-15T00:23:33Z

scheduler/feasible_test.go

@@ -342,6 +348,16 @@ func TestCheckConstraint(t *testing.T) {
 			lVal: "foo", rVal: "foo",
 			result: true,
 		},
+		{
+			op:   "==",


do a nil nil case

dadgar · 2018-11-15T00:25:43Z

scheduler/feasible.go

@@ -480,7 +480,8 @@ func checkConstraint(ctx Context, operand string, lVal, rVal interface{}) bool {

 // checkAffinity checks if a specific affinity is satisfied
 func checkAffinity(ctx Context, operand string, lVal, rVal interface{}) bool {
-	return checkConstraint(ctx, operand, lVal, rVal)
+	// We pass ok here for both values, because matchesAffinity prevalidates these


@preetapan Doesn't this need to change to pass through the lOk, rOk values: https://github.com/hashicorp/nomad/blob/master/scheduler/rank.go#L577

dadgar · 2018-11-15T00:26:31Z

scheduler/feasible.go

 }

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


Here should pass through as well the lFound/rFound values

dadgar · 2018-11-15T18:01:34Z

Once merged add to changelog please

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" } ```

github-actions · 2023-02-24T02:17:30Z

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

endocrimes force-pushed the f-constraints branch from 981dd2e to 44bd0c9 Compare November 14, 2018 03:14

endocrimes requested a review from preetapan November 14, 2018 04:01

endocrimes commented Nov 14, 2018

View reviewed changes

preetapan reviewed Nov 14, 2018

View reviewed changes

preetapan suggested changes Nov 14, 2018

View reviewed changes

dadgar requested changes Nov 14, 2018

View reviewed changes

preetapan approved these changes Nov 14, 2018

View reviewed changes

preetapan suggested changes Nov 14, 2018

View reviewed changes

endocrimes force-pushed the f-constraints branch from d7f7b13 to f450b28 Compare November 15, 2018 00:02

dadgar requested changes Nov 15, 2018

View reviewed changes

dadgar approved these changes Nov 15, 2018

View reviewed changes

endocrimes added 3 commits November 15, 2018 11:00

docs: Add is_set/is_not_set

cf8bc32

Add changelog entry for != operator

5aac304

endocrimes force-pushed the f-constraints branch from 343377e to 5aac304 Compare November 15, 2018 19:01

endocrimes merged commit 60c6cb8 into master Nov 15, 2018

endocrimes deleted the f-constraints branch November 15, 2018 19:04

github-actions bot locked as resolved and limited conversation to collaborators Feb 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scheduler: Make != constraints more flexible #4875

scheduler: Make != constraints more flexible #4875

endocrimes commented Nov 14, 2018 •

edited

Loading

endocrimes Nov 14, 2018

preetapan Nov 14, 2018 •

edited

Loading

dadgar Nov 15, 2018

preetapan Nov 15, 2018 •

edited

Loading

preetapan Nov 14, 2018 •

edited

Loading

preetapan Nov 14, 2018

endocrimes Nov 14, 2018

preetapan left a comment

endocrimes commented Nov 14, 2018

dadgar Nov 14, 2018

dadgar Nov 14, 2018

dadgar Nov 14, 2018

preetapan Nov 14, 2018

endocrimes Nov 14, 2018

preetapan left a comment •

edited

Loading

endocrimes commented Nov 15, 2018

dadgar Nov 15, 2018

dadgar Nov 15, 2018

dadgar Nov 15, 2018

dadgar commented Nov 15, 2018

github-actions bot commented Feb 24, 2023

		@@ -202,7 +202,8 @@ func TestConstraintChecker(t *testing.T) {

		nodes[0].Attributes["kernel.name"] = "freebsd"

scheduler: Make != constraints more flexible #4875

scheduler: Make != constraints more flexible #4875

Conversation

endocrimes commented Nov 14, 2018 • edited Loading

The "!=" operator

The "is_(not_)set" operators

Choose a reason for hiding this comment

preetapan Nov 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

preetapan Nov 15, 2018 • edited Loading

Choose a reason for hiding this comment

preetapan Nov 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

preetapan left a comment

Choose a reason for hiding this comment

endocrimes commented Nov 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

preetapan left a comment • edited Loading

Choose a reason for hiding this comment

endocrimes commented Nov 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dadgar commented Nov 15, 2018

github-actions bot commented Feb 24, 2023

endocrimes commented Nov 14, 2018 •

edited

Loading

The `"!="` operator

The `"is_(not_)set"` operators

preetapan Nov 14, 2018 •

edited

Loading

preetapan Nov 15, 2018 •

edited

Loading

preetapan Nov 14, 2018 •

edited

Loading

preetapan left a comment •

edited

Loading