From badabffbb04d17e575754016174060c246434caf Mon Sep 17 00:00:00 2001 From: Alex Dadgar Date: Wed, 26 Apr 2017 18:27:27 -0700 Subject: [PATCH 1/2] Protect against nil job in new allocation --- nomad/plan_apply_test.go | 1 - nomad/state/state_store.go | 15 +++++++++++++++ nomad/state/state_store_test.go | 14 ++++++++++++++ 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/nomad/plan_apply_test.go b/nomad/plan_apply_test.go index 7b0e3e659182..17fc0c6db7c8 100644 --- a/nomad/plan_apply_test.go +++ b/nomad/plan_apply_test.go @@ -119,7 +119,6 @@ func TestPlanApply_applyPlan(t *testing.T) { job := allocEvict.Job allocEvict.Job = nil alloc2 := mock.Alloc() - alloc2.Job = nil s1.State().UpsertJobSummary(1500, mock.JobSummary(alloc2.JobID)) plan = &structs.PlanResult{ NodeUpdate: map[string][]*structs.Allocation{ diff --git a/nomad/state/state_store.go b/nomad/state/state_store.go index 983c20a33716..7ed260e3614c 100644 --- a/nomad/state/state_store.go +++ b/nomad/state/state_store.go @@ -1131,6 +1131,21 @@ func (s *StateStore) UpsertAllocs(index uint64, allocs []*structs.Allocation) er alloc.CreateIndex = index alloc.ModifyIndex = index alloc.AllocModifyIndex = index + + // Issue https://github.com/hashicorp/nomad/issues/2583 uncovered + // the a race between a forced garbage collection and the scheduler + // marking an allocation as terminal. The issue is that the + // allocation from the scheduler has its job normalized and the FSM + // will only denormalize if the allocation is not terminal. However + // if the allocation is garbage collected, that will result in a + // allocation being upserted for the first time without a job + // attached. By returning an error here, it will cause the FSM to + // error, causing the plan_apply to error and thus causing the + // evaluation to be failed. This will force an index refresh that + // should solve this issue. + if alloc.Job == nil { + return fmt.Errorf("attempting to upsert allocation %q without a job", alloc.ID) + } } else { alloc.CreateIndex = exist.CreateIndex alloc.ModifyIndex = index diff --git a/nomad/state/state_store_test.go b/nomad/state/state_store_test.go index 0624417f8111..b14d478f7144 100644 --- a/nomad/state/state_store_test.go +++ b/nomad/state/state_store_test.go @@ -5,6 +5,7 @@ import ( "os" "reflect" "sort" + "strings" "testing" "time" @@ -2642,6 +2643,19 @@ func TestStateStore_UpsertAlloc_Alloc(t *testing.T) { } } +// Testing to ensure we keep issue +// https://github.com/hashicorp/nomad/issues/2583 fixed +func TestStateStore_UpsertAlloc_No_Job(t *testing.T) { + state := testStateStore(t) + alloc := mock.Alloc() + alloc.Job = nil + + err := state.UpsertAllocs(999, []*structs.Allocation{alloc}) + if err == nil || !strings.Contains(err.Error(), "without a job") { + t.Fatalf("expect err: %v", err) + } +} + func TestStateStore_UpsertAlloc_NoEphemeralDisk(t *testing.T) { state := testStateStore(t) alloc := mock.Alloc() From 741a71e0b3b2ab275ad7e12da1f878d39a5e8957 Mon Sep 17 00:00:00 2001 From: Alex Dadgar Date: Mon, 1 May 2017 13:54:26 -0700 Subject: [PATCH 2/2] Fix tests --- scheduler/context_test.go | 7 +++++-- scheduler/feasible_test.go | 25 +++++++++++++++++++++++++ scheduler/rank_test.go | 14 ++++++++++---- 3 files changed, 40 insertions(+), 6 deletions(-) diff --git a/scheduler/context_test.go b/scheduler/context_test.go index 00027c779008..7253950a5306 100644 --- a/scheduler/context_test.go +++ b/scheduler/context_test.go @@ -53,11 +53,13 @@ func TestEvalContext_ProposedAlloc(t *testing.T) { } // Add existing allocations + j1, j2 := mock.Job(), mock.Job() alloc1 := &structs.Allocation{ ID: structs.GenerateUUID(), EvalID: structs.GenerateUUID(), NodeID: nodes[0].Node.ID, - JobID: structs.GenerateUUID(), + JobID: j1.ID, + Job: j1, Resources: &structs.Resources{ CPU: 2048, MemoryMB: 2048, @@ -70,7 +72,8 @@ func TestEvalContext_ProposedAlloc(t *testing.T) { ID: structs.GenerateUUID(), EvalID: structs.GenerateUUID(), NodeID: nodes[1].Node.ID, - JobID: structs.GenerateUUID(), + JobID: j2.ID, + Job: j2, Resources: &structs.Resources{ CPU: 1024, MemoryMB: 1024, diff --git a/scheduler/feasible_test.go b/scheduler/feasible_test.go index 097bfc175bf7..86cd6f6f18a4 100644 --- a/scheduler/feasible_test.go +++ b/scheduler/feasible_test.go @@ -463,6 +463,7 @@ func TestDistinctHostsIterator_JobDistinctHosts(t *testing.T) { &structs.Allocation{ TaskGroup: tg1.Name, JobID: job.ID, + Job: job, ID: structs.GenerateUUID(), }, @@ -470,6 +471,7 @@ func TestDistinctHostsIterator_JobDistinctHosts(t *testing.T) { &structs.Allocation{ TaskGroup: tg2.Name, JobID: "ignore 2", + Job: job, ID: structs.GenerateUUID(), }, } @@ -477,6 +479,7 @@ func TestDistinctHostsIterator_JobDistinctHosts(t *testing.T) { &structs.Allocation{ TaskGroup: tg2.Name, JobID: job.ID, + Job: job, ID: structs.GenerateUUID(), }, @@ -484,6 +487,7 @@ func TestDistinctHostsIterator_JobDistinctHosts(t *testing.T) { &structs.Allocation{ TaskGroup: tg1.Name, JobID: "ignore 2", + Job: job, ID: structs.GenerateUUID(), }, } @@ -658,6 +662,7 @@ func TestDistinctPropertyIterator_JobDistinctProperty(t *testing.T) { &structs.Allocation{ TaskGroup: tg1.Name, JobID: job.ID, + Job: job, ID: alloc1ID, NodeID: nodes[0].ID, }, @@ -666,6 +671,7 @@ func TestDistinctPropertyIterator_JobDistinctProperty(t *testing.T) { &structs.Allocation{ TaskGroup: tg2.Name, JobID: "ignore 2", + Job: job, ID: structs.GenerateUUID(), NodeID: nodes[0].ID, }, @@ -674,6 +680,7 @@ func TestDistinctPropertyIterator_JobDistinctProperty(t *testing.T) { &structs.Allocation{ TaskGroup: tg2.Name, JobID: job.ID, + Job: job, ID: structs.GenerateUUID(), NodeID: nodes[2].ID, }, @@ -682,6 +689,7 @@ func TestDistinctPropertyIterator_JobDistinctProperty(t *testing.T) { &structs.Allocation{ TaskGroup: tg1.Name, JobID: "ignore 2", + Job: job, ID: structs.GenerateUUID(), NodeID: nodes[2].ID, }, @@ -693,6 +701,7 @@ func TestDistinctPropertyIterator_JobDistinctProperty(t *testing.T) { &structs.Allocation{ TaskGroup: tg2.Name, JobID: job.ID, + Job: job, ID: stoppingAllocID, NodeID: nodes[4].ID, }, @@ -704,6 +713,7 @@ func TestDistinctPropertyIterator_JobDistinctProperty(t *testing.T) { &structs.Allocation{ TaskGroup: tg1.Name, JobID: job.ID, + Job: job, ID: alloc1ID, EvalID: structs.GenerateUUID(), NodeID: nodes[0].ID, @@ -712,6 +722,7 @@ func TestDistinctPropertyIterator_JobDistinctProperty(t *testing.T) { &structs.Allocation{ TaskGroup: tg1.Name, JobID: job.ID, + Job: job, ID: structs.GenerateUUID(), EvalID: structs.GenerateUUID(), NodeID: nodes[1].ID, @@ -721,6 +732,7 @@ func TestDistinctPropertyIterator_JobDistinctProperty(t *testing.T) { &structs.Allocation{ TaskGroup: tg2.Name, JobID: "ignore 2", + Job: job, ID: structs.GenerateUUID(), EvalID: structs.GenerateUUID(), NodeID: nodes[1].ID, @@ -728,6 +740,7 @@ func TestDistinctPropertyIterator_JobDistinctProperty(t *testing.T) { &structs.Allocation{ TaskGroup: tg2.Name, JobID: job.ID, + Job: job, ID: structs.GenerateUUID(), EvalID: structs.GenerateUUID(), NodeID: nodes[3].ID, @@ -737,6 +750,7 @@ func TestDistinctPropertyIterator_JobDistinctProperty(t *testing.T) { &structs.Allocation{ TaskGroup: tg1.Name, JobID: "ignore 2", + Job: job, ID: structs.GenerateUUID(), EvalID: structs.GenerateUUID(), NodeID: nodes[3].ID, @@ -744,6 +758,7 @@ func TestDistinctPropertyIterator_JobDistinctProperty(t *testing.T) { &structs.Allocation{ TaskGroup: tg2.Name, JobID: job.ID, + Job: job, ID: stoppingAllocID, EvalID: structs.GenerateUUID(), NodeID: nodes[4].ID, @@ -803,6 +818,7 @@ func TestDistinctPropertyIterator_JobDistinctProperty_RemoveAndReplace(t *testin &structs.Allocation{ TaskGroup: tg1.Name, JobID: job.ID, + Job: job, ID: structs.GenerateUUID(), NodeID: nodes[0].ID, }, @@ -813,6 +829,7 @@ func TestDistinctPropertyIterator_JobDistinctProperty_RemoveAndReplace(t *testin &structs.Allocation{ TaskGroup: tg1.Name, JobID: job.ID, + Job: job, ID: stoppingAllocID, NodeID: nodes[0].ID, }, @@ -822,6 +839,7 @@ func TestDistinctPropertyIterator_JobDistinctProperty_RemoveAndReplace(t *testin &structs.Allocation{ TaskGroup: tg1.Name, JobID: job.ID, + Job: job, ID: stoppingAllocID, EvalID: structs.GenerateUUID(), NodeID: nodes[0].ID, @@ -886,6 +904,7 @@ func TestDistinctPropertyIterator_JobDistinctProperty_Infeasible(t *testing.T) { &structs.Allocation{ TaskGroup: tg1.Name, JobID: job.ID, + Job: job, ID: structs.GenerateUUID(), NodeID: nodes[0].ID, }, @@ -894,6 +913,7 @@ func TestDistinctPropertyIterator_JobDistinctProperty_Infeasible(t *testing.T) { &structs.Allocation{ TaskGroup: tg2.Name, JobID: job.ID, + Job: job, ID: structs.GenerateUUID(), EvalID: structs.GenerateUUID(), NodeID: nodes[1].ID, @@ -961,6 +981,7 @@ func TestDistinctPropertyIterator_TaskGroupDistinctProperty(t *testing.T) { &structs.Allocation{ TaskGroup: tg1.Name, JobID: job.ID, + Job: job, ID: structs.GenerateUUID(), NodeID: nodes[0].ID, }, @@ -972,6 +993,7 @@ func TestDistinctPropertyIterator_TaskGroupDistinctProperty(t *testing.T) { &structs.Allocation{ TaskGroup: tg1.Name, JobID: job.ID, + Job: job, ID: stoppingAllocID, NodeID: nodes[2].ID, }, @@ -981,6 +1003,7 @@ func TestDistinctPropertyIterator_TaskGroupDistinctProperty(t *testing.T) { &structs.Allocation{ TaskGroup: tg1.Name, JobID: job.ID, + Job: job, ID: structs.GenerateUUID(), EvalID: structs.GenerateUUID(), NodeID: nodes[1].ID, @@ -990,6 +1013,7 @@ func TestDistinctPropertyIterator_TaskGroupDistinctProperty(t *testing.T) { &structs.Allocation{ TaskGroup: tg1.Name, JobID: "ignore 2", + Job: job, ID: structs.GenerateUUID(), EvalID: structs.GenerateUUID(), NodeID: nodes[2].ID, @@ -998,6 +1022,7 @@ func TestDistinctPropertyIterator_TaskGroupDistinctProperty(t *testing.T) { &structs.Allocation{ TaskGroup: tg1.Name, JobID: job.ID, + Job: job, ID: stoppingAllocID, EvalID: structs.GenerateUUID(), NodeID: nodes[2].ID, diff --git a/scheduler/rank_test.go b/scheduler/rank_test.go index f42f6894c78c..83b1508213e0 100644 --- a/scheduler/rank_test.go +++ b/scheduler/rank_test.go @@ -202,11 +202,13 @@ func TestBinPackIterator_ExistingAlloc(t *testing.T) { static := NewStaticRankIterator(ctx, nodes) // Add existing allocations + j1, j2 := mock.Job(), mock.Job() alloc1 := &structs.Allocation{ ID: structs.GenerateUUID(), EvalID: structs.GenerateUUID(), NodeID: nodes[0].Node.ID, - JobID: structs.GenerateUUID(), + JobID: j1.ID, + Job: j1, Resources: &structs.Resources{ CPU: 2048, MemoryMB: 2048, @@ -219,7 +221,8 @@ func TestBinPackIterator_ExistingAlloc(t *testing.T) { ID: structs.GenerateUUID(), EvalID: structs.GenerateUUID(), NodeID: nodes[1].Node.ID, - JobID: structs.GenerateUUID(), + JobID: j2.ID, + Job: j2, Resources: &structs.Resources{ CPU: 1024, MemoryMB: 1024, @@ -286,11 +289,13 @@ func TestBinPackIterator_ExistingAlloc_PlannedEvict(t *testing.T) { static := NewStaticRankIterator(ctx, nodes) // Add existing allocations + j1, j2 := mock.Job(), mock.Job() alloc1 := &structs.Allocation{ ID: structs.GenerateUUID(), EvalID: structs.GenerateUUID(), NodeID: nodes[0].Node.ID, - JobID: structs.GenerateUUID(), + JobID: j1.ID, + Job: j1, Resources: &structs.Resources{ CPU: 2048, MemoryMB: 2048, @@ -303,7 +308,8 @@ func TestBinPackIterator_ExistingAlloc_PlannedEvict(t *testing.T) { ID: structs.GenerateUUID(), EvalID: structs.GenerateUUID(), NodeID: nodes[1].Node.ID, - JobID: structs.GenerateUUID(), + JobID: j2.ID, + Job: j2, Resources: &structs.Resources{ CPU: 1024, MemoryMB: 1024,