mirror of
https://github.com/cozystack/cozystack.git
synced 2026-10-27 07:14:04 +09:00
docs/agents-dev-principles
96
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 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> |
||
|
IvanHunters
|
b0e07cadbb |
fix(operator): rebuild each changed ArtifactGenerator generation once (#4594)
<!-- 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 After an upgrade, an ArtifactGenerator can report Ready on its new spec while its artifacts were built from the old one, and source-watcher never rebuilds them. That is how the bucket-system chart in #4593 shipped without cozy-lib. The cause is upstream (fluxcd/source-watcher#384). source-watcher builds from the spec it read when the reconcile started, but the final status patch stamps Ready with the live generation. An upgrade changes the packages source and the copy rules within seconds, so a spec change often lands during a build. The generator status has nothing that tells a stale build from a good one. So the operator now rebuilds every changed generation once, after source-watcher first reports it Ready, through the force-drift path the operator already uses for fluxcd/pkg#934. An unchanged spec rebuilds to the same digest, so ExternalArtifacts and HelmReleases do not move. Generation 1 is skipped, since the race needs a spec change during a build. On the first rollout every generator past generation 1 gets one extra rebuild, and its PackageSource shows `Unknown/AwaitingSourceWatcherRecovery` until that rebuild lands. Nothing waits on that condition today. The same rebuild repairs clusters already stuck like #4593. The recovery messages on the PackageSource no longer blame fluxcd/pkg#934, because the same path now serves both cases. ### How it was tested Unit tests in `internal/operator/packagesource_reconciler_test.go` cover the forced rebuild on generation 2 and on a later generation, every case where no rebuild must happen, the rebuild settling without a second one, both failure orders (force fails, recording the generation fails), and a stale recovery counter. Each condition of the new predicate was mutated once to confirm a test fails. I also ran this commit's operator image on an arm64 test cluster running v1.7.0-alpha.3, the one where #4593 was found. Before that I had repaired its bucket generator by hand: deleting the stale ExternalArtifact and requesting a reconcile produced an artifact with `charts/cozy-lib` and both bucket HelmReleases went Ready. With the patched operator, the only generator past generation 1 got exactly one forced rebuild and the verified-generation annotation. source-watcher reported all its ExternalArtifacts up to date. The digests of all 214 ExternalArtifacts and the revisions of all 104 HelmReleases matched a snapshot taken before the swap, and every PackageSource and HelmRelease stayed Ready. The race itself was not reproduced on the cluster, since that needs a spec change landing during a build. Closes #4593 ### 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: ### 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(operator): rebuild an ArtifactGenerator once after each spec change, so an upgrade no longer leaves a component on artifacts built from the previous spec (for example a chart missing a library it now needs). ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved PackageSource recovery when an artifact generator reports ready: the system now verifies the generated output and triggers a rebuild when needed. * Failed rebuild or verification attempts can be retried during a later reconciliation, and recovery tracking is cleared once the rebuilt generation settles. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
adc2dcb5ca |
fix(operator): rebuild each changed ArtifactGenerator generation once
source-watcher builds an ArtifactGenerator from the spec it read when its reconcile started, but its final status patch re-reads the live object and marks Ready with the live generation. If the spec changes during a build, which an upgrade does as it rolls out a new packages source and new copy rules within seconds of each other, the generator reports the new generation as built while its artifacts come from the old one, and source-watcher never rebuilds them. A chart can then ship without a library its templates now require. The generator status cannot tell a stale build from a good one, so the operator forces one rebuild of every changed generation once it reports Ready, through the existing force-drift path. An unchanged spec rebuilds to the same digest, so nothing downstream moves. The recovery messages no longer name one upstream race, since they now serve two. Upstream: fluxcd/source-watcher#384. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
a09eded9ec |
fix(kubernetes-worker-image): wait for goldens to import before Ready
The Talos worker image catalog renders its goldens the same way the VM image collection does: DataVolumes with immediate bind whose import starts at install. With no health check the release went Ready as soon as helm applied them, so a golden that never imported left the catalog green. Gate it on the DataVolume phase with the same expressions. Worker pools read each golden directly and do not wait on the catalog status, so the gate changes what the status reports and delays no pool. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
51ac18776e |
fix(vm-default-images): wait for golden images to import before Ready
Every image is a DataVolume with immediate bind, so its import starts at install, but nothing waited for it: the HelmRelease went Ready as soon as helm applied the DataVolumes. An image that failed or stalled left the component green, and the failure surfaced later on a vm-disk cloning from it, far from the cause. Gate the release on the DataVolume phase. Only Succeeded counts as ready, and a Failed phase, when CDI sets one, ends the wait at once. Every other phase, and a missing status, keeps the wait open until the install window expires. There is no per-component timeout, so a download longer than that window is retried as an upgrade until it finishes, and the retry strategy never uninstalls the volumes. Signed-off-by: Aleksei Sviridkin <f@lex.la> Assisted-by: LLM |
||
|
Aleksei Sviridkin
|
7a0aab2bdc |
refactor: apply the Go 1.27 modernizers
Run go fix over every first-party module: interface{} becomes any,
hand-written loops become slices, maps, min/max and range-over-int,
errors.As becomes errors.AsType, pointer helpers become new(expr) and
are inlined at their call sites, and so on. Every rewrite keeps
behaviour.
Three rewrites stay out. embedlit flattens embedded-field literals
such as ObjectMeta, syntax only Go 1.27 accepts, which would break
backports to release branches still on Go 1.26. omitzero drops
omitempty from struct fields, and openapi-gen reads omitempty as
optional, so metadata, spec and status would turn required in the
published schemas. And errorsastype rewrote an errors.As whose
target, apierrors.APIStatus, does not implement error, which
errors.AsType rejects at compile time.
Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <f@lex.la>
|
||
|
Aleksei Sviridkin
|
8f0f29e1bd |
test(operator): pin how component values files are layered
The first values file overwrites the component's values.yaml and each later one merges into it; no test held that order or the strategies. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
IvanHunters
|
aa20495df3 |
feat(marketplace): register a tapped repository's apps on connect
Connecting a marketplace repository materialized its PackageSource but created no Package, so its ApplicationDefinitions never rendered and its apps never appeared in the dashboard catalog until a manual `cozypkg add` the dashboard cannot drive (the package list is computed from the ApplicationDefinitions, which do not exist yet). The tap materializer now creates a tap-managed registration Package for each materialized PackageSource, the way the platform ships a Package per built-in app, so the ApplicationDefinition registrations deploy on connect and the apps become browsable; the app components carry no install block and are still instantiated per user resource. Privileged install components of an unconfirmed tap auto-registration (a Package that still carries the marketplace-tap label) are refused on several best-effort layers: the Package reconciler skips their HelmRelease and prunes a lingering one at the install site, and the PackageSource reconciler withholds their ExternalArtifact so the helm-controller has no privileged content to deploy, re-running on confirmation via a Package watch. `cozypkg add --allow-privileged` sheds the label as an explicit ownership handover, which is the confirmation; a platform Package never carries the label. A benign component flipped to privileged in a new revision can still be materialized by source-watcher before these layers observe the flip; closing that race fully requires decoupling the catalog registration from install and is tracked in issue #4359. `cozypkg untap` and the dashboard disconnect remove the registration Package (de-registering the apps) under a UID + ResourceVersion precondition, so a concurrent `cozypkg add` handover is never clobbered; running instances are left but unmanaged. `cozypkg add` re-opens a tap auto-registration for variant selection and hands it over. The cozystack-api ClusterRole gains delete on packages for the disconnect path. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
scooby87
|
afc003c464 |
feat(postgres): database read-replica autoscaling via KEDA (#3954)
## What this PR does Adds automatic horizontal autoscaling of a PostgreSQL database's **read replicas** in response to load, implementing the merged design proposal [Database Horizontal Autoscaler](https://github.com/cozystack/community/tree/main/design-proposals/database-horizontal-autoscaling) (community#44). The mechanism is entirely stock — no bespoke operator, no new CRD: - **KEDA as an optional platform component** (`packages/system/keda`, wired into `core/platform` as a source + system-bundle entry). It claims the `external.metrics.k8s.io` APIService that nothing served before. - **`cozy-lib.keda.scaledObject` helper** (`packages/library/cozy-lib/templates/_keda.tpl`) renders a KEDA `ScaledObject`, fail-closed on missing/invalid arguments. - **`packages/apps/postgres` autoscaling** — an `autoscaling` values block (`enabled`, `minReplicas`, `maxReplicas`, `metric`, `target`, `maxReplicationLagSeconds`, `dryRun`, `transition`) validated by `values.schema.json`; under active autoscaling the chart renders a `ScaledObject` and seeds `spec.instances` to a constant floor (`max(replicas, effectiveMin)`), while the postgres release is forced to client-side (two-way) Helm apply so the seed is a merge no-op on upgrade and the KEDA-managed HPA — the runtime writer via the CNPG `Cluster` scale subresource — is not reverted. - **Single-value metric (§1)** — one PromQL query returns `Σ(read load over standbys) + target`, so a stock HPA computes `desired = ceil((Σ+target)/target) = 1 + ceil(Σ/target) = primary + desiredRead`, with no per-pod emission. - **The four static brakes (§5)** — quorum floor `max(minReplicas, maxSyncReplicas+1, 2)` (quorum wins over `maxReplicas`), scale-down pacing (`Pods/1/600s`), a replication-lag clamp folded into the query (freeze both directions while lag exceeds the threshold and the primary is writing, with `max_over_time` hysteresis), and dry-run (render paused). - **Two-phase enable/disable migration** via a `transition` sub-flag: phase 1 stages `spec.instances` to the live count and brings KEDA up with its HPA paused in both directions; flipping to phase 2 hands the runtime count to KEDA, and because the seed re-renders the same count the flip is a merge no-op with no dip. - **Observability** — a database-autoscaling Grafana dashboard and PrometheusRule alerts. **Live validation (dev cluster, CNPG 1.30.0).** The full path works end-to-end: the CNPG `Cluster` scale subresource serves `status.selector`, KEDA renders the HPA, the metric reaches it (`ScalingActive=ValidMetricFound`, `TARGETS 151/150`, `desiredReplicas=2 = 1 primary + 1 replica` — the §1 arithmetic), and the lag-clamp query returns the base value on live VictoriaMetrics. Two packaging fixes were needed and are included: KEDA must run on the cozystack `cozy.local` cluster domain (the upstream `cluster.local` default makes the metrics gRPC target NXDOMAIN and every HPA hangs on `FailedGetExternalMetric`), and the operator's client-go throttle is raised so a single-replica operator does not lose its leader lease under scale-subresource polling. Implements #3953. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff. Two repositories are affected; both are follow-ups to open once this lands (and after its dependency #3951 merges and the values surface is final), left unticked here for a maintainer to confirm rather than claimed with a speculative link: - **cozystack/terraform-provider-cozystack** — this adds the nested `autoscaling` block to `packages/apps/postgres/values.schema.json`. The provider hand-models each app's fields (schema + model + expand/flatten), so it needs the new block to expose it in Terraform. - **cozystack/website** — KEDA is a new platform component (`guides/platform-stack`), and the postgres reference page gains the `autoscaling` params. The managed-app reference is regenerated from `README.md` by the release bot; the platform-stack component list is hand-maintained. - [ ] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: to open (new platform component + postgres autoscaling params) - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: to open (new `autoscaling` block in postgres values.schema.json) ### Depends on - #3951 — bump CloudNativePG to 1.30.0. The HPA can only drive the CNPG `Cluster` scale subresource on CNPG ≥ 1.28.4, where it serves `status.selector`; without it the HPA fails with `InvalidSelector`. This PR should not merge before #3951. ### Release note ```release-note feat(postgres): add optional horizontal autoscaling of read replicas via KEDA, configured through the new `autoscaling` values block; KEDA ships as a new optional platform component. Requires CloudNativePG >= 1.28.4 for the Cluster scale subresource selector. Note: every Postgres release now applies client-side (`release.cozystack.io/helm-server-side-apply: "false"`) so the autoscaler's live `/scale` count is not reverted by Helm; a kind-wide side effect is that chart-rendered fields on Postgres releases are no longer re-asserted by server-side-apply drift correction (driftDetection is unset), a documented trade-off with a per-instance apply strategy as follow-up. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added configurable PostgreSQL read-replica autoscaling with KEDA. * Added CPU and read-connection metrics, replica bounds, replication-lag safeguards, dry-run mode, and transition handling. * Added optional KEDA platform support, Grafana autoscaling dashboard, and Prometheus alerts. * **Documentation** * Clarified integer thresholds, prerequisites, configuration, and transition guidance. * **Bug Fixes** * Improved validation and actionable errors for unavailable KEDA or invalid settings. * **Tests** * Expanded coverage for autoscaling, observability, package selection, and validation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
IvanHunters
|
8738063287 |
fix(operator): reject tar entries containing ".." before any path operation
safeJoin already rejected traversal and re-checked the joined path against destDir, but the guard sits behind a call boundary that CodeQL's go/zipslip barrier does not follow, so the extraction loop reported three high-severity arbitrary-write alerts. Reject the entry name up front instead, which is both the fail-closed spelling and the shape the analysis recognises. Extend the traversal test to cover a ".." buried mid-path, not just a leading one. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |