## 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.
```
Keep the parser inputs tied to the constants registered as flag defaults,
but compare the results with literal 32Gi and 32Mi expectations. A shipped
default can no longer move while the defaults case stays green.
Assisted-By: LLM
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
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>
parseQuantity and the request-above-limit check were closures inside
main(), so no branch of either was reachable from a test: not empty, not
malformed, not negative, not a request of 0 against a real limit, and not
the pair check itself. That last one is the one worth having. A limit and
a request that each parse but contradict each other produce a LimitRange
the API server rejects, once per system namespace on every Package
reconcile, and nothing in that error points back at a flag.
Lift both into parseSystemNamespaceMemory, which returns an error instead
of calling os.Exit, and pin all of it: the shipped defaults, empty as the
off switch, an explicit 0 limit beside a set request, a 0 request against
a real limit (supported on purpose), malformed and negative values on
either flag, request above limit, and request equal to limit as the legal
boundary. main() keeps the same exit-on-error behaviour, now in one place.
Assisted-By: LLM
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
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>
Add isp-slim, isp-slim-generic and isp-hosted-slim to the platform
PackageSource so a Package can select the minimal presets shipped by
the platform chart.
Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <f@lex.la>
installPlatformPackageSource applied its PackageSources with Force: true and no
ownership check, so a platform upgrade could silently overwrite a PackageSource
a marketplace tap had materialized under a name the platform later ships. The
collision check cannot cover this: it runs at tap time, the overwrite months
later at upgrade.
Drop Force so server-side apply surfaces a field-ownership conflict from any
other manager as an error, and add an ownership pre-check that fails with a
specific message when the existing object carries the marketplace-tap marker,
leaving it untouched. Re-applying the platform's own object never conflicts.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
gofmt -w on the two touched test files: the case-table struct fields in
rest_helmrelease_spec_test.go and main_test.go were misaligned, and
main_test.go carried a trailing blank line. CI runs no gofmt gate
(pre-commit only runs make generate), so this would otherwise land
unformatted.
Address review feedback from lexfrei.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Lift parsePositiveDuration from cmd/cozystack-operator/main.go into
pkg/config so cozystack-api can reuse the same startup validator
without duplication. Operator main.go now imports and delegates;
the table-driven test moves to pkg/config/config_test.go.
Extend ReleaseConfig with the five HelmRelease generation knobs
the cozystack-operator already exposes via flags (PR #2509):
HelmReleaseInterval, HelmReleaseRetryInterval,
HelmReleaseInstallTimeout, HelmReleaseUpgradeTimeout,
HelmReleaseMaxHistory. The existing per-Application
HelmInstallTimeout (set from the
release.cozystack.io/helm-install-timeout annotation) keeps its
override semantics - it still wins over the new global
Install/Upgrade timeout defaults when set.
No behaviour change in this commit; the new fields are wired up in
follow-up commits.
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
The previous closure inside main() couldn't be tested because every error
path called os.Exit(1). Lifting it to a package-level function returning
(time.Duration, error) lets a table test exercise happy paths and rejections
("0s", "-5m", "5x", empty input) directly. main() retains the os.Exit
behaviour via a thin wrapper.
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Switch installPlatformPackageSource to server-side apply (SSA) with
field manager, matching the pattern used in crdinstall. SSA is idempotent
and preserves metadata fields managed by other controllers.
Create PackageSource unconditionally (not only when platformSourceURL is
set), matching the previous Helm template behavior where PackageSource
was always created regardless of source URL configuration.
Use a dedicated context with its own 2-minute timeout for PackageSource
creation, separate from the platform source resource installation.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Add explicit CRD wait (kubectl wait crd --for=condition=Established) in
E2E test before creating Package resources, preventing race condition
between operator CRD installation and resource creation.
Add unit tests for installPlatformPackageSource covering create, update,
and GitRepository sourceRef kind scenarios.
Document that hardcoded variant list is an intentional design choice
matching the previous Helm template behavior.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Aleksei Sviridkin