Skip to content

[Backport release-1.6] fix(backupstrategy-controller): repair lookup-gated backup objects - #3731

Merged
myasnikovdaniil merged 6 commits into
release-1.6from
backport-3524-to-release-1.6
Aug 18, 2026
Merged

[Backport release-1.6] fix(backupstrategy-controller): repair lookup-gated backup objects#3731
myasnikovdaniil merged 6 commits into
release-1.6from
backport-3524-to-release-1.6

Conversation

@github-actions

Copy link
Copy Markdown

Description

Backport of #3524 to release-1.6.

mattia-eleuteri and others added 6 commits August 10, 2026 10:10
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 51ad226)
… absent Secret

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 e85501e)
… when velero.bslEnabled=false

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 aa3accd)
…ESTMapper

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 bf152ba)
…n the gate fixture

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 8b00639)
…from the gate, not a chart fail

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 f859b71)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants