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

@@ -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) {
 			}
 		}
 	})
-
 }

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)

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

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()
 	}

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)
 		}
 	})