From 4b503dd6f9b3d30f0856539566388f61fc5d0c47 Mon Sep 17 00:00:00 2001 From: Preetha Appan Date: Tue, 10 Apr 2018 16:08:37 -0500 Subject: [PATCH 1/4] Simplify and update allocation gc eligibility logic --- nomad/core_sched.go | 32 +++++++++-------- nomad/core_sched_test.go | 75 ++++++++++++++++++---------------------- 2 files changed, 52 insertions(+), 55 deletions(-) diff --git a/nomad/core_sched.go b/nomad/core_sched.go index a3277f858..712df4804 100644 --- a/nomad/core_sched.go +++ b/nomad/core_sched.go @@ -323,7 +323,16 @@ func (c *CoreScheduler) gcEval(eval *structs.Evaluation, thresholdIndex uint64, gcEval := true var gcAllocIDs []string for _, alloc := range allocs { - if !allocGCEligible(alloc, job, time.Now(), thresholdIndex) { + var followupEval *structs.Evaluation + if alloc.FollowupEvalID != "" { + followupEval, err = c.snap.EvalByID(ws, alloc.FollowupEvalID) + if err != nil { + c.srv.logger.Printf("[ERR] sched.core: failed to lookup followup eval %s: %v", + eval.ID, err) + return false, nil, err + } + } + if !allocGCEligible(alloc, job, followupEval, thresholdIndex) { // Can't GC the evaluation since not all of the allocations are // terminal gcEval = false @@ -599,7 +608,7 @@ func (c *CoreScheduler) partitionDeploymentReap(deployments []string) []*structs // allocGCEligible returns if the allocation is eligible to be garbage collected // according to its terminal status and its reschedule trackers -func allocGCEligible(a *structs.Allocation, job *structs.Job, gcTime time.Time, thresholdIndex uint64) bool { +func allocGCEligible(a *structs.Allocation, job *structs.Job, followupEval *structs.Evaluation, thresholdIndex uint64) bool { // Not in a terminal status and old enough if !a.TerminalStatus() || a.ModifyIndex > thresholdIndex { return false @@ -615,24 +624,19 @@ func allocGCEligible(a *structs.Allocation, job *structs.Job, gcTime time.Time, if tg != nil { reschedulePolicy = tg.ReschedulePolicy } - // No reschedule policy or restarts are disabled - if reschedulePolicy == nil || reschedulePolicy.Attempts == 0 || reschedulePolicy.Interval == 0 { + // No reschedule policy or rescheduling is disabled + if reschedulePolicy == nil || (!reschedulePolicy.Unlimited && reschedulePolicy.Attempts == 0) { return true } // Restart tracking information has been carried forward if a.NextAllocation != "" { return true } - // Eligible for restarts but none have been attempted yet - if a.RescheduleTracker == nil || len(a.RescheduleTracker.Events) == 0 { - return false + + // If there's a follow up eval, use its status for determining GC eligibility + if followupEval != nil { + return followupEval.TerminalStatus() } - // Most recent reschedule attempt is within time interval - interval := reschedulePolicy.Interval - lastIndex := len(a.RescheduleTracker.Events) - lastRescheduleEvent := a.RescheduleTracker.Events[lastIndex-1] - timeDiff := gcTime.UTC().UnixNano() - lastRescheduleEvent.RescheduleTime - - return timeDiff > interval.Nanoseconds() + return true } diff --git a/nomad/core_sched_test.go b/nomad/core_sched_test.go index 68ac7dbc8..f39babeec 100644 --- a/nomad/core_sched_test.go +++ b/nomad/core_sched_test.go @@ -1798,10 +1798,10 @@ func TestCoreScheduler_PartitionJobReap(t *testing.T) { func TestAllocation_GCEligible(t *testing.T) { type testCase struct { Desc string - GCTime time.Time ClientStatus string DesiredStatus string JobStatus string + FollowupEvalStatus string JobStop bool ModifyIndex uint64 NextAllocID string @@ -1818,7 +1818,6 @@ func TestAllocation_GCEligible(t *testing.T) { Desc: "GC when non terminal", ClientStatus: structs.AllocClientStatusPending, DesiredStatus: structs.AllocDesiredStatusRun, - GCTime: fail, ModifyIndex: 90, ThresholdIndex: 90, ShouldGC: false, @@ -1828,7 +1827,6 @@ func TestAllocation_GCEligible(t *testing.T) { ClientStatus: structs.AllocClientStatusPending, DesiredStatus: structs.AllocDesiredStatusRun, JobStop: true, - GCTime: fail, ModifyIndex: 90, ThresholdIndex: 90, ShouldGC: false, @@ -1838,7 +1836,6 @@ func TestAllocation_GCEligible(t *testing.T) { ClientStatus: structs.AllocClientStatusPending, DesiredStatus: structs.AllocDesiredStatusRun, JobStatus: structs.JobStatusDead, - GCTime: fail, ModifyIndex: 90, ThresholdIndex: 90, ShouldGC: false, @@ -1847,7 +1844,6 @@ func TestAllocation_GCEligible(t *testing.T) { Desc: "GC when threshold not met", ClientStatus: structs.AllocClientStatusComplete, DesiredStatus: structs.AllocDesiredStatusStop, - GCTime: fail, ModifyIndex: 100, ThresholdIndex: 90, ReschedulePolicy: nil, @@ -1857,40 +1853,38 @@ func TestAllocation_GCEligible(t *testing.T) { Desc: "GC when no reschedule policy", ClientStatus: structs.AllocClientStatusFailed, DesiredStatus: structs.AllocDesiredStatusRun, - GCTime: fail, ReschedulePolicy: nil, ModifyIndex: 90, ThresholdIndex: 90, ShouldGC: true, }, { - Desc: "GC when empty policy", + Desc: "GC with empty reschedule policy", ClientStatus: structs.AllocClientStatusFailed, DesiredStatus: structs.AllocDesiredStatusRun, - GCTime: fail, - ReschedulePolicy: &structs.ReschedulePolicy{Attempts: 0, Interval: 0 * time.Minute}, + ReschedulePolicy: &structs.ReschedulePolicy{Attempts: 0, Interval: 0 * time.Minute, Unlimited: false}, ModifyIndex: 90, ThresholdIndex: 90, ShouldGC: true, }, { - Desc: "GC with no previous attempts", - ClientStatus: structs.AllocClientStatusFailed, - DesiredStatus: structs.AllocDesiredStatusRun, - GCTime: fail, - ModifyIndex: 90, - ThresholdIndex: 90, - ReschedulePolicy: &structs.ReschedulePolicy{Attempts: 1, Interval: 1 * time.Minute}, - ShouldGC: false, + Desc: "GC with pending followup eval", + ClientStatus: structs.AllocClientStatusFailed, + DesiredStatus: structs.AllocDesiredStatusRun, + FollowupEvalStatus: structs.EvalStatusPending, + ModifyIndex: 90, + ThresholdIndex: 90, + ReschedulePolicy: &structs.ReschedulePolicy{Attempts: 1, Interval: 1 * time.Minute}, + ShouldGC: false, }, { - Desc: "GC with prev reschedule attempt within interval", - ClientStatus: structs.AllocClientStatusFailed, - DesiredStatus: structs.AllocDesiredStatusRun, - ReschedulePolicy: &structs.ReschedulePolicy{Attempts: 2, Interval: 30 * time.Minute}, - GCTime: fail, - ModifyIndex: 90, - ThresholdIndex: 90, + Desc: "GC with blocked followup eval", + ClientStatus: structs.AllocClientStatusFailed, + DesiredStatus: structs.AllocDesiredStatusRun, + ReschedulePolicy: &structs.ReschedulePolicy{Attempts: 2, Interval: 30 * time.Minute}, + FollowupEvalStatus: structs.EvalStatusBlocked, + ModifyIndex: 90, + ThresholdIndex: 90, RescheduleTrackers: []*structs.RescheduleEvent{ { RescheduleTime: fail.Add(-5 * time.Minute).UTC().UnixNano(), @@ -1899,11 +1893,11 @@ func TestAllocation_GCEligible(t *testing.T) { ShouldGC: false, }, { - Desc: "GC with prev reschedule attempt outside interval", - ClientStatus: structs.AllocClientStatusFailed, - DesiredStatus: structs.AllocDesiredStatusRun, - GCTime: fail, - ReschedulePolicy: &structs.ReschedulePolicy{Attempts: 5, Interval: 30 * time.Minute}, + Desc: "GC with complete followup eval", + ClientStatus: structs.AllocClientStatusFailed, + DesiredStatus: structs.AllocDesiredStatusRun, + FollowupEvalStatus: structs.EvalStatusComplete, + ReschedulePolicy: &structs.ReschedulePolicy{Attempts: 5, Interval: 30 * time.Minute}, RescheduleTrackers: []*structs.RescheduleEvent{ { RescheduleTime: fail.Add(-45 * time.Minute).UTC().UnixNano(), @@ -1918,21 +1912,14 @@ func TestAllocation_GCEligible(t *testing.T) { Desc: "GC when next alloc id is set", ClientStatus: structs.AllocClientStatusFailed, DesiredStatus: structs.AllocDesiredStatusRun, - GCTime: fail, ReschedulePolicy: &structs.ReschedulePolicy{Attempts: 5, Interval: 30 * time.Minute}, - RescheduleTrackers: []*structs.RescheduleEvent{ - { - RescheduleTime: fail.Add(-3 * time.Minute).UTC().UnixNano(), - }, - }, - NextAllocID: uuid.Generate(), - ShouldGC: true, + NextAllocID: uuid.Generate(), + ShouldGC: true, }, { Desc: "GC when job is stopped", ClientStatus: structs.AllocClientStatusFailed, DesiredStatus: structs.AllocDesiredStatusRun, - GCTime: fail, ReschedulePolicy: &structs.ReschedulePolicy{Attempts: 5, Interval: 30 * time.Minute}, RescheduleTrackers: []*structs.RescheduleEvent{ { @@ -1946,7 +1933,6 @@ func TestAllocation_GCEligible(t *testing.T) { Desc: "GC when job status is dead", ClientStatus: structs.AllocClientStatusFailed, DesiredStatus: structs.AllocDesiredStatusRun, - GCTime: fail, ReschedulePolicy: &structs.ReschedulePolicy{Attempts: 5, Interval: 30 * time.Minute}, RescheduleTrackers: []*structs.RescheduleEvent{ { @@ -1965,6 +1951,13 @@ func TestAllocation_GCEligible(t *testing.T) { alloc.ClientStatus = tc.ClientStatus alloc.RescheduleTracker = &structs.RescheduleTracker{Events: tc.RescheduleTrackers} alloc.NextAllocation = tc.NextAllocID + var followupEval *structs.Evaluation + + if tc.FollowupEvalStatus != "" { + followupEval = mock.Eval() + followupEval.Status = tc.FollowupEvalStatus + } + job := mock.Job() alloc.TaskGroup = job.TaskGroups[0].Name job.TaskGroups[0].ReschedulePolicy = tc.ReschedulePolicy @@ -1974,7 +1967,7 @@ func TestAllocation_GCEligible(t *testing.T) { job.Stop = tc.JobStop t.Run(tc.Desc, func(t *testing.T) { - if got := allocGCEligible(alloc, job, tc.GCTime, tc.ThresholdIndex); got != tc.ShouldGC { + if got := allocGCEligible(alloc, job, followupEval, tc.ThresholdIndex); got != tc.ShouldGC { t.Fatalf("expected %v but got %v", tc.ShouldGC, got) } }) @@ -1985,5 +1978,5 @@ func TestAllocation_GCEligible(t *testing.T) { require := require.New(t) alloc := mock.Alloc() alloc.ClientStatus = structs.AllocClientStatusComplete - require.True(allocGCEligible(alloc, nil, time.Now(), 1000)) + require.True(allocGCEligible(alloc, nil, nil, 1000)) } From a5965d0cb0e8aec2c5d0404b3ed8d93cbcc2189c Mon Sep 17 00:00:00 2001 From: Preetha Appan Date: Tue, 10 Apr 2018 17:12:06 -0500 Subject: [PATCH 2/4] Fix unit test for core scheduler GC --- nomad/core_sched_test.go | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/nomad/core_sched_test.go b/nomad/core_sched_test.go index f39babeec..b6e881d4a 100644 --- a/nomad/core_sched_test.go +++ b/nomad/core_sched_test.go @@ -139,6 +139,12 @@ func TestCoreScheduler_EvalGC_ReschedulingAllocs(t *testing.T) { err = state.UpsertJob(1001, job) require.Nil(err) + // Insert a follow up eval that's complete + followupEval := mock.Eval() + followupEval.JobID = job.ID + followupEval.Status = structs.EvalStatusComplete + err = state.UpsertEvals(1002, []*structs.Evaluation{followupEval}) + // Insert failed alloc with an old reschedule attempt, can be GCed alloc := mock.Alloc() alloc.EvalID = eval.ID @@ -146,6 +152,7 @@ func TestCoreScheduler_EvalGC_ReschedulingAllocs(t *testing.T) { alloc.ClientStatus = structs.AllocClientStatusFailed alloc.JobID = eval.JobID alloc.TaskGroup = job.TaskGroups[0].Name + alloc.FollowupEvalID = followupEval.ID alloc.RescheduleTracker = &structs.RescheduleTracker{ Events: []*structs.RescheduleEvent{ { @@ -156,13 +163,18 @@ func TestCoreScheduler_EvalGC_ReschedulingAllocs(t *testing.T) { }, } - // Insert another failed alloc with a recent reschedule attempt, can't be GCed + // Insert another failed alloc with a follow up eval pending, can't be GCed + followupEval2 := mock.Eval() + followupEval2.JobID = job.ID + err = state.UpsertEvals(1002, []*structs.Evaluation{followupEval2}) + alloc2 := mock.Alloc() alloc2.EvalID = eval.ID alloc2.DesiredStatus = structs.AllocDesiredStatusRun alloc2.ClientStatus = structs.AllocClientStatusLost alloc2.JobID = eval.JobID alloc2.TaskGroup = job.TaskGroups[0].Name + alloc2.FollowupEvalID = followupEval2.ID alloc2.RescheduleTracker = &structs.RescheduleTracker{ Events: []*structs.RescheduleEvent{ { @@ -195,6 +207,7 @@ func TestCoreScheduler_EvalGC_ReschedulingAllocs(t *testing.T) { ws := memdb.NewWatchSet() out, err := state.EvalByID(ws, eval.ID) require.Nil(err) + require.NotNil(out) require.Equal(eval.ID, out.ID) outA, err := state.AllocByID(ws, alloc.ID) From 9cbba2517f45e711a601c5637772c147cccb2adb Mon Sep 17 00:00:00 2001 From: Preetha Appan Date: Wed, 11 Apr 2018 13:58:02 -0500 Subject: [PATCH 3/4] Update alloc GC eligility logic to not rely on follow up evals --- nomad/core_sched.go | 40 +++++++++----- nomad/core_sched_test.go | 116 ++++++++++++++++++++++----------------- 2 files changed, 92 insertions(+), 64 deletions(-) diff --git a/nomad/core_sched.go b/nomad/core_sched.go index 712df4804..5601dad09 100644 --- a/nomad/core_sched.go +++ b/nomad/core_sched.go @@ -323,16 +323,7 @@ func (c *CoreScheduler) gcEval(eval *structs.Evaluation, thresholdIndex uint64, gcEval := true var gcAllocIDs []string for _, alloc := range allocs { - var followupEval *structs.Evaluation - if alloc.FollowupEvalID != "" { - followupEval, err = c.snap.EvalByID(ws, alloc.FollowupEvalID) - if err != nil { - c.srv.logger.Printf("[ERR] sched.core: failed to lookup followup eval %s: %v", - eval.ID, err) - return false, nil, err - } - } - if !allocGCEligible(alloc, job, followupEval, thresholdIndex) { + if !allocGCEligible(alloc, job, time.Now(), thresholdIndex) { // Can't GC the evaluation since not all of the allocations are // terminal gcEval = false @@ -608,16 +599,24 @@ func (c *CoreScheduler) partitionDeploymentReap(deployments []string) []*structs // allocGCEligible returns if the allocation is eligible to be garbage collected // according to its terminal status and its reschedule trackers -func allocGCEligible(a *structs.Allocation, job *structs.Job, followupEval *structs.Evaluation, thresholdIndex uint64) bool { +func allocGCEligible(a *structs.Allocation, job *structs.Job, gcTime time.Time, thresholdIndex uint64) bool { // Not in a terminal status and old enough if !a.TerminalStatus() || a.ModifyIndex > thresholdIndex { return false } + // If the job is deleted, stopped or dead all allocs can be removed if job == nil || job.Stop || job.Status == structs.JobStatusDead { return true } + // If the alloc hasn't failed then we don't need to consider it for rescheduling + // Rescheduling needs to copy over information from the previous alloc so that it + // can enforce the reschedule policy + if a.ClientStatus != structs.AllocClientStatusFailed { + return true + } + var reschedulePolicy *structs.ReschedulePolicy tg := job.LookupTaskGroup(a.TaskGroup) @@ -633,10 +632,21 @@ func allocGCEligible(a *structs.Allocation, job *structs.Job, followupEval *stru return true } - // If there's a follow up eval, use its status for determining GC eligibility - if followupEval != nil { - return followupEval.TerminalStatus() + // This task has unlimited rescheduling and the alloc has not been replaced, so we can't GC it yet + if reschedulePolicy.Unlimited { + return false } - return true + // No restarts have been attempted yet + if a.RescheduleTracker == nil || len(a.RescheduleTracker.Events) == 0 { + return false + } + + // Don't GC if most recent reschedule attempt is within time interval + interval := reschedulePolicy.Interval + lastIndex := len(a.RescheduleTracker.Events) + lastRescheduleEvent := a.RescheduleTracker.Events[lastIndex-1] + timeDiff := gcTime.UTC().UnixNano() - lastRescheduleEvent.RescheduleTime + + return timeDiff > interval.Nanoseconds() } diff --git a/nomad/core_sched_test.go b/nomad/core_sched_test.go index b6e881d4a..0cb72dc0b 100644 --- a/nomad/core_sched_test.go +++ b/nomad/core_sched_test.go @@ -139,12 +139,6 @@ func TestCoreScheduler_EvalGC_ReschedulingAllocs(t *testing.T) { err = state.UpsertJob(1001, job) require.Nil(err) - // Insert a follow up eval that's complete - followupEval := mock.Eval() - followupEval.JobID = job.ID - followupEval.Status = structs.EvalStatusComplete - err = state.UpsertEvals(1002, []*structs.Evaluation{followupEval}) - // Insert failed alloc with an old reschedule attempt, can be GCed alloc := mock.Alloc() alloc.EvalID = eval.ID @@ -152,7 +146,7 @@ func TestCoreScheduler_EvalGC_ReschedulingAllocs(t *testing.T) { alloc.ClientStatus = structs.AllocClientStatusFailed alloc.JobID = eval.JobID alloc.TaskGroup = job.TaskGroups[0].Name - alloc.FollowupEvalID = followupEval.ID + alloc.NextAllocation = uuid.Generate() alloc.RescheduleTracker = &structs.RescheduleTracker{ Events: []*structs.RescheduleEvent{ { @@ -163,18 +157,12 @@ func TestCoreScheduler_EvalGC_ReschedulingAllocs(t *testing.T) { }, } - // Insert another failed alloc with a follow up eval pending, can't be GCed - followupEval2 := mock.Eval() - followupEval2.JobID = job.ID - err = state.UpsertEvals(1002, []*structs.Evaluation{followupEval2}) - alloc2 := mock.Alloc() alloc2.EvalID = eval.ID alloc2.DesiredStatus = structs.AllocDesiredStatusRun - alloc2.ClientStatus = structs.AllocClientStatusLost + alloc2.ClientStatus = structs.AllocClientStatusFailed alloc2.JobID = eval.JobID alloc2.TaskGroup = job.TaskGroups[0].Name - alloc2.FollowupEvalID = followupEval2.ID alloc2.RescheduleTracker = &structs.RescheduleTracker{ Events: []*structs.RescheduleEvent{ { @@ -1811,10 +1799,10 @@ func TestCoreScheduler_PartitionJobReap(t *testing.T) { func TestAllocation_GCEligible(t *testing.T) { type testCase struct { Desc string + GCTime time.Time ClientStatus string DesiredStatus string JobStatus string - FollowupEvalStatus string JobStop bool ModifyIndex uint64 NextAllocID string @@ -1831,6 +1819,7 @@ func TestAllocation_GCEligible(t *testing.T) { Desc: "GC when non terminal", ClientStatus: structs.AllocClientStatusPending, DesiredStatus: structs.AllocDesiredStatusRun, + GCTime: fail, ModifyIndex: 90, ThresholdIndex: 90, ShouldGC: false, @@ -1840,6 +1829,7 @@ func TestAllocation_GCEligible(t *testing.T) { ClientStatus: structs.AllocClientStatusPending, DesiredStatus: structs.AllocDesiredStatusRun, JobStop: true, + GCTime: fail, ModifyIndex: 90, ThresholdIndex: 90, ShouldGC: false, @@ -1849,14 +1839,26 @@ func TestAllocation_GCEligible(t *testing.T) { ClientStatus: structs.AllocClientStatusPending, DesiredStatus: structs.AllocDesiredStatusRun, JobStatus: structs.JobStatusDead, + GCTime: fail, ModifyIndex: 90, ThresholdIndex: 90, ShouldGC: false, }, + { + Desc: "GC when terminal but not failed ", + ClientStatus: structs.AllocClientStatusComplete, + DesiredStatus: structs.AllocDesiredStatusRun, + GCTime: fail, + ModifyIndex: 100, + ThresholdIndex: 90, + ReschedulePolicy: nil, + ShouldGC: false, + }, { Desc: "GC when threshold not met", ClientStatus: structs.AllocClientStatusComplete, DesiredStatus: structs.AllocDesiredStatusStop, + GCTime: fail, ModifyIndex: 100, ThresholdIndex: 90, ReschedulePolicy: nil, @@ -1866,38 +1868,40 @@ func TestAllocation_GCEligible(t *testing.T) { Desc: "GC when no reschedule policy", ClientStatus: structs.AllocClientStatusFailed, DesiredStatus: structs.AllocDesiredStatusRun, + GCTime: fail, ReschedulePolicy: nil, ModifyIndex: 90, ThresholdIndex: 90, ShouldGC: true, }, { - Desc: "GC with empty reschedule policy", + Desc: "GC when empty policy", ClientStatus: structs.AllocClientStatusFailed, DesiredStatus: structs.AllocDesiredStatusRun, - ReschedulePolicy: &structs.ReschedulePolicy{Attempts: 0, Interval: 0 * time.Minute, Unlimited: false}, + GCTime: fail, + ReschedulePolicy: &structs.ReschedulePolicy{Attempts: 0, Interval: 0 * time.Minute}, ModifyIndex: 90, ThresholdIndex: 90, ShouldGC: true, }, { - Desc: "GC with pending followup eval", - ClientStatus: structs.AllocClientStatusFailed, - DesiredStatus: structs.AllocDesiredStatusRun, - FollowupEvalStatus: structs.EvalStatusPending, - ModifyIndex: 90, - ThresholdIndex: 90, - ReschedulePolicy: &structs.ReschedulePolicy{Attempts: 1, Interval: 1 * time.Minute}, - ShouldGC: false, + Desc: "GC with no previous attempts", + ClientStatus: structs.AllocClientStatusFailed, + DesiredStatus: structs.AllocDesiredStatusRun, + GCTime: fail, + ModifyIndex: 90, + ThresholdIndex: 90, + ReschedulePolicy: &structs.ReschedulePolicy{Attempts: 1, Interval: 1 * time.Minute}, + ShouldGC: false, }, { - Desc: "GC with blocked followup eval", - ClientStatus: structs.AllocClientStatusFailed, - DesiredStatus: structs.AllocDesiredStatusRun, - ReschedulePolicy: &structs.ReschedulePolicy{Attempts: 2, Interval: 30 * time.Minute}, - FollowupEvalStatus: structs.EvalStatusBlocked, - ModifyIndex: 90, - ThresholdIndex: 90, + Desc: "GC with prev reschedule attempt within interval", + ClientStatus: structs.AllocClientStatusFailed, + DesiredStatus: structs.AllocDesiredStatusRun, + ReschedulePolicy: &structs.ReschedulePolicy{Attempts: 2, Interval: 30 * time.Minute}, + GCTime: fail, + ModifyIndex: 90, + ThresholdIndex: 90, RescheduleTrackers: []*structs.RescheduleEvent{ { RescheduleTime: fail.Add(-5 * time.Minute).UTC().UnixNano(), @@ -1906,11 +1910,11 @@ func TestAllocation_GCEligible(t *testing.T) { ShouldGC: false, }, { - Desc: "GC with complete followup eval", - ClientStatus: structs.AllocClientStatusFailed, - DesiredStatus: structs.AllocDesiredStatusRun, - FollowupEvalStatus: structs.EvalStatusComplete, - ReschedulePolicy: &structs.ReschedulePolicy{Attempts: 5, Interval: 30 * time.Minute}, + Desc: "GC with prev reschedule attempt outside interval", + ClientStatus: structs.AllocClientStatusFailed, + DesiredStatus: structs.AllocDesiredStatusRun, + GCTime: fail, + ReschedulePolicy: &structs.ReschedulePolicy{Attempts: 5, Interval: 30 * time.Minute}, RescheduleTrackers: []*structs.RescheduleEvent{ { RescheduleTime: fail.Add(-45 * time.Minute).UTC().UnixNano(), @@ -1925,14 +1929,34 @@ func TestAllocation_GCEligible(t *testing.T) { Desc: "GC when next alloc id is set", ClientStatus: structs.AllocClientStatusFailed, DesiredStatus: structs.AllocDesiredStatusRun, + GCTime: fail, ReschedulePolicy: &structs.ReschedulePolicy{Attempts: 5, Interval: 30 * time.Minute}, - NextAllocID: uuid.Generate(), - ShouldGC: true, + RescheduleTrackers: []*structs.RescheduleEvent{ + { + RescheduleTime: fail.Add(-3 * time.Minute).UTC().UnixNano(), + }, + }, + NextAllocID: uuid.Generate(), + ShouldGC: true, + }, + { + Desc: "GC when next alloc id is not set and unlimited restarts", + ClientStatus: structs.AllocClientStatusFailed, + DesiredStatus: structs.AllocDesiredStatusRun, + GCTime: fail, + ReschedulePolicy: &structs.ReschedulePolicy{Unlimited: true, Delay: 5 * time.Second, DelayFunction: "constant"}, + RescheduleTrackers: []*structs.RescheduleEvent{ + { + RescheduleTime: fail.Add(-3 * time.Minute).UTC().UnixNano(), + }, + }, + ShouldGC: false, }, { Desc: "GC when job is stopped", ClientStatus: structs.AllocClientStatusFailed, DesiredStatus: structs.AllocDesiredStatusRun, + GCTime: fail, ReschedulePolicy: &structs.ReschedulePolicy{Attempts: 5, Interval: 30 * time.Minute}, RescheduleTrackers: []*structs.RescheduleEvent{ { @@ -1946,6 +1970,7 @@ func TestAllocation_GCEligible(t *testing.T) { Desc: "GC when job status is dead", ClientStatus: structs.AllocClientStatusFailed, DesiredStatus: structs.AllocDesiredStatusRun, + GCTime: fail, ReschedulePolicy: &structs.ReschedulePolicy{Attempts: 5, Interval: 30 * time.Minute}, RescheduleTrackers: []*structs.RescheduleEvent{ { @@ -1964,13 +1989,6 @@ func TestAllocation_GCEligible(t *testing.T) { alloc.ClientStatus = tc.ClientStatus alloc.RescheduleTracker = &structs.RescheduleTracker{Events: tc.RescheduleTrackers} alloc.NextAllocation = tc.NextAllocID - var followupEval *structs.Evaluation - - if tc.FollowupEvalStatus != "" { - followupEval = mock.Eval() - followupEval.Status = tc.FollowupEvalStatus - } - job := mock.Job() alloc.TaskGroup = job.TaskGroups[0].Name job.TaskGroups[0].ReschedulePolicy = tc.ReschedulePolicy @@ -1980,10 +1998,10 @@ func TestAllocation_GCEligible(t *testing.T) { job.Stop = tc.JobStop t.Run(tc.Desc, func(t *testing.T) { - if got := allocGCEligible(alloc, job, followupEval, tc.ThresholdIndex); got != tc.ShouldGC { + if got := allocGCEligible(alloc, job, tc.GCTime, tc.ThresholdIndex); got != tc.ShouldGC { t.Fatalf("expected %v but got %v", tc.ShouldGC, got) } - }) + } } @@ -1991,5 +2009,5 @@ func TestAllocation_GCEligible(t *testing.T) { require := require.New(t) alloc := mock.Alloc() alloc.ClientStatus = structs.AllocClientStatusComplete - require.True(allocGCEligible(alloc, nil, nil, 1000)) + require.True(allocGCEligible(alloc, nil, time.Now(), 1000)) } From a499147d49225fee13282629eddfc0cac04522a4 Mon Sep 17 00:00:00 2001 From: Preetha Appan Date: Wed, 11 Apr 2018 15:12:23 -0500 Subject: [PATCH 4/4] Make test descriptions better --- nomad/core_sched_test.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/nomad/core_sched_test.go b/nomad/core_sched_test.go index 0cb72dc0b..4227c440d 100644 --- a/nomad/core_sched_test.go +++ b/nomad/core_sched_test.go @@ -1816,7 +1816,7 @@ func TestAllocation_GCEligible(t *testing.T) { harness := []testCase{ { - Desc: "GC when non terminal", + Desc: "Don't GC when non terminal", ClientStatus: structs.AllocClientStatusPending, DesiredStatus: structs.AllocDesiredStatusRun, GCTime: fail, @@ -1825,7 +1825,7 @@ func TestAllocation_GCEligible(t *testing.T) { ShouldGC: false, }, { - Desc: "GC when non terminal and job stopped", + Desc: "Don't GC when non terminal and job stopped", ClientStatus: structs.AllocClientStatusPending, DesiredStatus: structs.AllocDesiredStatusRun, JobStop: true, @@ -1835,7 +1835,7 @@ func TestAllocation_GCEligible(t *testing.T) { ShouldGC: false, }, { - Desc: "GC when non terminal and job dead", + Desc: "Don't GC when non terminal and job dead", ClientStatus: structs.AllocClientStatusPending, DesiredStatus: structs.AllocDesiredStatusRun, JobStatus: structs.JobStatusDead, @@ -1849,13 +1849,13 @@ func TestAllocation_GCEligible(t *testing.T) { ClientStatus: structs.AllocClientStatusComplete, DesiredStatus: structs.AllocDesiredStatusRun, GCTime: fail, - ModifyIndex: 100, + ModifyIndex: 90, ThresholdIndex: 90, ReschedulePolicy: nil, - ShouldGC: false, + ShouldGC: true, }, { - Desc: "GC when threshold not met", + Desc: "Don't GC when threshold not met", ClientStatus: structs.AllocClientStatusComplete, DesiredStatus: structs.AllocDesiredStatusStop, GCTime: fail, @@ -1885,7 +1885,7 @@ func TestAllocation_GCEligible(t *testing.T) { ShouldGC: true, }, { - Desc: "GC with no previous attempts", + Desc: "Don't GC when no previous reschedule attempts", ClientStatus: structs.AllocClientStatusFailed, DesiredStatus: structs.AllocDesiredStatusRun, GCTime: fail, @@ -1895,7 +1895,7 @@ func TestAllocation_GCEligible(t *testing.T) { ShouldGC: false, }, { - Desc: "GC with prev reschedule attempt within interval", + Desc: "Don't GC when prev reschedule attempt within interval", ClientStatus: structs.AllocClientStatusFailed, DesiredStatus: structs.AllocDesiredStatusRun, ReschedulePolicy: &structs.ReschedulePolicy{Attempts: 2, Interval: 30 * time.Minute}, @@ -1940,7 +1940,7 @@ func TestAllocation_GCEligible(t *testing.T) { ShouldGC: true, }, { - Desc: "GC when next alloc id is not set and unlimited restarts", + Desc: "Don't GC when next alloc id is not set and unlimited restarts", ClientStatus: structs.AllocClientStatusFailed, DesiredStatus: structs.AllocDesiredStatusRun, GCTime: fail, @@ -2001,7 +2001,7 @@ func TestAllocation_GCEligible(t *testing.T) { if got := allocGCEligible(alloc, job, tc.GCTime, tc.ThresholdIndex); got != tc.ShouldGC { t.Fatalf("expected %v but got %v", tc.ShouldGC, got) } - } + }) }