[Backport release-1.6] fix(backupstrategy-controller): repair lookup-gated backup objects - #3731
Merged
Merged
Conversation
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)
11 tasks
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Backport of #3524 to
release-1.6.