Repository navigation
fix(backups): stop the redis backup demo from reading empty values - #4399
Conversation
The redis backup demo read and wrote its marker key from a throwaway `kubectl run -i --rm` Pod. kubectl returns that Pod's stdout over an attach, the container runtime delivers only what the container writes after the attach is registered, and kubectl falls back to the Pod log only when the attach fails. A redis-cli that finishes in between comes back as an empty reply with exit 0, which is how a GET of a key the master held read as '' and failed the round-trip on PRs that do not touch redis. Run redis-cli by `kubectl exec` in a sentinel Pod instead, whose image already carries it. An exec'd process writes into pipes created with it, so nothing it prints is lost, and its exit status comes back too. The demo's reads also stop discarding stderr. The same empty string used to stand for a missing object, an API error and a lost stream alike, so a red run could not say which one it was. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
The redis backup demo read cozy-default-redis once and gave up if it was absent. That strategy is rendered behind a lookup of the platform bucket, so it only exists after a Helm upgrade that runs once the bucket is provisioned. The backupstrategy-controller forces that upgrade, but helm-controller will not run it while a release in the chart's dependsOn is not Ready. A platform change shortly before the suite starts is enough to hold it back. The e2e install patches the platform package late (exposed services, then OIDC), the operator rewrites the cozystack-basics release, which then waits on its own dependencies, and backupstrategy-controller waits on cozystack-basics. On one run the strategy was still missing eight minutes after those patches, and the demo's single read failed the suite as if the backups stack were not installed. Wait for the object with a bound instead. 15 minutes covers the nine minutes seen between those patches and the strategy's creation on another run, and still leaves the demo inside its 30 minute step. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
github-actions
Bot
added
area/storage
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: cozystack/cozystack/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe Redis backup demo now exposes kubectl errors, waits for the platform Redis backup strategy, and executes Redis commands through a sentinel Pod. New BATS tests cover password handling, command failures, and missing master responses. ChangesRedis backup demo
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Low Merge Risk: ⚪ Minimal · up to The demo waits for the backup Strategy and protects password delivery over stdin. No identified regression remains to block merging after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
github-actions
Bot
added
the
size/L
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
LGTM. This does what the title says: the demo fails loudly now instead of comparing against a value a swallowed error left empty, and the new bats test locks the exec-based helper in place. I ran the test (4/4 green) and mutated the sentinel guard to confirm it actually reddens; the exec addressing (rfs-redis-<app>, container sentinel, sentinel mymaster on 26379) matches what the in-repo chainsaw test execs, so the switch away from the throwaway Pod is sound. Three non-blocking notes below, all in examples/ and hack/.
Tested against the PR head; helpers verified offline with stubs, no cluster touched.
| # that derives the same base from .Release.Name. So app X's password lives in | ||
| # redis-X-auth. Empty when auth is disabled. | ||
| # redis-X-auth. The chart renders that Secret only with authEnabled, so its | ||
| # absence reads as no password; any other error still fails the read. |
There was a problem hiding this comment.
[MINOR] "any other error still fails the read" holds only for the direct calls
This line reads true for the direct redis_cmd … >/dev/null calls (01:30, 03:14), but not for the got=$(redis_cmd …) sites (01:31, 03:18, 03:36, 04:49) or h=$(redis_cmd …) at 00-helpers.sh:153. bash turns off errexit inside $(…) (inherit_errexit is off by default), so a non-NotFound failure of redis_pw at line 136 does not abort redis_cmd — it goes on to run the exec with an empty password. A real Secret-read error (RBAC denial, API timeout) then surfaces as a marker mismatch or a master-election timeout rather than as the read failure the comment promises. kubectl's own error still reaches stderr and the demo still fails, so this is about the diagnostic and the comment's accuracy, not a false-pass. Verified locally: with set -euo pipefail, got=$(redis_cmd …) reaches the next line with rc 0 and an empty password when the inner read returns non-zero; the direct call aborts as the comment claims.
| # a platform change made just before this runs can keep it absent for minutes. | ||
| # kubectl wait keeps polling through NotFound and fails on any other error. | ||
| log_substep "Waiting for the platform's cozy-default-redis strategy..." | ||
| kubectl wait --for=create redis.strategy.backups.cozystack.io/cozy-default-redis \ |
There was a problem hiding this comment.
[NIT] kubectl wait --for=create needs kubectl >= 1.31; the fallback message misattributes on older clients
--for=create landed in kubectl 1.31. On 1.30 it fails before any API call with error: unrecognized condition: "create" (verified locally), and the demo then logs cozy-default-redis strategy did not appear: enable the platform default backups …, which points at the wrong cause — the kubectl error is visible just above it, so it is misdirection rather than silence. The e2e sandbox pins a newer kubectl, so CI is unaffected, but examples/ is user-facing. A one-line version note, or keying the failure message off the actual error, would save an operator on an older client a wrong turn.
| [ -n "${STUB_NO_MASTER:-}" ] || printf '10.0.0.5\n6379\n' | ||
| exit 0 | ||
| fi | ||
| printf '%s|%s\n' "${REDISCLI_AUTH-unset}" "$*" >> "$STUB_DIR/redis-cli.calls" |
There was a problem hiding this comment.
[NIT] load-bearing REDISCLI_AUTH ordering and the sentinel-failure path are unpinned
The order that matters — REDISCLI_AUTH is exported after the sentinel query, so master discovery runs unauthenticated — is not held by any case: this stub's sentinel branch does not record REDISCLI_AUTH, so moving the export above the query survives all four tests. Same for the || exit 1 on the sentinel query (the stub always exits 0, so a sentinel connection failure is never exercised). The helper is correct as written; this is only about the test's discriminating power on two edits it would wave through.
2 participants
What this PR does
The redis backup round-trip suite went red on PRs that do not touch redis, each time on an empty read (#4395). The report snapshots of those runs point to two separate causes, and I fix both here.
The empty marker reads came from the demo helper that ran redis-cli in a throwaway
kubectl run -i --rmPod. kubectl gets that Pod's stdout over an attach, and containerd's attach only carries output written after the attach is registered. kubectl falls back to the Pod log only when the attach fails. A redis-cli that finishes in that gap comes back as an empty string with exit 0. In the failed runs, the master's log shows the key present, and sentinel logged no failover, yet the GET read ''.redis_cmdnow runs redis-cli bykubectl execin a sentinel Pod, which already has it. An exec'd process gets its own pipes, so its output can't be lost that way, and its exit code comes back. The password now goes over stdin rather than a Pod env var.The missing-strategy failure was real:
cozy-default-redisdid not exist yet. It gets rendered behind a lookup of the platform bucket, so it appears only after a forced Helm upgrade. helm-controller holds that upgrade whilecozystack-basicsis not Ready, and the e2e install's late platform patches keep it not Ready for several minutes. The demo read the object once. It now waits for the object withkubectl wait --for=createand a 15 minute bound. That wait fails right away on anything other than NotFound.The demo's reads also stop sending stderr to
/dev/null, so the next failure names its cause.hack/redis-backup-demo-helpers.batscoversredis_cmdwith stubbed kubectl and redis-cli: it reads through exec, keeps the password out of argv, sends no AUTH when there is no Secret, and fails loudly when the exec fails or sentinel has no master.Closes #4395
Screenshots
No UI changes.
Downstream repositories
The diff touches only
examples/backups/redis/and a new unit test underhack/. No entry in the trigger map covers either.Release note
Summary by CodeRabbit