## What this PR does
Backport of #4671 and #4672 to release-1.6.
- #4672 builds on #4671, so both go in one PR, picked in order.
- The test file needs the `errors` and `strings` imports on release-1.6,
one conflicting test hunk in #4672 is resolved to the original's side.
### Screenshots
Not a UI change.
### Downstream repositories
The website v1.6 backup-and-recovery page tells users to delete
`*-orig-*` PVCs to free storage, which a Retain PV does not do, and does
not mention the new reclaim-policy annotation, I will open a website
issue for it.
- [ ] No downstream repository is affected by this change
- [ ] [cozystack/website](https://github.com/cozystack/website) -
follow-up:
- [ ]
[cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack)
- follow-up:
- [ ]
[cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack)
- follow-up:
- [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up:
- [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up:
- [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) -
follow-up:
- [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) -
follow-up:
- [ ]
[cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server)
- follow-up:
- [ ]
[cozystack/external-apps-example](https://github.com/cozystack/external-apps-example)
- follow-up:
- [ ] [cozystack/examples](https://github.com/cozystack/examples) -
follow-up:
### Release note
```release-note
fix(backups): an in-place VM restore with keepOriginalPVC now fails, before the VM is halted, when the <disk>-orig-<hash> PVC it would keep the disk under already exists and was not created by this RestoreJob (for instance one left by a deleted RestoreJob of the same name), instead of skipping the rename and restoring without keeping the current disk. Any other failure to keep a disk, including a race in which the rename read its just-created -orig PVC back through the cache and failed, now holds the restore back and retries it before its DataVolume is deleted. Previously such a restore reported success without restoring the disk. The PV's reclaim policy before the switch to Retain is recorded in the backups.cozystack.io/original-reclaim-policy annotation on the PV and the -orig PVC; set it back on the PV before deleting the -orig PVC, or its volume is not reclaimed.
fix(backups): an in-place VMInstance RestoreJob with keepOriginalPVC=false now deletes each original disk PVC, and waits for it to be gone, before Velero restores it. Previously the PVC was left in place, Velero skipped restoring its data, the RestoreJob still reported Succeeded, and the disk's HelmRelease could never recreate its DataVolume ("Destination PVC already exists"). A Velero RestoreJob now also fails before touching the application when its Velero Backup is gone or not Completed, for example after the Backup expired at its TTL.
```
Deleting the original PVC of a keepOriginalPVC=false restore is only
safe when Velero can put the disk back. The Backup artifact outlives
the Velero Backup it points to, which Velero garbage-collects at TTL,
and nothing checked it before prepareForRestore halted the VM and
deleted the disks: a restore of an expired backup lost the volume where
it used to fail with the disk intact. The Velero Backup must now exist
and be Completed before any preparation starts.
A DataVolume that survives its deletion makes CDI recreate the claim
under the original name, which Velero then skips as an existing PVC,
so the restore would end Succeeded on a freshly imported disk. A failed
DataVolume deletion now fails the restore, and the PVCs are deleted
only once every DataVolume is gone. Reading the DataVolume back needs
get on datavolumes, which the ClusterRole did not grant.
Assisted-by: LLM
Signed-off-by: Mattia Eleuteri <mattia@hidora.io>
(cherry picked from commit 6d064a8602)
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
With keepOriginalPVC=false the restore deleted each DataVolume with
Orphan propagation and left its PVC in place for Velero to overwrite.
Velero never overwrites a PVC: its CSI restore action skips the data
mover for a claim that already exists, and existingResourcePolicy=update
fails to patch the immutable spec of a bound claim, so it only stamps
its labels on it. The RestoreJob then reports Succeeded with the disk
still holding its pre-restore data.
The claim left behind also breaks the disk for good. It has lost its
owner reference to the orphaned DataVolume, and it lacks the
cdi.kubevirt.io/storage.populatedFor annotation that the backed-up copy
carries, so the CDI webhook refuses the DataVolume the resumed
HelmRelease creates ("Destination PVC already exists"), on every
reconcile.
Delete the PVC once its DataVolume is gone, and hold the Velero Restore
until the claim has actually disappeared: one still mounted stays
Terminating behind pvc-protection, and Velero would take it for an
existing claim again.
The test file conflicted on release-1.6 only in the line the new helper
replaces, which this branch spells boolPtr(false) where main spells
new(false); the helper and both tests are taken as they are on main.
Assisted-by: LLM
Signed-off-by: Mattia Eleuteri <mattia@hidora.io>
(cherry picked from commit 6ea713d09b)
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
renamePVC created the -orig PVC, then read it back through the cached
client to learn its UID. The informer had often not seen the PVC yet, so
the read failed with NotFound. On a two-disk VM restore one disk was
kept and the other was not; with that error turned into an event, its
DataVolume was deleted, the original PVC stayed in place and Velero
skipped it, and the restore reported success without restoring the
disk. The -orig PVC was left Pending on the same PV, by then Retain.
Create already returns the UID, so the read-back goes. The other reads
the rename decides on, the -orig and original PVCs and the PV, go
through the manager's uncached reader, and the PV rebind retries on
conflict, since the PV controller writes the PV once the original PVC
is deleted.
A rename that fails for any reason other than an -orig name taken by
another PVC now requeues the restore instead of failing it. The
DataVolume is still there, and the retry finds its own -orig PVC by the
RestoreJob UID and resumes where it stopped.
Assisted-by: LLM
Signed-off-by: Mattia Eleuteri <mattia@hidora.io>
(cherry picked from commit cc7f98142a)
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
renamePVC returned early whenever a PVC named <dv>-orig-<hash> already
existed, without checking it came from this rename. The hash covers
only the RestoreJob name, so a RestoreJob created under the name of a
deleted one finds the -orig PVC its predecessor left behind. The rename
was skipped, the DataVolume deleted anyway, and the restore ran as if
keepOriginalPVC were false while reporting success: the live disk was
not kept, and the -orig name held an older one. Any other rename
failure ended the same way, since its error only became an event.
A -orig PVC now records the UID of the RestoreJob that created it. A
name held by any other PVC fails the restore before the VM is halted,
an interrupted rename of our own resumes instead of stopping halfway,
and a rename failure fails the restore before the DataVolume goes.
The PV is switched to Retain so it outlives the original PVC, and the
policy it had was discarded. Nothing in the controller deletes the
-orig PVC or restores that policy, so a PV that was Delete stayed
Retain and its volume leaked once an operator removed the claim. The
policy is now recorded on the PV and on the -orig PVC, and the restore
scenario documents how to set it back.
On release-1.6 the test file also imports strings and the apimachinery
errors package, which main's copy had already gained from earlier
commits that are not on this branch.
Assisted-by: LLM
Signed-off-by: Mattia Eleuteri <mattia@hidora.io>
(cherry picked from commit d8d9608e22)
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
The Velero driver resolves a restore's target from the Backup's
applicationRef, overriding only the kind and name, and restores the
Backup's resources. A targetApplicationRef.apiGroup never reaches it.
The restore gate honoured that group all the same, so a Velero
RestoreJob naming another group let a BackupJob of the application
actually being restored start against it, and held one of an
application the restore never touches.
Assisted-by: LLM
Signed-off-by: Mattia Eleuteri <mattia@hidora.io>
(cherry picked from commit 28e42fbe4d)
Nothing on the backup path looks at restores. A Plan creates its
BackupJobs on schedule, and the BackupJob reconciler dispatches them to
their driver whatever else is happening to the application. For CNPG that
is not merely a backup of a half-restored state: every BackupJob reconcile
re-applies the Cluster's barman plugin, and on a cluster under an in-place
restore it can land after the chart has re-rendered the Cluster with its
new serverName and put it back on the source's. The restored cluster then
archives onto the source's WAL prefix and its recovery fails with "WAL
archive check failed ... Expected empty archive". This was reproduced
with a backup triggered a few seconds after an in-place restore, and an
hourly plan overlaps a restore of up to thirty minutes about half the
time.
A BackupJob that has not started yet now waits while a non-terminal
RestoreJob in its namespace writes into the same application, resolved
as the drivers resolve it: the Backup's applicationRef, with the target's
kind and name when a targetApplicationRef is set, and the target's API
group only when it names one. A RestoreJob whose Backup is gone holds
nothing, since the restore controller fails it on its next pass.
options.targetNamespace is honoured only for a Backup taken by the Velero
strategy, the one driver that reads it; the CNPG and Job-based drivers
ignore the option and restore in place, so their restores hold whatever
it says.
The held job stays Pending with a Ready=False RestoreInProgress condition
naming the restore and is requeued, rather than failed, so a scheduled run
still happens once the restore ends. A failed write of that status is
returned, so the manager retries it. When the restore ends the condition is
removed before the job starts, since it would otherwise read "waiting"
until the driver next writes Ready, which for a healthy run is when the
backup completes. A run that has already started is left alone: its driver
owns state the gate would strand.
Assisted-by: LLM
Signed-off-by: Mattia Eleuteri <mattia@hidora.io>
(cherry picked from commit af8cf743cd)
The refusal pointed every kind at "the application's engine-native
strategy", but VMDisk, the only non-VMInstance kind the default
BackupClass routes to Velero, has none. Offer that remedy only to kinds
that can have one.
The check also runs on every reconcile, so a rename RestoreJob that was
already Running when the controller picked up the check is failed after
its Velero Restore has completed. That job now says the application may
already sit in the target namespace under its source name (a retry that
drops the rename would otherwise trip failIfTargetExists on that copy
without explanation), and its resource-modifier ConfigMap is released
right away instead of at the job's deletion.
Assisted-By: LLM
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
(cherry picked from commit d50385f310)
The guard's regression cases only exercised renames, so dropping the
IsRenamed conjunct (failing every non-VMInstance restore) or keying the
check on targetApplicationRef.kind instead of the backup's kind left the
package green. Add the inputs the guard must let through, a VMDisk copy
that keeps its name and a VMDisk in-place restore without
targetApplicationRef, and a VMDisk backup restored with a VMInstance
target kind, which must still be rejected by the source kind.
Assisted-By: LLM
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
(cherry picked from commit 11bac54f93)
A RestoreJob that set targetApplicationRef.name to a new name and
options.targetNamespace to another namespace reported Succeeded for any
Velero-backed application, but only a VMInstance was actually renamed.
postRestoreRename looks up the HelmRelease under the vm-instance- prefix,
gets NotFound for every other kind and returns nil, so a Postgres,
MariaDB, ClickHouse, MongoDB or standalone VMDisk copy came up under the
source name with no error surfaced.
Renaming those kinds through the Velero strategy cannot work: DataUpload
restores volumes under their original names, and only a VMInstance keeps
using them after its HelmRelease is renamed, because its disks keep their
names and CDI adopts the restored PVCs. A renamed database Cluster or
VMDisk renders fresh volumes and comes up empty.
Reject the rename up front for every kind other than VMInstance, before
the RestoreJob enters Running, with a message that points at omitting
targetApplicationRef.name or using the engine-native strategy. The
existing same-namespace guard keeps precedence, so its message is
unchanged.
Closes#3537
Assisted-By: LLM
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
(cherry picked from commit 88073bb209)
applyClusterPluginBackup preserves the serverName already on the Cluster, so
that a backup does not flip the archive prefix mid-stream. It read that value
through the manager's cached client, and this controller registers
For(&BackupJob{}) and nothing else, so a Cluster only reaches the cache
through a lazily started informer with no resync tied to its own changes.
An in-place RestoreJob deletes the Cluster and the chart re-renders it with
bootstrap.newServerName seconds later. A read served during that window
describes the object that was purged, so the source's serverName is written
back over the freshly rendered one and the restored cluster archives onto the
source's WAL prefix. barman-cloud-check-wal-archive then refuses to start with
"Expected empty archive" and the RestoreJob ends on its deadline -- the
failure newServerName was added to prevent.
The stale read carries a stale UID too, and that UID owns the ObjectStore this
function applies, so the store is garbage-collected as soon as it is written.
Reading through the dynamic client keeps the preservation rule and makes the
value it rests on truthful.
Assisted-by: LLM
Signed-off-by: Mattia Eleuteri <mattia@hidora.io>
(cherry picked from commit 67bf9ca317)
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
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)
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
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)
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>
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>
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>
Rewrite docs/agents/e2e-testing.md for the Chainsaw suite: declarative
asserts replace the until-get/wait backstops, Chainsaw cleanup replaces
test-level EXIT traps, catch blocks replace the `|| { ...; false; }` idiom,
and the TIA/diagnostics sections describe suites + `make test-chainsaw
CHAINSAW_SUITES`. Move the BATS->Chainsaw bullet out of 'in-flight' since it
is now the standard.
Fix references the migration left stale: hack/e2e-apps/ -> hack/e2e-chainsaw/
in the gemini styleguide and the nats/etcd backup READMEs. The
backup-postgres.bats / backup-foundationdb.bats files cited by the postgres
examples, the postgres README, and the foundationdb controller comment never
existed -- drop the false 'CI e2e drives this' claims and describe those
backup flows as the manual reference flows they are.
Assisted-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
## What
Implements the `strategy.backups.cozystack.io/Job` backup strategy — a
generic, application-agnostic driver that runs a user-supplied
Kubernetes Job to create (and restore) a backup.
## Background
This PR began as a draft written against an earlier shape of the backups
API. The API has since moved to a `BackupClass`-based dispatch model,
which broke the original draft on three counts:
- `BackupJob.Spec.StrategyRef` and `BackupJob.Spec.StorageRef` were
removed; the strategy and its parameters are now resolved from a
`BackupClass` into a `ResolvedBackupConfig`.
- Strategy handling moved from standalone per-strategy reconcilers to a
dispatch model: `BackupJobReconciler.Reconcile` /
`RestoreJobReconciler.Reconcile` resolve the strategy and switch into
per-strategy methods (`reconcileVelero`, `reconcileCNPG`,
`reconcileAltinity`, …).
- The draft's template context (`.Values`, `.Storage`) diverged from the
convention the other strategies adopted, and its `.Storage` half
targeted a storage API that does not exist.
## What changed
- **`reconcileJob` / `reconcileJobRestore`**
(`internal/backupcontroller/jobstrategy_controller.go`): replaces the
standalone `BackupJobStrategyReconciler` with the two dispatch methods
the `BackupJob` / `RestoreJob` reconcilers call. The Job strategy is the
generic counterpart of the Altinity driver — same lifecycle (render
`PodTemplateSpec` → run a one-shot `batch/v1.Job` → poll its terminal
condition → emit a `Backup`), minus the ClickHouse-specific
`applicationRef` gate, so it runs for any application. Shared package
helpers (`renderJobTemplate`, `jobConditionState`,
`getApplicationUnstructured`, status helpers, deterministic Job naming,
controller refs) are reused.
- **Template context** follows the in-tree convention used by the other
drivers: `.Application` (live application object), `.Release.Name` /
`.Release.Namespace`, `.Mode` (`backup` / `restore`), `.Parameters`
(resolved `BackupClass` parameters), and `.Backup` (restore runs only).
Strategy parameters round-trip through the `Backup`'s `driverMetadata`
so a later restore re-renders against the same values.
- **Restore** re-renders the same template with `.Mode == "restore"`.
Cross-namespace restore is unsupported (the RestoreJob target reference
is namespace-local), matching the Altinity driver.
- **`JobSpec.Template` doc + regenerated CRD**: the field doc previously
advertised `.Values`; it now documents the real context, and
`strategy.backups.cozystack.io_jobs.yaml` is regenerated to match (the
change is limited to that field's description).
- **Tests** (`jobstrategy_controller_test.go`): backup-Job creation /
args / labels / status, completion → `Backup` artifact with parameter
round-trip, Job-failure surfacing, missing-strategy terminal failure,
restore into the target namespace, restore completion, and the
parameter-prefix unit. A deliberately generic application kind pins that
the driver never gates on `applicationRef.kind`.
No RBAC changes are required: the controller already holds
`strategy.backups.cozystack.io/*`, `batch/jobs`, `apps.cozystack.io/*`,
and `backups` permissions.
## Notes for reviewers
- Backup and restore share one template, so a strategy author's
`PodTemplateSpec` must branch on `.Mode` to do anything
restore-specific. This matches the existing convention; worth confirming
it's the intended contract for the Job strategy's docs/examples.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **New Features**
* Implemented full backup/restore job strategy with deterministic Job
creation, idempotent execution, and status/condition updates; added demo
example scripts and reusable shell helpers for a NATS JetStream
backup/restore workflow.
* **Documentation**
* Clarified Job template rendering context and listed supported template
variables; added README and step-by-step scenario docs for the NATS
example.
* **Tests**
* Added comprehensive unit tests covering job creation,
completion/failure handling, parameter extraction, and unmappable-target
failure cases.
<!-- review_stack_entry_start -->
[](https://app.coderabbit.ai/change-stack/cozystack/cozystack/pull/1721?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack)
<!-- review_stack_entry_end -->
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Address review feedback on the Job backup strategy:
- Assert .Application is templated: the rendered backup Job now reads
.Application.spec.replicas, the load-bearing context key that lets a
generic backup tool read live app spec (previously only .Release,
.Mode, .Parameters and .Backup had coverage).
- Pin failure-path messages so a reordered error path can't pass on the
wrong branch: the missing-strategy test asserts the strategy name, and
the unmappable-kind tests assert "kind not registered".
- Document why the full Status().Update in the success branches is safe
(the object is freshly Get() each pass; the StartedAt write requeues).
- Note the fixed batch/v1.Job backoffLimit=2 and link the follow-up to
make it configurable (#2742).
Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
Replace ingress-nginx as the default ingress layer for cozystack-native
services with Cilium Gateway API, materialised per-tenant via a new
TenantGateway CRD reconciled by cozystack-controller.
The extra/gateway chart renders one gateway.cozystack.io/v1alpha1
TenantGateway per tenant; the controller owns the resulting Gateway,
per-tenant Issuer, Certificate(s), and the http->https redirect
HTTPRoute. This avoids the helm-vs-controller race on
Gateway.spec.listeners when route-driven listeners are added.
publishing.certificates.solver selects HTTP-01 (default; per-app HTTPS
listener + per-app Certificate) or DNS-01 (opt-in; single wildcard
Certificate per apex; providers: cloudflare, route53, digitalocean,
rfc2136). HTTP-01 keeps zero-config per-app onboarding; DNS-01 saves
Let's Encrypt budget on apex-shared clusters.
Security model -- seven independent layers, all admission-time gates
fail-closed:
1. Listener allowedRoutes namespace whitelist via the
kube-apiserver-written kubernetes.io/metadata.name label;
plain-HTTP listener is strictly narrower than HTTPS, and HTTPS /
passthrough listeners restrict allowedRoutes.kinds to
HTTPRoute / TLSRoute respectively.
2. cozystack-gateway-hostname-policy VAP on Gateway.
3. cozystack-gateway-attached-namespaces-policy VAP on Package --
blocks tenant-* in gateway.attachedNamespaces.
4. cozystack-tenant-host-policy VAP on Tenant -- blocks spec.host
writes from non-trusted callers.
5. cozystack-namespace-host-label-policy VAP on Namespace -- blocks
writes to namespace.cozystack.io/host from non-trusted callers.
6. Render-time fail in cozystack-basics for tenant-* in
gateway.attachedNamespaces.
7. cozystack-route-hostname-policy VAP on HTTPRoute / TLSRoute,
scoped to tenant-* namespaces.
Within-apex cross-namespace hostname conflict is resolved at reconcile
time by a HostnameConflict condition (cozy-* namespace wins).
Five reconcile paths refuse to rewrite pre-existing objects with the
controller-derived name but no OwnerReference back to the
TenantGateway: Gateway, redirect HTTPRoute, per-tenant Issuer, wildcard
Certificate, per-listener Certificate.
Derived-apex tenants (tenants whose host is a subdomain of a parent
tenant's) auto-enable Gateway when the platform-level gateway.enabled
is on.
Bonus fix: pkg/registry/apps/application/rest.go was ignoring the
createValidation and deleteValidation callbacks that genericapiserver
hands to every storage method, silently bypassing admission for Create
and Delete on every apps.cozystack.io resource (Tenant, MariaDB,
Postgres, Kubernetes, ...). Aligned with the existing updateValidation
pattern.
Always-on changes: cilium.envoy.enabled + cilium.gatewayAPI.enabled
flip to true (extra cilium-envoy DaemonSet, ~100 MB RAM/node idle),
and the cozystack-api admission-chain fix. Everything else is gated
behind gateway.enabled=false and tenant.spec.gateway=false; existing
clusters see no behavioural change until an operator opts in.
Tests: 60+ Go cases in internal/controller/tenantgateway/, admission-
chain regression tests in pkg/registry/apps/application/, helm-unittest
across 7 charts (extra/gateway, core/platform, apps/tenant,
system/cozystack-basics, system/cert-manager-issuers,
system/cozystack-api, system/dashboard), and 14 e2e bats scenarios in
hack/e2e-apps/gateway.bats. New make test-controllers target wired
into pull-requests.yaml.
Deps: sigs.k8s.io/gateway-api v1.4.1, cert-manager v1.17.4.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Complete the strategy.backups.cozystack.io/Job driver as the generic,
app-agnostic counterpart to the Altinity driver: render the strategy
PodTemplateSpec, run it as a one-shot batch/v1.Job, poll its terminal
condition, and emit a Backup. Wire it into the existing BackupClass
dispatch via reconcileJob/reconcileJobRestore rather than a standalone
reconciler, matching the post-BackupClass backups API (the old draft
read BackupJob.Spec.StrategyRef/StorageRef, which no longer exist).
Template context follows the in-tree convention (.Application, .Release,
.Mode, .Parameters, .Backup); the JobSpec.Template doc and regenerated
CRD are updated to match. Restore re-renders the same template with
.Mode=restore; cross-namespace restore is unsupported, mirroring
Altinity.
Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
The mysql chart actually deploys MariaDB via mariadb-operator, but was
incorrectly named "mysql". Rename all references to use the correct
"mariadb" name across the codebase.
Changes:
- Rename packages/apps/mysql -> packages/apps/mariadb
- Rename packages/system/mysql-rd -> packages/system/mariadb-rd
- Rename platform source and bundle references
- Update CRD kind from MySQL to MariaDB
- Update RBAC, e2e tests, backup controller tests
- Keep real MySQL CLI/config tool names unchanged (mysqldump, [mysqld], etc.)
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
myasnikovdaniil
