Repository navigation
fix(backups): share the walkthrough wait helpers and fix the S3 preflight cleanup - #4635
Conversation
mc alias set reaches the endpoint through the port-forward, so a tunnel that came up and then broke is a likely cause of that failure, and its log is the only record of it. The readiness and retry-timeout exits already print it; this one threw it away. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
The exit trap stopped the port-forward before running mc rm, so the delete went to a closed port and failed every time, leaving the object behind and printing a warning that said nothing about why. The delete now runs first and the warning carries mc's own error, or says the delete timed out when mc was killed before it could write one. A PUT that timed out is now also treated as uploaded: it can land and still outlive its time bound, and the trap never tried to remove such an object. A PUT the server refused is not, so the common case of access that never went live does not end in a delete doomed to the same refusal. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
kafka_run seeded and read topics through a throwaway `kubectl run -i` Pod per call. That Pod's stdout returns over an attach that carries only what the container writes after the attach registers, so a CLI that finishes first reads as an empty reply with exit 0 -- the empty-read defect already fixed in the redis, nats and kafka-metadata demos. The topic count, dump and replication-factor checks all read that reply. Run one long-lived CLI Pod, keeping the restricted security context, and exec into it. It carries its own name and owner label so it cannot collide with the kafka-metadata demo's Pod in a shared namespace, and cleanup.sh removes it by that label. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
Every backup walkthrough carried its own wait_for_field, and most their own wait_hr_ready and wait_deleted: twelve, eight and two copies, which had already drifted into five, three and two bodies. A fix made in one copy did not reach the others, and a diagnostics change carried by hand between two of them once went into the wrong function and was caught only by reading the diff. The helpers now live in examples/backups/_lib/wait-helpers.sh, sourced by each walkthrough's 00-helpers.sh. Where copies differed, the shared body keeps the diagnostics any of them had: wait_for_field leaves kubectl's stderr attached, and wait_hr_ready prints the conditions and history dump on Stalled or timeout. Call sites and budgets are unchanged. A new unit test fails when any walkthrough defines one of these helpers again, which replaces the byte-identity pin between copies. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
… to fail Both cases passed the expected kubectl error to the stub through an environment variable named on the command that runs the helper. The unit runner traces that command, with the value expanded, into the very stderr the case then greps, so the case stayed green with the helper's stderr sent to /dev/null. The stub now prints the message itself and the variable is only a flag. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
… refused Both demos read their CLI output through command substitutions, where bash does not apply errexit. When the ownership check refused a same-named Pod another party owns, the refusal was ignored and the exec went into that very Pod. A failed read of the Pod's phase or owner was likewise read as "no Pod", so the helper went on to delete and replace a Pod whose owner it had not seen. Every step of the Pod setup now returns on failure, and the phase and owner reads tell a missing Pod (--ignore-not-found) from a failed read. 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 (22)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe PR centralizes wait helpers used by backup walkthroughs, changes CLI Pod handling in Kafka-related demos, and updates S3 preflight cleanup and failure reporting. It also adds and updates Bats tests for these behaviors. ChangesShared backup wait helpers
Kafka topic demo CLI Pod
Backup CLI Pod error handling
S3 backup access preflight
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~40 minutes Change: Bug fix · Severity of issue fixed: Low Sequence Diagram(s)sequenceDiagram
participant kafka_run
participant kafka_cli_pod
participant kubectl
participant KafkaCLI as Kafka CLI Pod
kafka_run->>kafka_cli_pod: Ensure the named Pod is ready
kafka_cli_pod->>kubectl: Read Pod phase and ownership
kubectl-->>kafka_cli_pod: Return Pod state
kafka_cli_pod->>kubectl: Create Pod if needed and wait for Ready
kafka_cli_pod-->>kafka_run: Return setup result
kafka_run->>kubectl: Execute snippet in the named Pod
kubectl->>KafkaCLI: Run Kafka CLI command
KafkaCLI-->>kafka_run: Return command output
Merge Risk: ⚪ Minimal · up to This change moves the backup walkthrough wait helpers into one shared file and makes the Kafka, NATS, and S3 preflight failure paths stop earlier with clearer diagnostics. No concrete regression was found, and new tests cover the changed behavior, so the change looks ready to merge. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The main security-sensitive change is the reusable Kafka CLI Pod. Its ownership check relies on a mutable label, so safe reuse depends on who can manage Pods in the namespace. New Pods receive restricted security settings, failures stop execution, and S3 cleanup ordering improves. A cross-tenant exploit has not been established. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Out of Scope Changes checkExplanation The PR contains changes not connected to the linked issue objectives. The Kafka topic demo now uses a long-lived CLI Pod and changes Pod ownership, security, creation, and cleanup behavior. The NATS and kafka-metadata helpers also change Pod lookup, deletion, creation, and exec error propagation. Their README, cleanup, and dedicated tests support those changes. The linked issues cover shared backup wait helpers and S3 preflight diagnostics and cleanup, not these Kafka or NATS Pod lifecycle changes. Full details: Docstring CoverageExplanation Docstring coverage is 38.46% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 52 functions across 21 files. (1 skipped: 1 unsupported.)
✨ 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 |
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM with non-blocking notes
This only touches examples and hack, so nothing here lands on a customer cluster. What decides a consolidation like this is whether any copy of the helpers survived somewhere, whether the S3 preflight delete can reach anything but its own probe object, and whether the rewritten bats suites still bite. All three check out.
Findings
- [NIT]
examples/backups/_lib/wait-helpers.sh:49, vmi walkthrough loses its per-poll progress dot
- [NIT]
hack/e2e-chainsaw/_lib/backup-access-preflight.sh:121, a timed-out PUT that never landed produces a spurious cleanup warning
- [NIT]
examples/backups/kafka/00-helpers.sh:152, kafka_cli_pod reuses a Pod that is still terminating
Checked and correct
grep -rnE '(wait_for_field|wait_hr_ready|wait_deleted)\s*\(\)' over examples, packages and hack returns only examples/backups/_lib/wait-helpers.sh. Six of the per-db wait_hr_ready copies were byte-identical to the shared body already; redis and kafka-metadata had the short form and converge upward. No walkthrough loses a timeout, a flag, a namespace or an exit code. The 3580/3769 red-on-merge precedent doesn't apply here: backup-examples-shared-helpers.bats enforces single-source now, which is stronger than the byte-identity pin it replaces. The unchanged suites that still read examples/backups (redis, postgres, kafka-topic-strategy) are green at this head too.
On the S3 side, mc rm targets exactly backup-preflight/${bucket}/${object} with object=.cozystack-e2e-preflight/${access_name}-$(date +%s)-$$, set once before the PUT loop, and there's no --recursive. uploaded=true implies object is non-empty. The change arms a delete that used to be inert (it ran after the port-forward was already killed) and widens it to the 124 case. Neither path can reach tenant data.
Each mutation below was run here and reverted. Removing the cleanup's new mc rm block reddens backup-access-preflight.bats 9/10/12 while 11 stays green (nothing to delete). Removing the 124 branch reddens 10. Dropping --ignore-not-found or restoring 2>/dev/null in wait_for_field each redden their own backup-examples-shared-helpers.bats test. A bare kafka_cli_pod call reddens the kafka-topic ownership and cannot-read tests. All six touched suites are green at head and shellcheck -S warning is clean on the new lib and the preflight.
Caveats
- Walkthrough directories aren't self-contained anymore: each
00-helpers.shsources../_lib/wait-helpers.sh, so copying a singleexamples/backups/<db>/directory elsewhere breaks sourcing. The e2e sandbox is unaffected (docker cpof the whole tree), and the PR body already says the READMEs aren't updated. hack/select-e2e.shmapsexamples/backups/<app>/by directory name and skips_lib, so a future diff touching only the shared file selects no backup e2e suite, while the analogoushack/e2e-chainsaw/_lib/path escalates to the full run. Not triggered by this PR since every walkthrough is touched; already tracked as #4626.
| log_error "Timeout waiting for $resource_type/$resource_name (current: '$current', expected: '$desired')" | ||
| return 1 | ||
| fi | ||
| sleep 5 |
There was a problem hiding this comment.
[NIT] vmi walkthrough loses its per-poll progress dot
The vmi copy of wait_for_field on the merge base printed echo -n "." >&2 after every sleep 5; the shared body does not. During the vmi walkthrough's long waits (up to 600s) there is now no liveness signal on the terminal until the wait resolves. Cosmetic, and the only behaviour the per-db comparison found dropped: every other copy converges upward (redis and kafka-metadata gain the conditions/history dump in wait_hr_ready, foundationdb/nats/vmi gain the unused fail_value arg). If the dot is wanted back, it belongs in the shared body, not in a vmi override.
| "backup-preflight/${bucket}/${object}" >>"${workdir}/mc.log" 2>&1 || put_rc=$? | ||
| # A PUT cut off by its time bound (124) may have landed all the same. One | ||
| # the server refused did not, and deleting it would only fail again. | ||
| if [ "${put_rc}" -eq 0 ] || [ "${put_rc}" -eq 124 ]; then |
There was a problem hiding this comment.
[NIT] a timed-out PUT that never landed produces a spurious cleanup warning
Setting uploaded=true on exit 124 is the right call, since a PUT cut off by timeout may have landed. The flip side: when it did not land, cleanup runs mc rm on a key that does not exist, gets a non-zero exit, and prints WARNING: could not remove failed S3 preflight object ... followed by mc's own "does not exist" line. The round trip is already failing at that point, so the warning sends the reader chasing a cleanup problem that is not one. The every PUT was refused test covers the refused case (non-124), not this one. Consider treating an mc rm that reports the object as absent as success, or saying in the warning that absence is expected after a 124. Not verified against a live mc; derived from the comment at :117-118 and mc's documented behaviour.
| log_error "Pod $NAMESPACE/$KAFKA_TOPIC_CLI_POD exists but this demo does not own it; refusing to use or delete it" | ||
| return 1 | ||
| fi | ||
| if [ "$phase" != "Running" ] && [ "$phase" != "Pending" ]; then |
There was a problem hiding this comment.
[NIT] kafka_cli_pod reuses a Pod that is still terminating
kafka_cli_pod treats phase=Running as reusable without checking metadata.deletionTimestamp. cleanup.sh deletes the CLI Pod with --grace-period=1 and does not wait, so for a few seconds afterwards the Pod is Terminating while its phase still reads Running. A kafka_run inside that window reuses it and the subsequent kubectl wait --for=condition=Ready fails when the Pod vanishes. The window is seconds wide and run-all.sh places the first kafka_run minutes after cleanup, so this is unlikely to bite in the documented flow; it is new for the kafka topic demo (the old throwaway-Pod design had no reuse path) and the same shape already exists in the nats and kafka-metadata helpers. A deletionTimestamp check next to the phase read closes it.
2 participants
What this PR does
This fixes the S3 access preflight and a few backup walkthroughs, and moves the walkthrough wait helpers into one shared file.
The access preflight now keeps its evidence and cleans up after itself. When
mc alias setfailed, it dropped the port-forward log, and now it prints it like the other exits do. The exit trap used to kill the port-forward beforemc rm, so the delete always hit a closed port and the warning gave no reason. The delete now runs first, and the warning carries mc's own error, or says the delete timed out when mc was killed before writing one. A PUT that timed out (rc 124) may have landed, so it counts as uploaded. A PUT the server refused does not, so access that never went live gets no delete and no warning.The wait helpers were copied into every walkthrough and had drifted.
wait_for_field,wait_hr_readyandwait_deletedhad 12, 8 and 2 copies with 5, 3 and 2 different bodies. They now live inexamples/backups/_lib/wait-helpers.sh, and each00-helpers.shsources it. Call sites and wait budgets are unchanged. Where copies differed, the shared body keeps every diagnostic any copy had. What changes per walkthrough:wait_for_fieldin every walkthrough but redis now shows kubectl's stderr, so a Forbidden or transport error is visible instead of reading as an empty field.--ignore-not-foundkeeps a resource that does not exist yet from printing an error on every poll. Redis already kept stderr.wait_for_fieldin foundationdb, nats and vmi gains the optional 7th terminal-value argument. None of their callers pass it, so nothing changes at runtime.wait_hr_readyin redis and kafka-metadata now prints every condition and the release history on Stalled or timeout. It also says whether the release never appeared, was deleted, or could not be looked up.wait_deletedtimeout message reads "it is still present" instead of "still present".hack/backup-examples-shared-helpers.batsgoes red if a walkthrough defines one of these helpers again or stops providing them, and it pins both sides of thewait_for_fieldread: kubectl's reason for a failed read is shown, a missing resource prints nothing. The byte-identity pin between copies inhack/mariadb-first-boot-waits.batsis gone, and its errexit test runs through the mariadb walkthrough's helpers.The kafka topic-data demo had the empty-read race from #4400.
kafka_runread topic state through a throwawaykubectl run -iPod. It now execs into a long-lived CLI Pod that keeps the restricted security context, andcleanup.shremoves it by label. The Pod name comes from its ownKAFKA_TOPIC_CLI_POD, so setting the kafka-metadata demo'sKAFKA_CLI_PODdoes not move it. #4400 itself was already fixed on main for nats, kafka-metadata and rabbitmq.The nats and kafka-metadata demos could exec into a Pod they had just refused. They read CLI output inside
$(...), where bash does not apply errexit, so the ownership refusal was ignored. A failed read of the Pod's phase was treated as no Pod, and the demo went on to delete and replace it. Every setup step now returns on failure. Their "exec fails with kubectl's own error" tests could never fail: the runner's trace wrote the expected message into the same stderr the test grepped. The stub now prints the message itself.Each fix has a test that fails on main. For each fix I also reverted it and checked that only its own test goes red.
One gap stays open outside this PR.
hack/select-e2e.shmapsexamples/backups/<app>/paths by app name, so a later PR that changes onlyexamples/backups/_lib/wait-helpers.shwould select no suite. This PR is fine because it touches every walkthrough. The rule belongs to the select-e2e owners and is tracked in #4626.While reviewing I also found that the kafka README puts steps that create and exec into Pods under the tenant persona, and the tenant role can do neither. That is #4625, left for its own change.
Left out on purpose, because they are outside what this diff changes:
../_libnext to it.hack/mariadb-first-boot-waits.batsloops over one file. I kept it so the rebase against test(e2e): let HR-Ready waits outlast the install timeout of the release they wait on #4471 stays mechanical.Closes #3757, closes #4243, closes #4244
Screenshots
No UI changes.
Downstream repositories
Release note
Summary by CodeRabbit
New Features
Bug Fixes