From 1f54b68223f0036882f82b969ef26bb46656229e Mon Sep 17 00:00:00 2001 From: Alex Dadgar Date: Mon, 21 Aug 2017 12:41:19 -0700 Subject: [PATCH 1/2] Placing allocs counts towards placement limit This PR makes placing new allocations count towards the limit. We do not restrict how many new placements are made by the limit but we still count towards the limit. This has the nice affect that if you have a group with count = 5 and max_parallel = 1 but only 3 allocs exist for it and a change is made, you will create 2 more at the new version but not destroy one, taking you down to two running as you would have previously. Fixes https://github.com/hashicorp/nomad/issues/3053 --- scheduler/reconcile.go | 3 ++ scheduler/reconcile_test.go | 62 ++++++++++++++++++++++++++++++++----- 2 files changed, 57 insertions(+), 8 deletions(-) diff --git a/scheduler/reconcile.go b/scheduler/reconcile.go index dae2bb715f1..d4468615e1f 100644 --- a/scheduler/reconcile.go +++ b/scheduler/reconcile.go @@ -374,6 +374,9 @@ func (a *allocReconciler) computeGroup(group string, all allocSet) bool { for _, p := range place { a.result.place = append(a.result.place, p) } + + min := helper.IntMin(len(place), limit) + limit -= min } else if !deploymentPlaceReady && len(lost) != 0 { // We are in a situation where we shouldn't be placing more than we need // to but we have lost allocations. It is a very weird user experience diff --git a/scheduler/reconcile_test.go b/scheduler/reconcile_test.go index 282cdfadea9..bcf2d356b0c 100644 --- a/scheduler/reconcile_test.go +++ b/scheduler/reconcile_test.go @@ -69,6 +69,7 @@ Update stanza Tests: √ Finished deployment gets marked as complete √ The stagger is correctly calculated when it is applied across multiple task groups. √ Change job change while scaling up +√ Update the job when all allocations from the previous job haven't been placed yet. */ var ( @@ -2468,9 +2469,9 @@ func TestReconciler_TaintedNode_RollingUpgrade(t *testing.T) { PlacedAllocs: 7, } - // Create 3 allocations from the old job + // Create 2 allocations from the old job var allocs []*structs.Allocation - for i := 7; i < 10; i++ { + for i := 8; i < 10; i++ { alloc := mock.Alloc() alloc.Job = job alloc.JobID = job.ID @@ -2482,7 +2483,7 @@ func TestReconciler_TaintedNode_RollingUpgrade(t *testing.T) { // Create the healthy replacements handled := make(map[string]allocUpdateType) - for i := 0; i < 7; i++ { + for i := 0; i < 8; i++ { new := mock.Alloc() new.Job = job new.JobID = job.ID @@ -2501,7 +2502,7 @@ func TestReconciler_TaintedNode_RollingUpgrade(t *testing.T) { tainted := make(map[string]*structs.Node, 3) for i := 0; i < 3; i++ { n := mock.Node() - n.ID = allocs[3+i].NodeID + n.ID = allocs[2+i].NodeID if i == 0 { n.Status = structs.NodeStatusDown } else { @@ -2519,7 +2520,7 @@ func TestReconciler_TaintedNode_RollingUpgrade(t *testing.T) { createDeployment: nil, deploymentUpdates: nil, place: 2, - destructive: 3, + destructive: 2, stop: 2, followupEvalWait: 31 * time.Second, desiredTGUpdates: map[string]*structs.DesiredUpdates{ @@ -2527,13 +2528,13 @@ func TestReconciler_TaintedNode_RollingUpgrade(t *testing.T) { Place: 1, // Place the lost Stop: 1, // Stop the lost Migrate: 1, // Migrate the tainted - DestructiveUpdate: 3, - Ignore: 5, + DestructiveUpdate: 2, + Ignore: 6, }, }, }) - assertNamesHaveIndexes(t, intRange(7, 9), destructiveResultsToNames(r.destructiveUpdate)) + assertNamesHaveIndexes(t, intRange(8, 9), destructiveResultsToNames(r.destructiveUpdate)) assertNamesHaveIndexes(t, intRange(0, 1), placeResultsToNames(r.place)) assertNamesHaveIndexes(t, intRange(0, 1), stopResultsToNames(r.stop)) } @@ -3012,3 +3013,48 @@ func TestReconciler_JobChange_ScaleUp_SecondEval(t *testing.T) { }, }) } + +// Tests the reconciler doesn't stop allocations when doing a rolling upgrade +// where the count of the old job allocs is < desired count. +func TestReconciler_RollingUpgrade_MissingAllocs(t *testing.T) { + job := mock.Job() + job.TaskGroups[0].Update = noCanaryUpdate + + // Create 7 allocations from the old job + var allocs []*structs.Allocation + for i := 0; i < 7; i++ { + alloc := mock.Alloc() + alloc.Job = job + alloc.JobID = job.ID + alloc.NodeID = structs.GenerateUUID() + alloc.Name = structs.AllocName(job.ID, job.TaskGroups[0].Name, uint(i)) + alloc.TaskGroup = job.TaskGroups[0].Name + allocs = append(allocs, alloc) + } + + reconciler := NewAllocReconciler(testLogger(), allocUpdateFnDestructive, false, job.ID, job, nil, allocs, nil) + r := reconciler.Compute() + + d := structs.NewDeployment(job) + d.TaskGroups[job.TaskGroups[0].Name] = &structs.DeploymentState{ + DesiredTotal: 10, + } + + // Assert the correct results + assertResults(t, r, &resultExpectation{ + createDeployment: d, + deploymentUpdates: nil, + place: 3, + destructive: 1, + desiredTGUpdates: map[string]*structs.DesiredUpdates{ + job.TaskGroups[0].Name: { + Place: 3, + DestructiveUpdate: 1, + Ignore: 6, + }, + }, + }) + + assertNamesHaveIndexes(t, intRange(7, 9), placeResultsToNames(r.place)) + assertNamesHaveIndexes(t, intRange(0, 0), destructiveResultsToNames(r.destructiveUpdate)) +} From b6ed801ce0350d6fb2e20c2dffb60253ca782b42 Mon Sep 17 00:00:00 2001 From: Alex Dadgar Date: Mon, 21 Aug 2017 14:07:54 -0700 Subject: [PATCH 2/2] fix test --- scheduler/generic_sched_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/scheduler/generic_sched_test.go b/scheduler/generic_sched_test.go index c6f2aeea996..aefe8695af4 100644 --- a/scheduler/generic_sched_test.go +++ b/scheduler/generic_sched_test.go @@ -1596,7 +1596,7 @@ func TestServiceSched_JobModify_Rolling_FullNode(t *testing.T) { job2 := job.Copy() job2.TaskGroups[0].Count = 5 job2.TaskGroups[0].Update = &structs.UpdateStrategy{ - MaxParallel: 1, + MaxParallel: 5, HealthCheck: structs.UpdateStrategyHealthCheck_Checks, MinHealthyTime: 10 * time.Second, HealthyDeadline: 10 * time.Minute, @@ -1607,7 +1607,6 @@ func TestServiceSched_JobModify_Rolling_FullNode(t *testing.T) { job2.TaskGroups[0].Tasks[0].Config["command"] = "/bin/other" noErr(t, h.State.UpsertJob(h.NextIndex(), job2)) - // Create a mock evaluation to deal with drain eval := &structs.Evaluation{ ID: structs.GenerateUUID(), Priority: 50,