scheduler: perform feasibility checks for system canaries before computing placements

Canaries for system jobs are placed on a tg.update.canary percent of
eligible nodes. Some of these nodes may not be feasible, and until now
we removed infeasible nodes during placement computation. However, if it
happens to be that the first eligible node we picked to place a canary
on is infeasible, this will lead to the scheduler halting deployment.

The solution presented here simplifies canary deployments: initially,
system jobs that use canary updates get allocations placed on all
eligible nodes, but before we start computing actual placements, a
method called `evictCanaries` is called (much like `evictAndPlace` is
for honoring MaxParallel), and performs a feasibility check on each node
up to the amount of required canaries per task group. Feasibility checks
are expensive, but this way we only check all the nodes in the worst
case scenario (with canary=100), otherwise we stop checks once we know
we are ready to place enough canaries.

Author: pkazmierczak

scheduler/reconciler/reconcile_node.go

@@ -5,8 +5,6 @@ package reconciler
 
 import (
 	"fmt"
-	"maps"
-	"math"
 	"slices"
 	"time"
 
@@ -61,19 +59,14 @@ func (nr *NodeReconciler) Compute(
 	// Create the required task groups.
 	required := materializeSystemTaskGroups(job)
 
-	// Canary deployments deploy to the TaskGroup.UpdateStrategy.Canary
-	// percentage of eligible nodes, so we create a mapping of task group name
-	// to a list of nodes that canaries should be placed on.
-	canaryNodes, canariesPerTG := nr.computeCanaryNodes(required, nodeAllocs, terminal, eligibleNodes)
-
 	compatHadExistingDeployment := nr.DeploymentCurrent != nil
 
 	result := new(NodeReconcileResult)
 	var deploymentComplete bool
 	for nodeID, allocs := range nodeAllocs {
 		diff, deploymentCompleteForNode := nr.computeForNode(job, nodeID, eligibleNodes,
-			notReadyNodes, taintedNodes, canaryNodes[nodeID], canariesPerTG, required,
-			allocs, terminal, serverSupportsDisconnectedClients)
+			notReadyNodes, taintedNodes, required, allocs, terminal,
+			serverSupportsDisconnectedClients)
 		result.Append(diff)
 
 		deploymentComplete = deploymentCompleteForNode
@@ -95,88 +88,6 @@ func (nr *NodeReconciler) Compute(
 	return result
 }
 
-// computeCanaryNodes is a helper function that, given required task groups,
-// mappings of nodes to their live allocs and terminal allocs, and a map of
-// eligible nodes, outputs a map[nodeID] -> map[TG] -> bool which indicates
-// which TGs this node is a canary for, and a map[TG] -> int to indicate how
-// many total canaries are to be placed for a TG.
-func (nr *NodeReconciler) computeCanaryNodes(required map[string]*structs.TaskGroup,
-	liveAllocs map[string][]*structs.Allocation, terminalAllocs structs.TerminalByNodeByName,
-	eligibleNodes map[string]*structs.Node) (map[string]map[string]bool, map[string]int) {
-
-	canaryNodes := map[string]map[string]bool{}
-	eligibleNodesList := slices.Collect(maps.Values(eligibleNodes))
-	canariesPerTG := map[string]int{}
-
-	for _, tg := range required {
-		if tg.Update.IsEmpty() || tg.Update.Canary == 0 {
-			continue
-		}
-
-		// round up to the nearest integer
-		numberOfCanaryNodes := int(math.Ceil(float64(tg.Update.Canary) * float64(len(eligibleNodes)) / 100))
-		canariesPerTG[tg.Name] = numberOfCanaryNodes
-
-		// check if there are any live allocations on any nodes that are/were
-		// canaries.
-		for nodeID, allocs := range liveAllocs {
-			for _, a := range allocs {
-				eligibleNodesList, numberOfCanaryNodes = nr.findOldCanaryNodes(
-					eligibleNodesList, numberOfCanaryNodes, a, tg, canaryNodes, nodeID)
-			}
-		}
-
-		// check if there are any terminal allocations that were canaries
-		for nodeID, terminalAlloc := range terminalAllocs {
-			for _, a := range terminalAlloc {
-				eligibleNodesList, numberOfCanaryNodes = nr.findOldCanaryNodes(
-					eligibleNodesList, numberOfCanaryNodes, a, tg, canaryNodes, nodeID)
-			}
-		}
-
-		for i, n := range eligibleNodesList {
-			if i > numberOfCanaryNodes-1 {
-				break
-			}
-
-			if _, ok := canaryNodes[n.ID]; !ok {
-				canaryNodes[n.ID] = map[string]bool{}
-			}
-
-			canaryNodes[n.ID][tg.Name] = true
-		}
-	}
-
-	return canaryNodes, canariesPerTG
-}
-
-func (nr *NodeReconciler) findOldCanaryNodes(nodesList []*structs.Node, numberOfCanaryNodes int,
-	a *structs.Allocation, tg *structs.TaskGroup, canaryNodes map[string]map[string]bool, nodeID string) ([]*structs.Node, int) {
-
-	if a.DeploymentStatus == nil || a.DeploymentStatus.Canary == false ||
-		nr.DeploymentCurrent == nil {
-		return nodesList, numberOfCanaryNodes
-	}
-
-	nodes := nodesList
-	numberOfCanaries := numberOfCanaryNodes
-	if a.TaskGroup == tg.Name {
-		if _, ok := canaryNodes[nodeID]; !ok {
-			canaryNodes[nodeID] = map[string]bool{}
-		}
-		canaryNodes[nodeID][tg.Name] = true
-
-		// this node should no longer be considered when searching
-		// for canary nodes
-		numberOfCanaries -= 1
-		nodes = slices.DeleteFunc(
-			nodes,
-			func(n *structs.Node) bool { return n.ID == nodeID },
-		)
-	}
-	return nodes, numberOfCanaries
-}
-
 // computeForNode is used to do a set difference between the target
 // allocations and the existing allocations for a particular node. This returns
 // 8 sets of results:
@@ -199,8 +110,6 @@ func (nr *NodeReconciler) computeForNode(
 	eligibleNodes map[string]*structs.Node,
 	notReadyNodes map[string]struct{}, // nodes that are not ready, e.g. draining
 	taintedNodes map[string]*structs.Node, // nodes which are down (by node id)
-	canaryNode map[string]bool, // indicates whether this node is a canary node for tg
-	canariesPerTG map[string]int, // indicates how many canary placements we expect per tg
 	required map[string]*structs.TaskGroup, // set of allocations that must exist
 	liveAllocs []*structs.Allocation, // non-terminal allocations that exist
 	terminal structs.TerminalByNodeByName, // latest terminal allocations (by node, id)
@@ -225,9 +134,6 @@ func (nr *NodeReconciler) computeForNode(
 		deploymentFailed = nr.DeploymentCurrent.Status == structs.DeploymentStatusFailed
 	}
 
-	// Track desired total and desired canaries across all loops
-	desiredCanaries := map[string]int{}
-
 	// Track whether we're during a canary update
 	isCanarying := map[string]bool{}
 
@@ -388,17 +294,14 @@ func (nr *NodeReconciler) computeForNode(
 
 		// If the definition is updated we need to update
 		if job.JobModifyIndex != alloc.Job.JobModifyIndex {
-			if canariesPerTG[tg.Name] > 0 && dstate != nil && !dstate.Promoted {
+			if !tg.Update.IsEmpty() && tg.Update.Canary > 0 && dstate != nil && !dstate.Promoted {
 				isCanarying[tg.Name] = true
-				if canaryNode[tg.Name] {
-					result.Update = append(result.Update, AllocTuple{
-						Name:      name,
-						TaskGroup: tg,
-						Alloc:     alloc,
-						Canary:    true,
-					})
-					desiredCanaries[tg.Name] += 1
-				}
+				result.Update = append(result.Update, AllocTuple{
+					Name:      name,
+					TaskGroup: tg,
+					Alloc:     alloc,
+					Canary:    true,
+				})
 			} else {
 				result.Update = append(result.Update, AllocTuple{
 					Name:      name,
@@ -443,10 +346,6 @@ func (nr *NodeReconciler) computeForNode(
 			dstate.DesiredTotal = len(eligibleNodes)
 		}
 
-		if isCanarying[tg.Name] && !dstate.Promoted {
-			dstate.DesiredCanaries = canariesPerTG[tg.Name]
-		}
-
 		// Check for an existing allocation
 		if _, ok := existing[name]; !ok {