fix(tenant): gate VM-resource HelmReleases on victoria-metrics-operator readiness - #3141
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 addresses transient installation failures occurring during cold starts of the cluster. By introducing explicit dependencies on the 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
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThree tenant ChangesVMO webhook dependsOn gating
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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.
Code Review
This pull request updates the tenant application templates (etcd, ingress, and monitoring) to depend on the victoria-metrics-operator HelmRelease. This ensures that during a cold install, these releases wait for the operator to become ready before deploying, preventing installation failures caused by the operator's admission webhook being temporarily unavailable. Additionally, a new unit test suite has been added to verify these dependency gates and ensure that other releases (like seaweedfs and gateway) are not needlessly blocked. I have no feedback to provide.
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.
myasnikovdaniil
left a comment
There was a problem hiding this comment.
The root cause is real and the fix is minimal. victoria-metrics-operator ships failurePolicy: Fail on its ValidatingWebhookConfiguration (confirmed: values.yaml: policy: Fail), so any CREATE of a VM* resource while the webhook backend is not yet serving on :9443 is rejected, causing the tenant releases to churn through fail-install → rollback on a cold install. Adding spec.dependsOn on the operator's HelmRelease is the correct remedy and matches the existing pattern at the system-monitoring layer (sources/monitoring.yaml variant-level dependsOn: [cozystack.victoria-metrics-operator]). The cross-shard claim holds — helm-controller v1.1.0+ resolves dependsOn via an uncached API read; this repo pins Flux v2.8 (helm-controller v1.5.0). The chain works for monitoring too: gating the parent prevents the monitoring-system child HelmRelease from being created until the operator is Ready.
Two small suggestions inline; neither blocks.
| # HelmRelease becoming Ready to avoid an install failure → rollback. The system | ||
| # monitoring components gate on the operator the same way, via a variant-level | ||
| # dependsOn in packages/core/platform/sources/monitoring.yaml. | ||
| dependsOn: |
There was a problem hiding this comment.
The comment block (7 lines) is longer than needed. The WHY is genuinely non-obvious, but the mechanism detail (helm-controller cross-shard resolution, reference to monitoring.yaml) belongs in the PR description, not the template. A tighter version:
# VMPodScrape is guarded by the VMO admission webhook (failurePolicy: Fail);
# wait for the operator to be Ready to survive a cold install.
dependsOn:Same applies to the comment blocks in ingress.yaml and monitoring.yaml.
| namespace: cozy-victoria-metrics-operator | ||
|
|
||
| - it: ingress HR depends on the victoria-metrics-operator | ||
| asserts: |
There was a problem hiding this comment.
Using equal on the entire spec.dependsOn list means this test will fail if a second legitimate dependsOn entry is added later. contains is more resilient and equally expressive:
contains:
path: spec.dependsOn
content:
name: victoria-metrics-operator
namespace: cozy-victoria-metrics-operatorIf the intent is specifically to prevent any additional dependsOn from being added silently, equal is fine — just add a note saying so. Applies to all three positive test cases.
| - templates/gateway.yaml | ||
|
|
||
| release: | ||
| name: tenant-test |
There was a problem hiding this comment.
info.yaml is absent from the template list, so the negative coverage has a gap. extra/info creates only a ServiceAccount, ConfigMap, and kubeconfig Secret — no VM* resources — so the info HelmRelease should not carry the gate. no_upgrade_force_test.yaml already includes templates/info.yaml without issue, so it renders safely in helm-unittest.
Consider adding:
- templates/info.yamlto the templates: list and a corresponding test:
- it: info HR carries no operator gate
asserts:
- template: templates/info.yaml
notExists:
path: spec.dependsOn7727ebc to
01e2ad9
Compare
…rator The tenant etcd, ingress and monitoring HelmReleases create VM* custom resources (VMPodScrape directly in etcd/ingress; VMCluster, VMAlert, VLCluster, VMAlertmanager and VMServiceScrape via the monitoring-system child of monitoring) guarded by the victoria-metrics-operator validating webhook with failurePolicy: Fail. On a cold install the operator pod can be briefly unavailable on :9443, so these releases reconcile before the webhook backend is serving and are rejected with connection refused. They carry install.remediation.retries: -1 and would eventually recover, but the failed-install -> rollback -> retry churn can outlast a cold-install readiness deadline and surfaces as spurious install failures. Add a Flux spec.dependsOn from each of these releases to the victoria-metrics-operator HelmRelease in cozy-victoria-metrics-operator, so they wait for the operator to be Ready before creating VM* resources. The system monitoring components gate on the operator the same way, via a variant-level dependsOn. The cross-shard reference (tenant releases are sharded, the operator release is not) resolves via helm-controller's uncached dependency check (APIReader, v1.1.0+). Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
01e2ad9 to
6db5ae9
Compare
|
Successfully created backport PR for |
What this PR does
The tenant
etcd,ingressandmonitoringHelmReleases create VictoriaMetrics custom resources —VMPodScrapedirectly inetcdandingress, andVMCluster/VMAlert/VLCluster/VMAlertmanager/VMServiceScrapevia themonitoring-systemchild thatmonitoringcreates. All of these are intercepted by thevictoria-metrics-operatorvalidating webhook, which is configured withfailurePolicy: Fail.On a cold install the operator pod can be briefly unavailable on
:9443(for example a transient API-server blip during startup leaves the webhook backend not yet serving). In that window the tenant releases reconcile, the webhook rejects their VM* resources withconnection refused, and the releases churn through failed-install → rollback → retry. They carryinstall.remediation.retries: -1and recover eventually, but the churn can outlast a cold-install readiness deadline and surfaces as spurious install failures.This adds a Flux
spec.dependsOnfrom each of those three tenant releases to thevictoria-metrics-operatorHelmRelease incozy-victoria-metrics-operator, so they wait for the operator to be Ready before creating any VM* resource. The system monitoring components already gate on the operator the same way (a variant-leveldependsOnin the platform package sources); this brings the tenant releases to parity.failurePolicy: Failis preserved — validation is unchanged.The tenant releases are sharded (
sharding.fluxcd.io/key) while the operator release is not. helm-controller resolvesdependsOnthrough an uncached API read that bypasses the shard's watch-label-selector cache (since v1.1.0, PR fluxcd/helm-controller#1070), so the cross-shard reference is visible to the tenant shard's controller; this repo ships helm-controller v1.5.0.A helm-unittest suite pins the gate on all three releases and asserts that the tenant releases creating no VM* resources (
seaweedfs,gateway) do not carry it, documenting the intentional narrowness of the gate.Release note
Summary by CodeRabbit