From d06aeab087efede952626c22182ee7bbc633aeb0 Mon Sep 17 00:00:00 2001 From: Mahmood Ali Date: Tue, 21 May 2019 13:56:58 -0400 Subject: [PATCH 1/3] tests: fix data race in client/allocrunner/taskrunner/template TestTaskTemplateManager_Rerender_Signal Given that Signal may be called multiple times, blocking for `SignalCh` isn't sufficient to synchornizing access to Signals field. --- .../taskrunner/template/template_test.go | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/client/allocrunner/taskrunner/template/template_test.go b/client/allocrunner/taskrunner/template/template_test.go index a0d6484a6..db8c1ba01 100644 --- a/client/allocrunner/taskrunner/template/template_test.go +++ b/client/allocrunner/taskrunner/template/template_test.go @@ -8,6 +8,7 @@ import ( "os" "path/filepath" "strings" + "sync" "testing" "time" @@ -34,8 +35,9 @@ type MockTaskHooks struct { Restarts int RestartCh chan struct{} - Signals []string - SignalCh chan struct{} + Signals []string + SignalCh chan struct{} + signalLock sync.Mutex // SignalError is returned when Signal is called on the mock hook SignalError error @@ -68,7 +70,9 @@ func (m *MockTaskHooks) Restart(ctx context.Context, event *structs.TaskEvent, f } func (m *MockTaskHooks) Signal(event *structs.TaskEvent, s string) error { + m.signalLock.Lock() m.Signals = append(m.Signals, s) + m.signalLock.Unlock() select { case m.SignalCh <- struct{}{}: default: @@ -837,7 +841,10 @@ OUTER: case <-harness.mockHooks.RestartCh: t.Fatalf("Restart with signal policy: %+v", harness.mockHooks) case <-harness.mockHooks.SignalCh: - if len(harness.mockHooks.Signals) != 2 { + harness.mockHooks.signalLock.Lock() + s := harness.mockHooks.Signals + harness.mockHooks.signalLock.Unlock() + if len(s) != 2 { continue } break OUTER From 621c649a0da4552c33aa11e1f24e5ec59d2081b8 Mon Sep 17 00:00:00 2001 From: Mahmood Ali Date: Tue, 21 May 2019 14:29:45 -0400 Subject: [PATCH 2/3] tests: fix deploymentwatcher tests data races --- .../deployments_watcher_test.go | 137 +++++++++--------- 1 file changed, 72 insertions(+), 65 deletions(-) diff --git a/nomad/deploymentwatcher/deployments_watcher_test.go b/nomad/deploymentwatcher/deployments_watcher_test.go index a92a8e029..ce37f1324 100644 --- a/nomad/deploymentwatcher/deployments_watcher_test.go +++ b/nomad/deploymentwatcher/deployments_watcher_test.go @@ -66,16 +66,16 @@ func TestWatcher_WatchDeployments(t *testing.T) { }() w.SetEnabled(true, m.state) - testutil.WaitForResult(func() (bool, error) { return 1 == len(w.watchers), nil }, - func(err error) { require.Equal(1, len(w.watchers), "1 deployment returned") }) + testutil.WaitForResult(func() (bool, error) { return 1 == watchersCount(w), nil }, + func(err error) { require.Equal(1, watchersCount(w), "1 deployment returned") }) close(block1) - testutil.WaitForResult(func() (bool, error) { return 3 == len(w.watchers), nil }, - func(err error) { require.Equal(3, len(w.watchers), "3 deployment returned") }) + testutil.WaitForResult(func() (bool, error) { return 3 == watchersCount(w), nil }, + func(err error) { require.Equal(3, watchersCount(w), "3 deployment returned") }) close(block2) - testutil.WaitForResult(func() (bool, error) { return 2 == len(w.watchers), nil }, - func(err error) { require.Equal(3, len(w.watchers), "3 deployment returned - 1 terminal") }) + testutil.WaitForResult(func() (bool, error) { return 2 == watchersCount(w), nil }, + func(err error) { require.Equal(3, watchersCount(w), "3 deployment returned - 1 terminal") }) } // Tests that calls against an unknown deployment fail @@ -156,8 +156,8 @@ func TestWatcher_SetAllocHealth_Unknown(t *testing.T) { m.On("UpdateDeploymentAllocHealth", mocker.MatchedBy(matcher)).Return(nil) w.SetEnabled(true, m.state) - testutil.WaitForResult(func() (bool, error) { return 1 == len(w.watchers), nil }, - func(err error) { require.Equal(1, len(w.watchers), "Should have 1 deployment") }) + testutil.WaitForResult(func() (bool, error) { return 1 == watchersCount(w), nil }, + func(err error) { require.Equal(1, watchersCount(w), "Should have 1 deployment") }) // Call SetAllocHealth req := &structs.DeploymentAllocHealthRequest{ @@ -169,7 +169,7 @@ func TestWatcher_SetAllocHealth_Unknown(t *testing.T) { if assert.NotNil(err, "Set health of unknown allocation") { require.Contains(err.Error(), "unknown") } - require.Equal(1, len(w.watchers), "Deployment should still be active") + require.Equal(1, watchersCount(w), "Deployment should still be active") } // Test setting allocation health @@ -198,8 +198,8 @@ func TestWatcher_SetAllocHealth_Healthy(t *testing.T) { m.On("UpdateDeploymentAllocHealth", mocker.MatchedBy(matcher)).Return(nil) w.SetEnabled(true, m.state) - testutil.WaitForResult(func() (bool, error) { return 1 == len(w.watchers), nil }, - func(err error) { require.Equal(1, len(w.watchers), "Should have 1 deployment") }) + testutil.WaitForResult(func() (bool, error) { return 1 == watchersCount(w), nil }, + func(err error) { require.Equal(1, watchersCount(w), "Should have 1 deployment") }) // Call SetAllocHealth req := &structs.DeploymentAllocHealthRequest{ @@ -209,7 +209,7 @@ func TestWatcher_SetAllocHealth_Healthy(t *testing.T) { var resp structs.DeploymentUpdateResponse err := w.SetAllocHealth(req, &resp) require.Nil(err, "SetAllocHealth") - require.Equal(1, len(w.watchers), "Deployment should still be active") + require.Equal(1, watchersCount(w), "Deployment should still be active") m.AssertCalled(t, "UpdateDeploymentAllocHealth", mocker.MatchedBy(matcher)) } @@ -244,8 +244,8 @@ func TestWatcher_SetAllocHealth_Unhealthy(t *testing.T) { m.On("UpdateDeploymentAllocHealth", mocker.MatchedBy(matcher)).Return(nil) w.SetEnabled(true, m.state) - testutil.WaitForResult(func() (bool, error) { return 1 == len(w.watchers), nil }, - func(err error) { require.Equal(1, len(w.watchers), "Should have 1 deployment") }) + testutil.WaitForResult(func() (bool, error) { return 1 == watchersCount(w), nil }, + func(err error) { require.Equal(1, watchersCount(w), "Should have 1 deployment") }) // Call SetAllocHealth req := &structs.DeploymentAllocHealthRequest{ @@ -256,8 +256,8 @@ func TestWatcher_SetAllocHealth_Unhealthy(t *testing.T) { err := w.SetAllocHealth(req, &resp) require.Nil(err, "SetAllocHealth") - testutil.WaitForResult(func() (bool, error) { return 0 == len(w.watchers), nil }, - func(err error) { require.Equal(0, len(w.watchers), "Should have no deployment") }) + testutil.WaitForResult(func() (bool, error) { return 0 == watchersCount(w), nil }, + func(err error) { require.Equal(0, watchersCount(w), "Should have no deployment") }) m.AssertNumberOfCalls(t, "UpdateDeploymentAllocHealth", 1) } @@ -307,8 +307,8 @@ func TestWatcher_SetAllocHealth_Unhealthy_Rollback(t *testing.T) { m.On("UpdateDeploymentAllocHealth", mocker.MatchedBy(matcher)).Return(nil) w.SetEnabled(true, m.state) - testutil.WaitForResult(func() (bool, error) { return 1 == len(w.watchers), nil }, - func(err error) { require.Equal(1, len(w.watchers), "Should have 1 deployment") }) + testutil.WaitForResult(func() (bool, error) { return 1 == watchersCount(w), nil }, + func(err error) { require.Equal(1, watchersCount(w), "Should have 1 deployment") }) // Call SetAllocHealth req := &structs.DeploymentAllocHealthRequest{ @@ -319,8 +319,8 @@ func TestWatcher_SetAllocHealth_Unhealthy_Rollback(t *testing.T) { err := w.SetAllocHealth(req, &resp) require.Nil(err, "SetAllocHealth") - testutil.WaitForResult(func() (bool, error) { return 0 == len(w.watchers), nil }, - func(err error) { require.Equal(0, len(w.watchers), "Should have no deployment") }) + testutil.WaitForResult(func() (bool, error) { return 0 == watchersCount(w), nil }, + func(err error) { require.Equal(0, watchersCount(w), "Should have no deployment") }) m.AssertNumberOfCalls(t, "UpdateDeploymentAllocHealth", 1) } @@ -368,8 +368,8 @@ func TestWatcher_SetAllocHealth_Unhealthy_NoRollback(t *testing.T) { m.On("UpdateDeploymentAllocHealth", mocker.MatchedBy(matcher)).Return(nil) w.SetEnabled(true, m.state) - testutil.WaitForResult(func() (bool, error) { return 1 == len(w.watchers), nil }, - func(err error) { require.Equal(1, len(w.watchers), "Should have 1 deployment") }) + testutil.WaitForResult(func() (bool, error) { return 1 == watchersCount(w), nil }, + func(err error) { require.Equal(1, watchersCount(w), "Should have 1 deployment") }) // Call SetAllocHealth req := &structs.DeploymentAllocHealthRequest{ @@ -380,8 +380,8 @@ func TestWatcher_SetAllocHealth_Unhealthy_NoRollback(t *testing.T) { err := w.SetAllocHealth(req, &resp) require.Nil(err, "SetAllocHealth") - testutil.WaitForResult(func() (bool, error) { return 0 == len(w.watchers), nil }, - func(err error) { require.Equal(0, len(w.watchers), "Should have no deployment") }) + testutil.WaitForResult(func() (bool, error) { return 0 == watchersCount(w), nil }, + func(err error) { require.Equal(0, watchersCount(w), "Should have no deployment") }) m.AssertNumberOfCalls(t, "UpdateDeploymentAllocHealth", 1) } @@ -426,8 +426,8 @@ func TestWatcher_PromoteDeployment_HealthyCanaries(t *testing.T) { m.On("UpdateAllocDesiredTransition", mocker.MatchedBy(m1)).Return(nil).Once() w.SetEnabled(true, m.state) - testutil.WaitForResult(func() (bool, error) { return 1 == len(w.watchers), nil }, - func(err error) { require.Equal(1, len(w.watchers), "Should have 1 deployment") }) + testutil.WaitForResult(func() (bool, error) { return 1 == watchersCount(w), nil }, + func(err error) { require.Equal(1, watchersCount(w), "Should have 1 deployment") }) // Call PromoteDeployment req := &structs.DeploymentPromoteRequest{ @@ -437,7 +437,7 @@ func TestWatcher_PromoteDeployment_HealthyCanaries(t *testing.T) { var resp structs.DeploymentUpdateResponse err := w.PromoteDeployment(req, &resp) require.Nil(err, "PromoteDeployment") - require.Equal(1, len(w.watchers), "Deployment should still be active") + require.Equal(1, watchersCount(w), "Deployment should still be active") m.AssertCalled(t, "UpdateDeploymentPromotion", mocker.MatchedBy(matcher)) } @@ -475,8 +475,8 @@ func TestWatcher_PromoteDeployment_UnhealthyCanaries(t *testing.T) { m.On("UpdateDeploymentPromotion", mocker.MatchedBy(matcher)).Return(nil) w.SetEnabled(true, m.state) - testutil.WaitForResult(func() (bool, error) { return 1 == len(w.watchers), nil }, - func(err error) { require.Equal(1, len(w.watchers), "Should have 1 deployment") }) + testutil.WaitForResult(func() (bool, error) { return 1 == watchersCount(w), nil }, + func(err error) { require.Equal(1, watchersCount(w), "Should have 1 deployment") }) // Call SetAllocHealth req := &structs.DeploymentPromoteRequest{ @@ -489,7 +489,7 @@ func TestWatcher_PromoteDeployment_UnhealthyCanaries(t *testing.T) { require.Contains(err.Error(), `Task group "web" has 0/2 healthy allocations`, "Should error because canary isn't marked healthy") } - require.Equal(1, len(w.watchers), "Deployment should still be active") + require.Equal(1, watchersCount(w), "Deployment should still be active") m.AssertCalled(t, "UpdateDeploymentPromotion", mocker.MatchedBy(matcher)) } @@ -516,8 +516,8 @@ func TestWatcher_PauseDeployment_Pause_Running(t *testing.T) { m.On("UpdateDeploymentStatus", mocker.MatchedBy(matcher)).Return(nil) w.SetEnabled(true, m.state) - testutil.WaitForResult(func() (bool, error) { return 1 == len(w.watchers), nil }, - func(err error) { require.Equal(1, len(w.watchers), "Should have 1 deployment") }) + testutil.WaitForResult(func() (bool, error) { return 1 == watchersCount(w), nil }, + func(err error) { require.Equal(1, watchersCount(w), "Should have 1 deployment") }) // Call PauseDeployment req := &structs.DeploymentPauseRequest{ @@ -528,7 +528,7 @@ func TestWatcher_PauseDeployment_Pause_Running(t *testing.T) { err := w.PauseDeployment(req, &resp) require.Nil(err, "PauseDeployment") - require.Equal(1, len(w.watchers), "Deployment should still be active") + require.Equal(1, watchersCount(w), "Deployment should still be active") m.AssertCalled(t, "UpdateDeploymentStatus", mocker.MatchedBy(matcher)) } @@ -556,8 +556,8 @@ func TestWatcher_PauseDeployment_Pause_Paused(t *testing.T) { m.On("UpdateDeploymentStatus", mocker.MatchedBy(matcher)).Return(nil) w.SetEnabled(true, m.state) - testutil.WaitForResult(func() (bool, error) { return 1 == len(w.watchers), nil }, - func(err error) { require.Equal(1, len(w.watchers), "Should have 1 deployment") }) + testutil.WaitForResult(func() (bool, error) { return 1 == watchersCount(w), nil }, + func(err error) { require.Equal(1, watchersCount(w), "Should have 1 deployment") }) // Call PauseDeployment req := &structs.DeploymentPauseRequest{ @@ -568,7 +568,7 @@ func TestWatcher_PauseDeployment_Pause_Paused(t *testing.T) { err := w.PauseDeployment(req, &resp) require.Nil(err, "PauseDeployment") - require.Equal(1, len(w.watchers), "Deployment should still be active") + require.Equal(1, watchersCount(w), "Deployment should still be active") m.AssertCalled(t, "UpdateDeploymentStatus", mocker.MatchedBy(matcher)) } @@ -597,8 +597,8 @@ func TestWatcher_PauseDeployment_Unpause_Paused(t *testing.T) { m.On("UpdateDeploymentStatus", mocker.MatchedBy(matcher)).Return(nil) w.SetEnabled(true, m.state) - testutil.WaitForResult(func() (bool, error) { return 1 == len(w.watchers), nil }, - func(err error) { require.Equal(1, len(w.watchers), "Should have 1 deployment") }) + testutil.WaitForResult(func() (bool, error) { return 1 == watchersCount(w), nil }, + func(err error) { require.Equal(1, watchersCount(w), "Should have 1 deployment") }) // Call PauseDeployment req := &structs.DeploymentPauseRequest{ @@ -609,7 +609,7 @@ func TestWatcher_PauseDeployment_Unpause_Paused(t *testing.T) { err := w.PauseDeployment(req, &resp) require.Nil(err, "PauseDeployment") - require.Equal(1, len(w.watchers), "Deployment should still be active") + require.Equal(1, watchersCount(w), "Deployment should still be active") m.AssertCalled(t, "UpdateDeploymentStatus", mocker.MatchedBy(matcher)) } @@ -637,8 +637,8 @@ func TestWatcher_PauseDeployment_Unpause_Running(t *testing.T) { m.On("UpdateDeploymentStatus", mocker.MatchedBy(matcher)).Return(nil) w.SetEnabled(true, m.state) - testutil.WaitForResult(func() (bool, error) { return 1 == len(w.watchers), nil }, - func(err error) { require.Equal(1, len(w.watchers), "Should have 1 deployment") }) + testutil.WaitForResult(func() (bool, error) { return 1 == watchersCount(w), nil }, + func(err error) { require.Equal(1, watchersCount(w), "Should have 1 deployment") }) // Call PauseDeployment req := &structs.DeploymentPauseRequest{ @@ -649,7 +649,7 @@ func TestWatcher_PauseDeployment_Unpause_Running(t *testing.T) { err := w.PauseDeployment(req, &resp) require.Nil(err, "PauseDeployment") - require.Equal(1, len(w.watchers), "Deployment should still be active") + require.Equal(1, watchersCount(w), "Deployment should still be active") m.AssertCalled(t, "UpdateDeploymentStatus", mocker.MatchedBy(matcher)) } @@ -677,8 +677,8 @@ func TestWatcher_FailDeployment_Running(t *testing.T) { m.On("UpdateDeploymentStatus", mocker.MatchedBy(matcher)).Return(nil) w.SetEnabled(true, m.state) - testutil.WaitForResult(func() (bool, error) { return 1 == len(w.watchers), nil }, - func(err error) { require.Equal(1, len(w.watchers), "Should have 1 deployment") }) + testutil.WaitForResult(func() (bool, error) { return 1 == watchersCount(w), nil }, + func(err error) { require.Equal(1, watchersCount(w), "Should have 1 deployment") }) // Call PauseDeployment req := &structs.DeploymentFailRequest{ @@ -688,7 +688,7 @@ func TestWatcher_FailDeployment_Running(t *testing.T) { err := w.FailDeployment(req, &resp) require.Nil(err, "FailDeployment") - require.Equal(1, len(w.watchers), "Deployment should still be active") + require.Equal(1, watchersCount(w), "Deployment should still be active") m.AssertCalled(t, "UpdateDeploymentStatus", mocker.MatchedBy(matcher)) } @@ -739,8 +739,8 @@ func TestDeploymentWatcher_Watch_NoProgressDeadline(t *testing.T) { m.On("UpdateDeploymentStatus", mocker.MatchedBy(m2)).Return(nil) w.SetEnabled(true, m.state) - testutil.WaitForResult(func() (bool, error) { return 1 == len(w.watchers), nil }, - func(err error) { require.Equal(1, len(w.watchers), "Should have 1 deployment") }) + testutil.WaitForResult(func() (bool, error) { return 1 == watchersCount(w), nil }, + func(err error) { require.Equal(1, watchersCount(w), "Should have 1 deployment") }) // Update the allocs health to healthy which should create an evaluation for i := 0; i < 5; i++ { @@ -810,8 +810,8 @@ func TestDeploymentWatcher_Watch_NoProgressDeadline(t *testing.T) { } m3 := matchDeploymentStatusUpdateRequest(c2) m.AssertCalled(t, "UpdateDeploymentStatus", mocker.MatchedBy(m3)) - testutil.WaitForResult(func() (bool, error) { return 0 == len(w.watchers), nil }, - func(err error) { require.Equal(0, len(w.watchers), "Should have no deployment") }) + testutil.WaitForResult(func() (bool, error) { return 0 == watchersCount(w), nil }, + func(err error) { require.Equal(0, watchersCount(w), "Should have no deployment") }) } func TestDeploymentWatcher_Watch_ProgressDeadline(t *testing.T) { @@ -848,8 +848,8 @@ func TestDeploymentWatcher_Watch_ProgressDeadline(t *testing.T) { m.On("UpdateDeploymentStatus", mocker.MatchedBy(m2)).Return(nil) w.SetEnabled(true, m.state) - testutil.WaitForResult(func() (bool, error) { return 1 == len(w.watchers), nil }, - func(err error) { require.Equal(1, len(w.watchers), "Should have 1 deployment") }) + testutil.WaitForResult(func() (bool, error) { return 1 == watchersCount(w), nil }, + func(err error) { require.Equal(1, watchersCount(w), "Should have 1 deployment") }) // Update the alloc to be unhealthy and require that nothing happens. a2 := a.Copy() @@ -933,8 +933,8 @@ func TestDeploymentWatcher_ProgressCutoff(t *testing.T) { m.On("UpdateAllocDesiredTransition", mocker.MatchedBy(m1)).Return(nil).Once() w.SetEnabled(true, m.state) - testutil.WaitForResult(func() (bool, error) { return 1 == len(w.watchers), nil }, - func(err error) { require.Equal(1, len(w.watchers), "Should have 1 deployment") }) + testutil.WaitForResult(func() (bool, error) { return 1 == watchersCount(w), nil }, + func(err error) { require.Equal(1, watchersCount(w), "Should have 1 deployment") }) watcher, err := w.getOrCreateWatcher(d.ID) require.NoError(err) @@ -1024,8 +1024,8 @@ func TestDeploymentWatcher_Watch_ProgressDeadline_Canaries(t *testing.T) { m.On("UpdateAllocDesiredTransition", mocker.MatchedBy(m1)).Return(nil).Once() w.SetEnabled(true, m.state) - testutil.WaitForResult(func() (bool, error) { return 1 == len(w.watchers), nil }, - func(err error) { require.Equal(1, len(w.watchers), "Should have 1 deployment") }) + testutil.WaitForResult(func() (bool, error) { return 1 == watchersCount(w), nil }, + func(err error) { require.Equal(1, watchersCount(w), "Should have 1 deployment") }) // Update the alloc to be unhealthy and require that nothing happens. a2 := a.Copy() @@ -1107,8 +1107,8 @@ func TestDeploymentWatcher_PromotedCanary_UpdatedAllocs(t *testing.T) { require.Nil(m.state.UpsertAllocs(m.nextIndex(), []*structs.Allocation{a}), "UpsertAllocs") w.SetEnabled(true, m.state) - testutil.WaitForResult(func() (bool, error) { return 1 == len(w.watchers), nil }, - func(err error) { require.Equal(1, len(w.watchers), "Should have 1 deployment") }) + testutil.WaitForResult(func() (bool, error) { return 1 == watchersCount(w), nil }, + func(err error) { require.Equal(1, watchersCount(w), "Should have 1 deployment") }) m1 := matchUpdateAllocDesiredTransitions([]string{d.ID}) m.On("UpdateAllocDesiredTransition", mocker.MatchedBy(m1)).Return(nil).Twice() @@ -1189,8 +1189,8 @@ func TestDeploymentWatcher_Watch_StartWithoutProgressDeadline(t *testing.T) { m.On("UpdateAllocDesiredTransition", mocker.MatchedBy(m1)).Return(nil).Once() w.SetEnabled(true, m.state) - testutil.WaitForResult(func() (bool, error) { return 1 == len(w.watchers), nil }, - func(err error) { require.Equal(1, len(w.watchers), "Should have 1 deployment") }) + testutil.WaitForResult(func() (bool, error) { return 1 == watchersCount(w), nil }, + func(err error) { require.Equal(1, watchersCount(w), "Should have 1 deployment") }) // Update the alloc to be unhealthy a2 := a.Copy() @@ -1259,8 +1259,8 @@ func TestDeploymentWatcher_RollbackFailed(t *testing.T) { m.On("UpdateDeploymentStatus", mocker.MatchedBy(m2)).Return(nil) w.SetEnabled(true, m.state) - testutil.WaitForResult(func() (bool, error) { return 1 == len(w.watchers), nil }, - func(err error) { require.Equal(1, len(w.watchers), "Should have 1 deployment") }) + testutil.WaitForResult(func() (bool, error) { return 1 == watchersCount(w), nil }, + func(err error) { require.Equal(1, watchersCount(w), "Should have 1 deployment") }) // Update the allocs health to healthy which should create an evaluation for i := 0; i < 5; i++ { @@ -1365,8 +1365,8 @@ func TestWatcher_BatchAllocUpdates(t *testing.T) { m.On("UpdateAllocDesiredTransition", mocker.MatchedBy(m1)).Return(nil).Once() w.SetEnabled(true, m.state) - testutil.WaitForResult(func() (bool, error) { return 2 == len(w.watchers), nil }, - func(err error) { require.Equal(2, len(w.watchers), "Should have 2 deployment") }) + testutil.WaitForResult(func() (bool, error) { return 2 == watchersCount(w), nil }, + func(err error) { require.Equal(2, watchersCount(w), "Should have 2 deployment") }) // Update the allocs health to healthy which should create an evaluation req := &structs.ApplyDeploymentAllocHealthRequest{ @@ -1412,6 +1412,13 @@ func TestWatcher_BatchAllocUpdates(t *testing.T) { }) m.AssertCalled(t, "UpdateAllocDesiredTransition", mocker.MatchedBy(m1)) - testutil.WaitForResult(func() (bool, error) { return 2 == len(w.watchers), nil }, - func(err error) { require.Equal(2, len(w.watchers), "Should have 2 deployment") }) + testutil.WaitForResult(func() (bool, error) { return 2 == watchersCount(w), nil }, + func(err error) { require.Equal(2, watchersCount(w), "Should have 2 deployment") }) +} + +func watchersCount(w *Watcher) int { + w.l.Lock() + defer w.l.Unlock() + + return len(w.watchers) } From b4262ce7374408287d6db6c296a1451c96bc6389 Mon Sep 17 00:00:00 2001 From: Mahmood Ali Date: Tue, 21 May 2019 14:35:23 -0400 Subject: [PATCH 3/3] tests: fix some nomad/drainer test data races --- nomad/drainer/drain_testing.go | 7 +++++++ nomad/drainer/watch_nodes_test.go | 14 +++++++------- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/nomad/drainer/drain_testing.go b/nomad/drainer/drain_testing.go index 5af351fe8..61de31d81 100644 --- a/nomad/drainer/drain_testing.go +++ b/nomad/drainer/drain_testing.go @@ -43,3 +43,10 @@ func (m *MockNodeTracker) Update(node *structs.Node) { m.Nodes[node.ID] = node m.Events = append(m.Events, &MockNodeTrackerEvent{NodeUpdate: node}) } + +func (m *MockNodeTracker) events() []*MockNodeTrackerEvent { + m.Lock() + defer m.Unlock() + + return m.Events +} diff --git a/nomad/drainer/watch_nodes_test.go b/nomad/drainer/watch_nodes_test.go index ca64e2dd1..60cf262d9 100644 --- a/nomad/drainer/watch_nodes_test.go +++ b/nomad/drainer/watch_nodes_test.go @@ -49,7 +49,7 @@ func TestNodeDrainWatcher_AddDraining(t *testing.T) { require.Nil(state.UpsertNode(101, n2)) testutil.WaitForResult(func() (bool, error) { - return len(m.Events) == 1, nil + return len(m.events()) == 1, nil }, func(err error) { t.Fatal("No node drain events") }) @@ -78,7 +78,7 @@ func TestNodeDrainWatcher_Remove(t *testing.T) { // Wait for it to be tracked require.Nil(state.UpsertNode(100, n)) testutil.WaitForResult(func() (bool, error) { - return len(m.Events) == 1, nil + return len(m.events()) == 1, nil }, func(err error) { t.Fatal("No node drain events") }) @@ -90,7 +90,7 @@ func TestNodeDrainWatcher_Remove(t *testing.T) { // Change the node to be not draining and wait for it to be untracked require.Nil(state.UpdateNodeDrain(101, n.ID, nil, false, nil)) testutil.WaitForResult(func() (bool, error) { - return len(m.Events) == 2, nil + return len(m.events()) == 2, nil }, func(err error) { t.Fatal("No new node drain events") }) @@ -116,7 +116,7 @@ func TestNodeDrainWatcher_Remove_Nonexistent(t *testing.T) { // Wait for it to be tracked require.Nil(state.UpsertNode(100, n)) testutil.WaitForResult(func() (bool, error) { - return len(m.Events) == 1, nil + return len(m.events()) == 1, nil }, func(err error) { t.Fatal("No node drain events") }) @@ -128,7 +128,7 @@ func TestNodeDrainWatcher_Remove_Nonexistent(t *testing.T) { // Delete the node require.Nil(state.DeleteNode(101, n.ID)) testutil.WaitForResult(func() (bool, error) { - return len(m.Events) == 2, nil + return len(m.events()) == 2, nil }, func(err error) { t.Fatal("No new node drain events") }) @@ -154,7 +154,7 @@ func TestNodeDrainWatcher_Update(t *testing.T) { // Wait for it to be tracked require.Nil(state.UpsertNode(100, n)) testutil.WaitForResult(func() (bool, error) { - return len(m.Events) == 1, nil + return len(m.events()) == 1, nil }, func(err error) { t.Fatal("No node drain events") }) @@ -170,7 +170,7 @@ func TestNodeDrainWatcher_Update(t *testing.T) { // Wait for it to be updated testutil.WaitForResult(func() (bool, error) { - return len(m.Events) == 2, nil + return len(m.events()) == 2, nil }, func(err error) { t.Fatal("No new node drain events") })