WaitForCRDsEstablished reported a cancelled context two different ways.
The check at the top of the poll loop returned a message with no CRD
name; the select at the bottom named the CRD the poll had stopped on.
Which one ran was left to the scheduler: when the deadline and a tick
come ready in the same instant, a select picks between the ready cases
at random, so one cancellation reached the operator either way.
Build the message in one place and return it from both exits, so the two
are indistinguishable by construction. The name is carried across
iterations, since a cancellation noticed at the top of the loop has no
poll of its own to take one from, and starts as the first CRD of the set
for a wait cancelled before any poll observed anything.
Keeping the check at the top is what makes carrying the name worthwhile.
A client refuses Get on a dead context at the first name in the list
whichever CRD is actually lagging, so a loop that re-entered and polled
anyway would overwrite the name its last poll settled on. The check
closes that window and no more than that: a context dying part-way
through a poll still records the first Get to fail, because this branch
cannot tell a refused request from a CRD status.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <3811295@gmail.com>
(cherry picked from commit 5747747087)
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
getWorkloadMetadata filled the workloads.cozystack.io/kubevirt-vmi-instance-profile
label from the annotation kubevirt.io/cluster-instanceprofile-name, a key KubeVirt
never stamps. KubeVirt records the cluster preference (which carries the guest OS
profile) under kubevirt.io/cluster-preference-name, so the profile label was never
emitted and a VM's instance profile never reached the billing metric. The sibling
instance-type label worked only because its key is the real one.
Read the correct annotation. The emitted label name is unchanged, so the downstream
metric pipeline is untouched.
Assisted-by: LLM
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
(cherry picked from commit 1f9bad0643)
The Keycloak comment and the helper doc claimed the tenant LimitRange as the
reason for the resources, but cozy-keycloak has no LimitRange and neither
does a tenant that leaves resourceQuotas empty: there the sidecar had no
requests and no limit at all, and the change gives it a 1Gi ceiling where
there was none. The comments and both doc blocks now say what applies to
each caller. No rendered object changes.
Assisted-by: LLM
Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com>
(cherry picked from commit 4dc6065ec9)
The barman-cloud plugin injects its sidecar without resource requests or
limits. Tenant namespaces ship a LimitRange that defaults containers to
128Mi, so the sidecar inherits that default and is OOMKilled mid-backup:
the ObjectStore stays healthy, the Cluster stays Ready, and only the
backup fails. A cgroup high-water mark of 254 MiB on a 38 MB database
shows the default cannot hold it.
The shared cozy-lib helper and the Go path that builds the platform's own
ObjectStore now request 100m/256Mi and cap memory at 1Gi. No CPU limit is
set, matching the entityOperator precedent in the kafka chart: a
throttled sidecar stalls WAL archiving instead of failing it.
The helper is renamed from checksumSidecarConfiguration to
sidecarConfiguration, since it no longer carries only the checksum pin.
Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com>
(cherry picked from commit 9214430bd8)
## What this PR does
Hand-backport of #3443 to `release-1.6`. The change merged to `main` on
2026-07-24, after v1.6.0 was cut, so it never reached any 1.6 patch and
is currently slated to wait for 1.7. This lands it in 1.6.2.
The cozystack-controller meters S3 bucket sizes for billing by querying
SeaweedFS metrics from a VictoriaMetrics vmselect it discovers via the
`namespace.cozystack.io/monitoring` label on the tenant namespace. That
discovery assumes the monitoring stack scraping SeaweedFS runs in the
same cluster. Some deployments host SeaweedFS (API and metrics both) in
a separate cluster, where discovery can never produce a reachable
endpoint. This adds a single controller flag,
`--seaweedfs-metrics-endpoint`, exposed as the chart value
`cozystackController.seaweedfsMetricsEndpoint`, to point bucket-size
metrics at a remote Prometheus-compatible query API. Empty keeps the
existing label-based discovery, so behaviour is unchanged by default.
### Backport adaptations
Clean cherry-pick of `95e4faf68`; no adaptations required. `go build
./cmd/...` and the full `internal/controller` test suite pass.
### Screenshots
Not applicable — no UI change.
### Downstream repositories
- [x] No downstream repository is affected by this change
### Release note
```release-note
feat(workloadmonitor): add --seaweedfs-metrics-endpoint to fetch S3 bucket size metrics from a remote Prometheus-compatible endpoint, for deployments where SeaweedFS and its monitoring stack run in a separate cluster
```
An in-place RestoreJob (targetApplicationRef omitted) re-bootstraps the
Postgres app under its own name. On stock v1.6.2 it never comes back up,
for two independent reasons in the same restore patch - both surface only
in place, because a to-copy restore targets a different app.
1. Archive-vs-recovery serverName collision. The driver set the restored
cluster's WAL-archive serverName equal to the recovery source
(bootstrap.serverName = bootstrap.oldName = sourceServerName) while the
chart pinned the archiver serverName to the release name. In place those
coincide, so the cluster archived onto the source's own WAL prefix and
barman-cloud-check-wal-archive refused to start ("Expected empty
archive"). Fixed by giving the restored cluster a fresh
bootstrap.newServerName, keyed by the RestoreJob UID (unique per
restore, stable across reconciles); the chart archives there while
externalClusters still recovers from bootstrap.serverName. This is what
the manual CNPG strategies hit (serverName defaults to the cluster
name); the cozy-default flow dodged it (serverName is <ns>-<app>).
2. useSystemBucket guard. The restore populates explicit S3 coordinates
(destinationPath / s3CredentialsSecret / endpointCA) but left
backup.useSystemBucket=true, so the chart's "bootstrap.enabled is
incompatible with useSystemBucket" guard failed the HelmRelease upgrade
and the cluster never re-rendered. This is what the cozy-default
(system-bucket) flow hits on an in-place restore. Fixed by clearing
useSystemBucket in the restore patch, since the app is now on the
explicit-creds path. The driver's app view (postgresapp.Backup) gains
the field, without omitempty so a false survives the merge patch.
Both are exercised only by an in-place restore; the postgres backup e2e
drove only to-copy, so neither surfaced in CI. A step 50 in-place restore
now guards reason 1 (manual-strategy flow); reason 2 is covered by a Go
unit assertion (a system-bucket in-place e2e needs the platform bucket in
CI and is left as a follow-up).
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
(cherry picked from commit 5bcdb8cfb8)
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Assisted-By: LLM
Name all six proxy env spellings in the sanitisation list, and replace
the unestablished 'external HTTPS call' / 'flux-aio does not restart'
claims with the grounded in-cluster mechanism: after the
KUBERNETES_SERVICE_HOST drop above, the shard talks to the management
apiserver over the kubelet-injected ClusterIP, which NO_PROXY=.svc does
not match, so that startup call is what stalls behind an unreachable
proxy. Comment-only; no behaviour change.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
(cherry picked from commit 66df3b20e0)
Model the source liveness probe as flux-aio actually ships it (a bare httpGet
with no timing fields) and tighten the startupProbe assertions:
- assert the complete normalised budget (delay/period/success/failure) instead
of only FailureThreshold, so a later PeriodSeconds regression cannot silently
restore a short crashloop window;
- assert TimeoutSeconds is inherited from the source probe rather than a
hardcoded value, so forcing it stricter is caught;
- assert the startupProbe is a DeepCopy of liveness, not an alias, which would
otherwise stamp the 30-failure startup budget onto liveness and produce a
never-failing liveness probe.
The alias assertion is non-vacuous: it fails when the DeepCopy is regressed to
a plain alias.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
(cherry picked from commit a10fcb3e4a)
The BuildShardDeployment doc comment and the operator README are the only
record of how far the cloned shard container has diverged from flux-aio.
Add the two steps this fix introduced (dropping the inherited corporate-proxy
env and deriving a startupProbe) so the list stays the authoritative account
and does not invite re-adding what was deliberately removed.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
(cherry picked from commit 036a31b550)
Review follow-ups:
- Do not force the cloned startupProbe TimeoutSeconds to 1: keep the value
inherited from the liveness handler, otherwise a future flux-aio with a
larger liveness timeout would get a stricter startup probe and could
recreate the crashloop. Covered by a regression guard (liveness timeout 5s
must be inherited).
- Soften the proxy-strip rationale to the A/B-confirmed mechanism (blocking
external HTTPS call at startup) and note the remote spec.kubeConfig
exception that does not apply to cozystack in-cluster guest apiservers.
- Correct the test fixture comment: the proxy env is operator/patch-level, not
installer-injected (the installer injects only KUBERNETES_SERVICE_*).
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
(cherry picked from commit affdd70e9d)
The cloned shard Deployment inherits only a liveness probe (~30s window,
no startupProbe), so a controller still syncing caches on startup is killed
before it serves /healthz and never recovers. Derive a startupProbe from the
liveness handler with a generous budget so a slow start is not fatal, while
liveness still catches a wedged running pod.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
(cherry picked from commit d44c87d939)
A sharded helm-controller inherits HTTP_PROXY/HTTPS_PROXY/NO_PROXY from the
flux-aio helm-controller container. A standalone shard needs no external
egress (source-controller fetches artifacts), so behind an unreachable
corporate proxy it makes a blocking startup HTTPS call that never completes,
the manager never serves /healthz, and the liveness probe crashloops the pod
forever, freezing every HelmRelease carrying a sharding key. flux-aio itself
survives only because it starts once and never restarts.
Drop the proxy env (and the now-pointless NO_PROXY) when cloning, alongside
the existing localhost/KubePrism sanitisation, so the shard talks to the
in-cluster apiserver directly.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
(cherry picked from commit c475faae0f)
Address review on the S3 request-checksum fix:
- Extend the fix to the platform-managed useSystemBucket=true flow, whose
ObjectStore is built in Go by applyClusterPluginBackup (SSA-applied), not
chart-rendered. Add InstanceSidecarConfiguration to cnpgtypes.ObjectStoreSpec
and set AWS_REQUEST_CHECKSUM_CALCULATION=when_required on it via
barmanSidecarConfiguration(). The platform's own default system bucket is
SeaweedFS, a non-AWS S3 gateway of the same class as Ceph RGW. Unit test in
cnpgstrategy_controller_test.go asserts the applied ObjectStore carries it.
- Factor the duplicated instanceSidecarConfiguration block (3x across two
charts) into a shared cozy-lib helper (cozy-lib.barman.checksumSidecarConfiguration).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Andrey Kolkov <andrey.kolkov@aenix.io>
(cherry picked from commit b574e2176f)
Addresses the four findings on #3524.
**The `fail` in the bucket chart (MAJOR).** The <bucket>-<user>-credentials
Secret was made to `fail` the render while its COSI lookup was empty, so the
release would retry and converge instead of skipping the Secret permanently.
Helm cannot render a partial set, so that fail aborts the WHOLE
<bucket>-system release: the other users' Secrets, and the bucket UI
Deployment/Service/Ingress/HTTPRoute with them. One declared user whose
BucketAccess never provisions takes down every other user of that bucket,
and on an already-installed release parks it in Failed, blocking every later
upgrade.
**No per-bucket escape (MAJOR).** Rather than add one, remove the need for
it. The gate already forces the release that renders the lookup-gated
Strategy CRs; the credentials Secret is the same trap one release earlier,
and the gate reads that Secret to resolve the bucket name in the first
place. So while it is absent, or carries no bucket name, force the bucket's
<bucket>-system release instead of doing nothing. That cannot deadlock its
own precondition: the BucketClaim and the BucketAccess whose COSI Secret the
lookup reads are rendered unconditionally by the parent release, not by the
one being forced.
The two releases are throttled independently — they are forced in sequence
on a bootstrap, so a shared timestamp would delay the second by a full
MinForceInterval for no reason. Coordinates come from the chart under
provisionBucket, and are omitted on external S3 where the Secret is
admin-managed and no release renders it. No RBAC delta: patch on
helmreleases is already cluster-scoped.
The chart goes back to a partial render, with the comment now explaining why
the skip is not self-healing and why failing is the wrong lever, and a test
that renders the UI with every user unresolvable so a reintroduced `fail`
fails the suite. Tenant buckets keep their pre-existing behaviour;
generalising the repair to every bucket needs a controller that owns them
and is tracked separately.
**Stale gauge (MINOR).** cozystack_backup_default_objects_missing was only
written on the happy path, so the state the gate exists to catch reported 0
and the documented alert never fired. It is now written on the unresolved
path too — the credentials Secret counts as one of the objects the default
backups depend on, which is what it always was. On an API error the gauge is
deliberately left alone rather than flapping, so a new
cozystack_backup_default_objects_check_errors_total marks it stale, and the
doc pairs the two instead of overstating the gauge.
**Suspended releases (MINOR).** forceHelmRelease now does a point Get and
skips a release with spec.suspend: true. helm-controller ignores forceAt and
requestedAt while suspended (`cozyhr suspend` sets exactly that), so the gate
was re-stamping every MinForceInterval for the whole suspension and climbing
the force counter — which the runbook attributes to a render that is not
producing the objects, the wrong diagnosis. The skip is logged and not
counted, so a climbing counter keeps its documented meaning.
Reported-by: IvanHunters <xorokhotnikov@gmail.com>
Signed-off-by: Mattia Eleuteri <mattia@hidora.io>
Assisted-By: Claude <noreply@anthropic.com>
(cherry picked from commit f859b710a4)
The gate reads the BackupClass as the manifest of what must exist, but the Go
fixture listed only CNPG, Etcd and the two Velero routes while the shipped
backupclass-default.yaml also routes MariaDB and Altinity. Add both to the
fixture, and to the RESTMapper and list-kinds derived from it, so the
expected-object set the tests exercise equals the real route set.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
(cherry picked from commit 8b006391c2)
The BSL existence check used a hardcoded GVR and the dynamic client, whose
Get returns a plain 404 (IsNotFound) for an unserved API group. The
IsNoMatchError branch meant to skip an absent Velero API was therefore dead
code, and if Velero were uninstalled after bootstrap while bslEnabled=true
the BSL would be counted missing and force a Helm upgrade forever. Route the
lookup through the RESTMapper like the strategy loop, so an absent velero.io
API is a NoMatch we skip. A regression test fails without the change.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
(cherry picked from commit bf152ba864)
The cozy-default BackupClass routes VMInstance/VMDisk to the Velero
Strategy CRs unconditionally, but the chart gates those CRs on
velero.bslEnabled. With the BSL disabled they never render, so the gate
counted them as permanently missing and forced a Helm upgrade every
MinForceInterval forever, climbing force_reconciles_total and pinning the
missing gauge. Skip Velero-kind strategyRefs when the BSL is disabled
(empty VeleroNamespace), the same flag that gates their render. The
regression test now omits the manually-created Velero objects so it fails
without the skip. Correct the values.yaml comment that wrongly claimed the
Strategy CRs still ship in this mode.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
(cherry picked from commit aa3accd128)
Two defects found in review of the periodic recovery loop.
checkAndLog passed the manager's root context straight into Check, which
makes several sequential API calls. That context is only cancelled at
shutdown, so an API server stalling on any one of them blocked the call
indefinitely. The ticker loop is sequential, so one stuck call stopped
every later check for the life of the pod: the recovery this gate exists
to provide went silent, leaving a stale gauge and no further log line,
with only a restart to bring it back. Each check now runs under a
timeout of half the Period, capped at 30s, so it can never overlap the
next tick.
Check also propagated a NotFound on the projector's source Secret,
although the very next branch treats an empty bucket name as an expected
no-op. Those are the same bootstrap state seen a moment apart, so the
absent Secret logged a check failure on every tick of the whole
bootstrap window for a condition the doc comment frames as graceful.
Both are pinned by tests that fail against the previous behaviour: one
reverts to an error on the absent Secret, the other to a timeout equal
to Period.
Signed-off-by: Mattia Eleuteri <mattia@hidora.io>
(cherry picked from commit e85501e9f5)
The default Strategy CRs and the Velero BackupStorageLocation are
Helm-templated behind a `lookup` of the BucketClaim the same chart
creates, and the <bucket>-<user>-credentials Secret behind a `lookup` of
the COSI Secret. When those lookups are empty at install time the objects
were silently skipped, and helm-controller does not re-render a release
whose chart and values did not change (drift detection is off on
operator-generated HelmReleases), so the skip was permanent: clusters ran
for months with only BackupClass cozy-default and no Strategy CRs at all.
That also fail-closes the pre-adoption snapshot in the v1.6.0 etcd
migration, which reads the projected credentials Secret.
Add a DefaultObjectsGate runnable to backupstrategy-controller. Once the
bucket name is resolvable from the projector's source Secret, it checks
that every object cozy-default routes to exists and forces one real Helm
upgrade (reconcile.fluxcd.io/forceAt + requestedAt) when any is missing.
Helm remains the objects' only author; the gate only makes sure the render
that produces them actually happens. It cannot be a render-time `fail` in
this chart: the chart is the producer of the Bucket its own lookup reads,
so a failed render would deadlock the condition.
The bucket chart's user-credentials template CAN fail safely — the
BucketAccess that produces the COSI Secret belongs to the parent release
and that release retries forever — so make it fail loudly instead of
skipping, with requireUserCredentials=false as the offline-render escape.
Also correct the chart and doc comments that claimed Flux re-renders on
its interval, and document the two-release manual recovery for clusters
already affected.
Signed-off-by: Mattia Eleuteri <mattia@hidora.io>
(cherry picked from commit 51ad226706)
The controller discovers where to query SeaweedFS bucket size metrics
via the namespace.cozystack.io/monitoring label, which assumes the
monitoring stack that scrapes SeaweedFS runs in the same cluster. Some
deployments host SeaweedFS and its metrics in a separate cluster, so
add a --seaweedfs-metrics-endpoint flag to cozystack-controller (and a
matching cozystackController.seaweedfsMetricsEndpoint chart value): an
explicit base URL of a Prometheus-compatible query API, with scheme,
host, optional port and path prefix; /api/v1/query is appended. Basic
auth may be embedded as URL userinfo. The flag is validated and
normalized at startup, so a malformed URL fails the rollout rather
than the queries. Empty keeps label-based discovery unchanged.
Bucket sizes feed billing, so metrics failures no longer collapse to
"no size": previously any query failure yielded an empty result map
and the reconciler overwrote Workload resources with it, silently
wiping the recorded sizes. The reconciler now retains the last known
s3-storage-bytes and s3-physical-storage-bytes values, emits a
BucketMetricsUnavailable warning event on the WorkloadMonitor, and
returns the error from Reconcile so it lands in the reconcile error
metrics.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
(cherry picked from commit 95e4faf68e)
## What
Adds typed `waitStrategy` + `healthCheckExprs` fields so a package/app
can declare the Flux helm-controller v1.5.0 kstatus health gate for the
custom resource it wraps — making `HelmRelease.Ready` mean "the CR is
actually healthy" instead of "helm applied the CR". This is the
operator/apiserver foundation of the kstatus epic (#2642); the API
surface was reviewed and approved by the API owner in #3263.
Commits (per the review's "isolated step-0" request):
1. `build(deps)` — the dependency bump (see below).
2. `feat(api)` — the fields, plumbing, tests, and regenerated code.
3. `fix(api)` — match the openapi code-generator pin to the 0.35 runtime
(fixes an aggregated-apiserver crash the bump surfaced).
4. `fix(api)` — declare `OpenAPIModelName` on the apps types so SSA
resolves catalog kinds.
5. `docs(api)` — note that `disableWait` silently defeats
`healthCheckExprs`.
## API surface
Typed fields on **both** HelmRelease-building paths:
- `ApplicationDefinition.spec.release` (apiserver / catalog apps →
`convertApplicationToHelmRelease`)
- `ComponentInstall` (operator / platform packages →
`buildHelmReleaseSpec`)
```go
// WaitStrategy maps to HelmReleaseSpec.WaitStrategy.Name — a deliberate scalar
// simplification of the upstream {name} object. One of poller|legacy.
WaitStrategy string
// HealthCheckExprs maps to HelmReleaseSpec.HealthCheckExprs (the upstream
// kustomize.CustomHealthCheck type verbatim).
HealthCheckExprs []kustomize.CustomHealthCheck
```
`config.ResolveWaitStrategy` is shared by both builders and **couples
the default**: `healthCheckExprs` are only evaluated under the `poller`
strategy, so when expressions are set and no strategy is given, the
generated HelmRelease defaults to `poller`. A package that sets only
`healthCheckExprs` is therefore self-contained, independent of the
controller's global default.
## The dependency bump (commit 1)
The `WaitStrategy` / `kustomize.CustomHealthCheck` /
`HelmReleaseSpec.HealthCheckExprs` types do not exist below
`helm-controller/api` v1.5.0 — the runtime is already v1.5.0 (shipped by
#2602), so this closes a Go-types-vs-runtime skew. It is **not** an
isolated bump: v1.5.x requires **k8s 0.35 + controller-runtime 0.23**,
cascading the k8s stack 0.34 → 0.35 (and the go directive to 1.26).
cozystack replaces `k8s.io/apimachinery` with a fork carrying the
still-open upstream `ConvertToVersion` patch (k/k#135537). That single
commit rebases cleanly onto apimachinery v0.35.0; published at
`cozystack/apimachinery@release-1.35-pr135537`, and `go.mod` pins its
pseudo-version. **The repo builds with zero source changes under the new
stack.**
> Maintainer note: please confirm the `release-1.35-pr135537` fork
branch naming, or fold it into the `cozystack` branch as you prefer —
the pin can be updated to match.
## Not included (conscious YAGNI)
`disableWait` is intentionally omitted. 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.
## Tests
- `config.TestResolveWaitStrategy` — the couple-the-default logic
(unset+exprs→poller; explicit legacy honored; unset+no-exprs→nil).
- Operator + apiserver cases mirror each other
(`TestBuildHelmReleaseSpecHealthCheckExprs` /
`TestConvertApplicationToHelmRelease_HealthCheckExprs`).
- `go build ./...` clean, `go vet` clean, `gofmt` clean, codegen
regenerated and committed.
The 4 backupstrategy CRDs regenerate with benign k8s-0.35 PodSpec
description drift (DynamicResourceAllocation alpha→stable, toleration
Lt/Gt, resizePolicy note) — no functional change.
## Follow-up
Phase 3 (per-leaf adoption — postgres/kafka/mongodb/etc. declaring their
CEL via these fields) builds on this. The full CRD→CEL catalog is
maintained in the epic's readiness audit.
Refs #2642
```release-note
feat(api): add `waitStrategy` and `healthCheckExprs` to ApplicationDefinition and ComponentInstall, so a HelmRelease reports Ready only once the custom resource it wraps is actually healthy
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **New Features**
* Added configurable CEL health-check expressions for application
releases and package components.
* Added `waitStrategy` support (`poller` and `legacy`) with automatic
`poller` selection when health checks are configured and no strategy is
set.
* Propagated these settings into generated Helm releases, affecting
readiness/wait behavior.
* **Documentation**
* Updated Application and PackageSource schemas to include
`healthCheckExprs` and `waitStrategy`.
* Extended related Kubernetes CRD documentation, including projected pod
certificate annotations and workload reference fields.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Two defects of the same class — the WAL-archive S3 prefix (serverName)
changing or defaulting differently between components:
Recovery default regression (review blocker): native CNPG defaulted an
absent externalClusters serverName to the entry name (= bootstrap.oldName),
but the barman-cloud plugin defaults it to the NEW cluster's name, so a
legacy-flow restore with bootstrap.serverName unset would read the wrong
S3 prefix (hard failure, or silently restoring another cluster's data).
The chart now always emits serverName, defaulting to bootstrap.oldName.
Live-cluster serverName flip (found by the gated postgres backup e2e on a
real cluster): the chart renders spec.plugins without serverName
(effective prefix = cluster name) while a strategy template may name a
different one; the driver's ForceOwnership patch then flipped the prefix
mid-stream, splitting the WAL archive across two prefixes — the WAL
holding the backup's start was lost between them and the restore failed
with "WAL not found". The driver now preserves the live Cluster's
effective serverName (explicit parameter, or the cluster name when the
plugin is attached without one) and only applies the strategy's value to
a Cluster with no plugin attached; both charts pin serverName explicitly,
and the example strategy no longer sets one.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
Review follow-up (cozystack#3300): ensureCNPGBackup stamps spec.method=plugin +
spec.pluginConfiguration on the postgresql.cnpg.io/Backup it creates — the field
that routes a driver-initiated run through the barman-cloud plugin. If it
regressed to the default (barmanObjectStore) method, CNPG would run the legacy
method against a Cluster that no longer has one and every platform backup would
fail. Add TestEnsureCNPGBackup_UsesPluginMethod asserting method, plugin name,
and cluster ref. (The chart ScheduledBackup path was already covered; the driver
path was not.)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
Review follow-ups (cozystack#3300):
- Lifecycle/leak: applyClusterPluginBackup now sets an ownerReference from the
driver-created barmancloud.cnpg.io/ObjectStore to its cnpg.io/Cluster, so
Kubernetes garbage-collects it when the Cluster is deleted (tenant app
teardown or the RestoreJob purge). In the platform useSystemBucket flow the
chart does not render this ObjectStore, so previously it was orphaned in the
tenant namespace. Drop the now-accurate-only 'delete' verb on objectstores
from the driver RBAC (GC handles removal; the driver never deletes it) and
correct the RBAC comment that falsely claimed the purge deletes it.
- Diagnostics/comments: the RestoreJob WAL-archive-deadline failure message and
several doc comments still told operators to check spec.backup.barmanObjectStore
/ externalClusters[].barmanObjectStore, fields no longer emitted after the
migration. Point them at the barman-cloud plugin (spec.plugins + ObjectStore,
externalClusters[].plugin) instead so they don't mislead during an incident.
- Test: TestApplyClusterPluginBackup_PatchesExistingCluster now asserts the
ObjectStore carries an ownerReference to the Cluster (kind/name/uid).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
## What
Removes the `network.cozystack.io` API group — `ExposureClass` +
`ServiceExposure` (#3081) — and its reconciler, in favor of native
`Service` `type: LoadBalancer` + `loadBalancerClass`.
## Why
Full rationale and discussion in #3164. For **managed applications** the
chart already owns the Service, so:
- `type: LoadBalancer` + `loadBalancerClass` + an admin-provisioned pool
covers vendor-neutral external exposure without a dedicated API group
and controller;
- the `externalIPs` (node-IP) default never went through
`ServiceExposure` (its backend is a no-op) and is left unchanged;
- the only current consumer, the host ingress, keeps rendering `type:
LoadBalancer` and just drops the `ServiceExposure`; the admin provisions
the address pool / announcer out of band, as the install docs already
assume.
`ServiceExposure`'s one unique feature — dynamic per-namespace scoping
of a shared VIP pool — is a convenience the managed-app path does not
need, and it does not justify a second tenant-networking API group next
to `sdn.cozystack.io`.
## Changes
- Delete `api/network/v1alpha1` and
`internal/controller/serviceexposure` (+ CRD definitions, e2e bats,
controller registration, RBAC).
- Host ingress: drop the `ServiceExposure` template; add an optional
`publishing.loadBalancerClass` that sets `spec.loadBalancerClass` on the
host-ingress LoadBalancer Service.
- The `externalIPs` path and default behavior are unchanged.
## Verification
- `go build ./...` — green.
- `helm unittest` — `core/platform` 72/72, `extra/ingress` 25/25 (incl.
host-vs-tenant `loadBalancerClass` scoping and the immutable-field
migration guard).
## Migration
None required. `network.cozystack.io` (#3081) has never shipped in a
release — it is on `main` only (not backported to `release-1.5`; absent
from v1.4.x / v1.5.x), so no upgrading cluster has these CRDs and this
removes it before it is ever released.
Clusters that track `main` and explicitly set `publishing.exposureClass`
(opt-in, only since 2026-06-30) should provision their MetalLB / Cilium
address pool themselves and set `publishing.loadBalancerClass`; any
orphaned `cozystack-<class>` pool can be deleted by hand. Default
clusters (externalIPs path) are unaffected.
## Status
**Draft / RFC for the #3164 discussion** — this removes an API group
that landed only days ago, so nothing here should merge before the
design call. cc @lexfrei (author of #3081) and @lllamnyp (#3164).
Deferred: when Kubernetes actually removes `Service.spec.externalIPs`,
the node-IP default migrates via a MetalLB `IPAddressPool` with no
`L2Advertisement` (allocate-but-don't-announce) — a separate change.
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **New Features**
* Added support for configuring the ingress host Service with a native
`loadBalancerClass` value.
* Expanded chart tests to verify class wiring and immutability behavior
across install and upgrade scenarios.
* **Bug Fixes**
* Removed the legacy exposure-class/service-exposure flow and related
controller behavior.
* Updated templates and CRD packaging so the platform no longer renders
or depends on those deprecated resources.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
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 review feedback on the repeat in-place restore fix:
- Make the freshness discriminator strict (creationTimestamp > StartedAt) so
an exact tie resolves to "not fresh" and the caller purges, matching the
function's stated conservative default; a fresh Cluster is always created
well after StartedAt so this never misclassifies the status-write-race path.
- Document why a wall-clock timestamp comparison was chosen over recording the
purged Cluster's UID on the TargetPurged condition (the UID approach leans on
the same status write the race path assumes can fail).
- Add a reconcile-level regression test that drives reconcileCNPGRestore end to
end with a fake client: a stale leftover recovery Cluster from a prior restore
is purged, and a Cluster this restore just re-created is not re-purged. This
covers the actual call site where #3311 lived, not just the pure helpers.
Signed-off-by: Ivan Okhotnikov <xorokhotnikov@gmail.com>
A repeat in-place PostgreSQL RestoreJob into an already-restored target
silently no-opped: the destructive purge was skipped because the live
Cluster still carried spec.bootstrap.recovery left over from the previous
restore, and the re-purge guard mistook that for a freshly-recovered
Cluster produced by this restore's own purge. The job then reported
Succeeded while the PVC, disk, and data were never touched.
Tell the two apart by the Cluster's creationTimestamp relative to the
RestoreJob's StartedAt: only a recovery Cluster created at/after StartedAt
is treated as this restore's own and skipped; one that predates StartedAt
is leftover from an earlier completed restore and is purged so the restore
re-bootstraps from the backup.
Signed-off-by: Ivan Okhotnikov <xorokhotnikov@gmail.com>
## What this PR does
The operator-provided wildcard certificate
(`publishing.certificates.wildcardSecretName`) currently serves the root
tenant only: everything is same-namespace, so the root ingress
controller and Gateway read the operator's TLS Secret directly. Child
tenants run their own ingress controller / Gateway in their own
namespace under a default-deny policy and cannot read that Secret across
namespaces, so they fall back to per-host ACME.
This propagates the operator wildcard to per-tenant termination points
by replicating the Secret into each tenant namespace that owns one,
keeping the root-MVP invariant intact: the wildcard is deliberately
distributed to tenant namespaces, but no tenant RBAC is widened — every
consumer reads only its own-namespace copy.
A new controller in `cozystack-controller` does the replication with no
extra operator input. It reads the same platform values channel the
consumers read (`cozy-system/cozystack-values`): the wildcard Secret
name from `_cluster.wildcard-secret-name` and the publishing namespace
from `_cluster.expose-ingress`. It mirrors that Secret into every tenant
namespace whose `namespace.cozystack.io/ingress` or
`namespace.cozystack.io/gateway` label equals its own name — i.e. the
namespace runs its own controller / Gateway. Because the source is
derived from the same value that makes the consumers reference it, the
replica is created whenever the consumers expect it — no manual
labelling and no upgrade hazard where a child controller references a
Secret that nothing creates.
Replicas are refreshed on rotation and pruned when a namespace stops
terminating TLS. Teardown of every replica happens on exactly one
trigger: clearing `wildcardSecretName` (explicit disable). A source that
is merely absent or mistyped, or a transiently-missing values channel,
leaves existing replicas in place, so a brief gap, a delete+recreate
rotation, or a misconfigured publishing namespace never drops tenant
TLS. The platform controller's own ServiceAccount gains Secret writes;
tenant RBAC is untouched.
The controller does not cache every Secret in the cluster: the manager's
Secret informer is scoped to managed replicas and the values channel
only, and the operator source / foreign collisions are read through the
uncached APIReader. The source is not watched (its name is dynamic), so
an in-place rotation is picked up by a bounded periodic resync while the
publishing tenant serves the source with no lag.
On the consumer side, each tenant ingress controller now sets
`--default-ssl-certificate` to the wildcard Secret in its own namespace
(`.Release.Namespace/<name>`), so a child tenant serves the replica
instead of minting per-host ACME. The Gateway path already renders
`certMode: existingSecret` with a same-namespace `wildcardSecretRef` for
every tenant, so it consumes the replica with no chart change.
### Why not clustersecret-operator
The issue proposed the shipped `clustersecret-operator`. It materializes
inline template data into selected namespaces; it cannot mirror an
existing source Secret, and it is not installed in any default bundle.
Driving it would mean embedding the certificate and private key as
literal data in a cluster-scoped object rendered from a Helm `lookup` —
that broadcasts key material the design keeps off any shared channel,
and a `lookup` is invisible to the Flux helm-controller digest so it
would not re-render on rotation. Its namespace selector also cannot
express "the namespace whose owner label equals its own name", so it
would over-replicate the key into every tenant namespace. A small
dedicated reconciler targets exactly the termination namespaces and
keeps the key off the values channel.
### Coverage caveat
Replication delivers the certificate bytes, not SAN coverage: a single
`*.<root-host>` does not match `*.<child-apex>`. Operators serving
child-tenant subdomains supply a certificate whose SANs cover those
apexes; the docs spell this out. The replica carries the wildcard
private key into each terminating tenant namespace — the same exposure
as the per-host ACME Secret it replaces, now shared across tenants.
### Tests
- Go fake-client tests for the reconciler: termination-only replication
(ingress-owning and gateway-owning namespaces only, never inheriting
ones, never over the source), copy markers, rotation, prune on namespace
demotion, the non-destructive source-deletion contract, replica
self-heal after out-of-band deletion, the publishing-namespace
self-replication guard, disable-via-config teardown, source rename,
foreign-Secret collision safety (no overwrite, no requeue, does not
block siblings), transient-error requeue, non-TLS source ignored,
absent-channel keeps replicas, publishing-namespace mismatch keeps and
requeues, the cache-scope pin, the enqueue-wiring map, an
`ownsTerminationPoint` table, and the active-only requeue contract.
- helm-unittest for the ingress `default-ssl-certificate` wiring
(including a child-tenant controller pointing at its own-namespace
replica) and for the cozystack-controller ClusterRole's new Secret-write
rule.
### Screenshots
N/A — no UI changes.
### Release note
```release-note
feat(platform): the operator-provided wildcard TLS certificate (`publishing.certificates.wildcardSecretName`) is now served by per-tenant ingress controllers and Gateways automatically — the platform controller replicates it into each tenant namespace that terminates TLS, with no extra operator input and no cross-namespace Secret read
```
Closes#2820
Part of #2811
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
## Release Notes
* **New Features**
* Wildcard TLS Secret now replicates from the platform to tenant
namespaces that terminate TLS via their own ingress controller or
Gateway, including child-tenant scenarios.
* **Documentation**
* Clarified configuration, timing, and SAN matching requirements for
wildcard replication.
* Updated gateway and ingress-nginx default certificate documentation
for replica mirroring behavior.
* **Bug Fixes**
* Improved handling for missing/invalid source secrets (no unnecessary
pruning) and safe collision behavior.
* **Tests**
* Added comprehensive reconciliation/replication test coverage.
* Added Helm unittest target for the controller chart.
* **Chores**
* Updated controller RBAC to support wildcard TLS Secret replication.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Reconcile the Chainsaw e2e migration with ~500 commits of main drift:
- keep main's run-kubernetes.sh helpers (kubectl_wait_retry, tenant
drain, LINSTOR pool-free wait, SC-fallback probe) under the branch's
_lib/ rename with the Chainsaw finally/snapshot deltas re-applied
- resolve modify/delete on 10 drifted hack/e2e-apps/*.bats as deleted;
their post-branch assertion drift is re-ported into the Chainsaw
suites in follow-up commits
- drop the 4 new main-side bats (kubernetes-oidc-*, securitygroup,
serviceexposure) from _lib/ relocation; ported as Chainsaw suites in
follow-up commits
- docs/agents/e2e-testing.md: merge main's plan-job TIA wording with
the Chainsaw suite phrasing
Assisted-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Remove the network.cozystack.io API group (ExposureClass + ServiceExposure)
and its reconciler. Managed-application charts own their Service, so native
type: LoadBalancer + loadBalancerClass + an admin-provisioned pool covers
external exposure without a dedicated API group and controller. The host
ingress -- the only consumer -- keeps rendering type: LoadBalancer and gains
an optional publishing.loadBalancerClass to pick the LB controller; the
externalIPs (node-IP) default path is unchanged.
Rationale in cozystack#3164.
Assisted-By: Claude
Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
Native Go fuzzing over the flag parsers in config.go, asserting the
invariants that matter rather than just no-panic:
- ParseShardIndex round-trips with ShardName (accepted input is the canonical
name of the returned index; index is non-negative).
- ShardName round-trips back through ParseShardIndex for every valid index.
- ParseShardCount: a non-auto success yields count >= 1; auto yields 0.
- ParsePinnedTenants: every entry in a success has a non-empty key mapped to a
valid shard name.
Adds an OSS-Fuzz-detectable fuzzing signal for the OpenSSF Scorecard Fuzzing
check while covering parsers that sit on the sharding hot path.
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Assisted-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
## What this PR does
Closes two coverage gaps found while shadow-observing the
API-owner-review gate (#3167) against real PRs in the first hours after
merge:
1. **CRD discovery was gated on directory name** (`crds/`,
`definitions/`, `manifests/`), so a CRD shipped inside a chart's
`templates/` dir — a valid, existing convention when the CRD is wrapped
in a `{{- if }}` guard — was invisible to the gate. This was not
hypothetical: `#3149` (cozyplane) adds four brand-new CRDs (`VPC`,
`VPCBinding`, `VPCPeering`, `Port`) exactly this way, and none of them
tripped a review requirement.
2. **Groups served only by an external aggregated apiserver had no
representation at all.** The same `#3149` also registers
`sdn.cozystack.io` via a `kind: APIService` pointing at a prebuilt image
built outside this repository — no CRD, no vendored Go types, nothing to
diff.
### Fix
- CRD/APIService discovery is now content-based: every `.yaml`/`.yml`
file under `packages/` and `internal/` is a candidate, and whether it's
kept depends on whether it parses into the right `kind` with a
first-party group — not which directory it sits in.
- Both parsers now tolerate a Helm directive line (`{{- if }}`, `{{- end
}}`, a comment block) sharing a `---`-delimited document with
otherwise-valid YAML, rather than dropping the whole document. This
matters because the closing `{{- end }}` of a wrapping conditional has
no `---` before it, so it lands in the same document as the manifest it
closes.
- Added a fourth resource source, `SourceAPIService`, for `kind:
APIService` manifests registering a first-party group. Like the existing
static Go-backed groups, these carry no checked-in schema, so they
participate only in new-group / new-resource / removal detection — never
a fabricated breaking-change diff against a schema this repo can't see.
Verified against the real `feat/cozyplane` branch: the gate now reports
all four new CRDs, plus the removal of the old
`securitygroups`/`APIService` registration for the same group — a full
API-surface migration that was previously invisible end to end.
### Release note
```release-note
fix(ci): the API-owner-review gate now discovers CRDs and APIService registrations by content instead of directory name, closing a gap where a CRD or an externally-served aggregated API group could ship without triggering the required review.
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **New Features**
* Discovery now includes APIService registrations, enabling detection of
groups/versions exposed only via aggregated APIService manifests
(including merging versions from multiple templates).
* **Bug Fixes**
* Improved YAML discovery to tolerate mixed/templated multi-document
streams: non-CRD/non-APIService documents no longer stop discovery, and
embedded template directives are handled more reliably.
* File processing is more resilient during snapshot loading (includes a
safety cap on YAML size).
* **Tests**
* Added coverage for APIService discovery, templated neighbor documents,
and correct exclusion of third-party manifests.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
branch-review on #3184 measured a real regression: switching CRD/
APIService discovery to content-based scanning made every file in
packages/+internal a candidate, and each was split into documents and
fully unmarshaled twice — once per parser — inflating a full snapshot
load from ~0.5s to ~3.4s.
Split each file's documents once and probe each document's kind once
before deciding which shape to try, so the common case (a document
that is neither a CRD nor an APIService — the overwhelming majority
of what the wider walk now sees) pays for at most one parse attempt
instead of two. Add a raw substring fast-reject, at both the
per-document and whole-file level, ahead of any YAML parsing: neither
kind this package cares about can appear without its own name
appearing as a literal substring first.
Also fixes two smaller gaps flagged in the same review: an untrusted
PR head checkout could point discovery at an arbitrarily large file
with no size guard (capped at 8 MiB), and a group's merged APIService
resource only recorded the first contributing file's path, dropping
the others from the report.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
Shadow-observation of the merged gate found a real miss: a CRD shipped
inside a chart's templates/ dir (guarded by a Helm {{- if }}) was
invisible because discovery was gated on directory name (crds/,
definitions/, manifests/), and a group served only by an external
aggregated apiserver (kind: APIService, no CRD, no vendored Go types)
had no representation at all.
Switch CRD/APIService discovery from directory-name to content
sniffing, and tolerate Helm directive lines sharing a document with
otherwise-valid YAML (the closing `{{- end }}` of a wrapping
conditional has no leading `---`, so it lands in the same document as
the manifest). Add a fourth source, SourceAPIService, reusing the
existing empty-schema mechanism so a first-party group's first
APIService appearance requires review without pretending to diff a
schema that lives outside this repository.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Timofei Larkin <lllamnyp@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>
IvanHunters