Repository navigation
test(e2e): fix bucket.bats port-forward and S3 client reliability - #2944
myasnikovdaniil wants to merge 3 commits into
Conversation
github-actions
Bot
added
the
area/testing
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the reliability of end-to-end bucket tests by addressing common networking and process management issues. By forcing IPv4 loopback addresses and adjusting how background processes are handled, the changes eliminate intermittent connection failures during test execution. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
github-actions
Bot
added
the
size/S
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe SeaweedFS bucket e2e Bats test is updated to replace a subshell-wrapped ChangesSeaweedFS Bucket E2E Test Fix
Container Startup Probe Configuration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the port-forwarding and connection logic in the E2E bucket tests to run the port-forward process directly in the current shell and target 127.0.0.1 instead of localhost. Feedback on this change points out that the backgrounded kubectl port-forward process will leak because cozytest.sh does not actually terminate the process group as assumed. It is recommended to capture the PID of the port-forward process and clean it up using an EXIT trap.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| # Start port-forwarding in this shell (not via `bash -c '... &'` — the | ||
| # wrapper subshell exits immediately, the backgrounded kubectl becomes an | ||
| # orphan and gets reaped before the first mc S3 request, so `mc cp` sees | ||
| # `dial tcp 127.0.0.1:8333: connect: connection refused` even though the | ||
| # earlier `nc -z` probe succeeded). The forked port-forward inherits the | ||
| # test process group; cozytest.sh kills the whole group when the test | ||
| # function returns, so no explicit cleanup is needed. | ||
| kubectl port-forward service/seaweedfs-s3 -n tenant-root 8333:8333 >/dev/null 2>&1 & |
There was a problem hiding this comment.
The comment states that cozytest.sh kills the whole process group when the test function returns, so no explicit cleanup is needed. However, looking at hack/cozytest.sh, there is no process group termination logic (e.g., sending a signal to the process group). This means the backgrounded kubectl port-forward process will be leaked and continue running after the test finishes, potentially causing port conflicts on port 8333 in subsequent test runs.
Since cozytest.sh runs the test function inside a subshell, we can reliably clean up the background process by capturing its PID and setting an EXIT trap.
# Start port-forwarding in this shell (not via bash -c ... & - the
# wrapper subshell exits immediately, the backgrounded kubectl becomes an
# orphan and gets reaped before the first mc S3 request, so mc cp sees
# dial tcp 127.0.0.1:8333: connect: connection refused even though the
# earlier nc -z probe succeeded).
kubectl port-forward service/seaweedfs-s3 -n tenant-root 8333:8333 >/dev/null 2>&1 &
PORT_FORWARD_PID=$!
trap 'kill $PORT_FORWARD_PID 2>/dev/null || true' EXIT
`kubectl port-forward service/seaweedfs-s3 -n tenant-root 8333:8333` binds the listener to 127.0.0.1 only — it has no IPv6 listener. The runner's getaddrinfo resolves `localhost` to `::1` first, and `mc` keeps a single connection target across the whole alias, so the initial `mc alias set` (which only sets credentials, no network I/O) silently succeeds while the subsequent `mc cp` upload tries `https://[::1]:8333` and gets immediate `connection refused`. The test then exits with the misleading bucket-test failure even though the S3 backend, credentials, and bucket all reconciled correctly. Use a literal IPv4 address (`127.0.0.1`) for the mc aliases and the `nc -z` readiness probe so the test no longer depends on the runner's IPv4/IPv6 resolution policy. Signed-off-by: Ivan Okhotnikov <[email protected]>
The test used to wrap kubectl port-forward in a transient subshell: bash -c 'timeout 100s kubectl port-forward ... > /dev/null 2>&1 &' The wrapper subshell exits immediately after backgrounding the kubectl process, the kubectl child is reparented to PID 1, and on the GitHub-hosted runner it gets reaped between the `nc -z` readiness probe and the first `mc cp` call. The captured failure looks like + bash -c timeout 100s kubectl port-forward ... + timeout 30 sh -ec until nc -z localhost 8333; do sleep 1; done + mc alias set rw-user https://127.0.0.1:8333 ... --insecure Added `rw-user` successfully. + mc cp --insecure /tmp/rw-test.txt rw-user/$BUCKET/rw-test.txt mc: <ERROR> Put "https://127.0.0.1:8333/$BUCKET/...": dial tcp 127.0.0.1:8333: connect: connection refused even though every S3 resource (Bucket, BucketClaim, both BucketAccess) is Ready/Granted=true at that point and the seaweedfs-s3 Service is healthy in tenant-root. Run kubectl port-forward as a job of the calling shell, capture its PID, and tear it down via a bats RETURN trap. The trap is enough on its own: the test no longer needs the outer `timeout 100s` belt because the port-forward will be killed when the test function returns, even on failure. Also switch the readiness probe to 127.0.0.1 to match the mc alias and avoid any IPv6 resolution path on hosts where kubectl binds IPv4 only. Signed-off-by: Ivan Okhotnikov <[email protected]>
The previous attempt added a bash `trap '...' RETURN` to clean up kubectl port-forward when the test function returned. cozytest.sh sources its converted bats test bodies into the runner shell, which on the GitHub-hosted Linux runner is /bin/sh -> dash. dash does not implement the bash-specific RETURN pseudo-signal, so the trap command exits non-zero, the surrounding `set -eu` aborts the test body immediately, and the captured output stops two lines after `name=test` with no other diagnostic — the test never reaches the mc S3 operations at all. Drop the trap and rely on cozytest.sh's existing process-group cleanup: when the test function returns, the leftover port-forward job in the same group is terminated. This restores POSIX shell compatibility while keeping the lifecycle fix from the previous commit (no `bash -c '... &'` wrapper, port-forward stays alive through the mc operations because it is a child of the calling shell, not of a transient subshell that exits immediately). Signed-off-by: Ivan Okhotnikov <[email protected]>
108cf72 to
d937a91
Compare
## What Consolidated landing branch for a set of interdependent platform/CI stabilization fixes. It began as the source PRs in the table below (in dependency order) and has since grown with review-driven fixes and a few production-behavior changes — the full current inventory is under **Scope** below. Landing them as one unit lets CI run against the real combined state instead of a fragile bottom-up merge train. ## Source PRs (dependency order) | # | Commit | Source PR | Addresses | |---|--------|-----------|-----------| | 1 | fix(capi): startupProbe on capi-controller-manager | #2946 | capi crashloop during cert provisioning | | 2 | test(e2e): bucket.bats port-forward + S3 client reliability | #2944 | flaky bucket test | | 3 | fix(seaweedfs): restore -lock BucketClass, s3 svc name, drop volumeSizeLimitMB | #2943 | missing bucket/access classes | | 4 | test(e2e): in-cluster Cilium endpoint-leak healer (install + apps) | #2874 | cilium "IP already in use" leak | | 5 | refactor(build): mode=max registry cache + main-only warmer | #2938 | build cache | | 6 | ci(build): isolate each PR build on its own ephemeral runner VM | #2939 | shared-buildkit contention | | 7 | fix(e2e): LINSTOR post-install waits on a single 15m deadline | #2928 | LINSTOR provisioning timeouts | | 8 | test(metallb): assert digest-pinned image form, not version literal | #2873 | brittle metallb assertion | ## Why consolidated These form a dependency DAG (verified from CI logs): e.g. 2943 needs 2946, 2938 needs 2874, 2928 needs 2938/2939, 2873 needs 2928. Tested bottom-up, each lower PR runs with none of the fixes above it and can't go green alone. This branch carries all of them, so CI runs against the real combined state. ## Verification status CI is green on the latest head (`5653c30`): full-suite E2E passes end-to-end and is reproducible (2 of 3 runs on this SHA green). The one red run was a LINSTOR tie-breaker / DRBD-metadata infra flake on the sandbox, unrelated to the diff — a different environmental subsystem failed each run (details in the comments below). The earlier 3-hour crust-gather snapshot hang is fixed in `5653c30`. ## Scope beyond the original 8 The branch has grown past the 8 source PRs above with review-driven fixes and a few production-behavior changes. For reviewer transparency, the full set: **Review fixes (@lexfrei review):** - **B1 (blocker)** — `8f41910` converts the SeaweedFS `-lock`/`-readonly` BucketClasses and the s3 service-name override into `patches/`, wired into `make update`, so a re-vendor no longer drops them. - **FU1** — `cc99230` grants the cilium leak-healer `delete` on pods (both delete remedies were RBAC-forbidden). - **FU3** — `014de3f` adds a unit test for the HelmRelease update conflict-retry path. - **FU4** — `6deddc6` makes the leak-healer refuse a disconnect for any non-terminal owner phase, not just `Running`. - **FU7** — `f3914d2` sets `persist-credentials: false` on the build-cache checkout (SHA-pinning is handled repo-wide in #2849). **Production-behavior changes (not in the table above):** - VPA `updateMode: Auto → Initial` for etcd (`6e9ff90`) and monitoring (`0997105`), plus `vmselect`/`vmstorage` `minAllowed` floors — stops install-time eviction churn. - cozystack-api: `retry.RetryOnConflict` on the Application→HelmRelease update path (`728f676`). **E2E robustness follow-ups:** per-test crust-gather snapshots (`50e5f94`), EtcdBackupSchedule wait (`763b85e`), tenant-node single-deadline wait (`9e43d45`), harbor BucketClaim 10m budget (`d7f244f`), tenant API via LoadBalancer (`9beda4e`), crust-gather pin (`95f42aa`). **Not addressed (by decision):** FU2 — the s3 Service keeps `seaweedfs.name`-based naming via patch (rendered output unchanged; cozystack renders `fullname == seaweedfs`, so it matches the `componentName`/`fullname` siblings in practice). FU6 — fork PR build push is handled by the existing mirror-to-same-repo-branch workflow. ## Relates to #2946 #2944 #2943 #2874 #2938 #2939 #2928 #2873 — if this lands, those can be closed; otherwise they remain the granular per-PR review path. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added CI build cache warming for main branch builds. * Added a best-effort in-cluster Cilium endpoint self-heal watchdog for e2e installs. * Extended SeaweedFS COSI with object-lock support plus separate readonly access. * **Improvements** * Reduced e2e install flakiness with a shared readiness deadline, safer waits, and improved cleanup/diagnostics capture. * Prevented install-time churn by switching VPA update mode to **Initial**. * Added container startup probes and updated SeaweedFS volume sizing. * **Tests** * Improved e2e robustness (etcd backup schedule waiting, digest-pinned image checks, and corrected S3 port-forwarding). <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…zystack#2944) Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
|
superseded by #2948 |
2 participants
What this PR does
Three bucket.bats reliability fixes extracted from #2919:
mcwas resolvinglocalhostto::1on some runners, causing connection refused against the IPv4 port-forward.mc alias setand subsequent mc commands; now held open for the full test scope.Cherry-picks of commits authored by IvanHunters from #2919.
Test plan
Summary by CodeRabbit
127.0.0.1.https://127.0.0.1:8333for both admin and readonly credentials./healthzto improve controlled startup behavior.