Repository: hashicorp/nomad
File: scheduler/reconciler/reconcile_node.go (Go)

Code before:
		// 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 allocations on any nodes that are/were
		// canaries.
		for nodeID, allocs := range nodeAllocs {
			for _, a := range allocs {
				if a.DeploymentStatus == nil || a.DeploymentStatus.Canary == false {
					continue
				}
				if a.TaskGroup == tg.Name {
					canaryNodes[nodeID] = map[string]bool{}
					canaryNodes[nodeID][tg.Name] = true

					// this node should no longer be considered when searching
					// for canary nodes
					numberOfCanaryNodes -= 1
					eligibleNodesList = slices.DeleteFunc(
						eligibleNodesList,
						func(n *structs.Node) bool { return n.ID == nodeID },
					)
				}
			}
		}

		for i, n := range eligibleNodesList {
			canaryNodes[n.ID] = map[string]bool{}

			if i > numberOfCanaryNodes-1 {
				break
			}

			canaryNodes[n.ID][tg.Name] = true
		}
	}

	return canaryNodes, canariesPerTG
}

// 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:
// 1. the list of named task groups that need to be placed (no existing
// allocation),
// 2. the allocations that need to be updated (job definition is newer),
// 3. allocs that need to be migrated (node is draining),
// 4. allocs that need to be evicted (no longer required),
// 5. those that should be ignored,
// 6. those that are lost that need to be replaced (running on a lost node),
// 7. those that are running on a disconnected node but may resume, and

Review comment (suggestion):
We're overwriting previous task groups, but also we shouldn't populate this map until after the count check. In my tests I was getting an extra canary node with an empty map. That's safe in the current code (because the default value is false), but feels like it'll be brittle against changes. Let's move it here.

```suggestion
			}
			
			if _, ok := canaryNodes[n.ID]; !ok {
				canaryNodes[n.ID] = map[string]bool{}
			}
			
```

Code after:

		// 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 { // TODO: should we add this? || nr.DeploymentCurrent.ID != a.DeploymentID {
		return nodesList, numberOfCanaryNodes
	}

	nodes := nodesList
	numberOfCanaries := numberOfCanaryNodes
	if a.TaskGroup == tg.Name {
		if _, ok := canaryNodes[nodeID]; !ok {
			canaryNodes[nodeID] = map[string]bool{}