Skip to content

Commit 2a7d8aa

Browse files
committed
Replace WaitForAllNodesReady inline polling with readiness.Wait
The previous implementation polled cat-nodes for 60s with a single boolean predicate, producing "Condition never satisfied" on timeout. Switch to readiness.Wait(t, ctx, TargetClusterReady, WithCluster(c)) which observes per-node progression through LayerHTTP, LayerClusterJoin, and LayerStatsReady, and dumps a structured per-node diagnostic with the full last cat-nodes response on failure. Signed-off-by: Sean Chittenden <sean.chittenden@crowdstrike.com>
1 parent 8a578dc commit 2a7d8aa

3 files changed

Lines changed: 25 additions & 78 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
177177

178178
### Fixed
179179

180+
- Replace `WaitForAllNodesReady` inline `require.Eventually` loop with a layered readiness FSM (`internal/test/readiness`) that observes per-node progression through `LayerTCP -> LayerHTTP -> LayerClusterJoin -> LayerStatsReady`, records transitions including regressions, and emits a structured per-node diagnostic with the full last cat-nodes response on timeout. Per-layer budgets are tuned for CI pessimism (cold JVM startup is the long pole); total budget for `TargetClusterReady` is 6.5 minutes. ([#650](https://github.com/opensearch-project/opensearch-go/issues/650))
180181
- Fix bulk indexer HTML-escaping `_id` and `routing` values containing `<`, `>`, or `&` characters, causing OpenSearch to store escaped values (e.g., `\u003croot_account\u003e` stored instead of `<root_account>`), leading to duplicate documents, unreachable data on read-by-ID paths, and potential shard routing mismatches. Present since the `json.Marshal` migration in 2021 (commit `3da59092`). Replace `json.Marshal` with `json.NewEncoder` + `SetEscapeHTML(false)` in `opensearchutil.worker.writeMeta` and `opensearchutil.JSONReader`; replace per-worker `aux []byte` with `sync.Pool`-backed `*bytes.Buffer`; add table-driven test coverage for `writeMeta` edge cases and refactor remaining `TestBulkIndexer` subtests to table-driven `require`-based style ([#824](https://github.com/opensearch-project/opensearch-go/pull/824))
181182
- Fix pool replacement orphaning resurrection goroutines during node discovery, causing connections to become permanently dead with no active health checker ([#786](https://github.com/opensearch-project/opensearch-go/pull/786))
182183
- Fix multi-to-single pool demotion leaking resurrection goroutines by giving each `multiServerPool` its own derived context and cancelling it on demotion ([#830](https://github.com/opensearch-project/opensearch-go/pull/830))

opensearchtransport/testutil/helpers.go

Lines changed: 11 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@ import (
2121
"time"
2222

2323
"golang.org/x/mod/semver"
24+
25+
"github.com/opensearch-project/opensearch-go/v4/internal/test/readiness"
2426
)
2527

2628
// Common timeouts for testing
@@ -337,73 +339,23 @@ func RequireMinConns(
337339
func WaitForCluster(t *testing.T) {
338340
t.Helper()
339341

340-
const (
341-
maxAttempts = 25
342-
delayBetweenAttempts = 5 * time.Second
343-
requestTimeout = 2 * time.Second
344-
)
345-
346342
u := GetTestURL(t)
347343
healthURL := *u
348344
healthURL.Path = "/"
349345

350-
client := &http.Client{Transport: GetTestTransport(t)}
351-
352-
var eofCount int
353-
for attempt := range maxAttempts {
354-
ctx, cancel := context.WithTimeout(t.Context(), requestTimeout)
355-
356-
req, err := http.NewRequestWithContext(ctx, http.MethodGet, healthURL.String(), nil)
357-
if err != nil {
358-
cancel()
359-
t.Fatalf("WaitForCluster: failed to create request: %v", err)
360-
}
361-
362-
if IsSecure(t) {
363-
req.SetBasicAuth("admin", GetPassword(t))
364-
}
365-
366-
resp, err := client.Do(req)
367-
cancel()
346+
httpClient := &http.Client{Transport: GetTestTransport(t)}
368347

369-
if err != nil {
370-
if strings.Contains(err.Error(), "EOF") {
371-
eofCount++
372-
if eofCount >= 3 {
373-
t.Fatalf("WaitForCluster: cluster returned EOF on %d consecutive attempts "+
374-
"(SECURE_INTEGRATION=%s); verify the cluster scheme matches this setting: %v",
375-
eofCount, os.Getenv("SECURE_INTEGRATION"), err)
376-
}
377-
} else {
378-
eofCount = 0
379-
}
380-
t.Logf("WaitForCluster: attempt %d/%d: %v", attempt+1, maxAttempts, err)
381-
time.Sleep(delayBetweenAttempts)
382-
continue
383-
}
384-
385-
eofCount = 0
386-
resp.Body.Close()
387-
388-
if resp.StatusCode == http.StatusOK {
389-
if attempt > 0 {
390-
t.Logf("WaitForCluster: cluster ready after %d attempts", attempt+1)
391-
}
392-
return
393-
}
394-
395-
// Fail fast on authentication errors -- retrying with the same credentials won't help
396-
if resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden {
397-
t.Fatalf("WaitForCluster: cluster returned %d (SECURE_INTEGRATION=%s, OPENSEARCH_VERSION=%s); "+
398-
"verify credentials are correct -- the admin password changed in OpenSearch 2.12.0+",
399-
resp.StatusCode, os.Getenv("SECURE_INTEGRATION"), os.Getenv("OPENSEARCH_VERSION"))
348+
var prepareReq func(*http.Request)
349+
if IsSecure(t) {
350+
password := GetPassword(t)
351+
prepareReq = func(req *http.Request) {
352+
req.SetBasicAuth("admin", password)
400353
}
401-
402-
t.Logf("WaitForCluster: attempt %d/%d: status %d", attempt+1, maxAttempts, resp.StatusCode)
403-
time.Sleep(delayBetweenAttempts)
404354
}
405355

406-
t.Fatalf("WaitForCluster: cluster not ready after %d attempts (url=%s)", maxAttempts, healthURL.String())
356+
readiness.Wait(t, t.Context(), readiness.LayerHTTP,
357+
readiness.WithExpectedNodes(1),
358+
readiness.WithRawHTTP(&healthURL, httpClient, prepareReq))
407359
}
408360

409361
// ignoredFieldRule pairs a compiled regexp with a version expression.

osapi/testutil/helpers.go

Lines changed: 13 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import (
2525
"golang.org/x/sync/semaphore"
2626

2727
"github.com/opensearch-project/opensearch-go/v4"
28+
"github.com/opensearch-project/opensearch-go/v4/internal/test/readiness"
2829
tptestutil "github.com/opensearch-project/opensearch-go/v4/opensearchtransport/testutil"
2930
"github.com/opensearch-project/opensearch-go/v4/osapi"
3031
)
@@ -230,8 +231,6 @@ func InitClient(t *testing.T) (*osapi.Client, error) {
230231
// GET / using the layered readiness FSM in internal/test/readiness. The
231232
// readinessSem caps concurrent setups across tests so a stampede of
232233
// goroutines doesn't overload a small CI cluster.
233-
func WaitForClusterReady(t *testing.T, client *osapi.Client) {
234-
// goroutines doesn't overload a small CI cluster.
235234
func WaitForClusterReady(t *testing.T, client *osapi.Client) {
236235
t.Helper()
237236
if err := readinessSem.Acquire(t.Context(), 1); err != nil {
@@ -242,25 +241,20 @@ func WaitForClusterReady(t *testing.T, client *osapi.Client) {
242241
readiness.Wait(t, t.Context(), readiness.LayerHTTP, readiness.WithCluster(client))
243242
}
244243

245-
// WaitForAllNodesReady polls /_cat/nodes until every node reports non-nil cpu
246-
// and heap.percent metrics. This prevents flakes from nodes that haven't fully
247-
// initialized in CI (e.g. stats not yet collected after a fresh cluster start).
244+
// WaitForAllNodesReady blocks until every node in the test cluster has
245+
// reached LayerStatsReady — i.e. cluster-health reports the expected
246+
// node count AND _cat/nodes returns non-nil cpu+heap.percent for each
247+
// node. It uses the layered readiness FSM in internal/test/readiness so
248+
// that timeouts produce a structured per-node diagnostic instead of a
249+
// "Condition never satisfied" stub.
250+
//
251+
// Expected node count comes from OPENSEARCH_NODE_COUNT (defaults to 1).
252+
// Per-layer budgets are tuned for CI pessimism (cold JVM startup is the
253+
// long pole); see readiness.DefaultBudgets for the exact values.
248254
func WaitForAllNodesReady(t *testing.T, client *osapi.Client) {
249255
t.Helper()
250-
require.Eventually(t, func() bool {
251-
resp, err := client.Cat.Nodes(t.Context(), &osapi.CatNodesReq{
252-
Params: &osapi.CatNodesParams{DebugParams: osapi.DebugParams{Format: "json"}},
253-
})
254-
if err != nil || resp == nil || len(resp.Records) == 0 {
255-
return false
256-
}
257-
for _, node := range resp.Records {
258-
if node.Cpu == nil || node.HeapPercent == nil {
259-
return false
260-
}
261-
}
262-
return true
263-
}, 60*time.Second, 1*time.Second, "not all nodes reporting stats (cpu/heap.percent nil)")
256+
readiness.Wait(t, t.Context(), readiness.TargetClusterReady,
257+
readiness.WithCluster(client))
264258
}
265259

266260
// CompareRawJSONwithParsedJSON is a helper function to determine the difference between the parsed JSON and the raw JSON.

0 commit comments

Comments
 (0)