Improve resilience of api pkg tests (#4676)

* Add function to wait for serfHealth in api tests

* Disable connect when creating semaphore test clients

* Wait for serfHealth when creating sessions in their tests

* Add helper functions to create lock/semaphore sessions without checks

* Log passing tests to prevent timeout in Travis due to lack of output

GNUmakefile

@@ -162,7 +162,7 @@ test-internal:
 	@# hide it from travis as it exceeds their log limits and causes job to be
 	@# terminated (over 4MB and over 10k lines in the UI). We need to output
 	@# _something_ to stop them terminating us due to inactivity...
-	{ go test $(GOTEST_FLAGS) -tags '$(GOTAGS)' $(GOTEST_PKGS) 2>&1 ; echo $$? > exit-code ; } | tee test.log | egrep '^(ok|FAIL|panic:|--- FAIL)'
+	{ go test -v $(GOTEST_FLAGS) -tags '$(GOTAGS)' $(GOTEST_PKGS) 2>&1 ; echo $$? > exit-code ; } | tee test.log | egrep '^(ok|FAIL|panic:|--- FAIL|--- PASS)'
 	@echo "Exit code: $$(cat exit-code)"
 	@# This prints all the race report between ====== lines
 	@awk '/^WARNING: DATA RACE/ {do_print=1; print "=================="} do_print==1 {print} /^={10,}/ {do_print=0}' test.log || true

api/lock_test.go

@@ -13,18 +13,44 @@ import (
 	"github.com/hashicorp/consul/testutil/retry"
 )
 
+func createTestLock(t *testing.T, c *Client, key string) (*Lock, *Session) {
+	t.Helper()
+	session := c.Session()
+
+	se := &SessionEntry{
+		Name:     DefaultLockSessionName,
+		TTL:      DefaultLockSessionTTL,
+		Behavior: SessionBehaviorDelete,
+	}
+	id, _, err := session.CreateNoChecks(se, nil)
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+
+	opts := &LockOptions{
+		Key:         key,
+		Session:     id,
+		SessionName: se.Name,
+		SessionTTL:  se.TTL,
+	}
+	lock, err := c.LockOpts(opts)
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+
+	return lock, session
+}
+
 func TestAPI_LockLockUnlock(t *testing.T) {
 	t.Parallel()
 	c, s := makeClientWithoutConnect(t)
 	defer s.Stop()
 
-	lock, err := c.LockKey("test/lock")
+	lock, session := createTestLock(t, c, "test/lock")
-	if err != nil {
+	defer session.Destroy(lock.opts.Session, nil)
-		t.Fatalf("err: %v", err)
-	}
 
 	// Initial unlock should fail
-	err = lock.Unlock()
+	err := lock.Unlock()
 	if err != ErrLockNotHeld {
 		t.Fatalf("err: %v", err)
 	}
@@ -77,10 +103,8 @@ func TestAPI_LockForceInvalidate(t *testing.T) {
 	defer s.Stop()
 
 	retry.Run(t, func(r *retry.R) {
-		lock, err := c.LockKey("test/lock")
+		lock, session := createTestLock(t, c, "test/lock")
-		if err != nil {
+		defer session.Destroy(lock.opts.Session, nil)
-			t.Fatalf("err: %v", err)
-		}
 
 		// Should work
 		leaderCh, err := lock.Lock(nil)
@@ -119,10 +143,8 @@ func TestAPI_LockDeleteKey(t *testing.T) {
 	// territory.
 	for i := 0; i < 10; i++ {
 		func() {
-			lock, err := c.LockKey("test/lock")
+			lock, session := createTestLock(t, c, "test/lock")
-			if err != nil {
+			defer session.Destroy(lock.opts.Session, nil)
-				t.Fatalf("err: %v", err)
-			}
 
 			// Should work
 			leaderCh, err := lock.Lock(nil)
@@ -161,10 +183,8 @@ func TestAPI_LockContend(t *testing.T) {
 		wg.Add(1)
 		go func(idx int) {
 			defer wg.Done()
-			lock, err := c.LockKey("test/lock")
+			lock, session := createTestLock(t, c, "test/lock")
-			if err != nil {
+			defer session.Destroy(lock.opts.Session, nil)
-				t.Fatalf("err: %v", err)
-			}
 
 			// Should work eventually, will contend
 			leaderCh, err := lock.Lock(nil)
@@ -208,10 +228,8 @@ func TestAPI_LockDestroy(t *testing.T) {
 	c, s := makeClientWithoutConnect(t)
 	defer s.Stop()
 
-	lock, err := c.LockKey("test/lock")
+	lock, session := createTestLock(t, c, "test/lock")
-	if err != nil {
+	defer session.Destroy(lock.opts.Session, nil)
-		t.Fatalf("err: %v", err)
-	}
 
 	// Should work
 	leaderCh, err := lock.Lock(nil)
@@ -234,10 +252,8 @@ func TestAPI_LockDestroy(t *testing.T) {
 	}
 
 	// Acquire with a different lock
-	l2, err := c.LockKey("test/lock")
+	l2, session := createTestLock(t, c, "test/lock")
-	if err != nil {
+	defer session.Destroy(lock.opts.Session, nil)
-		t.Fatalf("err: %v", err)
-	}
 
 	// Should work
 	leaderCh, err = l2.Lock(nil)
@@ -277,10 +293,8 @@ func TestAPI_LockConflict(t *testing.T) {
 	c, s := makeClientWithoutConnect(t)
 	defer s.Stop()
 
-	sema, err := c.SemaphorePrefix("test/lock/", 2)
+	sema, session := createTestSemaphore(t, c, "test/lock/", 2)
-	if err != nil {
+	defer session.Destroy(sema.opts.Session, nil)
-		t.Fatalf("err: %v", err)
-	}
 
 	// Should work
 	lockCh, err := sema.Acquire(nil)
@@ -292,10 +306,8 @@ func TestAPI_LockConflict(t *testing.T) {
 	}
 	defer sema.Release()
 
-	lock, err := c.LockKey("test/lock/.lock")
+	lock, session := createTestLock(t, c, "test/lock/.lock")
-	if err != nil {
+	defer session.Destroy(lock.opts.Session, nil)
-		t.Fatalf("err: %v", err)
-	}
 
 	// Should conflict with semaphore
 	_, err = lock.Lock(nil)
@@ -315,6 +327,8 @@ func TestAPI_LockReclaimLock(t *testing.T) {
 	c, s := makeClientWithoutConnect(t)
 	defer s.Stop()
 
+	s.WaitForSerfCheck(t)
+
 	session, _, err := c.Session().Create(&SessionEntry{}, nil)
 	if err != nil {
 		t.Fatalf("err: %v", err)
@@ -383,6 +397,8 @@ func TestAPI_LockMonitorRetry(t *testing.T) {
 	raw, s := makeClientWithoutConnect(t)
 	defer s.Stop()
 
+	s.WaitForSerfCheck(t)
+
 	// Set up a server that always responds with 500 errors.
 	failer := func(w http.ResponseWriter, req *http.Request) {
 		w.WriteHeader(500)
@@ -498,6 +514,8 @@ func TestAPI_LockOneShot(t *testing.T) {
 	c, s := makeClientWithoutConnect(t)
 	defer s.Stop()
 
+	s.WaitForSerfCheck(t)
+
 	// Set up a lock as a one-shot.
 	opts := &LockOptions{
 		Key:         "test/lock",

api/semaphore_test.go

@@ -11,18 +11,45 @@ import (
 	"time"
 )
 
+func createTestSemaphore(t *testing.T, c *Client, prefix string, limit int) (*Semaphore, *Session) {
+	t.Helper()
+	session := c.Session()
+
+	se := &SessionEntry{
+		Name:     DefaultSemaphoreSessionName,
+		TTL:      DefaultSemaphoreSessionTTL,
+		Behavior: SessionBehaviorDelete,
+	}
+	id, _, err := session.CreateNoChecks(se, nil)
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+
+	opts := &SemaphoreOptions{
+		Prefix:      prefix,
+		Limit:       limit,
+		Session:     id,
+		SessionName: se.Name,
+		SessionTTL:  se.TTL,
+	}
+	sema, err := c.SemaphoreOpts(opts)
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+
+	return sema, session
+}
+
 func TestAPI_SemaphoreAcquireRelease(t *testing.T) {
 	t.Parallel()
 	c, s := makeClient(t)
 	defer s.Stop()
 
-	sema, err := c.SemaphorePrefix("test/semaphore", 2)
+	sema, session := createTestSemaphore(t, c, "test/semaphore", 2)
-	if err != nil {
+	defer session.Destroy(sema.opts.Session, nil)
-		t.Fatalf("err: %v", err)
-	}
 
 	// Initial release should fail
-	err = sema.Release()
+	err := sema.Release()
 	if err != ErrSemaphoreNotHeld {
 		t.Fatalf("err: %v", err)
 	}
@@ -74,10 +101,8 @@ func TestAPI_SemaphoreForceInvalidate(t *testing.T) {
 	c, s := makeClient(t)
 	defer s.Stop()
 
-	sema, err := c.SemaphorePrefix("test/semaphore", 2)
+	sema, session := createTestSemaphore(t, c, "test/semaphore", 2)
-	if err != nil {
+	defer session.Destroy(sema.opts.Session, nil)
-		t.Fatalf("err: %v", err)
-	}
 
 	// Should work
 	lockCh, err := sema.Acquire(nil)
@@ -109,10 +134,8 @@ func TestAPI_SemaphoreDeleteKey(t *testing.T) {
 	c, s := makeClient(t)
 	defer s.Stop()
 
-	sema, err := c.SemaphorePrefix("test/semaphore", 2)
+	sema, session := createTestSemaphore(t, c, "test/semaphore", 2)
-	if err != nil {
+	defer session.Destroy(sema.opts.Session, nil)
-		t.Fatalf("err: %v", err)
-	}
 
 	// Should work
 	lockCh, err := sema.Acquire(nil)
@@ -149,10 +172,8 @@ func TestAPI_SemaphoreContend(t *testing.T) {
 		wg.Add(1)
 		go func(idx int) {
 			defer wg.Done()
-			sema, err := c.SemaphorePrefix("test/semaphore", 2)
+			sema, session := createTestSemaphore(t, c, "test/semaphore", 2)
-			if err != nil {
+			defer session.Destroy(sema.opts.Session, nil)
-				t.Fatalf("err: %v", err)
-			}
 
 			// Should work eventually, will contend
 			lockCh, err := sema.Acquire(nil)
@@ -198,23 +219,19 @@ func TestAPI_SemaphoreBadLimit(t *testing.T) {
 
 	sema, err := c.SemaphorePrefix("test/semaphore", 0)
 	if err == nil {
-		t.Fatalf("should error")
+		t.Fatalf("should error, limit must be positive")
 	}
 
-	sema, err = c.SemaphorePrefix("test/semaphore", 1)
+	sema, session := createTestSemaphore(t, c, "test/semaphore", 1)
-	if err != nil {
+	defer session.Destroy(sema.opts.Session, nil)
-		t.Fatalf("err: %v", err)
-	}
 
 	_, err = sema.Acquire(nil)
 	if err != nil {
 		t.Fatalf("err: %v", err)
 	}
 
-	sema2, err := c.SemaphorePrefix("test/semaphore", 2)
+	sema2, session := createTestSemaphore(t, c, "test/semaphore", 2)
-	if err != nil {
+	defer session.Destroy(sema.opts.Session, nil)
-		t.Fatalf("err: %v", err)
-	}
 
 	_, err = sema2.Acquire(nil)
 	if err.Error() != "semaphore limit conflict (lock: 1, local: 2)" {
@@ -227,17 +244,13 @@ func TestAPI_SemaphoreDestroy(t *testing.T) {
 	c, s := makeClient(t)
 	defer s.Stop()
 
-	sema, err := c.SemaphorePrefix("test/semaphore", 2)
+	sema, session := createTestSemaphore(t, c, "test/semaphore", 2)
-	if err != nil {
+	defer session.Destroy(sema.opts.Session, nil)
-		t.Fatalf("err: %v", err)
-	}
 
-	sema2, err := c.SemaphorePrefix("test/semaphore", 2)
+	sema2, session := createTestSemaphore(t, c, "test/semaphore", 2)
-	if err != nil {
+	defer session.Destroy(sema.opts.Session, nil)
-		t.Fatalf("err: %v", err)
-	}
 
-	_, err = sema.Acquire(nil)
+	_, err := sema.Acquire(nil)
 	if err != nil {
 		t.Fatalf("err: %v", err)
 	}
@@ -283,10 +296,8 @@ func TestAPI_SemaphoreConflict(t *testing.T) {
 	c, s := makeClient(t)
 	defer s.Stop()
 
-	lock, err := c.LockKey("test/sema/.lock")
+	lock, session := createTestLock(t, c, "test/sema/.lock")
-	if err != nil {
+	defer session.Destroy(lock.opts.Session, nil)
-		t.Fatalf("err: %v", err)
-	}
 
 	// Should work
 	leaderCh, err := lock.Lock(nil)
@@ -298,10 +309,8 @@ func TestAPI_SemaphoreConflict(t *testing.T) {
 	}
 	defer lock.Unlock()
 
-	sema, err := c.SemaphorePrefix("test/sema/", 2)
+	sema, session := createTestSemaphore(t, c, "test/sema/", 2)
-	if err != nil {
+	defer session.Destroy(sema.opts.Session, nil)
-		t.Fatalf("err: %v", err)
-	}
 
 	// Should conflict with lock
 	_, err = sema.Acquire(nil)
@@ -321,6 +330,8 @@ func TestAPI_SemaphoreMonitorRetry(t *testing.T) {
 	raw, s := makeClient(t)
 	defer s.Stop()
 
+	s.WaitForSerfCheck(t)
+
 	// Set up a server that always responds with 500 errors.
 	failer := func(w http.ResponseWriter, req *http.Request) {
 		w.WriteHeader(500)
@@ -438,6 +449,8 @@ func TestAPI_SemaphoreOneShot(t *testing.T) {
 	c, s := makeClient(t)
 	defer s.Stop()
 
+	s.WaitForSerfCheck(t)
+
 	// Set up a semaphore as a one-shot.
 	opts := &SemaphoreOptions{
 		Prefix:           "test/sema/.lock",

api/session_test.go

@@ -14,6 +14,8 @@ func TestAPI_SessionCreateDestroy(t *testing.T) {
 	c, s := makeClient(t)
 	defer s.Stop()
 
+	s.WaitForSerfCheck(t)
+
 	session := c.Session()
 
 	id, meta, err := session.Create(nil, nil)
@@ -44,6 +46,8 @@ func TestAPI_SessionCreateRenewDestroy(t *testing.T) {
 	c, s := makeClient(t)
 	defer s.Stop()
 
+	s.WaitForSerfCheck(t)
+
 	session := c.Session()
 
 	se := &SessionEntry{
@@ -95,6 +99,8 @@ func TestAPI_SessionCreateRenewDestroyRenew(t *testing.T) {
 	c, s := makeClient(t)
 	defer s.Stop()
 
+	s.WaitForSerfCheck(t)
+
 	session := c.Session()
 
 	entry := &SessionEntry{
@@ -149,6 +155,8 @@ func TestAPI_SessionCreateDestroyRenewPeriodic(t *testing.T) {
 	c, s := makeClient(t)
 	defer s.Stop()
 
+	s.WaitForSerfCheck(t)
+
 	session := c.Session()
 
 	entry := &SessionEntry{
@@ -203,6 +211,8 @@ func TestAPI_SessionRenewPeriodic_Cancel(t *testing.T) {
 	c, s := makeClient(t)
 	defer s.Stop()
 
+	s.WaitForSerfCheck(t)
+
 	session := c.Session()
 	entry := &SessionEntry{
 		Behavior: SessionBehaviorDelete,
@@ -279,6 +289,8 @@ func TestAPI_SessionInfo(t *testing.T) {
 	c, s := makeClient(t)
 	defer s.Stop()
 
+	s.WaitForSerfCheck(t)
+
 	session := c.Session()
 
 	id, _, err := session.Create(nil, nil)
@@ -358,6 +370,8 @@ func TestAPI_SessionNode(t *testing.T) {
 	c, s := makeClient(t)
 	defer s.Stop()
 
+	s.WaitForSerfCheck(t)
+
 	session := c.Session()
 
 	id, _, err := session.Create(nil, nil)
@@ -393,6 +407,8 @@ func TestAPI_SessionList(t *testing.T) {
 	c, s := makeClient(t)
 	defer s.Stop()
 
+	s.WaitForSerfCheck(t)
+
 	session := c.Session()
 
 	id, _, err := session.Create(nil, nil)

testutil/server.go

@@ -395,3 +395,56 @@ func (s *TestServer) waitForLeader() error {
 	}
 	return nil
 }
+
+// WaitForSerfCheck ensures we have a node with serfHealth check registered
+// Behavior mirrors testrpc.WaitForTestAgent but avoids the dependency cycle in api pkg
+func (s *TestServer) WaitForSerfCheck(t *testing.T) {
+	retry.Run(t, func(r *retry.R) {
+		// Query the API and check the status code.
+		url := s.url("/v1/catalog/nodes?index=0")
+		resp, err := s.HTTPClient.Get(url)
+		if err != nil {
+			r.Fatal("failed http get", err)
+		}
+		defer resp.Body.Close()
+		if err := s.requireOK(resp); err != nil {
+			r.Fatal("failed OK response", err)
+		}
+
+		// Watch for the anti-entropy sync to finish.
+		var payload []map[string]interface{}
+		dec := json.NewDecoder(resp.Body)
+		if err := dec.Decode(&payload); err != nil {
+			r.Fatal(err)
+		}
+		if len(payload) < 1 {
+			r.Fatal("No nodes")
+		}
+
+		// Ensure the serfHealth check is registered
+		url = s.url(fmt.Sprintf("/v1/health/node/%s", payload[0]["Node"]))
+		resp, err = s.HTTPClient.Get(url)
+		if err != nil {
+			r.Fatal("failed http get", err)
+		}
+		defer resp.Body.Close()
+		if err := s.requireOK(resp); err != nil {
+			r.Fatal("failed OK response", err)
+		}
+		dec = json.NewDecoder(resp.Body)
+		if err = dec.Decode(&payload); err != nil {
+			r.Fatal(err)
+		}
+
+		var found bool
+		for _, check := range payload {
+			if check["CheckID"].(string) == "serfHealth" {
+				found = true
+				break
+			}
+		}
+		if !found {
+			r.Fatal("missing serfHealth registration")
+		}
+	})
+}