-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
distinct_property
constraint
#2418
Merged
+1,282
−105
Merged
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
86b0304
Unoptimized implementation + testing
dadgar fdc4309
Property Set
dadgar 868cbe1
cleanup
dadgar 653a1c3
Split distinct property and host iterator and add iterator to system …
dadgar e4954a4
Refactor
dadgar ddb9292
Fix filtering issue and add a test that would catch it
dadgar cab1a8d
Feedback addressed
dadgar 336a976
Fix in-place update
dadgar File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
job "foo" { | ||
constraint { | ||
distinct_property = "${meta.rack}" | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -142,12 +142,10 @@ func (c *DriverChecker) hasDrivers(option *structs.Node) bool { | |
return true | ||
} | ||
|
||
// ProposedAllocConstraintIterator is a FeasibleIterator which returns nodes that | ||
// match constraints that are not static such as Node attributes but are | ||
// effected by proposed alloc placements. Examples are distinct_hosts and | ||
// tenancy constraints. This is used to filter on job and task group | ||
// constraints. | ||
type ProposedAllocConstraintIterator struct { | ||
// DistinctHostsIterator is a FeasibleIterator which returns nodes that pass the | ||
// distinct_hosts constraint. The constraint ensures that multiple allocations | ||
// do not exist on the same node. | ||
type DistinctHostsIterator struct { | ||
ctx Context | ||
source FeasibleIterator | ||
tg *structs.TaskGroup | ||
|
@@ -159,44 +157,47 @@ type ProposedAllocConstraintIterator struct { | |
jobDistinctHosts bool | ||
} | ||
|
||
// NewProposedAllocConstraintIterator creates a ProposedAllocConstraintIterator | ||
// from a source. | ||
func NewProposedAllocConstraintIterator(ctx Context, source FeasibleIterator) *ProposedAllocConstraintIterator { | ||
return &ProposedAllocConstraintIterator{ | ||
// NewDistinctHostsIterator creates a DistinctHostsIterator from a source. | ||
func NewDistinctHostsIterator(ctx Context, source FeasibleIterator) *DistinctHostsIterator { | ||
return &DistinctHostsIterator{ | ||
ctx: ctx, | ||
source: source, | ||
} | ||
} | ||
|
||
func (iter *ProposedAllocConstraintIterator) SetTaskGroup(tg *structs.TaskGroup) { | ||
func (iter *DistinctHostsIterator) SetTaskGroup(tg *structs.TaskGroup) { | ||
iter.tg = tg | ||
iter.tgDistinctHosts = iter.hasDistinctHostsConstraint(tg.Constraints) | ||
} | ||
|
||
func (iter *ProposedAllocConstraintIterator) SetJob(job *structs.Job) { | ||
func (iter *DistinctHostsIterator) SetJob(job *structs.Job) { | ||
iter.job = job | ||
iter.jobDistinctHosts = iter.hasDistinctHostsConstraint(job.Constraints) | ||
} | ||
|
||
func (iter *ProposedAllocConstraintIterator) hasDistinctHostsConstraint(constraints []*structs.Constraint) bool { | ||
func (iter *DistinctHostsIterator) hasDistinctHostsConstraint(constraints []*structs.Constraint) bool { | ||
for _, con := range constraints { | ||
if con.Operand == structs.ConstraintDistinctHosts { | ||
return true | ||
} | ||
} | ||
|
||
return false | ||
} | ||
|
||
func (iter *ProposedAllocConstraintIterator) Next() *structs.Node { | ||
func (iter *DistinctHostsIterator) Next() *structs.Node { | ||
for { | ||
// Get the next option from the source | ||
option := iter.source.Next() | ||
|
||
// Hot-path if the option is nil or there are no distinct_hosts constraints. | ||
if option == nil || !(iter.jobDistinctHosts || iter.tgDistinctHosts) { | ||
// Hot-path if the option is nil or there are no distinct_hosts or | ||
// distinct_property constraints. | ||
hosts := iter.jobDistinctHosts || iter.tgDistinctHosts | ||
if option == nil || !hosts { | ||
return option | ||
} | ||
|
||
// Check if the host constraints are satisfied | ||
if !iter.satisfiesDistinctHosts(option) { | ||
iter.ctx.Metrics().FilterNode(option, structs.ConstraintDistinctHosts) | ||
continue | ||
|
@@ -208,7 +209,7 @@ func (iter *ProposedAllocConstraintIterator) Next() *structs.Node { | |
|
||
// satisfiesDistinctHosts checks if the node satisfies a distinct_hosts | ||
// constraint either specified at the job level or the TaskGroup level. | ||
func (iter *ProposedAllocConstraintIterator) satisfiesDistinctHosts(option *structs.Node) bool { | ||
func (iter *DistinctHostsIterator) satisfiesDistinctHosts(option *structs.Node) bool { | ||
// Check if there is no constraint set. | ||
if !(iter.jobDistinctHosts || iter.tgDistinctHosts) { | ||
return true | ||
|
@@ -237,10 +238,111 @@ func (iter *ProposedAllocConstraintIterator) satisfiesDistinctHosts(option *stru | |
return true | ||
} | ||
|
||
func (iter *ProposedAllocConstraintIterator) Reset() { | ||
func (iter *DistinctHostsIterator) Reset() { | ||
iter.source.Reset() | ||
} | ||
|
||
// DistinctPropertyIterator is a FeasibleIterator which returns nodes that pass the | ||
// distinct_property constraint. The constraint ensures that multiple allocations | ||
// do not use the same value of the given property. | ||
type DistinctPropertyIterator struct { | ||
ctx Context | ||
source FeasibleIterator | ||
tg *structs.TaskGroup | ||
job *structs.Job | ||
|
||
hasDistinctPropertyConstraints bool | ||
jobPropertySets []*propertySet | ||
groupPropertySets map[string][]*propertySet | ||
} | ||
|
||
// NewDistinctPropertyIterator creates a DistinctPropertyIterator from a source. | ||
func NewDistinctPropertyIterator(ctx Context, source FeasibleIterator) *DistinctPropertyIterator { | ||
return &DistinctPropertyIterator{ | ||
ctx: ctx, | ||
source: source, | ||
groupPropertySets: make(map[string][]*propertySet), | ||
} | ||
} | ||
|
||
func (iter *DistinctPropertyIterator) SetTaskGroup(tg *structs.TaskGroup) { | ||
iter.tg = tg | ||
|
||
// Check if there is a distinct property | ||
iter.hasDistinctPropertyConstraints = len(iter.jobPropertySets) != 0 || len(iter.groupPropertySets[tg.Name]) != 0 | ||
} | ||
|
||
func (iter *DistinctPropertyIterator) SetJob(job *structs.Job) { | ||
iter.job = job | ||
|
||
// Build the property sets | ||
for _, c := range job.Constraints { | ||
if c.Operand != structs.ConstraintDistinctProperty { | ||
continue | ||
} | ||
|
||
pset := NewPropertySet(iter.ctx, job) | ||
pset.SetJobConstraint(c) | ||
iter.jobPropertySets = append(iter.jobPropertySets, pset) | ||
} | ||
|
||
for _, tg := range job.TaskGroups { | ||
for _, c := range tg.Constraints { | ||
if c.Operand != structs.ConstraintDistinctProperty { | ||
continue | ||
} | ||
|
||
pset := NewPropertySet(iter.ctx, job) | ||
pset.SetTGConstraint(c, tg.Name) | ||
iter.groupPropertySets[tg.Name] = append(iter.groupPropertySets[tg.Name], pset) | ||
} | ||
} | ||
} | ||
|
||
func (iter *DistinctPropertyIterator) Next() *structs.Node { | ||
OUTER: | ||
for { | ||
// Get the next option from the source | ||
option := iter.source.Next() | ||
|
||
// Hot path if there is nothing to check | ||
if option == nil || !iter.hasDistinctPropertyConstraints { | ||
return option | ||
} | ||
|
||
// Check if the constraints are met | ||
for _, ps := range iter.jobPropertySets { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can probably just have a helper method for |
||
if satisfies, reason := ps.SatisfiesDistinctProperties(option, iter.tg.Name); !satisfies { | ||
iter.ctx.Metrics().FilterNode(option, reason) | ||
continue OUTER | ||
} | ||
} | ||
|
||
for _, ps := range iter.groupPropertySets[iter.tg.Name] { | ||
if satisfies, reason := ps.SatisfiesDistinctProperties(option, iter.tg.Name); !satisfies { | ||
iter.ctx.Metrics().FilterNode(option, reason) | ||
continue OUTER | ||
} | ||
} | ||
|
||
return option | ||
} | ||
} | ||
|
||
func (iter *DistinctPropertyIterator) Reset() { | ||
iter.source.Reset() | ||
|
||
for _, ps := range iter.jobPropertySets { | ||
ps.PopulateProposed() | ||
} | ||
|
||
for _, sets := range iter.groupPropertySets { | ||
for _, ps := range sets { | ||
ps.PopulateProposed() | ||
} | ||
} | ||
} | ||
|
||
// ConstraintChecker is a FeasibilityChecker which returns nodes that match a | ||
// given set of constraints. This is used to filter on job, task group, and task | ||
// constraints. | ||
|
@@ -327,7 +429,7 @@ func resolveConstraintTarget(target string, node *structs.Node) (interface{}, bo | |
func checkConstraint(ctx Context, operand string, lVal, rVal interface{}) bool { | ||
// Check for constraints not handled by this checker. | ||
switch operand { | ||
case structs.ConstraintDistinctHosts: | ||
case structs.ConstraintDistinctHosts, structs.ConstraintDistinctProperty: | ||
return true | ||
default: | ||
break | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not do this inside
SetTaskGroup
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just realizes
SetTFConstraint
populates, so doing this as lazily as possible seems preferable.