Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Protect against nil job in new allocation #2592

Merged
merged 2 commits into from
May 1, 2017
Merged

Protect against nil job in new allocation #2592

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
badabff
Protect against nil job in new allocation
dadgar Apr 27, 2017
741a71e
Fix tests
dadgar May 1, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion nomad/plan_apply_test.go
View file
Open in desktop
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
14 changes: 14 additions & 0 deletions nomad/state/state_store_test.go
View file
Open in desktop
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 @@ -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()
Expand Down
7 changes: 5 additions & 2 deletions scheduler/context_test.go
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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