Repository navigation
fix(e2e): drain tenant VMs/PVCs between kubernetes tests to fix the node-join flake - #3159
Conversation
The inter-test cleanup hook deletes the tenant Kubernetes CR and waits only for the CR to disappear. That wait finalizes as soon as the CR's finalizers clear, which merely TRIGGERS teardown of the CAPK worker VMs and their DataVolume-backed disk PVCs -- it does not await it. The virt-launcher pods keep their guest RAM reserved until the VMIs are gone, so the next back-to-back tenant test's worker VMs begin scheduling against a sandbox the previous tenant has not yet vacated. Under the constrained CI sandbox this starves memory, a worker VM misses the node-join budget, and the run flakes on worker-node-join (the MachineHealthCheck cannot remediate when both workers are unhealthy). Block cozy_cleanup until the tenant VMs, VMIs and disk PVCs have actually drained from tenant-test before it returns, so the next test starts on a freed sandbox. The wait is bounded (5m) and best-effort: cozytest runs cozy_cleanup wrapped in `|| true` and the waiter returns loudly on timeout, so a stuck teardown can never hang the job longer than the deadline. The probe is namespace-scoped -- tenant-test is provisioned with etcd/monitoring/seaweedfs disabled so it carries no baseline PVCs, and e2e apps run sequentially each cleaning up after themselves, so the only VMs/VMIs/PVCs present belong to the tenant being torn down. The pure drain exit-condition (cozy_tenant_drained) is unit-tested in hack/run-kubernetes-drain_test.bats, including the invariant that a failed probe's "err" sentinel is never misread as a completed drain. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
github-actions
Bot
added
area/testing
📝 WalkthroughWalkthroughAdds a whitespace-only drain predicate (cozy_tenant_drained) and a bounded polling wait (cozy_wait_tenant_drained) for tenant VMs/VMIs/PVCs to the Kubernetes e2e script, wires it into cleanup teardown, expands node-join failure diagnostics, and adds a bats test suite covering the predicate's behavior. ChangesTenant Drain Wait and Diagnostics
Sequence Diagram(s)sequenceDiagram
participant cozy_cleanup
participant cozy_wait_tenant_drained
participant cozy_tenant_drained
participant KubernetesAPI
cozy_cleanup->>cozy_wait_tenant_drained: cozy_wait_tenant_drained 300
loop until drained or timeout
cozy_wait_tenant_drained->>KubernetesAPI: probe VMs, VMIs, PVCs
KubernetesAPI-->>cozy_wait_tenant_drained: resource captures
cozy_wait_tenant_drained->>cozy_tenant_drained: check whitespace-only
cozy_tenant_drained-->>cozy_wait_tenant_drained: drained / not-drained
end
cozy_wait_tenant_drained-->>cozy_cleanup: success or timeout warning
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
When a tenant Kubernetes cluster hits the 12m node-Ready deadline with fewer than two nodes joined, the failure path dumped only `describe nodes` and `get hr`. That cannot tell apart the two node-join failure sub-modes: a worker VM that never booted (virt-launcher Pending or OOMKilled) versus a VM that booted fine but whose kubelet never registered a Node. The tenant cilium-operator HelmRelease reports "InProgress" identically in both, so the current dump is not enough to diagnose the failure or design a fix. Dump scoped, guarded diagnostics before failing fast: - worker VM/VMI/virt-launcher state on the management cluster, which is what actually splits the two sub-modes; - tenant kubelet CSRs plus the talos-csr-signer sidecar log, which reveal a boot-but-no-Node stall. In-guest Talos/kubelet capture is deliberately left out: talosctl needs a tenant-cluster talosconfig the runner does not have (the tenant workers use a separate Talos CA from the management sandbox), so the gap is documented in the failure path rather than shipped as a call that would fail mTLS and capture nothing. Every capture is guarded with `|| true` so a capture failure never masks the real exit. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
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 a recurring flake in the tenant-Kubernetes e2e suite caused by resource contention in the sandbox. By implementing a wait mechanism that ensures complete teardown of tenant infrastructure between sequential tests, the change prevents memory and storage starvation for subsequent test runs. Additionally, it improves diagnostic logging for node-join failures to facilitate faster debugging of future issues. 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
|
dosubot
Bot
added
the
kind/flake
![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 a mechanism to wait for tenant KubeVirt VMs, VMIs, and PVCs to be fully drained during teardown in hack/e2e-apps/run-kubernetes.sh to prevent resource starvation and flakes in subsequent tests. It also adds detailed diagnostic outputs on node-join failures and includes Bats unit tests for the new cozy_tenant_drained helper in hack/run-kubernetes-drain_test.bats. The reviewer feedback points out that several variables in the newly added shell functions (cozy_tenant_drained and cozy_wait_tenant_drained) are not declared as local, which can cause them to leak into the global environment of the calling shell when the script is sourced.
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.
| cozy_tenant_drained() { | ||
| for _capture in "$@"; do | ||
| case "$_capture" in | ||
| *[![:space:]]*) return 1 ;; | ||
| esac | ||
| done | ||
| return 0 | ||
| } |
There was a problem hiding this comment.
The loop variable _capture is not declared as local. Since this script is sourced by other test scripts, failing to declare variables as local causes them to leak into the global shell environment, which can lead to unexpected side effects or name collisions.
| cozy_tenant_drained() { | |
| for _capture in "$@"; do | |
| case "$_capture" in | |
| *[![:space:]]*) return 1 ;; | |
| esac | |
| done | |
| return 0 | |
| } | |
| cozy_tenant_drained() { | |
| local _capture | |
| for _capture in "$@"; do | |
| case "$_capture" in | |
| *[![:space:]]*) return 1 ;; | |
| esac | |
| done | |
| return 0 | |
| } |
| cozy_wait_tenant_drained() { | ||
| _ns=tenant-test | ||
| _timeout="${1:-300}" | ||
| _deadline=$(( $(date +%s) + _timeout )) | ||
| while :; do | ||
| _vm=$(kubectl -n "$_ns" get virtualmachines.kubevirt.io -o name 2>/dev/null) || _vm=err | ||
| _vmi=$(kubectl -n "$_ns" get virtualmachineinstances.kubevirt.io -o name 2>/dev/null) || _vmi=err | ||
| _pvc=$(kubectl -n "$_ns" get pvc -o name 2>/dev/null) || _pvc=err | ||
| if cozy_tenant_drained "$_vm" "$_vmi" "$_pvc"; then | ||
| echo "» tenant VMs/VMIs/PVCs drained from $_ns" | ||
| return 0 | ||
| fi | ||
| if [ "$(date +%s)" -ge "$_deadline" ]; then | ||
| echo "» WARNING: tenant teardown did not drain within ${_timeout}s; continuing (next test may face memory/storage pressure)" >&2 | ||
| kubectl -n "$_ns" get virtualmachines.kubevirt.io,virtualmachineinstances.kubevirt.io,pvc 2>&1 | sed 's/^/ drain-leftover: /' >&2 || true | ||
| return 1 | ||
| fi | ||
| sleep 5 | ||
| done | ||
| } |
There was a problem hiding this comment.
The variables _ns, _timeout, _deadline, _vm, _vmi, and _pvc are not declared as local. Since this script is sourced, these variables will leak into the global environment of the calling shell, potentially causing conflicts with other scripts or tests.
cozy_wait_tenant_drained() {
local _ns=tenant-test
local _timeout="${1:-300}"
local _deadline=$(( $(date +%s) + _timeout ))
local _vm _vmi _pvc
while :; do
_vm=$(kubectl -n "$_ns" get virtualmachines.kubevirt.io -o name 2>/dev/null) || _vm=err
_vmi=$(kubectl -n "$_ns" get virtualmachineinstances.kubevirt.io -o name 2>/dev/null) || _vmi=err
_pvc=$(kubectl -n "$_ns" get pvc -o name 2>/dev/null) || _pvc=err
if cozy_tenant_drained "$_vm" "$_vmi" "$_pvc"; then
echo "» tenant VMs/VMIs/PVCs drained from $_ns"
return 0
fi
if [ "$(date +%s)" -ge "$_deadline" ]; then
echo "» WARNING: tenant teardown did not drain within ${_timeout}s; continuing (next test may face memory/storage pressure)" >&2
kubectl -n "$_ns" get virtualmachines.kubevirt.io,virtualmachineinstances.kubevirt.io,pvc 2>&1 | sed 's/^/ drain-leftover: /' >&2 || true
return 1
fi
sleep 5
done
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
hack/e2e-apps/run-kubernetes.sh (2)
39-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winScope the polling loop's scratch variables with
local.
_ns,_timeout,_deadline,_vm,_vmi,_pvcare all unscoped, inconsistent with thelocal-based convention used throughoutrun_kubernetes_testin this same file. Givencozy_cleanup/cozy_wait_tenant_drainedrun in the same shell as the rest of the cozytest hooks, these generic names (especially_ns,_deadline) are collision-prone with future additions.♻️ Proposed fix
cozy_wait_tenant_drained() { + local _ns _timeout _deadline _vm _vmi _pvc _ns=tenant-test _timeout="${1:-300}"🤖 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 39 - 58, The polling helper cozy_wait_tenant_drained currently uses unscoped scratch variables (_ns, _timeout, _deadline, _vm, _vmi, _pvc), which can collide with other shell state in the same process. Update cozy_wait_tenant_drained to declare these temporary values with local, matching the convention used in run_kubernetes_test, and keep the rest of the loop logic unchanged so the function’s behavior stays the same while avoiding variable leakage.
12-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeclare
_captureaslocal.
cozy_tenant_draineddoesn't scope_capturewithlocal, unlike the rest of this file's functions (e.g.run_kubernetes_testuseslocal version_expr,local k8s_version, etc.). Since this is bash (per thelocalusage elsewhere), an unscoped loop variable leaks into the caller's shell/global namespace and could collide with a same-named variable in another cozytest-sourced function running in the same shell.♻️ Proposed fix
cozy_tenant_drained() { + local _capture for _capture in "$@"; do case "$_capture" in *[![:space:]]*) return 1 ;; esac done return 0 }🤖 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 12 - 19, The cozy_tenant_drained function leaks its loop variable into the caller’s shell because _capture is not scoped locally. Update cozy_tenant_drained in run-kubernetes.sh to declare _capture with local before the loop, matching the style used by run_kubernetes_test and the other bash helpers, so the variable stays confined to the function.
🤖 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 39-58: The polling helper cozy_wait_tenant_drained currently uses
unscoped scratch variables (_ns, _timeout, _deadline, _vm, _vmi, _pvc), which
can collide with other shell state in the same process. Update
cozy_wait_tenant_drained to declare these temporary values with local, matching
the convention used in run_kubernetes_test, and keep the rest of the loop logic
unchanged so the function’s behavior stays the same while avoiding variable
leakage.
- Around line 12-19: The cozy_tenant_drained function leaks its loop variable
into the caller’s shell because _capture is not scoped locally. Update
cozy_tenant_drained in run-kubernetes.sh to declare _capture with local before
the loop, matching the style used by run_kubernetes_test and the other bash
helpers, so the variable stays confined to the function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f333729e-dd4e-4c50-805a-0e4cbbf4bc0f
📒 Files selected for processing (2)
hack/e2e-apps/run-kubernetes.shhack/run-kubernetes-drain_test.bats
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM
The drain barrier is correct, bounded, and best-effort; the critical err-sentinel invariant holds and is unit-tested, so the barrier cannot silently degrade into a no-op — no blocking issues found.
Findings
None.
The change was verified end to end:
- All 7 unit tests pass under the real runner (
hack/cozytest.sh hack/run-kubernetes-drain_test.bats), 0 failures. - The new code (
cozy_tenant_drained,cozy_wait_tenant_drained) introduces zero shellcheck findings — every shellcheck hit onrun-kubernetes.shis on pre-existing lines (SC3043local,SC2034port,SC2086on${test_name}), none in the added ranges. - err-sentinel path traced:
_vm=$(kubectl ... 2>/dev/null) || _vm=errsets the literalerronly on a non-zero kubectl exit;cozy_tenant_drainedmatcheserrunder*[![:space:]]*and returns not-drained. Empty stdout from a successful probe is whitespace and reads as drained; empty stdout from a failed probe becomeserrand does not. The "barrier is a no-op → PR fails to fix the flake" failure mode does not occur. - Bounded wait confirmed:
_deadlinecomputed once fromdate +%s, timeout branch returns 1 loudly with adrain-leftover:dump to stderr, andcozy_cleanupcalls it ascozy_wait_tenant_drained 300 || true.cozytest.shadditionally runs the wholecozy_cleanupunder|| true, so it can never hang or fail the job past 5m. cozy_cleanupis a file-scope function;cozytest.sh's parent-shell EXIT trap doescommand -v cozy_cleanup && cozy_cleanup || true— confirmed in the runner source.- The unit test sources
run-kubernetes.shinside each@testbody, which runs in a subshell inrun_one, socozy_cleanupdoes NOT leak into the unit-test runner's parent EXIT trap —make unit-testswill not trigger any cluster teardown.
Claim mismatches
None. All PR-body claims verified [OK]:
- "wait finalizes as soon as the CR disappears but does not await VM/PVC teardown" [OK] —
kubectl wait --for=deleteon theKubernetesCR returns on finalizer clearance; CAPK/KubeVirt child VMs/VMIs and DataVolume-backed PVCs are torn down asynchronously by their own controllers, outliving CR removal. - "barrier blocks until VMs/VMIs/disk PVCs drain from tenant-test" [OK] — namespace-scoped probes on
virtualmachines.kubevirt.io,virtualmachineinstances.kubevirt.io,pvc. - "bounded 5m and best-effort" [OK].
- "
cozy_tenant_drainedfactored out and unit-tested including the err-sentinel invariant" [OK] — 7 passing tests including the err case. - "tenant-test provisioned with etcd/monitoring/seaweedfs disabled, no baseline PVCs" [OK] — confirmed at
hack/e2e-install-cozystack.bats:307-321(etcd: false,monitoring: false,seaweedfs: false). - release-note block present (
NONE).
Operational risks
None that block. The best-effort-on-timeout is a deliberate design: on a genuine teardown regression the barrier will not fail CI, it warns and dumps leftovers. This is acceptable for an inter-test cleanup hook (its job is to free the sandbox, not to gate on teardown health), and the loud stderr warning + drain-leftover: dump keeps a real regression visible in CI logs. Worth a one-line acknowledgement that this hook will not turn a teardown regression red — but not a defect.
Nitpick (non-blocking)
hack/e2e-apps/run-kubernetes.sh— the scratch variables incozy_tenant_drained(_capture) andcozy_wait_tenant_drained(_ns,_timeout,_deadline,_vm,_vmi,_pvc) are not declaredlocal. Since these functions run in the same shell as the other cozytest hooks, the generic names are collision-prone. Declaring themlocalmatches the convention used byrun_kubernetes_testin the same file.
Caveats
- No
packages/touched — files are exactlyhack/e2e-apps/run-kubernetes.shandhack/run-kubernetes-drain_test.bats. Blast radius is CI-only; zero customer-cluster upgrade or fresh-install impact. - The new bats file is auto-discovered by
make unit-testsviaBATS_UNIT_FILES := $(filter-out hack/e2e-%.bats,$(wildcard hack/*.bats))— it matcheshack/*.batsand is not ane2e-file, so it is picked up. It defines nosetup_file/setup/teardownhooks (which cozytest.sh ignores anyway), so no dead-code hooks were introduced.
Recommended follow-ups
hack/e2e-apps/vminstance.batsalso provisions KubeVirt VMs intenant-test, but it deletes itsvminstances.apps.cozystack.io/vmdisks.apps.cozystack.ioCRs inline with blockingkubectl delete --timeout=3mand is a single lightweight VM — it does not exhibit the back-to-back Kamaji-tenant RAM-starvation pattern this PR targets. No shared helper is warranted now; no action needed unless that test later grows heavier VM footprints.
2 participants
What this PR does
Fixes the dominant flake in the tenant-Kubernetes e2e suite, where a run fails on worker-node-join under sandbox memory/storage pressure.
The two heavy Kamaji tenant tests —
kubernetes-latestthenkubernetes-previous— run back-to-back in one CI sandbox, each provisioning a tenant cluster with KubeVirt worker VMs. The inter-test cleanup hook (cozy_cleanupinhack/e2e-apps/run-kubernetes.sh) deletes the tenantKubernetesCR and waits only for the CR to disappear. That wait finalizes as soon as the CR's finalizers clear, which merely triggers teardown of the CAPK worker VMs and their DataVolume-backed disk PVCs — it does not await it. The virt-launcher pods keep their guest RAM reserved until the VMIs are gone, so the next tenant test's worker VMs begin scheduling against a sandbox the previous tenant has not yet vacated. Under the constrained sandbox this starves memory, a worker VM misses the node-join budget, and the MachineHealthCheck cannot remediate when both workers are unhealthy — so the run flakes on worker-node-join.This adds a bounded drain barrier to
cozy_cleanup: after deleting the CR it now blocks until the tenant's VirtualMachines, VirtualMachineInstances and disk PVCs have actually drained from thetenant-testnamespace, so the next test starts on a freed sandbox.The wait is bounded (5m) and best-effort: cozytest already runs
cozy_cleanupwrapped in|| true, and the waiter returns loudly (warning plus a leftover dump) on timeout, so a stuck teardown can never hang the job past the deadline — it leaves the sandbox no worse than before. The probe is namespace-scoped:tenant-testis provisioned with etcd/monitoring/seaweedfs disabled so it carries no baseline PVCs, and the e2e apps run sequentially each cleaning up after themselves, so the only VMs/VMIs/PVCs present belong to the tenant being torn down.The pure drain exit-condition (
cozy_tenant_drained) is factored out and unit-tested inhack/run-kubernetes-drain_test.bats(runs undermake unit-tests), including the invariant that a failed probe'serrsentinel is never misread as a completed drain.Screenshots
Not applicable — e2e harness change, no UI.
Release note
Summary by CodeRabbit