From f788316385d3f7a617afd8e8ccd6660fc16a59dd Mon Sep 17 00:00:00 2001 From: Drew Bailey <2614075+drewbailey@users.noreply.github.com> Date: Wed, 22 Jan 2020 11:12:30 -0500 Subject: [PATCH 1/3] keep placed canaries aligned with alloc status --- nomad/state/state_store.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/nomad/state/state_store.go b/nomad/state/state_store.go index c47b465f214d..39e3488189c1 100644 --- a/nomad/state/state_store.go +++ b/nomad/state/state_store.go @@ -3569,6 +3569,7 @@ func (s *StateStore) updateDeploymentWithAlloc(index uint64, alloc, existing *st alloc.DeploymentStatus.ModifyIndex = index } + // Create a copy of the deployment object deploymentCopy := deployment.Copy() deploymentCopy.ModifyIndex = index @@ -3578,6 +3579,21 @@ func (s *StateStore) updateDeploymentWithAlloc(index uint64, alloc, existing *st state.HealthyAllocs += healthy state.UnhealthyAllocs += unhealthy + // Ensure PlacedCanaries accurately reflects the alloc canary status + if alloc.DeploymentStatus != nil { + if alloc.DeploymentStatus.Canary { + found := false + for _, canary := range state.PlacedCanaries { + if alloc.ID == canary { + found = true + } + } + if !found { + state.PlacedCanaries = append(state.PlacedCanaries, alloc.ID) + } + } + } + // Update the progress deadline if pd := state.ProgressDeadline; pd != 0 { // If we are the first placed allocation for the deployment start the progress deadline. From 895e5634615a49880fb452825a94421059377d3a Mon Sep 17 00:00:00 2001 From: Drew Bailey <2614075+drewbailey@users.noreply.github.com> Date: Wed, 22 Jan 2020 11:43:29 -0500 Subject: [PATCH 2/3] nomad state store must be modified through raft, rm local state change --- nomad/state/state_store.go | 20 +++++++++----------- scheduler/generic_sched.go | 4 ---- 2 files changed, 9 insertions(+), 15 deletions(-) diff --git a/nomad/state/state_store.go b/nomad/state/state_store.go index 39e3488189c1..df3fe4d30a0f 100644 --- a/nomad/state/state_store.go +++ b/nomad/state/state_store.go @@ -3569,7 +3569,6 @@ func (s *StateStore) updateDeploymentWithAlloc(index uint64, alloc, existing *st alloc.DeploymentStatus.ModifyIndex = index } - // Create a copy of the deployment object deploymentCopy := deployment.Copy() deploymentCopy.ModifyIndex = index @@ -3580,18 +3579,17 @@ func (s *StateStore) updateDeploymentWithAlloc(index uint64, alloc, existing *st state.UnhealthyAllocs += unhealthy // Ensure PlacedCanaries accurately reflects the alloc canary status - if alloc.DeploymentStatus != nil { - if alloc.DeploymentStatus.Canary { - found := false - for _, canary := range state.PlacedCanaries { - if alloc.ID == canary { - found = true - } - } - if !found { - state.PlacedCanaries = append(state.PlacedCanaries, alloc.ID) + if alloc.DeploymentStatus != nil && alloc.DeploymentStatus.Canary { + found := false + for _, canary := range state.PlacedCanaries { + if alloc.ID == canary { + found = true + break } } + if !found { + state.PlacedCanaries = append(state.PlacedCanaries, alloc.ID) + } } // Update the progress deadline diff --git a/scheduler/generic_sched.go b/scheduler/generic_sched.go index d9e5c7cc31c9..11e4bc9e3998 100644 --- a/scheduler/generic_sched.go +++ b/scheduler/generic_sched.go @@ -529,10 +529,6 @@ func (s *GenericScheduler) computePlacements(destructive, place []placementResul // If we are placing a canary and we found a match, add the canary // to the deployment state object and mark it as a canary. if missing.Canary() && s.deployment != nil { - if state, ok := s.deployment.TaskGroups[tg.Name]; ok { - state.PlacedCanaries = append(state.PlacedCanaries, alloc.ID) - } - alloc.DeploymentStatus = &structs.AllocDeploymentStatus{ Canary: true, } From f7fb6219a9c369b19cb3e9e7961279d402a2100a Mon Sep 17 00:00:00 2001 From: Drew Bailey <2614075+drewbailey@users.noreply.github.com> Date: Wed, 22 Jan 2020 15:34:03 -0500 Subject: [PATCH 3/3] add state store test to ensure PlacedCanaries is updated --- nomad/state/state_store_test.go | 103 ++++++++++++++++++++++++++++++++ scheduler/generic_sched_test.go | 27 ++++++--- 2 files changed, 122 insertions(+), 8 deletions(-) diff --git a/nomad/state/state_store_test.go b/nomad/state/state_store_test.go index 652458f573eb..fb1d51b7489c 100644 --- a/nomad/state/state_store_test.go +++ b/nomad/state/state_store_test.go @@ -6382,6 +6382,109 @@ func TestStateStore_UpsertDeploymentAllocHealth_BadAlloc_Nonexistent(t *testing. } } +// Test that a deployments PlacedCanaries is properly updated +func TestStateStore_UpsertDeploymentAlloc_Canaries(t *testing.T) { + t.Parallel() + + state := testStateStore(t) + + // Create a deployment + d1 := mock.Deployment() + require.NoError(t, state.UpsertDeployment(2, d1)) + + // Create a Job + job := mock.Job() + require.NoError(t, state.UpsertJob(3, job)) + + // Create alloc with canary status + a := mock.Alloc() + a.JobID = job.ID + a.DeploymentID = d1.ID + a.DeploymentStatus = &structs.AllocDeploymentStatus{ + Healthy: helper.BoolToPtr(false), + Canary: true, + } + require.NoError(t, state.UpsertAllocs(4, []*structs.Allocation{a})) + + // Pull the deployment from state + ws := memdb.NewWatchSet() + deploy, err := state.DeploymentByID(ws, d1.ID) + require.NoError(t, err) + + // Ensure that PlacedCanaries is accurate + require.Equal(t, 1, len(deploy.TaskGroups[job.TaskGroups[0].Name].PlacedCanaries)) + + // Create alloc without canary status + b := mock.Alloc() + b.JobID = job.ID + b.DeploymentID = d1.ID + b.DeploymentStatus = &structs.AllocDeploymentStatus{ + Healthy: helper.BoolToPtr(false), + Canary: false, + } + require.NoError(t, state.UpsertAllocs(4, []*structs.Allocation{b})) + + // Pull the deployment from state + ws = memdb.NewWatchSet() + deploy, err = state.DeploymentByID(ws, d1.ID) + require.NoError(t, err) + + // Ensure that PlacedCanaries is accurate + require.Equal(t, 1, len(deploy.TaskGroups[job.TaskGroups[0].Name].PlacedCanaries)) + + // Create a second deployment + d2 := mock.Deployment() + require.NoError(t, state.UpsertDeployment(5, d2)) + + c := mock.Alloc() + c.JobID = job.ID + c.DeploymentID = d2.ID + c.DeploymentStatus = &structs.AllocDeploymentStatus{ + Healthy: helper.BoolToPtr(false), + Canary: true, + } + require.NoError(t, state.UpsertAllocs(6, []*structs.Allocation{c})) + + ws = memdb.NewWatchSet() + deploy2, err := state.DeploymentByID(ws, d2.ID) + require.NoError(t, err) + + // Ensure that PlacedCanaries is accurate + require.Equal(t, 1, len(deploy2.TaskGroups[job.TaskGroups[0].Name].PlacedCanaries)) +} + +func TestStateStore_UpsertDeploymentAlloc_NoCanaries(t *testing.T) { + t.Parallel() + + state := testStateStore(t) + + // Create a deployment + d1 := mock.Deployment() + require.NoError(t, state.UpsertDeployment(2, d1)) + + // Create a Job + job := mock.Job() + require.NoError(t, state.UpsertJob(3, job)) + + // Create alloc with canary status + a := mock.Alloc() + a.JobID = job.ID + a.DeploymentID = d1.ID + a.DeploymentStatus = &structs.AllocDeploymentStatus{ + Healthy: helper.BoolToPtr(true), + Canary: false, + } + require.NoError(t, state.UpsertAllocs(4, []*structs.Allocation{a})) + + // Pull the deployment from state + ws := memdb.NewWatchSet() + deploy, err := state.DeploymentByID(ws, d1.ID) + require.NoError(t, err) + + // Ensure that PlacedCanaries is accurate + require.Equal(t, 0, len(deploy.TaskGroups[job.TaskGroups[0].Name].PlacedCanaries)) +} + // Test that allocation health can't be set for an alloc with mismatched // deployment ids func TestStateStore_UpsertDeploymentAllocHealth_BadAlloc_MismatchDeployment(t *testing.T) { diff --git a/scheduler/generic_sched_test.go b/scheduler/generic_sched_test.go index f7b06da7e9e1..7dc44bc460be 100644 --- a/scheduler/generic_sched_test.go +++ b/scheduler/generic_sched_test.go @@ -2029,17 +2029,28 @@ func TestServiceSched_JobModify_Canaries(t *testing.T) { if plan.Deployment == nil { t.Fatalf("bad: %#v", plan) } - state, ok := plan.Deployment.TaskGroups[job.TaskGroups[0].Name] - if !ok { - t.Fatalf("bad: %#v", plan) - } - if state.DesiredTotal != 10 && state.DesiredCanaries != desiredUpdates { - t.Fatalf("bad: %#v", state) - } + + // Ensure local state was not altered in scheduler + staleState, ok := plan.Deployment.TaskGroups[job.TaskGroups[0].Name] + require.True(t, ok) + + require.Equal(t, 0, len(staleState.PlacedCanaries)) + + ws := memdb.NewWatchSet() + + // Grab the latest state + deploy, err := h.State.DeploymentByID(ws, plan.Deployment.ID) + require.NoError(t, err) + + state, ok := deploy.TaskGroups[job.TaskGroups[0].Name] + require.True(t, ok) + + require.Equal(t, 10, state.DesiredTotal) + require.Equal(t, state.DesiredCanaries, desiredUpdates) // Assert the canaries were added to the placed list if len(state.PlacedCanaries) != desiredUpdates { - t.Fatalf("bad: %#v", state) + assert.Fail(t, "expected PlacedCanaries to equal desiredUpdates", state) } }