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>
📝 WalkthroughWalkthroughThe E2E tests now use authenticated Valkey clusters, discover live shard roles, track writes during primary termination, verify replica promotion within the grace period, and confirm acknowledged and pre-failover keys remain readable. ChangesValkey failover E2E coverage
Sequence Diagram(s)sequenceDiagram
participant FailoverE2E
participant PrimaryPod
participant ReplicaPod
participant ContinuousWriter
FailoverE2E->>PrimaryPod: discover role and seed keys
FailoverE2E->>ContinuousWriter: start authenticated writes
FailoverE2E->>PrimaryPod: terminate primary
ReplicaPod->>ReplicaPod: promote to primary
FailoverE2E->>ReplicaPod: verify promotion
ContinuousWriter-->>FailoverE2E: report acknowledged writes
FailoverE2E->>ReplicaPod: verify key persistence
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: build linters: plugin(logcheck): plugin "logcheck" not found 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/e2e/valkeycluster_test.go (1)
2199-2200: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the authenticated-exec construction from
execValkeyPodShell.Lines 2199-2200 repeat the
kubectl exec ... sh -ccommand and theVALKEYCLI_AUTHexport from lines 2078-2079. The two copies must stay in sync.startContinuousWriterneeds the*exec.Cmdinstead of the output, so extract a small builder and let both call it.♻️ Proposed refactor
+// valkeyPodShellCmd builds a kubectl exec command that runs the script in the +// pod's server container with VALKEYCLI_AUTH set to the default user's +// password. +func valkeyPodShellCmd(pod string, script string) *exec.Cmd { + return exec.Command("kubectl", "exec", pod, "-c", "server", "--", + "sh", "-c", fmt.Sprintf("export VALKEYCLI_AUTH=%q; ", failoverDefaultPassword)+script) +} + func execValkeyPodShell(pod string, script string) (string, error) { - cmd := exec.Command("kubectl", "exec", pod, "-c", "server", "--", - "sh", "-c", fmt.Sprintf("export VALKEYCLI_AUTH=%q; ", failoverDefaultPassword)+script) - return utils.Run(cmd) + return utils.Run(valkeyPodShellCmd(pod, script)) }- cmd := exec.Command("kubectl", "exec", pod, "-c", "server", "--", - "sh", "-c", fmt.Sprintf("export VALKEYCLI_AUTH=%q; ", failoverDefaultPassword)+script) + cmd := valkeyPodShellCmd(pod, script)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/valkeycluster_test.go` around lines 2199 - 2200, Extract the shared authenticated kubectl command construction from execValkeyPodShell into a small builder that returns *exec.Cmd, preserving the pod, server container, shell, and VALKEYCLI_AUTH setup. Update both execValkeyPodShell and startContinuousWriter to use this builder, while keeping startContinuousWriter’s command-based flow unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/valkeycluster_test.go`:
- Around line 1169-1178: Validate that shardIndex is non-empty immediately after
utils.Run succeeds and before invoking getShardRoles in the failover flow. Add
an explicit assertion with a clear message identifying the missing
valkey.io/shard-index label, while preserving the existing Eventually checks for
primary and replica pods.
- Around line 1132-1136: Update the pre-create cleanup for the failover cluster
to delete both the ValkeyCluster and its associated users Secret before running
kubectl create. Reuse the existing failoverClusterName and ignore-not-found
behavior, while preserving the deferred cleanup after the test.
- Around line 2251-2275: Update verifyAcknowledgedWrites to retry the read-back
using the same Eventually pattern and timing used by verifySeededKeys. Retry
readKeysBatch until transient errors and stale MOVED responses resolve, while
preserving the existing acknowledged-key comparison and failure message after
retries are exhausted.
---
Nitpick comments:
In `@test/e2e/valkeycluster_test.go`:
- Around line 2199-2200: Extract the shared authenticated kubectl command
construction from execValkeyPodShell into a small builder that returns
*exec.Cmd, preserving the pod, server container, shell, and VALKEYCLI_AUTH
setup. Update both execValkeyPodShell and startContinuousWriter to use this
builder, while keeping startContinuousWriter’s command-based flow unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 985212dc-e330-4a3a-b530-bfeb7ce3b333
📒 Files selected for processing (1)
test/e2e/valkeycluster_test.go
| defer func() { | ||
| cmd := exec.Command("kubectl", "delete", "valkeycluster", failoverClusterName, "--ignore-not-found=true", "--wait=false") | ||
| _, _ = utils.Run(cmd) | ||
| cmd = exec.Command("kubectl", "delete", "secret", failoverClusterName+"-users", "--ignore-not-found=true") | ||
| _, _ = utils.Run(cmd) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Delete the Secret before the create step, not only after the test.
The cleanup deletes the Secret at line 1135. The pre-create cleanup at line 1140 deletes only the ValkeyCluster. If a previous run aborted before the deferred cleanup ran, the Secret survives. kubectl create -f then fails with AlreadyExists and the whole spec fails on rerun. Add the Secret to the pre-create cleanup, or use kubectl apply -f.
🛠️ Proposed fix
By("applying the CR")
cmd := exec.Command("kubectl", "delete", "valkeycluster", failoverClusterName, "--ignore-not-found=true")
_, _ = utils.Run(cmd)
+ cmd = exec.Command("kubectl", "delete", "secret", failoverClusterName+"-users", "--ignore-not-found=true")
+ _, _ = utils.Run(cmd)
cmd = exec.Command("kubectl", "create", "-f", manifestFile)🧰 Tools
🪛 ast-grep (0.45.0)
[error] 1134-1134: An argument passed to exec.Command/exec.CommandContext is built by concatenating a string literal with dynamic input. If that input is attacker-controlled (and especially when the command is a shell such as sh -c/bash -c), this enables OS command injection. Pass untrusted data as separate, fixed arguments instead of interpolating it into a command string, avoid invoking a shell, and validate/escape the input where a shell is unavoidable.
Context: exec.Command("kubectl", "delete", "secret", failoverClusterName+"-users", "--ignore-not-found=true")
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(command-injection-exec-concat-arg-go)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/valkeycluster_test.go` around lines 1132 - 1136, Update the
pre-create cleanup for the failover cluster to delete both the ValkeyCluster and
its associated users Secret before running kubectl create. Reuse the existing
failoverClusterName and ignore-not-found behavior, while preserving the deferred
cleanup after the test.
| cmd = exec.Command("kubectl", "get", "statefulset", primaryStatefulset, | ||
| "-o", "jsonpath={.metadata.labels.valkey\\.io/shard-index}") | ||
| shardIndex, err := utils.Run(cmd) | ||
| Expect(err).NotTo(HaveOccurred(), "Failed to get shard index of the primary statefulset") | ||
| var primaryPod, replicaPod string | ||
| Eventually(func(g Gomega) { | ||
| primaryPod, replicaPod = getShardRoles(g, failoverClusterName, shardIndex) | ||
| g.Expect(primaryPod).NotTo(BeEmpty(), "shard has no primary") | ||
| g.Expect(replicaPod).NotTo(BeEmpty(), "shard has no in-sync replica") | ||
| }).WithTimeout(3 * time.Minute).Should(Succeed()) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert shardIndex is non-empty before you use it as a label selector.
If the valkey.io/shard-index label is absent, utils.Run returns an empty string with no error. getShardRoles then builds the selector valkey.io/shard-index=, which matches no pods, and the Eventually block fails after 3 minutes with "shard has no primary". That message hides the real cause. Add an explicit check.
🛠️ Proposed fix
shardIndex, err := utils.Run(cmd)
Expect(err).NotTo(HaveOccurred(), "Failed to get shard index of the primary statefulset")
+ shardIndex = strings.TrimSpace(shardIndex)
+ Expect(shardIndex).NotTo(BeEmpty(),
+ "statefulset %s has no valkey.io/shard-index label", primaryStatefulset)
var primaryPod, replicaPod string📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cmd = exec.Command("kubectl", "get", "statefulset", primaryStatefulset, | |
| "-o", "jsonpath={.metadata.labels.valkey\\.io/shard-index}") | |
| shardIndex, err := utils.Run(cmd) | |
| Expect(err).NotTo(HaveOccurred(), "Failed to get shard index of the primary statefulset") | |
| var primaryPod, replicaPod string | |
| Eventually(func(g Gomega) { | |
| primaryPod, replicaPod = getShardRoles(g, failoverClusterName, shardIndex) | |
| g.Expect(primaryPod).NotTo(BeEmpty(), "shard has no primary") | |
| g.Expect(replicaPod).NotTo(BeEmpty(), "shard has no in-sync replica") | |
| }).WithTimeout(3 * time.Minute).Should(Succeed()) | |
| cmd = exec.Command("kubectl", "get", "statefulset", primaryStatefulset, | |
| "-o", "jsonpath={.metadata.labels.valkey\\.io/shard-index}") | |
| shardIndex, err := utils.Run(cmd) | |
| Expect(err).NotTo(HaveOccurred(), "Failed to get shard index of the primary statefulset") | |
| shardIndex = strings.TrimSpace(shardIndex) | |
| Expect(shardIndex).NotTo(BeEmpty(), | |
| "statefulset %s has no valkey.io/shard-index label") | |
| var primaryPod, replicaPod string | |
| Eventually(func(g Gomega) { | |
| primaryPod, replicaPod = getShardRoles(g, failoverClusterName, shardIndex) | |
| g.Expect(primaryPod).NotTo(BeEmpty(), "shard has no primary") | |
| g.Expect(replicaPod).NotTo(BeEmpty(), "shard has no in-sync replica") | |
| }).WithTimeout(3 * time.Minute).Should(Succeed()) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/valkeycluster_test.go` around lines 1169 - 1178, Validate that
shardIndex is non-empty immediately after utils.Run succeeds and before invoking
getShardRoles in the failover flow. Add an explicit assertion with a clear
message identifying the missing valkey.io/shard-index label, while preserving
the existing Eventually checks for primary and replica pods.
| func verifyAcknowledgedWrites(pod string, acked map[string]string) { | ||
| GinkgoHelper() | ||
|
|
||
| maxIdx := 0 | ||
| for idx := range acked { | ||
| var i int | ||
| _, err := fmt.Sscanf(idx, "%d", &i) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| if i > maxIdx { | ||
| maxIdx = i | ||
| } | ||
| } | ||
|
|
||
| readable, err := readKeysBatch(pod, "e2e:cw:", 0, maxIdx) | ||
| Expect(err).NotTo(HaveOccurred(), "Failed to read back acknowledged writes") | ||
|
|
||
| var lost []string | ||
| for idx, want := range acked { | ||
| if readable[idx] != want { | ||
| lost = append(lost, idx) | ||
| } | ||
| } | ||
| Expect(lost).To(BeEmpty(), | ||
| fmt.Sprintf("%d acknowledged write(s) were lost across the handoff: %v", len(lost), lost)) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Retry the read-back, as verifySeededKeys does.
verifyAcknowledgedWrites runs at line 1223, immediately after the disruption and before the cluster-recovery wait at line 1267. At that moment the shard can still return transient errors or stale MOVED redirects, and readKeysBatch uses a 2-second connection timeout. A single failed batch read reports every acknowledged key as lost, which makes the spec flaky. verifySeededKeys already guards against this with Eventually. Apply the same pattern here.
🛠️ Proposed fix
maxIdx := 0
for idx := range acked {
var i int
_, err := fmt.Sscanf(idx, "%d", &i)
Expect(err).NotTo(HaveOccurred())
if i > maxIdx {
maxIdx = i
}
}
- readable, err := readKeysBatch(pod, "e2e:cw:", 0, maxIdx)
- Expect(err).NotTo(HaveOccurred(), "Failed to read back acknowledged writes")
-
- var lost []string
- for idx, want := range acked {
- if readable[idx] != want {
- lost = append(lost, idx)
- }
- }
- Expect(lost).To(BeEmpty(),
- fmt.Sprintf("%d acknowledged write(s) were lost across the handoff: %v", len(lost), lost))
+ Eventually(func(g Gomega) {
+ readable, err := readKeysBatch(pod, "e2e:cw:", 0, maxIdx)
+ g.Expect(err).NotTo(HaveOccurred(), "Failed to read back acknowledged writes")
+
+ var lost []string
+ for idx, want := range acked {
+ if readable[idx] != want {
+ lost = append(lost, idx)
+ }
+ }
+ g.Expect(lost).To(BeEmpty(),
+ fmt.Sprintf("%d acknowledged write(s) were lost across the handoff: %v", len(lost), lost))
+ }).Should(Succeed())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func verifyAcknowledgedWrites(pod string, acked map[string]string) { | |
| GinkgoHelper() | |
| maxIdx := 0 | |
| for idx := range acked { | |
| var i int | |
| _, err := fmt.Sscanf(idx, "%d", &i) | |
| Expect(err).NotTo(HaveOccurred()) | |
| if i > maxIdx { | |
| maxIdx = i | |
| } | |
| } | |
| readable, err := readKeysBatch(pod, "e2e:cw:", 0, maxIdx) | |
| Expect(err).NotTo(HaveOccurred(), "Failed to read back acknowledged writes") | |
| var lost []string | |
| for idx, want := range acked { | |
| if readable[idx] != want { | |
| lost = append(lost, idx) | |
| } | |
| } | |
| Expect(lost).To(BeEmpty(), | |
| fmt.Sprintf("%d acknowledged write(s) were lost across the handoff: %v", len(lost), lost)) | |
| } | |
| func verifyAcknowledgedWrites(pod string, acked map[string]string) { | |
| GinkgoHelper() | |
| maxIdx := 0 | |
| for idx := range acked { | |
| var i int | |
| _, err := fmt.Sscanf(idx, "%d", &i) | |
| Expect(err).NotTo(HaveOccurred()) | |
| if i > maxIdx { | |
| maxIdx = i | |
| } | |
| } | |
| Eventually(func(g Gomega) { | |
| readable, err := readKeysBatch(pod, "e2e:cw:", 0, maxIdx) | |
| g.Expect(err).NotTo(HaveOccurred(), "Failed to read back acknowledged writes") | |
| var lost []string | |
| for idx, want := range acked { | |
| if readable[idx] != want { | |
| lost = append(lost, idx) | |
| } | |
| } | |
| g.Expect(lost).To(BeEmpty(), | |
| fmt.Sprintf("%d acknowledged write(s) were lost across the handoff: %v", len(lost), lost)) | |
| }).Should(Succeed()) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/valkeycluster_test.go` around lines 2251 - 2275, Update
verifyAcknowledgedWrites to retry the read-back using the same Eventually
pattern and timing used by verifySeededKeys. Retry readKeysBatch until transient
errors and stale MOVED responses resolve, while preserving the existing
acknowledged-key comparison and failure message after retries are exhausted.
| lastAckTime := -1.0 | ||
| for _, line := range utils.GetNonEmptyLines(w.output.String()) { | ||
| fields := strings.Fields(line) | ||
| if len(fields) != 3 { | ||
| continue | ||
| } | ||
| status, idx := fields[0], fields[1] | ||
| var ts float64 | ||
| if _, err := fmt.Sscanf(fields[2], "%f", &ts); err != nil { | ||
| continue | ||
| } | ||
| // The "end" sentinel closes the window: a write outage running | ||
| // through the end of the loop counts as a gap instead of being | ||
| // silently dropped. | ||
| if status != "ack" && status != "end" { | ||
| continue | ||
| } | ||
| if lastAckTime >= 0 && ts-lastAckTime > maxGap { | ||
| maxGap = ts - lastAckTime | ||
| } |
There was a problem hiding this comment.
lastAckTime is initialized only after an ack, while preceding fail records are discarded. If the primary is deleted before the writer's first successful command, an outage longer than the ten-second bound can be followed by one successful write and still report an under-threshold maxGap. Initialize the measurement window from the first valid writer timestamp, including failures, so the first acknowledgement is compared with the start of the observed window.
Artifacts
Authored focused reproduction source
- A runnable Go program feeds identical >10-second initial-failure writer output to current and window-start accounting, showing the comparison scope.
Reference accounting output (before)
- The executed reference run measures the 11.500-second outage before the first acknowledgement and reports a nonempty acknowledged map, showing the expected accounting.
Current accounting output (after)
- The executed current-code run returns `acked=map[3:v3]` and `maxGap=0.100s` for the same 11.500-second initial outage, confirming the defect.
- The captured target source lines show `lastAckTime` is initialized only by acknowledged writes while failures are skipped, explaining the omission.
5 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)