test(e2e): add shutdown-on-sigterm failover test - #295
Conversation
| Expect(output).To(ContainSubstring(fmt.Sprintf("written=%d", keyCount)), | ||
| fmt.Sprintf("Not all keys were written: %s", output)) | ||
|
|
||
| By("gracefully terminating the primary pod (SIGTERM with default grace period)") |
There was a problem hiding this comment.
i think there is some overlap between this test and https://github.com/sandeepkunusoth/valkey-k8s-operator/blob/5eb8a3380eb55a6151d6e9e7df7f838de654726e/test/e2e/valkeycluster_test.go#L951. can we merge both of them together?
There was a problem hiding this comment.
@sandeepkunusoth can you get your PR merged if not done already? I can rebase my PR on your changes.
There was a problem hiding this comment.
This is already in main from very long time
There was a problem hiding this comment.
hi i think u didn't get my previous comment instead of having duplicate test at both places i was suggesting if we we can move this to existing e2e test where we are doing failover already. you may just need to verify key are persisted and some other changes.
|
nice, this closes the e2e i raised in #270. the structure is solid, and a couple of details are better than the outline i sketched: reading roles live from INFO replication with the one thing worth thinking about, since i wrote the shutdown-on-sigterm change (#268): the current assertions would mostly pass with the feature turned off too, so the test doesn't fully isolate what it's meant to prove.
what shutdown-on-sigterm actually buys is an orderly handoff: the primary fails over to a caught-up replica before it exits, so there's no window where the shard has no writer and no acknowledged write is dropped. to target that, i'd write continuously through the termination (a loop of SET with unique values while the pod is terminating) and then assert every acknowledged write is readable afterwards, plus that the write-error window is near zero. that's the behaviour the directive changes; the current version mostly checks that a failover eventually happened. not blocking, it's a good baseline, just flagging that it would likely stay green even if #268 were reverted. |
Cover the graceful-termination handover deferred from valkey-io#268: delete a shard primary with the default grace period and assert a replica is promoted before the grace period ends, the shard keeps serving writes, the replaced pod rejoins as a replica, and no keys are lost. Roles are read live from INFO replication rather than ValkeyNode status, which can report stale roles right after cluster formation (see valkey-io#261). VALKEYCLI_AUTH is unset when running valkey-cli inside the server container, since valkey-cli would otherwise auto-send AUTH as the default user and fail. Closes valkey-io#270 Signed-off-by: Sagar Utekar <sagarutekar2366@gmail.com>
Merge the primary-workload-deletion recovery scenario suggested in review into the failover suite as a second spec: delete the shard primary's StatefulSet, assert the replica is promoted, the operator recreates the StatefulSet, the recreated pod rejoins as a replica, and the ValkeyCluster returns to Ready with no keys lost. Cluster creation, key seeding, and health/data assertions are shared between both specs. Signed-off-by: Sagar Utekar <sagarutekar2366@gmail.com>
Give the failover test clusters a password-protected default user via a passwordSecret, following the pattern from valkey-io#292, and set VALKEYCLI_AUTH to that password for every valkey-cli invocation instead of unsetting it. The tests no longer rely on the default user being passwordless. Signed-off-by: Sagar Utekar <sagarutekar2366@gmail.com>
e63d186 to
4dcc856
Compare
The StatefulSet-deletion recovery scenario is already covered in main by 'should detect and recover when a primary deployment is deleted' in valkeycluster_test.go. Keep this suite focused on what that test does not exercise: the graceful shutdown-on-sigterm handoff of a terminating primary pod. Signed-off-by: Sagar Utekar <sagarutekar2366@gmail.com>
Strengthen the sigterm failover spec per review: a continuous writer runs through the termination recording per-attempt acks, and the test asserts every acknowledged write is readable afterwards and that the longest gap between acknowledged writes stays bounded. The failover path (coordinated handoff vs failure detection) is detected from the promoted replica's log and reported. The handoff path is reported rather than hard-asserted: valkey 9.0's clusterAutoFailoverOnShutdown requires exact ack-offset equality when selecting a replica and intermittently falls back to failure-detection promotion (~1 in 3 under write load in local testing), so a hard assertion would flake until that promotion is deterministic. Observed on Kind with the handoff engaged: 7490 acknowledged writes through the disruption, longest writer gap 0.05s, zero lost. Signed-off-by: Sagar Utekar <sagarutekar2366@gmail.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Per review, drop the separate failover spec and extend the existing 'should detect and recover when a primary deployment is deleted' test instead, so failover coverage lives in one place. The existing test gains, around its StatefulSet-deletion disruption: - a password-protected default user (passwordSecret, as in valkey-io#292) so valkey-cli commands run authenticated - 50 keys seeded before the disruption and verified after recovery - a continuous writer through the disruption with per-attempt acks and a 2s connection timeout (a stale MOVED redirect to the terminated primary's IP otherwise hangs a connect for the ~130s TCP SYN timeout) - promotion of the shard's replica asserted within the 30s termination grace period - verification that every write acknowledged during the disruption is readable afterwards - detection of whether the shutdown-on-sigterm handoff engaged (from the promoted replica's log), with the writer-gap bound asserted on the handoff path; the path itself is reported rather than hard-asserted because valkey 9.0's replica selection on shutdown requires exact ack-offset equality and intermittently falls back to failure detection Full e2e suite on Kind: 41 of 41 specs passed. On the handoff path the writer recorded 7904 acknowledged writes with a 0.23s longest gap and zero lost. Signed-off-by: Sagar Utekar <sagarutekar2366@gmail.com>
|
|
||
| script := fmt.Sprintf( | ||
| "for i in $(seq 0 %d); do v=$(valkey-cli -t 2 -c get e2e:cw:$i 2>/dev/null | tail -n 1); echo \"$i $v\"; done", maxIdx) | ||
| output, err := execValkeyPodShell(pod, script) |
There was a problem hiding this comment.
Right now every key starts a new valkey-cli process
Instead is there anyway we can open valey-cli once and do these commands through a single instance.
Per review, stop spawning one valkey-cli process per key when verifying data after the disruption. Both read-back paths now pipe their GETs into one valkey-cli over stdin, with an ECHO KEY:<index> marker before each GET so the raw-mode output is correlated per key without relying on line ordering. Missing keys print an empty line and simply leave no entry. The continuous writer intentionally keeps one process per attempt: each attempt samples fresh-connection availability (what a refilling client pool experiences during the disruption) and records a per-attempt timestamp between commands, which a single long-lived instance would not measure. Verified on Kind: spec passes with the handoff engaged, 7597 acknowledged writes, all read back through the batched path. Signed-off-by: Sagar Utekar <sagarutekar2366@gmail.com>
Apply the same single-instance pattern to writeTestKeys: all seed SETs are piped through one valkey-cli, counting the OK responses. Only the continuous writer keeps one process per attempt, deliberately, to sample fresh-connection availability during the disruption. Signed-off-by: Sagar Utekar <sagarutekar2366@gmail.com>
4 participants
This PR closes #270
Summary
Adds the E2E test deferred from #268: verify that when a primary pod is gracefully terminated, the
shutdown-on-sigterm failoverdirective hands the shard off to a replica before the pod exits, so the shard keeps a writer through the disruption and no data is lost.Features / Behaviour Changes
Test-only change; no operator behaviour is modified. The new spec runs under the
failoverGinkgo label.Implementation
The test follows the outline in #270:
ValkeyClusterwithshards: 3, replicas: 1and waits forReady.role:masterwithin the 30s grace window (Eventuallytimeout =terminationGracePeriodSeconds), i.e. the handover beat SIGKILL.role:slave,cluster_state:ok, and all 50 keys read back intact.Two things reviewers may want to pay attention to, both learned from runs of this test against Kind:
INFO replication, not fromValkeyNode.status.role. Right after cluster formation the status can report two primaries for a shard (every node boots as a master beforeCLUSTER REPLICATE, and the status refresh lags) — this is the staleness described in [enhancement] Event-driven ValkeyNode reconcile for timely Status.Role after failover / cluster changes #261, and the first draft of this test flaked on exactly that. The replica is also only accepted oncemaster_link_status:up, so the failover is not attempted against a still-syncing replica.VALKEYCLI_AUTHis unset before runningvalkey-cliinside the server container (execValkeyPodShellhelper). The operator injects that variable for the probe scripts, andvalkey-cliauto-sendsAUTHas the default user whenever it is set — which fails (ERR AUTH ... without any password configured for the default user) and pollutes command output. Commands run as the default nopass user, consistent with the rest of the e2e suite. The_operatoruser cannot be used instead because its ACL has noSET/GET.Limitations
kubectl delete pod(graceful, default grace period) rather than a node drain; the issue notes name this as the acceptable lighter proxy for CI. A drain-based variant can be layered on later.terminationGracePeriodSeconds; the observed promotion latency in practice is ~4s.Testing
Run against a 3-node Kind cluster via:
Result:
1 Passed | 0 Failedin 159s. Timeline from the passing run: SIGTERM at22:21:06.4, replica reportedrole:masterby22:21:10.1(~4s, well inside the 30s grace period), replaced pod rejoined as replica ~4s later,cluster_state:ok,readable=50keys plus the write made during the disruption.go vet -tags=e2eandgolangci-lint run --build-tags e2eare clean for the new file.Checklist
Before submitting the PR make sure the following are checked:
pre-commit run --all-filesor hooks on commit)