From 7dbacf297c61edf15dfc1f8791263756405ec7bf Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 7 Aug 2020 15:51:13 -0400 Subject: [PATCH] testing: Remove NotifyShutdown NotifyShutdown was only used for testing. Now that t.Cleanup exists, we can use that instead of attaching cleanup to the Server shutdown. The Autopilot test which used NotifyShutdown doesn't need this notification because Shutdown is synchronous. Waiting for the function to return is equivalent. --- agent/consul/autopilot_test.go | 17 ++--------------- agent/consul/client_test.go | 20 +++++--------------- agent/consul/config.go | 3 --- agent/consul/server.go | 4 ---- agent/consul/server_test.go | 19 +++++-------------- 5 files changed, 12 insertions(+), 51 deletions(-) diff --git a/agent/consul/autopilot_test.go b/agent/consul/autopilot_test.go index 922d72a28d..4a2e737319 100644 --- a/agent/consul/autopilot_test.go +++ b/agent/consul/autopilot_test.go @@ -401,7 +401,6 @@ func TestAutopilot_PromoteNonVoter(t *testing.T) { func TestAutopilot_MinQuorum(t *testing.T) { dc := "dc1" - closeMap := make(map[string]chan struct{}) conf := func(c *Config) { c.Datacenter = dc c.Bootstrap = false @@ -409,13 +408,6 @@ func TestAutopilot_MinQuorum(t *testing.T) { c.AutopilotConfig.MinQuorum = 3 c.RaftConfig.ProtocolVersion = raft.ProtocolVersion(2) c.AutopilotInterval = 100 * time.Millisecond - //Let us know when a server is actually gone - ch := make(chan struct{}) - c.NotifyShutdown = func() { - t.Logf("%v is shutdown", c.NodeName) - close(ch) - } - closeMap[c.NodeName] = ch } dir1, s1 := testServerWithConfig(t, conf) defer os.RemoveAll(dir1) @@ -463,8 +455,7 @@ func TestAutopilot_MinQuorum(t *testing.T) { if dead == nil { t.Fatalf("no members set") } - dead.Shutdown() - <-closeMap[dead.config.NodeName] + require.NoError(t, dead.Shutdown()) retry.Run(t, func(r *retry.R) { leader := findStatus(true) if leader == nil { @@ -480,10 +471,7 @@ func TestAutopilot_MinQuorum(t *testing.T) { delete(servers, dead.config.NodeName) //Autopilot should not take this one into left dead = findStatus(false) - if err := dead.Shutdown(); err != nil { - t.Fatalf("could not shut down %s, error %v", dead.config.NodeName, err) - } - <-closeMap[dead.config.NodeName] + require.NoError(t, dead.Shutdown()) retry.Run(t, func(r *retry.R) { leader := findStatus(true) @@ -496,5 +484,4 @@ func TestAutopilot_MinQuorum(t *testing.T) { } } }) - } diff --git a/agent/consul/client_test.go b/agent/consul/client_test.go index 4ec005ecd3..8addc2e087 100644 --- a/agent/consul/client_test.go +++ b/agent/consul/client_test.go @@ -23,18 +23,15 @@ import ( func testClientConfig(t *testing.T) (string, *Config) { dir := testutil.TempDir(t, "consul") + t.Cleanup(func() { + os.RemoveAll(dir) + }) config := DefaultConfig() ports := freeport.MustTake(2) - - returnPortsFn := func() { - // The method of plumbing this into the client shutdown hook doesn't - // cover all exit points, so we insulate this against multiple - // invocations and then it's safe to call it a bunch of times. + t.Cleanup(func() { freeport.Return(ports) - config.NotifyShutdown = nil // self-erasing - } - config.NotifyShutdown = returnPortsFn + }) config.Datacenter = "dc1" config.DataDir = dir @@ -82,9 +79,6 @@ func testClientWithConfigWithErr(t *testing.T, cb func(c *Config)) (string, *Cli } client, err := NewClient(config, WithLogger(logger), WithTLSConfigurator(tlsConf)) - if err != nil { - config.NotifyShutdown() - } return dir, client, err } @@ -446,7 +440,6 @@ func TestClient_RPC_TLS(t *testing.T) { defer s1.Shutdown() dir2, conf2 := testClientConfig(t) - defer conf2.NotifyShutdown() conf2.VerifyOutgoing = true configureTLS(conf2) c1, err := newClient(t, conf2) @@ -503,7 +496,6 @@ func TestClient_RPC_RateLimit(t *testing.T) { testrpc.WaitForLeader(t, s1.RPC, "dc1") dir2, conf2 := testClientConfig(t) - defer conf2.NotifyShutdown() conf2.RPCRate = 2 conf2.RPCMaxBurst = 2 c1, err := newClient(t, conf2) @@ -571,7 +563,6 @@ func TestClient_SnapshotRPC_RateLimit(t *testing.T) { testrpc.WaitForLeader(t, s1.RPC, "dc1") dir2, conf1 := testClientConfig(t) - defer conf1.NotifyShutdown() conf1.RPCRate = 2 conf1.RPCMaxBurst = 2 c1, err := newClient(t, conf1) @@ -614,7 +605,6 @@ func TestClient_SnapshotRPC_TLS(t *testing.T) { defer s1.Shutdown() dir2, conf2 := testClientConfig(t) - defer conf2.NotifyShutdown() conf2.VerifyOutgoing = true configureTLS(conf2) c1, err := newClient(t, conf2) diff --git a/agent/consul/config.go b/agent/consul/config.go index ebfca98249..a48effe441 100644 --- a/agent/consul/config.go +++ b/agent/consul/config.go @@ -119,9 +119,6 @@ type Config struct { // configured at this point. NotifyListen func() - // NotifyShutdown is called after Server is completely Shutdown. - NotifyShutdown func() - // RPCAddr is the RPC address used by Consul. This should be reachable // by the WAN and LAN RPCAddr *net.TCPAddr diff --git a/agent/consul/server.go b/agent/consul/server.go index 5ba45d71ce..56d2a557ac 100644 --- a/agent/consul/server.go +++ b/agent/consul/server.go @@ -933,10 +933,6 @@ func (s *Server) Shutdown() error { s.acls.Close() } - if s.config.NotifyShutdown != nil { - s.config.NotifyShutdown() - } - if s.fsm != nil { s.fsm.State().Abandon() } diff --git a/agent/consul/server_test.go b/agent/consul/server_test.go index 18d61618f0..037730b70b 100644 --- a/agent/consul/server_test.go +++ b/agent/consul/server_test.go @@ -129,18 +129,15 @@ func waitForLeaderEstablishment(t *testing.T, servers ...*Server) { func testServerConfig(t *testing.T) (string, *Config) { dir := testutil.TempDir(t, "consul") + t.Cleanup(func() { + os.RemoveAll(dir) + }) config := DefaultConfig() ports := freeport.MustTake(3) - - returnPortsFn := func() { - // The method of plumbing this into the server shutdown hook doesn't - // cover all exit points, so we insulate this against multiple - // invocations and then it's safe to call it a bunch of times. + t.Cleanup(func() { freeport.Return(ports) - config.NotifyShutdown = nil // self-erasing - } - config.NotifyShutdown = returnPortsFn + }) config.NodeName = uniqueNodeName(t.Name()) config.Bootstrap = true @@ -154,7 +151,6 @@ func testServerConfig(t *testing.T) (string, *Config) { nodeID, err := uuid.GenerateUUID() if err != nil { - returnPortsFn() t.Fatal(err) } config.NodeID = types.NodeID(nodeID) @@ -211,9 +207,6 @@ func testServerConfig(t *testing.T) (string, *Config) { "IntermediateCertTTL": "288h", }, } - - config.NotifyShutdown = returnPortsFn - return dir, config } @@ -270,8 +263,6 @@ func testServerWithConfig(t *testing.T, cb func(*Config)) (string, *Server) { var err error srv, err = newServer(t, config) if err != nil { - config.NotifyShutdown() - os.RemoveAll(dir) r.Fatalf("err: %v", err) } })