Skip to content

Commit 61a016d

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 ebd7774 commit 61a016d

2 files changed

Lines changed: 14 additions & 17 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))

osapi/testutil/helpers.go

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

2828
"github.com/opensearch-project/opensearch-go/v4"
29+
"github.com/opensearch-project/opensearch-go/v4/internal/test/readiness"
2930
tptestutil "github.com/opensearch-project/opensearch-go/v4/opensearchtransport/testutil"
3031
"github.com/opensearch-project/opensearch-go/v4/osapi"
3132
)
@@ -320,25 +321,20 @@ func WaitForClusterReady(t *testing.T, client *osapi.Client) error {
320321
return fmt.Errorf("cluster not ready after %d attempts (version %d.%d.%d)", maxAttempts, major, minor, patch)
321322
}
322323

323-
// WaitForAllNodesReady polls /_cat/nodes until every node reports non-nil cpu
324-
// and heap.percent metrics. This prevents flakes from nodes that haven't fully
325-
// initialized in CI (e.g. stats not yet collected after a fresh cluster start).
324+
// WaitForAllNodesReady blocks until every node in the test cluster has
325+
// reached LayerStatsReady — i.e. cluster-health reports the expected
326+
// node count AND _cat/nodes returns non-nil cpu+heap.percent for each
327+
// node. It uses the layered readiness FSM in internal/test/readiness so
328+
// that timeouts produce a structured per-node diagnostic instead of a
329+
// "Condition never satisfied" stub.
330+
//
331+
// Expected node count comes from OPENSEARCH_NODE_COUNT (defaults to 1).
332+
// Per-layer budgets are tuned for CI pessimism (cold JVM startup is the
333+
// long pole); see readiness.DefaultBudgets for the exact values.
326334
func WaitForAllNodesReady(t *testing.T, client *osapi.Client) {
327335
t.Helper()
328-
require.Eventually(t, func() bool {
329-
resp, err := client.Cat.Nodes(t.Context(), &osapi.CatNodesReq{
330-
Params: &osapi.CatNodesParams{DebugParams: osapi.DebugParams{Format: "json"}},
331-
})
332-
if err != nil || resp == nil || len(resp.Records) == 0 {
333-
return false
334-
}
335-
for _, node := range resp.Records {
336-
if node.Cpu == nil || node.HeapPercent == nil {
337-
return false
338-
}
339-
}
340-
return true
341-
}, 60*time.Second, 1*time.Second, "not all nodes reporting stats (cpu/heap.percent nil)")
336+
readiness.Wait(t, t.Context(), readiness.TargetClusterReady,
337+
readiness.WithCluster(client))
342338
}
343339

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

0 commit comments

Comments
 (0)