From d3061f93e38e8f781b3c0222577a005d57bf6d49 Mon Sep 17 00:00:00 2001 From: Preetha Appan Date: Wed, 15 May 2019 20:34:06 -0500 Subject: [PATCH] Fix bug in plan applier introduced in PR-5602 This fixes a bug in the state store during plan apply. When denormalizing preempted allocations it incorrectly set the preemptor's job during the update. This eventually causes a panic downstream in the client. Added a test assertion that failed before and passes after this fix --- nomad/state/state_store.go | 7 ++++--- nomad/state/state_store_test.go | 2 ++ 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/nomad/state/state_store.go b/nomad/state/state_store.go index 276c6920d29..550d34b5780 100644 --- a/nomad/state/state_store.go +++ b/nomad/state/state_store.go @@ -4144,7 +4144,7 @@ func (s *StateSnapshot) DenormalizeAllocationSlice(allocs []*structs.Allocation, // DenormalizeAllocationDiffSlice queries the Allocation for each AllocationDiff and merges // the updated attributes with the existing Allocation, and attaches the Job provided -func (s *StateSnapshot) DenormalizeAllocationDiffSlice(allocDiffs []*structs.AllocationDiff, job *structs.Job) ([]*structs.Allocation, error) { +func (s *StateSnapshot) DenormalizeAllocationDiffSlice(allocDiffs []*structs.AllocationDiff, planJob *structs.Job) ([]*structs.Allocation, error) { // Output index for denormalized Allocations j := 0 @@ -4160,15 +4160,16 @@ func (s *StateSnapshot) DenormalizeAllocationDiffSlice(allocDiffs []*structs.All // Merge the updates to the Allocation allocCopy := alloc.CopySkipJob() - allocCopy.Job = job if allocDiff.PreemptedByAllocation != "" { - // If alloc is a preemption + // If alloc is a preemption set the job from the alloc read from the state store + allocCopy.Job = alloc.Job.Copy() allocCopy.PreemptedByAllocation = allocDiff.PreemptedByAllocation allocCopy.DesiredDescription = getPreemptedAllocDesiredDescription(allocDiff.PreemptedByAllocation) allocCopy.DesiredStatus = structs.AllocDesiredStatusEvict } else { // If alloc is a stopped alloc + allocCopy.Job = planJob allocCopy.DesiredDescription = allocDiff.DesiredDescription allocCopy.DesiredStatus = structs.AllocDesiredStatusStop if allocDiff.ClientStatus != "" { diff --git a/nomad/state/state_store_test.go b/nomad/state/state_store_test.go index 013f9cab932..9552d2321ff 100644 --- a/nomad/state/state_store_test.go +++ b/nomad/state/state_store_test.go @@ -396,6 +396,8 @@ func TestStateStore_UpsertPlanResults_PreemptedAllocs(t *testing.T) { require.NoError(err) require.Equal(preempted.DesiredStatus, structs.AllocDesiredStatusEvict) require.Equal(preempted.DesiredDescription, fmt.Sprintf("Preempted by alloc ID %v", alloc.ID)) + require.Equal(preempted.Job.ID, preemptedAlloc.Job.ID) + require.Equal(preempted.Job, preemptedAlloc.Job) // Verify eval for preempted job preemptedJobEval, err := state.EvalByID(ws, eval2.ID)