fix(e2e): drive LINSTOR post-install waits off a single 15m deadline - #2928
fix(e2e): drive LINSTOR post-install waits off a single 15m deadline#2928myasnikovdaniil wants to merge 1 commit into
Conversation
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 the LINSTOR post-install preparation script by addressing race conditions between the script's execution and the platform's reconcile chain. By moving to a shared deadline and a more robust polling mechanism, the script now correctly handles delayed resource creation without failing, significantly reducing flaky E2E test failures. 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
|
There was a problem hiding this comment.
Code Review
This pull request refactors the hack/e2e-post-install-prep.sh script to use a shared 15-minute deadline for waiting on LINSTOR prerequisites instead of individual, racing timeouts. It introduces a wait_for helper function to handle resource non-existence gracefully without triggering set -e aborts. Feedback on these changes includes declaring variables as local within the wait_for function to prevent global scope leakage, and wrapping the timeout 300 command for LINSTOR nodes in a conditional block to print a clear error message upon timeout instead of silently aborting.
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.
|
|
||
| echo "[post-install-prep] waiting for 3 LINSTOR nodes Online" | ||
| timeout 60 sh -ec 'until [ $(kubectl exec -n cozy-linstor deploy/linstor-controller -- linstor node list | grep -c Online) -eq 3 ]; do sleep 1; done' | ||
| timeout 300 sh -ec 'until [ $(kubectl exec -n cozy-linstor deploy/linstor-controller -- linstor node list | grep -c Online) -eq 3 ]; do sleep 2; done' |
There was a problem hiding this comment.
If the timeout 300 command times out, it will exit with status 124. Because set -e is active, the script will abort immediately without printing any error message, which can make debugging CI failures difficult. Wrapping it in an if statement allows us to print a clear error message to stderr before exiting.
| timeout 300 sh -ec 'until [ $(kubectl exec -n cozy-linstor deploy/linstor-controller -- linstor node list | grep -c Online) -eq 3 ]; do sleep 2; done' | |
| if ! timeout 300 sh -ec 'until [ $(kubectl exec -n cozy-linstor deploy/linstor-controller -- linstor node list | grep -c Online) -eq 3 ]; do sleep 2; done'; then | |
| echo "[post-install-prep] timed out waiting for 3 LINSTOR nodes Online" >&2 | |
| exit 1 | |
| fi |
| wait_for() { | ||
| desc=$1 | ||
| shift | ||
| echo "[post-install-prep] waiting for ${desc}" | ||
| while :; do | ||
| remaining=$(( DEADLINE - $(date +%s) )) |
There was a problem hiding this comment.
Declaring variables as local inside functions is a best practice in shell scripting to prevent them from leaking into the global scope and causing unexpected side effects, especially in larger scripts.
| wait_for() { | |
| desc=$1 | |
| shift | |
| echo "[post-install-prep] waiting for ${desc}" | |
| while :; do | |
| remaining=$(( DEADLINE - $(date +%s) )) | |
| wait_for() { | |
| local desc=$1 | |
| shift | |
| echo "[post-install-prep] waiting for ${desc}" | |
| while :; do | |
| local remaining=$(( DEADLINE - $(date +%s) )) |
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughThis PR spans build infrastructure, CI workflows, E2E test hardening, and storage configuration improvements. It introduces a centralized Docker Buildx cache macro applied across all image builds, adds a dedicated cache-warming job for the main branch, refactors e2e post-install waits around a shared deadline, implements in-cluster Cilium endpoint-leak healing, and updates SeaweedFS storage classes and volume sizing. ChangesBuild cache foundation and CI integration
E2E test infrastructure hardening
Docker Buildx cache adoption across Makefiles
SeaweedFS storage and health configuration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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-post-install-prep.sh`:
- Line 40: The kubectl wait command on line 40 of the e2e-post-install-prep.sh
script suppresses all stderr output by redirecting to /dev/null, which hides not
only expected NotFound errors but also authentication failures, invalid syntax,
and network errors, making debugging impossible. Remove the stderr redirection
(2>/dev/null) from the kubectl wait command entirely. This will allow all error
messages to be displayed in the logs, making it clear what the script is waiting
for and immediately surfacing any genuine misconfiguration issues, while the
script will still continue retrying based on the exit code alone.
- Line 50: The kubectl wait command uses a lowercase condition name in the
--for=condition=available parameter, but Kubernetes condition types are
case-sensitive and require Available with a capital A. Update the condition
parameter from --for=condition=available to --for=condition=Available to match
the correct Kubernetes Deployment condition type, preventing the kubectl wait
command from failing immediately during the e2e test execution.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff978f35-3d90-4432-ae46-9fc445406dbc
📒 Files selected for processing (1)
hack/e2e-post-install-prep.sh
| echo "[post-install-prep] timed out waiting for ${desc}" >&2 | ||
| return 1 | ||
| fi | ||
| if kubectl wait "$@" --timeout="${remaining}s" 2>/dev/null; then |
There was a problem hiding this comment.
Silencing all stderr makes debugging impossible.
Line 40 redirects all kubectl stderr to /dev/null to tolerate NotFound errors, but this also suppresses authentication failures, invalid syntax, network errors, and any other genuine problem. When a prerequisite fails to appear, operators will see the script retry silently for 15 minutes with no diagnostic output.
Consider one of these approaches:
- Capture stderr and only suppress lines containing "NotFound" or "no matching resources"
- Run
kubectl get <resource>first to distinguish "doesn't exist yet" from other errors - Let stderr through and rely on exit code alone (kubectl wait already retries internally)
The third option is simplest: remove 2>/dev/null entirely, accept that "NotFound" messages will appear in the log (which is actually helpful for understanding timing), and let the loop continue regardless.
Suggested approach: remove stderr suppression
- if kubectl wait "$@" --timeout="${remaining}s" 2>/dev/null; then
+ if kubectl wait "$@" --timeout="${remaining}s"; then
return 0
fiThis will log all errors (including expected "NotFound" during startup), making it clear what the script is waiting for and surfacing any genuine misconfiguration immediately.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if kubectl wait "$@" --timeout="${remaining}s" 2>/dev/null; then | |
| if kubectl wait "$@" --timeout="${remaining}s"; then |
🤖 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-post-install-prep.sh` at line 40, The kubectl wait command on line
40 of the e2e-post-install-prep.sh script suppresses all stderr output by
redirecting to /dev/null, which hides not only expected NotFound errors but also
authentication failures, invalid syntax, and network errors, making debugging
impossible. Remove the stderr redirection (2>/dev/null) from the kubectl wait
command entirely. This will allow all error messages to be displayed in the
logs, making it clear what the script is waiting for and immediately surfacing
any genuine misconfiguration issues, while the script will still continue
retrying based on the exit code alone.
| wait_for "linstor HelmRelease to be Ready" \ | ||
| helmrelease/linstor -n cozy-linstor --for=condition=Ready | ||
| wait_for "linstor-controller Deployment to be Available" \ | ||
| deployment/linstor-controller -n cozy-linstor --for=condition=available |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify kubectl wait accepts lowercase "available" for Deployment conditions
# Check if kubectl wait --help documents case requirements
kubectl wait --help 2>&1 | grep -A5 "condition="
# Attempt to validate the condition syntax (will fail on cluster without the deployment, but syntax errors surface immediately)
kubectl wait deployment/nonexistent -n default --for=condition=available --timeout=1s 2>&1 | head -20Repository: cozystack/cozystack
Length of output: 109
🏁 Script executed:
cat -n hack/e2e-post-install-prep.sh | sed -n '45,55p'Repository: cozystack/cozystack
Length of output: 678
🌐 Web query:
kubectl wait condition names case sensitivity Available kubernetes
💡 Result:
When using the kubectl wait command, the behavior regarding case sensitivity depends on what part of the --for parameter you are referencing. 1. Condition Values: When specifying the status value of a condition (e.g., --for=condition=Ready=true), the comparison is performed after Unicode simple case folding [1][2][3]. This is a form of case-insensitivity that allows for more flexible matching of the status value [4][5]. 2. Condition Names: The actual condition names (the type, such as Ready, PodScheduled, or Initialized) are case-sensitive [6]. These names must match the condition type defined in the resource's status exactly as it appears in the Kubernetes API [6]. Using incorrect casing (e.g., using --for=condition=ready instead of --for=condition=Ready) will result in the command failing to find the expected condition, often causing it to exit immediately or wait indefinitely if the condition name is not recognized [6]. Therefore, while the status value is evaluated with case-insensitive logic, you must always provide the correct, case-sensitive condition type name as defined by the Kubernetes API for the resource you are monitoring [6].
Citations:
- 1: https://kubernetes.io/docs/reference/kubectl/generated/kubectl_wait/
- 2: https://github.com/kubernetes/kubectl/blob/master/pkg/cmd/wait/wait.go
- 3: https://man.archlinux.org/man/extra/kubectl/kubectl-wait.1.en
- 4: https://www.mankier.com/1/kubectl-wait
- 5: klts-io/kubernetes@d7fab66
- 6: Fix: kubectl wait condition case from ready to Ready kubeflow/community-distribution#3076
🌐 Web query:
Kubernetes Deployment condition names Available capital
💡 Result:
In Kubernetes, Available is one of the standard condition types for a Deployment [1][2]. It is reported within the status.conditions array of a Deployment resource [1][3]. The Available condition indicates whether the Deployment has reached its minimum required availability [1][4]. Specifically, a status of True means that the Deployment has the minimum number of replicas available as required by the deployment strategy [1][2]. Key details regarding the Available condition: - Type: Available [3][2] - Status: True, False, or Unknown [3][2] - Reasons: Common reasons include MinimumReplicasAvailable (when status is True) and MinimumReplicasUnavailable (when status is False) [1][5][2]. - Calculation: The condition considers whether the number of available pods meets the minimum threshold defined by the deployment strategy (often influenced by maxUnavailable) [1][4]. A pod is considered available if it is ready for at least minReadySeconds [4][6]. Other standard Deployment conditions include Progressing and ReplicaFailure [2]. All deployment conditions follow the same schema, including fields such as lastTransitionTime, lastUpdateTime, message, reason, status, and type [3][2].
Citations:
- 1: https://kubernetes.io/docs/concepts/workloads/controllers/deployment/
- 2: https://oneuptime.com/blog/post/2026-02-09-deployment-conditions-monitor-rollout-health/view
- 3: https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/deployment-v1/
- 4: https://dev.to/maelvls/understanding-the-available-condition-of-a-kubernetes-deployment-51li
- 5: https://github.com/kubernetes/kubernetes/blob/e9b95827/pkg/controller/deployment/util/deployment_util.go
- 6: https://discuss.kubernetes.io/t/what-does-available-mean-in-context-of-deployments/6255
Change --for=condition=available to --for=condition=Available.
The Kubernetes Deployment condition type is case-sensitive and must be Available with a capital A. Using lowercase available will cause kubectl wait to fail immediately, breaking the e2e test flow.
🤖 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-post-install-prep.sh` at line 50, The kubectl wait command uses a
lowercase condition name in the --for=condition=available parameter, but
Kubernetes condition types are case-sensitive and require Available with a
capital A. Update the condition parameter from --for=condition=available to
--for=condition=Available to match the correct Kubernetes Deployment condition
type, preventing the kubectl wait command from failing immediately during the
e2e test execution.
The post-install-prep helper waited for each LINSTOR prerequisite with a separate fixed timeout anchored to the script's start: timeout 60 for the linstor HelmRelease object to exist, timeout 300 for the controller Deployment, timeout 60 for nodes Online. Those budgets raced the platform reconcile chain. The cozystack-operator alone needs ~70s just to emit the linstor HR, so the 60s exists-wait expired ~10s before the object appeared and 'set -e' aborted the whole script, failing the install with a single log line and no diagnostics. Across recent CI runs healthy LINSTOR convergence is ~11-13 min, so the per-hop guesses had no margin and passed only on favourable variance. Replace the exists+ready pairs with a wait_for helper that polls 'kubectl wait' under one shared 15m deadline -- the same window the installer's 'kubectl wait hr --all' already uses -- tolerating not-yet-created objects (kubectl wait exits non-zero on NotFound) without the set -e cliff, while still failing hard if a resource never becomes ready inside the budget. Bump the 3-nodes-Online wait 60s to 300s for the same reason. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
9e0a6d1 to
6ce1404
Compare
…t budgets kubernetes-latest flaked because the worker-readiness wait was split into 8m-to-join + 3m-to-become-Ready. Under sandbox load a slow KubeVirt VM boot consumes most of the join budget, then the tenant cluster's cilium CNI needs several more minutes to make the freshly-joined nodes Ready, overflowing the fixed 3m Ready window even though the CNI converges fine (its HelmRelease reached True shortly after). Replace with one 12m deadline polling for >=2 Ready nodes, robust to wherever the time goes. Same single-deadline shape as the LINSTOR post-install fix (#2928). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
## 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 -->
…ozystack#2928) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
|
superseded by #2948 |
What this PR does
hack/e2e-post-install-prep.shwaited for each LINSTOR prerequisite with a separate fixed timeout anchored to the script's start (timeout 60for the linstor HelmRelease to exist,timeout 300for the controller Deployment,timeout 60for nodes Online). Those budgets race the platform reconcile chain. The cozystack-operator alone needs ~70s just to emit the linstor HR, so the 60s exists-wait expired ~10s before the object appeared andset -eaborted the whole script — the install failed with a single log line and no diagnostics. This is what broke E2E on the v1.5.0-rc.3 release-cut PR (#2924); that run's platform actually converged at the normal ~10:40 mark.Across recent CI runs healthy LINSTOR convergence is ~11–13 min, so the per-hop guesses had no margin and only passed on favourable variance.
This replaces the exists+ready pairs with a
wait_forhelper that pollskubectl waitunder one shared 15m deadline — the same window the installer'skubectl wait hr --allalready uses — tolerating not-yet-created objects (kubectl waitexits non-zero on NotFound) without theset -ecliff, while still failing hard if a resource never becomes ready inside the budget. The 3-nodes-Online wait goes 60s → 300s for the same reason.Verified by running the edited script end-to-end under a fake
kubectl: slow-then-ready completes; never-ready fails at the deadline; a late HelmRelease (the rc.3 trigger) is now tolerated.Release note
Summary by CodeRabbit