From d157d931adb44e6d7009cab6191412f786ec4645 Mon Sep 17 00:00:00 2001 From: "Chris S. Kim" Date: Thu, 5 Oct 2023 16:01:16 -0400 Subject: [PATCH 1/3] Retry tests --- agent/metrics_test.go | 69 +++++++++++++++++++------------------------ 1 file changed, 30 insertions(+), 39 deletions(-) diff --git a/agent/metrics_test.go b/agent/metrics_test.go index fd4f9f8d2e1..97286cdde98 100644 --- a/agent/metrics_test.go +++ b/agent/metrics_test.go @@ -20,6 +20,7 @@ import ( "github.com/hashicorp/consul/agent/rpc/middleware" "github.com/hashicorp/consul/lib/retry" "github.com/hashicorp/consul/sdk/testutil" + testretry "github.com/hashicorp/consul/sdk/testutil/retry" "github.com/hashicorp/consul/testrpc" "github.com/hashicorp/consul/tlsutil" ) @@ -30,8 +31,7 @@ func skipIfShortTesting(t *testing.T) { } } -func recordPromMetrics(t *testing.T, a *TestAgent, respRec *httptest.ResponseRecorder) { - t.Helper() +func recordPromMetrics(t require.TestingT, a *TestAgent, respRec *httptest.ResponseRecorder) { req, err := http.NewRequest("GET", "/v1/agent/metrics?format=prometheus", nil) require.NoError(t, err, "Failed to generate new http request.") @@ -483,28 +483,24 @@ func TestHTTPHandlers_AgentMetrics_WAL_Prometheus(t *testing.T) { defer a.Shutdown() testrpc.WaitForLeader(t, a.RPC, "dc1") - respRec := httptest.NewRecorder() - recordPromMetrics(t, a, respRec) + testretry.Run(t, func(r *testretry.R) { + respRec := httptest.NewRecorder() + recordPromMetrics(r, a, respRec) + + out := respRec.Body.String() + require.Contains(r, out, "agent_5_raft_wal_head_truncations") + require.Contains(r, out, "agent_5_raft_wal_last_segment_age_seconds") + require.Contains(r, out, "agent_5_raft_wal_log_appends") + require.Contains(r, out, "agent_5_raft_wal_log_entries_read") + require.Contains(r, out, "agent_5_raft_wal_log_entries_written") + require.Contains(r, out, "agent_5_raft_wal_log_entry_bytes_read") + require.Contains(r, out, "agent_5_raft_wal_log_entry_bytes_written") + require.Contains(r, out, "agent_5_raft_wal_segment_rotations") + require.Contains(r, out, "agent_5_raft_wal_stable_gets") + require.Contains(r, out, "agent_5_raft_wal_stable_sets") + require.Contains(r, out, "agent_5_raft_wal_tail_truncations") + }) - out := respRec.Body.String() - defer func() { - if t.Failed() { - t.Log("--- Failed output START ---") - t.Log(out) - t.Log("--- Failed output END ---") - } - }() - require.Contains(t, out, "agent_5_raft_wal_head_truncations") - require.Contains(t, out, "agent_5_raft_wal_last_segment_age_seconds") - require.Contains(t, out, "agent_5_raft_wal_log_appends") - require.Contains(t, out, "agent_5_raft_wal_log_entries_read") - require.Contains(t, out, "agent_5_raft_wal_log_entries_written") - require.Contains(t, out, "agent_5_raft_wal_log_entry_bytes_read") - require.Contains(t, out, "agent_5_raft_wal_log_entry_bytes_written") - require.Contains(t, out, "agent_5_raft_wal_segment_rotations") - require.Contains(t, out, "agent_5_raft_wal_stable_gets") - require.Contains(t, out, "agent_5_raft_wal_stable_sets") - require.Contains(t, out, "agent_5_raft_wal_tail_truncations") }) t.Run("server without WAL enabled emits no WAL metrics", func(t *testing.T) { @@ -590,22 +586,17 @@ func TestHTTPHandlers_AgentMetrics_LogVerifier_Prometheus(t *testing.T) { defer a.Shutdown() testrpc.WaitForLeader(t, a.RPC, "dc1") - respRec := httptest.NewRecorder() - recordPromMetrics(t, a, respRec) - - out := respRec.Body.String() - defer func() { - if t.Failed() { - t.Log("--- Failed output START ---") - t.Log(out) - t.Log("--- Failed output END ---") - } - }() - require.Contains(t, out, "agent_5_raft_logstore_verifier_checkpoints_written") - require.Contains(t, out, "agent_5_raft_logstore_verifier_dropped_reports") - require.Contains(t, out, "agent_5_raft_logstore_verifier_ranges_verified") - require.Contains(t, out, "agent_5_raft_logstore_verifier_read_checksum_failures") - require.Contains(t, out, "agent_5_raft_logstore_verifier_write_checksum_failures") + testretry.Run(t, func(r *testretry.R) { + respRec := httptest.NewRecorder() + recordPromMetrics(r, a, respRec) + + out := respRec.Body.String() + require.Contains(r, out, "agent_5_raft_logstore_verifier_checkpoints_written") + require.Contains(r, out, "agent_5_raft_logstore_verifier_dropped_reports") + require.Contains(r, out, "agent_5_raft_logstore_verifier_ranges_verified") + require.Contains(r, out, "agent_5_raft_logstore_verifier_read_checksum_failures") + require.Contains(r, out, "agent_5_raft_logstore_verifier_write_checksum_failures") + }) }) t.Run("server with verifier disabled emits no extra metrics", func(t *testing.T) { From 9da2d0cd6b2dadd2c2dfec01fa2f812a340bec94 Mon Sep 17 00:00:00 2001 From: "Chris S. Kim" Date: Thu, 5 Oct 2023 16:09:40 -0400 Subject: [PATCH 2/3] Retry tests --- agent/health_endpoint_test.go | 26 ++++++++++++++------------ agent/http_test.go | 6 ++++-- agent/metrics_test.go | 4 ++++ 3 files changed, 22 insertions(+), 14 deletions(-) diff --git a/agent/health_endpoint_test.go b/agent/health_endpoint_test.go index 2be4f8b1abd..aedef6043d8 100644 --- a/agent/health_endpoint_test.go +++ b/agent/health_endpoint_test.go @@ -442,19 +442,21 @@ func TestHealthServiceChecks_NodeMetaFilter(t *testing.T) { t.Fatalf("err: %v", err) } - req, _ = http.NewRequest("GET", "/v1/health/checks/consul?dc=dc1&node-meta=somekey:somevalue", nil) - resp = httptest.NewRecorder() - obj, err = a.srv.HealthServiceChecks(resp, req) - if err != nil { - t.Fatalf("err: %v", err) - } - assertIndex(t, resp) + retry.Run(t, func(r *retry.R) { + req, _ = http.NewRequest("GET", "/v1/health/checks/consul?dc=dc1&node-meta=somekey:somevalue", nil) + resp = httptest.NewRecorder() + obj, err = a.srv.HealthServiceChecks(resp, req) + if err != nil { + r.Fatalf("err: %v", err) + } + assertIndex(r, resp) - // Should be 1 health check for consul - nodes = obj.(structs.HealthChecks) - if len(nodes) != 1 { - t.Fatalf("bad: %v", obj) - } + // Should be 1 health check for consul + nodes = obj.(structs.HealthChecks) + if len(nodes) != 1 { + r.Fatalf("bad: %v", obj) + } + }) } func TestHealthServiceChecks_Filtering(t *testing.T) { diff --git a/agent/http_test.go b/agent/http_test.go index 83510d9c06e..f2469acc835 100644 --- a/agent/http_test.go +++ b/agent/http_test.go @@ -1628,8 +1628,10 @@ func TestAllowedNets(t *testing.T) { } // assertIndex tests that X-Consul-Index is set and non-zero -func assertIndex(t *testing.T, resp *httptest.ResponseRecorder) { - t.Helper() +func assertIndex(t require.TestingT, resp *httptest.ResponseRecorder) { + if tt, ok := t.(*testing.T); ok { + tt.Helper() + } require.NoError(t, checkIndex(resp)) } diff --git a/agent/metrics_test.go b/agent/metrics_test.go index 97286cdde98..f7e15ca40e3 100644 --- a/agent/metrics_test.go +++ b/agent/metrics_test.go @@ -32,6 +32,10 @@ func skipIfShortTesting(t *testing.T) { } func recordPromMetrics(t require.TestingT, a *TestAgent, respRec *httptest.ResponseRecorder) { + if tt, ok := t.(*testing.T); ok { + tt.Helper() + } + req, err := http.NewRequest("GET", "/v1/agent/metrics?format=prometheus", nil) require.NoError(t, err, "Failed to generate new http request.") From 91ca74d17f058dd8bdb60440267778b7a36cda01 Mon Sep 17 00:00:00 2001 From: "Chris S. Kim" Date: Thu, 5 Oct 2023 16:37:48 -0400 Subject: [PATCH 3/3] Retry port grabs --- agent/testagent.go | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/agent/testagent.go b/agent/testagent.go index 8ae2220adc6..84751ba0f76 100644 --- a/agent/testagent.go +++ b/agent/testagent.go @@ -405,6 +405,19 @@ func (a *TestAgent) consulConfig() *consul.Config { return c } +// Using sdk/freeport with *retry.R is not possible without changing +// function signatures. We use this shim instead to save the headache +// of syncing sdk submodule updates. +type retryShim struct { + *retry.R + + name string +} + +func (r *retryShim) Name() string { + return r.name +} + // pickRandomPorts selects random ports from fixed size random blocks of // ports. This does not eliminate the chance for port conflict but // reduces it significantly with little overhead. Furthermore, asking @@ -414,7 +427,10 @@ func (a *TestAgent) consulConfig() *consul.Config { // Instead of relying on one set of ports to be sufficient we retry // starting the agent with different ports on port conflict. func randomPortsSource(t *testing.T, useHTTPS bool) string { - ports := freeport.GetN(t, 7) + var ports []int + retry.RunWith(retry.TwoSeconds(), t, func(r *retry.R) { + ports = freeport.GetN(&retryShim{r, t.Name()}, 7) + }) var http, https int if useHTTPS {