distinct_property constraint
#2418
Conversation
scheduler/feasible.go
Outdated
| // Hot-path if the option is nil or there are no distinct_hosts or | ||
| // distinct_property constraints. | ||
| hosts := iter.jobDistinctHosts || iter.tgDistinctHosts | ||
| properties := iter.distinctProperties.HasDistinctPropertyConstraints() |
There was a problem hiding this comment.
Why not call HasDistinctPropertyConstraints() in SetJob and then just check for a nil distinctProperties here? Seems more efficient that way, avoids some allocations.
scheduler/feasible.go
Outdated
| } | ||
|
|
||
| // Check if the property constraints are satisfied | ||
| if properties { |
There was a problem hiding this comment.
I wonder if this should just be a new iterator? Seem's odd to create a split code path like this, instead of just having another iterator type.
dadgar
force-pushed
the
f-distinct-property
branch
2 times, most recently
from
81fc49c
to
d688a7f
Compare
dadgar
force-pushed
the
f-distinct-property
branch
2 times, most recently
from
5432abc
to
e03bb92
Compare
dadgar
force-pushed
the
f-distinct-property
branch
from
e03bb92
to
ddb9292
Compare
scheduler/feasible.go
Outdated
| iter.jobPropertySets = append(iter.jobPropertySets, pset) | ||
| } | ||
|
|
||
| for _, tg := range job.TaskGroups { |
There was a problem hiding this comment.
Why not do this inside SetTaskGroup?
There was a problem hiding this comment.
Just realizes SetTFConstraint populates, so doing this as lazily as possible seems preferable.
scheduler/feasible.go
Outdated
| } | ||
|
|
||
| // Check if the constraints are met | ||
| for _, ps := range iter.jobPropertySets { |
There was a problem hiding this comment.
You can probably just have a helper method for satisfiesProperties() which takes a list, and call it once for jobPropertySets and groupPropertySets to DRY
| both := make([]*structs.Allocation, 0, len(stopping)+len(proposed)) | ||
| both = append(both, stopping...) | ||
| both = append(both, proposed...) | ||
| nodes, err := p.buildNodeMap(both) |
There was a problem hiding this comment.
I think there should be a helper that takes []*Allocation and map[string]struct{} and does both the node map building, and property setting. It seems that pattern exists a few times here.
| continue | ||
| } | ||
|
|
||
| return false, fmt.Sprintf("distinct_property: %s=%s already used", p.constraint.LTarget, nValue) |
There was a problem hiding this comment.
I wonder if including the value which may be distinct could case an explosion of annotations for AllocMetrics e.g. if I have high cardinality. Might be not worth considering at this point.
There was a problem hiding this comment.
One of your tests is failing, otherwise looks good to my untrained eye.
--- FAIL: TestServiceSched_JobModify_DistinctProperty (0.00s)
generic_sched_test.go:1649: bad: []*structs.Allocation{(*structs.Allocation)(0xc420443400), (*structs.Allocation)(0xc420442100), (*structs.Allocation)(0xc420442600), (*structs.Allocation)(0xc420442b00), (*structs.Allocation)(0xc420442f00), (*structs.Allocation)(0xc420443200)}
|
@schmichael Thanks! Its fixed in the most recent commit! |
|
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. |
This PR introduces a distinct property constraint. This allows constraints such as:
Which causes each placement to be on a node with a distinct value of
${meta.rack}.Fixes #1387