mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-03 02:27:01 +09:00
docs/roadmap-dco
542
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 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
|
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> |
||
|
Myasnikov Daniil
|
5630954e34 |
fix(operator): hold a raised memory ceiling before lowering it
A raised container memory ceiling was recomputed from the scan on every reconcile, so a single scan that found nothing lowered it back to the configured limit. An empty scan is not evidence that the oversized request is gone: it is equally consistent with the pod being between deletion and recreation. For a blocker that appears only as a live Pod, which is a class this repo ships -- every CNPG Cluster in a system namespace builds its instance pods directly rather than through a StatefulSet -- an ordinary Talos node drain opens that window, and reconciles follow HelmRelease status churn, of which a node upgrade produces plenty. The ceiling was lowered under a pod that was merely being rescheduled; its recreation was then defaulted a limit below its own request and rejected at admission for good, because the rejected pod never exists so no later scan can find it, and the namespace settles at the configured limit where the pod half of the scan stops running at all. The drop was not even logged. Lowering now waits until the same emptiness has held for memoryCeilingDropGrace, with the instant it was first observed recorded on the object so it survives an operator restart and a change of leader. A wall clock rather than a count of consecutive clean scans, because the reconcile rate rises exactly when the risk does and any small count can be burned inside one rescheduling window. A blocker seen again cancels the countdown, an unreadable stamp restarts it, and both the hold and the drop are logged. The steady-state comparison now also covers the two metadata fields this reconciler manages, because the grace stamp is one of them. That incidentally fixes the managed-by label never being re-stamped when only metadata had drifted, which had left turning the knob off unable to remove an object the operator did own. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
3c1d8d6988 |
docs(operator): bound the managed-by guard to the disabled state
The comment on the label check in deleteSystemDefaultsLimitRange reads as a general promise not to touch an administrator's own LimitRange of that name. It is narrower than that, and the difference is worth writing down: while the feature is enabled the apply takes the name over with ForceOwnership and stamps the managed-by label itself, so a hand-written cozystack-system-defaults is adopted and then deleted here when the knob goes off. That is the intended SSA behaviour, not a defect. What the label actually buys is that a LimitRange of that name the reconciler never wrote to survives the knob, which is a real guarantee and a smaller one. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
accb4f9e1b |
refactor(operator): drop the dead error check from the settled-state guard
The guard read `err == nil && apiequality.Semantic.DeepEqual(...)`, which looks like it consults the earlier Get and does not. That Get's error was reassigned by the scan two statements up, and the scan's non-nil case has already returned, so the clause is always true by the time it is evaluated. It was harmless only by accident: a namespace whose Get returned NotFound carries an empty spec, which never compares equal to the desired one, so the apply happened anyway. Removing the clause makes that the reason rather than a coincidence sitting behind a condition that cannot fire. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
ead330ccc3 |
fix(operator): keep scanning pods while the ceiling stays raised
The pod half of the memory scan was skipped whenever any container memory default was in force. That is the weaker of the two available conditions, and it silently retracted a ceiling this reconciler had just raised. The default written into a system namespace is either the configured limit or a ceiling raised above it to clear an oversized request, and the raise is recomputed from the scan every reconcile rather than read back off the object. So a raise that only the pod half could justify -- a live pod requesting more than the limit with no limit of its own, in no workload template the scan can read, which is what an operator building pods from its own custom resource produces -- lasted exactly one reconcile. The raised LimitRange satisfied "in force", the next reconcile skipped the pod list, the scan came back empty, and the configured limit was reapplied underneath a still-running pod. That pod's next recreation was rejected with "must be less than or equal to memory limit", and because the skip stayed in effect nothing ever raised the ceiling again: a healthy component goes down at a node reboot and stays down, with the revert unlogged. Lowering the knob reached the same place with no race in it. An operator running at 16Gi has pods admitted with requests only that ceiling allowed; dropping the flag to 4Gi wrote 4Gi straight over them. Gate the skip on the namespace already defaulting memory at exactly the configured limit instead. That keeps the pod list alive for as long as a namespace sits raised, which is what lets the raise be re-justified or dropped on evidence rather than frozen or forgotten, and it still skips the list in the steady state the skip was added for. The comparison is by value for the same reason the spec comparison is semantic: 4096Mi and 4Gi are one ceiling. The cost is that a raised namespace reads the pod list every reconcile. That is the price of being able to lower the ceiling again, bounded by how many namespaces sit raised -- none, once the oversized requests are gone. Three tests cover it, each of which fails on the old condition: a raised ceiling held while the pod needing it runs, the same ceiling dropping back once that pod is gone, and the configured limit falling below a ceiling already in place. A fourth pins the notation equality. The fixture comment on the steady-state test claimed its 8Gi pod could not exist beside a settled 4Gi LimitRange; it can, through a narrow window, so it now says so. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
26a1ce5be0 |
test(operator): drop the incidental fixture from the no-pods case
The scaled-to-zero test seeded a spec-less LimitRange, which is not part of the case it names. It is the first application in the namespace that makes a workload with no pods invisible to a pod-only scan, so start from no LimitRange at all and let the assertion rest on the workload rather than on the fixture. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
b62fac474c |
refactor(operator): move both server-side applies off the deprecated client.Apply
client.Apply as a patch type is deprecated in controller-runtime v0.23.1 in favour of Client.Apply taking a runtime.ApplyConfiguration. Convert both call sites — the system defaults LimitRange, new on this branch, and the Namespace apply that predates it — keeping the same field owner and force-ownership options. The LimitRange keeps its typed builder, because that is what the spec comparison and the tests read, and gains a small function that restates it as an apply configuration immediately before the wire. The Namespace apply now sends only the labels and annotations the reconciler actually sets, rather than a whole typed object whose spec and status were empty. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
c7af79b734 |
fix(operator): only delete the LimitRange this reconciler owns
deleteSystemDefaultsLimitRange matched on name alone, and the name is not reserved: nothing stops an administrator creating a LimitRange called cozystack-system-defaults by hand. Turning the knob off would then silently remove policy this operator never wrote, in every system namespace at once. Stamp app.kubernetes.io/managed-by on the object the reconciler applies and require it before deleting. Anything else is left where it is, which is also the right answer if the name is ever taken over by another component. Nothing in the wild carries the old unlabelled shape, since the feature has not shipped. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
dbc5e5ea13 |
perf(operator): read before deleting the disabled LimitRange
With the limit set to 0 the disabled path runs for every system namespace on every Package reconcile and issued a DELETE each time, swallowing the 404 that nearly all of them returned. That is a write attempt per namespace per reconcile, forever, and every one of them lands in the audit log. Get from the informer cache first and return when there is nothing there. The test now counts deletes, so a second reconcile over an already-empty namespace has to stay at zero rather than merely not erroring. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
89b78cbf20 |
docs(operator): drop the duplicated field from the ceiling-raise log
request and raisedLimit were logged with the same value, because the raised ceiling is the blocking request. Keep request, which names the thing an operator has to go and change, and say in the message that the ceiling becomes it. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
87f79c17c3 |
fix(operator): keep scanning templates after the LimitRange settles
The steady-state fast path returned as soon as the LimitRange matched the configured spec, on the grounds that LimitRanger gates anything a scan could still find. That holds for pods and not for templates: a Deployment added to a settled namespace with a memory request above the ceiling and no limit of its own is admitted exactly as written, and only the pods it then tries to create are rejected with "must be less than or equal to memory limit". Nothing raised the ceiling, because every later reconcile returned at the same line, so a chart bump became a rollout that could never produce a pod and logged nothing about it. Split the scan instead. Templates are read on every reconcile; the pod list, which is the expensive half, is read only while no memory default is in force in the namespace — once one is, LimitRanger fills in a limit for every container admitted without one, so a container carrying a request with no limit beside it can no longer come into existence. The skip keys on the default actually being in force rather than on the object existing, so a LimitRange whose spec was stripped scans pods again. The write is now guarded by a semantic comparison against the spec the scan settled on rather than against the configured spec, so a namespace already sitting at a raised ceiling stops taking an identical apply on every reconcile as well. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
c9cacda1a0 |
docs(operator): pin the admission order by rule, not plugin index
The scan's justification for treating a VerticalPodAutoscaler as harmless rested on LimitRanger being "position 6 of AllOrderedPlugins" and MutatingAdmissionWebhook being "35". Both numbers are properties of one release rather than of the ordering, and they had already drifted out from under the comment. Under k8s 1.35, which this tree builds against, LimitRanger is the 6th entry but MutatingAdmissionWebhook is the 33rd; index 35 is ResourceQuota and the 35th entry is ValidatingAdmissionWebhook, neither of which mutates, so the sentence named the wrong plugin under either counting convention. In 1.31 the same plugin sat at 30, and SecurityContextDeny being dropped in 1.35 is what moved LimitRanger. Restate it on the invariant the list asserts about itself -- webhook, resourcequota and deny plugins must go at the end -- with LimitRanger compiled in above that marker and every plugin that can rewrite a container's resources below it. That survives a release bump where an index does not, and it covers MutatingAdmissionPolicy as well, a second mutator that the positional wording predated and never accounted for. Record what the order buys, which the old wording left implied. Run the other way round, a plugin mutating before LimitRanger would write a request into a container that has no limit yet, LimitRanger would then default the limit to the configured ceiling, and core validation would reject the fully mutated pod. The scan can see none of that -- the request is in no template and the pod is never admitted -- so the order is the only thing keeping VPA out of the residual gap this comment already records for controlledValues: RequestsOnly. Fix the matching rationale on the live-pod case in TestFindRequestAboveDefaultLimit, which still claimed the pod's request was raised by a webhook after the template was rendered. The ordering rules that out; what the case actually rests on is a pod admitted before the LimitRange existed, or one its controller builds from a custom resource no template in the scan can show. No behaviour change, and no new test: a container that carries its own memory limit is already pinned out of the scan's scope for a template, in TestFindRequestAboveDefaultLimit, and for a live pod, in TestReconcileSystemDefaultsLimitRangeAppliesWhenLargeRequestCarriesItsOwnLimit. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
16b8ea4f0e |
docs(operator): correct the VPA rationale on the request scan
The comment argued that the danger this scan guards against is a VPA webhook writing a request above the defaulted limit, which is then rejected at admission. Admission ordering rules that out. LimitRanger is position 6 of AllOrderedPlugins and MutatingAdmissionWebhook is 35, so the LimitRange has already defaulted the container's memory limit before VPA is called, and VPA rescales that limit in proportion to the request it writes (GetProportionalLimit, under the default controlledValues: RequestsAndLimits). Request and limit stay consistent and the pod is admitted. Rewrite it around what can actually happen: a request already in the spec when it reaches admission with no limit beside it. In this tree that means an operator lowering --system-namespace-memory-limit below a statically declared request -- the largest is 2Gi, in packages/system/rabbitmq-operator, so the 4Gi default clears them all today. Two narrower shapes are real too: a pod that predates the LimitRange, whose own controller may build it from a custom resource no workload template in this scan can show; and a VPA set to controlledValues: RequestsOnly, where nothing rescales the defaulted limit and the rejection returns. The second doubles as the residual gap, since a first pod rejected outright appears in neither a template nor a pod list. The scan itself is unchanged; only the justification was wrong. Also record why withholding the LimitRange had to go rather than merely being suboptimal: in a namespace with no LimitRange nothing defaults a memory limit, so the next VPA-sized pod arrives with a large request and no limit, the scan finds it again, and the namespace is withheld again. The old behaviour could never get its foot in the door in cozy-monitoring. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
d682158d1f |
test(operator): pin the tenant namespace boundary
One strings.HasPrefix keeps this feature out of tenant namespaces, and nothing tested it: "tenant-" appeared twice in package_reconciler.go and not once in the test file, so replacing the check with isSystem := true left the whole package green. What it protects is not subtle. packages/apps/tenant ships tenant-range-limits, which defaults container memory to 128Mi in every tenant namespace that declares resourceQuotas. A system LimitRange landing beside it leaves two LimitRanges defaulting the same containers, and which of them wins is decided by the order LimitRanger happens to iterate them -- neither chart controls that -- so a tenant container's default memory limit would move between 128Mi and 4Gi with nothing in the tree changing. Reconcile a variant with one system and one tenant namespace, and assert no LimitRange is written for the tenant one. The system namespace in the same run is the positive control: without it the test would still pass if LimitRanges stopped being written anywhere at all. The cozystack.io/system label is asserted too, since the same boundary gates both. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
2e1ab6682c |
fix(operator): raise the namespace ceiling instead of retracting it
When a container in a system namespace requests more memory than the configured default, the guard used to withhold the LimitRange and delete any it had applied earlier. That leaves the namespace with no default memory limit at all, which is the precise state this feature exists to remove: without memory.max on the pod cgroup the Talos OOM handler keeps the pod in its victim set, and a tenant workload's memory pressure gets paid for by a system component. Default that namespace's memory to the offending request instead. A looser ceiling still sets memory.max, so the pod scores zero in victim selection exactly as it would at the configured limit -- 8Gi protects it as well as 4Gi does, and both protect it infinitely better than nothing. The raise needs no knowledge of where the request came from, and it undoes itself: once the request is gone the scan finds nothing, desired falls back to the configured limit, and the next reconcile applies it. This is not a hypothetical namespace. packages/system/monitoring pins maxAllowed.memory at 8Gi for vmselect and vmstorage, packages/system/monitoring-agents pins 6G for vmagent, all three run updateMode: Initial, and no monitoring template declares a memory limit. Initial never evicts, so those requests do not come down on their own: under the old behaviour cozy-monitoring, which holds the largest memory consumers on the platform, was permanently excluded from the feature on exactly the clusters it was written for. A raised namespace no longer matches the base desired spec, so it scans on every reconcile. That is deliberate: the raise has to track the current largest request in order to be able to drop back, and caching it would freeze a ceiling that nothing would ever lower again. The set of raised namespaces is small. The log moves from Error to Info. Nothing is wrong when this fires; the operator is reporting a ceiling it chose. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
c65db1ad52 |
perf(operator): skip the memory scan when the LimitRange is settled
reconcileSystemDefaultsLimitRange runs for every system namespace on every Package reconcile, and Package reconciles follow HelmRelease status churn through Owns, so on a platform with dozens of system namespaces the pod and workload scan behind the guard is by far the most expensive thing in the function -- and it is uncached by design, because caching it would mean a cluster-wide Pod informer in an operator whose job here is to bound memory. Build the desired LimitRange first, read the existing one from the informer cache, and return without scanning when the two specs already match. That is safe because the LimitRange being applied is itself the guarantee: from that moment LimitRanger rejects any new pod whose memory request exceeds this default, so the only requests a scan could still find are ones that predate the LimitRange, on pods already running, and there is nothing it would do differently about them. Steady state becomes one cached GET. The comparison is apiequality.Semantic, not reflect.DeepEqual, because resource.Quantity carries its parsed format and cached string: 4Gi and 4294967296 are the same ceiling and must not read as drift, or every namespace would scan and re-apply forever. The test counts List calls rather than only checking the resulting LimitRange, which cannot tell a skipped scan from a repeated one that reached the same answer, and seeds an 8Gi pod so a scan that ran would also change the object. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
b55c4f347a |
docs(operator): keep the residual-gap note on the request scan
The pod-only scan documented what it could not see: a request that only ever exists post-admission, on a workload whose first pod is rejected outright. Reading pod templates narrows that gap rather than closing it, and the rewritten comment dropped the note entirely, which reads as a claim that nothing is left. Restore it in its narrower form: a template under the limit, a webhook that raises the request past it, and a first pod rejected before it exists, is still invisible to this scan. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
10acfe025d |
test(operator): cover both terminal pod phases in the skip
findRequestAboveDefaultLimit skips a pod in PodSucceeded or PodFailed, because neither is ever recreated from that spec. Only the Succeeded half had a fixture, so deleting the PodFailed clause left the whole ./internal suite green -- half the condition was uncovered. Give the case a Failed pod as well. Deleting either clause now fails it. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
654412acd2 |
fix(operator): scan workload templates for oversized memory requests
reconcileSystemDefaultsLimitRange withholds the LimitRange when a container in the namespace requests more memory than the configured default limit, because LimitRanger would then default a limit below that request and the API server rejects the pod. The scan behind that decision listed pods only, so it could not see a workload that currently has no pods: a Deployment or StatefulSet at replicas: 0, a CronJob between runs, a Job with spec.suspend. Those are ordinary steady states rather than corner cases. The namespace looked safe, the LimitRange was applied, and the pod was rejected at the next scale-up or the next schedule with "must be less than or equal to memory limit of 4Gi" -- a workload that cannot come back, discovered at the moment somebody needs it. Scan the pod templates of Deployments, StatefulSets, DaemonSets, CronJobs and Jobs as well as live pods. Both halves are needed: a template is the only place a scaled-to-zero workload appears at all, and a live pod is the only place a request that the VPA admission webhook raised after admission appears. ReplicaSets are deliberately left out -- a live one copies its Deployment's template, and the older revisions a Deployment keeps carry superseded templates that only an explicit rollout undo would ever instantiate, so scanning them would withhold the LimitRange over a request replaced releases ago. Jobs are named separately because not every Job has an owner whose template would cover it, and a finished one is skipped for the same reason a terminated pod is. A suspended CronJob is not skipped: suspend is the same dormancy as replicas: 0, undone by the same kind of human action, and exempting it would leave the gap open for one kind. Templates are scanned before pods and the comparison stays strict, so a workload and its own pods report once, as the workload -- the object an operator has to edit, and one that outlives pod names. A live pod outranks its own template only when its request is strictly larger, which is exactly the webhook-injected case. memoryRequestBlocker therefore carries Kind/name instead of a bare pod name, and the log key is "workload" rather than "pod". Every added List fails closed, which the caller already relies on: a scan that cannot read is not evidence that applying is safe, so the namespace is skipped and logged instead. The test for that seeds a sentinel LimitRange, because asserting the object still exists cannot tell "left alone" from "applied blind" -- a swallowed List error applies a fresh LimitRange that exists just the same. RBAC needs no chart change: the operator's ServiceAccount is bound to cluster-admin, so it can already list these kinds. The kubebuilder markers are extended anyway to record what the controller reads. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
7cc090b05f |
test(operator): pin the largest-offender scan to all three fields
The "largest offender is reported" case asserted only the container name, and systemPod named every container "app", so the assertion held whichever of the two pods findRequestAboveDefaultLimit had picked. The comparison that keeps the largest request was uncovered. Name the fixture container after its pod, assert the reported pod, container and quantity together, and give the case three offenders whose largest sits in the middle of the name-ordered pod listing, so keeping the first, the last or the smallest offender all name a different pod and quantity. The init-container case now pins the same three fields. No production behaviour changes. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
546d8e6a54 |
feat(operator): default system namespace memory via LimitRange
The Talos userspace OOM handler (v1.12+) picks its victim by ranking
cgroups with `memory_max.hasValue() ? 0.0 : {Besteffort: 1.0, Burstable:
0.5, ...}[class] * memory_current`, discarding every cgroup that scores
zero. Only a memory limit sets memory.max on the pod cgroup, so only a
limit takes a pod out of the candidate set: a request merely demotes
BestEffort to Burstable, which under the v1.13.6 default
strictCgroupClassOrdering still means "killed second". PriorityClass has
no effect at all, because the handler walks cgroupfs and never talks to
the API server.
Victim selection is decoupled from the trigger, so the cgroup that
caused the pressure and the cgroup that gets SIGKILLed are unrelated by
design. Tenant workloads generally carry limits and system components
generally did not, which inverted the intended order: a tenant workload
thrashing against its own limit drives node-wide memory PSI, and
metallb-speaker or linstor-satellite is killed for it while the workload
responsible is never touched.
An audit found roughly 70 workloads across ~30 packages with at least
one container missing a memory limit, so per-chart resources would
neither cover the current set nor keep covering it. Maintain a
LimitRange in every system namespace instead, at the single choke point
that already creates them and already distinguishes system from tenant
(reconcileNamespaces). Tenant namespaces are left alone because
packages/apps/tenant owns LimitRange policy there.
The limit is a ceiling, not a reservation: it defaults well above real
usage (4Gi against a largest observed system request of 512Mi).
defaultRequest is set explicitly and small, because Kubernetes otherwise
defaults each request to the limit and reserves that much at schedule
time. Setting the limit to 0 disables the LimitRange and removes any the
operator previously created.
Two things keep the feature from being able to hurt more than it helps.
A LimitRange default only reaches a container that declares no memory
limit of its own, and the API server then rejects a request above its
limit. So before applying, scan the namespace for a container that
requests more than the configured limit and sets none, and where one
exists skip the namespace, retract any LimitRange applied earlier, and
log the pod, container, request and limit. This is not hypothetical:
packages/system/monitoring and packages/system/monitoring-agents ship
VPAs for cozy-monitoring with maxAllowed.memory of 8Gi and 6G under
updateMode: Initial, so the VPA admission webhook -- which runs after
the in-tree LimitRanger -- can write a request past 4Gi that no chart in
the tree declares. That is why the check runs against live pods rather
than rendered templates, and why it reads through APIReader rather than
starting a cluster-wide Pod informer.
And no failure inside the feature propagates: a LimitRange error, or a
pod list that cannot be read, is logged and stepped over instead of
failing the Package reconcile. A namespace without a LimitRange is back
to the behaviour of every earlier release, while a namespace that never
reconciles leaves its components uninstalled.
Assisted-By: LLM
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
|
||
|
Aleksei Sviridkin
|
520af57905 |
chore: drop private cluster, host and personal names from the tree
Private infrastructure names, a real hostname, a commented-out internal mirror block, an internal product name, and a reviewer's name in a comment do not belong in a public repository; drop them and replace what needs a stand-in with neutral placeholders. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
d2e3eb4f82 |
fix(kubernetes, e2e): unblock autoscaling below min size, size the computeplane pool, fix the backupcontroller build (#4768)
## What this PR does Makes the backupcontroller test package compile again on main. #4587 added a flavor argument to `createCNPGBackupArtifact` and `marshalCNPGBackupSnapshot`, and #4669, whose CI ran before that, added two test calls with the old argument lists. Both PRs were green on their own, and together they break `go test ./internal/backupcontroller/`. The two calls now pass the same default flavor as the other tests in the file. The second commit fixes an e2e suite that fails the required E2E check here. It is the `computeplane` suite from #4713, and its worker pool was one node with a single vCPU. After the kubelet reservations that node has 900m of CPU to allocate. The tenant addons (cilium, coredns, csi, ingress-nginx, metrics-server, cert-manager) request exactly 900m between them. If cert-manager's startupapicheck hook pod is created last, it can't be scheduled and the install times out after 10m. The reinstall after remediation runs into the same full node. Whether that pod comes last depends on install order, so the suite passed on #4713 and failed here. The pool now has two workers with one vCPU each, the same shape the kubernetes suites use. One vCPU per worker stays on purpose, because of #3513. Two workers exposed a deadlock in the tenant cluster-autoscaler. The suite's pool is created at zero replicas and then gets a min size of 2, which CAPI does not apply to the live replica count on upgrade (#4786). The autoscaler's scale-up for a pending pod asks for one node, and the clusterapi provider refuses it because one is under the min size, although it is an increase. That refusal backs the group off, and the min-size enforcement skips groups in backoff, so the pool never leaves zero. Upstream had the same report in kubernetes/autoscaler#4942, closed as stale, and it is now tracked in kubernetes/autoscaler#10400. Our autoscaler image already patches the mirror case for max size, and the patch now refuses only a decrease below the min size. `go vet` and `go test ./internal/backupcontroller/...` pass, and so do the bats pins for the computeplane suite. The autoscaler patch adds a test for each direction, and the clusterapi package tests pass with it applied to 1.32.0. ### Screenshots Not a UI change. ### Downstream repositories - [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 NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved cluster autoscaling for worker groups outside configured size limits: groups below their minimum can scale up toward it, and nodes can be removed when a group exceeds its maximum. Scale-down requests below the minimum remain restricted. * **Tests** * Updated backup and restore test calls with PostgreSQL and empty flavor values. * Updated ComputePlane checks for a two-worker pool and clarified cert-manager installation timing expectations. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
ff3c409881 |
fix(backups): refuse a foreign -orig PVC and record the kept disk's reclaim policy (#4671)
## What this PR does Two defects in `renamePVC` (`internal/backupcontroller/velerostrategy_controller.go`), the step of an in-place VMInstance restore that keeps each disk as `<dv>-orig-<shortHash(RestoreJob name)>` when `keepOriginalPVC` is true, the default. ### A taken `-orig` name skipped the rename, and the restore went on `renamePVC` returned `nil` as soon as a PVC with the `-orig` name existed (`main:1263-1266`), whatever that PVC was. `prepareForRestore` then reached Step 4 (`main:1172-1183`), which deletes the DataVolume of every disk regardless of what Step 3 did, because a Step 3 error was only ever turned into a Warning event (`main:1165-1168`). The restore therefore ran exactly as `keepOriginalPVC: false` does and reported `Succeeded`: the live disk was not kept, and the `-orig` name held a different, older disk that the user takes for the pre-restore copy. When that name can already exist: - A RestoreJob created under the name of a deleted one. The hash covers only the RestoreJob name, and nothing in the controller ever deletes an `-orig` PVC, so the predecessor's PVC is still there. This is the realistic case, and it is what a scripted restore with a fixed RestoreJob name runs into on its second run unless the `-orig` PVC was removed in between. - Two RestoreJobs with different names whose 4-hex-digit hashes collide (16 bits) while restoring the same disk in the same namespace. Rare, but nothing guards it. - A reconcile of the same RestoreJob after a controller restart in the middle of a rename. The name is ours, but the early return treated a half-done rename as complete: the original PVC stayed in place and the PV stayed bound to it. - Truncation does not apply: the name is not truncated, and one longer than 253 characters makes the `Create` fail, an error that was swallowed in the same way. Each `-orig` PVC now carries the UID of the RestoreJob that created it, in `backups.cozystack.io/restore-job-uid`. `prepareForRestore` checks every `-orig` name before it suspends or halts anything, and fails the RestoreJob, naming the PVC, when any of them is held by a PVC another RestoreJob created. When the PVC is ours, `renamePVC` resumes: it deletes the original PVC only while it still claims the kept PV, and repoints the PV unless it already points at the `-orig` PVC, so a rerun is a no-op. Any other rename error now holds the restore back before Step 4 deletes a DataVolume, and the restore is retried, instead of the error becoming an event (see the next section). An `-orig` PVC created by an earlier controller has no UID annotation and is refused like any foreign PVC. The only RestoreJob that loses by this is one whose controller was upgraded after its rename and before its Velero Restore was created; it fails with the message above rather than going on. ### The rename read its own write back through the cache `renamePVC` created the `-orig` PVC, then read it back through the cached client to learn its UID for the PV's claimRef (`main:1314-1316`). The informer often has not seen a PVC created a moment earlier. On our lab, running Cozystack v1.6.3, an in-place restore of a two-disk VM with `keepOriginalPVC: true` renamed and restored the system disk, while the data disk lost the race, with this event on the RestoreJob: `Failed to keep old PVC vm-disk-e2e-dc-fix-data: failed to get -orig PVC vm-disk-e2e-dc-fix-data-orig-a574: PersistentVolumeClaim "vm-disk-e2e-dc-fix-data-orig-a574" not found`. As the error was only an event, the data disk's DataVolume was deleted and its original PVC stayed in place. Velero skipped that PVC and created no DataDownload for it, so its content was not restored, yet the restore finished `Succeeded`. The `-orig` PVC was left `Pending` on the same PV, which was by then `Retain`. `Create` already fills in the UID, so the read-back is gone. The other reads the rename decides on (the `-orig` PVC, the original PVC and the PV, before and after its own writes) go through the manager's uncached reader, wired as `APIReader`. The PV rebind retries on conflict, since the PV controller also writes the PV once the original PVC is deleted. A rename that fails for any reason other than a foreign `-orig` name requeues the restore rather than failing it. The DataVolume is still there, and the retry finds its own `-orig` PVC by the RestoreJob UID and resumes where it stopped. A cause that never clears keeps the RestoreJob `Running`, with a Warning event on each attempt. #4673 adds the same `APIReader` field and `apiReader()` helper to `RestoreJobReconciler`, for its finished-job guard. Whichever of the two lands second drops its own copy when it rebases. ### The PV's reclaim policy was overwritten without being recorded `renamePVC` switches the PV to `Retain` so it survives the deletion of the original PVC (`main:1289-1293`), and the policy it had was lost. I looked for a path in the controller that releases or cleans up `-orig` PVCs, and there is none: `cleanupOnDelete` on a Velero RestoreJob removes only the Velero `Restore` and the resource-modifier ConfigMaps, and no code deletes an `-orig` PVC or touches its PV again. A PV that was `Delete` therefore stays `Retain` for good, and its volume leaks when an operator deletes the `-orig` PVC. The original policy is now recorded in `backups.cozystack.io/original-reclaim-policy`, on the PV and on the `-orig` PVC, following the `backups.cozystack.io/` prefix the controller already uses for `underlying-resources`. A value already present on the PV is kept, as it predates any rename. Since there is no release path to restore it from, `examples/backups/vmi/scenario-user-restore.md` gains a section for the operator who deletes an `-orig` PVC: set the recorded policy back on the PV first, with a snippet that leaves a PV without the annotation on `Retain`. Restoring the policy from the controller itself, for instance once the Velero Restore has completed and the PV is `Bound` to the `-orig` PVC, would close the leak without the operator. I did not do it, because it changes what deleting an `-orig` PVC destroys, from nothing to the volume; that is a maintainer call, and the annotation is what it would read. ### How this fits with #3427 #3427 is complementary, and the two do not touch the same code: it changes `collectUnderlyingResources`, this PR changes `prepareForRestore` and `renamePVC`. Both append tests to `velerostrategy_controller_test.go`, so whichever lands second may need a trivial rebase. - #3427 makes a standalone VMDisk report its own disk, which sends VMDisk in-place restores through this same `renamePVC`. Once both land, the VMDisk path gets the same guard and the same annotation. - Its `examples/backups/{vmi,vmdisk}/cleanup.sh` and the Chainsaw `finally` reclaim each `-orig` PVC by patching its PV to `Delete` before deleting the PVC. That works around, in the e2e teardown, the leak described above. It is correct on the LINSTOR classes, which are `Delete`, but it would destroy a volume whose class was `Retain`. With this PR the scripts can read `backups.cozystack.io/original-reclaim-policy` instead of hard-coding `Delete`. I have not changed them here, since they only exist on that branch. - Its round-trips use fixed RestoreJob names and assert that a `<pvc>-orig-*` PVC exists after the restore. If a failed teardown leaves one behind, today the next run skips the rename and the assertion still passes on the stale PVC; with this PR the RestoreJob fails and names it. - Its cleanup comments describe the `-orig` PVC as a bare ObjectMeta with no labels or owner. That stays true: this PR adds annotations only, so its name-based lookup is unaffected. ### Tests New tests in `internal/backupcontroller/velerostrategy_controller_test.go`, run through `prepareForRestore` with the DataVolume in the dynamic fake client: - `TestPrepareForRestore_KeepOriginalPVC_RefusesForeignOrigPVC`: an `-orig` PVC without the annotation, and one carrying another RestoreJob's UID, both fail the restore with the PVC named, and leave the DataVolume, the live PVC, the PV and the foreign PVC untouched. - `TestPrepareForRestore_KeepOriginalPVC_ResumesOwnRename`: a rename interrupted after the `-orig` PVC was created completes on the next pass, and a second pass changes nothing. - `TestPrepareForRestore_KeepOriginalPVC_FailedRenameKeepsDataVolume`: a rename that fails for another reason (missing PV) requeues the restore without failing it and keeps the DataVolume, and once the PV exists the retry completes the rename. - `TestPrepareForRestore_KeepOriginalPVC_CacheLagsBehindWrites`: the reconciler's cached client serves every read from the objects as they were seeded, through an `interceptor` over the fake client, as an informer that has not caught up with this reconcile's writes would, while `APIReader` sees them. The rename must complete. - `TestRenamePVC_RecordsOriginalReclaimPolicy`: a `Delete` PV and a `Retain` PV both end up `Retain`, with their original policy recorded on the PV and on the `-orig` PVC. With the controller reverted to `main` (only the two annotation constants kept so the tests compile), all four fail: `prepareForRestore() = ready true, nil error` with `DataVolume was deleted although the disk could not be kept` for both foreign cases and for the failed rename, `original PVC still exists` and the PV still bound to it for the interrupted rename, and an empty `original-reclaim-policy` on the PV and on the PVC. For the cache race, the first commit alone fails `CacheLagsBehindWrites` with the lab's error, `failed to keep PVC vm-disk-ubuntu-source: failed to get -orig PVC vm-disk-ubuntu-source-orig-de3e: persistentvolumeclaims "vm-disk-ubuntu-source-orig-de3e" not found`, and `FailedRenameKeepsDataVolume` with a terminal error instead of a requeue. On `main` the race test reproduces the lab outcome: `original PVC still exists`, and the PV still claimed by it. With both commits, `go test -count=1 ./internal/backupcontroller/...` passes. No API type, CRD, chart or values file changes, so there is nothing to regenerate. ### Screenshots N/A, no UI change. ### Downstream repositories Walked the trigger map against the diff: the restore preparation in `internal/backupcontroller/velerostrategy_controller.go`, its tests, and `examples/backups/vmi/scenario-user-restore.md`. No CRD, ApplicationDefinition, values, schema, enum, namespace, label, metric or tooling change, so terraform-provider-cozystack, ansible-cozystack, ccp, talm, cozyhr, cozy-proxy, cozystack-telemetry-server, external-apps-example, examples and community are not reached. `RestoreJob.spec.options` stays opaque to the provider. cozystack/website is reached: `content/en/docs/next/virtualization/backup-and-recovery.md` (and each released copy) tells users to delete the `*-orig-*` PVCs "to free up storage". A PV left on `Retain` frees nothing, and the page should explain the recorded policy and the new failure for a reused RestoreJob name. No follow-up exists there yet, so the box stays empty until one is filed and linked. - [ ] 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 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. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * In-place restores that preserve original PVCs can now resume safely after interruptions, including when reads lag behind recent changes. * Restores stop before suspending the VM if a conflicting `-orig` PVC already exists. PVC rename failures can be retried, while conflicting names remain fatal. * Original PV reclaim policies are recorded and restored before preserved PVCs are deleted. * **Documentation** * Updated the restore guide with disk-reclamation steps and guidance for resolving leftover PVC name conflicts. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
ae738e6047 |
test(backups): pass the flavor in the major-version test calls
Two tests that came with the PostgreSQL major-version check call createCNPGBackupArtifact and marshalCNPGBackupSnapshot with the argument lists from before the flavor parameter was added, so the backupcontroller test package does not compile on main. Both calls take the default flavor the other tests in the file pass. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Mattia Eleuteri
|
5956b459fb |
fix(backups): give each Postgres instance its own default WAL prefix
The default CNPG strategy archived a Postgres app under <namespace>-<name>, inside a destinationPath that is also keyed by namespace and name, and the driver fell back to the Cluster name when a strategy set no serverName. Deleting the app does not empty the bucket, so an app recreated under the same name archived its WALs and base backups into its predecessor's stream: both start on timeline 1, and a later restore of the old archive could pick the new cluster's base backup or WAL segments. barman-cloud refuses a non-empty archive only while .check-empty-wal-archive exists in PGDATA. CNPG removes that file as soon as ContinuousArchiving turns True, and with no plugin attached archive_command is a successful no-op, so the file is gone minutes after initdb, before the first BackupJob attaches the plugin. Suffix the default serverName with the application UID, in the strategy template and in the driver's fallback. The template guards the UID with `with`, so an object without one renders the previous name rather than "<no value>". A Cluster that already has the plugin keeps its serverName, so only clusters never backed up take the new prefix, and restores keep reading the source prefix from driverMetadata. Signed-off-by: Mattia Eleuteri <mattia@hidora.io> Assisted-by: LLM |
||
|
Aleksei Sviridkin
|
3fc45ed183 |
fix(backups, postgres): refuse a restore into another PostgreSQL major (#4669)
<!-- 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 A CNPG restore is physical, so the recovered data files only open on the PostgreSQL major version that wrote them. `reconcileCNPGRestore` purged the target Cluster and its PVCs without comparing the two. Observed on v1.6.3: a PostgreSQL 18 backup restored into a v16 application emptied the target, the `full-recovery` pods then looped on `FATAL: database files are incompatible with server`, and the RestoreJob failed at the 30-minute deadline with the target left empty. An in-place restore into an application whose `spec.version` was raised since the backup ends the same way. The restore now compares the two majors before the purge, ahead of the WAL-archive gate, and when they differ marks the RestoreJob `Failed` with reason `MajorVersionMismatch` and a message naming both versions. Nothing is suspended, patched or deleted on that path. - Target major: the Postgres application's `spec.version` (`v13` to `v18`). That is what the chart renders the recovered Cluster from once the live one is gone, so the live Cluster's image would be the wrong thing to compare. - Backup major: the `cnpg.io/Backup`'s `status.majorVersion`, now copied into the artifact's `driverMetadata` as `cnpg.io/major-version` at backup time, because barman retention deletes the `cnpg.io/Backup` while the artifact stays restorable. An artifact taken before this change falls back to the `cnpg.io/Backup` while it still exists. - When either side is unknown (no record and the `cnpg.io/Backup` gone, a CNPG that predates `majorVersion`, an empty `spec.version`), the restore goes ahead as before and the controller logs it, so existing backups keep restoring. `TestReconcileCNPGRestore_RefusesCrossMajorRestore` drives the real reconcile against a fake client with a live Cluster and PVC on the target and counts every `Delete`. With the check removed, the two refusal cases (18 into a v16 copy, 16 in place into an app moved to v18) and the fallback case fail: the Cluster and the PVC are deleted and the app is patched for recovery. With the check in place they end `Failed` with zero deletes, while the same-major and unknown-version cases still reach the purge. `TestCreateCNPGBackupArtifact_RecordsMajorVersion` covers the `driverMetadata` record. ### 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. --> ### 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. --> - [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: Walked against the diff: it touches only `internal/backupcontroller`, with no CRD, no app `values.yaml` or schema and no ApplicationDefinition change. The new `cnpg.io/major-version` entry lives in the free-form `Backup.spec.driverMetadata` map, which no downstream repository reads by key. ### 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, postgres): a Postgres restore whose backup was taken on another PostgreSQL major version than the target's spec.version now fails before touching the target, instead of purging it and then failing to recover. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Cross-major PostgreSQL restores are now refused before the target is changed. The error identifies the backup and target versions. * Restores proceed when either version is unavailable or cannot be parsed. * **New Features** * Backup metadata now records the PostgreSQL major version when it is available. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
9734a16692 |
feat(postgres): add a postgis image flavor (#4587)
<!-- 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 Adds a `flavor: postgresql | postgis` enum to the `postgres` app. `postgis` runs the cluster on the CloudNativePG PostGIS operand image, so tenants can enable `postgis`, `postgis_raster`, `postgis_topology`, `postgis_sfcgal`, `postgis_tiger_geocoder` and `address_standardizer` through the existing `databases.<name>.extensions`. The default stays `postgresql` and renders exactly the image it rendered before this change. This picks up #2672 by @matthieu-robin, which the stale bot closed after a rebase request, with these changes: - The postgis tags in #2672 (`17.7-3.5`, `16.11-3.5`, ...) do not exist in `ghcr.io/cloudnative-pg/postgis`; the registry answers 404, so the flavor would have ended in `ImagePullBackOff`. The map now uses tags that exist, `<minor>-<postgis>-standard-trixie`, on the same PostgreSQL minor as `files/versions.yaml`. - v18 is supported: PostGIS images for 18 now exist upstream. - `standard` rather than `system`: the chart archives through the barman-cloud plugin since the backup rework, so the operand no longer needs the barman binaries of the deprecated `system` variant. - `hack/update-versions.sh` derives the postgis map from the minors it picks, and `hack/check-postgres-postgis-versions.bats` fails if the two maps drift to different minors or away from `standard-trixie`. - The chart refuses a flavor change on a live `Cluster`. The default images are built on Debian bullseye (glibc 2.31) and the postgis ones on trixie (glibc 2.41): CNPG would roll the swap out as a plain image update, under an initialised data directory, which silently invalidates every index on collatable text, and the postgis-to-postgresql direction also drops the libraries behind every PostGIS object. Only the image name is compared, so minor bumps and registry mirrors keep passing. - `pgrouting` is not claimed: it is absent from the `18.1-3.6.1` build the chart pins and only appears in later postgis builds. A general mechanism (CNPG ImageVolume extensions) was considered and left out: it needs PostgreSQL 18 and Kubernetes 1.35 or the `ImageVolume` gate, so it cannot serve v13-v17 nor most management clusters today, and PostGIS is the only extension CloudNativePG ships as a full operand image. No other extension request exists in this repository. Testing: - `helm unittest packages/apps/postgres`: 145/145, including 6 new cases in `tests/flavor_test.yaml` (default image, postgis on the Cluster and on the init Job, the guard in both directions with a mocked live Cluster, and a mirrored digest-pinned image taking a new minor). Removing the guard makes exactly the two guard cases fail. - `hack/cozytest.sh hack/check-postgres-postgis-versions.bats` passes, and fails on a shifted minor or a `system` tag. - `make -C packages/apps/postgres generate` leaves no diff. - The rendered `ghcr.io/cloudnative-pg/postgis:18.1-3.6.1-standard-trixie` was started locally: `CREATE EXTENSION` for citext, postgis, postgis_raster and postgis_topology succeeds and `ST_Buffer` answers. A three-instance cluster on the `system` sibling of that image has been running PostGIS in production since 2026-09-29. ### 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. --> ### 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. --> - [ ] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [x] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: cozystack/terraform-provider-cozystack#63 (issue: the provider models every field by hand, and the default handling needs a decision there) - [ ] [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 feat(postgres): add a `flavor` field accepting `postgresql` (default) or `postgis`. `postgis` bootstraps the cluster from the CloudNativePG PostGIS image, so PostGIS extensions can be enabled per database through `databases.<name>.extensions`. The flavor is fixed at creation: changing it on an existing cluster is refused, and so is restoring a backup into a cluster of the other flavor, whether through a RestoreJob or `bootstrap.enabled`; move data across flavors with a logical dump. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added a `flavor` option for PostgreSQL clusters. The default `postgresql` flavor remains available, while `postgis` selects an image with PostGIS extensions. - PostGIS images are available for PostgreSQL versions 13–18. - Cluster flavor is fixed at creation. To switch flavors, create a new cluster and migrate data with a logical dump. - **Bug Fixes** - Restores are refused when the backup and target cluster use different flavors. - **Documentation** - Updated PostgreSQL configuration guidance to describe flavor options and limitations. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
f527ef17ee |
test(fluxcontract): accept a timeout-bounded history read
The monitoring suite bounds its HelmRelease history read with `timeout 30 kubectl`, which keeps a hung API call from eating the step's whole budget and the diagnostic with it. The shared-read pin admitted kubectl only bare or through the bounded wrapper, so the two landed side by side and left the unit tests red on main. The pin now takes an optional `timeout [-k N] N` in front of kubectl, the forms the suites already use for bounded reads. The bound is spelled out instead of allowing any prefix, since `$(echo kubectl ...)` would name the expansion without running it; that form, bare and behind a timeout, is now a refuse case. So is a timeout in front of _cozy_guard_kubectl: timeout runs a program, the wrapper is a shell function, and the read would fail every time. The command name has to end at a blank, so `kubectlx` or `_cozy_guard_kubectl_v2` is not taken for either one. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |