Feedback addressed

scheduler/feasible.go

@@ -268,6 +268,19 @@ func NewDistinctPropertyIterator(ctx Context, source FeasibleIterator) *Distinct
 func (iter *DistinctPropertyIterator) SetTaskGroup(tg *structs.TaskGroup) {
 	iter.tg = tg
 
+	// Build the property set at the taskgroup level
+	if _, ok := iter.groupPropertySets[tg.Name]; !ok {
+		for _, c := range tg.Constraints {
+			if c.Operand != structs.ConstraintDistinctProperty {
+				continue
+			}
+
+			pset := NewPropertySet(iter.ctx, iter.job)
+			pset.SetTGConstraint(c, tg.Name)
+			iter.groupPropertySets[tg.Name] = append(iter.groupPropertySets[tg.Name], pset)
+		}
+	}
+
 	// Check if there is a distinct property
 	iter.hasDistinctPropertyConstraints = len(iter.jobPropertySets) != 0 || len(iter.groupPropertySets[tg.Name]) != 0
 }
@@ -275,7 +288,7 @@ func (iter *DistinctPropertyIterator) SetTaskGroup(tg *structs.TaskGroup) {
 func (iter *DistinctPropertyIterator) SetJob(job *structs.Job) {
 	iter.job = job
 
-	// Build the property sets
+	// Build the property set at the job level
 	for _, c := range job.Constraints {
 		if c.Operand != structs.ConstraintDistinctProperty {
 			continue
@@ -285,22 +298,9 @@ func (iter *DistinctPropertyIterator) SetJob(job *structs.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()
@@ -311,24 +311,28 @@ OUTER:
 		}
 
 		// Check if the constraints are met
-		for _, ps := range iter.jobPropertySets {
-			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
-			}
+		if !iter.satisfiesProperties(option, iter.jobPropertySets) ||
+			!iter.satisfiesProperties(option, iter.groupPropertySets[iter.tg.Name]) {
+			continue
 		}
 
 		return option
 	}
 }
 
+// satisfiesProperties returns whether the option satisfies the set of
+// properties. If not it will be filtered.
+func (iter *DistinctPropertyIterator) satisfiesProperties(option *structs.Node, set []*propertySet) bool {
+	for _, ps := range set {
+		if satisfies, reason := ps.SatisfiesDistinctProperties(option, iter.tg.Name); !satisfies {
+			iter.ctx.Metrics().FilterNode(option, reason)
+			return false
+		}
+	}
+
+	return true
+}
+
 func (iter *DistinctPropertyIterator) Reset() {
 	iter.source.Reset()
 

scheduler/propertyset.go

@@ -94,14 +94,8 @@ func (p *propertySet) populateExisting(constraint *structs.Constraint) {
 		return
 	}
 
-	for _, alloc := range allocs {
-		nProperty, ok := p.getProperty(nodes[alloc.NodeID], constraint.LTarget)
-		if !ok {
-			continue
-		}
-
-		p.existingValues[nProperty] = struct{}{}
-	}
+	// Build existing properties map
+	p.populateProperties(allocs, nodes, p.existingValues)
 }
 
 // PopulateProposed populates the proposed values and recomputes any cleared
@@ -139,26 +133,14 @@ func (p *propertySet) PopulateProposed() {
 	}
 
 	// Populate the cleared values
-	for _, alloc := range stopping {
-		nProperty, ok := p.getProperty(nodes[alloc.NodeID], p.constraint.LTarget)
-		if !ok {
-			continue
-		}
-
-		p.clearedValues[nProperty] = struct{}{}
-	}
+	p.populateProperties(stopping, nodes, p.clearedValues)
 
 	// Populate the proposed values
-	for _, alloc := range proposed {
-		nProperty, ok := p.getProperty(nodes[alloc.NodeID], p.constraint.LTarget)
-		if !ok {
-			continue
-		}
+	p.populateProperties(proposed, nodes, p.proposedValues)
 
-		p.proposedValues[nProperty] = struct{}{}
-
-		// If it was cleared, it is now being used
-		delete(p.clearedValues, nProperty)
+	// Remove any cleared value that is now being used by the proposed allocs
+	for value := range p.proposedValues {
+		delete(p.clearedValues, value)
 	}
 }
 
@@ -173,7 +155,7 @@ func (p *propertySet) SatisfiesDistinctProperties(option *structs.Node, tg strin
 	}
 
 	// Get the nodes property value
-	nValue, ok := p.getProperty(option, p.constraint.LTarget)
+	nValue, ok := getProperty(option, p.constraint.LTarget)
 	if !ok {
 		return false, fmt.Sprintf("missing property %q", p.constraint.LTarget)
 	}
@@ -246,8 +228,23 @@ func (p *propertySet) buildNodeMap(allocs []*structs.Allocation) (map[string]*st
 	return nodes, nil
 }
 
+// populateProperties goes through all allocations and builds up the used
+// properties from the nodes storing the results in the passed properties map.
+func (p *propertySet) populateProperties(allocs []*structs.Allocation, nodes map[string]*structs.Node,
+	properties map[string]struct{}) {
+
+	for _, alloc := range allocs {
+		nProperty, ok := getProperty(nodes[alloc.NodeID], p.constraint.LTarget)
+		if !ok {
+			continue
+		}
+
+		properties[nProperty] = struct{}{}
+	}
+}
+
 // getProperty is used to lookup the property value on the node
-func (p *propertySet) getProperty(n *structs.Node, property string) (string, bool) {
+func getProperty(n *structs.Node, property string) (string, bool) {
 	if n == nil || property == "" {
 		return "", false
 	}