mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-10 06:24:09 +09:00
v1.7.0
570
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 myasnikovdaniil
|
315b335cf7 |
[Backport release-1.7] fix(marketplace): do not install a connected repository's components on connect (#5117)
# Description Backport of #5071 to `release-1.7`. |
||
|
Myasnikov Daniil
|
d2a452e890 |
fix(controller): carry healthCheckExprs and waitStrategy to existing releases
cozystack-api writes healthCheckExprs and the resolved wait strategy
from the ApplicationDefinition only when it builds a HelmRelease, and
the definition reconciler that keeps existing releases in step copied
neither. A release created before its definition exempted an object
kept waiting on that object at every upgrade. For vm-disk that is
every disk created before the DataVolume expression was added: a disk
whose import is still running after a platform upgrade fails the
upgrade at the timeout and, under RetryOnFailure, retries forever
while holding a helm-controller worker.
The reconciler now derives both fields the way the API does, in both
directions, so an existing release ends up with what a fresh write
would produce. On the shipped definitions this only relaxes waits: the
vm-disk DataVolume and the postgres KEDA ScaledObject. A release that
another chart renders carries Flux's origin label and keeps the gate
its chart sets.
A failed update used to be logged and dropped, and nothing else
re-enqueues the definition for hours, so that release stayed out of
sync. The errors are now returned and the definition is retried.
Fixes https://github.com/cozystack/cozystack/issues/4409
Assisted-by: LLM
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
(cherry picked from commit
|
||
|
IvanHunters
|
25f8f11187 |
fix(marketplace): reconcile tap texts with no-install-on-connect
Removing on-connect auto-registration made several user-facing and doc texts
wrong: they promised that tapping registers a repository's apps in the catalog
and runs its charts. Tapping now installs nothing, so the texts told operators to
wait for apps that never appear.
Reword the `cozypkg tap`/`untap` help, the tap success output (now printing a
`cozypkg add <name>` line per materialized PackageSource), the `cozypkg init`
scaffold README, the packages-index trust model, the dashboard Repositories
banner and per-repository empty state, and the stale re-registration code
comments. No behaviour changes beyond the added `cozypkg add` hint in the tap
output.
Assisted-by: LLM
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
(cherry picked from commit
|
||
|
IvanHunters
|
0e3d2cd3d6 |
fix(marketplace): do not install a connected repository's components on connect
Connecting a marketplace repository materialized its PackageSources and then
auto-created a tap-managed Package per source. The Package reconciler installed
a HelmRelease for every install-marked component of the default variant, which
Flux runs under its cluster-admin service account with no impersonation. A
publisher's install chart therefore ran as cluster-admin on connect, with no
operator step, contradicting the documented behaviour that tapping installs
nothing.
Remove the on-connect auto-registration. Connecting now only materializes
PackageSources; installing an app is an explicit cozypkg add (or dashboard
action), which is the sole remaining creator of a Package. This also stops a
Package the admin removed with cozypkg del from being re-created on the next
source revision.
Catalog population from PackageSource metadata without an install is left to
the long-term design in #4359, and the privileged-confirmation gaps #4358 and
#4359 stay open.
Assisted-by: LLM
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
(cherry picked from commit
|
||
|
Mattia Eleuteri
|
691d7bce03 |
docs(backups): say CDI adopts a restored PVC through the DataVolume
The restore controller and the user-restore example described the
allowClaimAdoption annotation its resource modifiers put on restored
PVCs as what lets CDI adopt them. CDI v1.66 reads that annotation only
on the DataVolume; on a PVC it reads nothing but createdForDataVolume.
Adoption rests on the vm-disk chart annotating the DataVolume Flux
recreates, and a reader trusting the PVC rule could rely on it for a
restore without such a DataVolume, or drop the DataVolume annotation as
redundant.
Assisted-by: LLM
Signed-off-by: Mattia Eleuteri <mattia@hidora.io>
(cherry picked from commit
|
||
|
Andrey Kolkov
|
da209397a2 |
fix(backups): prepare standalone VMDisk restore targets before the data mover runs
collectUnderlyingResources returned nothing for a non-VMInstance kind, so
prepareForRestore did nothing for a standalone VMDisk: no HelmRelease suspend and
no DataVolume delete. An in-place VMDisk restore then raced Flux/CDI — the
restored HelmRelease reinstalled and CDI recreated the DataVolume over the same
PVC name the data mover was repopulating, wedging the RestoreJob.
Report a standalone VMDisk's own disk as its underlying resource, so the existing
(VMInstance-proven) restore preparation suspends the vm-disk-<name> HelmRelease,
deletes its DataVolume and — with keepOriginalPVC — renames the live PVC before
the data mover recreates and repopulates it. No VM, so no IP/MAC is collected.
A VMInstance is halted before its disks are touched; a VMDisk target is not,
and every VMInstance disk is a VMDisk. Renaming the PVC of a disk a running VM
mounts leaves the original Terminating behind pvc-protection, Velero skips a
PVC that still exists, and the RestoreJob would report Succeeded having moved
nothing while the VM loses its disk on the next restart. An in-place VMDisk
restore now fails before changing anything when a Pod that has not terminated
mounts the disk's PVC, naming the Pod and telling the user to stop the VM or
restore the VMInstance instead.
A VMDisk Backup taken before the disk was recorded as its underlying
resource has none, so its disk is now derived from the application name
and such a restore is prepared and checked for a mounting Pod as well.
The per-disk orLabelSelectors stay a VMInstance concern. A VMDisk
strategy already selects its disk, and Velero rejects a Backup or
Restore that carries both labelSelector and orLabelSelectors, so
appending them for a VMDisk would break a strategy written with
labelSelector.
When an in-place restore fails after preparation, nothing is rolled
back: resuming the disk's HelmRelease while its PVC is renamed and its
DataVolume deleted would have CDI import a fresh disk under the original
name, which looks healthy and hides that the data sits in the -orig-
PVC. The failure message and a Warning event now name the suspended
HelmReleases and the -orig- PVCs so the user can recover them, read back
from the cluster, so a failure before a step ran does not name what it
would have left.
Preparation takes the target apart before Velero validates the Restore,
so it is refused up front when the Velero Backup is gone or not
Completed. That check now runs on every pass behind the same failure
path as the rest of preparation, and a FailedValidation Restore fails
the RestoreJob like a Failed one instead of leaving it Running. A
HelmRelease suspend or a VM halt that did not happen now stops
preparation instead of renaming and deleting under a running release or
VM, and a VMInstance's disks are renamed only once its launcher Pod has
let go of them. A VMDisk held only by a Pod already being deleted is
waited for rather than refused, for
up to 30 minutes after the RestoreJob started.
Between renaming a kept disk's PVC and deleting its DataVolume, CDI can
recreate an empty PVC under the original name. Velero then skips the
PVC as already present, the RestoreJob reports Succeeded and the VM
boots a blank disk. Once the DataVolume is gone, preparation now
deletes a PVC found under the original name, read uncached, before the
Velero Restore is created.
Preparation spans several reconciles, so the RestoreJob records a
PreparationStarted condition before it first changes the target, and
retries without changing anything when recording it fails. A
refusal on a later pass (the Velero Backup expired, a VM started on the
disk) then names what preparation left behind instead of claiming that
nothing changed, and a preparation still waiting 30 minutes after it
started fails the same way instead of polling forever.
Now that a VMDisk target is prepared, keepOriginalPVC=false reaches it
too: its DataVolume is deleted with orphan propagation and the bound PVC
is deleted before the Restore, as for a VMInstance disk. Left in place,
Velero would skip it and CDI would refuse the DataVolume the resumed
HelmRelease creates.
Assisted-by: LLM
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
(cherry picked from commit
|
||
|
Andrey Kolkov
|
82af1a457b |
feat(backups): back up Valkey with the Redis strategy
Valkey runs on the same spotahome RedisFailover operator as Redis and
speaks the same replication protocol and RDB format, so it needs no
strategy kind of its own. The only difference the backup Job sees is the
chart's resource-name prefix: valkey-<app> instead of redis-<app> for the
sentinel Service and the auth Secret.
The Redis driver now accepts applicationRef kind Valkey as well as Redis
and hands the template .Release.Prefix, derived from the app's kind, which
cozy-default-redis uses for every name it builds. A kind with no known
prefix fails the render instead of producing an empty one. cozy-default
routes apps.cozystack.io/Valkey to cozy-default-redis. A restore must
target an app of the same kind as the backup's source: replaying a Redis
dump into Valkey, or the reverse, is not something the strategy can
promise across engine releases.
The shipped template reads the prefix as `.Release.Prefix | default
"redis-"`: during an upgrade the previous controller, which has no Prefix
in its render context and serves only Redis, renders the new CR, and a
bare reference would print "<no value>" into the names.
The redis:8 image already loads RDB v11, which both shipped Valkey
versions (7.2, 8.1) write.
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
Assisted-By: LLM
(cherry picked from commit
|
||
|
Alexey Artamonov
|
4323363870 |
test(vm-disk): pin the importer error against a real stuck DataVolume
The table test builds its Running conditions by hand, so it can only
confirm the shape it already assumes. A DataVolume read off a cluster
where the importer had failed on a 404 for days carries a Bound
condition with the same error, which the message must not pick up, and
proves the error sits where the controller looks for it.
Assisted-by: LLM
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
(cherry picked from commit
|
||
|
Alexey Artamonov
|
095d9a189f |
fix(vm-disk): name the importer error in the disk's WorkloadsReady message
A disk whose HTTP source answers 404 or does not resolve never leaves
ImportInProgress: CDI fails an import only on a fatal error and restarts
the importer pod otherwise, so the condition only ever said the disk was
still importing. The reason it will not finish was visible only in the
importer logs and on the DataVolume, whose Running condition carries the
error through the back-off between restarts.
Appending that message lets the user see why the disk is stuck from the
application status and the console, which already shows the
WorkloadsReady message for a disk.
Assisted-by: LLM
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
(cherry picked from commit
|
||
|
Alexey Artamonov
|
683e80ba0c |
fix(fluxcd): give each release its own share of the hook's retries
The pre-delete hook's retry budget was one window spent in listing
order, so a release that could never be read or unfenced, listed
first, took all of it and every release after it got a single attempt.
The same transient failure then released a fence or not depending on
where its release sorted. Each release now gets its own share of the
window, capped at the window's end, and the steps that cover every
release keep the whole of it.
Two comments said more than holds. The retry window is measured from
the script's start, not the Job's, so scheduling and the image pull come
out of its one-minute reserve rather than being covered by it. And the
five-minute conflict retry inside kubectl scale never runs here, since
without a precondition it sends a merge patch with no resourceVersion
to conflict on.
helm-controller's finalizer, which the hook reads to tell whether a
release being deleted is still held, is now tied to the constant the
controller uses, as the fence already was.
Assisted-by: LLM
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
(cherry picked from commit
|
||
|
Alexey Artamonov
|
18c5acae7b |
fix(fluxcd): warn about a foreign suspension only on a stuck release
flux-plunger raised its SuspensionNotOwned and TerminatingWhileSuspended
Warnings on a suspension it does not own when the release carried its
processed-version annotation. That annotation is never removed, so
every later suspension of such a release, by a migration, a backup
restore or an operator, drew a Warning: one telling the reader to
resume it, which mid-restore is the one thing nobody wants, and one
calling a migration's deliberate suspend-then-delete an accident. Both
also missed the leftover they are for when the first recovery stopped
before writing the annotation.
A suspension an earlier flux-plunger left mid-recovery freezes the
release in the failure recovery started from, since helm-controller
rewrites no condition of a suspended release. Both Warnings now key on
that state instead, and the README's candidate query with them.
Two comments are qualified where they claimed more than holds: a stop
that overran the manager's grace period can still let a reconcile set
a fence after the drain's last listing, and a process stopped before
controller-runtime closes Elected holds the Lease yet does not drain.
Assisted-by: LLM
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
(cherry picked from commit
|
||
|
Alexey Artamonov
|
1fce03594a |
fix(fluxcd): make flux-plunger's shutdown drain report what it left
The drain's only report was an Info line, so an alert on errors never
saw fences left behind; it named no release, and printed held: -1 when
no listing had answered. A flux-plunger that never led logged that it
had released every fence next to the line saying it drained nothing.
Fences left behind are now an error naming each release, an unanswered
listing says so, and a process that never led says only that.
The drain writes its Events itself, since the manager's recorder stops
with the manager, but it created one Event object per pass for a
release it revisits every second, and each write had a deadline of its
own outside the drain's. Repeats now fold into one counted Event, as the
manager's recorder folds them, and every write is bounded by the drain.
Tests now hold the outcome line, the retry of a release whose fence
cannot be given up yet, and the pause between passes, none of which any
suite noticed when removed.
Assisted-by: LLM
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
(cherry picked from commit
|
||
|
Alexey Artamonov
|
56dd6c2e41 |
fix(fluxcd): refuse a flux-plunger drain deadline that leaves no time
A drain deadline of zero or less is already spent when the drain
starts, so flux-plunger would exit holding every uninstall fence
without listing one. The chart could produce one: its 40-second floor
was a literal, so raising the manager's stop bound or the margin before
SIGKILL, which the drain subtraction is already tied to, would leave the
floor rendering a non-positive deadline. The binary now refuses such a
flag at startup, before it reaches for a cluster, and the chart's floor
and the timings the README and the values comment state are held to
the same constants.
A whole number of a million seconds or more printed in exponent form
and was refused as a fraction; the range is now checked first, so it
gets the range message.
Assisted-by: LLM
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
(cherry picked from commit
|
||
|
Alexey Artamonov
|
021040795b |
fix(fluxcd): keep Helm uninstall working during flux-plunger recovery
flux-plunger recovers a "has no deployed releases" HelmRelease by
briefly suspending it, deleting the stale storage Secret and
unsuspending. helm-controller v1.5.x skips the uninstall and drops its
finalizer when it handles a delete on a suspended HelmRelease, and it
wakes on the same delete event flux-plunger does, so a delete inside
that window orphaned the release's DataVolume, PVCs, RBAC and storage
Secret. Lifting the suspension on the delete event cannot win that race
reliably.
Hold the release with a flux-plunger finalizer while it is suspended.
The finalizer is applied together with the suspension in one server-side
apply. On a delete flux-plunger lifts its suspension and keeps the
finalizer, helm-controller reconciles the unsuspended generation and
runs the uninstall, and flux-plunger removes the finalizer once
helm-controller's own is gone and that generation is observed. This
holds whichever controller handles the delete first. A completed
recovery lifts the suspension and the finalizer together, and a
terminating release never begins recovery.
Decide ownership of a suspension by which field manager owns
spec.suspend in metadata.managedFields rather than by an annotation. The
apiserver transfers the field away the moment an operator, cozyhr resume
or a backup controller writes a different value to it, so a later
suspension is never misread as flux-plunger's, and one set through the
shared flux-client-side-apply manager (platform migrations, the tenant
delete hook, a manual kubectl apply) is never claimed. A release deleted
under another manager's suspension is left to that manager's decision.
Acquire applies without forcing first and, on a conflict, re-fetches and
forces only if spec.suspend is still false: a backup controller owns the
field at false through a dynamic-client Update, which a non-forced apply
can never take, while true is an active suspension to leave alone. Every
apply is preconditioned on the resourceVersion and uid of the read that
decided on it. Without the resourceVersion, the unforced apply of true
over a foreign true is accepted as shared ownership, a window the
optimistic-locked merge patch this replaces had closed, and lifting the
suspension and the finalizer from a release read as live would delete it
without an uninstall if helm-controller had meanwhile handled a delete.
Without the uid, an apply to a release that has gone would create an
empty one. The apply carries only spec.suspend and the finalizer, so it
cannot leak the required spec.interval.
Read the release, every re-check and the storage Secret list from the
apiserver rather than the informer cache. A lagging cache would let the
processed-version guard delete the revision below the one just
reclaimed, and the ClusterRole grants secrets get and list but not the
watch an informer needs.
Emit a Warning when flux-plunger holds a suspension it failed to clear,
and Normal events when it leaves an external suspension alone or defers
a recovery. An external suspension on a release flux-plunger has
recovered before is a Warning instead: it is most likely one an earlier
flux-plunger left under the shared manager, which nothing lifts unless
an operator does. A release deleted under another manager's suspension
is a Normal event when that was a migration's choice, and a Warning when
the release carries flux-plunger's processed-version annotation, since
then the skipped uninstall is an accident.
Keep a single writer with leader election. A second process able to act
under the same field manager could clear a suspension the first is still
recovering, and no rollout strategy prevents one: the next pod starts as
soon as the old one is terminating, and an eviction or a node drain
replaces a pod the same way. The old pod keeps its Lease, unreleased,
through its stop and drain, and the Lease outlasts both, so the next pod
acts only after the old one is done; the price is that a new pod waits
for the Lease to run out. The strategy sets maxSurge 0 with
maxUnavailable 1 and stays RollingUpdate because existing installs carry
the apiserver-defaulted rollingUpdate block and helm-controller applies
server-side, where type Recreate over it is rejected on upgrade.
Give the finalizer a way out when flux-plunger itself goes. Once the
manager has stopped, within a bounded time, and with it every reconcile,
flux-plunger drains its finalizers under a deadline that, with the stop,
fits inside the pod's termination grace period. A stop that overran its
bound is drained too and still reported, and only a pod that led drains.
The drain writes its events straight to the apiserver, since the
manager's recorder stops with the manager. A recovery in progress is
aborted, the suspension and the finalizer lifted together for the next
flux-plunger to start again, and a release being deleted has the
suspension lifted and keeps the finalizer until helm-controller has run
the uninstall. A release helm-controller does not handle in time keeps
it. The waiting lives in the controller, where the envtest suites hold
it against a real apiserver's generations and finalizers, rather than in
a shell script against a model of them.
A pre-delete hook on the chart is the backstop for what the drain could
not finish, so uninstalling the component never leaves a release that
tenant teardown waits on forever. It stops flux-plunger, waits out its
grace period, then lifts any remaining suspension and finalizer without
waiting again, naming each release it lets go of before helm-controller
uninstalled it. It lifts a suspension only where flux-plunger alone
holds it, with a JSON patch preconditioned on the resourceVersion and
uid it read: kubectl apply --server-side on a release last written by a
client-side apply moves that apply's fields to the applying manager and
strips every one the body leaves out. The Job's deadline sits under the
300s helm-controller gives a pre-delete hook by default. A Deployment
that cannot be stopped or a listing that never succeeds ends the hook
before any finalizer is touched, and a finalizer it cannot remove ends
it after the sweep; either way it exits non-zero: a failed pre-delete
hook leaves the release untouched and helm-controller retries the
uninstall, hook included, from a state it can still converge from, where
exiting 0 would remove the component and strand the finalizer. A
flux-plunger pod still present after its grace period, typically one
stuck Terminating on a NotReady node, is only reported, since failing on
it would fail every retry until the node returns; because it may still
be setting finalizers, they are listed again after the sweep and any
found fails the hook. The Job runs once, since helm-controller's retry
is the one that starts afresh, and is kept after success so that its
report stays readable; helm.sh/hook-output-log-policy would make
helm-controller call a nil output writer (fluxcd/helm-controller#1562).
A delete still held after ten minutes raises a Warning, and the held
state is re-checked on a timer instead of only on helm-controller's next
status write. The chart README documents the finalizer and how to clear
it.
Run the envtest suites on every pull request through a new make target
that fetches a kube-apiserver and etcd with a pinned setup-envtest and
takes the HelmRelease CRD from the in-tree Flux manifests: the unit
tests drive a fake client, and everything the finalizer rests on (apply
preconditions, the finalizer set-merge, real managedFields) only holds
against a real apiserver. CI caches those binaries, and the target uses
them without asking the release index, so a pull request does not depend
on their sources being reachable; the control plane gets a longer start
timeout because it starts beside the other test goals.
Assisted-by: LLM
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
(cherry picked from commit
|
||
|
Andrey Kolkov
|
6a75883013 |
fix(backups): keep the CNPG purge for unmarked Clusters this restore did not render
A recovery Cluster without the restored-server-name annotation fell back to the creation-time comparison. On the current chart the annotation is absent when bootstrap.newServerName is empty, i.e. a manually configured recovery: re-rendered during the WAL-archive wait it is created after StartedAt, the purge was skipped and the restore could report success on the manual recovery source. Use the creation-time fallback only when the application's bootstrap.newServerName already carries this restore's server name, which keeps it for Clusters rendered by a chart that predates the annotation. Assisted-By: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
35ca217839 |
fix(backups): name the restore that goes first in ConcurrentRestore
restoreAhead returned the alphabetically smallest of the jobs ahead, so with more than one ahead the ConcurrentRestore message could name a job that is not the one holding the application. Return the first job in restoreGoesFirst order, and pin the creation-time tie-break with cases where creation order and name order disagree. Assisted-By: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
e4e991e837 |
fix(backups): skip the CNPG restore purge only for this restore's Cluster
The in-place CNPG restore skips deleting the live Cluster when it carries bootstrap.recovery and was created after the RestoreJob started, taking that as "this restore's own purge and re-render produced it". The timestamp does not say which restore rendered the Cluster. A Cluster deleted while the restore waits for the WAL archive is re-rendered by the still-active HelmRelease from the previous restore's values and is created after StartedAt too; the restore then skipped its purge and reported Succeeded on the previous restore's data. The restore patch already sets bootstrap.newServerName to restoredServerName(cluster, RestoreJob UID). The chart now annotates the Cluster with it as postgres.cozystack.io/restored-server-name, whether or not it renders the barman plugin that also carries the value, and the purge is skipped only when the annotation names this RestoreJob. The mark lives on the rendered Cluster rather than in RestoreJob status, so it still covers the failed status write the skip exists for. A Cluster rendered by a chart that predates the annotation keeps the creation-time check. Fixes #3322 Assisted-by: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
3daf942dce |
fix(backups): refuse a restore into an application another restore holds
Nothing stopped two RestoreJobs from rewriting one application at once. Each restores the target from its own Backup, so the application ends up on whichever finishes last, and the CNPG driver's purge decision cannot tell which of the two produced the live Cluster: the second job could skip its purge and report Succeeded on the first job's data. A RestoreJob that has not started now fails with reason ConcurrentRestore when another unfinished RestoreJob in the namespace writes into the same application and goes first. A started restore goes before one that has not; between two that have not, the older one goes first, by creation time and then name, so a running restore is never failed by a newer one. The refused job fails instead of waiting, so the application is not restored a second time later, after the first restore, by a job its author may no longer expect to run. The gate lists RestoreJobs through the manager's API reader rather than the informer cache. Creation times have one-second precision, so two jobs created in the same second are ordered by name, and a job reconciled before the cache held its peer's StartedAt would see that peer as not started and proceed alongside it. The controller runs one worker, so a peer's StartedAt written by an earlier reconcile is on the API server by the time the next job reads it; SetupWithManager now states that. The target is resolved as the BackupJob hold resolves it, so a Velero restore into another namespace does not collide with one in place. Refs #3322 Assisted-by: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Aleksei Sviridkin
|
f4bde9e50d |
fix(backups): delete the original PVC of an in-place VM restore without keepOriginalPVC (#4672)
<!-- Thank you for making a contribution! Here are some tips for you:
- Use Conventional Commits for the PR title: `type(scope): description`
- Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore
- Scopes are not an exhaustive list — pick the most specific scope for
the change and extend the list when a genuinely new area appears.
Examples:
- System components: dashboard, platform, operator, cilium, kube-ovn,
linstor, fluxcd, cluster-api
- Managed apps: postgres, mariadb, redis, kafka, clickhouse,
virtual-machine, kubernetes
- Development and maintenance: api, hack, tests, ci, docs, maintenance
- Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or
add a `BREAKING CHANGE:` footer
- If it's a work in progress, consider creating this PR as a draft.
- Don't hesistate to ask for opinion and review in the community chats,
even if it's still a draft.
- Add the label `kind/backport` if it's a bugfix that needs to be
backported to a previous version.
-->
## What this PR does
An in-place VMInstance restore with `keepOriginalPVC: false` reports
`Succeeded` without restoring the disks, and leaves every disk it
touched unable to come back. `prepareForRestore` deletes each DataVolume
with Orphan propagation and leaves the PVC in place, on the assumption,
stated in the `deleteDataVolume` comment, that Velero overwrites it.
Velero does not overwrite a PVC. Its CSI restore action returns early
for a claim that already exists, so no DataDownload is ever created, and
with `existingResourcePolicy: update` the merge patch it attempts
instead is rejected because a bound claim's spec is immutable. All that
reaches the claim is the labels-only fallback patch, which also drops
the `cdi.kubevirt.io/allowClaimAdoption` annotation the resource
modifier added.
The claim left behind then blocks the disk for good. Orphan propagation
has removed its owner reference to the DataVolume, and it lacks the
`cdi.kubevirt.io/storage.populatedFor` annotation that
kubevirt-velero-plugin writes on the backed-up copy, so when the
HelmRelease resumes, the CDI webhook refuses the DataVolume it creates
with `Destination PVC <ns>/<name> already exists`, on every reconcile.
Deleting the disk through the API leaves the PVC behind, and a new disk
of the same name fails the same way.
Observed on a v1.6.3 cluster, for a VMInstance restore with
`keepOriginalPVC: false` whose disk was attached to the VM. The Velero
log for the restore, trimmed:
```text
level=info msg="Retrieved per-restore resource modifiers from configmap cozy-velero/restore-modifiers-..."
level=warning msg="PVC already exists and ExistingVolumeDataPolicy is not in-place restore. Skip restore this PVC." Action=PVCCSIRestoreItemAction PVC=tenant-datalab/vm-disk-e2edisk
level=info msg="Applying resource modifier patch on tenant-datalab/vm-disk-e2edisk"
level=warning msg="patch attempt failed for /v1, Kind=PersistentVolumeClaim tenant-datalab/vm-disk-e2edisk: PersistentVolumeClaim \"vm-disk-e2edisk\" is invalid: spec: Forbidden: spec is immutable after creation except resources.requests and volumeAttributesClassName for bound claims"
level=info msg="backup/restore labels successfully updated for PersistentVolumeClaim tenant-datalab/vm-disk-e2edisk"
```
`velero restore describe --details` lists the PVC as `(failed)` and the
Restore is `Completed` with two warnings. Afterwards the claim had its
original creation timestamp and UID, no owner reference, no
`allowClaimAdoption`, no `populatedFor`, and the disk's HelmRelease was
`Ready=False` with the webhook error above. That cluster runs a Velero
build from `main`, whose wording is quoted above; the released v1.17.1
and v1.18.3 return early at the same point with `PVC already exists.
Skip restore this PVC.` (`pkg/restore/actions/csi/pvc_action.go`).
The fix deletes the PVC when `keepOriginalPVC` is false, only once its
DataVolume is confirmed gone so that CDI cannot recreate it, and holds
the Velero Restore until the claim has actually disappeared. A claim
still mounted stays `Terminating` behind `kubernetes.io/pvc-protection`,
and creating the Restore then would hit the same existing-claim path.
Velero then creates the PVC from the backup, the data mover fills it,
and the recreated DataVolume adopts it through `populatedFor`. A failed
deletion now fails the RestoreJob instead of letting it report
`Succeeded` over the old data. Before
|
||
|
Aleksei Sviridkin
|
b1192bfcbc |
fix(backups): create no driver object for a job that already finished (#4673)
<!-- Thank you for making a contribution! Here are some tips for you:
- Use Conventional Commits for the PR title: `type(scope): description`
- Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore
- Scopes are not an exhaustive list — pick the most specific scope for
the change and extend the list when a genuinely new area appears.
Examples:
- System components: dashboard, platform, operator, cilium, kube-ovn,
linstor, fluxcd, cluster-api
- Managed apps: postgres, mariadb, redis, kafka, clickhouse,
virtual-machine, kubernetes
- Development and maintenance: api, hack, tests, ci, docs, maintenance
- Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or
add a `BREAKING CHANGE:` footer
- If it's a work in progress, consider creating this PR as a draft.
- Don't hesistate to ask for opinion and review in the community chats,
even if it's still a draft.
- Add the label `kind/backport` if it's a bugfix that needs to be
backported to a previous version.
-->
## What this PR does
The BackupJob and RestoreJob reconcilers decide a job is still active
from the informer cache, and the cache can trail their own writes. A
driver's first pass stamps `StartedAt`, which requeues the job, and may
then fail the job in the same pass on a transient error, for instance a
503 from the aggregated `apps.cozystack.io` API while
`createVeleroBackup` reads the application. The requeued pass can start
before the informer has seen `Failed`: it retries the step that failed
and, once the error has cleared, creates the driver object anyway. Its
own status write then conflicts on the stale resourceVersion, the next
pass sees `Failed` and skips the job, and a Velero Backup is left
running that no `Backup` artifact will reference and nothing tracks.
Reproduced against a real apiserver (envtest 1.34) with the real manager
cache and the reconciler wired through `SetupWithManager`, nothing else
writing the BackupJobs. The only fault injected is one 503 on the first
GET of each application through `apps.cozystack.io`. Out of 40
BackupJobs, every one ends `Failed` with `failed to create Velero
Backup: the server is currently unable to handle the request`, and on
`main` (
|
||
|
Aleksei Sviridkin
|
d9689bd6e7 |
fix(gateway): stop describing Cilium 1.19.5 as the shipped release
With Cilium at 1.20.2 the tenant gateway comments, README and one status message still explained behaviour as that of "the pin" at 1.19.5. Several of those claims no longer hold: a Gateway with two routed passthrough ports now gets an Envoy listener per port, Cilium judges a route's kinds against the listener the route names, and the shipped operator accepts an HTTPS and a passthrough listener on one port when the passthrough hostname is strictly narrower. Behaviour that is unchanged is re-anchored on the 1.20.2 source, behaviour that is gone is stated as history, and the refusal of one hostname on two passthrough ports no longer blames a Cilium limitation the shipped release does not have. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
ad3f15d4f3 |
feat(gateway)!: reserve a passthrough listener's name by the spec alone
The TenantGateway controller withheld the HTTPS-terminate listener for a name a passthrough listener answers only once a TLSRoute was attached there and resolved a backend, mirroring how Cilium builds its filter chains. That kept a served endpoint up on Cilium 1.19.5, but it tied every route's status to one implementation's translator and needed Service and ReferenceGrant watches to land the deferred decision. The shipped operator accepts a wildcard terminate listener beside a narrower passthrough listener on 443 and keeps conflicting every other overlap. A terminate listener for the passthrough name itself is not narrower, so on 443 the rule now protects the dataplane outright; on a native port Gateway API would serve the pair, and the rule holds as a policy that keeps a platform name with its passthrough listener. Reading it off the spec makes the outcome the same on every implementation. Under http01 an HTTPRoute claiming such a name gets no listener and no certificate whether or not a TLSRoute is attached. A route whose every hostname is reserved reads Accepted=False with NoMatchingListenerHostname; one that also claims a free hostname stays Accepted and is served there, as Gateway API accepts a route whose hostnames intersect at least one listener. dns01 and existingSecret are unchanged: their wildcard listener still covers such a name, and the passthrough listener answers it by its exact server name. The chart writes the passthrough services on the publishing tenant alone, so the names reserved by default are the ones the platform publishes there through its own TLSRoutes. The shape an upgrade changes is an HTTPRoute on such a name whose platform TLSRoute forwards nowhere, because it never attached (the package owning the service is not installed) or because none of its backends resolve: that route kept a terminate listener and now gets none. The backend resolution, its watches and the RBAC rules that served them go away. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
bdca165f4c |
feat(gateway): report a GatewayClass that does not serve TLSRoute
A GatewayClass lists the features it serves in status.supportedFeatures, and TLS passthrough is TLSRoute. On a class that leaves it out the passthrough listeners are accepted and serve nothing, so the TenantGateway now reads Ready=False/GatewayClassUnsupported naming the class instead of Ready. A class that lists no features at all is not judged: implementations predating the field leave it empty. The class writes that list after it is accepted and may change it when its controller is upgraded, so a GatewayClass event requeues the TenantGateways on it. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
d624049a3a |
feat(gateway): allow tlsPassthroughListeners under dns01 and existingSecret
The refusal existed because Cilium 1.19.5 served every Gateway port from one Envoy listener matched by SNI alone, so the "*.<apex>" terminate listener of the wildcard modes and a passthrough listener under the apex could reach each other's backend. The shipped Cilium 1.20.2 gives each port its own Envoy listener when SNI overlaps across ports, and Gateway API treats listeners on different ports as compatible, so the pair is served as declared. edge still refuses the field: it renders no TLS listener for an entry to bind to. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Mattia Eleuteri
|
06e72b26dd |
test(backups): give the restore stale-cache test a Completed Velero Backup
A Velero RestoreJob is about to require its Velero Backup to exist and be Completed before it halts the application. Without one in the fixture, the first pass of this test fails the RestoreJob before it suspends the HelmRelease, so the test would stop exercising the stale cache it is about. Assisted-by: LLM Signed-off-by: Mattia Eleuteri <mattia@hidora.io> |
||
|
Aleksei Sviridkin
|
f203a47afe |
fix(backups): hold a Postgres backup until the plugin rollout is over (#4674)
<!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `kind/backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does The first BackupJob on a Postgres app whose `cnpg.io/Cluster` has never carried the barman-cloud plugin — a fresh platform-flow app, or one an in-place restore re-rendered — attaches the plugin and creates the `cnpg.io/Backup` in the same reconcile. Attaching the plugin changes the instance pod spec, so CNPG rolls every instance (`Pod rollout required: volumes: element plugins has been added`). The Backup then lands on an instance without the plugin sidecar (`requested plugin is not available: barman-cloud.cloudnative-pg.io`) or on one CNPG restarts under it (`instance manager was restarted during backup`). CNPG does not retry a failed Backup, so the BackupJob stays Running in `CNPGBackupTransientFailure` until `cnpgDefaultBackupDeadline` and fails. The first backup of each such Postgres is always lost; this was observed on a v1.6.3 cluster and reproduced against `main`. The driver now holds the Backup back until that rollout is over. Before attaching the plugin to a Cluster that has none, it records `PluginRolledOut=False` on the BackupJob, with `Ready=False` reason `WaitingForPluginRollout` for the user, and requeues; the next pass attaches the plugin. The Backup is created once CNPG reports the Cluster in its healthy phase and every instance pod (`cnpg.io/cluster`, `cnpg.io/podRole=instance`) was created after the condition and is Ready; the condition then turns True and `Ready` is cleared, as before. Recording the condition first means a lost status write leaves the plugin unattached for the next pass instead of a Cluster that looks ready while it rolls. A Cluster that already runs the plugin is not rolled by the driver and its Backup is created on the first pass, as today. The wait is bounded by the existing `cnpgDefaultBackupDeadline`, after which the BackupJob fails with the phase or pod CNPG is stuck on. Why this signal, checked against the CNPG 1.29 source and the 1.30 Cluster CRD vendored here: the Cluster status has no `observedGeneration` and its `Ready` condition is derived from `status.phase`, so right after the attach the phase still reads healthy from before until CNPG reconciles the new spec. `status.pluginStatus` does not help either, since CNPG also lists plugins referenced only from `externalClusters` there, which is exactly the state of a Cluster a restore re-rendered. A pod spec change is never applied in place (only a pending Postgres restart is), so the instances are recreated and pods newer than the attach are the ones running the sidecar. This reads pod metadata and CNPG's documented labels only, not the plugin's container or volume names, and needs no RBAC change: the controller already lists and watches pods. Tests: `TestReconcileCNPG_WaitsForPluginRolloutBeforeBackup` drives a Cluster without the plugin through the attach, a stale healthy phase over the old pod, a rolling phase over the recreated pod, and the healthy phase, and asserts no Backup exists until the last step. `TestReconcileCNPG_AttachedPluginDoesNotDelayBackup` covers a Cluster that already runs the plugin, and `TestReconcileCNPG_PluginRolloutWaitIsBounded` the deadline. With the gate disabled, the first and third fail; with only the phase checked and not the pods, the first fails at the attach pass. ### Screenshots <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> Not applicable: no UI change. ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> The diff touches only `internal/backupcontroller` Go code: no package, values, chart, CRD, `hack/` tooling, telemetry metric or contributor convention. Walked against the trigger map, it reaches none of the repositories below, and the website documents no workaround for the lost first backup that this would make obsolete. - [x] 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: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note fix(backups): the CNPG backup driver no longer loses the first backup of a Postgres app whose Cluster had no barman-cloud plugin yet (a new platform-flow app, or one restored in place). It now waits for CNPG to finish rolling the instances onto the plugin before creating the cnpg.io Backup, and reports the wait on the BackupJob as Ready=False with reason WaitingForPluginRollout. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Reliability** * Backups wait for the backup plugin rollout to complete and cluster instances to become healthy before starting. * If rollout does not finish before the backup deadline, the backup job fails instead of waiting indefinitely. * Backups can start without waiting for a new rollout when the plugin is already attached. * **Backup Configuration** * When no server name is configured, backups use the cluster name and application UID, giving recreated applications distinct archive prefixes. * Existing server names are preserved when the plugin is already attached. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Mattia Eleuteri
|
eafa441b9c |
fix(backups): create no driver object for a job that already finished
The BackupJob and RestoreJob reconcilers decide a job is still active from the informer cache, and the cache can trail their own writes. A driver's first pass stamps StartedAt, which requeues the job, and may then fail the job in the same pass on a transient error, such as a 503 from the aggregated apps.cozystack.io API while it reads the application. The requeued pass can start before the informer has seen Failed: it retries the step that failed and, once the error has cleared, creates the driver object anyway. Its own status write then conflicts, the pass after it sees Failed and skips the job, and a Velero Backup is left running that no Backup artifact will reference and nothing tracks. Against a real apiserver and the manager's cache, with one 503 on the first application read of each job, 5 to 10 BackupJobs out of 40 ended Failed with a Velero Backup. A restore pass that starts on such a stale copy does worse, since the step it retries can be destructive: suspending the VM's HelmRelease and halting the VM before creating a Velero Restore, or patching the Postgres application and purging its cluster. Both reconcilers now re-read the job through the manager's uncached reader when the cached copy says it is still active, and skip it when the apiserver holds it as finished. The check sits ahead of the dispatch, so it covers every driver, and a job the cache already reports as finished costs no extra request. Assisted-by: LLM Signed-off-by: Mattia Eleuteri <mattia@hidora.io> |
||
|
Mattia Eleuteri
|
6d064a8602 |
fix(backups): delete a restored disk's PVC only once the restore can run
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> |
||
|
Mattia Eleuteri
|
6ea713d09b |
fix(backups): delete the original PVC of an in-place VM restore without keepOriginalPVC
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.
Assisted-by: LLM
Signed-off-by: Mattia Eleuteri <mattia@hidora.io>
|
||
|
Mattia Eleuteri
|
54ab863c0e |
fix(backups): hold a Postgres backup until the plugin rollout is over
A Cluster that has never carried the barman-cloud plugin gets it from
the first BackupJob: a fresh platform-flow app, or one an in-place
restore re-rendered. Attaching it changes the instance pod spec, so
CNPG rolls every instance. The cnpg.io/Backup the same reconcile
created then runs on an instance without the plugin sidecar ("requested
plugin is not available") or on one CNPG restarts under it ("instance
manager was restarted during backup"). CNPG never retries it, so the
BackupJob sat in CNPGBackupTransientFailure until the backup deadline
and failed: the first backup of every such Postgres was lost.
The driver now records a PluginRolledOut=False condition before it
attaches the plugin, and creates no Backup until CNPG reports the
Cluster healthy and every instance pod was created after that
condition and is Ready. Recording it first means a lost status write
leaves the plugin unattached for the next pass, rather than a Cluster
that looks ready while it rolls. A Cluster that already runs the plugin
is not rolled by the driver and is not held back.
The phase alone cannot tell the rollout is over. Cluster status has no
observedGeneration and its Ready condition is derived from the phase,
so until CNPG reconciles the new spec the phase still reads healthy
from before the attach. status.pluginStatus is no better: CNPG also
lists plugins referenced only from externalClusters there, which is
exactly the state of a Cluster a restore re-rendered. A pod spec change
is never applied in place, so the instances are recreated, and pods
newer than the attach are the ones running the sidecar. That reads only
pod metadata and CNPG's documented labels, not the plugin's container
names. The wait stays bounded by the existing backup deadline.
Signed-off-by: Mattia Eleuteri <mattia@hidora.io>
Assisted-by: LLM
|
||
|
Aleksei Sviridkin
|
8c1a78dc9b |
feat(operator): default system namespace memory via LimitRange (#3494)
## What this PR does
System components now get a default container memory limit, so their pod
cgroups carry a `memory.max` and drop out of the Talos userspace OOM
handler's victim set. `cozystack-operator` maintains a LimitRange named
`cozystack-system-defaults` in every system namespace it reconciles.
The Talos handler (v1.12+) scores cgroups with `memory_max.hasValue() ?
0.0 : {Besteffort: 1.0, Burstable: 0.5, ...}[class] * memory_current`
and drops everything that scores zero. Only a memory limit sets
`memory.max`, so only a limit takes a pod out of the candidate set. A
request just moves a pod from BestEffort to Burstable, which under the
default `strictCgroupClassOrdering: true` still means killed second.
`PriorityClass` does nothing here, because the handler reads cgroupfs
and never talks to the apiserver. Victim selection is also unrelated to
the trigger, so a tenant workload thrashing against its own limit can
drive node memory pressure while a system component with no limit is
what gets killed.
An audit of a running cluster found around 70 workloads across ~30
packages with at least one container missing a memory limit. Per-chart
resources would not cover that set, and would not keep covering it as
packages are added. So the LimitRange is written by
`reconcileNamespaces`, which already creates these namespaces and
already tells system from tenant. It also covers init containers, which
#3495 can't reach: one of them without a limit leaves the pod cgroup
without `memory.max`, whatever the main containers declare.
Tenant namespaces are skipped. Where the tenant chart's own
`tenant-range-limits` exists (only when the operator configured
`resourceQuotas`), a second LimitRange next to it would leave the result
to the order LimitRanger iterates them. Where it doesn't exist, a tenant
namespace is not the place for the platform to impose a default.
### Why the default is 32Gi
The handler grants immunity for `memory.max` existing at all, whatever
its value. So the ceiling doesn't need to be fitted, and 32Gi is high
enough that nothing plausibly reaches it. A ceiling low enough to bind
would turn a rare pressure-driven kill into a deterministic one for any
component whose real usage nobody measured. This repo already has one
such case, recorded in the `vertical-pod-autoscaler` values, where a
110Mi limit OOM-killed the updater and failed HelmReleases with it.
Scheduling is unaffected, since pods are scheduled against requests and
the paired default request is 32Mi. An operator who wants the ceiling to
bind can lower it, per component in that component's chart or fleet-wide
with the knob, once real usage is measured.
### When the default is withheld
A default below a container's own request makes that pod impossible to
admit, so the operator checks before it applies. It scans Deployments,
StatefulSets, DaemonSets, CronJobs, Jobs and Pods in the namespace, init
containers included, for a container that requests more than the
configured limit and declares no limit of its own. If it finds one, it
withdraws the managed LimitRange and records a hold in the
`operator.cozystack.io/system-memory-state` annotation on the Namespace.
The hold does not clear by itself when the blocker disappears, because
an empty scan looks the same as a pod between deletion and recreation.
Lowering a previously applied limit also creates a hold, even with no
visible blocker, because LimitRanger writes the defaulted limit onto
admitted pods and the scan can no longer tell which containers declare
one. A cluster administrator releases the hold by copying the
`<hold>:<target>` token from the state into the
`operator.cozystack.io/system-memory-ack` annotation. The token is used
once and only for that hold and target. If a blocker is still visible,
the token is rejected and the hold stays. Package reconciles are
scheduled at least every five minutes, so an acknowledgement takes
effect on a quiet cluster too. The reason and the token also appear in
the operator log under `withholding the default container memory limit`.
### Namespace another LimitRange already governs
If a system namespace already has a LimitRange that says anything about
memory, at the Container or the Pod scope, the operator writes nothing
there and withdraws what it wrote before. A second default leaves the
effective value to iteration order. A foreign `max` below this default,
a `min` above the default request or a `maxLimitRequestRatio` rejects
pods outright, which takes out a whole namespace. At the Pod scope the
collision can't be decided in advance, because it depends on how many
containers a future pod has. A LimitRange that bounds only cpu or
storage coexists normally. A same-named LimitRange without the
operator's managed-by label is treated as foreign and left alone.
This matters most in `kube-system`, which is in scope because
`cozystack-scheduler` installs there, and where a distribution's or an
administrator's own LimitRange is likeliest to sit.
### Disabling
Setting `cozystackOperator.systemNamespaceMemoryLimit` to `0` disables
the feature. On the next reconcile the operator removes every LimitRange
it owns cluster-wide, found by the managed-by label. While the feature
is on, it also removes its LimitRange from namespaces no active Package
targets any more. The safety state on the Namespace stays in both cases.
### Docs
`docs/operations/system-memory-limits.md` landed in #3496 ahead of this
code, so main describes a `4Gi` default and installer values that do
nothing yet. The page is corrected to the 32Gi default and now describes
withholding, the acknowledgement, and the foreign LimitRange rule.
### Not in this PR
linstor policy. #3824 extends the controller's existing exemption from
limits to the satellite, and a linstor limit here would contradict it,
so linstor stays with that PR.
The scan reads six workload kinds through `APIReader`, uncached, on
every reconcile of every system namespace. That cost is tracked in
#3977, since the system namespace set isn't known when the cache is
built and the right fix depends on a reconcile-rate measurement nobody
has taken.
### Verification
`go vet` and `go test` pass for `cmd/cozystack-operator` and
`internal/operator`, and `helm unittest` passes on the installer chart
for the knobs unset, set, and the limit at `0`. The reconciler tests
cover withholding on an oversized request, the hold surviving the
blocker's removal, the hold on a lowered limit, acknowledgement and
token rejection, every memory field of a foreign LimitRange at both
scopes, ownership by label, the disabled sweep, and a deleting Package
not creating policy.
The withhold design has not been run on a live cluster yet. An earlier
design of this PR was, and that run does not carry over.
### Screenshots
Not a UI change.
### Downstream repositories
I walked the trigger map against the diff.
`packages/core/installer/values.yaml` gains two keys, but the
`ansible-cozystack` trigger is for a required key or a rename of one the
role passes, and these are new, optional and default to `""`, so the
role is unaffected. Nothing else in the map is reached by the Go
changes, the operator template or the page.
- [x] 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:
- [ ] [cozystack/community](https://github.com/cozystack/community) -
follow-up:
### Release note
```release-note
feat(operator): system namespaces now get a default container memory limit through a LimitRange maintained by cozystack-operator, which keeps system components out of the Talos userspace OOM handler's victim set. Tune it with cozystackOperator.systemNamespaceMemoryLimit (default 32Gi) and cozystackOperator.systemNamespaceMemoryRequest (default 32Mi); set the limit to 0 to disable it. The default is far above any real working set on purpose, because the handler grants immunity for a memory.max existing at all, not for its value. A namespace with a container that requests more than the limit and declares no limit of its own gets no default, and stays without one until a cluster administrator acknowledges the hold recorded on the Namespace (see docs/operations/system-memory-limits.md); lowering a previously applied limit needs the same acknowledgement. A namespace where another LimitRange already sets anything about memory is left alone. A container that declares its own memory limit is never changed.
```
|
||
|
Myasnikov Daniil
|
9927c299d4 |
fix(operator): keep deleting packages out of memory policy creation
A foreground-deleting Package can survive long enough to reconcile after cleanup retires its default. It must not reinstate that policy or consume an acknowledgement while its components are being removed. An active Package sharing the namespace can still manage the default. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
9d5d6d8f45 |
test(operator): exercise failed memory safety scans
Foreign ownership short-circuited the read-error fixtures before their interceptors ran. Require the injected failure to occur and preserve both policy and Namespace state, including a pending acknowledgement. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
5b585d6eda |
fix(operator): persist holds for unsafe memory defaults
An admitted Pod loses the distinction between explicit and defaulted limits. Its disappearance cannot prove a lower default is safe for its replacement. Keep that decision on the Namespace until an administrator acknowledges the particular target, without waiving current blockers or foreign policy ownership. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
30ecc41070 |
fix(operator): close memory policy safety gaps
Preserve foreign same-name objects and keep create-time SSA non-force. Only ignore failed pods controlled by terminal Jobs. Make the failure-containment test inject Apply errors for real. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
22a27e75ac |
test(operator): cover memory cleanup failures
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> Assisted-By: LLM |
||
|
Myasnikov Daniil
|
e83b4eb492 |
fix(operator): align memory policy ownership messages
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> Assisted-By: LLM |
||
|
Myasnikov Daniil
|
0d5b2e35c4 |
docs(operator): align scan comments with withholding
Replace three stale descriptions of the removed ceiling-raise behavior with the current withhold and withdraw semantics. Historical explanations of why the raise design was rejected remain intact. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
6a521f0c68 |
fix(operator): sweep memory defaults when disabled
List LimitRanges by the ownership label once per Package reconcile when the feature is disabled, and remove managed objects even in namespaces outside the current Package target set. Skip the per-namespace path while disabled and keep cleanup failures non-fatal to namespace reconciliation. Update the flag help, installer guidance, and operations documentation to describe the global cleanup boundary. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
492c9acdd0 |
fix(operator): ignore failed pods from finished jobs
Record terminal Job UIDs while scanning templates, then ignore only Failed pods owned by those Jobs. Failures from unfinished controllers and ownerless pods remain evidence because they may still be recreated. Also remove the stale constructor comment that still described raising a namespace ceiling. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
2e3c40b125 |
fix(operator): ignore non-pod LimitRange scopes
Kubernetes accepts memory keys on PVC-scoped LimitRange items when a storage bound is also present, but those quantities cannot affect pod admission. Restrict the foreign-policy guard to Container and Pod items. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
f6dcb69b61 |
refactor(operator): withhold instead of raising, and drop the grace period
The raise-and-grace machinery cannot work: LimitRanger writes the raised limit onto the very container that justified the raise, so at that container's next admission it carries a limit, the scan skips containers that have one, and the raise has erased its own evidence. The ceiling then drops on schedule and the workload is rejected permanently. Any such machinery approximates admission logic ahead of admission, and that is incomplete by construction: a LimitRange default is one number for a whole namespace and cannot mean "the configured limit, or this container's request, whichever is larger". Only per-pod conditional defaulting expresses that, which is a mutating webhook and not this. The justification for the machinery had also expired. It was designed when the default was 4Gi, where withholding was self-perpetuating for VPA-managed namespaces. At 32Gi nothing in the tree approaches the ceiling -- the largest static memory request is 2Gi -- and VPA sets requests and limits together, so those containers carry their own limit and were never blockers. So a namespace holding a container that requests more than the configured limit with no limit of its own is now left with no default at all, and anything written there earlier is withdrawn. It fails in the safe direction, which is the whole argument: a namespace with no memory.max is a namespace behaving as it did in every release before this, while a default below a request is a namespace whose workload cannot be admitted. It needs no stored state and no timer, because withholding is not a decision that has to survive between reconciles -- once the oversized request is gone the default is applied again on the next pass. Gone with it: the grace period, the clear-since annotation and its clamp, the injected clock, containerMemoryDefault, raisedCeiling, and fourteen tests. The scan stays, because deciding whether to apply or withhold is what it was always for. The foreign-LimitRange ownership guard, the disable path and the 32Gi default are untouched. The residual sharp edge is stated rather than machinery: a workload whose only evidence is a live Pod, built from a custom resource with no template this scan can read, can still be missed if the scan runs while that Pod is absent, and applying the default then rejects its recreation. At 32Gi that is unreachable in this tree. It becomes reachable if an operator lowers the knob below such a workload's request, which the flag help, values.yaml and the operations page now say plainly. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
d3df7c3d4d |
style(operator): gofmt stray blank lines in the package reconciler
Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
588ecbaeeb |
fix(operator): scan pods every reconcile, and key ownership on the label
The pod half of the scan was skipped once a namespace defaulted container memory at exactly the configured limit, on the argument that the shape the scan looks for could no longer be admitted there. That argument proves such a pod cannot come to exist. It does not prove one cannot be attempted, and a failed attempt leaves nothing behind: a workload built from a custom resource whose request grows above the limit after its namespace settled was rejected permanently, with no artefact anywhere that could justify raising the ceiling again. The premise was not sound even for existing pods, since LimitRanger reads LimitRanges through an informer and the shape is admissible in the window before the plugin sees the apply. The skip bought one namespaced List out of six and paid for it with a silent permanent-rejection mode, so it is gone along with memoryDefaultSettledAt and the two tests that pinned it. One of those tests carried the counterexample in its own fixture: a settled 4Gi LimitRange beside an 8Gi pod, asserted invisible. The coexistence guard identified its own object by name. The name is not reserved, so an administrator's LimitRange called cozystack-system-defaults was skipped by the guard and then overwritten by the forced apply, and because LimitRangeSpec.Limits is an atomic list that replaced every item in it, taking their max and cpu policy along. Ownership is now the managed-by label, which is what the delete path has always used. That leaves a same-named object with no label ambiguous: ours with the label stripped, or an administrator's. The two mistakes are not symmetric, so the ambiguous object is treated as foreign and left entirely alone rather than adopted. Re-stamping it with the label goes, and its test is replaced, because the same path reaches an administrator's object and adopting it means the disable path later deletes their policy. Declining to delete is no longer silent either. Also: containerMemoryDefault now takes the last matching item rather than the first, which is what LimitRanger applies, so a multi-item spec no longer reports a ceiling the cluster is not using. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
c6342bf885 |
fix(operator): bring the flag help in line with the guard and removal scope
The --help text still carried two claims that values.yaml and the operations page had already narrowed, and it is the one surface an operator reads without opening the tree. Both were wrong in the direction that costs the reader something. "A namespace where another LimitRange already defaults or caps container memory is left alone" described the guard before it was widened, so an administrator holding a container memory min, a maxLimitRequestRatio or any Pod-scoped bound would read it, expect the LimitRange to be applied there anyway, and get a namespace with no memory.max for a reason the help says does not apply. "Removes any previously created one" is the universal removal claim, and removal is scoped to namespaces an active Package still targets. Two comments corrected while here. constrainsMemory said it switched on the item's own maps "rather than a hand-picked few", which contradicts the hand-picked list immediately below it; what is actually true is that a field added upstream has to be added there too, which is the reason it has its own table test. And memoryDefaultSettledAt claimed the count of raised namespaces falls to none once the oversized requests are gone, which stopped being true when the scan started keeping Failed pods: a carcass Kubernetes never collects holds both the raise and that namespace's per-reconcile pod List open. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
81c12bc953 |
fix(operator): withhold on any foreign memory policy, not two fields
The coexistence guard treated a namespace as already-governed only when another LimitRange carried a container memory default or max, and said so in its comment. That is not the set of fields that collide with what this reconciler writes. A container min above the default request rejects the pod, and so does a maxLimitRequestRatio, since a 32Gi default against a 32Mi default request is a ratio of 1024:1 and no cap allows it. So the namespace-wide admission failure the guard exists to prevent stayed reachable through fields the guard did not read, and kube-system is where it is likeliest, this feature having deliberately brought it in scope. Enumerating the colliding fields does not fix it, because at the Pod scope the collision is not decidable here. Whether a per-container default of 32Gi breaches a pod-wide max depends on how many containers the pod has, and the pods in question do not exist yet: any arithmetic about them is a guess, and a wrong guess costs a namespace its entire pod admission. So the rule is ownership rather than arithmetic. If anything else in the namespace has an opinion about memory, at either scope and in any field, the namespace is not this reconciler's to default. Narrow still on the resource, because nothing here writes cpu or storage and a LimitRange bounding those must keep coexisting. This is the rule the tenant-namespace exclusion already rests on. The pod scan no longer skips Failed pods, only Succeeded. A pod that ran to completion is never recreated from its spec, but an eviction or a node crash leaves a Failed carcass holding the same oversized request that its controller will recreate, and that carcass is frequently the only surviving evidence the namespace needs a raised ceiling, since the shape the scan looks for cannot be admitted once the namespace has settled. Dropping it lowered the ceiling under a workload that was coming back, which is what the grace period exists to prevent. A grace stamp dated ahead of the clock is clamped to now. Unclamped it held the ceiling for the skew plus the grace period, silently, so an hour of skew bought ninety minutes instead of thirty. Clamping never shortens the wait, it only stops it being extended. The two flag defaults are named constants the flag registration and its test both read. The test previously duplicated the literal and still asserted 4Gi after the default moved to 32Gi, so it misinformed a reader about the shipped value and pinned nothing. The disable claim is narrowed to what the code guarantees. "Removes any previously created one" was universal, but the reconciler only runs for namespaces an active Package still targets, so a namespace whose Package was removed keeps its LimitRange. The flag help, values.yaml and the operations page now say that, and name the managed-by label for finding leftovers by hand. The test asserting a Pod-scoped memory max should be ignored is gone, since under the ownership rule a Pod-scoped memory bound withholds. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
30ee57f892 |
fix(operator): route every downward ceiling move through the grace period
The grace period guarded only the branch where the scan came back empty, which left the same defect standing in the branch where it finds something smaller than the ceiling already in place. A namespace raised to 16Gi for a pod visible only as a live Pod, with a second oversized pod at 8Gi beside it, loses the larger pod to a node drain. The scan still finds the 8Gi one, so the empty-scan guard never engages, and the ceiling was written straight down to 8Gi -- rejecting the larger pod's recreation exactly as writing the configured limit would have. A blocker smaller than the standing ceiling is not evidence that a larger one is gone. So the rule is now about direction rather than about why: the ceiling the evidence justifies is computed first, and any move that lowers it goes through the grace period, whichever of the three reasons prompted it -- an empty scan, a smaller blocker, or the configured limit itself being lowered beneath a standing ceiling. The eventual drop lands on what the scan justifies rather than on the configured limit, so a smaller blocker that is genuinely still there keeps the ceiling it needs. The raise is also no longer logged when it changes nothing. A namespace holding a permanently oversized pod re-justifies the same ceiling on every reconcile, and announcing that put a line in the log for every such namespace on every HelmRelease status change. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
9fe93a3d7e |
docs(operator): fix three claims that overstated what the code does
The static-pod note said containers built from an on-disk manifest are unaffected "in both directions". True of the container, which the kubelet runs from disk whatever the API says, but the mirror pod is an ordinary API object that passes admission like any other, so a static pod declaring a memory request above the ceiling with no limit of its own has its mirror pod rejected -- leaving the container running with no representation in the API. Worth naming, since kube-system is in scope by design. The scan does cover it: a mirror pod is a live Pod. The ReplicaSet exclusion was justified on live ones being copies of their Deployment's template. That does not reach an ownerless ReplicaSet, which at replicas 0 has neither pods to be seen through nor a Deployment template standing in for it. It stays out of scope -- nothing here ships a bare ReplicaSet -- but as a gap left open rather than one the stated reason closes. The delete guard's comment claimed the enabled apply always adopts and stamps a same-named foreign object. It does not when that object's spec already matches, because then the apply is skipped entirely. The outcome is the same one the guard gives on purpose, but it is reached by the apply not running. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
e131a49514 |
feat(operator): default the system namespace memory ceiling to 32Gi
The guard in front of this default reads declared memory requests. It cannot see a container's real working set, so a system component that requests little but legitimately uses more than the ceiling, and declares no limit of its own, was handed a deterministic kernel OOM on its next recreation in exchange for leaving the Talos handler's victim set. Since the feature ships enabled, that trade fell on the whole fleet on upgrade. It is not a trade this feature has to make. The handler discards a cgroup for carrying a memory.max at all, whatever its value, so immunity does not require a fitted ceiling -- only a real one. A default far above any plausible working set buys the immunity and leaves nothing for the ceiling to bind on, which also keeps it clear of the failure mode this repository already carries a scar from, where an undersized limit converted a rare pressure-driven kill into a reliable one. Nothing about scheduling changes: only requests are scheduled against, and the paired default request stays 32Mi. An operator who wants the ceiling to bind lowers it, per component in a chart or fleet-wide with this knob, once real usage is measured -- which is the direction that needs evidence, and now the direction that needs it rather than the default. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
ad3c9ef04e |
fix(operator): stay out of a namespace another LimitRange governs
The reconcile only read its own LimitRange by name, so a system namespace already carrying an administrator's LimitRange got a second one beside it. LimitRanger applies defaults from every LimitRange in the namespace and then validates the pod against every one, and nothing orders them, so the effective container ceiling moved between the administrator's value and this one for reasons no chart author could see. That is the same hazard this feature cites to keep itself out of tenant namespaces, and it does not stop being one because the namespace is a system namespace. A max is the sharper half. This reconciler writes a default and no max, so an administrator's LimitRange carrying a container memory max below the configured default had every container that declares no limit of its own rejected: the default is written first, then validated against the max it exceeds. That takes out a whole namespace, and it reaches kube-system, which this feature enters for cozystack-scheduler and which is where a distribution's or an administrator's own LimitRange is likeliest to sit. Such a namespace is now left alone and logged, and anything this reconciler wrote there earlier is withdrawn rather than left sitting beside the foreign object, since that object is half of the ambiguity. The check is deliberately narrow -- only a container memory default or max collides with what is written here -- because read more widely it would be a blanket opt-out that an unrelated cpu policy could trip. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |