feat(flux): kstatus health gates on CR-wrapping umbrella HelmReleases - #3264

Verdict

NOT LGTM

feat(flux): kstatus health gates on CR-wrapping umbrella HelmReleases. Two MAJOR issues: the failed predicates for CNPG Cluster and EtcdCluster are dead code, so genuinely-broken dependencies are classified inProgress forever instead of failed. (Noted: this PR is a draft with an open self-checklist — flagging early.)

Findings

[MAJOR] packages/apps/harbor/templates/harbor.yaml:112 and packages/extra/monitoring/templates/helmrelease.yaml:91 — the failed CEL predicate for the CNPG Cluster is dead code. The condition e.reason != 'ClusterIsNotReady' is unreachable: in the pinned CloudNativePG 1.27.1 (SetClusterReadyCondition, transactions.go:34-56), Ready=False for ANY non-healthy phase always carries reason: ClusterIsNotReady — there is no second reason; the differentiated cause goes into status.phase, not reason. So a genuinely-broken Postgres (harbor-db, alerta-db, grafana-db) is permanently classified inProgress, never failed; the only backstop is timeout + remediation.retries: -1 (infinite silent retry). The same bug is already in production in seaweedfs-db.yaml:33-34; this PR triples the blast radius. Match on status.phase (or drop the reason discriminator) instead.

[MAJOR] packages/apps/tenant/templates/etcd.yaml:50 — the failed predicate for EtcdCluster (status.brokenMembers > 0) is dead for the default PVC-backed storage. Confirmed even by the vendored CRD (etcdclusters.yaml:1859-1860: "While the auto-replacement predicate is a stub it is always 0") and upstream etcd-operator v0.5.2 (isBroken returns false except for StorageMediumMemory). A tenant etcd losing quorum stays inProgress forever, even though the operator already writes a legible Degraded=True, reason=QuorumLost in an adjacent field. Gate on that condition instead.

[MINOR] PR body is missing the required ```release-note block (.github/PULL_REQUEST_TEMPLATE.md).

Caveats

• CI is inconclusive on the new logic: the single E2E run failed on an unrelated dependency (grafana-operator dashboards sidecar) and never reached the code this PR touches — so the new health-check logic was neither confirmed nor refuted by CI.
• The other four new/extended predicates (VM* updateStatus, Grafana stage/stageStatus, TenantGateway Ready) were checked against upstream and are correct.

NOT LGTM. The new Grafana readiness hook wedges the monitoring install, and CI reproduces it on this head.

Business context: an umbrella HelmRelease wrapping a custom resource flips Ready as soon as Helm applies the CR, so dependants start before the backing workload is usable. This gates those releases on the CR's own readiness.

Both major findings from the earlier review are fixed. I checked them against the pinned operators, not against the diff.

The CNPG failed predicate is gone, and harbor and monitoring now declare current only. The EtcdCluster gate no longer keys on brokenMembers; it matches Available=False with reason BootstrapFailed or DeadlineExceeded, pinned to metadata.generation. In the operator at the pinned appVersion v0.5.5 both reasons are written, and Progressing=False with reason Reconciled lands in the same status update as Available=True, so current is reachable. Worth knowing that EtcdClusterStatus has no root-level observedGeneration, so the per-condition generation clauses are the only staleness guard the evaluator gets here. Dropping them would silently remove it.

Data safety on the gated Postgres releases holds. Harbor, the tenant etcd child and monitoring all set RetryOnFailure on install and upgrade, so a gate that never goes green times out and retries in place instead of falling through to uninstall remediation, which is the path that deletes the CNPG Cluster and its PVCs.

Blockers

B1: the Grafana readiness hook never completes and blocks the install

packages/system/monitoring/templates/grafana/oidc-users-job.yaml. The Job now renders in every mode, not only when spec.oidc.users is non-empty, and it holds the release until it can read the Grafana CR and its Deployment. In the run on this head it never read either one.

The run for 16e119dba fails at the install step. Every other branch failing in that window fails at the test step instead, so this is not the fleet-wide red. The cluster report has the chain: monitoring-system reports Helm install failed ... failed post-install: timeout waiting for: [Job/tenant-root/monitoring-system-oidc-users status: 'InProgress'], then monitoring reports timeout waiting for: [HelmRelease/tenant-root/monitoring-system status: 'InProgress'], then tenant-root reports timeout waiting for: [HelmRelease/tenant-root/monitoring status: 'InProgress'].

The Job log is 114 identical lines of Cannot read Grafana or its Deployment; retrying named API GETs from 19:04:17, ending in Grafana did not reach the desired healthy rollout within 10m; aborting at 19:14:21. It never got past the two kubectl get --raw calls, so it never evaluated readiness once.

Grafana was already healthy before the Job started. grafana-deployment hit readyReplicas: 2, availableReplicas: 2, updatedReplicas: 2, observedGeneration: 1 at 19:02:51, carrying monitoring.cozystack.io/grafana-spec-hash: 7652101d... matching the CR. The hash propagation works: the operator copies the annotation to the Deployment and the ReplicaSet. Everything the gate checks was true 90 seconds before the Job started.

The rendered Job has no GF_ADMIN_USER, OIDC_MODE or DESIRED_USERS_JSON env, so $manageUsers was false, and the old if and (ne $mode "None") (gt (len $users) 0) guard would have emitted nothing. This configuration gained a blocking post-install hook it did not have before.

I could not work out why the reads failed, and B2 is why. RBAC is shaped right and was created at 19:00:55, the ServiceAccount exists, the pod carries policy.cozystack.io/allow-to-apiserver and the matching policy is valid. I considered that label newly switching on egress enforcement and ruled it out: allow-internal-communication and allow-to-dns both use an empty endpointSelector, so every pod in the namespace was already under egress policy and already had DNS. Treating it as unrelated.

Since this is a post-install hook timeout under RetryOnFailure, it retries indefinitely rather than surfacing, and every dependant waits behind it.

B2: the hook throws away the error from every API read

Same file. Both kubectl get --raw calls send stderr to /dev/null, and so does the jq after them. A permission denial, an unreachable API server, a missing object and a Grafana that is genuinely still rolling out all produce the same one-line message.

Ten minutes of one repeated line, no status code, no API error, nothing separating "not ready yet" from "cannot reach the API at all". That is the exact distinction this gate exists to make. It is also why B1 cannot be root-caused from artifacts the run already collected. Keep stderr and log it, at least on the final attempt before aborting, so the abort message carries the real failure.

B3: the PR description credits an authoring tool

The body ends with a Generated with [Claude Code] line. Contributor guidance forbids naming an AI model or its vendor in an authorship line in a PR description. The commits get this right: all five carry a bare Assisted-By: LLM trailer and a sign-off. Drop the line from the body.

Non-blocking

1. The description drifted. The table still says monitoring extends the gate to Grafana on stage/stageStatus, but the rendered HelmRelease has six expressions and none is a Grafana; readiness moved to the rollout hook. The body also never mentions the health-check test module, the readiness RBAC, or the new make target, which is most of the diff.
2. The release-note block the PR template asks for is still missing, as the earlier review said.
3. One clause of the etcd current expression is unpinned. Changing status.readyMembers == spec.replicas to <= leaves the suite green, because no fixture pairs Available=True/QuorumHealthy and Progressing=False/Reconciled with partial membership. quorum-available-but-one-member-unhealthy has no Progressing condition, so a different clause holds it. The rest is pinned properly: renaming the Reconciled reason reddens healthy and single-member-healthy, which is how I checked.
4. The VMAgent expression matches nothing in tenant-root or cozy-monitoring, since templates/vm/vmagent.yaml is guarded not to render there. An expression matching no object in the release is ignored without complaint, so the gate covers one resource fewer in those namespaces than the list reads. A comment would help if that is deliberate.

Rendering the real charts and running the shipped CEL evaluator against status fixtures is the right shape for this, and I confirmed by mutation that both it and the chart tests fail when the rendered values change.