scheduler: reconciler should constrain placements to count (#26239)

While working on property testing in #26172 we discovered there are scenarios
where the reconciler will produce more than the expected number of
placements. Testing of those scenarios at the whole-scheduler level shows that
this gets handled correctly downstream of the reconciler, but this makes it
harder to reason about reconciler behavior. Cap the number of placements in the
reconciler.

Ref: #26172

Author: tgross

scheduler/reconciler/reconcile_cluster.go

@@ -110,15 +110,17 @@ type ReconcileResults struct {
 	Stop []AllocStopResult
 
 	// AttributeUpdates are updates to the allocation that are not from a
-	// jobspec change.
+	// jobspec change. map of alloc ID -> *Allocation
 	AttributeUpdates map[string]*structs.Allocation
 
 	// DisconnectUpdates is the set of allocations are on disconnected nodes, but
 	// have not yet had their ClientStatus set to AllocClientStatusUnknown.
+	// map of alloc ID -> *Allocation
 	DisconnectUpdates map[string]*structs.Allocation
 
 	// ReconnectUpdates is the set of allocations that have ClientStatus set to
 	// AllocClientStatusUnknown, but the associated Node has reconnected.
+	// map of alloc ID -> *Allocation
 	ReconnectUpdates map[string]*structs.Allocation
 
 	// DesiredTGUpdates captures the desired set of changes to make for each
@@ -634,6 +636,12 @@ func (a *AllocReconciler) computeGroup(group string, all allocSet) (*ReconcileRe
 		result.Deployment = a.jobState.DeploymentCurrent
 	}
 
+	// We can never have more placements than the count
+	if len(result.Place) > tg.Count {
+		result.Place = result.Place[:tg.Count]
+		result.DesiredTGUpdates[tg.Name].Place = uint64(tg.Count)
+	}
+
 	deploymentComplete := a.isDeploymentComplete(group, destructive, inplace,
 		migrate, rescheduleNow, result.Place, rescheduleLater, requiresCanaries)
 
@@ -1123,8 +1131,8 @@ func (a *AllocReconciler) computeStop(group *structs.TaskGroup, nameIndex *Alloc
 
 	// Hot path the nothing to do case
 	//
-	// Note that this path can result in duplication allocation indexes in a
-	// scenario with a destructive job change (ex. image update) happens with
+	// Note that this path can result in duplicated allocation indexes in a
+	// scenario where a destructive job change (ex. image update) happens with
 	// an increased group count. Once the canary is replaced, and we compute
 	// the next set of stops, the untainted set equals the new group count,
 	// which results is missing one removal. The duplicate alloc index is

scheduler/reconciler/reconcile_cluster_test.go

@@ -4048,7 +4048,7 @@ func TestReconciler_NewCanaries_CountGreater(t *testing.T) {
 
 	// Create 3 allocations from the old job
 	var allocs []*structs.Allocation
-	for i := 0; i < 3; i++ {
+	for i := range 3 {
 		alloc := mock.Alloc()
 		alloc.Job = job
 		alloc.JobID = job.ID
@@ -4087,18 +4087,18 @@ func TestReconciler_NewCanaries_CountGreater(t *testing.T) {
 	assertResults(t, r, &resultExpectation{
 		createDeployment:  newD,
 		deploymentUpdates: nil,
-		place:             7,
+		place:             3,
 		inplace:           0,
 		stop:              0,
 		desiredTGUpdates: map[string]*structs.DesiredUpdates{
 			job.TaskGroups[0].Name: {
 				Canary: 7,
 				Ignore: 3,
+				Place:  3,
 			},
 		},
 	})
-
-	assertNamesHaveIndexes(t, intRange(0, 2, 3, 6), placeResultsToNames(r.Place))
+	assertNamesHaveIndexes(t, []int{0, 1, 2}, placeResultsToNames(r.Place))
 }
 
 // Tests the reconciler creates new canaries when the job changes for multiple