## 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.
```
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>
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>
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>
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>
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>
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>
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>
The scope of the default LimitRange is every namespace a Package targets
whose name does not start with `tenant-`, and cozystack-scheduler installs
into kube-system, so a namespace owned by the underlying Kubernetes
distribution is defaulted like any other. Neither the flag help nor the
installer value said so, and both are what somebody setting the knob
reads.
docs/operations/system-memory-limits.md already covers this, including the
part that surprises: containers the kubelet builds from an on-disk
manifest never pass through admission, so a Talos control-plane static pod
gets no memory.max from this feature in the first place and the mirror pod
is the only thing the LimitRange reaches. Both notes now carry the same
statement, so neither reading of the knob leaves it out.
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>
The flag help and the installer values comment both still said the
operator skips a namespace holding a request above the configured limit
and retracts any LimitRange it applied there. It stopped doing that in
3127c0fad: it raises that namespace's ceiling to clear the request and
drops back once the request is gone. An operator reading the shipped help
and then finding an 8Gi LimitRange in cozy-monitoring had no way to
connect the two.
Both texts now describe the raise, the drop-back and the log line to grep
for. The values comment also loses its VPA warning, which this branch's
own comment in findRequestAboveDefaultLimit refutes — VPA rescales the
limit in proportion to the request under the default controlledValues, so
it cannot be what puts a namespace over — and gains a pointer to
declaring a limit in the component's own chart, which is how
packages/system/linstor now stays out of the default's reach.
Assisted-By: LLM
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
--system-namespace-memory-request=0 passes the startup check, since 0 is
not greater than the limit, and produces a LimitRange with
defaultRequest: 0. Whether that is a footgun to reject or a supported
setting to explain was open, so I asked a real API server: envtest with
kube-apiserver 1.36.0 and LimitRanger enabled, with a second namespace
using 32Mi as the positive control that the plugin was actually running.
It is supported. The API server accepts the LimitRange and stores
defaultRequest as 0 rather than folding it back to the default -- the
Default-to-DefaultRequest copy only fills keys that are absent, and this
one is present. A container declaring no memory of its own is then
admitted with an explicit request of 0 and a limit of 4Gi, QoS Burstable.
So LimitRanger does not read zero as absent, and the container still gets
memory.max on its cgroup, which is the whole reason this feature exists.
What it costs is the scheduling signal: nothing is reserved for that
container, which is exactly where system components stood before this
feature. Say so in the flag help text, where an operator setting the knob
will see it, and note at the startup check that 0 passing is deliberate so
nobody hardens it into a rejection later.
Assisted-By: LLM
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
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>
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>
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>
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>
Tapped repositories keep their own declared PackageSource names instead of
being renamed under the community. prefix. Tap-ness is now marked by the
marketplace-tap label (and source annotation) on the OCIRepository and every
materialized PackageSource, so untap, the Flux-source cleanup, and idempotent
re-tap key on the marker rather than a name prefix.
A declared name that clashes with an existing core component (or another tap)
is rejected: the CLI tap fails fast before applying anything, and the operator
materializer refuses to overwrite the foreign object, records the reason on the
source (annotation and Event), and does not stamp the revision so a corrected
artifact retries. The shared check lives in internal/marketplace/collision.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
Add server-side connect to complete the marketplace write path. Creating a Tap
records the intent only: the API validates the oci:// reference and creates a
labeled Flux OCIRepository (with an optional pull-credential secretRef), never
blocking the request on a registry pull, and returns a Tap whose status shows
materialization pending. The connect inputs (url, tag, secretRef) are write-only
and are never echoed back or returned on read.
A new tap materializer reconciler in the operator watches community-tap
OCIRepositories, and once source-controller has pulled the artifact, fetches the
tarball over the Flux artifact URL (digest-verified, size-bounded, path-traversal
guarded), parses the PackageSource(s) it carries, and materializes them under the
community. prefix with their sourceRef repointed at the tap's source. A finalizer
cleans up the materialized PackageSources when the source is removed, leaving
installed Packages in place.
The shared label/annotation keys live in internal/marketplace/tapconst so the API
and the operator cannot drift on the connect-to-materialize handoff.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
- Run `go mod tidy` to move github.com/fluxcd/pkg/apis/meta from
indirect to direct require: it became a direct import after the
SecretRef type fix.
- Refine the --platform-source-secret flag docstring and the matching
values.yaml comment: drop identity.pub from the SSH required-keys
list (Flux source-controller only consumes identity + known_hosts)
and mention bearerToken as an alternative to username+password for
Git over HTTPS.
- Note in values.yaml that the Secret must exist in cozy-system before
the operator reconciles; otherwise source-controller reports a
failed condition until the Secret is created.
Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
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>
time.ParseDuration accepts "0s" and negative values, but Flux
HelmRelease.Interval / Install.Timeout / Strategy.RetryInterval require
strictly positive durations to function. Validate at startup so a
misconfigured flag fails fast instead of propagating into every
generated HelmRelease.
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
The HelmRelease generation in package_reconciler hardcoded three values
that have caused recurring pain:
- Install.Timeout = 10m (line 214)
- Upgrade.Timeout = 10m (line 220)
- MaxHistory = unset (Helm default 5)
The 10m timeout in particular contradicts the per-Application timeout
work done in `pkg/config/config.go` and
`pkg/registry/apps/application/rest.go` (commit 7b146cbe), which lets
individual Applications override the install/upgrade timeout via
annotation. The PackageSource side had no equivalent — etcd works around
it by hand-rolling its HR YAML in `packages/apps/tenant/templates/etcd.yaml`
to set a 30m timeout, harbor has had multiple commits chasing the same
problem from the other side. This closes that gap.
New operator flags (each maps to a chart value with empty default;
operator uses its own default when value is empty):
- `--helmrelease-install-timeout` (default 10m) → Spec.Install.Timeout
- `--helmrelease-upgrade-timeout` (default 10m) → Spec.Upgrade.Timeout
- `--helmrelease-max-history` (default 5) → Spec.MaxHistory
Production behaviour unchanged. E2E and edge cases (cert rotation,
slow-installing charts) can override per-cluster without modifying
chart manifests.
Per-component overrides on `ComponentInstall` (mirroring the existing
`UpgradeCRDs` pattern) would be a strictly better fix for charts like
etcd that need an outlier timeout — left as a deliberate followup since
it touches the Package CR API.
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
The operator generated HelmReleases with `Install.Remediation{Retries: -1}`
and `Upgrade.Remediation{Retries: -1}`, which is the v2 way of saying
"retry forever, never remediate". Combined with the spec-level
`Interval: 5m`, this meant a failed install/upgrade waited a full 5
minutes before the next attempt — even when the underlying race that
caused the failure (e.g. a chart artifact not quite Ready, a HR
dependency reconciling out of order) cleared in seconds.
Audit of the last 30 successful PR runs found that every single run had
its `Install Cozystack` step's `kubectl wait hr/seaweedfs-system` time
out at the 2-minute mark and recover only after an inline
`flux reconcile --force` workaround. Same root cause: 5-minute retry
interval was longer than the test's wait window. That workaround can be
removed once this change lands.
Switch to `Strategy.Name=RetryOnFailure` with `RetryInterval` exposed via
a new `--helmrelease-retry-interval` operator flag (default 30s):
- Functionally equivalent to the previous configuration ("retry forever
on failure") since `Retries: -1` meant remediation never fired anyway.
- Decouples retry-on-failure timing from `spec.Interval` so failed
releases recover at 30s without polling healthy releases at the same
cadence (which would multiply controller load by ~10x for no benefit).
- Helm chart exposes `cozystackOperator.helmReleaseRetryInterval`,
defaulting to empty (operator uses its own 30s default).
- Same flag pattern as the existing `--helmrelease-interval` (5m default),
also added in this PR via cherry-pick.
Verified: go build green, go test ./internal/operator/... pass, helm
template renders both flags conditionally on values.
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Adds --helmrelease-interval (default 5m, matching today's behaviour) to the
operator and a corresponding cozystackOperator.helmReleaseInterval value
on the cozy-installer chart (default empty -> no flag rendered ->
operator default 5m applies).
E2E install sets it to 30s. Rationale: with the 5m default, dependency-
blocked HRs (waiting on cert-manager webhooks, CRDs) are requeued only
every 5 min, producing an 8-10 min dead zone in the E2E install where
the fast pack of HRs is Ready but the slow tail hasn't been retried yet.
30s collapses the gap.
Production defaults are unchanged. The override is opt-in per install.
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
When platformSourceURL is empty no Flux source resource is created, so
creating a PackageSource that references it would leave a dangling
SourceRef in a permanent error state. Guard the creation block on
platformSourceURL != "".
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Replace repetitive Variant struct literals with a loop over variant
data, making it easier to add new variants and reducing copy-paste
errors.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Change default --platform-source-name from "cozystack-packages" to
"cozystack-platform" to match the value passed by the Helm template.
Gate PackageSource creation on --install-crds flag: when the operator
does not manage CRDs, the PackageSource CRD may not exist yet, so
skip creation and let an external process handle it.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Explicitly check error from parsePlatformSourceURL instead of relying on
the implicit guarantee that installPlatformSourceResource already checked
it. This prevents latent bugs if startup order is ever restructured.
Add wait for platform PackageSource existence in E2E test before creating
Package resource, preventing flaky failures when operator startup is slow.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
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>
Check error from parsePlatformSourceURL instead of discarding it, preventing
silent fallback to OCIRepository on invalid URLs.
Move variant validation from deleted packagesource.yaml to
cozystack-operator.yaml template so invalid cozystackOperator.variant
values still fail at helm template/install time.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Replace the Helm hook approach with programmatic PackageSource creation
in the operator startup sequence. Helm hooks are unsuitable for persistent
resources like PackageSource because before-hook-creation policy causes
cascade deletion of owned ArtifactGenerators during upgrades.
The operator now creates the platform PackageSource after installing CRDs
and the Flux source resource, using the same create-or-update pattern as
installPlatformSourceResource(). The sourceRef.kind is derived from the
platform source URL (OCIRepository for oci://, GitRepository for git).
Also fix stale comment in e2e test referencing deleted crds/ directory.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
- Remove duplicate "Starting controller manager" log before install
phases, keep only the one before mgr.Start()
- Rename misleading test "document without kind returns error" to
"decoder rejects document without kind" to match actual behavior
- Document Helm uninstall CRD behavior in deployment template comment
- Use --health-probe-bind-address=0 consistently with metrics-bind
- Exclude all dotfiles in verify-crds diff, not just .gitattributes
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
- Check both apiVersion and kind when validating embedded CRD manifests
to prevent applying objects with wrong API group
- Move ctrl.SetupSignalHandler() before install phases so CRD and Flux
installs respect SIGTERM instead of blocking for up to 2 minutes
- Replace custom contains/searchString helpers with strings.Contains
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Embed Package and PackageSource CRDs in the operator binary using Go
embed, following the same pattern as --install-flux. The operator applies
CRDs at startup using server-side apply, ensuring they are updated on
every operator restart/upgrade.
This addresses the CRD lifecycle concern: Helm crds/ directory handles
initial install, while the operator manages updates on subsequent
deployments.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
The cozystack-operator's manager cache is filtered to only include
specific secrets and namespaces with certain labels. This caused
telemetry collection to fail because resources like kube-system
namespace, nodes, services, and PVs were not in the cache.
Switch to using mgr.GetAPIReader() which bypasses the cache and
queries the API server directly. This is appropriate for telemetry
since it only runs every 15 minutes.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Aleksei Sviridkin