From d5179becf64b36319404c57bb6cf700abfe91194 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 14 Aug 2020 16:33:16 -0400 Subject: [PATCH] Merge pull request #8509 from hashicorp/dnephin/use-t.cleanup-in-testagent testing: Use t.cleanup in TestAgent , and fix two flaky tests --- agent/acl_test.go | 36 +++++--------- agent/agent_endpoint_test.go | 9 ++-- agent/agent_test.go | 39 ++++----------- agent/dns_test.go | 6 +-- agent/keyring_test.go | 30 +++++++++-- agent/service_manager_test.go | 17 ++----- agent/testagent.go | 93 +++++------------------------------ sdk/testutil/io.go | 17 +++++-- 8 files changed, 82 insertions(+), 165 deletions(-) diff --git a/agent/acl_test.go b/agent/acl_test.go index 9f36be512f..f511001e9f 100644 --- a/agent/acl_test.go +++ b/agent/acl_test.go @@ -3,6 +3,7 @@ package agent import ( "fmt" "io" + "os" "testing" "time" @@ -25,23 +26,6 @@ type authzResolver func(string) (structs.ACLIdentity, acl.Authorizer, error) type identResolver func(string) (structs.ACLIdentity, error) type TestACLAgent struct { - // Name is an optional name of the agent. - Name string - - HCL string - - // Config is the agent configuration. If Config is nil then - // TestConfig() is used. If Config.DataDir is set then it is - // the callers responsibility to clean up the data directory. - // Otherwise, a temporary data directory is created and removed - // when Shutdown() is called. - Config *config.RuntimeConfig - - // DataDir is the data directory which is used when Config.DataDir - // is not set. It is created automatically and removed when - // Shutdown() is called. - DataDir string - resolveAuthzFn authzResolver resolveIdentFn identResolver @@ -52,11 +36,15 @@ type TestACLAgent struct { // Basically it needs a local state for some of the vet* functions, a logger and a delegate. // The key is that we are the delegate so we can control the ResolveToken responses func NewTestACLAgent(t *testing.T, name string, hcl string, resolveAuthz authzResolver, resolveIdent identResolver) *TestACLAgent { - a := &TestACLAgent{Name: name, HCL: hcl, resolveAuthzFn: resolveAuthz, resolveIdentFn: resolveIdent} - dataDir := `data_dir = "acl-agent"` + a := &TestACLAgent{resolveAuthzFn: resolveAuthz, resolveIdentFn: resolveIdent} + + dataDir := testutil.TempDir(t, "acl-agent") + t.Cleanup(func() { + os.RemoveAll(dataDir) + }) logger := hclog.NewInterceptLogger(&hclog.LoggerOptions{ - Name: a.Name, + Name: name, Level: hclog.Debug, Output: testutil.NewLogBuffer(t), }) @@ -66,22 +54,22 @@ func NewTestACLAgent(t *testing.T, name string, hcl string, resolveAuthz authzRe WithBuilderOpts(config.BuilderOpts{ HCL: []string{ TestConfigHCL(NodeID()), - a.HCL, - dataDir, + hcl, + fmt.Sprintf(`data_dir = "%s"`, dataDir), }, }), } agent, err := New(opts...) require.NoError(t, err) - a.Config = agent.GetConfig() + cfg := agent.GetConfig() a.Agent = agent agent.logger = logger agent.MemSink = metrics.NewInmemSink(1*time.Second, time.Minute) a.Agent.delegate = a - a.Agent.State = local.NewState(LocalConfig(a.Config), a.Agent.logger, a.Agent.tokens) + a.Agent.State = local.NewState(LocalConfig(cfg), logger, a.Agent.tokens) a.Agent.State.TriggerSyncChanges = func() {} return a } diff --git a/agent/agent_endpoint_test.go b/agent/agent_endpoint_test.go index 0a2821b72e..c1743f9f59 100644 --- a/agent/agent_endpoint_test.go +++ b/agent/agent_endpoint_test.go @@ -4504,12 +4504,15 @@ func TestAgent_Monitor(t *testing.T) { req = req.WithContext(cancelCtx) resp := httptest.NewRecorder() - errCh := make(chan error) + chErr := make(chan error) + chStarted := make(chan struct{}) go func() { + close(chStarted) _, err := a.srv.AgentMonitor(resp, req) - errCh <- err + chErr <- err }() + <-chStarted require.NoError(t, a.Shutdown()) // Wait until we have received some type of logging output @@ -4518,7 +4521,7 @@ func TestAgent_Monitor(t *testing.T) { }, 3*time.Second, 100*time.Millisecond) cancelFunc() - err := <-errCh + err := <-chErr require.NoError(t, err) got := resp.Body.String() diff --git a/agent/agent_test.go b/agent/agent_test.go index d2eeb8da5a..5143e0a613 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -1652,15 +1652,12 @@ func TestAgent_RestoreServiceWithAliasCheck(t *testing.T) { a.logger.Info("testharness: " + fmt.Sprintf(format, args...)) } - dataDir := testutil.TempDir(t, "agent") // we manage the data dir cfg := ` server = false bootstrap = false enable_central_service_config = false - data_dir = "` + dataDir + `" ` - a := StartTestAgent(t, TestAgent{HCL: cfg, DataDir: dataDir}) - defer os.RemoveAll(dataDir) + a := StartTestAgent(t, TestAgent{HCL: cfg}) defer a.Shutdown() testCtx, testCancel := context.WithCancel(context.Background()) @@ -1743,7 +1740,7 @@ node_name = "` + a.Config.NodeName + `" t.Helper() // Reload and retain former NodeID and data directory. - a2 := StartTestAgent(t, TestAgent{HCL: futureHCL, DataDir: dataDir}) + a2 := StartTestAgent(t, TestAgent{HCL: futureHCL, DataDir: a.DataDir}) defer a2.Shutdown() a = nil @@ -2128,15 +2125,11 @@ func TestAgent_PersistService(t *testing.T) { func testAgent_PersistService(t *testing.T, extraHCL string) { t.Helper() - dataDir := testutil.TempDir(t, "agent") // we manage the data dir - defer os.RemoveAll(dataDir) - cfg := ` server = false bootstrap = false - data_dir = "` + dataDir + `" ` + extraHCL - a := StartTestAgent(t, TestAgent{HCL: cfg, DataDir: dataDir}) + a := StartTestAgent(t, TestAgent{HCL: cfg}) defer a.Shutdown() svc := &structs.NodeService{ @@ -2202,7 +2195,7 @@ func testAgent_PersistService(t *testing.T, extraHCL string) { a.Shutdown() // Should load it back during later start - a2 := StartTestAgent(t, TestAgent{HCL: cfg, DataDir: dataDir}) + a2 := StartTestAgent(t, TestAgent{HCL: cfg, DataDir: a.DataDir}) defer a2.Shutdown() restored := a2.State.ServiceState(structs.NewServiceID(svc.ID, nil)) @@ -2340,15 +2333,11 @@ func TestAgent_PurgeServiceOnDuplicate(t *testing.T) { func testAgent_PurgeServiceOnDuplicate(t *testing.T, extraHCL string) { t.Helper() - dataDir := testutil.TempDir(t, "agent") // we manage the data dir - defer os.RemoveAll(dataDir) - cfg := ` - data_dir = "` + dataDir + `" server = false bootstrap = false ` + extraHCL - a := StartTestAgent(t, TestAgent{HCL: cfg, DataDir: dataDir}) + a := StartTestAgent(t, TestAgent{HCL: cfg}) defer a.Shutdown() svc1 := &structs.NodeService{ @@ -2371,7 +2360,7 @@ func testAgent_PurgeServiceOnDuplicate(t *testing.T, extraHCL string) { tags = ["bar"] port = 9000 } - `, DataDir: dataDir}) + `, DataDir: a.DataDir}) defer a2.Shutdown() sid := svc1.CompoundServiceID() @@ -2385,15 +2374,12 @@ func testAgent_PurgeServiceOnDuplicate(t *testing.T, extraHCL string) { func TestAgent_PersistCheck(t *testing.T) { t.Parallel() - dataDir := testutil.TempDir(t, "agent") // we manage the data dir cfg := ` - data_dir = "` + dataDir + `" server = false bootstrap = false enable_script_checks = true ` - a := StartTestAgent(t, TestAgent{HCL: cfg, DataDir: dataDir}) - defer os.RemoveAll(dataDir) + a := StartTestAgent(t, TestAgent{HCL: cfg}) defer a.Shutdown() check := &structs.HealthCheck{ @@ -2449,7 +2435,7 @@ func TestAgent_PersistCheck(t *testing.T) { a.Shutdown() // Should load it back during later start - a2 := StartTestAgent(t, TestAgent{Name: "Agent2", HCL: cfg, DataDir: dataDir}) + a2 := StartTestAgent(t, TestAgent{Name: "Agent2", HCL: cfg, DataDir: a.DataDir}) defer a2.Shutdown() result := requireCheckExists(t, a2, check.CheckID) @@ -2500,18 +2486,14 @@ func TestAgent_PurgeCheck(t *testing.T) { func TestAgent_PurgeCheckOnDuplicate(t *testing.T) { t.Parallel() nodeID := NodeID() - dataDir := testutil.TempDir(t, "agent") a := StartTestAgent(t, TestAgent{ - DataDir: dataDir, HCL: ` node_id = "` + nodeID + `" node_name = "Node ` + nodeID + `" - data_dir = "` + dataDir + `" server = false bootstrap = false enable_script_checks = true `}) - defer os.RemoveAll(dataDir) defer a.Shutdown() check1 := &structs.HealthCheck{ @@ -2531,11 +2513,10 @@ func TestAgent_PurgeCheckOnDuplicate(t *testing.T) { // Start again with the check registered in config a2 := StartTestAgent(t, TestAgent{ Name: "Agent2", - DataDir: dataDir, + DataDir: a.DataDir, HCL: ` node_id = "` + nodeID + `" node_name = "Node ` + nodeID + `" - data_dir = "` + dataDir + `" server = false bootstrap = false enable_script_checks = true @@ -2550,7 +2531,7 @@ func TestAgent_PurgeCheckOnDuplicate(t *testing.T) { defer a2.Shutdown() cid := check1.CompoundCheckID() - file := filepath.Join(dataDir, checksDir, cid.StringHash()) + file := filepath.Join(a.DataDir, checksDir, cid.StringHash()) if _, err := os.Stat(file); err == nil { t.Fatalf("should have removed persisted check") } diff --git a/agent/dns_test.go b/agent/dns_test.go index adf5b44a9f..2e7b904036 100644 --- a/agent/dns_test.go +++ b/agent/dns_test.go @@ -5830,16 +5830,12 @@ func TestDNS_NonExistentDC_RPC(t *testing.T) { server = false `) defer c.Shutdown() - testrpc.WaitForLeader(t, s.RPC, "dc1") // Join LAN cluster addr := fmt.Sprintf("127.0.0.1:%d", s.Config.SerfPortLAN) _, err := c.JoinLAN([]string{addr}) require.NoError(t, err) - retry.Run(t, func(r *retry.R) { - require.Len(r, s.LANMembers(), 2) - require.Len(r, c.LANMembers(), 2) - }) + testrpc.WaitForTestAgent(t, c.RPC, "dc1") m := new(dns.Msg) m.SetQuestion("consul.service.dc2.consul.", dns.TypeANY) diff --git a/agent/keyring_test.go b/agent/keyring_test.go index 2b160d4f78..16acc01611 100644 --- a/agent/keyring_test.go +++ b/agent/keyring_test.go @@ -54,7 +54,10 @@ func TestAgent_LoadKeyrings(t *testing.T) { // Server should auto-load LAN and WAN keyring files t.Run("server with keys", func(t *testing.T) { - a2 := StartTestAgent(t, TestAgent{Key: key}) + dataDir := testutil.TempDir(t, "keyfile") + writeKeyRings(t, key, dataDir) + + a2 := StartTestAgent(t, TestAgent{DataDir: dataDir}) defer a2.Shutdown() c2 := a2.consulConfig() @@ -80,10 +83,16 @@ func TestAgent_LoadKeyrings(t *testing.T) { // Client should auto-load only the LAN keyring file t.Run("client with keys", func(t *testing.T) { - a3 := StartTestAgent(t, TestAgent{HCL: ` + dataDir := testutil.TempDir(t, "keyfile") + writeKeyRings(t, key, dataDir) + + a3 := StartTestAgent(t, TestAgent{ + HCL: ` server = false bootstrap = false - `, Key: key}) + `, + DataDir: dataDir, + }) defer a3.Shutdown() c3 := a3.consulConfig() @@ -105,6 +114,16 @@ func TestAgent_LoadKeyrings(t *testing.T) { }) } +func writeKeyRings(t *testing.T, key string, dataDir string) { + t.Helper() + writeKey := func(key, filename string) { + path := filepath.Join(dataDir, filename) + require.NoError(t, initKeyring(path, key), "Error creating keyring %s", path) + } + writeKey(key, SerfLANKeyring) + writeKey(key, SerfWANKeyring) +} + func TestAgent_InmemKeyrings(t *testing.T) { t.Parallel() key := "tbLJg26ZJyJ9pK3qhc9jig==" @@ -272,11 +291,14 @@ func TestAgentKeyring_ACL(t *testing.T) { key1 := "tbLJg26ZJyJ9pK3qhc9jig==" key2 := "4leC33rgtXKIVUr9Nr0snQ==" + dataDir := testutil.TempDir(t, "keyfile") + writeKeyRings(t, key1, dataDir) + a := StartTestAgent(t, TestAgent{HCL: TestACLConfig() + ` acl_datacenter = "dc1" acl_master_token = "root" acl_default_policy = "deny" - `, Key: key1}) + `, DataDir: dataDir}) defer a.Shutdown() // List keys without access fails diff --git a/agent/service_manager_test.go b/agent/service_manager_test.go index e71e21f222..8cb3ce2a24 100644 --- a/agent/service_manager_test.go +++ b/agent/service_manager_test.go @@ -9,7 +9,6 @@ import ( "testing" "github.com/hashicorp/consul/agent/structs" - "github.com/hashicorp/consul/sdk/testutil" "github.com/hashicorp/consul/sdk/testutil/retry" "github.com/hashicorp/consul/testrpc" "github.com/stretchr/testify/require" @@ -293,16 +292,12 @@ func TestServiceManager_PersistService_API(t *testing.T) { ) // Now launch a single client agent - dataDir := testutil.TempDir(t, "agent") // we manage the data dir - defer os.RemoveAll(dataDir) - cfg := ` enable_central_service_config = true server = false bootstrap = false - data_dir = "` + dataDir + `" ` - a := StartTestAgent(t, TestAgent{HCL: cfg, DataDir: dataDir}) + a := StartTestAgent(t, TestAgent{HCL: cfg}) defer a.Shutdown() // Join first @@ -465,7 +460,7 @@ func TestServiceManager_PersistService_API(t *testing.T) { serverAgent.Shutdown() // Should load it back during later start. - a2 := StartTestAgent(t, TestAgent{HCL: cfg, DataDir: dataDir}) + a2 := StartTestAgent(t, TestAgent{HCL: cfg, DataDir: a.DataDir}) defer a2.Shutdown() { @@ -510,9 +505,6 @@ func TestServiceManager_PersistService_ConfigFiles(t *testing.T) { ) // Now launch a single client agent - dataDir := testutil.TempDir(t, "agent") // we manage the data dir - defer os.RemoveAll(dataDir) - serviceSnippet := ` service = { kind = "connect-proxy" @@ -535,12 +527,11 @@ func TestServiceManager_PersistService_ConfigFiles(t *testing.T) { cfg := ` enable_central_service_config = true - data_dir = "` + dataDir + `" server = false bootstrap = false ` + serviceSnippet - a := StartTestAgent(t, TestAgent{HCL: cfg, DataDir: dataDir}) + a := StartTestAgent(t, TestAgent{HCL: cfg}) defer a.Shutdown() // Join first @@ -639,7 +630,7 @@ func TestServiceManager_PersistService_ConfigFiles(t *testing.T) { serverAgent.Shutdown() // Should load it back during later start. - a2 := StartTestAgent(t, TestAgent{HCL: cfg, DataDir: dataDir}) + a2 := StartTestAgent(t, TestAgent{HCL: cfg, DataDir: a.DataDir}) defer a2.Shutdown() { diff --git a/agent/testagent.go b/agent/testagent.go index 27acb8e441..69c8538a3f 100644 --- a/agent/testagent.go +++ b/agent/testagent.go @@ -8,13 +8,10 @@ import ( "crypto/x509" "fmt" "io" - "io/ioutil" "math/rand" "net/http/httptest" - "os" "path/filepath" "strconv" - "strings" "testing" "text/template" "time" @@ -40,9 +37,6 @@ func init() { rand.Seed(time.Now().UnixNano()) // seed random number generator } -// TempDir defines the base dir for temporary directories. -var TempDir = os.TempDir() - // TestAgent encapsulates an Agent with a default configuration and // startup procedure suitable for testing. It panics if there are errors // during creation or startup instead of returning errors. It manages a @@ -60,22 +54,16 @@ type TestAgent struct { // when Shutdown() is called. Config *config.RuntimeConfig - // returnPortsFn will put the ports claimed for the test back into the - // general freeport pool - returnPortsFn func() - // LogOutput is the sink for the logs. If nil, logs are written // to os.Stderr. LogOutput io.Writer - // DataDir is the data directory which is used when Config.DataDir - // is not set. It is created automatically and removed when - // Shutdown() is called. + // DataDir may be set to a directory which exists. If is it not set, + // TestAgent.Start will create one and set DataDir to the directory path. + // In all cases the agent will be configured to use this path as the data directory, + // and the directory will be removed once the test ends. DataDir string - // Key is the optional encryption key for the LAN and WAN keyring. - Key string - // UseTLS, if true, will disable the HTTP port and enable the HTTPS // one. UseTLS bool @@ -155,35 +143,14 @@ func (a *TestAgent) Start(t *testing.T) (err error) { name = "TestAgent" } - var cleanupTmpDir = func() { - // Clean out the data dir if we are responsible for it before we - // try again, since the old ports may have gotten written to - // the data dir, such as in the Raft configuration. - if a.DataDir != "" { - if err := os.RemoveAll(a.DataDir); err != nil { - fmt.Printf("%s Error resetting data dir: %s", name, err) - } - } - } - - var hclDataDir string if a.DataDir == "" { - dirname := "agent" - if name != "" { - dirname = name + "-agent" - } - dirname = strings.Replace(dirname, "/", "_", -1) - d, err := ioutil.TempDir(TempDir, dirname) - if err != nil { - return fmt.Errorf("Error creating data dir %s: %s", filepath.Join(TempDir, dirname), err) - } - // Convert windows style path to posix style path - // to avoid illegal char escape error when hcl - // parsing. - d = filepath.ToSlash(d) - hclDataDir = `data_dir = "` + d + `"` - a.DataDir = d + dirname := name + "-agent" + a.DataDir = testutil.TempDir(t, dirname) } + // Convert windows style path to posix style path to avoid illegal char escape + // error when hcl parsing. + d := filepath.ToSlash(a.DataDir) + hclDataDir := fmt.Sprintf(`data_dir = "%s"`, d) logOutput := a.LogOutput if logOutput == nil { @@ -198,7 +165,7 @@ func (a *TestAgent) Start(t *testing.T) (err error) { }) portsConfig, returnPortsFn := randomPortsSource(a.UseTLS) - a.returnPortsFn = returnPortsFn + t.Cleanup(returnPortsFn) nodeID := NodeID() @@ -221,36 +188,8 @@ func (a *TestAgent) Start(t *testing.T) (err error) { ), } - defer func() { - if err != nil && a.returnPortsFn != nil { - a.returnPortsFn() - a.returnPortsFn = nil - } - }() - - // write the keyring - if a.Key != "" { - writeKey := func(key, filename string) error { - path := filepath.Join(a.DataDir, filename) - if err := initKeyring(path, key); err != nil { - cleanupTmpDir() - return fmt.Errorf("Error creating keyring %s: %s", path, err) - } - return nil - } - if err = writeKey(a.Key, SerfLANKeyring); err != nil { - cleanupTmpDir() - return err - } - if err = writeKey(a.Key, SerfWANKeyring); err != nil { - cleanupTmpDir() - return err - } - } - agent, err := New(opts...) if err != nil { - cleanupTmpDir() return fmt.Errorf("Error creating agent: %s", err) } @@ -261,7 +200,6 @@ func (a *TestAgent) Start(t *testing.T) (err error) { id := string(a.Config.NodeID) if err := agent.Start(context.Background()); err != nil { - cleanupTmpDir() agent.ShutdownAgent() agent.ShutdownEndpoints() @@ -274,7 +212,6 @@ func (a *TestAgent) Start(t *testing.T) (err error) { a.Agent.StartSync() if err := a.waitForUp(); err != nil { - cleanupTmpDir() a.Shutdown() t.Logf("Error while waiting for test agent to start: %v", err) return errwrap.Wrapf(name+": {{err}}", err) @@ -357,14 +294,6 @@ func (a *TestAgent) Shutdown() error { return nil } - // Return ports last of all - defer func() { - if a.returnPortsFn != nil { - a.returnPortsFn() - a.returnPortsFn = nil - } - }() - // shutdown agent before endpoints defer a.Agent.ShutdownEndpoints() if err := a.Agent.ShutdownAgent(); err != nil { diff --git a/sdk/testutil/io.go b/sdk/testutil/io.go index a137fc6a3f..28f215b677 100644 --- a/sdk/testutil/io.go +++ b/sdk/testutil/io.go @@ -29,21 +29,28 @@ func init() { } } +var noCleanup = strings.ToLower(os.Getenv("TEST_NOCLEANUP")) == "true" + // TempDir creates a temporary directory within tmpdir // with the name 'testname-name'. If the directory cannot // be created t.Fatal is called. func TempDir(t *testing.T, name string) string { - if t != nil && t.Name() != "" { - name = t.Name() + "-" + name + if t == nil { + panic("argument t must be non-nil") } + name = t.Name() + "-" + name name = strings.Replace(name, "/", "_", -1) d, err := ioutil.TempDir(tmpdir, name) if err != nil { - if t == nil { - panic(err) - } t.Fatalf("err: %s", err) } + t.Cleanup(func() { + if noCleanup { + t.Logf("skipping cleanup because TEST_NOCLEANUP was enabled") + return + } + os.RemoveAll(d) + }) return d }