fix(e2e): budget tenant scheduling separately from workload readiness - #3579
Conversation
The tenant backend Deployment had a single 300s budget to reach condition=Available, and two unrelated variable costs shared it. The first is scheduling. What the suite establishes before that point is two tenant nodes Ready, which is weaker than schedulable: a Ready node still carries node.cilium.io/agent-not-ready until the tenant cilium agent claims it, and a node the bringup has not finished with is SchedulingDisabled. Scheduling took 2m18s and 1m57s in the two tenant suites of one run, leaving the image pull to finish inside what was left. It did not, and both suites failed with the same message for two different shortfalls. Wait for a node that actually accepts a toleration-free Pod before creating the workload, on its own budget and its own failure message. The gate encodes the scheduler's rule for such a Pod (Ready, not unschedulable, no NoSchedule or NoExecute taint) over a custom-columns probe, prints the node table on both outcomes so a timeout names the taint that held it, and treats a failed probe as not-schedulable so an API blip cannot release it. The readiness wait keeps its 300s, now starting from a schedulable node, and dumps deployment, pod and event state when it runs out. Pin the workload image by digest. The tenant workers reach no registry mirror, so nginx is pulled from Docker Hub on every run either way; the digest fixes what that pull returns instead of leaving a floating tag free to change size and content under a fixed deadline. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
📝 WalkthroughWalkthroughThe Kubernetes test flow now waits for a Ready, uncordoned, untainted tenant node before backend readiness timing. It adds bounded polling, failure diagnostics, schedulability tests, and a digest-pinned nginx image. ChangesTenant scheduling and backend readiness
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@hack/e2e-chainsaw/_lib/run-kubernetes.sh`:
- Line 180: The node probe in hack/e2e-chainsaw/_lib/run-kubernetes.sh:180 must
pass a finite --request-timeout value, such as 10s, to kubectl get nodes. Update
the stubbed probe assertion in hack/run-kubernetes-schedulable_test.bats:195-226
to require this flag while preserving the existing probe behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 26609d47-c3d9-4b4e-be8c-625d975f5f1c
📒 Files selected for processing (2)
hack/e2e-chainsaw/_lib/run-kubernetes.shhack/run-kubernetes-schedulable_test.bats
| _timeout="${2:-300}" | ||
| _deadline=$(( $(date +%s) + _timeout )) | ||
| while :; do | ||
| _nodes=$(kubectl --kubeconfig "$_kc" get nodes --no-headers -o custom-columns='NAME:.metadata.name,READY:.status.conditions[?(@.type=="Ready")].status,UNSCHEDULABLE:.spec.unschedulable,TAINTS:.spec.taints[*].effect' 2>/dev/null) || _nodes="" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
lib='hack/e2e-chainsaw/_lib/run-kubernetes.sh'
test='hack/run-kubernetes-schedulable_test.bats'
grep -n -- '--request-timeout=10s' "$lib"
sed -n '195,226p' "$test" | grep -n -- '--request-timeout=10s'Repository: cozystack/cozystack
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
lib='hack/e2e-chainsaw/_lib/run-kubernetes.sh'
test='hack/run-kubernetes-schedulable_test.bats'
printf 'File exists: '
test -f "$lib" && echo "$lib" || echo "missing $lib"
test -f "$test" && echo "$test" || echo "missing $test"
printf '\nRelevant lib section:\n'
sed -n '150,195p' "$lib"
printf '\nRelevant test section:\n'
sed -n '195,230p' "$test"
printf '\nSearch for request-timeout or _deadline checks:\n'
rg -n -- '--request-timeout|_deadline|functimeout|timeout' "$lib" "$test"Repository: cozystack/cozystack
Length of output: 12100
🌐 Web query:
kubectl documentation request-timeout default 0
💡 Result:
In kubectl, the default value for the --request-timeout flag is "0" [1][2][3]. A value of "0" indicates that there is no timeout, meaning the client will wait indefinitely for a server request to complete [1][4][5]. When providing a non-zero value, you must include a corresponding time unit, such as "1s", "2m", or "3h" [1][6][5]. This flag controls the length of time the kubectl client waits for a single request to the API server [1][2]. It is distinct from command-specific timeout flags (like those used for wait or delete operations), which typically define the duration of an operation rather than the individual HTTP request [7].
Citations:
- 1: https://kubernetes.io/docs/reference/kubectl/generated/kubectl_options/
- 2: https://kubernetes.io/docs/reference/kubectl/kubectl/
- 3: https://kubernetes.io/docs/reference/kubectl/generated/kubectl/
- 4: Add global timeout flag kubernetes/kubernetes#33958
- 5: https://man.archlinux.org/man/kubectl.1.en
- 6: https://kubernetes.io/docs/reference/kubectl/generated/kubectl_config/kubectl_config_set/
- 7: https://stackoverflow.com/questions/72945357/is-there-an-actual-difference-between-the-request-timeout-flag-vs-the-timeout
Set a finite timeout for each node probe.
The loop limits total probe time, but kubectl get nodes does not cancel until it returns. The kubectl --request-timeout default is 0, so a stalled API request can exceed the 300-second gate and return no node table for diagnostics.
- Add a finite
--request-timeout, such as10s, to the node probe:hack/e2e-chainsaw/_lib/run-kubernetes.sh#L180. - Assert that the stubbed probe includes this flag:
hack/run-kubernetes-schedulable_test.bats#L195.
📍 Affects 2 files
hack/e2e-chainsaw/_lib/run-kubernetes.sh#L180-L180(this comment)hack/run-kubernetes-schedulable_test.bats#L195-L226
🤖 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-chainsaw/_lib/run-kubernetes.sh` at line 180, The node probe in
hack/e2e-chainsaw/_lib/run-kubernetes.sh:180 must pass a finite
--request-timeout value, such as 10s, to kubectl get nodes. Update the stubbed
probe assertion in hack/run-kubernetes-schedulable_test.bats:195-226 to require
this flag while preserving the existing probe behavior.
VerdictLGTM with non-blocking notes The scheduling gate is fail-closed, the parsing predicate matches the scheduler's admission rule, and the 21-case unit suite is non-vacuous and wired into Findings[MINOR] _nodes=$(kubectl --kubeconfig "$_kc" get nodes ... 2>/dev/null) || _nodes=""The gate is correctly fail-closed: a failed probe yields an empty capture, [MINOR] The Caveats
|
What this PR does
The tenant backend Deployment in the kubernetes suites had one 300s budget to reach
condition=Available, and two unrelated variable costs shared it.The first is scheduling. What the suite establishes before that point is two tenant nodes Ready, which is weaker than schedulable: a Ready node still carries
node.cilium.io/agent-not-readyuntil the tenant cilium agent claims it, and a node the bringup has not finished with isSchedulingDisabled. In the run recorded in #3577 scheduling took 2m18s and 1m57s in the two suites, which left the image pull to finish inside what remained. It did not, and both suites reported the sametimed out waiting for the conditionfor what were two different shortfalls.So this waits for a node that actually accepts a Pod before creating the workload, on its own budget and its own failure message. The gate encodes the scheduler's rule for a Pod that tolerates nothing (Ready, not unschedulable, no
NoScheduleorNoExecutetaint) over acustom-columnsprobe, prints the node table on both outcomes so a timeout names the taint that held it, and treats a failed probe as not-schedulable so an API blip cannot release it. On the happy path it adds no wall time: it spends the seconds the Pod would otherwise spend Pending, plus at most one 5s poll interval. On a failing run the two budgets stack, so a run that exhausts both now gives up at ~600s where it used to give up at 300s, inside the 40m Chainsaw script op the suites document as a ~25m bringup. The readiness wait keeps its 300s, now starting from a schedulable node, and dumps deployment, pod and event state when it runs out.The workload image is also pinned by digest. The tenant workers reach no Docker Hub mirror (
hack/e2e-talos-image-cache.yamlserves the Talos worker OS disk image over HTTP and is not a registry mirror), so nginx is pulled from Docker Hub on every run either way. The digest does not take the pull off the critical path; it fixes what that pull returns instead of leaving a floating tag free to change size and content under a fixed deadline. Preloading the image would need infrastructure this tree does not have, and is not attempted here. Nothing will bump the pin: no renovate manager readshack/, and the comment says so, because for a throwaway test workload the freeze is the point.What this does not claim: it removes two measured consumers from a fixed budget, both taken from the events of a single run. Whether that budget was the only thing making those suites red is not established from one run, so this is not offered as the fix for a red pipeline.
hack/run-kubernetes-schedulable_test.batscovers the new logic: every branch of the predicate, the multi-node scan, the poll-again path, the deadline, and a probe that fails. Each test was verified by mutating the helper and checking that the intended test, and no other, went red.Note for whoever reviews alongside #3575: that branch mirrors
ghcr.iofor tenant worker pulls, notdocker.io, so it does not change the Docker Hub pull described here.git merge-treereports no conflict between the two, nor with #3548.Observed in #3577.
Screenshots
Not a UI change.
Downstream repositories
The trigger map was walked against the diff. The change is confined to
hack/e2e-chainsaw/_lib/run-kubernetes.shand one newhack/*.batsunit test. It moves and renames nothing underhack/, changes no make target, and does not touchhack/e2e-prepare-cluster.bats, any package, any CRD or any namespace name, so none of the listed repositories are reached.Release note
Summary by CodeRabbit
Bug Fixes
Tests