From 62ac40ab099bee69f3208dc9f74088aae5196604 Mon Sep 17 00:00:00 2001 From: Danielle Tomlinson Date: Tue, 4 Dec 2018 18:16:39 +0100 Subject: [PATCH 1/4] allocrunner: Basic test alloc runner --- client/allocrunner/alloc_runner_test.go | 63 ------------------- client/allocrunner/testing.go | 81 +++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 63 deletions(-) create mode 100644 client/allocrunner/testing.go diff --git a/client/allocrunner/alloc_runner_test.go b/client/allocrunner/alloc_runner_test.go index 0f1c6897c..3c1464560 100644 --- a/client/allocrunner/alloc_runner_test.go +++ b/client/allocrunner/alloc_runner_test.go @@ -2,79 +2,16 @@ package allocrunner import ( "fmt" - "sync" "testing" "time" - "github.com/hashicorp/nomad/client/allocwatcher" - "github.com/hashicorp/nomad/client/config" - consulapi "github.com/hashicorp/nomad/client/consul" - "github.com/hashicorp/nomad/client/devicemanager" "github.com/hashicorp/nomad/client/state" - "github.com/hashicorp/nomad/client/vaultclient" "github.com/hashicorp/nomad/nomad/mock" "github.com/hashicorp/nomad/nomad/structs" - "github.com/hashicorp/nomad/plugins/shared/catalog" - "github.com/hashicorp/nomad/plugins/shared/singleton" "github.com/hashicorp/nomad/testutil" "github.com/stretchr/testify/require" ) -// MockStateUpdater implements the AllocStateHandler interface and records -// alloc updates. -type MockStateUpdater struct { - Updates []*structs.Allocation - mu sync.Mutex -} - -// AllocStateUpdated implements the AllocStateHandler interface and records an -// alloc update. -func (m *MockStateUpdater) AllocStateUpdated(alloc *structs.Allocation) { - m.mu.Lock() - m.Updates = append(m.Updates, alloc) - m.mu.Unlock() -} - -// Last returns a copy of the last alloc (or nil) update. Safe for concurrent -// access with updates. -func (m *MockStateUpdater) Last() *structs.Allocation { - m.mu.Lock() - defer m.mu.Unlock() - n := len(m.Updates) - if n == 0 { - return nil - } - return m.Updates[n-1].Copy() -} - -// Reset resets the recorded alloc updates. -func (m *MockStateUpdater) Reset() { - m.mu.Lock() - m.Updates = nil - m.mu.Unlock() -} - -// testAllocRunnerConfig returns a new allocrunner.Config with mocks and noop -// versions of dependencies along with a cleanup func. -func testAllocRunnerConfig(t *testing.T, alloc *structs.Allocation) (*Config, func()) { - pluginLoader := catalog.TestPluginLoader(t) - clientConf, cleanup := config.TestClientConfig(t) - conf := &Config{ - // Copy the alloc in case the caller edits and reuses it - Alloc: alloc.Copy(), - Logger: clientConf.Logger, - ClientConfig: clientConf, - StateDB: state.NoopDB{}, - Consul: consulapi.NewMockConsulServiceClient(t, clientConf.Logger), - Vault: vaultclient.NewMockVaultClient(), - StateUpdater: &MockStateUpdater{}, - PrevAllocWatcher: allocwatcher.NoopPrevAlloc{}, - PluginSingletonLoader: singleton.NewSingletonLoader(clientConf.Logger, pluginLoader), - DeviceManager: devicemanager.NoopMockManager(), - } - return conf, cleanup -} - // TestAllocRunner_AllocState_Initialized asserts that getting TaskStates via // AllocState() are initialized even before the AllocRunner has run. func TestAllocRunner_AllocState_Initialized(t *testing.T) { diff --git a/client/allocrunner/testing.go b/client/allocrunner/testing.go new file mode 100644 index 000000000..3e18379dc --- /dev/null +++ b/client/allocrunner/testing.go @@ -0,0 +1,81 @@ +package allocrunner + +import ( + "sync" + "testing" + + "github.com/hashicorp/nomad/client/allocwatcher" + clientconfig "github.com/hashicorp/nomad/client/config" + "github.com/hashicorp/nomad/client/consul" + "github.com/hashicorp/nomad/client/devicemanager" + "github.com/hashicorp/nomad/client/state" + "github.com/hashicorp/nomad/client/vaultclient" + "github.com/hashicorp/nomad/nomad/structs" + "github.com/hashicorp/nomad/plugins/shared/catalog" + "github.com/hashicorp/nomad/plugins/shared/singleton" + "github.com/stretchr/testify/require" +) + +// MockStateUpdater implements the AllocStateHandler interface and records +// alloc updates. +type MockStateUpdater struct { + Updates []*structs.Allocation + mu sync.Mutex +} + +// AllocStateUpdated implements the AllocStateHandler interface and records an +// alloc update. +func (m *MockStateUpdater) AllocStateUpdated(alloc *structs.Allocation) { + m.mu.Lock() + m.Updates = append(m.Updates, alloc) + m.mu.Unlock() +} + +// Last returns a copy of the last alloc (or nil) update. Safe for concurrent +// access with updates. +func (m *MockStateUpdater) Last() *structs.Allocation { + m.mu.Lock() + defer m.mu.Unlock() + n := len(m.Updates) + if n == 0 { + return nil + } + return m.Updates[n-1].Copy() +} + +// Reset resets the recorded alloc updates. +func (m *MockStateUpdater) Reset() { + m.mu.Lock() + m.Updates = nil + m.mu.Unlock() +} + +func testAllocRunnerConfig(t *testing.T, alloc *structs.Allocation) (*Config, func()) { + pluginLoader := catalog.TestPluginLoader(t) + clientConf, cleanup := clientconfig.TestClientConfig(t) + conf := &Config{ + // Copy the alloc in case the caller edits and reuses it + Alloc: alloc.Copy(), + Logger: clientConf.Logger, + ClientConfig: clientConf, + StateDB: state.NoopDB{}, + Consul: consul.NewMockConsulServiceClient(t, clientConf.Logger), + Vault: vaultclient.NewMockVaultClient(), + StateUpdater: &MockStateUpdater{}, + PrevAllocWatcher: allocwatcher.NoopPrevAlloc{}, + PluginSingletonLoader: singleton.NewSingletonLoader(clientConf.Logger, pluginLoader), + DeviceManager: devicemanager.NoopMockManager(), + } + return conf, cleanup +} + +func TestAllocRunnerFromAlloc(t *testing.T, alloc *structs.Allocation) (*allocRunner, func()) { + t.Helper() + cfg, cleanup := testAllocRunnerConfig(t, alloc) + ar, err := NewAllocRunner(cfg) + if err != nil { + require.NoError(t, err, "Failed to setup AllocRunner") + } + + return ar, cleanup +} From 419ae3299cac4375112bca2c7f937dfb454c2f20 Mon Sep 17 00:00:00 2001 From: Danielle Tomlinson Date: Tue, 4 Dec 2018 18:16:50 +0100 Subject: [PATCH 2/4] client: Re-enable GC tests --- client/gc_test.go | 149 +++++++++++++++++++++++++++------------------- 1 file changed, 89 insertions(+), 60 deletions(-) diff --git a/client/gc_test.go b/client/gc_test.go index 8b9b33bbe..c20fb7adb 100644 --- a/client/gc_test.go +++ b/client/gc_test.go @@ -1,7 +1,5 @@ package client -/* -TODO(clientv2) import ( "fmt" "testing" @@ -28,7 +26,7 @@ func gcConfig() *GCConfig { // exitAllocRunner is a helper that updates the allocs on the given alloc // runners to be terminal -func exitAllocRunner(runners ...*allocrunner.AllocRunner) { +func exitAllocRunner(runners ...AllocRunner) { for _, ar := range runners { terminalAlloc := ar.Alloc() terminalAlloc.DesiredStatus = structs.AllocDesiredStatusStop @@ -40,15 +38,19 @@ func TestIndexedGCAllocPQ(t *testing.T) { t.Parallel() pq := NewIndexedGCAllocPQ() - _, ar1 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc(), false) - _, ar2 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc(), false) - _, ar3 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc(), false) - _, ar4 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc(), false) + ar1, cleanup1 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc()) + defer cleanup1() + ar2, cleanup2 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc()) + defer cleanup2() + ar3, cleanup3 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc()) + defer cleanup3() + ar4, cleanup4 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc()) + defer cleanup4() - pq.Push(ar1) - pq.Push(ar2) - pq.Push(ar3) - pq.Push(ar4) + pq.Push(ar1.Alloc().ID, ar1) + pq.Push(ar2.Alloc().ID, ar2) + pq.Push(ar3.Alloc().ID, ar3) + pq.Push(ar4.Alloc().ID, ar4) allocID := pq.Pop().allocRunner.Alloc().ID if allocID != ar1.Alloc().ID { @@ -119,11 +121,13 @@ func (m *MockStatsCollector) Stats() *stats.HostStats { func TestAllocGarbageCollector_MarkForCollection(t *testing.T) { t.Parallel() - logger := testlog.Logger(t) + logger := testlog.HCLogger(t) gc := NewAllocGarbageCollector(logger, &MockStatsCollector{}, &MockAllocCounter{}, gcConfig()) - _, ar1 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc(), false) - gc.MarkForCollection(ar1) + ar1, cleanup1 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc()) + defer cleanup1() + + gc.MarkForCollection(ar1.Alloc().ID, ar1) gcAlloc := gc.allocRunners.Pop() if gcAlloc == nil || gcAlloc.allocRunner != ar1 { @@ -133,16 +137,19 @@ func TestAllocGarbageCollector_MarkForCollection(t *testing.T) { func TestAllocGarbageCollector_Collect(t *testing.T) { t.Parallel() - logger := testlog.Logger(t) + logger := testlog.HCLogger(t) gc := NewAllocGarbageCollector(logger, &MockStatsCollector{}, &MockAllocCounter{}, gcConfig()) - _, ar1 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc(), false) - _, ar2 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc(), false) + ar1, cleanup1 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc()) + defer cleanup1() + ar2, cleanup2 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc()) + defer cleanup2() + go ar1.Run() go ar2.Run() - gc.MarkForCollection(ar1) - gc.MarkForCollection(ar2) + gc.MarkForCollection(ar1.Alloc().ID, ar1) + gc.MarkForCollection(ar2.Alloc().ID, ar2) // Exit the alloc runners exitAllocRunner(ar1, ar2) @@ -156,13 +163,16 @@ func TestAllocGarbageCollector_Collect(t *testing.T) { func TestAllocGarbageCollector_CollectAll(t *testing.T) { t.Parallel() - logger := testlog.Logger(t) + logger := testlog.HCLogger(t) gc := NewAllocGarbageCollector(logger, &MockStatsCollector{}, &MockAllocCounter{}, gcConfig()) - _, ar1 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc(), false) - _, ar2 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc(), false) - gc.MarkForCollection(ar1) - gc.MarkForCollection(ar2) + ar1, cleanup1 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc()) + defer cleanup1() + ar2, cleanup2 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc()) + defer cleanup2() + + gc.MarkForCollection(ar1.Alloc().ID, ar1) + gc.MarkForCollection(ar2.Alloc().ID, ar2) gc.CollectAll() gcAlloc := gc.allocRunners.Pop() @@ -173,19 +183,22 @@ func TestAllocGarbageCollector_CollectAll(t *testing.T) { func TestAllocGarbageCollector_MakeRoomForAllocations_EnoughSpace(t *testing.T) { t.Parallel() - logger := testlog.Logger(t) + logger := testlog.HCLogger(t) statsCollector := &MockStatsCollector{} conf := gcConfig() conf.ReservedDiskMB = 20 gc := NewAllocGarbageCollector(logger, statsCollector, &MockAllocCounter{}, conf) - _, ar1 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc(), false) - _, ar2 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc(), false) + ar1, cleanup1 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc()) + defer cleanup1() + ar2, cleanup2 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc()) + defer cleanup2() + go ar1.Run() go ar2.Run() - gc.MarkForCollection(ar1) - gc.MarkForCollection(ar2) + gc.MarkForCollection(ar1.Alloc().ID, ar1) + gc.MarkForCollection(ar2.Alloc().ID, ar2) // Exit the alloc runners exitAllocRunner(ar1, ar2) @@ -212,19 +225,22 @@ func TestAllocGarbageCollector_MakeRoomForAllocations_EnoughSpace(t *testing.T) func TestAllocGarbageCollector_MakeRoomForAllocations_GC_Partial(t *testing.T) { t.Parallel() - logger := testlog.Logger(t) + logger := testlog.HCLogger(t) statsCollector := &MockStatsCollector{} conf := gcConfig() conf.ReservedDiskMB = 20 gc := NewAllocGarbageCollector(logger, statsCollector, &MockAllocCounter{}, conf) - _, ar1 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc(), false) - _, ar2 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc(), false) + ar1, cleanup1 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc()) + defer cleanup1() + ar2, cleanup2 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc()) + defer cleanup2() + go ar1.Run() go ar2.Run() - gc.MarkForCollection(ar1) - gc.MarkForCollection(ar2) + gc.MarkForCollection(ar1.Alloc().ID, ar1) + gc.MarkForCollection(ar2.Alloc().ID, ar2) // Exit the alloc runners exitAllocRunner(ar1, ar2) @@ -252,19 +268,22 @@ func TestAllocGarbageCollector_MakeRoomForAllocations_GC_Partial(t *testing.T) { func TestAllocGarbageCollector_MakeRoomForAllocations_GC_All(t *testing.T) { t.Parallel() - logger := testlog.Logger(t) + logger := testlog.HCLogger(t) statsCollector := &MockStatsCollector{} conf := gcConfig() conf.ReservedDiskMB = 20 gc := NewAllocGarbageCollector(logger, statsCollector, &MockAllocCounter{}, conf) - _, ar1 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc(), false) - _, ar2 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc(), false) + ar1, cleanup1 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc()) + defer cleanup1() + ar2, cleanup2 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc()) + defer cleanup2() + go ar1.Run() go ar2.Run() - gc.MarkForCollection(ar1) - gc.MarkForCollection(ar2) + gc.MarkForCollection(ar1.Alloc().ID, ar1) + gc.MarkForCollection(ar2.Alloc().ID, ar2) // Exit the alloc runners exitAllocRunner(ar1, ar2) @@ -288,19 +307,22 @@ func TestAllocGarbageCollector_MakeRoomForAllocations_GC_All(t *testing.T) { func TestAllocGarbageCollector_MakeRoomForAllocations_GC_Fallback(t *testing.T) { t.Parallel() - logger := testlog.Logger(t) + logger := testlog.HCLogger(t) statsCollector := &MockStatsCollector{} conf := gcConfig() conf.ReservedDiskMB = 20 gc := NewAllocGarbageCollector(logger, statsCollector, &MockAllocCounter{}, conf) - _, ar1 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc(), false) - _, ar2 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc(), false) + ar1, cleanup1 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc()) + cleanup1() + ar2, cleanup2 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc()) + cleanup2() + go ar1.Run() go ar2.Run() - gc.MarkForCollection(ar1) - gc.MarkForCollection(ar2) + gc.MarkForCollection(ar1.Alloc().ID, ar1) + gc.MarkForCollection(ar2.Alloc().ID, ar2) // Exit the alloc runners exitAllocRunner(ar1, ar2) @@ -324,13 +346,15 @@ func TestAllocGarbageCollector_MakeRoomForAllocations_GC_Fallback(t *testing.T) // TestAllocGarbageCollector_MaxAllocs asserts that when making room for new // allocs, terminal allocs are GC'd until old_allocs + new_allocs <= limit func TestAllocGarbageCollector_MaxAllocs(t *testing.T) { + t.Skip("Test fails, unsure if behaviour change or broken test") + t.Parallel() server, serverAddr := testServer(t, nil) defer server.Shutdown() testutil.WaitForLeader(t, server.RPC) const maxAllocs = 6 - client := TestClient(t, func(c *config.Config) { + client, cleanup := TestClient(t, func(c *config.Config) { c.GCMaxAllocs = maxAllocs c.GCDiskUsageThreshold = 100 c.GCInodeUsageThreshold = 100 @@ -341,14 +365,14 @@ func TestAllocGarbageCollector_MaxAllocs(t *testing.T) { c.Servers = []string{serverAddr} c.ConsulConfig.ClientAutoJoin = new(bool) // squelch logs }) - defer client.Shutdown() + defer cleanup() waitTilNodeReady(client, t) callN := 0 assertAllocs := func(expectedAll, expectedDestroyed int) { // Wait for allocs to be started callN++ - client.logger.Printf("[TEST] %d -- Waiting for %d total allocs, %d GC'd", callN, expectedAll, expectedDestroyed) + client.logger.Info(fmt.Sprintf("[TEST] %d -- Waiting for %d total allocs, %d GC'd", callN, expectedAll, expectedDestroyed)) testutil.WaitForResult(func() (bool, error) { all, destroyed := 0, 0 for _, ar := range client.getAllocRunners() { @@ -362,10 +386,10 @@ func TestAllocGarbageCollector_MaxAllocs(t *testing.T) { expectedAll, all, expectedDestroyed, destroyed, ) }, func(err error) { - client.logger.Printf("[TEST] %d -- FAILED to find %d total allocs, %d GC'd!", callN, expectedAll, expectedDestroyed) + client.logger.Info(fmt.Sprintf("[TEST] %d -- FAILED to find %d total allocs, %d GC'd!", callN, expectedAll, expectedDestroyed)) t.Fatalf("%d alloc state: %v", callN, err) }) - client.logger.Printf("[TEST] %d -- Found %d total allocs, %d GC'd!", callN, expectedAll, expectedDestroyed) + client.logger.Info(fmt.Sprintf("[TEST] %d -- Found %d total allocs, %d GC'd!", callN, expectedAll, expectedDestroyed)) } // Create a job @@ -453,19 +477,22 @@ func TestAllocGarbageCollector_MaxAllocs(t *testing.T) { func TestAllocGarbageCollector_UsageBelowThreshold(t *testing.T) { t.Parallel() - logger := testlog.Logger(t) + logger := testlog.HCLogger(t) statsCollector := &MockStatsCollector{} conf := gcConfig() conf.ReservedDiskMB = 20 gc := NewAllocGarbageCollector(logger, statsCollector, &MockAllocCounter{}, conf) - _, ar1 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc(), false) - _, ar2 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc(), false) + ar1, cleanup1 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc()) + defer cleanup1() + ar2, cleanup2 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc()) + defer cleanup2() + go ar1.Run() go ar2.Run() - gc.MarkForCollection(ar1) - gc.MarkForCollection(ar2) + gc.MarkForCollection(ar1.Alloc().ID, ar1) + gc.MarkForCollection(ar2.Alloc().ID, ar2) // Exit the alloc runners exitAllocRunner(ar1, ar2) @@ -489,19 +516,22 @@ func TestAllocGarbageCollector_UsageBelowThreshold(t *testing.T) { func TestAllocGarbageCollector_UsedPercentThreshold(t *testing.T) { t.Parallel() - logger := testlog.Logger(t) + logger := testlog.HCLogger(t) statsCollector := &MockStatsCollector{} conf := gcConfig() conf.ReservedDiskMB = 20 gc := NewAllocGarbageCollector(logger, statsCollector, &MockAllocCounter{}, conf) - _, ar1 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc(), false) - _, ar2 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc(), false) + ar1, cleanup1 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc()) + defer cleanup1() + ar2, cleanup2 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc()) + defer cleanup2() + go ar1.Run() go ar2.Run() - gc.MarkForCollection(ar1) - gc.MarkForCollection(ar2) + gc.MarkForCollection(ar1.Alloc().ID, ar1) + gc.MarkForCollection(ar2.Alloc().ID, ar2) // Exit the alloc runners exitAllocRunner(ar1, ar2) @@ -524,4 +554,3 @@ func TestAllocGarbageCollector_UsedPercentThreshold(t *testing.T) { t.Fatalf("gcAlloc: %v", gcAlloc) } } -*/ From f3c057d7e01e4257cfbcae8e71730693e53a89a9 Mon Sep 17 00:00:00 2001 From: Danielle Tomlinson Date: Wed, 5 Dec 2018 16:24:06 +0100 Subject: [PATCH 3/4] client/gc: Replace GC integration test with unit The previous integration test was broken during the client refactor, and it seems to be some sort of race with state updating. I'm going to try and construct a replacement test as part of work on performance, but for now, the underlying behaviour is still being tested. --- client/gc_test.go | 155 +++++++++++----------------------------------- 1 file changed, 35 insertions(+), 120 deletions(-) diff --git a/client/gc_test.go b/client/gc_test.go index c20fb7adb..4ddecbe13 100644 --- a/client/gc_test.go +++ b/client/gc_test.go @@ -1,17 +1,15 @@ package client import ( - "fmt" "testing" "time" "github.com/hashicorp/nomad/client/allocrunner" - "github.com/hashicorp/nomad/client/config" "github.com/hashicorp/nomad/client/stats" "github.com/hashicorp/nomad/helper/testlog" "github.com/hashicorp/nomad/nomad/mock" "github.com/hashicorp/nomad/nomad/structs" - "github.com/hashicorp/nomad/testutil" + "github.com/stretchr/testify/require" ) func gcConfig() *GCConfig { @@ -343,136 +341,53 @@ func TestAllocGarbageCollector_MakeRoomForAllocations_GC_Fallback(t *testing.T) } } -// TestAllocGarbageCollector_MaxAllocs asserts that when making room for new +// TestAllocGarbageCollector_MakeRoomFor_MaxAllocs asserts that when making room for new // allocs, terminal allocs are GC'd until old_allocs + new_allocs <= limit -func TestAllocGarbageCollector_MaxAllocs(t *testing.T) { - t.Skip("Test fails, unsure if behaviour change or broken test") - +func TestAllocGarbageCollector_MakeRoomFor_MaxAllocs(t *testing.T) { t.Parallel() - server, serverAddr := testServer(t, nil) - defer server.Shutdown() - testutil.WaitForLeader(t, server.RPC) + logger := testlog.HCLogger(t) + statsCollector := &MockStatsCollector{} + conf := gcConfig() + conf.MaxAllocs = 3 + gc := NewAllocGarbageCollector(logger, statsCollector, &MockAllocCounter{}, conf) - const maxAllocs = 6 - client, cleanup := TestClient(t, func(c *config.Config) { - c.GCMaxAllocs = maxAllocs - c.GCDiskUsageThreshold = 100 - c.GCInodeUsageThreshold = 100 - c.GCParallelDestroys = 1 - c.GCInterval = time.Hour + ar1, cleanup1 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc()) + defer cleanup1() + ar2, cleanup2 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc()) + defer cleanup2() + ar3, cleanup3 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc()) + defer cleanup3() - c.RPCHandler = server - c.Servers = []string{serverAddr} - c.ConsulConfig.ClientAutoJoin = new(bool) // squelch logs - }) - defer cleanup() - waitTilNodeReady(client, t) + go ar1.Run() + go ar2.Run() + go ar3.Run() - callN := 0 - assertAllocs := func(expectedAll, expectedDestroyed int) { - // Wait for allocs to be started - callN++ - client.logger.Info(fmt.Sprintf("[TEST] %d -- Waiting for %d total allocs, %d GC'd", callN, expectedAll, expectedDestroyed)) - testutil.WaitForResult(func() (bool, error) { - all, destroyed := 0, 0 - for _, ar := range client.getAllocRunners() { - all++ - if ar.IsDestroyed() { - destroyed++ - } - } - return all == expectedAll && destroyed == expectedDestroyed, fmt.Errorf( - "expected %d allocs (found %d); expected %d destroy (found %d)", - expectedAll, all, expectedDestroyed, destroyed, - ) - }, func(err error) { - client.logger.Info(fmt.Sprintf("[TEST] %d -- FAILED to find %d total allocs, %d GC'd!", callN, expectedAll, expectedDestroyed)) - t.Fatalf("%d alloc state: %v", callN, err) - }) - client.logger.Info(fmt.Sprintf("[TEST] %d -- Found %d total allocs, %d GC'd!", callN, expectedAll, expectedDestroyed)) - } + exitAllocRunner(ar1) - // Create a job - state := server.State() - job := mock.Job() - job.TaskGroups[0].Tasks[0].Driver = "mock_driver" - job.TaskGroups[0].Tasks[0].Config = map[string]interface{}{ - "run_for": "30s", - } - nodeID := client.Node().ID - if err := state.UpsertJob(98, job); err != nil { - t.Fatalf("error upserting job: %v", err) - } - if err := state.UpsertJobSummary(99, mock.JobSummary(job.ID)); err != nil { - t.Fatalf("error upserting job summary: %v", err) - } + gc.MarkForCollection(ar1.Alloc().ID, ar1) + gc.MarkForCollection(ar2.Alloc().ID, ar2) + gc.MarkForCollection(ar3.Alloc().ID, ar3) - newAlloc := func() *structs.Allocation { - alloc := mock.Alloc() - alloc.JobID = job.ID - alloc.Job = job - alloc.NodeID = nodeID - return alloc - } - - // Create the allocations - allocs := make([]*structs.Allocation, 7) - for i := 0; i < len(allocs); i++ { - allocs[i] = newAlloc() - } - - // Upsert a copy of the allocs as modifying the originals later would - // cause a race { - allocsCopy := make([]*structs.Allocation, len(allocs)) - for i, a := range allocs { - allocsCopy[i] = a.Copy() - } - if err := state.UpsertAllocs(100, allocsCopy); err != nil { - t.Fatalf("error upserting initial allocs: %v", err) - } + alloc := mock.Alloc() + err := gc.MakeRoomFor([]*structs.Allocation{alloc}) + require.NoError(t, err) } - // 7 total, 0 GC'd - assertAllocs(7, 0) + // We GC a single alloc runner. + require.Equal(t, true, ar1.IsDestroyed()) + require.Equal(t, false, ar2.IsDestroyed()) - // Set the first few as terminal so they're marked for gc - const terminalN = 4 - for i := 0; i < terminalN; i++ { - // Copy the alloc so the pointers aren't shared - alloc := allocs[i].Copy() - alloc.DesiredStatus = structs.AllocDesiredStatusStop - allocs[i] = alloc - } - if err := state.UpsertAllocs(101, allocs[:terminalN]); err != nil { - t.Fatalf("error upserting stopped allocs: %v", err) + { + alloc := mock.Alloc() + err := gc.MakeRoomFor([]*structs.Allocation{alloc}) + require.NoError(t, err) } - // 7 total, 1 GC'd to get down to limit of 6 - assertAllocs(7, 1) - - // Add one more alloc - if err := state.UpsertAllocs(102, []*structs.Allocation{newAlloc()}); err != nil { - t.Fatalf("error upserting new alloc: %v", err) - } - - // 8 total, 1 GC'd to get down to limit of 6 - // If this fails it may be due to the gc's Run and MarkRoomFor methods - // gc'ing concurrently. May have to disable gc's run loop if this test - // is flaky. - assertAllocs(8, 2) - - // Add new allocs to cause the gc of old terminal ones - newAllocs := make([]*structs.Allocation, 4) - for i := 0; i < len(newAllocs); i++ { - newAllocs[i] = newAlloc() - } - if err := state.UpsertAllocs(200, newAllocs); err != nil { - t.Fatalf("error upserting %d new allocs: %v", len(newAllocs), err) - } - - // 12 total, 4 GC'd total because all other allocs are alive - assertAllocs(12, 4) + // We GC a second alloc runner. + require.Equal(t, true, ar1.IsDestroyed()) + require.Equal(t, true, ar2.IsDestroyed()) + require.Equal(t, false, ar3.IsDestroyed()) } func TestAllocGarbageCollector_UsageBelowThreshold(t *testing.T) { From f6e2687f5b936088996a22d75940659b476ae528 Mon Sep 17 00:00:00 2001 From: Danielle Tomlinson Date: Thu, 6 Dec 2018 15:04:37 +0100 Subject: [PATCH 4/4] gc: Fix maxallocs integration test --- client/gc_test.go | 168 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 131 insertions(+), 37 deletions(-) diff --git a/client/gc_test.go b/client/gc_test.go index 4ddecbe13..b40030368 100644 --- a/client/gc_test.go +++ b/client/gc_test.go @@ -1,14 +1,18 @@ package client import ( + "fmt" "testing" "time" "github.com/hashicorp/nomad/client/allocrunner" + "github.com/hashicorp/nomad/client/config" "github.com/hashicorp/nomad/client/stats" "github.com/hashicorp/nomad/helper/testlog" + "github.com/hashicorp/nomad/nomad" "github.com/hashicorp/nomad/nomad/mock" "github.com/hashicorp/nomad/nomad/structs" + "github.com/hashicorp/nomad/testutil" "github.com/stretchr/testify/require" ) @@ -344,50 +348,140 @@ func TestAllocGarbageCollector_MakeRoomForAllocations_GC_Fallback(t *testing.T) // TestAllocGarbageCollector_MakeRoomFor_MaxAllocs asserts that when making room for new // allocs, terminal allocs are GC'd until old_allocs + new_allocs <= limit func TestAllocGarbageCollector_MakeRoomFor_MaxAllocs(t *testing.T) { - t.Parallel() - logger := testlog.HCLogger(t) - statsCollector := &MockStatsCollector{} - conf := gcConfig() - conf.MaxAllocs = 3 - gc := NewAllocGarbageCollector(logger, statsCollector, &MockAllocCounter{}, conf) + const maxAllocs = 6 + require := require.New(t) - ar1, cleanup1 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc()) - defer cleanup1() - ar2, cleanup2 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc()) - defer cleanup2() - ar3, cleanup3 := allocrunner.TestAllocRunnerFromAlloc(t, mock.Alloc()) - defer cleanup3() + server, serverAddr := testServer(t, nil) + defer server.Shutdown() + testutil.WaitForLeader(t, server.RPC) - go ar1.Run() - go ar2.Run() - go ar3.Run() + client, cleanup := TestClient(t, func(c *config.Config) { + c.GCMaxAllocs = maxAllocs + c.GCDiskUsageThreshold = 100 + c.GCInodeUsageThreshold = 100 + c.GCParallelDestroys = 1 + c.GCInterval = time.Hour + c.RPCHandler = server + c.Servers = []string{serverAddr} + c.ConsulConfig.ClientAutoJoin = new(bool) + }) + defer cleanup() + waitTilNodeReady(client, t) - exitAllocRunner(ar1) - - gc.MarkForCollection(ar1.Alloc().ID, ar1) - gc.MarkForCollection(ar2.Alloc().ID, ar2) - gc.MarkForCollection(ar3.Alloc().ID, ar3) - - { - alloc := mock.Alloc() - err := gc.MakeRoomFor([]*structs.Allocation{alloc}) - require.NoError(t, err) + job := mock.Job() + job.TaskGroups[0].Count = 1 + job.TaskGroups[0].Tasks[0].Driver = "mock_driver" + job.TaskGroups[0].Tasks[0].Config = map[string]interface{}{ + "run_for": "30s", } - // We GC a single alloc runner. - require.Equal(t, true, ar1.IsDestroyed()) - require.Equal(t, false, ar2.IsDestroyed()) - - { - alloc := mock.Alloc() - err := gc.MakeRoomFor([]*structs.Allocation{alloc}) - require.NoError(t, err) + index := uint64(98) + nextIndex := func() uint64 { + index++ + return index } - // We GC a second alloc runner. - require.Equal(t, true, ar1.IsDestroyed()) - require.Equal(t, true, ar2.IsDestroyed()) - require.Equal(t, false, ar3.IsDestroyed()) + upsertJobFn := func(server *nomad.Server, j *structs.Job) { + state := server.State() + require.NoError(state.UpsertJob(nextIndex(), j)) + require.NoError(state.UpsertJobSummary(nextIndex(), mock.JobSummary(j.ID))) + } + + // Insert the Job + upsertJobFn(server, job) + + upsertAllocFn := func(server *nomad.Server, a *structs.Allocation) { + state := server.State() + require.NoError(state.UpsertAllocs(nextIndex(), []*structs.Allocation{a})) + } + + upsertNewAllocFn := func(server *nomad.Server, j *structs.Job) *structs.Allocation { + alloc := mock.Alloc() + alloc.Job = j + alloc.JobID = j.ID + alloc.NodeID = client.NodeID() + + upsertAllocFn(server, alloc) + + return alloc.Copy() + } + + var allocations []*structs.Allocation + + // Fill the node with allocations + for i := 0; i < maxAllocs; i++ { + allocations = append(allocations, upsertNewAllocFn(server, job)) + } + + // Wait until the allocations are ready + testutil.WaitForResult(func() (bool, error) { + ar := len(client.getAllocRunners()) + + return ar == maxAllocs, fmt.Errorf("Expected %d allocs, got %d", maxAllocs, ar) + }, func(err error) { + t.Fatalf("Allocs did not start: %v", err) + }) + + // Mark the first three as terminal + for i := 0; i < 3; i++ { + allocations[i].DesiredStatus = structs.AllocDesiredStatusStop + upsertAllocFn(server, allocations[i].Copy()) + } + + // Wait until the allocations are stopped + testutil.WaitForResult(func() (bool, error) { + ar := client.getAllocRunners() + stopped := 0 + for _, r := range ar { + if r.Alloc().TerminalStatus() { + stopped++ + } + } + + return stopped == 3, fmt.Errorf("Expected %d terminal allocs, got %d", 3, stopped) + }, func(err error) { + t.Fatalf("Allocs did not terminate: %v", err) + }) + + // Upsert a new allocation + // This does not get appended to `allocations` as we do not use them again. + upsertNewAllocFn(server, job) + + // A single allocation should be GC'd + testutil.WaitForResult(func() (bool, error) { + ar := client.getAllocRunners() + destroyed := 0 + for _, r := range ar { + if r.IsDestroyed() { + destroyed++ + } + } + + return destroyed == 1, fmt.Errorf("Expected %d gc'd ars, got %d", 1, destroyed) + }, func(err error) { + t.Fatalf("Allocs did not get GC'd: %v", err) + }) + + // Upsert a new allocation + // This does not get appended to `allocations` as we do not use them again. + upsertNewAllocFn(server, job) + + // 2 allocations should be GC'd + testutil.WaitForResult(func() (bool, error) { + ar := client.getAllocRunners() + destroyed := 0 + for _, r := range ar { + if r.IsDestroyed() { + destroyed++ + } + } + + return destroyed == 2, fmt.Errorf("Expected %d gc'd ars, got %d", 2, destroyed) + }, func(err error) { + t.Fatalf("Allocs did not get GC'd: %v", err) + }) + + require.Len(client.getAllocRunners(), 8) } func TestAllocGarbageCollector_UsageBelowThreshold(t *testing.T) {