Connecting a marketplace repository materialized its PackageSource but created no Package, so its ApplicationDefinitions never rendered and its apps never appeared in the dashboard catalog until a manual `cozypkg add` the dashboard cannot drive (the package list is computed from the ApplicationDefinitions, which do not exist yet).
The tap materializer now creates a tap-managed registration Package for each materialized PackageSource, the way the platform ships a Package per built-in app, so the ApplicationDefinition registrations deploy on connect and the apps become browsable; the app components carry no install block and are still instantiated per user resource.
Privileged install components of an unconfirmed tap auto-registration (a Package that still carries the marketplace-tap label) are refused on several best-effort layers: the Package reconciler skips their HelmRelease and prunes a lingering one at the install site, and the PackageSource reconciler withholds their ExternalArtifact so the helm-controller has no privileged content to deploy, re-running on confirmation via a Package watch. `cozypkg add --allow-privileged` sheds the label as an explicit ownership handover, which is the confirmation; a platform Package never carries the label. A benign component flipped to privileged in a new revision can still be materialized by source-watcher before these layers observe the flip; closing that race fully requires decoupling the catalog registration from install and is tracked in issue #4359.
`cozypkg untap` and the dashboard disconnect remove the registration Package (de-registering the apps) under a UID + ResourceVersion precondition, so a concurrent `cozypkg add` handover is never clobbered; running instances are left but unmanaged. `cozypkg add` re-opens a tap auto-registration for variant selection and hands it over. The cozystack-api ClusterRole gains delete on packages for the disconnect path.
Assisted-by: LLM
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
## What this PR does
Adds automatic horizontal autoscaling of a PostgreSQL database's **read
replicas** in response to load, implementing the merged design proposal
[Database Horizontal
Autoscaler](https://github.com/cozystack/community/tree/main/design-proposals/database-horizontal-autoscaling)
(community#44). The mechanism is entirely stock — no bespoke operator,
no new CRD:
- **KEDA as an optional platform component** (`packages/system/keda`,
wired into `core/platform` as a source + system-bundle entry). It claims
the `external.metrics.k8s.io` APIService that nothing served before.
- **`cozy-lib.keda.scaledObject` helper**
(`packages/library/cozy-lib/templates/_keda.tpl`) renders a KEDA
`ScaledObject`, fail-closed on missing/invalid arguments.
- **`packages/apps/postgres` autoscaling** — an `autoscaling` values
block (`enabled`, `minReplicas`, `maxReplicas`, `metric`, `target`,
`maxReplicationLagSeconds`, `dryRun`, `transition`) validated by
`values.schema.json`; under active autoscaling the chart renders a
`ScaledObject` and seeds `spec.instances` to a constant floor
(`max(replicas, effectiveMin)`), while the postgres release is forced to
client-side (two-way) Helm apply so the seed is a merge no-op on upgrade
and the KEDA-managed HPA — the runtime writer via the CNPG `Cluster`
scale subresource — is not reverted.
- **Single-value metric (§1)** — one PromQL query returns `Σ(read load
over standbys) + target`, so a stock HPA computes `desired =
ceil((Σ+target)/target) = 1 + ceil(Σ/target) = primary + desiredRead`,
with no per-pod emission.
- **The four static brakes (§5)** — quorum floor `max(minReplicas,
maxSyncReplicas+1, 2)` (quorum wins over `maxReplicas`), scale-down
pacing (`Pods/1/600s`), a replication-lag clamp folded into the query
(freeze both directions while lag exceeds the threshold and the primary
is writing, with `max_over_time` hysteresis), and dry-run (render
paused).
- **Two-phase enable/disable migration** via a `transition` sub-flag:
phase 1 stages `spec.instances` to the live count and brings KEDA up
with its HPA paused in both directions; flipping to phase 2 hands the
runtime count to KEDA, and because the seed re-renders the same count
the flip is a merge no-op with no dip.
- **Observability** — a database-autoscaling Grafana dashboard and
PrometheusRule alerts.
**Live validation (dev cluster, CNPG 1.30.0).** The full path works
end-to-end: the CNPG `Cluster` scale subresource serves
`status.selector`, KEDA renders the HPA, the metric reaches it
(`ScalingActive=ValidMetricFound`, `TARGETS 151/150`, `desiredReplicas=2
= 1 primary + 1 replica` — the §1 arithmetic), and the lag-clamp query
returns the base value on live VictoriaMetrics. Two packaging fixes were
needed and are included: KEDA must run on the cozystack `cozy.local`
cluster domain (the upstream `cluster.local` default makes the metrics
gRPC target NXDOMAIN and every HPA hangs on `FailedGetExternalMetric`),
and the operator's client-go throttle is raised so a single-replica
operator does not lose its leader lease under scale-subresource polling.
Implements #3953.
### Downstream repositories
Walked the trigger map in `docs/agents/contributing.md` against the
diff. Two repositories are affected; both are follow-ups to open once
this lands (and after its dependency #3951 merges and the values surface
is final), left unticked here for a maintainer to confirm rather than
claimed with a speculative link:
- **cozystack/terraform-provider-cozystack** — this adds the nested
`autoscaling` block to `packages/apps/postgres/values.schema.json`. The
provider hand-models each app's fields (schema + model +
expand/flatten), so it needs the new block to expose it in Terraform.
- **cozystack/website** — KEDA is a new platform component
(`guides/platform-stack`), and the postgres reference page gains the
`autoscaling` params. The managed-app reference is regenerated from
`README.md` by the release bot; the platform-stack component list is
hand-maintained.
- [ ] No downstream repository is affected by this change
- [ ] [cozystack/website](https://github.com/cozystack/website) -
follow-up: to open (new platform component + postgres autoscaling
params)
- [ ]
[cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack)
- follow-up: to open (new `autoscaling` block in postgres
values.schema.json)
### Depends on
- #3951 — bump CloudNativePG to 1.30.0. The HPA can only drive the CNPG
`Cluster` scale subresource on CNPG ≥ 1.28.4, where it serves
`status.selector`; without it the HPA fails with `InvalidSelector`. This
PR should not merge before #3951.
### Release note
```release-note
feat(postgres): add optional horizontal autoscaling of read replicas via KEDA, configured through the new `autoscaling` values block; KEDA ships as a new optional platform component. Requires CloudNativePG >= 1.28.4 for the Cluster scale subresource selector. Note: every Postgres release now applies client-side (`release.cozystack.io/helm-server-side-apply: "false"`) so the autoscaler's live `/scale` count is not reverted by Helm; a kind-wide side effect is that chart-rendered fields on Postgres releases are no longer re-asserted by server-side-apply drift correction (driftDetection is unset), a documented trade-off with a per-instance apply strategy as follow-up.
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **New Features**
* Added configurable PostgreSQL read-replica autoscaling with KEDA.
* Added CPU and read-connection metrics, replica bounds, replication-lag
safeguards, dry-run mode, and transition handling.
* Added optional KEDA platform support, Grafana autoscaling dashboard,
and Prometheus alerts.
* **Documentation**
* Clarified integer thresholds, prerequisites, configuration, and
transition guidance.
* **Bug Fixes**
* Improved validation and actionable errors for unavailable KEDA or
invalid settings.
* **Tests**
* Expanded coverage for autoscaling, observability, package selection,
and validation.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
safeJoin already rejected traversal and re-checked the joined path against
destDir, but the guard sits behind a call boundary that CodeQL's go/zipslip
barrier does not follow, so the extraction loop reported three high-severity
arbitrary-write alerts. Reject the entry name up front instead, which is both
the fail-closed spelling and the shape the analysis recognises. Extend the
traversal test to cover a ".." buried mid-path, not just a leading one.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
Without the community. rename, a tapped repository's cozyrds
ApplicationDefinition references its component's artifact by the concrete
<packagesource>-<variant>-<component> name again, so the operator no longer
injects a cozystackArtifactPrefix value and the -rd chart renders its cozyrds
asset with a plain Files.Get. cozypkg init scaffolds the concrete chartRef.
Reserved name prefixes are removed: repositories choose their own PackageSource
names, and a clash with a core component is caught on-cluster at tap time (the
collision check) rather than by refusing prefixes offline.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
Tapped repositories keep their own declared PackageSource names instead of
being renamed under the community. prefix. Tap-ness is now marked by the
marketplace-tap label (and source annotation) on the OCIRepository and every
materialized PackageSource, so untap, the Flux-source cleanup, and idempotent
re-tap key on the marker rather than a name prefix.
A declared name that clashes with an existing core component (or another tap)
is rejected: the CLI tap fails fast before applying anything, and the operator
materializer refuses to overwrite the foreign object, records the reason on the
source (annotation and Event), and does not stamp the revision so a corrected
artifact retries. The shared check lives in internal/marketplace/collision.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
Fixes the rename-vs-chartRef break found on dev3: a community-tapped
PackageSource is renamed to community.<org>.<repo>, so the assembled artifact
name changes, but a repo's cozyrds ApplicationDefinition baked chartRef.name
under the author's original name and the ExternalArtifact it referenced no
longer existed, leaving every community-installed app permanently ArtifactFailed.
Implements @kvaps's option 2 (derive at the -rd component, keep platform charts
literal):
- naming.ArtifactPrefix exposes the per-(source,variant) artifact-name prefix.
- The Package reconciler injects it as the cozystackArtifactPrefix Helm value,
but ONLY for community.-prefixed sources, so platform HelmRelease values are
byte-for-byte unchanged (no churn, no platform chart edits).
- cozypkg init now scaffolds a community-style -rd chart: cozyrd.yaml renders the
cozyrds asset through tpl, and chartRef.name templates
'{{ .Values.cozystackArtifactPrefix }}-<app>', resolving to the real artifact
whatever the tap-time rename produces.
- validate skips the dangling-chartRef check for a templated name (it cannot be
cross-checked at authoring time; the operator resolves it at install).
Verified end-to-end: init -> validate is clean, and helm template with the
prefix value renders chartRef.name to the exact artifact the reconciler emits.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
From @kvaps's review: a repository author could name a PackageSource
'cozystack.<x>' or 'community.<x>' and claim the platform or marketplace
namespace. Until now validate only rejected an empty metadata.name and the
forced rename in the materializer was the sole guard.
- validate now flags 'cozystack.'- and 'community.'-prefixed PackageSource names
as an error. The escape hatch --allow-reserved-names lives on the caller side
only: the index gate (validate-entry.sh) never passes it, so a submitter
cannot hand it to themselves.
- cozypkg init refuses the same prefixes early with a neutral-name hint.
- Document why the ClusterIP fail-open is safe: it falls back only on address
resolution, and integrity stays enforced by the fail-closed digest check, so
a wrong host yields a mismatch error rather than a content substitution.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
The tap materializer fetches the Flux artifact from status.artifact.url, which is
a cluster-DNS name (flux.<ns>.svc/...). cozystack-operator runs with
hostNetwork: true and dnsPolicy: ClusterFirst, so it resolves against the host's
DNS rather than CoreDNS and that name is NXDOMAIN — every dashboard-driven
connect (API Create, which relies entirely on the materializer) then hangs in
'connecting' forever, while the CLI tap works only because it pulls via
'flux pull artifact' from the public registry.
Resolve the artifact host to the backing Service's ClusterIP (which a
hostNetwork pod reaches directly, the same way the in-cluster apiserver is
reached via KUBERNETES_SERVICE_HOST) before fetching. This fails open: a
non-cluster host, a missing Service, or a headless/empty ClusterIP falls back to
the original URL, so clusters where DNS already works are unaffected. Services
are readable through the operator's existing cluster-wide get/list/watch grant.
Found on dev3 via cozystack-pr-test; the connect->materialize path never worked
on a stock operator deployment before this.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
From @kvaps's review of the untrusted-artifact path:
- Refuse to silently retarget: two repositories that share an org/repo path on
different registry hosts derive the same tap name. The API and CLI now reject
a connect when an existing source points at a different spec.url, instead of
blindly repointing the first at the second.
- Fail closed on integrity: verifyAndExtract now errors on an absent digest
(not just a mismatch), and the reconciler waits for status.artifact.digest,
so a tap artifact is never materialized without a verified digest.
- Centralize the load-bearing community. prefix in internal/marketplace/tapconst
(removing two duplicate consts and several literals across the CLI, API,
materializer, and dashboard) so the disconnect guard, the community badge, and
the anti-shadowing guarantee cannot drift.
- Split multi-document YAML with the apimachinery reader so a PackageSource
whose values contain a separator-looking line is not fractured.
(The short-name version-pin drop was already fixed in an earlier commit.)
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
Address the remaining review findings:
- The materializer now prunes PackageSources it materialized from an earlier
revision that the current artifact no longer contains (including a single/multi
rename), so a removed package leaves the catalog; installed Packages stay.
- A repeat connect updates the fetched OCIRepository's spec, label and name
annotation instead of replacing the object, so the operator's finalizer and
materialized-revision annotation survive; covered by a test.
- OCIRepository write verbs move from the cluster-wide cozystack-api ClusterRole
to a namespaced Role in cozy-system (get/list/watch stay cluster-wide for the
watch cache), since the API only manages tap sources there.
- 'cozypkg validate --require-signature' now pulls the exact manifest digest
cosign verified, closing the tag-mutation window between verify and pull.
- safeJoin rejects any '..' path segment outright (defense in depth and to
satisfy the Zip-Slip scanner) on top of the existing containment check.
- Document why the index gate's merge/label steps need a workflow_run split for
fork pull requests.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
Address the linters flagged in review: check the previously-ignored writer,
Close, and RemoveAll return values (errcheck), lowercase an error string
(ST1005), drop the embedded-field selector (QF1008), remove an unused const,
and switch the materializer's post-finalizer requeue from the deprecated
Result.Requeue to RequeueAfter.
Also fix 'cozypkg init': capitalize now builds an upper CamelCase kind from a
hyphenated --app (foo-bar -> FooBar) so the scaffolded ApplicationDefinition is
not rejected by the apiserver at install time; covered by a test.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
Close the two non-blocking notes from the third review round:
- objectExists now fails safe: it returns 'not existing' (eligible for
rollback) only on a definite NotFound; any other Get error (RBAC, throttling,
timeout) is treated as possibly-pre-existing, so an idempotent re-tap that
hits a transient probe error cannot delete a pre-existing resource on
rollback. Mirrors the fail-direction of sourceStillReferenced; covered by a
new interceptor-based test.
- The PackageSource reconciler now calls internal/marketplace/naming.ArtifactName
instead of inlining the formula, so the artifact-name convention has a single
definition shared by the producer, the marketplace backend, and the
validator, and the parity test guards producer-vs-helper.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
Add server-side connect to complete the marketplace write path. Creating a Tap
records the intent only: the API validates the oci:// reference and creates a
labeled Flux OCIRepository (with an optional pull-credential secretRef), never
blocking the request on a registry pull, and returns a Tap whose status shows
materialization pending. The connect inputs (url, tag, secretRef) are write-only
and are never echoed back or returned on read.
A new tap materializer reconciler in the operator watches community-tap
OCIRepositories, and once source-controller has pulled the artifact, fetches the
tarball over the Flux artifact URL (digest-verified, size-bounded, path-traversal
guarded), parses the PackageSource(s) it carries, and materializes them under the
community. prefix with their sourceRef repointed at the tap's source. A finalizer
cleans up the materialized PackageSources when the source is removed, leaving
installed Packages in place.
The shared label/annotation keys live in internal/marketplace/tapconst so the API
and the operator cannot drift on the connect-to-materialize handoff.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
Add release.cozystack.io/helm-server-side-apply on the ApplicationDefinition,
threaded through ReleaseConfig into every emitted HelmRelease's
Install/Upgrade.ServerSideApply. Empty leaves the helm-controller default
(server-side apply on v1.5.0); "false"/"disabled" forces client-side apply.
The postgres read-replica autoscaler needs this: KEDA's HPA writes the CNPG
Cluster's /scale subresource (spec.instances) while the chart renders a constant
seed. Under helm-controller's default server-side apply the rendered field is
force-owned and reverts KEDA's live value on every reconcile. Under client-side
apply helm-controller patches the CRD from the previous-vs-new rendered manifest
without consulting live state, so the unchanged constant yields no patch and the
/scale value survives (verified against Helm v4 createPatch: unstructured/CRD
objects take the two-way jsonpatch.CreateMergePatch(original, modified) path
unless threeWayMergeForUnstructured is set, which helm-controller v1.5.0 leaves
at its default).
Guarded by TestConvertApplicationToHelmRelease_ServerSideApplyOverride on the
tenant-release builder (convertApplicationToHelmRelease), and by
TestParseHelmServerSideApplyAnnotation. Removes a stale package_reconciler_test
guard that asserted a false invariant on the wrong (platform Package) builder.
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
Address review findings on the KEDA read-replica autoscaler where the
implementation diverged from the merged design proposal.
Fail-safe read load. Gate the `or vector(0)` floor on the existence of ANY CNPG
instance pod (`count(kube_pod_labels{...instance_role=~".+"}) > 0`) — primary or
replica — so the query is empty ONLY when no CNPG pod labels exist at all (a
broken kube-state-metrics pipeline), not during initial provisioning when the
sole replica-side pod is CNPG's join Job (jobRole, not instanceRole). Also pin
`ignoreNullValues: "false"` on the trigger: KEDA's prometheus scaler defaults it
true, turning an EMPTY result into metric 0 (not an error) — the HPA would then
desire 0 and clamp to the floor, i.e. silently scale a broken-pipeline cluster
DOWN. With it false an empty result is a scaler error, so the HPA HOLDS and
DatabaseAutoscalerScalerErrors fires; gating on any instance role keeps a healthy
cluster (including a long basebackup) from tripping that alert.
Constant seed = max(replicas, effectiveMin), so enable/dryRun/transition flips
are a merge no-op with no dip and never undercut the quorum floor.
Fail closed when the quorum floor exceeds maxReplicas (was: effectiveMax silently
raised both bounds, unbounded for a large maxSyncReplicas).
Dry-run / transition keep the HPA (unidirectional pause pair, not the HPA-deleting
full pause) but both SelectPolicy=Disabled, so desired normalizes to current — the
recommendation is read from the read-load metric, not a diverging desired count.
Observability. DatabaseAutoscalerAtMax keys on desired >= maxReplicas, NOT the
ScalingLimited condition (which is also true at the floor, so it fired on every
idle cluster with wrong "raise maxReplicas" advice). Dashboard read-load panels
are per-cluster (label_cnpg_io_cluster) with a CPU panel for ReadCPUUtilization
and the exact clamp in descriptions; ScaleStuck's Pending intersection is scoped
to CNPG pods; alerts.yaml narrative comments moved to Go-template comments so they
do not ship into the PrometheusRule.
Guard the seeding invariant: TestBuildHelmReleaseSpecNoForceOrDriftDetection.
Regenerated README/schema/types.
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
Add first-class fields so a package/app can declare the Flux helm-controller
v1.5.0 kstatus health gate for the custom resource it wraps, so
HelmRelease.Ready means "the CR is actually healthy" instead of "helm applied
the CR" (#2642). API-owner approved in #3263.
- api: WaitStrategy (scalar, maps to HelmReleaseSpec.WaitStrategy.Name — a
deliberate simplification) + HealthCheckExprs ([]kustomize.CustomHealthCheck,
the upstream type verbatim) on both ApplicationDefinition.spec.release (the
apiserver path) and ComponentInstall (the operator path).
- config.ResolveWaitStrategy: shared by both HelmRelease builders. Couples the
default — healthCheckExprs imply the poller strategy when none is set, since
exprs are only evaluated under poller. This removes the "must also set
waitStrategy: poller" footgun and does not depend on the controller's global
default.
- thread through buildHelmReleaseSpec (operator) and
convertApplicationToHelmRelease (apiserver); the apiserver loads the typed
spec.release fields in start.go.
- regenerate deepcopy, the ApplicationDefinition and PackageSource CRDs, and
openapi. The backupstrategy CRDs also regenerate — benign k8s-0.35 PodSpec
description drift, not a functional change.
disableWait is intentionally not added (YAGNI): the only known need (the
kubernetes tenant-addon deadlock) was resolved child-side, and waitStrategy:
legacy is not equivalent (legacy still waits Helm-v3-style; disableWait skips
waiting entirely). Add it later when a concrete leaf needs a true no-gate
escape hatch.
Refs #2642
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Address lexfrei's non-blocking follow-ups on the LGTM approval:
Follow-up #1 — give-up preempted the final backoff. `decideRecovery`
returned `recoveryActionGiveUp` as soon as `attempts >= maxRecoveryAttempts`,
without checking whether the final-force backoff window had elapsed. The
Owns()-watch re-fire from the N-th force-drift's own writes triggered a
reconcile milliseconds after the force, so the effective time-to-give-up
was ~7.5m (30s+60s+2m+4m) — not the ~11.5m documented on the constants
block. The final 4-minute response window the last nudge was supposed to
grant source-watcher was never actually granted.
Fix: extend the exhaustion branch to hold `Wait` until
`now - lastRecoveryAt >= backoffFor(maxRecoveryAttempts)` before switching
to GiveUp. Now the documented ~11.5m budget matches the observed
behaviour, and the last force gets a real chance to succeed (Owns
watch → source-watcher Ready=True copy-through) before we surface
SourceWatcherStalled.
Tests:
* `TestDecideRecovery` gains a case pinning `Wait` when
attempts=maxRecoveryAttempts but the final backoff has not yet
elapsed (the exact re-fire shape the previous behaviour tripped on).
* `TestUpdateStatus_OwnMarkerAttemptsExhaustedButFinalBackoffPending`
drives the full updateStatus path against the same shape, asserting
the PackageSource stays in Unknown/AwaitingSourceWatcherRecovery and
the RequeueAfter equals the remaining backoff, NOT zero (GiveUp).
Follow-up #3 — upstream version-skew note. The Go module dependency
tracks `fluxcd/source-watcher/api/v2 v2.0.3` (CRD types package only),
but the deployed binary this workaround targets is pinned to v2.1.0
at `internal/fluxinstall/manifests/fluxcd.yaml`. Comments referencing
`drift.go:52` / `controller.go:164/254` describe the deployed binary,
not the compiled API package. Documented on the constants block so a
future flux bump re-verifies the anchors AND the watch predicate /
drift-detection heuristics, which are the load-bearing upstream
assumptions.
Follow-up #2 (envtest over full Reconcile→Owns→updateStatus cycle) is
architectural and out of scope for this PR — noted for a follow-up.
Signed-off-by: Ivan Okhotnikov <xorokhotnikov@gmail.com>
Self-review nits raised in a skeptical pass over the current tip:
* The status-patch docstring in forceArtifactGeneratorDrift described
the JSON-merge-patch full-array-replace on `status.conditions[]` as
"net-neutral". That was too optimistic: if source-watcher writes a
concurrent Reconciling condition between our Get and our Patch, our
patch does transiently wipe it. source-watcher restores it on its
next reconcile (which our requestedAt bump is about to trigger), but
downstream tooling watching for Reconciling can observe the momentary
absence. Rewrite the comment to name that trade-off explicitly and
point at server-side apply as the fuller fix if the operator ever
migrates to SSA globally.
* The unified own-marker/progressing/stuck gate in updateStatus is
asymmetric on ObservedGeneration — isProgressingInRecovery checks it,
isOwnMarker does not. That asymmetry is deliberate (a stale own-marker
after a spec edit must still route to the state machine to avoid
leaking the synthetic reason to the PackageSource — exactly the B2
regression), not an oversight. Add a comment explaining the
reasoning so a future edit doesn't "symmetrise" the check and re-open
B2.
* forceArtifactGeneratorDrift's step-2 (metadata patch) failure was
silent when it happened after a successful step-1 status patch. In
that state source-watcher has been nudged, but the tracking
annotations never persisted, so the next reconcile reads attempts=0
and re-forces from scratch instead of accumulating toward
SourceWatcherStalled — a persistent metadata-write-only apiserver
malfunction would loop forever. Log the failure loudly with the
intended attempt count so operators can spot this specific mode
from cozystack-operator logs.
Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
Self-review after the previous commit removed the awaitSourceWatcherResponse
helper (routing unified through maybeRecoverArtifactGenerator's state machine
via a single own-marker / progressing-in-recovery / stuck gate). Four
comments and two test names still referenced the now-deleted symbol —
misleading anyone tracing the architecture.
* maybeRecoverArtifactGenerator docstring: rewrote the trade-off paragraph
to describe the unified gate and Wait branch instead of the removed
helper.
* TestMaybeRecoverArtifactGenerator_DoesNotResetCounterOnFreshTransition
comment: same rewrite.
* TestUpdateStatus_OwnMarkerRoutesToAwait renamed to
TestUpdateStatus_OwnMarkerHoldsTrackingViaStateMachine + comment
rewritten to describe the state-machine Wait branch.
* TestUpdateStatus_ProgressingDuringRecoveryRoutesToAwait renamed to
TestUpdateStatus_ProgressingDuringRecoveryHoldsTracking + comment
rewritten in parallel.
Pure doc / naming; no code or test-body changes. Tests still pass with
-race, vet clean, golangci-lint 0 issues.
Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
Self-review on 467b788b8 caught a MAJOR bug introduced by the separate
awaitSourceWatcherResponse helper: if source-watcher never responds to
a forced drift (network partition, missing predicate, dead pod), our
own reasonRecoveryForced marker keeps reflecting back via Owns() forever,
and the wait-forever helper schedules a 1-second requeue tight loop
without ever advancing the retry state machine. SourceWatcherStalled
give-up was unreachable.
Fix: remove awaitSourceWatcherResponse and unify all three recovery
entry conditions — our own marker, source-watcher's Progressing during
rebuild, and the classic first-time stuck detection — through a single
gate that routes to maybeRecoverArtifactGenerator. The state machine's
decideRecovery is the single arbiter of wait/force/give-up, and its
Wait branch is bounded by the same backoff schedule as Force. When
backoff elapses on a still-reflecting own-marker the machine escalates
to another force (bumps attempts, re-stamps requestedAt); when attempts
exhaust it surfaces SourceWatcherStalled honestly.
The own-marker filter also no longer requires attempts>0, so an
operator crash between step 1 (status patch) and step 2 (metadata
patch) of forceArtifactGeneratorDrift is still routed through the
state machine on the next reconcile rather than leaking the synthetic
marker onto the PackageSource.
Also address the non-blocking notes from the same self-review:
* Docstring on maybeRecoverArtifactGenerator was still describing the
removed reset-counter branch and the "requestedAt alone is a no-op"
framing. Rewrite to explain the current shape: status write triggers
the drift signal, requestedAt bump triggers the reconcile event —
both are required, neither alone works.
* TestForceArtifactGeneratorDrift_MetadataPatchFailure_RollbackAnnotations
now asserts reconcile.fluxcd.io/requestedAt is also rolled back on
the metadata-patch failure branch (symmetric with the tracking
annotations).
* TestUpdateStatus_OwnMarkerRoutesToAwait asserts an exact 25s
RequeueAfter (backoffFor(1)=30s minus 5s elapsed) so a silent
regression in the backoff arithmetic is caught.
New regression tests for the source-watcher-unresponsive path:
* TestUpdateStatus_OwnMarkerStale_TriggersRetry: with attempts=1 and
lastRecoveryAt older than backoffFor(1), the reconcile must
escalate through the Force branch and bump attempts to 2 rather
than idle in a tight requeue loop.
* TestUpdateStatus_OwnMarkerAttemptsExhausted_GivesUp: with
attempts=maxRecoveryAttempts and the marker still reflecting back,
the reconcile must set PackageSource Ready=False /
SourceWatcherStalled instead of extending the loop indefinitely.
Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
Address lexfrei's three blockers on the force-drift approach — as
committed, the recovery driver did not function on a live cluster.
**B1**: `forceArtifactGeneratorDrift` wrote `Ready=False` on the AG's
status subresource but did not touch its metadata annotations.
source-watcher v2.1.0 registers its ArtifactGenerator watch with
`predicate.Or(GenerationChangedPredicate, ReconcileRequestedPredicate)`
— a status-only patch changes neither the generation nor
`reconcile.fluxcd.io/requestedAt`, so it does NOT enqueue a
source-watcher reconcile. The AG's periodic self-requeue is hardcoded
to `time.Hour`, well outside the 15m HR install timeout. Fix: also
stamp `reconcile.fluxcd.io/requestedAt` in the metadata patch so the
ReconcileRequestedPredicate fires and source-watcher's next reconcile
picks up the `IsFalse(Ready)` drift signal.
**B2**: `Owns(&ArtifactGenerator{})` re-fires this reconciler on the
force-drift's own writes. Both the status patch (Ready=False, reason
`SourceWatcherRecoveryForced`) and the metadata patch enqueue a
PackageSource reconcile. Blindly running `clearRecoveryTracking` on
those re-entrant fires nuked the attempts counter — the give-up
budget was unreachable, and the synthetic marker leaked to
`PackageSource.status.conditions[Ready]`. Fix: in `updateStatus`,
detect our own reason and route into a new `awaitSourceWatcherResponse`
helper that holds tracking and reports Unknown /
AwaitingSourceWatcherRecovery on the PS. The same helper covers the
Progressing/Unknown write source-watcher issues during rebuild once
tracking is present (attempts > 0) — otherwise the transient
Progressing would trigger the same premature tracking clear.
**B3**: The attempt-counter reset keyed on
`Ready.LastTransitionTime > lastRecoveryAt`. That inequality is true
on every source-watcher response during recovery (the Progressing
write is always newer than our force), so under the very race this
driver targets the counter would reset every cycle and
`recoveryActionGiveUp` was unreachable. Fix: remove the reset. The
canonical counter reset is the natural `clearRecoveryTracking` on the
not-stuck path (Ready=True from source-watcher, or a real upstream
Ready=False), and the HA/leader-election requirement already
contains the "missed recovery event on operator restart" edge case
the reset was trying to paper over.
Extract `reasonRecoveryForced` to a named constant so both the
writer (forceArtifactGeneratorDrift) and the reader (updateStatus'
own-marker filter) use the same symbol.
New tests pin all three fixes:
- ForceArtifactGeneratorDrift_Success now asserts the
reconcile.fluxcd.io/requestedAt annotation is set (B1).
- UpdateStatus_OwnMarkerRoutesToAwait asserts we do NOT clear
tracking and do NOT copy the synthetic Ready=False through when we
see our own marker (B2).
- UpdateStatus_ProgressingDuringRecoveryRoutesToAwait covers the
second Owns() re-entrancy path — source-watcher's Progressing
write during rebuild (B2).
- UpdateStatus_ReadyTrueClearsTracking pins that the natural not-
stuck path IS the canonical counter reset after B3.
- The old ResetsCounterOnFreshTransition test is replaced with
DoesNotResetCounterOnFreshTransition, asserting the opposite
behaviour that closes B3.
Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
E2E on dev3 caught a dormancy bug that renders the whole recovery driver
inactive in practice: when updateStatus copies Ready=Unknown across but
the AG's LastTransitionTime is still within stuckGracePeriod,
artifactGeneratorStuck returns false and we fall through to the copy
path with no RequeueAfter set. If the reconcile happens to fire in the
[stuck_start, stuck_start+30s) window — a routine case since operator
restarts touch every PackageSource — the controller then sleeps
waiting for an AG-status change that will never come (that IS the
fluxcd/pkg#934 lost-write race), up to the 10h informer resync.
Observed on dev3: `cozystack.networking` stuck at Ready=Unknown for 9+
minutes with zero recovery attempts. Two reconciles fired at
stuck_start+23s and +24s — both correctly detected "not yet stuck" —
and then nothing until observation ended. Upstream OCIRepository
revision was unchanged throughout, ruling out a coincidental source
update as the cause.
Fix: whenever we copy Ready=Unknown to the PackageSource (or set a
synthetic Unknown because no Ready condition exists yet), compute the
remaining grace period against the AG's LastTransitionTime (or
CreationTimestamp for the missing-Ready branch) and schedule a
RequeueAfter at that expiry. A 1s floor prevents a zero-delay tight
loop when the anchor is right at or past grace expiry.
Extracted the delay computation to `agFollowUpDelay` for direct unit
testing. Adds three tests: the follow-up scheduling on Ready=Unknown
in-grace, the same on the missing-Ready branch, and a table test on
agFollowUpDelay covering the floor edge cases.
Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
Address the fourth-pass self-review's non-blocking findings:
* Add TestMaybeRecoverArtifactGenerator_{ForceBranch, WaitBranch,
GiveUpBranch, ResetsCounterOnFreshTransition}. The three switch
outcomes of the driver (force / wait / give-up) and the reset-counter
guard on a fresh Ready.LastTransitionTime transition were previously
only covered indirectly through their pure-function components. Now
each outcome is asserted end-to-end against a fake client with a
status subresource, verifying the Ready condition, tracking
annotations, RequeueAfter, and — for the wait branch — that no AG
writes are issued at all.
* Tighten TestDecideRecovery's wait-branch assertions from a loose lower
bound to exact equality. decideRecovery is deterministic
(needed - elapsed) so an exact assertion catches subtle backoff
regressions a lower bound would miss (coderabbit's inline nit).
* Rename the local var `wantWaitMin` to `wantWait` for the tighter
assertion shape.
* Fix two stale identifier references in comments
(maybeRequeueArtifactGenerator, AwaitingSourceWatcherRequeue) left
over from the rename pass.
* Add a docstring paragraph on forceArtifactGeneratorDrift explaining
why the JSON-merge-patch full-array-replace on
`.status.conditions[]` is net-neutral in this workaround's context.
Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
Rework the fluxcd/pkg#934 recovery driver after myasnikovdaniil's B1
finding: bumping reconcile.fluxcd.io/requestedAt cannot recover a stuck
ArtifactGenerator because source-watcher v2.1.0's reconcile is
drift-gated. detectDrift (artifactgenerator_drift.go:52) returns
NoDriftDetected for exactly the stuck signature we target — Inventory
populated, ObservedSourcesDigest set, Ready=Unknown on the current
generation — so the ReconcileRequestedPredicate fires a reconcile that
lands on the no-drift early-return at controller.go:164 and MarkTrue at
:254 is never reached. Every attempt in the previous shape was a no-op.
New mechanism: force drift by patching Ready=False on the AG's status
subresource. detectDrift picks this up as NotReady drift, source-watcher
takes the drifted branch, and either MarkTrue (on healthy rebuild) or
MarkFalse with a real reason (on genuine failure) replaces our synthetic
False. Owns() watch fires this reconciler to copy the resolved condition
through. Same bounded-retry harness (30s / 60s / 2m / 4m / 4m budget
inside the 15m HR install timeout, SourceWatcherStalled surfaced on
exhaustion), same annotation-based state on the AG, same rollback
contract on Patch failure — only the action itself changed.
Additional fixes bundled in:
* Reset attempt counter when the AG's Ready.LastTransitionTime is newer
than our lastRecoveryAt, so a fresh stall episode after a previous
give-up gets a fresh budget rather than an immediate re-give-up
(myasnikovdaniil's follow-up #3).
* Propagate updateStatus errors from Reconcile instead of swallowing them,
so controller-runtime's exponential backoff retries transient failures
on force-drift / status-update writes rather than dropping them silently
(coderabbit's inline finding).
* forceArtifactGeneratorDrift documents the two-subresource shape (status
patch first, metadata patch second) and the partial-failure semantics.
* New tests cover the status-patch success / status-patch failure /
metadata-patch failure paths, all with fake client + interceptor. The
successful case asserts Ready=False actually lands in the apiserver via
WithStatusSubresource.
* All annotation / action / decision / helper names renamed from
*Requeue* to *Recovery* to match the new semantics.
Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
Address the fourth-pass self-review's non-blocking NITs:
* Add TestBumpArtifactGeneratorRequeue_PatchFailure_RollbackRestoresPriorBump
to cover the subtle case the two existing rollback tests miss: when the
tracking annotations were already set from a prior bump, a failed Patch
must restore the PRIOR values, not delete the keys. A refactor that
naively swapped rollback for `delete(ag.Annotations, key)` would pass
the nil-rollback tests but silently regress this contract.
* Rename the cloneAnnotations subtest from "deep copy" to "independent
copy" — the code documents shallow copy semantics and the misnomer was
the only thing pointing readers at the wrong mental model.
* Hoist `now.UTC().Format(time.RFC3339Nano)` to a local so the two
timestamp annotations use one formatted value instead of two identical
Format calls.
Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
Address the third-pass self-review's blocker: the mutation contract on
bumpArtifactGeneratorRequeue / clearRequeueTracking, the DeletionTimestamp
early-return in Reconcile, and the nil-preserving cloneAnnotations helper
had no regression tests, so a future refactor could silently break the
contracts documented in comments.
New fake-client + interceptor-based tests:
* TestCloneAnnotations — pins the "nil in / nil out" sentinel behaviour
and mutation-independence of the shallow copy.
* TestBumpArtifactGeneratorRequeue_Success — asserts the persisted state
matches the caller's in-memory `ag.Annotations` on success, including
that reconcile.fluxcd.io/requestedAt is actually written (previously
never asserted anywhere).
* TestBumpArtifactGeneratorRequeue_PatchFailure_Rollback and its
PreservesUnrelated variant — drive the Patch-failure branch via an
interceptor client and assert both that our tracking annotations are
rolled back and that pre-existing unrelated annotations survive.
* TestClearRequeueTracking_Noop — asserts we don't issue a no-op Patch on
every healthy reconcile when there are no tracking annotations to clear.
* TestClearRequeueTracking_PatchFailure_Rollback — same rollback contract
as bump, on the clear path.
* TestReconcile_DeletionTimestamp_EarlyReturn — asserts a PackageSource
with a deletionTimestamp does not receive any writes and does not
schedule a RequeueAfter.
Doc tightening in the same commit:
* Reword clearRequeueTracking's comment on the flux annotation semantics —
standard flux controllers do not clear reconcile.fluxcd.io/requestedAt,
they only advance .status.lastHandledReconcileAt via the
ReconcileRequestedPredicate.
* Note controller-runtime v0.15+'s default RecoverPanic behaviour where
the retry-driver's unhandled-action panic is raised, so a future
SetupWithManager flip that disables it is easier to spot.
* Explain why cloneAnnotations' shallow copy is sound (annotation values
are immutable strings).
Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
Address second-pass self-review findings on the bounded-requeue driver:
* Skip reconcile early when the PackageSource has a deletionTimestamp so
the retry driver stops writing "AwaitingSourceWatcherRequeue" onto an
object about to be garbage collected. ownerReference cascade handles
the ArtifactGenerator cleanup.
* Fix the mutation contract on bumpArtifactGeneratorRequeue and
clearRequeueTracking: the previous deep-copy shape left the caller's
`ag` desynchronised from the apiserver on success. Now mutations land
on `ag` itself with a rollback to the pre-call annotation snapshot on
Patch failure — so success reflects persisted state and failure does
not leak unpersisted state to subsequent code paths.
* Convert the "unreachable" default in maybeRequeueArtifactGenerator to
an explicit panic so a future requeueAction added without a matching
case fails loudly instead of freezing the Ready condition silently.
* Document the timing constants' rationale (grace period matches the
observed <10s source-watcher settle time on dev3; total ~11.5m budget
keeps SourceWatcherStalled inside the 15m HR install timeout) and note
the operator must run with leader election when replicas > 1 so the
annotation-based retry state does not race.
Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
Address self-review nits on the bounded-requeue driver:
* Sample `now := time.Now()` once at Reconcile entry and thread it into
`updateStatus`, `artifactGeneratorStuck` and `maybeRequeueArtifactGenerator`
so every time-sensitive check inside a reconcile agrees on the same
wall-clock reading.
* Set `ObservedGeneration` in the three early Ready-condition branches
(`SourceRefNotSet`, `ArtifactGeneratorNotFound`, `ArtifactGeneratorNotReady`)
for parity with the retry-driver branches.
* Patch the ArtifactGenerator via a deep copy in
`bumpArtifactGeneratorRequeue` and `clearRequeueTracking` so a failed Patch
no longer leaves the caller's in-memory `ag` desynchronised from the
apiserver.
* Document the retry driver's dependency on source-watcher's
ReconcileRequestedPredicate on ArtifactGenerator, so a future upstream
change dropping the predicate is easier to trace.
Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
Replaces the Ready=True synthesis for PackageSources whose ArtifactGenerator
hit the fluxcd/pkg#934 patch.Helper split-write race with a bounded retry
driver: on detected stall, bump reconcile.fluxcd.io/requestedAt with
exponential backoff (30s to 4m cap) up to 5 attempts, then surface as
PackageSource Ready=False with reason SourceWatcherStalled.
The previous approach could mask a genuine regeneration failure if the same
patch.Helper race that the workaround exists for also swallowed a real
Ready=False write, and it permanently reinterpreted PackageSource.Ready
semantics from "current revision processed" to "valid consumable artifacts
exist". Requeue keeps the CRD contract intact and, on stall exhaustion,
produces an honest failure signal an operator can act on instead of a
false-positive True.
Retry state (attempts + last-bump timestamp) lives on the AG as
cozystack.io/source-watcher-* annotations so it survives operator restarts.
Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
review-branch skill flagged that the comment claimed
meta.SetStatusCondition rewrites LastTransitionTime on Status OR
Reason change. Actually only Status change triggers a
LastTransitionTime rewrite — Reason and Message are updated in
place, LastTransitionTime is preserved. Outcome (repeated
identical calls are no-op-y on persisted state) is unchanged;
just tightening the technical claim to match apimachinery.
Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
Reviewer nit was to file an upstream issue so the workaround does
not silently outlive its cause. Checked first — fluxcd/pkg already
has one open: #934 \"patch.Helper creates race conditions in
downstream controllers\" (May 2025). Same root cause we traced:
patch.Helper writes .status and .status.conditions as separate
apiserver requests, so an etcd blip between the two leaves the
object half-persisted. The upstream issue is broader than just
source-watcher — every fluxcd downstream controller consuming
fluxcd/pkg's runtime patch helper is affected.
Update the TODO(remove) marker on our workaround to point at
that issue and note that both source-watcher v2.1.0 and v2.2.x
(flux-aio latest) pull the same fluxcd/pkg runtime, so bumping
source-watcher alone does not remove the need for this synthesis.
Compile + unit tests unchanged.
Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
kvaps and lexfrei confirmed the diagnosis by tracing source-watcher
v2.1.0 (same code paths in v2.2.x / flux-aio latest, so a simple
version bump does not remove the need for this workaround):
* fluxcd's patch.Helper patches .status and .status.conditions as
SEPARATE apiserver requests, so an etcd timeout during first-
install load can land between them — leaving Inventory /
ObservedSourcesDigest persisted while the Ready-condition write
is lost.
* On the next reconcile the !hasDrifted branch returns before
touching conditions. The deferred summarizeStatus still runs and
adjusts the Reconciling condition, but it never promotes a
stuck Unknown Ready to True — that promotion only happens on the
successful hasDrifted path.
Two substantive changes prompted by the reviews:
1. Document the semantic change the workaround introduces on
PackageSource.status.conditions[Ready]. fluxcd/pkg/runtime/
conditions.Set writes condition.ObservedGeneration =
ag.Generation on EVERY condition mutation including the
Progressing/Unknown mark at the start of a rebuild. And
ArtifactGeneratorStatus does not expose an object-level
ObservedGeneration (only ReconcileRequestStatus is inlined).
So Generation-matching cannot distinguish 'current revision
fully processed' from 'rebuild in progress, previous
Inventory/digest still persisted' — meaning the predicate can
(and does) fire during a content-only regeneration (new OCI
revision, same .metadata.generation).
Net effect: PackageSource Ready no longer dips through Unknown
while a new revision is being processed; it now means 'valid
consumable artifacts exist' rather than 'the current revision
has been processed'. Kvaps flagged this as the better contract
for install/upgrade ordering (no InProgress flapping, and a
genuine regeneration failure still surfaces as Ready=False on
the successful hasDrifted path). Document it inline instead of
implying the workaround only fires in the stuck-status window.
2. Reworked the predicate signature to take the pre-resolved
Ready condition as a parameter. The caller was already doing
the FindStatusCondition lookup for the pass-through path, so
the helper no longer duplicates it (kvaps' nit).
Also refreshed the pre-existing comments:
* The 'early-exits before touching status again' step is now
correctly worded as 'returns before touching conditions again;
the deferred summarizeStatus never promotes a stuck Unknown
Ready to True'.
* Bug attribution now names the code paths ('!hasDrifted branch',
'summarizeStatus deferred'), notes that v2.1.0 and v2.2.x
('flux-aio latest') both ship the affected code, and carries a
TODO(remove once upstream is fixed) marker so the workaround
cannot silently outlive its cause.
Tests: existing 8 subtests updated to construct the ready condition
manually so the call matches the new signature, plus one new
subtest locking in the intentional 'content-only regeneration
predicate fires' behaviour so a future refactor that tightens the
predicate back to 'current revision processed' — which would
re-introduce the flapping — fails loudly. 9/9 pass.
Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
Own-audit found the first pass had a spec-update race: if a user
edited PackageSource.spec.variants on a running cluster, cozystack
would bump the derived ArtifactGenerator to Generation=2, and
between "source-watcher started reconciling gen=2" and "source-
watcher wrote new Inventory + ObservedSourcesDigest for gen=2" the
ArtifactGenerator still exposes the previous generation's
Inventory / ObservedSourcesDigest AND Ready=Unknown (Progressing).
The workaround would synthesise Ready=True on stale artifacts, and
downstream Flux would consume the previous-generation
ExternalArtifacts as if the new spec had converged.
Tighten the predicate to require the Ready condition's
ObservedGeneration to match ag.Generation. Only when source-watcher
has actually processed the current spec (which happens strictly
after it writes the current-generation Inventory + Digest) does the
Ready condition carry ObservedGeneration == ag.Generation. That is
exactly the window where Inventory / Digest are guaranteed current-
spec AND only the status patch on Ready itself failed — the actual
upstream bug this workaround exists to paper over.
Consequence: if Ready is missing entirely we return false now
(was true). We cannot verify ObservedGeneration on an absent
condition, and the missing-Ready state is indistinguishable from
mid-reconcile-of-a-newer-generation. On the next reconcile source-
watcher will write at least a Progressing condition, at which point
the current-spec Generation check will be evaluable and the
workaround will kick in if — and only if — Inventory / Digest also
match the current spec.
Tests updated:
* changed 'no Ready condition' case from want=true to want=false
(reflecting the tighter invariant);
* added a case where Ready=Unknown carries an OLD Generation
(want=false — do not synthesise on stale artifacts);
* stamped ObservedGeneration on every existing condition-bearing
case so they exercise the matching-Generation branch;
* introduced a small agWithGen helper to keep the fixtures
readable now that Generation is load-bearing.
All 8 subtests pass; existing operator suite unaffected.
Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
Symptom
cozystack-platform HelmRelease times out at 15m on fresh install
with a single PackageSource stuck in InProgress. Snapshot from
#3178 CI run 28587656430:
PackageSource cozystack.vm-disk-application
Ready: Unknown, reason: Progressing
lastTransitionTime: 2026-07-02T13:39:26Z
(snapshot generated 30+ minutes later — no state change)
ArtifactGenerator cozystack.vm-disk-application
status.inventory: 2 items with sha256 digests
status.observedSourcesDigest: populated
status.conditions[Ready]: Unknown, reason: Progressing
status.conditions[Reconciling]: True, reason: Progressing
ExternalArtifacts are produced. Downstream Flux consumes them and
its HelmReleases install fine. But PackageSourceReconciler
faithfully copies the Unknown Ready condition through, so
cozystack-platform (which waits on PackageSource.Ready=True) never
converges.
Root cause
Traced in fluxcd/source-watcher:v2.1.0 log for the affected
ArtifactGenerator:
13:39:28.475 ExternalArtifact vm-disk reconciled ✅
13:39:28.641 ExternalArtifact vm-disk-rd reconciled ✅
13:39:45.719 ❌ failed to update status: etcdserver: request timed out
13:39:45.720 ❌ Reconciler error (etcd timeout)
13:39:46.485 ✅ No drift detected, 2 artifact(s) up to date
——— nothing else for this ArtifactGenerator ———
Under first-install etcd pressure the patchStatus call times out.
The reconciler returns the etcd error; controller-runtime prints
the "returned both a result and non-nil error, RequeueAfter
ignored" warning and requeues. The next reconcile hits the "No
drift detected" short-circuit (artifacts are already up to date)
and early-exits before touching status. Result: the Ready
condition is stuck Unknown until either the process restarts or
something forces a state change on the ArtifactGenerator. Neither
happens in a real install run.
This is an upstream bug in source-watcher's reconcile loop. It has
been papered over once already at the runtime-tuning level in
#3163 (source-watcher concurrency + CPU), but a bad etcd second
during install still lands us here. Cozystack cannot afford to
wait for an upstream release cycle to unblock every install.
Fix
PackageSourceReconciler.updateStatus now checks two invariants
before falling back to copying the (potentially stuck) upstream
Ready condition:
1. ArtifactGenerator.status.Inventory is non-empty
2. ArtifactGenerator.status.ObservedSourcesDigest is set
Both fields are only written by source-watcher after the artifacts
have actually been produced — status.Inventory carries the
ExternalArtifact digests downstream Flux is already consuming. If
BOTH are present AND the upstream Ready condition is still Unknown
(or absent), the PackageSource Ready condition is synthesised as
True with a distinctive reason `ArtifactsGeneratedAwaitingUpstreamStatus`
and a message that names the upstream bug and the observable state
it relied on. If the upstream Ready ever catches up (True or False)
the Owns() watch re-fires the reconciler and the real condition
overwrites the synthetic one — True stays True, False surfaces the
real failure. Failures are NEVER masked: the synthesis path only
triggers when Ready is Unknown or missing.
Safety
* Never synthesises True over a real False → upstream failure is
surfaced unchanged.
* Requires BOTH Inventory non-empty AND ObservedSourcesDigest set
→ a fresh ArtifactGenerator that has literally not started work
cannot be prematurely marked Ready.
* ObservedGeneration is stamped so a spec change re-invalidates
the synthesis until the next observable-artifacts pass.
* Reason string is greppable so operators can audit which
PackageSources came up via the workaround.
Tests
Added TestArtifactGeneratorObservablyReady with 7 cases covering
every branch of the predicate:
- fresh generator: false
- inventory only: false
- digest only: false
- both set + Unknown Ready: true (the actual bug)
- both set + no Ready cond: true (same window pre-stamp)
- both set + Ready=True: false (pass real condition)
- both set + Ready=False: false (never mask failure)
All pass; existing operator unit tests are unaffected.
Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
The XValidation justification was factually wrong: the deployed
helm-controller CRD has no rule rejecting a Strategy alongside a
Remediation entry (only .retryInterval is gated on Strategy.Name), so
the claimed "silently break every HR" safety net does not exist.
Reword the comment in all three test files to the real reason
Remediation stays nil: retries are driven by a single
Strategy.RetryInterval path, matching cozystack-operator's
PackageReconciler.buildHelmReleaseSpec.
Address review feedback from lexfrei on rest_helmrelease_spec_test.go,
rest_timeout_test.go and internal/operator/package_reconciler_test.go.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
helm-controller's XValidation rule rejects Strategy.Name=RetryOnFailure
with RetryInterval set alongside a Remediation entry, so a future
"for safety" reintroduction of Remediation{Retries: -1} would silently
break every HR. Pin the deliberate omission.
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Avoid pointer aliasing across all generated HelmReleases. r.HelmReleaseMaxHistory
is set once at startup today, but a future mutation through the field would
silently affect every in-flight HelmReleaseSpec.
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Chart template now uses `ne (toString …) ""` for the five helmRelease*
knobs, so integer 0 (notably MaxHistory=0 = unlimited history per Helm
semantics, documented in values.yaml) is no longer silently dropped.
Extract buildHelmReleaseSpec from Reconcile and add unit tests
asserting Interval, MaxHistory, Install/Upgrade timeouts,
Strategy=RetryOnFailure, RetryInterval, and ChartRef propagate from
PackageReconciler config to the generated HelmRelease.
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
The HelmRelease generation in package_reconciler hardcoded three values
that have caused recurring pain:
- Install.Timeout = 10m (line 214)
- Upgrade.Timeout = 10m (line 220)
- MaxHistory = unset (Helm default 5)
The 10m timeout in particular contradicts the per-Application timeout
work done in `pkg/config/config.go` and
`pkg/registry/apps/application/rest.go` (commit 7b146cbe), which lets
individual Applications override the install/upgrade timeout via
annotation. The PackageSource side had no equivalent — etcd works around
it by hand-rolling its HR YAML in `packages/apps/tenant/templates/etcd.yaml`
to set a 30m timeout, harbor has had multiple commits chasing the same
problem from the other side. This closes that gap.
New operator flags (each maps to a chart value with empty default;
operator uses its own default when value is empty):
- `--helmrelease-install-timeout` (default 10m) → Spec.Install.Timeout
- `--helmrelease-upgrade-timeout` (default 10m) → Spec.Upgrade.Timeout
- `--helmrelease-max-history` (default 5) → Spec.MaxHistory
Production behaviour unchanged. E2E and edge cases (cert rotation,
slow-installing charts) can override per-cluster without modifying
chart manifests.
Per-component overrides on `ComponentInstall` (mirroring the existing
`UpgradeCRDs` pattern) would be a strictly better fix for charts like
etcd that need an outlier timeout — left as a deliberate followup since
it touches the Package CR API.
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
The operator generated HelmReleases with `Install.Remediation{Retries: -1}`
and `Upgrade.Remediation{Retries: -1}`, which is the v2 way of saying
"retry forever, never remediate". Combined with the spec-level
`Interval: 5m`, this meant a failed install/upgrade waited a full 5
minutes before the next attempt — even when the underlying race that
caused the failure (e.g. a chart artifact not quite Ready, a HR
dependency reconciling out of order) cleared in seconds.
Audit of the last 30 successful PR runs found that every single run had
its `Install Cozystack` step's `kubectl wait hr/seaweedfs-system` time
out at the 2-minute mark and recover only after an inline
`flux reconcile --force` workaround. Same root cause: 5-minute retry
interval was longer than the test's wait window. That workaround can be
removed once this change lands.
Switch to `Strategy.Name=RetryOnFailure` with `RetryInterval` exposed via
a new `--helmrelease-retry-interval` operator flag (default 30s):
- Functionally equivalent to the previous configuration ("retry forever
on failure") since `Retries: -1` meant remediation never fired anyway.
- Decouples retry-on-failure timing from `spec.Interval` so failed
releases recover at 30s without polling healthy releases at the same
cadence (which would multiply controller load by ~10x for no benefit).
- Helm chart exposes `cozystackOperator.helmReleaseRetryInterval`,
defaulting to empty (operator uses its own 30s default).
- Same flag pattern as the existing `--helmrelease-interval` (5m default),
also added in this PR via cherry-pick.
Verified: go build green, go test ./internal/operator/... pass, helm
template renders both flags conditionally on values.
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Adds --helmrelease-interval (default 5m, matching today's behaviour) to the
operator and a corresponding cozystackOperator.helmReleaseInterval value
on the cozy-installer chart (default empty -> no flag rendered ->
operator default 5m applies).
E2E install sets it to 30s. Rationale: with the 5m default, dependency-
blocked HRs (waiting on cert-manager webhooks, CRDs) are requeued only
every 5 min, producing an 8-10 min dead zone in the E2E install where
the fast pack of HRs is Ready but the slow tail hasn't been retried yet.
30s collapses the gap.
Production defaults are unchanged. The override is opt-in per install.
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Add an opt-in UpgradeCRDs field to ComponentInstall that maps to
HelmRelease.Spec.Upgrade.CRDs, allowing a PackageSource component to
declare how Flux should handle CRDs from the chart's crds/ directory
on upgrade.
The helm-controller default on upgrade is Skip, which means new CRDs
added between chart versions never reach existing clusters and must be
applied manually. Setting upgradeCRDs: CreateReplace makes Flux apply
new CRDs declaratively with the chart.
Allowed values are restricted to Skip, Create, CreateReplace via a
kubebuilder enum marker. Empty / unset preserves the existing Flux
default, so all existing PackageSource resources keep working.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Instead of using per-Package SSA field owners (which is a workaround relying
on SSA mechanics), properly resolve whether a namespace should be privileged
by iterating all PackageSources and their active Packages. A namespace gets
the privileged PodSecurity label if ANY Package has a component with
privileged: true installed in it.
This fixes the race condition where Packages sharing a namespace (e.g.
linstor and linstor-scheduler in cozy-linstor) would overwrite each other's
labels depending on reconciliation order.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
When multiple Packages share a namespace (e.g. cozystack.linstor and
cozystack.linstor-scheduler both use cozy-linstor), the shared SSA field
owner "cozystack-package-controller" caused a race condition: the last
Package to reconcile would overwrite the namespace labels set by others.
This meant that if cozystack.linstor set pod-security.kubernetes.io/enforce=
privileged and then cozystack.linstor-scheduler reconciled without the
privileged flag, SSA would remove the label. On Talos clusters (which
default to "baseline" PodSecurity enforcement), this caused privileged
satellite pods to be rejected with PodSecurity violations.
Fix by using per-Package field owners (cozystack-package-{name}), so each
Package independently manages its own namespace labels without interfering
with other Packages sharing the same namespace.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Add dedicated flux-tenants controller with label selector
--watch-label-selector=sharding.fluxcd.io/key=tenants to handle
tenant workloads separately from platform components.
Update all kubernetes app HelmReleases to:
- Use chartRef with ExternalArtifact instead of OCIRepository sourceRef
- Add sharding.fluxcd.io/key=tenants label
- Add cozystack.io/target-cluster-name label
Update fluxinstall to parse multiple YAML manifest files and
use flux service for storage-adv-addr.
Add cozystack-basics package for core tenant/namespace setup.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Add automatic injection of cozystack-values secret reference into
HelmReleases created by Package reconciler. This enables charts to
access cluster and namespace configuration via .Values._cluster and
.Values._namespace.
Add annotation operator.cozystack.io/skip-cozystack-values to disable
injection for specific PackageSources (used for platform PackageSource
to avoid circular dependency).
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
IvanHunters