Visitar URL original
We already have a fault-injector tier; it answers several of the open… · seredis/StackExchange.Redis@be285b5 · GitHub
Skip to content

Commit be285b5

Browse files
committed
We already have a fault-injector tier; it answers several of the open questions
It is not merged - tests/StackExchange.Redis.FaultInjector.Tests lives on marc/maint-optin-server, open as #3191 (107 files, +13160) - which is presumably why I did not find it when I grepped a branch taken fresh from main. Two pieces of it bear directly on this. FaultInjectorClient wraps the same POST /action + GET /action/{id} contract go-redis, redis-py and node-redis converged on, and adds GetValidTriggersAsync, so we can ask a deployment which lag-inducing effects it supports instead of guessing at an action name. And ClusterRestClient is already a Redis Enterprise REST client on 9443 - Basic auth, System.Text.Json, GET /v1/bdbs?fields=uid,name - so the availability call is a method on an existing class, not a new project. It also settles by observation what I had listed as open and sourced only from documentation: the management certificate is self-signed per environment and pinned to a CA from the config directory, so the probe needs CA configuration and cannot rely on the ambient trust store; auth really is Basic; and the bdbs listing returns the requested fields, so discovery is easy - which weakens the avoid-JSON argument to "it keeps JSON out of src/", not "discovery is hard". Sequencing consequence recorded: an end-to-end test is downstream of #3191. And #3191's own summary is the best argument for validating before building - same vendor, same class of API: the specifications are prose, the payloads were never published, and nearly every starting assumption was wrong.
1 parent 62a55bc commit be285b5

1 file changed

Lines changed: 65 additions & 49 deletions

File tree

‎notes/lag-aware/findings.md‎

Lines changed: 65 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -286,10 +286,8 @@ things currently taken on trust:
286286
- what does a failure body actually look like — are the composed codes
287287
(`bdb_unavailable_shard_unreachable_port_unbound`) real, and is `error_code` reliably present?
288288
- is a `Host: cnm.cluster.fqdn` header actually required, as the docs' header table implies?
289-
- what does `GET /v1/bdbs?fields=uid,endpoints` really return? That shape decides whether the
290-
JSON-avoidance argument in §5 holds, or whether discovery is cheap enough to include after all.
291-
- TLS on 9443: self-signed by default? That decides whether we need redis-py's `ca_file` /
292-
`client_cert_file` / `verify_tls` surface or can rely on the ambient trust store.
289+
(`ClusterRestClient` sets no such header and works, so: apparently not — but it only issues
290+
`GET /v1/bdbs`, so worth re-checking on the availability route.)
293291
- **what lag do real geo-replicated links actually show?** This is the empirical way to settle
294292
100 ms versus Lettuce's 5000 ms (§3), and it is a much better argument than reading either
295293
default off a page.
@@ -298,44 +296,59 @@ The two then compose rather than compete, and the synthesis is the useful bit: *
298296
cluster to capture responses, and the fakes to replay them.** A recorded set of genuine 200/503/404
299297
bodies baked into the stub means the deterministic tests stop guessing at the wire format.
300298

301-
### The fault injector — correcting the assumption above
299+
### The fault injector — and we already have one
302300

303301
I had assumed a real cluster could not be made to show "reachable but stale" on demand. **That is
304-
wrong**, and it is worth knowing before planning any of this: Redis ships a **fault injection
305-
service** that the client test suites drive over HTTP, precisely so failover behaviour can be tested
306-
against real Redis Enterprise deployments.
307-
308-
From Lettuce's `src/test/java/io/lettuce/scenario/FaultInjectionClient.java`:
309-
310-
- base URL `http://127.0.0.1:20324` by default, overridable with `FAULT_INJECTION_API_URL`
311-
- `POST /action` with `{ "type": <action>, "parameters": { … } }` → `{ "action_id": … }`, then poll
312-
that id until the action completes
313-
- generous timeouts by category — up to 5 minutes for migrations and failovers — plus a
314-
stabilisation delay afterwards
315-
316-
Action types visible in their suite:
317-
318-
| action | parameters | used for |
319-
| --- | --- | --- |
320-
| `network_latency` | `bdb_id`, `delay_ms`, `duration` | **Active-Active failover scenarios** |
321-
| `network_failure` | endpoint | reconnection / pub-sub recovery |
322-
| `dmc_restart` | endpoint | reconnection / pub-sub recovery |
323-
324-
`ActiveActiveFailoverScenarioTest` injects `network_latency` against the primary's `bdb_id` for a
325-
bounded duration — which is exactly the mechanism for driving replication lag past the tolerance and
326-
watching the lag-aware check flip while the plain check stays green. So the authoritative end-to-end
327-
test for this feature is real cluster + fault injector, and it is a road Lettuce has already built.
328-
329-
That demotes the fakes but does not remove them. They remain worth having for: CI and dev machines
330-
with no Enterprise cluster; speed (the injector's failover actions budget minutes, and there is a
331-
10-second stabilisation wait); and the cases the injector does not obviously cover — specific
332-
`error_code` bodies, 401/403 credential expiry, and slow-but-successful responses for probe-timeout
333-
behaviour.
334-
335-
Their scenario tests also follow the same gating shape we already use: endpoints come from
336-
configuration (`Endpoints.DEFAULT.getEndpoint("re-standalone")`) with `assumeTrue(... != null,
337-
"Skipping test because no Redis endpoint is configured!")`, which is our `Skip.IfNoServer` pattern
338-
under a different name.
302+
wrong twice over**: Redis ships a fault injection service for exactly this, *and we already have a
303+
test tier built on it.*
304+
305+
**`tests/StackExchange.Redis.FaultInjector.Tests`** exists on `marc/maint-optin-server`, open as
306+
**[#3191](https://github.com/StackExchange/StackExchange.Redis/pull/3191)** (107 files, +13,160,
307+
unmerged at the time of writing). Per its README it drives *"a real Redis Enterprise deployment
308+
through the fault injector, and watch[es] how SE.Redis reacts … the tier that can observe what no
309+
in-process fake can: real DNS, real TLS identity, real timing."* Gating is
310+
`SER_FI_CONFIG_DIR` plus an explicit `E2E_SCENARIO_TESTS=true` opt-in, with `FAULT_INJECTION_API_URL`
311+
defaulting to `http://127.0.0.1:20324`.
312+
313+
Two pieces of it matter directly here.
314+
315+
**`FaultInjector/FaultInjectorClient.cs`** — `POST /action` returning an id, `GET /action/{id}` to
316+
poll, with its own note that this is *"the same shape go-redis, redis-py and node-redis wrap …
317+
they integrated independently and converged, so the contract is stable"*. It is richer than Lettuce's:
318+
alongside `StartActionAsync` / `RunActionAsync` / `WaitForActionAsync` there is
319+
`GetValidTriggersAsync(scenario, effect, clusterIndex)`, so **we can ask the injector what
320+
lag-inducing effects the deployment actually supports rather than guessing at an action name.**
321+
Lettuce's own Active-Active failover test uses `network_latency` with `bdb_id`, `delay_ms` and
322+
`duration`, which is the obvious candidate to look for.
323+
324+
**`Environment/ClusterRestClient.cs`** — *already a Redis Enterprise REST client*, described as "the
325+
cluster's own REST API on port 9443, for the few facts the fault injector does not expose". It
326+
already does `GET /v1/bdbs?fields=uid,name`, with HTTP Basic auth and `System.Text.Json`. Adding
327+
`GET /v1/bdbs/{uid}/availability?extend_check=lag` beside it is a method, not a project.
328+
329+
That client also **answers several questions in §9 empirically**, which is worth more than the
330+
documentation they were drawn from:
331+
332+
- **TLS on 9443 is self-signed per environment.** The client pins to a CA from the config directory
333+
via `ServerCertificateCustomValidationCallback` with `X509ChainTrustMode.CustomRootTrust`, with the
334+
comment *"this channel carries credentials"*. So a lag-aware probe **does** need CA/cert
335+
configuration in the redis-py mould; relying on the ambient trust store will not do.
336+
- **Auth is HTTP Basic**, confirmed against a real cluster rather than inferred from Lettuce.
337+
- **`/v1/bdbs?fields=…` returns an array of objects carrying the requested fields**, so Lettuce's
338+
`fields=uid,endpoints` discovery is straightforward. That weakens the §5 argument a little: skipping
339+
discovery still keeps JSON out of **`src/`**, but "discovery is hard" is not the reason to skip it.
340+
341+
The fakes are demoted, not removed: they still cover machines with no Enterprise cluster, they are
342+
far faster (injector actions budget minutes, plus a stabilisation wait), and they reach what the
343+
injector does not obviously reach — specific `error_code` bodies, 401/403 credential expiry, and
344+
slow-but-successful responses for probe-timeout behaviour.
345+
346+
**Sequencing consequence:** an end-to-end lag-aware test is downstream of #3191. That is a real
347+
dependency to plan around, not a detail.
348+
349+
And #3191's own write-up is the strongest possible support for validating §1 before writing code:
350+
*"the specifications are prose, the payloads were never published, and nearly every assumption I
351+
started with was wrong in some way that mattered."* Same vendor, same class of API, same trap.
339352

340353
Gating is a solved problem here: `tests/.../Helpers/Skip.cs` already has `IfNoServer(host, port)`,
341354
`IfNoCluster()`, `IfNoFailoverPair()` and `UnlessLongRunning()`, and `TestConfig` carries per-role
@@ -443,12 +456,15 @@ external probe can already reach `context.Server.Multiplexer.RawConfig.Tunnel` w
443456
needs to map endpoint → REST configuration, which per-member health checks make unnecessary but do
444457
not forbid.
445458
6. **Credentials and rotation.** Lettuce takes a `Supplier<RedisCredentials>` so credentials can
446-
rotate; redis-py supports Basic plus mTLS. Whatever we do should not bake in a static password.
447-
7. **How is this tested?** Answered in §7: real cluster plus Redis's **fault injection service** is
448-
the authoritative path — `network_latency` against a `bdb_id` drives real replication lag past
449-
the tolerance, which is how Lettuce tests Active-Active failover — with stubbed HTTP transport
450-
for the deterministic/CI tier and `toys/KestrelRedisServer` (already serving HTTP on 5000) in
451-
between. Open: whether we stand up a fault-injector-backed scenario suite at all, who owns it,
452-
and whether the injector is available to us alongside the clusters. The real-cluster validation
453-
in any case wants doing *before* code, since several §1 facts are documentation rather than
454-
observation.
459+
rotate; redis-py supports Basic plus mTLS; our own `ClusterRestClient` pins a CA because the
460+
management certificate is self-signed per environment (§7). So the probe needs *at least* Basic
461+
plus CA pinning, and should not bake in a static password.
462+
7. ~~**How is this tested?**~~ Largely answered in §7: the tier exists, in #3191. What is genuinely
463+
open is **sequencing and scope** — an end-to-end lag-aware test is downstream of an unmerged
464+
107-file PR, so either this waits for #3191 or it starts at the stubbed tier and the scenario test
465+
follows. Also open: whether the deployments we have access to expose a lag-inducing effect at all
466+
(ask `GetValidTriggersAsync` rather than assuming `network_latency`).
467+
8. **Does any of this need `src/` changes at all?** On the evidence so far, possibly none:
468+
`HealthCheckProbe` is externally subclassable, `RawConfig`/`Tunnel` are public, and the REST
469+
plumbing precedent lives in the test tier. The `Tunnel` addition in §8 is the one thing that would
470+
have to land in the core package — and only if we want the management plane to honour tunnels.

0 commit comments

Comments
 (0)