Repository navigation
fix(kubernetes): close admin-kubeconfig race on tenant cluster bootstrap - #2413
Conversation
Adds a failing unit test for convertApplicationToHelmRelease asserting that Install.Timeout and Upgrade.Timeout are at least 15 minutes. The default flux helm-controller timeout is too short to cover cold-start Kamaji control-plane bootstrap (image pull + etcd bootstrap + apiserver Ready + admin-kubeconfig Secret generation) and causes install remediation loops. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Parent HelmRelease created by cozystack-api for Kubernetes tenants contains CAPI/Kamaji resources (Cluster, KamajiControlPlane, MachineDeployment) that asynchronously provision the *-admin-kubeconfig Secret. Three Deployments in the same chart (cluster-autoscaler, kccm, kcsi-controller) mount that Secret directly, so the helm-wait cannot complete until control-plane bootstrap finishes. Default flux helm-controller timeout is too short for a cold-node first-tenant bootstrap (image pull + etcd bootstrap + apiserver Ready + admin-kubeconfig generation routinely exceed it). On timeout, install.remediation triggers uninstall, which removes the Cluster CR and restarts the cycle indefinitely. Bumping Install.Timeout and Upgrade.Timeout to 15m gives realistic bootstrap headroom while remaining bounded. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Adds failing helm unittest suite for packages/apps/kubernetes covering: - cluster-autoscaler, kccm, and csi controller Deployments mount the admin-kubeconfig Secret with optional: true - each of those Deployments has a wait-for-kubeconfig initContainer that mounts the same kubeconfig path - cluster.yaml renders a helm fail with a descriptive message when the tenant has no etcd DataStore (empty _namespace.etcd) Also wires up a test target in the chart Makefile so helm-unit-tests.sh picks it up. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Three Deployments in the Kubernetes app chart mount the tenant admin-kubeconfig Secret directly as a volume: cluster-autoscaler, kccm, and the kcsi controller. That Secret is provisioned asynchronously by Kamaji after control-plane bootstrap, so on a fresh install the pods used to hit FailedMount and the parent HelmRelease ran out of its wait budget. Mark the Secret volume optional and add a wait-for-kubeconfig initContainer that polls the mounted path until the Secret appears. Kubelet remounts the optional Secret within its sync period once Kamaji publishes it, the init container exits, and the main container starts cleanly. The Deployment becomes Available without the helm-wait ever seeing a FailedMount. Pins a busybox image for the init container via images/busybox.tag (same format as the other pinned tags in this chart). Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
When a Kubernetes tenant is created without a parent tenant that has etcd enabled, .Values._namespace.etcd is empty and the rendered KamajiControlPlane spec carries an empty dataStoreName. The Kamaji admission webhook then rejects every TenantControlPlane create with "tenant-root DataStore does not exist" and the control plane never comes up. Add a helm template-level guard that fails rendering with a descriptive, actionable error message before the HelmRelease even reaches the webhook. This also closes the narrow race where a Kubernetes HelmRelease reconciles before the etcd HelmRelease has created the DataStore CR - flux retries on its interval and picks up the DataStore once it appears. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Before cleanup, inspect the parent HelmRelease installFailures and upgradeFailures counters. A non-zero value means flux helm-controller hit its wait timeout, ran install/upgrade remediation (uninstall), and re-installed - the exact race condition this PR closes. Fail the bats test in that case so the signal surfaces in CI instead of being masked by a green retry. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Shell && and || have equal precedence and left-to-right associativity, so the previous guard parsed as (((A && B) || C) && D) and silently passed on the canonical failure mode: install_failures=1 with an empty upgrade_failures. Extract the check into helmrelease_has_remediation_cycle() in a dedicated helper sourced from run-kubernetes.sh, and add unit tests under hack/remediation-guard.bats that pin the expected behavior for every combination of empty, zero, and positive counter values. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
The previous change set Install.Timeout and Upgrade.Timeout to 15m on every Application's parent HelmRelease, but the admin-kubeconfig race documented in #2412 is specific to the Kubernetes kind: only its parent chart creates CAPI/Kamaji resources whose admin-kubeconfig Secret is asynchronously provisioned and mounted by Deployments in the same chart. Other kinds (Qdrant, MongoDB, Postgres, ...) have no such race and should not have their failed installs linger three times longer before flux triggers remediation. Gate the timeout on r.kindName == "Kubernetes". Rewrite rest_timeout_test.go to cover both sides: Kubernetes must get a >= 15m timeout, other kinds must keep the flux defaults. Both tests also pin Install/Upgrade Remediation.Retries == -1 so a future edit that removes unbounded remediation would show up as a red test. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Adds images/busybox/Dockerfile and an image-busybox Makefile target that mirror the same pattern as the rest of this chart's images (the Dockerfile pins the upstream busybox by digest; the Makefile target builds and tags for ghcr.io/cozystack/cozystack/busybox the same way cluster-autoscaler et al. are handled). Also wires it into the umbrella image target so 'make image' rebuilds everything. Until the first release build runs image-busybox and rewrites the .tag to point at ghcr.io, the .tag keeps a fully-qualified docker.io/library/busybox:1.37.0@sha256:... reference so pods do not silently resolve the short name via the default registry and pulls remain immutable by digest. The release workflow overwrites this file the same way it does for the other images. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Adds an assertion that cluster.yaml renders successfully when _namespace.etcd is set and produces a KamajiControlPlane whose dataStoreName equals the tenant's etcd DataStore name. Without this positive case a future edit that inverts or removes the existing etcd guard would pass the suite as long as the negative case still fails. Also adds documentSelector: kind=Deployment to the kccm and csi controller assertions so the jsonpath filter operates on a single document, matching the cluster-autoscaler case and removing reliance on helm-unittest filter-vs-single-value coercion. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Cap the wait-for-kubeconfig init container at 20m. If Kamaji genuinely fails to produce the admin-kubeconfig Secret (misconfigured tenant, etcd outage after the guard already passed, Kamaji crash-loop), the pod now exits non-zero and goes CrashLoopBackOff so the failure is visible in dashboards, instead of silently sleeping in Init forever and leaving only the flux helm-wait timeout to surface the problem. Reword the etcd DataStore guard to reference the parent Tenant application's etcd flag (not .Values.etcd of the Kubernetes chart, which is a different chart). Update the helm unittest errorPattern accordingly. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
…tore
A hard helm fail in cluster.yaml made every cold bootstrap racy: if
the parent Tenant chart had not yet populated _namespace.etcd in
cozystack-values when the Kubernetes HelmRelease first reconciled, the
fail fired, install.remediation triggered, installFailures incremented
and the new e2e remediation-guard flagged it as a bug. That directly
contradicts the race the rest of this PR is trying to close.
Replace fail with a graceful skip: render only a status-beacon
ConfigMap (test-awaiting-etcd) when etcd is empty, wrap all
CAPI/Kamaji resources in {{ if $etcd }}. The HelmRelease installs
successfully and goes Ready; flux retries on its 5m interval and picks
up the DataStore as soon as the Tenant chart finishes reconciling.
Update the helm unittest: positive test still asserts dataStoreName on
KamajiControlPlane; the negative test now asserts exactly one
ConfigMap document with status=awaiting-etcd, no Cluster / KCP /
KubevirtCluster / WorkloadMonitor rendered.
Assisted-By: Claude <[email protected]>
Signed-off-by: Aleksei Sviridkin <[email protected]>
…lper The three control-plane-side Deployments (cluster-autoscaler, kccm, kcsi-controller) carried three copies of the same 20-line init container. That already drifted: the CSI copy used 4-space nesting while the other two used 2-space. Any future update to the image, the deadline, or the poll script had to land in three places or silently diverge. Extract the block into a new kubernetes.waitForAdminKubeconfig helper in templates/_helpers.tpl and include it at each call site. Tighten the deadline from 20m to 10m so it stays strictly below the 15m HelmRelease Install.Timeout and the CrashLoopBackOff surfaces in dashboards before flux remediation can fire. Also clarify the wait message so operators debugging a stuck init container do not chase Kamaji for what is actually kubelet's optional-Secret refresh cadence. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Replace the hardcoded r.kindName == "Kubernetes" switch in rest.go with a config-driven path. The ApplicationDefinition CR now accepts a release.cozystack.io/helm-install-timeout annotation that is parsed at cozystack-api startup into config.ReleaseConfig.HelmInstallTimeout and applied to both Install.Timeout and Upgrade.Timeout on the rendered HelmRelease. Applications that leave the annotation unset keep flux defaults so their failed installs remediate on the normal cadence - only the Kubernetes kind carries the override and gets a 15m budget. New kinds with a similar race can opt in by setting the same annotation; no rest.go patch needed. Kubernetes-rd sets the annotation to 15m. Table-driven test in rest_timeout_test.go covers three cases: Kubernetes with 15m, Qdrant unset, and an arbitrary future kind with 20m - all of which pin the Remediation.Retries == -1 contract as well. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
The per-template unittests in packages/apps/kubernetes/tests/ assert that cluster-autoscaler, kccm, and the csi controller each mount the admin-kubeconfig Secret optional and carry the wait-for-kubeconfig init. That locks in today's three Deployments by name - a fourth Deployment that mounts the same Secret but forgets the guard would slip past them. Add a bats-unit test that renders the entire chart, enumerates every Deployment whose spec mounts a Secret ending in -admin-kubeconfig, and asserts optional:true plus wait-for-kubeconfig init on all of them. Verified by temporarily removing optional:true from csi/deploy.yaml: the test correctly flagged invariant-kcsi-controller as an offender. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
run-kubernetes.sh extracts .status.installFailures and .status.upgradeFailures via kubectl -o jsonpath. If a future flux release renames the counters, kubectl returns empty, the guard reports no cycle, and e2e silently misses real remediation loops. Add a bats unit test that feeds a pinned HelmRelease v2 status snippet through the same jsonpath and asserts the extraction still yields the expected values. Also leave a pointer comment in run-kubernetes.sh so a future flux bump surfaces the version coupling in review. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
The soft-skip wrap in cluster.yaml only silenced Cluster and
KamajiControlPlane rendering when _namespace.etcd is empty. The three
CP-side Deployments (cluster-autoscaler, kccm, kcsi-controller) still
rendered, their wait-for-kubeconfig init containers CrashLoopBackOff'd
forever (no KamajiControlPlane = no admin-kubeconfig Secret),
HelmRelease hit its 15m wait timeout and triggered the very install
remediation cycle the rest of this PR prevents. Self-contradiction.
Wrap each of the three Deployment templates in
{{- if .Values._namespace.etcd }}...{{- end }} so they render only
when there is a DataStore to back them. Add an invariant bats test
that renders the whole chart with etcd empty and asserts zero
Deployments reference *-admin-kubeconfig.
Assisted-By: Claude <[email protected]>
Signed-off-by: Aleksei Sviridkin <[email protected]>
Flux helm-controller's ClearFailures() zeroes installFailures and upgradeFailures on every successful reconciliation (see the upstream HelmReleaseStatus method). The previous guard ran after the HelmRelease was Ready, at which point the counters were always 0 - the assertion was vacuous and would have passed against a reverted fix. Switch to .status.history, which retains per-revision release Snapshots that survive a subsequent successful reconciliation. A remediation cycle leaves behind a Snapshot with status=uninstalled (the install-remediation code path) or status=failed (Helm release failure that remediation then uninstalled). Either one signals the race actually fired. Rewrite the bats unit tests to cover: empty history, deployed-only, deployed+superseded (happy path - not detected), single failed, single uninstalled, uninstalled-then-deployed, and deployed-then-failed (all detected). The pinned-shape test feeds a realistic HR status snippet through yq the same way run-kubernetes.sh does via kubectl -o jsonpath. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
CI runs make unit-tests on every PR, which already covers helm unittests and bats (hack/admin-kubeconfig-invariant.bats and hack/remediation-guard.bats are both picked up by the existing hack/*.bats glob). What was missing was any go test invocation. Add a go-unit-tests target scoped to pkg/registry, pkg/config, and pkg/cmd/server - the cozystack-api surface this repo actually owns and tests in-tree. Running go test ./... pulls in generated-code round-trip suites whose behavior is governed by generator tool versions outside this repo's control; those are better exercised from their own generator workflows. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
…rectly The image-busybox Makefile target + images/busybox/Dockerfile wrapper just rebuilt an upstream busybox digest as ghcr.io/cozystack/cozystack/ busybox. Payload is a one-shot sh loop run once per pod; the pinned upstream digest is already immutable, so maintaining a private mirror adds churn (rebuild on every release) for no real hardening benefit. Delete the wrapper and reference docker.io/library/busybox:<digest> directly from images/busybox.tag. Document the choice in _helpers.tpl. Also drop the false coupling in the go table test: the "unrelated kind without configured timeout" case used the real kind name Qdrant, which tied the test to the Qdrant ApplicationDefinition for no reason. Switch to a clearly fictional kind so a future Qdrant timeout override does not break this assertion. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
17 child HelmReleases (cilium, coredns, csi, cert-manager, metrics-server, ...) referenced *-admin-kubeconfig via kubeConfig.secretRef and rendered even when _namespace.etcd was empty. On an etcd-less tenant each one sat in NotReady forever because the admin-kubeconfig Secret only exists after a KamajiControlPlane reconciles, and KamajiControlPlane now only renders when etcd is set. The outcome contradicted the "beacon only" contract claimed in the soft-skip commit. Extend the existing addon guards to also require _namespace.etcd, and wrap the four unconditional HelmReleases (csi, metrics-server, prometheus-operator-crds, volumesnapshot-crd) plus the always-on cilium/coredns HR resources in the same gate. Add an invariant bats test that renders the whole chart with etcd empty and asserts zero HelmReleases reference *-admin-kubeconfig. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
… gate Pulls the release.cozystack.io/helm-install-timeout parsing out of start.go into ParseHelmInstallTimeoutAnnotation in pkg/config. The helper rejects units that time.ParseDuration accepts but Flux helm-controller rejects (ns, us, µs): feeding one of those through would cause the HelmRelease admission webhook to reject the object at install time, giving a silent drop to flux defaults that is hard to debug. Fail loudly at cozystack-api startup instead. Adds a table-driven unit test covering: unset (empty), accepted units ms/s/m/h, compound 2h30m, decimal 1.5m, and the rejected shapes (bare digits, garbage, negative, ns/us/µs). The test lives in pkg/config so it runs under the existing go-unit-tests make target. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
- Log .status.history regardless of content so a silently empty result (Flux field rename) shows up in CI logs, and treat empty history on a Ready HelmRelease as a distinct failure. A Ready HR by definition has at least one snapshot; anything else is a shape-drift signal. - Replace the unquoted heredoc in remediation-guard.sh with a printf | grep pipeline. printf %s treats statuses as literal payload (no $ expansion surprises for future callers), grep --quiet --extended-regexp returns exit status the caller can forward directly. - Share the etcd-absent values file between both invariant tests (packages/apps/kubernetes/tests/values-ci-no-etcd.yaml) instead of duplicating the --set block. - Fix typo "override applied" -> "override is applied" in the Kubernetes ApplicationDefinition. - Add a coupling comment in the ApplicationDefinition annotation that points at the wait-for-kubeconfig init deadline in _helpers.tpl, so a future operator raising the HR timeout updates the init deadline too. - Clarify the per-annotation timeout comment in rest.go so it stops implying the feature is Kubernetes-only (it is not - only today's one user is). Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
📝 WalkthroughWalkthroughConstructs per-application Helm install/upgrade timeout from ApplicationDefinition annotation; gates many Kubernetes/HelmRelease templates on presence of an etcd namespace; adds a BusyBox-based initContainer helper to wait for admin-kubeconfig and marks secret volumes optional; adds tests and a remediation-cycle guard; expands Makefile test targets. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant API as ApplicationDefinition (CR)
participant Server as CozyServer (start.go)
participant Config as pkg/config
participant Registry as pkg/registry (REST)
participant Flux as Flux (HelmRelease)
API->>Server: Provide ApplicationDefinition (includes annotations)
Server->>Config: ParseHelmInstallTimeoutAnnotation(annotation)
alt parse success
Config-->>Server: time.Duration
Server->>Registry: Build ReleaseConfig (HelmInstallTimeout set)
Registry->>Flux: convertApplicationToHelmRelease (sets spec.install/upgrade.timeouts)
Flux-->>Cluster: HelmRelease with timeouts applied
else parse failure
Config-->>Server: error
Server-->>API: return error (invalid annotation)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Trivy (0.69.3)Failed to read Trivy output file: ENOENT: no such file or directory, open '/inmem/1273/nsjail-bf65412a-6caf-4b1c-aebf-d837eee2ad4b/merged/.trivy-output.json' 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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, reopen this pull request to trigger a review.
dosubot
Bot
added
the
size/L
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 race condition during tenant Kubernetes cluster bootstrap where the parent HelmRelease would prematurely trigger remediation cycles because the Kamaji-provisioned admin-kubeconfig Secret was not yet available. The changes introduce a more resilient initialization sequence, configurable Helm release timeouts, and conditional resource rendering to ensure the system waits gracefully for necessary dependencies without entering infinite failure loops. Highlights
🧠 New Feature in Public Preview: You can now enable Memory 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 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 counter productive. 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
|
![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 implements a mechanism to prevent Flux remediation loops during Kubernetes tenant bootstrapping by gating workloads on etcd availability and introducing a configurable Helm install timeout. It adds a wait-for-kubeconfig init container and comprehensive BATS/E2E guards to ensure stability. Feedback identifies several Helm templates where values blocks are rendered outside the etcd conditional, which would produce invalid Kubernetes manifests when the DataStore is missing.
| {{- end }} | ||
|
|
||
| {{- if .Values.addons.certManager.enabled }} | ||
| {{- if and .Values.addons.certManager.enabled .Values._namespace.etcd }} |
There was a problem hiding this comment.
This condition only guards the HelmRelease resource. However, the template block at the top of the file (lines 1-9) which defines the cert-manager: values override will still be rendered when _namespace.etcd is empty. This results in the chart producing invalid Kubernetes YAML (a bare cert-manager: key without a resource header) which may cause failures during helm install or when Flux applies the manifest. Consider wrapping the entire file content in the _namespace.etcd check, or combining it with the existing conditional at line 1, similar to the approach taken in cert-manager-crds.yaml and monitoring-agents.yaml.
There was a problem hiding this comment.
Per the Helm docs on named templates: "a define does not produce output unless it is called with a template" (https://helm.sh/docs/chart_template_guide/named_templates/). The define block at the top of this file registers a named template; its only call site is the include inside the HelmRelease spec, which is already gated on .Values._namespace.etcd. Verified on the chart:
helm template test packages/apps/kubernetes \
--values packages/apps/kubernetes/tests/values-ci-no-etcd.yaml \
--show-only templates/helmreleases/cert-manager.yaml
# Error: could not find template templates/helmreleases/cert-manager.yaml in chart
Zero documents on the output. The chart-wide invariant chart emits zero admin-kubeconfig HelmReleases when tenant has no etcd DataStore in hack/admin-kubeconfig-invariant.bats asserts the same across all 17 HelmRelease templates and passes today.
| {{- end }} | ||
| {{- end }} | ||
|
|
||
| {{- if .Values._namespace.etcd }} |
There was a problem hiding this comment.
Similar to cert-manager.yaml, the values override block at the top of this file (lines 1-15) is not guarded by the _namespace.etcd check. When etcd is missing, this template will render an invalid manifest containing only the cilium: key. Please ensure the entire file's output is gated on _namespace.etcd.
There was a problem hiding this comment.
Per the Helm docs on named templates: "a define does not produce output unless it is called with a template" (https://helm.sh/docs/chart_template_guide/named_templates/). The define block at the top of this file registers a named template; its only call site is the include inside the HelmRelease spec, which is already gated on .Values._namespace.etcd. Verified on the chart:
helm template test packages/apps/kubernetes \
--values packages/apps/kubernetes/tests/values-ci-no-etcd.yaml \
--show-only templates/helmreleases/cilium.yaml
# Error: could not find template templates/helmreleases/cilium.yaml in chart
Zero documents on the output. The chart-wide invariant chart emits zero admin-kubeconfig HelmReleases when tenant has no etcd DataStore in hack/admin-kubeconfig-invariant.bats asserts the same across all 17 HelmRelease templates and passes today.
| clusterIP: "10.95.0.10" | ||
| {{- end }} | ||
|
|
||
| {{- if .Values._namespace.etcd }} |
There was a problem hiding this comment.
The values block at lines 1-5 will be rendered even when _namespace.etcd is empty, leading to invalid YAML output. Consider moving the _namespace.etcd check to the top of the file to ensure no output is produced when the tenant is awaiting etcd.
There was a problem hiding this comment.
Per the Helm docs on named templates: "a define does not produce output unless it is called with a template" (https://helm.sh/docs/chart_template_guide/named_templates/). The define block at the top of this file registers a named template; its only call site is the include inside the HelmRelease spec, which is already gated on .Values._namespace.etcd. Verified on the chart:
helm template test packages/apps/kubernetes \
--values packages/apps/kubernetes/tests/values-ci-no-etcd.yaml \
--show-only templates/helmreleases/coredns.yaml
# Error: could not find template templates/helmreleases/coredns.yaml in chart
Zero documents on the output. The chart-wide invariant chart emits zero admin-kubeconfig HelmReleases when tenant has no etcd DataStore in hack/admin-kubeconfig-invariant.bats asserts the same across all 17 HelmRelease templates and passes today.
| {{- end }} | ||
|
|
||
| {{- if .Values.addons.ingressNginx.enabled }} | ||
| {{- if and .Values.addons.ingressNginx.enabled .Values._namespace.etcd }} |
There was a problem hiding this comment.
The values block at lines 1-21 is not guarded by the _namespace.etcd check, which will result in invalid YAML output when etcd is missing. Please wrap the entire file content in the check to maintain a clean manifest during the bootstrap phase.
There was a problem hiding this comment.
Per the Helm docs on named templates: "a define does not produce output unless it is called with a template" (https://helm.sh/docs/chart_template_guide/named_templates/). The define block at the top of this file registers a named template; its only call site is the include inside the HelmRelease spec, which is already gated on .Values._namespace.etcd. Verified on the chart:
helm template test packages/apps/kubernetes \
--values packages/apps/kubernetes/tests/values-ci-no-etcd.yaml \
--show-only templates/helmreleases/ingress-nginx.yaml
# Error: could not find template templates/helmreleases/ingress-nginx.yaml in chart
Zero documents on the output. The chart-wide invariant chart emits zero admin-kubeconfig HelmReleases when tenant has no etcd DataStore in hack/admin-kubeconfig-invariant.bats asserts the same across all 17 HelmRelease templates and passes today.
| {{- end }} | ||
|
|
||
| {{- if .Values.addons.monitoringAgents.enabled }} | ||
| {{- if and .Values.addons.monitoringAgents.enabled .Values._namespace.etcd }} |
There was a problem hiding this comment.
The values block at lines 1-25 is not guarded by the _namespace.etcd check. This will cause the template to output invalid YAML when etcd is missing. Consider gating the whole file to avoid polluting the release with non-resource YAML fragments.
There was a problem hiding this comment.
Per the Helm docs on named templates: "a define does not produce output unless it is called with a template" (https://helm.sh/docs/chart_template_guide/named_templates/). The define block at the top of this file registers a named template; its only call site is the include inside the HelmRelease spec, which is already gated on .Values._namespace.etcd. Verified on the chart:
helm template test packages/apps/kubernetes \
--values packages/apps/kubernetes/tests/values-ci-no-etcd.yaml \
--show-only templates/helmreleases/vertical-pod-autoscaler.yaml
# Error: could not find template templates/helmreleases/vertical-pod-autoscaler.yaml in chart
Zero documents on the output. The chart-wide invariant chart emits zero admin-kubeconfig HelmReleases when tenant has no etcd DataStore in hack/admin-kubeconfig-invariant.bats asserts the same across all 17 HelmRelease templates and passes today.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Makefile (1)
1-1:⚠️ Potential issue | 🟡 MinorAdd
go-unit-teststo.PHONY.The new target isn't listed in the
.PHONYdeclaration on line 1, unlikehelm-unit-tests/bats-unit-tests. If a file or directory namedgo-unit-testsever appears in the repo root the recipe will silently be skipped as up-to-date.🔧 Suggested fix
-.PHONY: manifests assets unit-tests helm-unit-tests bats-unit-tests preflight +.PHONY: manifests assets unit-tests helm-unit-tests bats-unit-tests go-unit-tests preflightAlso applies to: 95-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 1, Add the new make target name go-unit-tests to the .PHONY declaration(s) so the recipe always runs even if a file/dir named go-unit-tests exists; update the .PHONY line(s) that currently list manifests assets unit-tests helm-unit-tests bats-unit-tests preflight (and any other .PHONY declarations around lines where .PHONY appears) to include go-unit-tests.
🧹 Nitpick comments (6)
hack/e2e-apps/remediation-guard.sh (1)
35-35: Consider shortgrepflags for portability.
grep --extended-regexp --quietrelies on GNU long options; BSDgrep(macOS default) only accepts-E -q. CI runs on Linux so this is not a blocker, but developers runninghack/remediation-guard.batson macOS would hit an "illegal option" failure. Swapping to short flags costs nothing and keeps the local dev loop portable.♻️ Proposed change
- if printf '%s\n' "${statuses}" | grep --extended-regexp --quiet '^(failed|uninstalled)$'; then + if printf '%s\n' "${statuses}" | grep -Eq '^(failed|uninstalled)$'; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/e2e-apps/remediation-guard.sh` at line 35, Replace GNU-style long grep options with short flags for portability: in the conditional using printf and grep (the line starting with "if printf '%s\n' \"${statuses}\" | grep --extended-regexp --quiet '^(failed|uninstalled)$'; then"), change grep --extended-regexp --quiet to grep -E -q so the command becomes: if printf '%s\n' "${statuses}" | grep -E -q '^(failed|uninstalled)$'; then, preserving the same regex and behavior.packages/apps/kubernetes/Makefile (1)
7-8: Optional: consider wiring into the shared package.mk test convention.The new
testtarget is fine in isolation, but since it's a top-level entrypoint for Helm unittests in this chart, consider whether it should be invoked from a shared make target (e.g., via../../../hack/package.mk) so CI discovers it uniformly with other packages' tests. Not a blocker if CI already iteratesmake -C packages/apps/kubernetes test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/apps/kubernetes/Makefile` around lines 7 - 8, The Makefile's top-level test target (target name "test") currently runs "helm unittest ." but is not wired into the repo-wide test convention; update the Makefile so the "test" target delegates to the shared package.mk test entrypoint (e.g., include or invoke "../../../hack/package.mk" or call its test target) or add a phony target that forwards to the shared make logic so CI and common tooling discover this chart's tests the same way as other packages; reference the Makefile's "test" target when making the change.packages/apps/kubernetes/images/busybox.tag (1)
1-1: Image pin looks good.Pinning
docker.io/library/busybox:1.37.0by digest is correct for the init-container use case. Ensure a renovation/update workflow exists for this tag file so the pin doesn't go stale (other*.tagfiles inpackages/apps/kubernetes/images/appear to be updated via theimage-*Make targets; busybox is pulled, not built, so it may need a separate refresh path).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/apps/kubernetes/images/busybox.tag` at line 1, The busybox tag file pins docker.io/library/busybox:1.37.0 by digest but lacks an automated refresh path; add a renovation/update workflow (e.g., extend the existing image-* Make targets or create a new image-busybox target) so packages/apps/kubernetes/images/busybox.tag is periodically updated (or regenerated) like the other *.tag files, and ensure CI/Make invokes that target and/or a Renovate/GH action to refresh the digest when upstream releases change.packages/system/kubernetes-rd/cozyrds/kubernetes.yaml (1)
5-18: LGTM — annotation value and documented coupling are consistent.
"15m"is accepted by the parser regex inpkg/config/config.goand comfortably exceeds the 10m wait-for-kubeconfig init deadline documented in the adjacent comment, preserving the intended ordering (pod CrashLoopBackOff surfaces before Flux remediation fires). The embedded note about maintaining that invariant when raising this value is valuable — consider also asserting it in the helm-unittest / bats suite so it doesn't silently drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/kubernetes-rd/cozyrds/kubernetes.yaml` around lines 5 - 18, Add a regression test that asserts the HelmRelease annotation release.cozystack.io/helm-install-timeout remains greater than the hard-coded wait-for-kubeconfig init deadline (10m) so they can't drift; implement this in the helm-unittest or bats suite by loading the rendered Kubernetes manifest and failing if the annotation value (validated against the duration regex used in pkg/config/config.go) is <= 10m, and document the test next to the coupling comment and/or in packages/apps/kubernetes/templates/_helpers.tpl for discoverability.packages/apps/kubernetes/templates/_helpers.tpl (1)
62-66: Hard-coded 600s deadline silently couples to therelease.cozystack.io/helm-install-timeoutvalue for the Kubernetes kind.The comment accurately notes the 10m deadline must stay below the HR Install.Timeout, but the number is a string literal in shell. If someone later lowers the Kubernetes kind's annotation below ~11m without touching this helper, the init will still outlast the HR timeout and the exact remediation cycle this PR fixes comes back. Consider either deriving the deadline from a chart value (e.g.
.Values.waitForKubeconfig.deadlineSeconds) so the coupling is explicit, or adding a chart invariant test that fails if someone sets the annotation below the deadline. Leaving as-is is acceptable given the loud comment, but flagging for future-proofing.Also applies to: 84-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/apps/kubernetes/templates/_helpers.tpl` around lines 62 - 66, The helper currently hard-codes the 600s shell timeout (the "600" literal) which silently couples to the release.cozystack.io/helm-install-timeout annotation; replace that literal by reading a chart value (e.g. use .Values.waitForKubeconfig.deadlineSeconds with a sensible default) in the template logic that builds the shell timeout and update any related helper/template (the same helper that emits the 600s string) to reference that value; additionally add a chart-level invariant/unit test (or a Helm chart validation) that fails if release.cozystack.io/helm-install-timeout for the Kubernetes kind is set to a value <= the configured .Values.waitForKubeconfig.deadlineSeconds so the coupling becomes explicit and guarded.hack/admin-kubeconfig-invariant.bats (1)
28-31: Optional: consider not silencinghelm templatestderr.
2>/dev/nullat lines 31, 89, 126 discards helm's diagnostic output. Bats' default errexit still fails the test on a nonzero helm exit, so correctness is fine, but when something regresses in a values file or a template you lose the actual helm error message and have to re-run locally to debug. Ateeto a log file under$tmp(or just letting stderr through) would make CI failures self-explanatory.♻️ Example tweak
helm template invariant packages/apps/kubernetes \ --namespace tenant-root \ --values "$values_file" \ - 2>/dev/null > "$tmp/rendered.yaml" + > "$tmp/rendered.yaml" 2> "$tmp/helm.stderr" || { + echo "helm template failed:" >&2 + cat "$tmp/helm.stderr" >&2 + exit 1 + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/admin-kubeconfig-invariant.bats` around lines 28 - 31, The helm template invocation currently silences stderr via `2>/dev/null` when rendering to "$tmp/rendered.yaml", which hides diagnostic errors; change this by removing the `2>/dev/null` redirection or route stderr to a log (e.g., `2> "$tmp/helm-template.log"` or `2>&1 | tee "$tmp/helm-template.log"`) so helm's error output is preserved for CI debugging while keeping the rendered manifest written to "$tmp/rendered.yaml".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/remediation-guard.bats`:
- Line 84: Rename the Bats test declaration string to accurately reflect what it
pins: change the `@test` name "installFailures extraction pins HR v2
status.history shape" to something like "status.history extraction pins HR v2
status.history shape" (or "history extraction pins HR v2 status.history shape")
so it no longer references installFailures; update the `@test` declaration text in
the test block that begins with `@test` "installFailures extraction pins HR v2
status.history shape" to the corrected name to avoid misleading grep and future
confusion.
---
Outside diff comments:
In `@Makefile`:
- Line 1: Add the new make target name go-unit-tests to the .PHONY
declaration(s) so the recipe always runs even if a file/dir named go-unit-tests
exists; update the .PHONY line(s) that currently list manifests assets
unit-tests helm-unit-tests bats-unit-tests preflight (and any other .PHONY
declarations around lines where .PHONY appears) to include go-unit-tests.
---
Nitpick comments:
In `@hack/admin-kubeconfig-invariant.bats`:
- Around line 28-31: The helm template invocation currently silences stderr via
`2>/dev/null` when rendering to "$tmp/rendered.yaml", which hides diagnostic
errors; change this by removing the `2>/dev/null` redirection or route stderr to
a log (e.g., `2> "$tmp/helm-template.log"` or `2>&1 | tee
"$tmp/helm-template.log"`) so helm's error output is preserved for CI debugging
while keeping the rendered manifest written to "$tmp/rendered.yaml".
In `@hack/e2e-apps/remediation-guard.sh`:
- Line 35: Replace GNU-style long grep options with short flags for portability:
in the conditional using printf and grep (the line starting with "if printf
'%s\n' \"${statuses}\" | grep --extended-regexp --quiet
'^(failed|uninstalled)$'; then"), change grep --extended-regexp --quiet to grep
-E -q so the command becomes: if printf '%s\n' "${statuses}" | grep -E -q
'^(failed|uninstalled)$'; then, preserving the same regex and behavior.
In `@packages/apps/kubernetes/images/busybox.tag`:
- Line 1: The busybox tag file pins docker.io/library/busybox:1.37.0 by digest
but lacks an automated refresh path; add a renovation/update workflow (e.g.,
extend the existing image-* Make targets or create a new image-busybox target)
so packages/apps/kubernetes/images/busybox.tag is periodically updated (or
regenerated) like the other *.tag files, and ensure CI/Make invokes that target
and/or a Renovate/GH action to refresh the digest when upstream releases change.
In `@packages/apps/kubernetes/Makefile`:
- Around line 7-8: The Makefile's top-level test target (target name "test")
currently runs "helm unittest ." but is not wired into the repo-wide test
convention; update the Makefile so the "test" target delegates to the shared
package.mk test entrypoint (e.g., include or invoke "../../../hack/package.mk"
or call its test target) or add a phony target that forwards to the shared make
logic so CI and common tooling discover this chart's tests the same way as other
packages; reference the Makefile's "test" target when making the change.
In `@packages/apps/kubernetes/templates/_helpers.tpl`:
- Around line 62-66: The helper currently hard-codes the 600s shell timeout (the
"600" literal) which silently couples to the
release.cozystack.io/helm-install-timeout annotation; replace that literal by
reading a chart value (e.g. use .Values.waitForKubeconfig.deadlineSeconds with a
sensible default) in the template logic that builds the shell timeout and update
any related helper/template (the same helper that emits the 600s string) to
reference that value; additionally add a chart-level invariant/unit test (or a
Helm chart validation) that fails if release.cozystack.io/helm-install-timeout
for the Kubernetes kind is set to a value <= the configured
.Values.waitForKubeconfig.deadlineSeconds so the coupling becomes explicit and
guarded.
In `@packages/system/kubernetes-rd/cozyrds/kubernetes.yaml`:
- Around line 5-18: Add a regression test that asserts the HelmRelease
annotation release.cozystack.io/helm-install-timeout remains greater than the
hard-coded wait-for-kubeconfig init deadline (10m) so they can't drift;
implement this in the helm-unittest or bats suite by loading the rendered
Kubernetes manifest and failing if the annotation value (validated against the
duration regex used in pkg/config/config.go) is <= 10m, and document the test
next to the coupling comment and/or in
packages/apps/kubernetes/templates/_helpers.tpl for discoverability.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3c607b3c-bb29-4e90-8ce3-5bf917da9d67
📒 Files selected for processing (38)
Makefilehack/admin-kubeconfig-invariant.batshack/e2e-apps/remediation-guard.shhack/e2e-apps/run-kubernetes.shhack/remediation-guard.batspackages/apps/kubernetes/Makefilepackages/apps/kubernetes/images/busybox.tagpackages/apps/kubernetes/templates/_helpers.tplpackages/apps/kubernetes/templates/cluster-autoscaler/deployment.yamlpackages/apps/kubernetes/templates/cluster.yamlpackages/apps/kubernetes/templates/csi/deploy.yamlpackages/apps/kubernetes/templates/helmreleases/cert-manager-crds.yamlpackages/apps/kubernetes/templates/helmreleases/cert-manager.yamlpackages/apps/kubernetes/templates/helmreleases/cilium.yamlpackages/apps/kubernetes/templates/helmreleases/coredns.yamlpackages/apps/kubernetes/templates/helmreleases/csi.yamlpackages/apps/kubernetes/templates/helmreleases/fluxcd.yamlpackages/apps/kubernetes/templates/helmreleases/gateway-api-crds.yamlpackages/apps/kubernetes/templates/helmreleases/gpu-operator.yamlpackages/apps/kubernetes/templates/helmreleases/ingress-nginx.yamlpackages/apps/kubernetes/templates/helmreleases/metrics-server.yamlpackages/apps/kubernetes/templates/helmreleases/monitoring-agents.yamlpackages/apps/kubernetes/templates/helmreleases/prometheus-operator-crds.yamlpackages/apps/kubernetes/templates/helmreleases/velero.yamlpackages/apps/kubernetes/templates/helmreleases/vertical-pod-autoscaler-crds.yamlpackages/apps/kubernetes/templates/helmreleases/vertical-pod-autoscaler.yamlpackages/apps/kubernetes/templates/helmreleases/victoria-metrics-operator.yamlpackages/apps/kubernetes/templates/helmreleases/volumesnapshot-crd.yamlpackages/apps/kubernetes/templates/kccm/manager.yamlpackages/apps/kubernetes/tests/admin_kubeconfig_wait_test.yamlpackages/apps/kubernetes/tests/values-ci-no-etcd.yamlpackages/apps/kubernetes/tests/values-ci.yamlpackages/system/kubernetes-rd/cozyrds/kubernetes.yamlpkg/cmd/server/start.gopkg/config/config.gopkg/config/config_test.gopkg/registry/apps/application/rest.gopkg/registry/apps/application/rest_timeout_test.go
The test body asserts .status.history[].status extraction, but the test name still referenced the old installFailures counter (leftover from when the guard used that field before switching to status.history to avoid ClearFailures zeroing the counters on successful reconcile). Address review feedback from coderabbitai on hack/remediation-guard.bats:84: rename so grep for what the test actually pins matches the test name. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
There was a problem hiding this comment.
dockerhub may be ratelimited for downloads and no override parameter exists, need to be changed to configurable or more reliable source.
There was a problem hiding this comment.
The pin is intentional and documented at templates/_helpers.tpl:68-72: a digest-pinned, one-shot tiny image used only by the wait-for-kubeconfig init container. Making the image source configurable would mean threading a new value through the chart's globals, the helper, and the tests — exactly the kind of expansion this PR is trying to avoid. It's a stability patch for a known cold-bootstrap race, not a redesign of how this chart sources images.
Pull rate limits are a fair concern in principle, but the workload here is one container start per tenant Kubernetes bootstrap, with kubelet caching the digest after the first pull. If we ever observe this actually rate-limiting in the field, the override can be added then — until that happens the digest pin keeps the pull immutable at minimal complexity cost.
There was a problem hiding this comment.
Reconsidering after re-reading. The pin guarantees immutability against tag drift, but it doesn't help an air-gapped or rate-limited operator who cannot reach docker.io at all — that's the part of your concern I underweighted in my first reply.
Pushed 37ecd7b to address it:
- New optional chart value
images.waitForKubeconfig(defaults to empty). - Helper falls back to
images/busybox.tagwhen the value is empty, so the existing digest pin remains the default for everyone who doesn't override. - Operators in restricted networks set the value to any registry path kubelet can pull (mirror, internal registry, alternative image), no other plumbing required.
- Two new helm-unittest cases cover the default and the override paths.
Pin-by-default stays, override path is now there.
| {{- if not $etcd }} | ||
| --- | ||
| apiVersion: v1 | ||
| kind: ConfigMap | ||
| metadata: | ||
| name: {{ .Release.Name }}-awaiting-etcd | ||
| namespace: {{ .Release.Namespace }} | ||
| labels: | ||
| app.kubernetes.io/managed-by: {{ .Release.Service }} | ||
| app.kubernetes.io/name: kubernetes | ||
| app.kubernetes.io/instance: {{ .Release.Name }} | ||
| annotations: | ||
| cozystack.io/status-beacon: "true" | ||
| data: | ||
| status: "awaiting-etcd" | ||
| message: | | ||
| No DataStore is available for this tenant Kubernetes cluster. The parent | ||
| Tenant application has not populated _namespace.etcd. Set spec.etcd: true | ||
| on an ancestor Tenant (usually tenant-root) and wait for its HelmRelease | ||
| to reconcile - this HelmRelease will pick up the DataStore on its next | ||
| 5m reconcile loop and provision the cluster. | ||
| {{- else }} |
There was a problem hiding this comment.
who will see this message? what is cozystack.io/status-beacon: "true" annottion? i think this needs to be some validating webook, not bunch of ifs in templates of the chart
There was a problem hiding this comment.
The beacon is rendered as a ConfigMap with cozystack.io/status-beacon: "true" so the HelmRelease can reach Ready while a tenant Kubernetes is still waiting for _namespace.etcd to be populated by the parent Tenant chart. Visibility today is via kubectl get cm -n <ns> -l cozystack.io/status-beacon=true and via the HelmRelease's manifest list — concretely it gives the operator something to look at when a Kubernetes tenant has been "created but nothing is happening" for several minutes, instead of the previous behaviour of an install failing on a missing DataStore and triggering the remediation cycle from #2412.
The annotation itself is a forward-looking marker — nothing consumes it yet. Naming it now lets a future status surface (controller field, dashboard query, whatever shape it takes) discriminate "intentional signalling" from real workload ConfigMaps without name-pattern matching.
A validating webhook would be the cleaner long-term answer, but it's a whole new component to deploy, certify, and operate. The scope here is closing a remediation-loop bug at minimum cost, not introducing a new admission surface. The if not $etcd branch is one template fork in one chart — when there's appetite for a webhook we can replace this with one and the beacon becomes its public contract.
There was a problem hiding this comment.
remember that this future plans live only in your mind, and when this change will be accepted to main, it will be treated as source of truth, so other contributors may start building new logic like this. Document your intentions or don't add unplanned features yet.
There was a problem hiding this comment.
Agreed — the annotation had no consumer in the chart and no documented contract. Pushed fb1ef59 to drop it.
The ConfigMap itself stays: <release>-awaiting-etcd with data.status: "awaiting-etcd" and the human-readable message in data.message. Operators discover it via kubectl get cm -n <ns> and read the message; no new annotation, no new convention.
The validating-webhook alternative you raised is fair as a longer-term shape — the soft-skip path was chosen because the etcd-missing window is recoverable (the parent Tenant chart populates _namespace.etcd on its own reconcile and we want the kubernetes HR to pick it up without operator intervention), but a webhook that rejects up-front is defensible if the soft-skip semantics turn out to confuse operators in practice. Happy to file that as a separate issue if you'd like.
| // Convert to ResourceConfig | ||
| o.ResourceConfig = &config.ResourceConfig{} | ||
| for _, crd := range crdList.Items { | ||
| release := config.ReleaseConfig{ |
There was a problem hiding this comment.
what applications may benefit from this new setting except of kubernetes app? looks like extra timeout is just workaround, and core problem of ordering resource creation in complex apps like kubernetes is not solved.
There was a problem hiding this comment.
Yes, deliberately a workaround. The PR body calls this out explicitly: Option 2 from #2412 (a separate HelmRelease with dependsOn, restructuring the chart's HelmRelease topology) was intentionally not taken because the cost — splitting the chart, building the dependency graph, dealing with separate reconciliation domains — is far higher than the bug it fixes. The per-app timeout annotation is the smallest knob that lets the kubernetes-rd kind survive Kamaji cold start without flux uninstalling the Cluster CR mid-bootstrap.
Today only kubernetes-rd sets it (15m); every other ApplicationDefinition leaves it unset and flux defaults apply, so blast radius is one app. The same shape would fit any future app whose parent chart hard-mounts an asynchronously-provisioned Secret/ConfigMap (CNPG-backed apps come to mind), but those would land in their own PRs with their own justification — this one doesn't widen the contract.
There was a problem hiding this comment.
workaround closes only 1 case when something is not as fast deployable as we expected, and it will be deployed just a little later. However most problems in my opinion go from etcd issues (disabling tenant etcd), kamaji resource throttling, or for CNPG clusters it may be storage shortage, increasing timeouts for this just adds time to realization that something broken.
There was a problem hiding this comment.
Splitting the failure modes apart, because they each end up in different places:
- Etcd issues — not affected by this knob. When
_namespace.etcdis empty the chart soft-skips every workload (separate change in this same PR), no install runs at all, the 15m never applies. Detection is the operator seeing the<release>-awaiting-etcdConfigMap andkubectl get hrReady=true with no children, not a timeout. - Kamaji throttling / pod Pending — stays visible. The
wait-for-kubeconfiginit container has its own 10m deadline strictly below the HelmRelease's 15m, so a stuck Kamaji surfaces as CrashLoopBackOff on the CP-side Deployments at 10m. Samekubectl get podssignal as today, just on a Pod instead of an HR. - CNPG storage shortage — different HelmRelease, different ApplicationDefinition. The kubernetes-rd timeout annotation does not apply.
The bug being closed is narrower than "slow startup". cozystack-api hardcodes Install.Remediation.Retries: -1 for every Application (pkg/registry/apps/application/rest.go:1513), and helm-controller's default Install.Timeout is 5m. Kamaji cold-start routinely exceeds 5m on a fresh tenant. That combination doesn't just mean "user waits longer to see failure" — it means flux uninstalls the Kamaji Cluster CR every 5m and reinstalls it, never converging. The 15m breaks that destructive loop. Time-to-fail is now 10m on a Pod via the init container, where it was previously "never, in a remediation cycle".
You're right that today only kubernetes-rd consumes the annotation. The mechanism is generic by construction, not by speculation: any future Application kind whose parent HR contains an asynchronously-provisioned dependency opts in by setting the same annotation in its ApplicationDefinition, no code change required. Single consumer today, no extra plumbing for the next one.
…gapped registries Operators in air-gapped or rate-limited environments cannot reach docker.io and the bundled busybox digest pin gives them no escape hatch. Add an optional images.waitForKubeconfig chart value that, when set, replaces the helper's image reference with any registry path kubelet can pull. Empty value falls back to images/busybox.tag, so the prior digest-pinned default is preserved. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
…ting-etcd ConfigMap The cozystack.io/status-beacon: "true" annotation had no consumer in the chart, no documented contract, and no convention defined for other charts to follow. It would have become accidental precedent for contributors copying the pattern without understanding it. The ConfigMap itself is self-explanatory: the name <release>-awaiting-etcd, data.status: "awaiting-etcd", and the human-readable message in data.message all surface the same operator signal via kubectl get cm. Drop the annotation; keep the ConfigMap. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
github-actions
Bot
added
area/kubernetes
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/apps/kubernetes/tests/admin_kubeconfig_wait_test.yaml (1)
30-32: Consider refactoring initContainer assertions to use name filters for improved resilience.The volume assertions throughout this suite use
[?(@.name=="kubeconfig")]filters (lines 18, 21, 47, 50, 76, 79), but every initContainer assertion relies on positional indexing. If a future chart change prepends another init container, all six tests will silently point to the wrong container instead of failing on the actual breakage.Replace
initContainers[0]withinitContainers[?(@.name=="wait-for-kubeconfig")]in all assertions to maintain consistency with the volume filter pattern:Affected assertions
- Lines 30–32, 59–61, 88–90: name equality checks
- Lines 34, 63, 92: volumeMounts containment checks
- Lines 105–106, 118–119: image assertions
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/apps/kubernetes/tests/admin_kubeconfig_wait_test.yaml` around lines 30 - 32, The initContainer assertions currently use positional indexing (initContainers[0]) which is brittle; update every assertion that references initContainers[0] to use a name filter selector initContainers[?(@.name=="wait-for-kubeconfig")] instead (this applies to the name equality checks, the volumeMounts containment checks, and the image assertions referring to the wait-for-kubeconfig container), ensuring all occurrences (name, volumeMounts, image) target the container by name rather than by index to match the existing volume filter pattern and avoid silent breakage if containers are reordered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/apps/kubernetes/templates/cluster.yaml`:
- Around line 111-116: The operator-facing message in
packages/apps/kubernetes/templates/cluster.yaml hardcodes "5m reconcile loop";
update the message to avoid a stale cadence by either removing the specific
interval text or referencing a single chart/templated value (e.g., use a Helm
value such as .Values.releaseInterval or a chart global that mirrors the parent
Application's spec.interval or the release.cozystack.io/helm-install-timeout
knob) instead of "5m"; edit the message block (the YAML message: | string) to
use the chosen approach and ensure wording remains clear to operators about
waiting for the parent HelmRelease to reconcile.
---
Nitpick comments:
In `@packages/apps/kubernetes/tests/admin_kubeconfig_wait_test.yaml`:
- Around line 30-32: The initContainer assertions currently use positional
indexing (initContainers[0]) which is brittle; update every assertion that
references initContainers[0] to use a name filter selector
initContainers[?(@.name=="wait-for-kubeconfig")] instead (this applies to the
name equality checks, the volumeMounts containment checks, and the image
assertions referring to the wait-for-kubeconfig container), ensuring all
occurrences (name, volumeMounts, image) target the container by name rather than
by index to match the existing volume filter pattern and avoid silent breakage
if containers are reordered.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d7a0885c-e3ed-4b34-832c-5725e3e7d9ed
📒 Files selected for processing (8)
api/apps/v1alpha1/kubernetes/types.gopackages/apps/kubernetes/README.mdpackages/apps/kubernetes/templates/_helpers.tplpackages/apps/kubernetes/templates/cluster.yamlpackages/apps/kubernetes/tests/admin_kubeconfig_wait_test.yamlpackages/apps/kubernetes/values.schema.jsonpackages/apps/kubernetes/values.yamlpackages/system/kubernetes-rd/cozyrds/kubernetes.yaml
✅ Files skipped from review due to trivial changes (1)
- packages/apps/kubernetes/values.schema.json
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/apps/kubernetes/templates/_helpers.tpl
- packages/system/kubernetes-rd/cozyrds/kubernetes.yaml
| message: | | ||
| No DataStore is available for this tenant Kubernetes cluster. The parent | ||
| Tenant application has not populated _namespace.etcd. Set spec.etcd: true | ||
| on an ancestor Tenant (usually tenant-root) and wait for its HelmRelease | ||
| to reconcile - this HelmRelease will pick up the DataStore on its next | ||
| 5m reconcile loop and provision the cluster. |
There was a problem hiding this comment.
Minor: "5m reconcile loop" is hardcoded in the operator-facing message.
The actual HelmRelease spec.interval on the parent Application is configured elsewhere (and per PR objectives this PR even adds a release.cozystack.io/helm-install-timeout knob driving Install.Timeout). If the interval is ever tuned, this beacon will quietly lie to the operator. Consider either dropping the specific cadence or sourcing it from a single chart value to avoid drift.
📝 Suggested wording
- No DataStore is available for this tenant Kubernetes cluster. The parent
- Tenant application has not populated _namespace.etcd. Set spec.etcd: true
- on an ancestor Tenant (usually tenant-root) and wait for its HelmRelease
- to reconcile - this HelmRelease will pick up the DataStore on its next
- 5m reconcile loop and provision the cluster.
+ No DataStore is available for this tenant Kubernetes cluster. The parent
+ Tenant application has not populated _namespace.etcd. Set spec.etcd: true
+ on an ancestor Tenant (usually tenant-root) and wait for its HelmRelease
+ to reconcile - this HelmRelease will pick up the DataStore on its next
+ Flux reconcile interval and provision the cluster.📝 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.
| message: | | |
| No DataStore is available for this tenant Kubernetes cluster. The parent | |
| Tenant application has not populated _namespace.etcd. Set spec.etcd: true | |
| on an ancestor Tenant (usually tenant-root) and wait for its HelmRelease | |
| to reconcile - this HelmRelease will pick up the DataStore on its next | |
| 5m reconcile loop and provision the cluster. | |
| message: | | |
| No DataStore is available for this tenant Kubernetes cluster. The parent | |
| Tenant application has not populated _namespace.etcd. Set spec.etcd: true | |
| on an ancestor Tenant (usually tenant-root) and wait for its HelmRelease | |
| to reconcile - this HelmRelease will pick up the DataStore on its next | |
| Flux reconcile interval and provision the cluster. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/apps/kubernetes/templates/cluster.yaml` around lines 111 - 116, The
operator-facing message in packages/apps/kubernetes/templates/cluster.yaml
hardcodes "5m reconcile loop"; update the message to avoid a stale cadence by
either removing the specific interval text or referencing a single
chart/templated value (e.g., use a Helm value such as .Values.releaseInterval or
a chart global that mirrors the parent Application's spec.interval or the
release.cozystack.io/helm-install-timeout knob) instead of "5m"; edit the
message block (the YAML message: | string) to use the chosen approach and ensure
wording remains clear to operators about waiting for the parent HelmRelease to
reconcile.
3 participants
What this PR does
Closes #2412. On a cold tenant-Kubernetes bootstrap, the parent HelmRelease raced the admin-kubeconfig Secret that Kamaji provisions asynchronously. Three CP-side Deployments (cluster-autoscaler, kccm, kcsi-controller) mounted that Secret as a hard volume, flux helm-controller's default wait budget was too short for Kamaji cold start, and
install.remediation { retries: -1 }then uninstalled the Cluster CR and restarted the cycle forever.Implements a defense-in-depth fix:
optional: trueon the admin-kubeconfig Secret volume in all three Deployments so kubelet no longer FailedMounts while Kamaji is still bootstrapping.wait-for-kubeconfiginit container (intemplates/_helpers.tpl) that polls forsuper-admin.svcwith a 10m deadline, strictly below the HelmRelease Install.Timeout so a broken tenant falls into CrashLoopBackOff visibly instead of hanging forever.release.cozystack.io/helm-install-timeoutannotation on ApplicationDefinition. Kubernetes-rd sets it to15m; other kinds leave it unset and keep flux defaults, so their failed installs remediate on the normal cadence. Parser rejects ns/us/µs (accepted bytime.ParseDuration, rejected by Flux's CRD pattern) at startup._namespace.etcdis empty: the CP-side Deployments, the Cluster/KamajiControlPlane/KubevirtCluster/WorkloadMonitor CRs, and every child HelmRelease that references admin-kubeconfig now render only when an etcd DataStore exists for this tenant. Anawaiting-etcdConfigMap is emitted as a user-visible status beacon sohelm installstill succeeds and flux retries on its 5m interval until the Tenant chart catches up..status.history[].status(the Snapshot shape), not on.status.installFailures-ClearFailures()zeroes the latter on every successful reconciliation, which made the previous guard vacuous.Tests:
All wired into the existing
make unit-teststarget (go-unit-testsadded alongsidehelm-unit-testsandbats-unit-tests).Option 2 from the ticket (separate HelmRelease with
dependsOn) was intentionally not taken: the combination above closes the same race without restructuring the chart's HelmRelease topology.Release note
Summary by CodeRabbit
New Features
Tests
Chores