Repository navigation
fix(e2e): wait for the redis replica link instead of sampling it once - #4252
Conversation
The redis-tls topology check read INFO replication from both pods once and failed the suite when a replica reported role:slave without master_link_status:up. That value is legitimately in motion there. Redis reports the link up only in REPL_STATE_CONNECTED, while the operator's readiness script admits a replica on two different predicates -- master_host off 127.0.0.1, and sync not in the transfer phase -- so every handshake state between the two is ready and unlinked, and a dropped link re-enters that window. Ready replicas carry no evidence about the link at all, which is more than the step's own comment allowed for and makes its claim of determinism wrong. Poll the topology to a 60s deadline instead, and on expiry report each pod's observed state along with how long it waited. Nothing models the link declaratively -- the RedisFailover CRD defines no status, so there is no condition to assert on and the poll carries the marker the e2e conventions require. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
github-actions
Bot
added
area/testing
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe Redis TLS Chainsaw test now polls until replication reaches one master and one linked replica. It fails after a 60-second deadline with observed states. The script operation timeout increased from three to five minutes, and the comments document the wait. ChangesRedis TLS topology convergence
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The Redis TLS topology check now waits for replication to converge within a bounded window, reducing transient test failures without introducing an identified merge risk. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM with non-blocking notes
Reproduced against real INFO replication from redis 8.4.0 (what files/versions.yaml maps v8 to). The pre-fix block fails on a link that comes up on the fourth sample, the post-fix block passes, and the assertion is untouched, so nothing is relaxed:
$ SCENARIO=converges sh harness-old.sh # merge-base block, link up on sample 4
rfr-redis-secure-1 replicates with no link up
exit=1
$ SCENARIO=converges sh harness.sh # PR block, same fixtures
LOOP EXITED NORMALLY after 8 reads
exit=0
$ SCENARIO=settled sh harness.sh # already linked: no added cost
LOOP EXITED NORMALLY after 2 reads
Fixtures include the ready-but-unlinked state you describe (master_host off 127.0.0.1, master_sync_in_progress:0, master_link_status:down), and the readiness predicate reads as you state it at the pinned upstream: freshworks-oss/[email protected] operator/redisfailover/service/generator.go:299-322 exits 0 when neither master_sync_in_progress:1 nor master_host:127.0.0.1 matches, and never reads master_link_status.
Findings
- [MINOR]
hack/e2e-chainsaw/redis/chainsaw-test.yaml:472, the poll prints nothing when it settles late - [MINOR]
hack/e2e-chainsaw/redis/chainsaw-test.yaml:457, one failed read ends the wait at t=0 - [MINOR]
hack/e2e-chainsaw/redis/chainsaw-test.yaml:447, link_wait equals redis's own repl-timeout default
Claim mismatches
[PARTIAL] "Two pre-existing diagnostics defects ... Neither is touched here." #4250 is about this step's op timeout and the diff raises it from 3m to 5m; #4250 says so itself. Not fixed is accurate, not touched is not; the gap narrows (510s against 180s, now 575s against 300s) and stays open.
Caveats
- The red check on this head is
E2E (in-tree), stepRun E2E tests, and it is not this change.redis-tlspassed at 73.82s;redis-2-backup-roundtripfailed onstrategy Kind "Redis" is not supported by this controller.internal/backupcontroller/backupjob_controller.go:192-205lists eleven kinds here and the controller named seven, so the running binary is older:packages/system/backupstrategy-controller/values.yaml:2pinsv1.6.0, andgit merge-base --is-ancestor b0eb10427 v1.6.0is false, so that image predates the Redis driver. Worth its own issue. - No
packages/file is touched: no install/upgrade surface. - Nothing in-tree would catch a revert: shell inside a chainsaw
scriptreaches neither shellcheck nor helm-unittest, which is why the mutation above is mine rather than CI's.
| esac | ||
| seen="$seen rfr-redis-secure-$i=$state" | ||
| done | ||
| if [ "$masters" = "1" ] && [ "$linked" = "1" ]; then break; fi |
There was a problem hiding this comment.
[MINOR] the poll prints nothing when it settles late
break goes straight to the sentinel check, so a run that needed eight samples and one that needed one read identically in the job log. A regression pushing the link to 30-59s is absorbed silently, where the old code made it red. One line before the break naming the elapsed seconds when it took more than one pass restores the signal.
| for i in 0 1; do | ||
| if ! info=$(k -n "$NAMESPACE" exec "rfr-redis-secure-$i" -c redis -- \ | ||
| sh -c 'REDISCLI_AUTH="$REDIS_PASSWORD" timeout 15 redis-cli --tls --cacert /tls/ca.crt -h localhost -p 6379 info replication'); then | ||
| echo "rfr-redis-secure-$i did not answer INFO replication within the 15s cap" >&2 |
There was a problem hiding this comment.
[MINOR] one failed read ends the wait at t=0
A read error exits immediately, spending none of the 60s.
$ SCENARIO=execfail sh harness.sh # one transient read error on the 2nd read
rfr-redis-secure-1 did not answer INFO replication within the 15s cap
exit=1
The step now runs that read path up to a dozen times per run instead of twice, so it has the most exposure and least tolerance, and the message blames the 15s cap for what may be the 30s k() cap or a 403 (#4246, re-emitted here on an added line). The tenantsecret loop earlier in this same step already draws the distinction the poll drops: it refuses to read any non-NotFound error as absence. Recording the failure as a state in $seen and letting the deadline decide stays fail-closed and names the real cause.
| # motion here and one sampling would decide on it. Wait for it to | ||
| # settle, then assert the sentinels moved off the bootstrap address, | ||
| # which nothing above covers at all. | ||
| link_wait=60 |
There was a problem hiding this comment.
[MINOR] link_wait equals redis's own repl-timeout default
$ docker run --rm redis:8.4.0 ... redis-cli config get repl-timeout
repl-timeout
60
packages/apps/redis/templates/redisfailover.yaml:85-91 overrides only tcp-keepalive, loglevel, appendonly and save, so the 60s default stands. A sync attempt that stalls is abandoned by redis at the same second this deadline expires, so the budget spans exactly one attempt and never the retry after it. Unlikely to bite on an empty dataset, but the number buys less than it looks.
2 participants
What this PR does
The
verify-tenant-ca-projectionstep in the redis suite readINFO replicationfrom both pods once and failed the run when a replica reportedrole:slavewithoutmaster_link_status:up. That value is still in motion at that point, so one sampling decided the result.Redis reports the link up only once a replica reaches
REPL_STATE_CONNECTED. The operator's readiness script admits a replica on two other predicates:master_hostbeing off127.0.0.1, andmaster_sync_in_progressnot being1.master_hostis set the momentREPLICAOFis issued, ahead of the link coming up, andmaster_sync_in_progressis1only during the transfer phase, so every handshake state between the two is ready and unlinked. A dropped link re-enters the same window.readyReplicas: 2therefore establishes nothing about the link, and the comment on the step calling the check deterministic was wrong.The step now polls the topology to a bounded deadline and fails only if it does not settle, reporting each pod's observed state and the elapsed time. Nothing models the link declaratively: the RedisFailover CRD defines no status at all. So the poll carries the
TODO(e2e-replace-fixed-timeouts):marker the e2e conventions require for a wait with no object behind it.What the step does now is wait for convergence where it used to sample once. No claim about flake rate follows from that. A green run says nothing about a failure that shows up in a minority of runs.
Shell embedded in a chainsaw step has no unit-test surface in this repository, so this change is exercised by the suite and nothing else.
Two pre-existing diagnostics defects in the same file turned up while reading it, tracked in #4246 and #4250. Neither is touched here. The first one matters a little more after this change: the poll runs its read path about a dozen times per step instead of twice.
Closes #4238
Screenshots
Not a UI change.
Downstream repositories
I walked the trigger map against the diff rather than the title. The change edits one file under
hack/e2e-chainsaw/, moves and renames nothing, and alters no make target, so none of the listed triggers fire. The node-prerequisite trigger that reachesansible-cozystackandtalmishack/e2e-prepare-cluster.bats, which this does not touch.Release note
Summary by CodeRabbit