Merge pull request #218 from abays/fix_bmh_label_filtering

Fix assignment of BMHs for individual compute bmhLabelSelectors

api/v1beta1/openstackbaremetalset.go

@@ -3,6 +3,7 @@ package v1beta1
 import (
 	"context"
 	"fmt"
+	"sort"
 	"strings"
 
 	"github.com/go-logr/logr"
@@ -13,7 +14,8 @@ import (
 
 const (
 	// ServiceName -
-	ServiceName = "openstackbaremetalset"
+	ServiceName                    = "openstackbaremetalset"
+	IndividualComputeLabelMismatch = "one or more computes did not match the available Baremetalhosts due to their bmhLabelSelector(s)"
 )
 
 // GetBaremetalHosts - Get all BaremetalHosts in the chosen namespace with (optional) labels
@@ -91,11 +93,25 @@ func VerifyBaremetalSetScaleUp(
 	instance *OpenStackBaremetalSet,
 	allBmhs *metal3v1.BareMetalHostList,
 	existingBmhs *metal3v1.BareMetalHostList) (map[string]metal3v1.BareMetalHost, error) {
+
+	// Figure out which compute hosts are new
+	newComputes := map[string]InstanceSpec{}
+
+	for hostName, compute := range instance.Spec.BaremetalHosts {
+		// Any host name not found in the instance's status' BaremetalHosts map
+		// is a new compute
+		if _, found := instance.Status.BaremetalHosts[hostName]; !found {
+			newComputes[hostName] = compute
+		}
+	}
+
 	// How many new BaremetalHost allocations do we need (if any)?
-	newBmhsNeededCount := len(instance.Spec.BaremetalHosts) - len(existingBmhs.Items)
-	selectedBaremetalHosts := map[string]metal3v1.BareMetalHost{}
+	newBmhsNeededCount := len(newComputes)
+	availableBaremetalHosts := []metal3v1.BareMetalHost{}
+	var selectedBaremetalHosts map[string]metal3v1.BareMetalHost
 
 	labelStr := ""
+	errIndividualLabelsStr := ""
 
 	if newBmhsNeededCount > 0 {
 		if len(instance.Spec.BmhLabelSelector) > 0 {
@@ -106,18 +122,9 @@ func VerifyBaremetalSetScaleUp(
 		l.Info("Attempting to find BaremetalHosts for scale-up of OpenStackBaremetalSet", "OpenStackBaremetalSet",
 			instance.Name, "namespace", instance.Spec.BmhNamespace, "quantity", newBmhsNeededCount, "labels", labelStr)
 
-		selectedCount := 0
+		// First find BMHs that match everything WITHOUT considering individual compute host labels
 		for _, baremetalHost := range allBmhs.Items {
-
-			if selectedCount == newBmhsNeededCount {
-				break
-			}
 			mismatch := false
-			hostName, matched := verifyBaremetalSetInstanceLabelMatch(l, instance, &baremetalHost)
-			if !matched {
-				l.Info("BaremetalHost cannot be used as it does not match node labels for", "BMH", baremetalHost.ObjectMeta.Name)
-				mismatch = true
-			}
 
 			if !verifyBaremetalSetHardwareMatch(l, instance, &baremetalHost) {
 				l.Info("BaremetalHost cannot be used because it does not match hardware requirements", "BMH", baremetalHost.ObjectMeta.Name)
@@ -144,12 +151,60 @@ func VerifyBaremetalSetScaleUp(
 				continue
 			}
 
-			l.Info("Available BaremetalHost", "BMH", baremetalHost.ObjectMeta.Name)
+			l.Info("Available BaremetalHost (compute labels not yet processed)", "BMH", baremetalHost.ObjectMeta.Name)
 
-			selectedBaremetalHosts[hostName] = baremetalHost
-			selectedCount++
+			availableBaremetalHosts = append(availableBaremetalHosts, baremetalHost)
+		}
+
+		// We only want to continue to individual compute label matching if we actually have
+		// enough BMHs remaining given the filtering above
+		if len(availableBaremetalHosts) >= newBmhsNeededCount {
+
+			// Now try to fit all the requested new compute hosts into the set of available BMHs
+			// given bmhLabelSelectors on each compute (if any) and the labels on the BMHs
+			//
+			// The problem we are trying to solve can be demonstrated through an example...
+			// Imagine we have 3 available BMHs at this point with the following simplified labels:
+			//
+			// BMH1 (A, B, C)
+			// BMH2 (A, C)
+			// BMH3 (A, B)
+			//
+			// Imagine we have 3 new compute hosts with the following simplified labels:
+			//
+			// COMP1 (A, B)
+			// COMP2 (A, C)
+			// COMP3 (A, B)
+			//
+			// We want to make compute-to-BMH selections that allow all computes to find a BMH,
+			// but imagine if the following valid matches were chosen for the first two computes:
+			//
+			// COMP1 -> BMH3 ((A, B) is a subset of (A, B))
+			// COMP2 -> BMH1 ((A, C) is a subset of (A, B, C))
+			//
+			// Now trying to match COMP3, nothing remaining fits, because the potential satisfactory
+			// matches, BMH1 and BMH3, were consumed by the first two computes already.  We would
+			// have preferred instead that our algorithm made either of these sets of selections:
+			//
+			// COMP1 -> BMH1 ((A, B) is a subset of (A, B, C))
+			// COMP2 -> BMH2 ((A, C) is a subset of (A, C))
+			// COMP3 -> BMH3 ((A, B) is a subset of (A, B))
+			// OR
+			// COMP1 -> BMH3 ((A, B) is a subset of (A, B))
+			// COMP2 -> BMH2 ((A, C) is a subset of (A, C))
+			// COMP3 -> BMH1 ((A, B) is a subset of (A, B, C))
+			//
+			// The function called here accomplishes this (see its definition below for details)...
+
+			selectedBaremetalHosts = findValidBaremetalSetInstanceLabelAssignments(newComputes, availableBaremetalHosts)
+
+			if len(selectedBaremetalHosts) < 1 {
+				l.Info("Unable to match requested new computes to satisfactory set of BaremetalHosts due to labeling")
+				errIndividualLabelsStr = fmt.Sprintf(": %s", IndividualComputeLabelMismatch)
+			}
 		}
 	}
+
 	// If we can't satisfy the new requested BaremetalHost count, explicitly state so
 	if newBmhsNeededCount > len(selectedBaremetalHosts) {
 		errLabelStr := ""
@@ -158,12 +213,13 @@ func VerifyBaremetalSetScaleUp(
 			errLabelStr = fmt.Sprintf(" with labels %s", labelStr)
 		}
 
-		return nil, fmt.Errorf("unable to find %d requested BaremetalHosts%s in namespace %s for scale-up (%d in use, %d available)",
+		return nil, fmt.Errorf("unable to find %d requested BaremetalHosts%s in namespace %s for scale-up (%d in use, %d available)%s",
 			len(instance.Spec.BaremetalHosts),
 			errLabelStr,
 			instance.Spec.BmhNamespace,
 			len(existingBmhs.Items),
-			len(selectedBaremetalHosts))
+			len(availableBaremetalHosts),
+			errIndividualLabelsStr)
 	}
 
 	l.Info("Found sufficient quantity of BaremetalHosts for scale-up of OpenStackBaremetalSet",
@@ -190,19 +246,81 @@ func VerifyBaremetalSetScaleDown(
 	return nil
 }
 
-func verifyBaremetalSetInstanceLabelMatch(
-	l logr.Logger,
-	instance *OpenStackBaremetalSet,
-	bmh *metal3v1.BareMetalHost) (string, bool) {
+// Function to find valid assignments for computes-to-BMHs, given their labels, using backtracking
+func findValidBaremetalSetInstanceLabelAssignments(computes map[string]InstanceSpec, bmhs []metal3v1.BareMetalHost) map[string]metal3v1.BareMetalHost {
+	// First create map of valid computes-to-BMHs possibilities
+	computesToBmhs := map[string][]metal3v1.BareMetalHost{}
+	for compName, comp := range computes {
+		for _, bmh := range bmhs {
+			if IsMapSubset(bmh.GetLabels(), comp.BmhLabelSelector) {
+				computesToBmhs[compName] = append(computesToBmhs[compName], bmh)
+			}
+		}
+	}
+
+	// Now sort computes by the number of valid matches, as sorting helps in
+	// cases with large numbers of computes and/or BMHs, for a more optimal
+	// backtracking
+
+	// Make array of compute host names for use in "backtrack" func
+	computeArray := make([]string, len(computes))
+
+	i := 0
+	// The keys of the "computes" map will be traversed in this for loo[] in random
+	// order, but that does not matter, as we are defining the order of keys in
+	// this array that we are creating and we will adhere to that from here on out
+	for k := range computes {
+		computeArray[i] = k
+		i++
+	}
 
-	bmhLabels := bmh.GetLabels()
-	for hostName, instanceSpec := range instance.Spec.BaremetalHosts {
-		if IsMapSubset(bmhLabels, instanceSpec.BmhLabelSelector) {
-			return hostName, true
+	// Now we can do the actual sorting by number of preliminary matches
+	sort.Slice(computeArray[:], func(i, j int) bool {
+		return len(computesToBmhs[computeArray[i]]) < len(computesToBmhs[computeArray[j]])
+	})
+
+	// Finally create and use a backtracking func to find valid assignments
+	assignedBMHs := map[string]bool{}                 // Keep track of assigned BMHs
+	assignment := map[string]metal3v1.BareMetalHost{} // Store the final assignments
+
+	// The backtracking function that we will use to crawl all potential
+	// compute-to-BMH assignments, using the initial matching map that we
+	// created earlier
+	var backtrack func(index int) bool
+
+	backtrack = func(index int) bool {
+		if index == len(computes) {
+			return true // All computes are assigned
 		}
+
+		comp := computeArray[index]
+		for _, bmh := range computesToBmhs[comp] {
+			if !assignedBMHs[bmh.Name] {
+				// Assign this BMH to the compute host
+				assignment[comp] = bmh
+				assignedBMHs[bmh.Name] = true
+
+				// Recur to assign the next compute
+				if backtrack(index + 1) {
+					return true
+				}
+
+				// If this assignment didn't work, backtrack
+				delete(assignment, comp)
+				assignedBMHs[bmh.Name] = false
+			}
+		}
+		return false
+	}
+
+	// Look for matching BMHs starting with the first compute, and for each matching BMH found, recurse
+	// for the next compute with the consumed BMHs removed from consideration -- backtracking down the
+	// stack whenever matches are needed but exhausted for that particular path
+	if backtrack(0) {
+		return assignment // Return the valid assignments we have chosen
+	} else {
+		return nil // No valid assignments found to satisify all computes' bmhLabelSelectors
 	}
-	l.Info("BaremetalHost does not match any of the node labels as requested", "BMH", bmh.ObjectMeta.Name)
-	return "", false
 }
 
 func IsMapSubset[K, V comparable](m map[K]V, sub map[K]V) bool {