fix(e2e): widen tenant-root monitoring bring-up waits to 15m under load - #3160
Conversation
📝 WalkthroughWalkthroughModified the tenant-root e2e install test to standardize VictoriaMetrics and Grafana readiness waits at 15 minutes and to print resource YAML plus tenant-root pod status when waits fail. ChangesTenant install readiness checks
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: Poem
🚥 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 improves the robustness of the E2E installation process by adjusting timeout thresholds for VictoriaMetrics components. By aligning these timeouts with existing resource behavior under load and adding diagnostic output, the change reduces flaky test failures caused by resource contention while maintaining the ability to detect genuine system regressions. 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 increases the timeout budget for VictoriaMetrics components to 15 minutes in the E2E installation script to prevent timeouts under heavy install-time load, and adds failure blocks to dump live status and pod information upon timeout. The review feedback suggests redirecting these diagnostic outputs to stderr for consistency, and notes that the PR title or commit message should use a valid scope (such as hack or tests) according to the repository style guide.
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.
| kubectl wait sts/etcd -n tenant-root --for=jsonpath='{.status.readyReplicas}'=3 --timeout=5m | ||
|
|
||
| # VictoriaMetrics components | ||
| # VictoriaMetrics components. vmalert/vmalertmanager, vlclusters/generic and |
There was a problem hiding this comment.
The commit scope e2e used in the PR title/commit message is not a valid scope according to the repository style guide. Please use one of the valid scopes, such as hack or tests.
References
- Each commit must follow Conventional Commits format with a valid scope. Valid scopes for meta/testing are
api,hack,tests,ci,docs. (link)
| kubectl wait vmalert/vmalert-shortterm vmalertmanager/alertmanager -n tenant-root --for=jsonpath='{.status.updateStatus}'=operational --timeout=15m || { | ||
| echo "=== vmalert/vmalert-shortterm, vmalertmanager/alertmanager did not reach updateStatus=operational ===" | ||
| kubectl get vmalert/vmalert-shortterm vmalertmanager/alertmanager -n tenant-root -o yaml 2>&1 || true | ||
| echo "=== tenant-root pods ===" | ||
| kubectl get pods -n tenant-root -o wide 2>&1 || true | ||
| false | ||
| } |
There was a problem hiding this comment.
To maintain consistency with the rest of the script (which redirects error diagnostics to stderr using >&2), please redirect the diagnostic output of this failure block to stderr.
kubectl wait vmalert/vmalert-shortterm vmalertmanager/alertmanager -n tenant-root --for=jsonpath='{.status.updateStatus}'=operational --timeout=15m || {
echo "=== vmalert/vmalert-shortterm, vmalertmanager/alertmanager did not reach updateStatus=operational ===" >&2
kubectl get vmalert/vmalert-shortterm vmalertmanager/alertmanager -n tenant-root -o yaml >&2 || true
echo "=== tenant-root pods ===" >&2
kubectl get pods -n tenant-root -o wide >&2 || true
false
}
| kubectl wait vlclusters/generic -n tenant-root --for=jsonpath='{.status.updateStatus}'=operational --timeout=15m || { | ||
| echo "=== vlclusters/generic did not reach updateStatus=operational ===" | ||
| kubectl get vlclusters/generic -n tenant-root -o yaml 2>&1 || true | ||
| echo "=== tenant-root pods ===" | ||
| kubectl get pods -n tenant-root -o wide 2>&1 || true | ||
| false | ||
| } |
There was a problem hiding this comment.
To maintain consistency with the rest of the script (which redirects error diagnostics to stderr using >&2), please redirect the diagnostic output of this failure block to stderr.
kubectl wait vlclusters/generic -n tenant-root --for=jsonpath='{.status.updateStatus}'=operational --timeout=15m || {
echo "=== vlclusters/generic did not reach updateStatus=operational ===" >&2
kubectl get vlclusters/generic -n tenant-root -o yaml >&2 || true
echo "=== tenant-root pods ===" >&2
kubectl get pods -n tenant-root -o wide >&2 || true
false
}
| kubectl wait vmcluster/shortterm vmcluster/longterm -n tenant-root --for=jsonpath='{.status.updateStatus}'=operational --timeout=15m || { | ||
| echo "=== vmcluster/shortterm,longterm did not reach updateStatus=operational ===" | ||
| kubectl get vmcluster/shortterm vmcluster/longterm -n tenant-root -o yaml 2>&1 || true | ||
| echo "=== tenant-root pods ===" | ||
| kubectl get pods -n tenant-root -o wide 2>&1 || true | ||
| false | ||
| } |
There was a problem hiding this comment.
To maintain consistency with the rest of the script (which redirects error diagnostics to stderr using >&2), please redirect the diagnostic output of this failure block to stderr.
kubectl wait vmcluster/shortterm vmcluster/longterm -n tenant-root --for=jsonpath='{.status.updateStatus}'=operational --timeout=15m || {
echo "=== vmcluster/shortterm,longterm did not reach updateStatus=operational ===" >&2
kubectl get vmcluster/shortterm vmcluster/longterm -n tenant-root -o yaml >&2 || true
echo "=== tenant-root pods ===" >&2
kubectl get pods -n tenant-root -o wide >&2 || true
false
}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
hack/e2e-install-cozystack.bats (2)
228-250: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepeated wait+diagnostics boilerplate across three blocks.
The three
kubectl wait ... || { echo ...; kubectl get ... -o yaml; echo ...; kubectl get pods ...; false; }blocks are structurally identical except for resource kind/names. Extracting a small shell helper (e.g.,wait_operational <desc> <resources...>) would remove the duplication and make future timeout/diagnostic tweaks single-sourced.♻️ Example helper extraction
+ wait_operational() { + desc=$1; shift + kubectl wait "$@" -n tenant-root --for=jsonpath='{.status.updateStatus}'=operational --timeout=15m || { + echo "=== ${desc} did not reach updateStatus=operational ===" + kubectl get "$@" -n tenant-root -o yaml 2>&1 || true + echo "=== tenant-root pods ===" + kubectl get pods -n tenant-root -o wide 2>&1 || true + false + } + } + timeout 60 sh -ec 'until kubectl get vmalert/vmalert-shortterm -n tenant-root >/dev/null 2>&1; do sleep 2; done' timeout 60 sh -ec 'until kubectl get vmalertmanager/alertmanager -n tenant-root >/dev/null 2>&1; do sleep 2; done' - kubectl wait vmalert/vmalert-shortterm vmalertmanager/alertmanager -n tenant-root --for=jsonpath='{.status.updateStatus}'=operational --timeout=15m || { - echo "=== vmalert/vmalert-shortterm, vmalertmanager/alertmanager did not reach updateStatus=operational ===" - kubectl get vmalert/vmalert-shortterm vmalertmanager/alertmanager -n tenant-root -o yaml 2>&1 || true - echo "=== tenant-root pods ===" - kubectl get pods -n tenant-root -o wide 2>&1 || true - false - } + wait_operational "vmalert/vmalert-shortterm, vmalertmanager/alertmanager" vmalert/vmalert-shortterm vmalertmanager/alertmanager🤖 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-install-cozystack.bats` around lines 228 - 250, The three kubectl wait diagnostic blocks in hack/e2e-install-cozystack.bats are duplicated boilerplate; extract the shared wait-and-dump-failure flow into a small shell helper and reuse it for the vmalert/vmalertmanager, vlclusters/generic, and vmcluster/shortterm,longterm checks. Keep the helper focused on taking a description plus resource arguments, then performing the wait, emitting the same YAML/pods diagnostics on failure, and returning a nonzero status so the existing test flow remains unchanged.
226-250: 🚀 Performance & Scalability | 🔵 TrivialWorst-case runtime for this block triples (15m → 45m).
Raising all three waits to 15m sequentially raises this block's worst-case budget from 15m to 45m, on top of ~30m of other waits already in this test. This is an explicit, documented tradeoff per the PR rationale, but worth keeping in mind for overall CI job timeout headroom if install-time contention worsens further.
🤖 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-install-cozystack.bats` around lines 226 - 250, The sequential kubectl wait calls in the e2e install test are increasing the worst-case runtime from 15m to 45m. Review the wait logic around the vmalert/alertmanager, vlclusters/generic, and vmcluster/shortterm/longterm checks in hack/e2e-install-cozystack.bats and either reduce the per-resource timeout or consolidate the waits so the block does not consume a full 15m three times in sequence. Keep the existing readiness checks and failure diagnostics, but make the total budget explicit and bounded.
🤖 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-install-cozystack.bats`:
- Around line 228-250: The three kubectl wait diagnostic blocks in
hack/e2e-install-cozystack.bats are duplicated boilerplate; extract the shared
wait-and-dump-failure flow into a small shell helper and reuse it for the
vmalert/vmalertmanager, vlclusters/generic, and vmcluster/shortterm,longterm
checks. Keep the helper focused on taking a description plus resource arguments,
then performing the wait, emitting the same YAML/pods diagnostics on failure,
and returning a nonzero status so the existing test flow remains unchanged.
- Around line 226-250: The sequential kubectl wait calls in the e2e install test
are increasing the worst-case runtime from 15m to 45m. Review the wait logic
around the vmalert/alertmanager, vlclusters/generic, and
vmcluster/shortterm/longterm checks in hack/e2e-install-cozystack.bats and
either reduce the per-resource timeout or consolidate the waits so the block
does not consume a full 15m three times in sequence. Keep the existing readiness
checks and failure diagnostics, but make the total budget explicit and bounded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c51c3c94-c81a-4c39-8b86-0b4deb8dd86c
📒 Files selected for processing (1)
hack/e2e-install-cozystack.bats
vlclusters/generic and vmcluster wait for updateStatus=operational with a 10m budget, while their vmalert/vmalertmanager sibling in the same block already uses 15m. All three are vm-operator-managed monitoring resources that only become operational once their workloads schedule and turn Ready, and during platform bring-up they contend for node resources with the rest of the install. On a calm sandbox each converges in under a second, but under install-time load (concurrent e2e sandboxes on one runner) monitoring bring-up is slow, and a 10m/15m split inside a block whose resources all contend for the same node capacity risks failing the whole install on a PR that never touched monitoring. Unify vlclusters and vmcluster on the sibling's 15m budget so the whole block shares one headroom, and dump resource YAML plus pod state on timeout across all three waits so a genuine stuck-not-slow regression stays legible instead of a bare "timed out" line. The extra budget costs near-zero wall-time on the happy path and stays inside the E2E job budget. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
The Configure-Tenant install test completes the tenant-root monitoring bring-up by waiting for the grafana-db CNPG cluster and the grafana-deployment Deployment. Both used a 10m budget while the VictoriaMetrics waits above them use 15m. They complete the same monitoring bring-up and contend for the same node resources during install, so under install-time load (concurrent e2e sandboxes on one runner) either can be slow, and the 10m/15m split within the same contend-for-resources bring-up risks failing the whole install on a PR that never touched monitoring. Raise both Grafana waits to the same uniform 15m budget as the vm-operator waits and wrap them in an on-timeout diagnostic block that dumps the resource YAML plus tenant-root pod state, so a genuinely stuck resource still fails at 15m with legible diagnostics instead of a bare "timed out" line. The extra budget costs near-zero wall-time on the happy path and stays inside the E2E job budget. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
9c0d939 to
09f9d0b
Compare
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM
Test-only change that uniformly widens six monitoring bring-up waits to 15m and adds on-timeout diagnostics; the diagnostic wrapper correctly still fails the test on a genuine timeout, so the gate is not weakened.
Operational risks
None. The single material risk for this PR — whether the || { ...; false; } diagnostic wrapper masks a real timeout and lets the test pass — was checked directly. The wrapper's diagnostic kubectl get commands are each guarded with || true (so they cannot abort the block early), and every block ends with a bare false. Under both bash and POSIX sh/dash with set -e, the construct wait --timeout=15m || { ...; false; } returns non-zero and aborts the function. Verified empirically in both shells: diagnostics print, the post-block statement does NOT run, and the function exits 1, which cozytest.sh records as a failed test (__RC__ capture at hack/cozytest.sh:39,56). A genuinely stuck resource still fails, now at 15m.
Caveats
- This PR touches only
hack/e2e-install-cozystack.bats— an e2e test. Confirmed the diff touches no file underpackages/. Phase 5b upgrade / fresh-install impact analysis therefore does not apply: no chart, values, schema, CRD, RBAC, or migration surface is affected, and there is no runtime cluster impact. - No bats lifecycle hooks (
setup_file/teardown_file/setup/teardown) and no top-levelcozy_cleanupwere added, so there is no silently-dead-code concern undercozytest.sh. release-noteblock is present and set toNONE, which is appropriate for a test-only change.
Claim alignment (all OK, listed for the record)
- "lifts the whole tenant-root monitoring wait-set to a single uniform 15m budget" — OK. All waits now use
--timeout=15m:hack/e2e-install-cozystack.bats:232(vmalert + vmalertmanager),:240(vlclusters/generic),:248(vmcluster shortterm + longterm),:265(grafana-db CNPG cluster),:273(grafana-deployment). - "wraps each wait in an on-timeout diagnostic block that dumps the resource YAML + tenant-root pod state" — OK. Each of the five waits gains a
|| { echo ...; kubectl get <res> -o yaml || true; kubectl get pods -n tenant-root -o wide || true; false; }block. - "A genuinely stuck resource still fails — now at 15m" — OK. Failure propagation verified.
Notes (non-blocking)
- No stale-
Ready=Truerace applies here: none of these waits is immediately preceded by akubectl patchof the same object. The only patch in the test (:188) is separated from this block by the tenant-root parent-HR readiness wait (:201-202).
What this PR does
The
Configure Tenant and wait for applicationsinstall test brings up the entire root-tenant monitoring stack and waits for each component to become ready/operational. Those waits span the VictoriaMetrics operator resources (vmalert/vmalertmanager,vlclusters/generic,vmcluster/shortterm+longterm) and the Grafana pair (grafana-dbCNPG cluster,grafana-deployment). Each converges only once its underlying workloads schedule and turn Ready, and during platform bring-up they contend for the same node resources as the rest of the install — so their convergence is load-sensitive.Only
vmalert/vmalertmanagerused a 15m budget; the other four used 10m, so this block carried a 10m/15m split even though all its resources contend for the same node capacity during install. Under install-time load (multiple concurrent e2e sandboxes on one runner) the monitoring bring-up is slow across the whole block rather than on a single resource, so the shorter budgets risk failing the whole install on PRs whose branches never touched monitoring — and per-resource bumps are whack-a-mole: raising one budget just moves the risk to the next component in the block. On a calm sandbox each of these reaches ready/operational in under a second, which is what distinguishes the two cases — slow-not-stuck convergence under contention, not a product regression.This lifts the whole tenant-root monitoring wait-set to a single uniform 15m budget and wraps each wait in an on-timeout diagnostic block that dumps the resource YAML (status conditions /
updateStatus) plus tenant-root pod state. A genuinely stuck resource still fails — now at 15m, and with legible diagnostics instead of a baretimed outline — so this does not weaken the gate's ability to catch real breakage. The extra budget costs near-zero wall-time on the happy path and stays well inside the E2E job's 180m cap.The remaining upstream angle is the real driver: how long the VictoriaMetrics/VictoriaLogs stack and the Grafana bring-up take to become operational under resource contention during install. This change addresses the e2e robustness side; reducing that bring-up cost is a separate, product-side improvement.
Screenshots
Not applicable — no UI changes.
Release note
Summary by CodeRabbit
Bug Fixes
Tests