Repository navigation
test(e2e): wait for LINSTOR pool free capacity between tenant Kubernetes tests - #3210
Conversation
…tes tests kubernetes-previous.bats intermittently fails with `zfs create -V ... returned with exitcode 1: cannot create '...': out of space` on a satellite even though the LINSTOR pool measured 30+ minutes later shows 100+ GiB free per satellite. Root cause proven from a satellite ErrorReport captured in the cozyreport artefact (PR #3044 run 28751310913, report id 6A4AADFD-349B2-000000): the previous tenant Kubernetes test's cozy_wait_tenant_drained gate returned as soon as its PVCs disappeared from the API, but the satellite-side ZFS `zvol destroy` for the just-deleted replicated (autoPlace=3, DRBD) 20 GiB worker disks lagged tens of seconds behind that signal as DRBD adjusted, unref counts drained, and ZFS batch-destroyed the datasets. The next tenant test's CreateVolume then landed inside that window against a pool that had not yet reclaimed the space, LINSTOR-CSI retried autoplace against its own still-being-torn-down previous placement, and worker Machine bringup stretched past the MHC nodeStartupTimeout budget with no cluster-side fault. Add a companion `cozy_wait_linstor_pool_free` helper that polls the LINSTOR controller for the smallest FreeCapacity across every ZFS satellite pool and blocks until it reaches a per-satellite threshold (default 90 GiB — covers the ~82 GiB worst-case footprint of two 20 GiB replicated worker targets across all three satellites plus two 21 GiB CDI scratch PVCs landing on the same node via the local storageClass). Called from cozy_cleanup immediately after cozy_wait_tenant_drained. Bounded and best-effort: 300 s timeout, `|| true` at the call site, warning printed with the observed pool state if the threshold is not reached in budget so the next test does not misdiagnose the pressure. The threshold is a floor for the concrete failure mode observed in the E2E suite; the surrounding kubernetes-latest -> kubernetes-previous handoff is the only run in the current suite that immediately follows a heavy tenant teardown with another tenant provisioning, so this gate is the natural place for the check. E2E-scope only, no product or chart change. The proximate observation (`zfs create out of space`) surfaced because of PR #3209 which taught hack/cozyreport.sh to bundle satellite ErrorReports; without that artefact the root cause was ambiguous between multiple plausible LINSTOR internal races. Ships alongside that PR. Signed-off-by: Ivan Okhotnikov <[email protected]>
github-actions
Bot
added
area/testing
📝 WalkthroughWalkthroughAdds a new ChangesLINSTOR teardown wait
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
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 addresses an intermittent race condition in the end-to-end test suite where Kubernetes tenant tests fail due to insufficient ZFS storage capacity. While the API reports PVCs as deleted, the underlying satellite storage takes time to reclaim space. By adding a polling mechanism that verifies LINSTOR pool capacity, the suite now ensures storage is actually available before proceeding to subsequent tests. 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
|
![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 introduces the cozy_wait_linstor_pool_free helper function in hack/e2e-apps/run-kubernetes.sh to wait for ZFS storage pools on LINSTOR satellites to reclaim space before proceeding to subsequent tenant tests, resolving potential out-of-space errors. The reviewer provided valuable feedback to improve the robustness of the jq parsing against null values and to use a more standard and portable sort command for parsing the output.
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.
| _min_line=$(kubectl -n cozy-linstor exec deploy/linstor-controller -- sh -c ' | ||
| linstor --machine-readable sp l 2>/dev/null | | ||
| jq -r "first | .[] | select((.provider_kind | test(\"^ZFS\")) and .free_capacity != null) | \"\(.free_capacity):\(.node_name)\"" | | ||
| sort -n | head -n 1 | ||
| ' 2>/dev/null) || _min_line="" |
There was a problem hiding this comment.
Robustness and Portability Improvements
-
Safer
jqParsing:
Usingfirst | .[]can throw ajqerror (e.g.,Cannot iterate over null) if the input array is empty or null. Replacing it with.[0][]?safely handles empty/null arrays without throwing errors.
Additionally, if.provider_kindis ever missing or null, passing it directly totest()will cause ajqcrash. Adding a fallback like.provider_kind // ""prevents this. -
Standardized Numeric Sorting:
Usingsort -non strings containing non-numeric characters (like:) relies on implementation-defined behavior of how differentsortversions handle delimiters. Specifying the field separator and key explicitly withsort -t: -k1,1nis standard and highly portable.
| _min_line=$(kubectl -n cozy-linstor exec deploy/linstor-controller -- sh -c ' | |
| linstor --machine-readable sp l 2>/dev/null | | |
| jq -r "first | .[] | select((.provider_kind | test(\"^ZFS\")) and .free_capacity != null) | \"\(.free_capacity):\(.node_name)\"" | | |
| sort -n | head -n 1 | |
| ' 2>/dev/null) || _min_line="" | |
| _min_line=$(kubectl -n cozy-linstor exec deploy/linstor-controller -- sh -c ' | |
| linstor --machine-readable sp l 2>/dev/null | | |
| jq -r ".[0][]? | select((.provider_kind // \"\" | test(\"^ZFS\")) and .free_capacity != null) | \"\(.free_capacity):\(.node_name)\"" | | |
| sort -t: -k1,1n | head -n 1 | |
| ' 2>/dev/null) || _min_line="" |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hack/e2e-apps/run-kubernetes.sh (1)
140-143: 🚀 Performance & Scalability | 🔵 TrivialWorst-case teardown barrier now ~600s. Since this wait is additive to the existing 300s drain wait, cleanup can block up to 300+300s before yielding to the next tenant test. It's bounded and best-effort (
|| true), but confirm this stays within any job-level wall-clock backstop so back-to-back tenant tests don't risk hitting the CI step timeout.🤖 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 `@hack/e2e-apps/run-kubernetes.sh` around lines 140 - 143, The teardown path in cozy_wait_linstor_pool_free currently adds another long best-effort wait on top of the existing drain delay, so make sure the combined tenant cleanup time stays safely under the CI job’s wall-clock limit. Review the timeout values around the wait in run-kubernetes.sh and either cap this extra linstor pool-free wait, shorten it, or gate it so back-to-back tenant tests cannot approach the step timeout.
🤖 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.
Nitpick comments:
In `@hack/e2e-apps/run-kubernetes.sh`:
- Around line 140-143: The teardown path in cozy_wait_linstor_pool_free
currently adds another long best-effort wait on top of the existing drain delay,
so make sure the combined tenant cleanup time stays safely under the CI job’s
wall-clock limit. Review the timeout values around the wait in run-kubernetes.sh
and either cap this extra linstor pool-free wait, shorten it, or gate it so
back-to-back tenant tests cannot approach the step timeout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e117ced3-2d8e-4b60-9dc6-6e44f8eae0bf
📒 Files selected for processing (1)
hack/e2e-apps/run-kubernetes.sh
myasnikovdaniil
left a comment
myasnikovdaniil
left a comment
There was a problem hiding this comment.
LGTM
Best-effort, e2e-only teardown gate — it can't wedge the suite worse than the status quo (|| true at the call site, bounded 300s, loud return on timeout). One design question worth confirming before merge, plus a few non-blocking notes. The automated reviewers' overlapping points (jq/sort hardening, combined wall-clock) are valid and folded in below rather than repeated.
Non-blocking follow-ups
-
Threshold viability — the one worth confirming.
cozy_wait_linstor_pool_freeblocks until every ZFS pool reportsFreeCapacity >= 90 GiB. That only helps if each satellite pool can actually reach 90 GiB free after a drain. If any pool's total (or reliably-reclaimable) capacity is below 90 GiB, the wait can never succeed and silently degrades to a fixed 300s delay every cleanup. The verification snapshot in the description shows two of three satellites under 90 GiB free at that instant (a live mid-use cluster, granted — not the idle post-drain state). Could you confirm the CI pools clear 90 GiB free after a tenant drain, or derive the threshold from poolTotalCapacityso it self-scales to the runner? -
Combined teardown budget.
cozy_cleanupnow runs the 300s drain wait and this 300s pool-free wait back-to-back (additive, both best-effort), so worst-case teardown roughly doubles. Worth confirming it stays inside the job-level wall-clock backstop so back-to-back tenant tests can't approach the CI step timeout — this matches the point already raised inline by the automated reviewer. -
jq/sort robustness. The inline suggestion from the automated reviewer (
.[0][]?,.provider_kind // "",sort -t: -k1,1n) is valid: as written, an empty/null outer array or a null/absentprovider_kindmakesjqerror, and because stderr is discarded that surfaces as an empty result -> poll-to-timeout rather than a clear "can't read pools" failure. Cheap to fold in and it removes the silent-empty mode. -
Tests. No unit coverage, which matches the existing precedent: the sibling I/O wait
cozy_wait_tenant_drainedis itself untested — only its extracted pure predicatecozy_tenant_drainedhas a bats case (run-kubernetes-drain_test.bats). The new function's only pure logic is a trivial numeric-ge; its substantive logic is the in-podjq, which needs a live LINSTOR. Not a gate. If the parse/compare is ever extracted into a pure helper, a matching bats case would mirror that precedent.
| # each retry racing the still-being-torn-down previous placement and | ||
| # stretching worker-Machine bringup past the MHC nodeStartupTimeout. | ||
| # | ||
| # Two 20 GiB replicated worker targets (60 GiB total per satellite, since |
There was a problem hiding this comment.
The per-satellite arithmetic reads inconsistently: for the 2-worker md0 fixture (minReplicas: 2), autoPlace=3 places one replica of each 20 GiB volume per satellite, so the worker footprint is ~40 GiB/satellite, not 60 GiB (60 GiB would be one volume's cluster-wide total). The 90 GiB threshold is conservative either way, so behavior is unaffected — just the number. The merged sibling commit states it as "2 x 20Gi target + 2 x 21Gi scratch ~= 82Gi", which lines up with the 40+42 reading.
2 participants
What this PR does
Adds a companion
cozy_wait_linstor_pool_freehelper tohack/e2e-apps/run-kubernetes.shand calls it fromcozy_cleanupright aftercozy_wait_tenant_drained. The helper polls LINSTOR for the smallest ZFS poolFreeCapacityacross every satellite and blocks until it reaches a per-satellite threshold (default 90 GiB), bounded to 300 s with a loud warning on timeout.Root cause (proven)
kubernetes-previous.batsintermittently failed withzfs create -V 20976048KB data/pvc-... returned with exitcode 1: cannot create '...': out of spaceon a satellite that measured 132 GiB free 35 minutes later — so this is not an absolute capacity issue but a timing race.Root cause is captured in the cozyreport artefact of run 28751310913 (PR #3044), satellite ErrorReport
6A4AADFD-349B2-000000. The previous tenant Kubernetes test'scozy_wait_tenant_drainedgate returned as soon as its PVCs disappeared from the API, but the satellite-side ZFSzvol destroyfor the just-deleted replicated (autoPlace=3, DRBD) 20 GiB worker disks lagged tens of seconds behind that signal as DRBD adjusted, unref counts drained, and ZFS batch-destroyed the datasets. The next tenant test'sCreateVolumelanded inside that window, LINSTOR-CSI retried autoplace against the still-being-torn-down previous placement (Do not place with resource: pvc-<same-uid>implicit anti-collocation, no CSI bug), and worker Machine bringup stretched past the MHCnodeStartupTimeoutbudget with no cluster-side fault.The proximate observation (
zfs create out of space) surfaced only because of PR #3209 which teacheshack/cozyreport.shto bundle satellite ErrorReports into the artefact. Without that, the root cause was ambiguous between several plausible LINSTOR internal races and I nearly opened a wrong upstream issue.Threshold rationale
Two 20 GiB replicated worker targets = 60 GiB per satellite (autoPlace=3 places one replica per node) plus two 21 GiB CDI scratch PVCs (worst case both landing on the same node via the
localstorageClass) = ~82 GiB per-satellite peak footprint. 90 GiB default threshold covers that with margin.Scope
hack/e2e-apps/run-kubernetes.shonly. E2E-scope, no product or chart change. Threshold and timeout are function args with sensible defaults; caller can override.Verified
jqexpression against a live LINSTOR 1.33.2 cluster:Matches
linstor sp loutput exactly (2.95 GiB / 40.51 GiB / 188.82 GiB). ZFS-only filter excludes DISKLESSLong.MAX_VALUEsentinel;and .free_capacity != nullguard prevents OFFLINE satellite pool objects (whosefree_capacityis optional per REST schema) from ranking asnullin the sort and hanging the loop.Contract mirrors
cozy_wait_tenant_drained: bounded deadline,|| trueat the call site, loud timeout warning that includes the smallest observed pool and its satellite. Timeout can never wedge the suite worse than the status quo.Release note
Summary by CodeRabbit