Skip to content

Commit

Permalink
Merge pull request #2592 from hashicorp/b-gc-race
Browse files Browse the repository at this point in the history 
Protect against nil job in new allocation
  • Loading branch information
@dadgar
dadgar committed May 1, 2017
2 parents fcbaf69 + 741a71e commit cb7710d
Show file tree
Hide file tree
Showing 6 changed files with 69 additions and 7 deletions.
1 change: 0 additions & 1 deletion nomad/plan_apply_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
15 changes: 15 additions & 0 deletions nomad/state/state_store.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1150,6 +1150,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
Expand Down
14 changes: 14 additions & 0 deletions nomad/state/state_store_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"os"
"reflect"
"sort"
"strings"
"testing"
"time"

Expand Down Expand Up @@ -2665,6 +2666,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()
Expand Down
7 changes: 5 additions & 2 deletions scheduler/context_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down
25 changes: 25 additions & 0 deletions scheduler/feasible_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -463,27 +463,31 @@ func TestDistinctHostsIterator_JobDistinctHosts(t *testing.T) {
&structs.Allocation{
TaskGroup: tg1.Name,
JobID: job.ID,
Job: job,
ID: structs.GenerateUUID(),
},

// Should be ignored as it is a different job.
&structs.Allocation{
TaskGroup: tg2.Name,
JobID: "ignore 2",
Job: job,
ID: structs.GenerateUUID(),
},
}
plan.NodeAllocation[nodes[1].ID] = []*structs.Allocation{
&structs.Allocation{
TaskGroup: tg2.Name,
JobID: job.ID,
Job: job,
ID: structs.GenerateUUID(),
},

// Should be ignored as it is a different job.
&structs.Allocation{
TaskGroup: tg1.Name,
JobID: "ignore 2",
Job: job,
ID: structs.GenerateUUID(),
},
}
Expand Down Expand Up @@ -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,
},
Expand All @@ -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,
},
Expand All @@ -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,
},
Expand All @@ -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,
},
Expand All @@ -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,
},
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -721,13 +732,15 @@ 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,
},
&structs.Allocation{
TaskGroup: tg2.Name,
JobID: job.ID,
Job: job,
ID: structs.GenerateUUID(),
EvalID: structs.GenerateUUID(),
NodeID: nodes[3].ID,
Expand All @@ -737,13 +750,15 @@ 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,
},
&structs.Allocation{
TaskGroup: tg2.Name,
JobID: job.ID,
Job: job,
ID: stoppingAllocID,
EvalID: structs.GenerateUUID(),
NodeID: nodes[4].ID,
Expand Down Expand Up @@ -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,
},
Expand All @@ -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,
},
Expand All @@ -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,
Expand Down Expand Up @@ -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,
},
Expand All @@ -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,
Expand Down Expand Up @@ -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,
},
Expand All @@ -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,
},
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand Down
14 changes: 10 additions & 4 deletions scheduler/rank_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down

0 comments on commit cb7710d

Please sign in to comment.