From 37d9018e8991a06eda318d5e17beee0e3342cfdd Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Fri, 14 Apr 2023 16:26:19 -0400 Subject: [PATCH 1/4] Honor value for distinct_hosts constraint --- jobspec2/hcl_conversions.go | 5 ++++- scheduler/feasible.go | 9 ++++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/jobspec2/hcl_conversions.go b/jobspec2/hcl_conversions.go index 4e4286a6efa..8ce337a54a7 100644 --- a/jobspec2/hcl_conversions.go +++ b/jobspec2/hcl_conversions.go @@ -235,8 +235,11 @@ func decodeConstraint(body hcl.Body, ctx *hcl.EvalContext, val interface{}) hcl. c.RTarget = constraint } - if d := v.GetAttr(api.ConstraintDistinctHosts); !d.IsNull() && d.True() { + // The shortcut form of the distinct_hosts constraint is a cty.Bool + // so it can not use the `attr` func defined earlier + if d := v.GetAttr(api.ConstraintDistinctHosts); !d.IsNull() { c.Operand = api.ConstraintDistinctHosts + c.RTarget = fmt.Sprint(d.True()) } if property := attr(api.ConstraintDistinctProperty); property != "" { diff --git a/scheduler/feasible.go b/scheduler/feasible.go index b0ae969f675..8ca25bb5c9f 100644 --- a/scheduler/feasible.go +++ b/scheduler/feasible.go @@ -572,7 +572,14 @@ func (iter *DistinctHostsIterator) SetJob(job *structs.Job) { func (iter *DistinctHostsIterator) hasDistinctHostsConstraint(constraints []*structs.Constraint) bool { for _, con := range constraints { if con.Operand == structs.ConstraintDistinctHosts { - return true + // Maintain the old behavior around unset RTargets + if con.RTarget == "" { + return true + } + enabled, err := strconv.ParseBool(con.RTarget) + // If the value is unparsable as a boolean, fall back to the old behavior + // of enforcing the constraint when it appears. + return err != nil || enabled } } From 26537fa334921d57015dbfb09a657b60f22944a8 Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Mon, 17 Apr 2023 14:51:32 -0400 Subject: [PATCH 2/4] Add test for feasibility checking for `false` --- scheduler/feasible_test.go | 99 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 99 insertions(+) diff --git a/scheduler/feasible_test.go b/scheduler/feasible_test.go index 64111e536c3..5cd1bfad4ed 100644 --- a/scheduler/feasible_test.go +++ b/scheduler/feasible_test.go @@ -1437,6 +1437,105 @@ func TestDistinctHostsIterator_JobDistinctHosts(t *testing.T) { } } +func TestDistinctHostsIterator_JobDistinctHosts_Table(t *testing.T) { + ci.Parallel(t) + + // Create a job with a distinct_hosts constraint and a task group. + tg1 := &structs.TaskGroup{Name: "bar"} + job := &structs.Job{ + ID: "foo", + Namespace: structs.DefaultNamespace, + Constraints: []*structs.Constraint{{ + Operand: structs.ConstraintDistinctHosts, + }}, + TaskGroups: []*structs.TaskGroup{tg1}, + } + makeJobAllocs := func(js []*structs.Job) []*structs.Allocation { + na := make([]*structs.Allocation, len(js)) + for i, j := range js { + allocID := uuid.Generate() + na[i] = &structs.Allocation{ + Namespace: structs.DefaultNamespace, + TaskGroup: j.TaskGroups[0].Name, + JobID: j.ID, + Job: j, + ID: allocID, + } + } + return na + } + + n1 := mock.Node() + n2 := mock.Node() + n3 := mock.Node() + + testCases := []struct { + name string + RTarget string + expectLen int + expectNodes []*structs.Node + }{ + { + name: "unset", + RTarget: "", + expectLen: 1, + expectNodes: []*structs.Node{n3}, + }, + { + name: "true", + RTarget: "true", + expectLen: 1, + expectNodes: []*structs.Node{n3}, + }, + { + name: "false", + RTarget: "false", + expectLen: 3, + expectNodes: []*structs.Node{n1, n2, n3}, + }, + { + name: "unparsable", + RTarget: "not_a_bool", + expectLen: 1, + expectNodes: []*structs.Node{n3}, + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + tc := tc + ci.Parallel(t) + + _, ctx := testContext(t) + static := NewStaticIterator(ctx, []*structs.Node{n1, n2, n3}) + + j := job.Copy() + j.Constraints[0].RTarget = tc.RTarget + + oj := j.Copy() + oj.ID = "otherJob" + + plan := ctx.Plan() + // Add allocations so that some of the nodes will be ineligible + // to receive the job when the distinct_hosts constraint + // is active. This will require the job be placed on n3. + // + // Another job is placed on all of the nodes to ensure that there + // are no unexpected interactions. + plan.NodeAllocation[n1.ID] = makeJobAllocs([]*structs.Job{j, oj}) + plan.NodeAllocation[n2.ID] = makeJobAllocs([]*structs.Job{j, oj}) + plan.NodeAllocation[n3.ID] = makeJobAllocs([]*structs.Job{oj}) + + proposed := NewDistinctHostsIterator(ctx, static) + proposed.SetTaskGroup(tg1) + proposed.SetJob(j) + + out := collectFeasible(proposed) + must.Len(t, tc.expectLen, out, must.Sprintf("Bad: %v", out)) + must.SliceContainsAll(t, tc.expectNodes, out) + }) + } +} + func TestDistinctHostsIterator_JobDistinctHosts_InfeasibleCount(t *testing.T) { ci.Parallel(t) From 768747f88942e3a9682072480bf8396d8c238719 Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Mon, 17 Apr 2023 16:13:18 -0400 Subject: [PATCH 3/4] Add changelog --- .changelog/16907.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/16907.txt diff --git a/.changelog/16907.txt b/.changelog/16907.txt new file mode 100644 index 00000000000..6b31fb4933a --- /dev/null +++ b/.changelog/16907.txt @@ -0,0 +1,3 @@ +```release-note:bug +scheduler: honor false value for distinct_hosts constraint +``` From 633894d5526b529777ca4fe7913a23d8b1b74781 Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Mon, 17 Apr 2023 17:35:42 -0400 Subject: [PATCH 4/4] Apply suggestion from code review Co-authored-by: Michael Schurter --- scheduler/feasible.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scheduler/feasible.go b/scheduler/feasible.go index 8ca25bb5c9f..03e619e7d64 100644 --- a/scheduler/feasible.go +++ b/scheduler/feasible.go @@ -572,7 +572,7 @@ func (iter *DistinctHostsIterator) SetJob(job *structs.Job) { func (iter *DistinctHostsIterator) hasDistinctHostsConstraint(constraints []*structs.Constraint) bool { for _, con := range constraints { if con.Operand == structs.ConstraintDistinctHosts { - // Maintain the old behavior around unset RTargets + // distinct_hosts defaults to true if con.RTarget == "" { return true }