mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-09 22:13:18 +09:00
7b08fcdca911b2df8a11452d72de37a5bc3da33c
4631
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Ivan Okhotnikov
|
7b08fcdca9 |
fix(operator): unify recovery gating through state machine, close source-watcher-unresponsive dormancy loop
Self-review on 467b788b8 caught a MAJOR bug introduced by the separate awaitSourceWatcherResponse helper: if source-watcher never responds to a forced drift (network partition, missing predicate, dead pod), our own reasonRecoveryForced marker keeps reflecting back via Owns() forever, and the wait-forever helper schedules a 1-second requeue tight loop without ever advancing the retry state machine. SourceWatcherStalled give-up was unreachable. Fix: remove awaitSourceWatcherResponse and unify all three recovery entry conditions — our own marker, source-watcher's Progressing during rebuild, and the classic first-time stuck detection — through a single gate that routes to maybeRecoverArtifactGenerator. The state machine's decideRecovery is the single arbiter of wait/force/give-up, and its Wait branch is bounded by the same backoff schedule as Force. When backoff elapses on a still-reflecting own-marker the machine escalates to another force (bumps attempts, re-stamps requestedAt); when attempts exhaust it surfaces SourceWatcherStalled honestly. The own-marker filter also no longer requires attempts>0, so an operator crash between step 1 (status patch) and step 2 (metadata patch) of forceArtifactGeneratorDrift is still routed through the state machine on the next reconcile rather than leaking the synthetic marker onto the PackageSource. Also address the non-blocking notes from the same self-review: * Docstring on maybeRecoverArtifactGenerator was still describing the removed reset-counter branch and the "requestedAt alone is a no-op" framing. Rewrite to explain the current shape: status write triggers the drift signal, requestedAt bump triggers the reconcile event — both are required, neither alone works. * TestForceArtifactGeneratorDrift_MetadataPatchFailure_RollbackAnnotations now asserts reconcile.fluxcd.io/requestedAt is also rolled back on the metadata-patch failure branch (symmetric with the tracking annotations). * TestUpdateStatus_OwnMarkerRoutesToAwait asserts an exact 25s RequeueAfter (backoffFor(1)=30s minus 5s elapsed) so a silent regression in the backoff arithmetic is caught. New regression tests for the source-watcher-unresponsive path: * TestUpdateStatus_OwnMarkerStale_TriggersRetry: with attempts=1 and lastRecoveryAt older than backoffFor(1), the reconcile must escalate through the Force branch and bump attempts to 2 rather than idle in a tight requeue loop. * TestUpdateStatus_OwnMarkerAttemptsExhausted_GivesUp: with attempts=maxRecoveryAttempts and the marker still reflecting back, the reconcile must set PackageSource Ready=False / SourceWatcherStalled instead of extending the loop indefinitely. Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io> |
||
|
Ivan Okhotnikov
|
934ca46a36 |
fix(operator): bump requestedAt with force-drift, filter own writes, remove reset-counter loop
Address lexfrei's three blockers on the force-drift approach — as
committed, the recovery driver did not function on a live cluster.
**B1**: `forceArtifactGeneratorDrift` wrote `Ready=False` on the AG's
status subresource but did not touch its metadata annotations.
source-watcher v2.1.0 registers its ArtifactGenerator watch with
`predicate.Or(GenerationChangedPredicate, ReconcileRequestedPredicate)`
— a status-only patch changes neither the generation nor
`reconcile.fluxcd.io/requestedAt`, so it does NOT enqueue a
source-watcher reconcile. The AG's periodic self-requeue is hardcoded
to `time.Hour`, well outside the 15m HR install timeout. Fix: also
stamp `reconcile.fluxcd.io/requestedAt` in the metadata patch so the
ReconcileRequestedPredicate fires and source-watcher's next reconcile
picks up the `IsFalse(Ready)` drift signal.
**B2**: `Owns(&ArtifactGenerator{})` re-fires this reconciler on the
force-drift's own writes. Both the status patch (Ready=False, reason
`SourceWatcherRecoveryForced`) and the metadata patch enqueue a
PackageSource reconcile. Blindly running `clearRecoveryTracking` on
those re-entrant fires nuked the attempts counter — the give-up
budget was unreachable, and the synthetic marker leaked to
`PackageSource.status.conditions[Ready]`. Fix: in `updateStatus`,
detect our own reason and route into a new `awaitSourceWatcherResponse`
helper that holds tracking and reports Unknown /
AwaitingSourceWatcherRecovery on the PS. The same helper covers the
Progressing/Unknown write source-watcher issues during rebuild once
tracking is present (attempts > 0) — otherwise the transient
Progressing would trigger the same premature tracking clear.
**B3**: The attempt-counter reset keyed on
`Ready.LastTransitionTime > lastRecoveryAt`. That inequality is true
on every source-watcher response during recovery (the Progressing
write is always newer than our force), so under the very race this
driver targets the counter would reset every cycle and
`recoveryActionGiveUp` was unreachable. Fix: remove the reset. The
canonical counter reset is the natural `clearRecoveryTracking` on the
not-stuck path (Ready=True from source-watcher, or a real upstream
Ready=False), and the HA/leader-election requirement already
contains the "missed recovery event on operator restart" edge case
the reset was trying to paper over.
Extract `reasonRecoveryForced` to a named constant so both the
writer (forceArtifactGeneratorDrift) and the reader (updateStatus'
own-marker filter) use the same symbol.
New tests pin all three fixes:
- ForceArtifactGeneratorDrift_Success now asserts the
reconcile.fluxcd.io/requestedAt annotation is set (B1).
- UpdateStatus_OwnMarkerRoutesToAwait asserts we do NOT clear
tracking and do NOT copy the synthetic Ready=False through when we
see our own marker (B2).
- UpdateStatus_ProgressingDuringRecoveryRoutesToAwait covers the
second Owns() re-entrancy path — source-watcher's Progressing
write during rebuild (B2).
- UpdateStatus_ReadyTrueClearsTracking pins that the natural not-
stuck path IS the canonical counter reset after B3.
- The old ResetsCounterOnFreshTransition test is replaced with
DoesNotResetCounterOnFreshTransition, asserting the opposite
behaviour that closes B3.
Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
|
||
|
Ivan Okhotnikov
|
f64081b232 |
fix(operator): schedule follow-up reconcile when Ready is Unknown within grace
E2E on dev3 caught a dormancy bug that renders the whole recovery driver inactive in practice: when updateStatus copies Ready=Unknown across but the AG's LastTransitionTime is still within stuckGracePeriod, artifactGeneratorStuck returns false and we fall through to the copy path with no RequeueAfter set. If the reconcile happens to fire in the [stuck_start, stuck_start+30s) window — a routine case since operator restarts touch every PackageSource — the controller then sleeps waiting for an AG-status change that will never come (that IS the fluxcd/pkg#934 lost-write race), up to the 10h informer resync. Observed on dev3: `cozystack.networking` stuck at Ready=Unknown for 9+ minutes with zero recovery attempts. Two reconciles fired at stuck_start+23s and +24s — both correctly detected "not yet stuck" — and then nothing until observation ended. Upstream OCIRepository revision was unchanged throughout, ruling out a coincidental source update as the cause. Fix: whenever we copy Ready=Unknown to the PackageSource (or set a synthetic Unknown because no Ready condition exists yet), compute the remaining grace period against the AG's LastTransitionTime (or CreationTimestamp for the missing-Ready branch) and schedule a RequeueAfter at that expiry. A 1s floor prevents a zero-delay tight loop when the anchor is right at or past grace expiry. Extracted the delay computation to `agFollowUpDelay` for direct unit testing. Adds three tests: the follow-up scheduling on Ready=Unknown in-grace, the same on the missing-Ready branch, and a table test on agFollowUpDelay covering the floor edge cases. Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io> |
||
|
Ivan Okhotnikov
|
4df18df5de |
test(operator): pin maybeRecoverArtifactGenerator branches, tighten wait assertions, polish comments
Address the fourth-pass self-review's non-blocking findings:
* Add TestMaybeRecoverArtifactGenerator_{ForceBranch, WaitBranch,
GiveUpBranch, ResetsCounterOnFreshTransition}. The three switch
outcomes of the driver (force / wait / give-up) and the reset-counter
guard on a fresh Ready.LastTransitionTime transition were previously
only covered indirectly through their pure-function components. Now
each outcome is asserted end-to-end against a fake client with a
status subresource, verifying the Ready condition, tracking
annotations, RequeueAfter, and — for the wait branch — that no AG
writes are issued at all.
* Tighten TestDecideRecovery's wait-branch assertions from a loose lower
bound to exact equality. decideRecovery is deterministic
(needed - elapsed) so an exact assertion catches subtle backoff
regressions a lower bound would miss (coderabbit's inline nit).
* Rename the local var `wantWaitMin` to `wantWait` for the tighter
assertion shape.
* Fix two stale identifier references in comments
(maybeRequeueArtifactGenerator, AwaitingSourceWatcherRequeue) left
over from the rename pass.
* Add a docstring paragraph on forceArtifactGeneratorDrift explaining
why the JSON-merge-patch full-array-replace on
`.status.conditions[]` is net-neutral in this workaround's context.
Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
|
||
|
Ivan Okhotnikov
|
7c3af85e58 |
fix(operator): force source-watcher drift via status patch, not requeue bump
Rework the fluxcd/pkg#934 recovery driver after myasnikovdaniil's B1 finding: bumping reconcile.fluxcd.io/requestedAt cannot recover a stuck ArtifactGenerator because source-watcher v2.1.0's reconcile is drift-gated. detectDrift (artifactgenerator_drift.go:52) returns NoDriftDetected for exactly the stuck signature we target — Inventory populated, ObservedSourcesDigest set, Ready=Unknown on the current generation — so the ReconcileRequestedPredicate fires a reconcile that lands on the no-drift early-return at controller.go:164 and MarkTrue at :254 is never reached. Every attempt in the previous shape was a no-op. New mechanism: force drift by patching Ready=False on the AG's status subresource. detectDrift picks this up as NotReady drift, source-watcher takes the drifted branch, and either MarkTrue (on healthy rebuild) or MarkFalse with a real reason (on genuine failure) replaces our synthetic False. Owns() watch fires this reconciler to copy the resolved condition through. Same bounded-retry harness (30s / 60s / 2m / 4m / 4m budget inside the 15m HR install timeout, SourceWatcherStalled surfaced on exhaustion), same annotation-based state on the AG, same rollback contract on Patch failure — only the action itself changed. Additional fixes bundled in: * Reset attempt counter when the AG's Ready.LastTransitionTime is newer than our lastRecoveryAt, so a fresh stall episode after a previous give-up gets a fresh budget rather than an immediate re-give-up (myasnikovdaniil's follow-up #3). * Propagate updateStatus errors from Reconcile instead of swallowing them, so controller-runtime's exponential backoff retries transient failures on force-drift / status-update writes rather than dropping them silently (coderabbit's inline finding). * forceArtifactGeneratorDrift documents the two-subresource shape (status patch first, metadata patch second) and the partial-failure semantics. * New tests cover the status-patch success / status-patch failure / metadata-patch failure paths, all with fake client + interceptor. The successful case asserts Ready=False actually lands in the apiserver via WithStatusSubresource. * All annotation / action / decision / helper names renamed from *Requeue* to *Recovery* to match the new semantics. Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io> |
||
|
Ivan Okhotnikov
|
4c3099d6f2 |
test(operator): pin overwrite-rollback contract, minor polish
Address the fourth-pass self-review's non-blocking NITs: * Add TestBumpArtifactGeneratorRequeue_PatchFailure_RollbackRestoresPriorBump to cover the subtle case the two existing rollback tests miss: when the tracking annotations were already set from a prior bump, a failed Patch must restore the PRIOR values, not delete the keys. A refactor that naively swapped rollback for `delete(ag.Annotations, key)` would pass the nil-rollback tests but silently regress this contract. * Rename the cloneAnnotations subtest from "deep copy" to "independent copy" — the code documents shallow copy semantics and the misnomer was the only thing pointing readers at the wrong mental model. * Hoist `now.UTC().Format(time.RFC3339Nano)` to a local so the two timestamp annotations use one formatted value instead of two identical Format calls. Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io> |
||
|
Ivan Okhotnikov
|
28834ca874 |
test(operator): cover rollback contract, deletion guard, and clone helper for the requeue driver
Address the third-pass self-review's blocker: the mutation contract on bumpArtifactGeneratorRequeue / clearRequeueTracking, the DeletionTimestamp early-return in Reconcile, and the nil-preserving cloneAnnotations helper had no regression tests, so a future refactor could silently break the contracts documented in comments. New fake-client + interceptor-based tests: * TestCloneAnnotations — pins the "nil in / nil out" sentinel behaviour and mutation-independence of the shallow copy. * TestBumpArtifactGeneratorRequeue_Success — asserts the persisted state matches the caller's in-memory `ag.Annotations` on success, including that reconcile.fluxcd.io/requestedAt is actually written (previously never asserted anywhere). * TestBumpArtifactGeneratorRequeue_PatchFailure_Rollback and its PreservesUnrelated variant — drive the Patch-failure branch via an interceptor client and assert both that our tracking annotations are rolled back and that pre-existing unrelated annotations survive. * TestClearRequeueTracking_Noop — asserts we don't issue a no-op Patch on every healthy reconcile when there are no tracking annotations to clear. * TestClearRequeueTracking_PatchFailure_Rollback — same rollback contract as bump, on the clear path. * TestReconcile_DeletionTimestamp_EarlyReturn — asserts a PackageSource with a deletionTimestamp does not receive any writes and does not schedule a RequeueAfter. Doc tightening in the same commit: * Reword clearRequeueTracking's comment on the flux annotation semantics — standard flux controllers do not clear reconcile.fluxcd.io/requestedAt, they only advance .status.lastHandledReconcileAt via the ReconcileRequestedPredicate. * Note controller-runtime v0.15+'s default RecoverPanic behaviour where the retry-driver's unhandled-action panic is raised, so a future SetupWithManager flip that disables it is easier to spot. * Explain why cloneAnnotations' shallow copy is sound (annotation values are immutable strings). Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io> |
||
|
Ivan Okhotnikov
|
c3f9125163 |
fix(operator): skip deleted PackageSource, restore ag mutation contract, document HA + timing rationale
Address second-pass self-review findings on the bounded-requeue driver: * Skip reconcile early when the PackageSource has a deletionTimestamp so the retry driver stops writing "AwaitingSourceWatcherRequeue" onto an object about to be garbage collected. ownerReference cascade handles the ArtifactGenerator cleanup. * Fix the mutation contract on bumpArtifactGeneratorRequeue and clearRequeueTracking: the previous deep-copy shape left the caller's `ag` desynchronised from the apiserver on success. Now mutations land on `ag` itself with a rollback to the pre-call annotation snapshot on Patch failure — so success reflects persisted state and failure does not leak unpersisted state to subsequent code paths. * Convert the "unreachable" default in maybeRequeueArtifactGenerator to an explicit panic so a future requeueAction added without a matching case fails loudly instead of freezing the Ready condition silently. * Document the timing constants' rationale (grace period matches the observed <10s source-watcher settle time on dev3; total ~11.5m budget keeps SourceWatcherStalled inside the 15m HR install timeout) and note the operator must run with leader election when replicas > 1 so the annotation-based retry state does not race. Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io> |
||
|
Ivan Okhotnikov
|
c5845db221 |
refactor(operator): thread single now through reconcile, harden patches, tighten status
Address self-review nits on the bounded-requeue driver: * Sample `now := time.Now()` once at Reconcile entry and thread it into `updateStatus`, `artifactGeneratorStuck` and `maybeRequeueArtifactGenerator` so every time-sensitive check inside a reconcile agrees on the same wall-clock reading. * Set `ObservedGeneration` in the three early Ready-condition branches (`SourceRefNotSet`, `ArtifactGeneratorNotFound`, `ArtifactGeneratorNotReady`) for parity with the retry-driver branches. * Patch the ArtifactGenerator via a deep copy in `bumpArtifactGeneratorRequeue` and `clearRequeueTracking` so a failed Patch no longer leaves the caller's in-memory `ag` desynchronised from the apiserver. * Document the retry driver's dependency on source-watcher's ReconcileRequestedPredicate on ArtifactGenerator, so a future upstream change dropping the predicate is easier to trace. Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io> |
||
|
Ivan Okhotnikov
|
ff314797e9 |
refactor(operator): drive source-watcher via bounded requeue instead of synthesising PackageSource Ready
Replaces the Ready=True synthesis for PackageSources whose ArtifactGenerator hit the fluxcd/pkg#934 patch.Helper split-write race with a bounded retry driver: on detected stall, bump reconcile.fluxcd.io/requestedAt with exponential backoff (30s to 4m cap) up to 5 attempts, then surface as PackageSource Ready=False with reason SourceWatcherStalled. The previous approach could mask a genuine regeneration failure if the same patch.Helper race that the workaround exists for also swallowed a real Ready=False write, and it permanently reinterpreted PackageSource.Ready semantics from "current revision processed" to "valid consumable artifacts exist". Requeue keeps the CRD contract intact and, on stall exhaustion, produces an honest failure signal an operator can act on instead of a false-positive True. Retry state (attempts + last-bump timestamp) lives on the AG as cozystack.io/source-watcher-* annotations so it survives operator restarts. Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io> |
||
|
Ivan Okhotnikov
|
88d62612fe |
docs(operator): correct meta.SetStatusCondition rewrite semantics in comment
review-branch skill flagged that the comment claimed meta.SetStatusCondition rewrites LastTransitionTime on Status OR Reason change. Actually only Status change triggers a LastTransitionTime rewrite — Reason and Message are updated in place, LastTransitionTime is preserved. Outcome (repeated identical calls are no-op-y on persisted state) is unchanged; just tightening the technical claim to match apimachinery. Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io> |
||
|
Ivan Okhotnikov
|
122584045b |
docs(operator): link workaround to fluxcd/pkg#934 (existing upstream issue)
Reviewer nit was to file an upstream issue so the workaround does not silently outlive its cause. Checked first — fluxcd/pkg already has one open: #934 \"patch.Helper creates race conditions in downstream controllers\" (May 2025). Same root cause we traced: patch.Helper writes .status and .status.conditions as separate apiserver requests, so an etcd blip between the two leaves the object half-persisted. The upstream issue is broader than just source-watcher — every fluxcd downstream controller consuming fluxcd/pkg's runtime patch helper is affected. Update the TODO(remove) marker on our workaround to point at that issue and note that both source-watcher v2.1.0 and v2.2.x (flux-aio latest) pull the same fluxcd/pkg runtime, so bumping source-watcher alone does not remove the need for this synthesis. Compile + unit tests unchanged. Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io> |
||
|
Ivan Okhotnikov
|
2fcf0bf5be |
fix(operator): document PackageSource Ready contract change + address review nits
kvaps and lexfrei confirmed the diagnosis by tracing source-watcher
v2.1.0 (same code paths in v2.2.x / flux-aio latest, so a simple
version bump does not remove the need for this workaround):
* fluxcd's patch.Helper patches .status and .status.conditions as
SEPARATE apiserver requests, so an etcd timeout during first-
install load can land between them — leaving Inventory /
ObservedSourcesDigest persisted while the Ready-condition write
is lost.
* On the next reconcile the !hasDrifted branch returns before
touching conditions. The deferred summarizeStatus still runs and
adjusts the Reconciling condition, but it never promotes a
stuck Unknown Ready to True — that promotion only happens on the
successful hasDrifted path.
Two substantive changes prompted by the reviews:
1. Document the semantic change the workaround introduces on
PackageSource.status.conditions[Ready]. fluxcd/pkg/runtime/
conditions.Set writes condition.ObservedGeneration =
ag.Generation on EVERY condition mutation including the
Progressing/Unknown mark at the start of a rebuild. And
ArtifactGeneratorStatus does not expose an object-level
ObservedGeneration (only ReconcileRequestStatus is inlined).
So Generation-matching cannot distinguish 'current revision
fully processed' from 'rebuild in progress, previous
Inventory/digest still persisted' — meaning the predicate can
(and does) fire during a content-only regeneration (new OCI
revision, same .metadata.generation).
Net effect: PackageSource Ready no longer dips through Unknown
while a new revision is being processed; it now means 'valid
consumable artifacts exist' rather than 'the current revision
has been processed'. Kvaps flagged this as the better contract
for install/upgrade ordering (no InProgress flapping, and a
genuine regeneration failure still surfaces as Ready=False on
the successful hasDrifted path). Document it inline instead of
implying the workaround only fires in the stuck-status window.
2. Reworked the predicate signature to take the pre-resolved
Ready condition as a parameter. The caller was already doing
the FindStatusCondition lookup for the pass-through path, so
the helper no longer duplicates it (kvaps' nit).
Also refreshed the pre-existing comments:
* The 'early-exits before touching status again' step is now
correctly worded as 'returns before touching conditions again;
the deferred summarizeStatus never promotes a stuck Unknown
Ready to True'.
* Bug attribution now names the code paths ('!hasDrifted branch',
'summarizeStatus deferred'), notes that v2.1.0 and v2.2.x
('flux-aio latest') both ship the affected code, and carries a
TODO(remove once upstream is fixed) marker so the workaround
cannot silently outlive its cause.
Tests: existing 8 subtests updated to construct the ready condition
manually so the call matches the new signature, plus one new
subtest locking in the intentional 'content-only regeneration
predicate fires' behaviour so a future refactor that tightens the
predicate back to 'current revision processed' — which would
re-introduce the flapping — fails loudly. 9/9 pass.
Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
|
||
|
Ivan Okhotnikov
|
a03e879316 |
fix(operator): require Ready.ObservedGeneration == ag.Generation before synthesising
Own-audit found the first pass had a spec-update race: if a user
edited PackageSource.spec.variants on a running cluster, cozystack
would bump the derived ArtifactGenerator to Generation=2, and
between "source-watcher started reconciling gen=2" and "source-
watcher wrote new Inventory + ObservedSourcesDigest for gen=2" the
ArtifactGenerator still exposes the previous generation's
Inventory / ObservedSourcesDigest AND Ready=Unknown (Progressing).
The workaround would synthesise Ready=True on stale artifacts, and
downstream Flux would consume the previous-generation
ExternalArtifacts as if the new spec had converged.
Tighten the predicate to require the Ready condition's
ObservedGeneration to match ag.Generation. Only when source-watcher
has actually processed the current spec (which happens strictly
after it writes the current-generation Inventory + Digest) does the
Ready condition carry ObservedGeneration == ag.Generation. That is
exactly the window where Inventory / Digest are guaranteed current-
spec AND only the status patch on Ready itself failed — the actual
upstream bug this workaround exists to paper over.
Consequence: if Ready is missing entirely we return false now
(was true). We cannot verify ObservedGeneration on an absent
condition, and the missing-Ready state is indistinguishable from
mid-reconcile-of-a-newer-generation. On the next reconcile source-
watcher will write at least a Progressing condition, at which point
the current-spec Generation check will be evaluable and the
workaround will kick in if — and only if — Inventory / Digest also
match the current spec.
Tests updated:
* changed 'no Ready condition' case from want=true to want=false
(reflecting the tighter invariant);
* added a case where Ready=Unknown carries an OLD Generation
(want=false — do not synthesise on stale artifacts);
* stamped ObservedGeneration on every existing condition-bearing
case so they exercise the matching-Generation branch;
* introduced a small agWithGen helper to keep the fixtures
readable now that Generation is load-bearing.
All 8 subtests pass; existing operator suite unaffected.
Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
|
||
|
Ivan Okhotnikov
|
73e88e8ff9 |
fix(operator): synthesise PackageSource Ready when source-watcher status-patch stalls
Symptom cozystack-platform HelmRelease times out at 15m on fresh install with a single PackageSource stuck in InProgress. Snapshot from #3178 CI run 28587656430: PackageSource cozystack.vm-disk-application Ready: Unknown, reason: Progressing lastTransitionTime: 2026-07-02T13:39:26Z (snapshot generated 30+ minutes later — no state change) ArtifactGenerator cozystack.vm-disk-application status.inventory: 2 items with sha256 digests status.observedSourcesDigest: populated status.conditions[Ready]: Unknown, reason: Progressing status.conditions[Reconciling]: True, reason: Progressing ExternalArtifacts are produced. Downstream Flux consumes them and its HelmReleases install fine. But PackageSourceReconciler faithfully copies the Unknown Ready condition through, so cozystack-platform (which waits on PackageSource.Ready=True) never converges. Root cause Traced in fluxcd/source-watcher:v2.1.0 log for the affected ArtifactGenerator: 13:39:28.475 ExternalArtifact vm-disk reconciled ✅ 13:39:28.641 ExternalArtifact vm-disk-rd reconciled ✅ 13:39:45.719 ❌ failed to update status: etcdserver: request timed out 13:39:45.720 ❌ Reconciler error (etcd timeout) 13:39:46.485 ✅ No drift detected, 2 artifact(s) up to date ——— nothing else for this ArtifactGenerator ——— Under first-install etcd pressure the patchStatus call times out. The reconciler returns the etcd error; controller-runtime prints the "returned both a result and non-nil error, RequeueAfter ignored" warning and requeues. The next reconcile hits the "No drift detected" short-circuit (artifacts are already up to date) and early-exits before touching status. Result: the Ready condition is stuck Unknown until either the process restarts or something forces a state change on the ArtifactGenerator. Neither happens in a real install run. This is an upstream bug in source-watcher's reconcile loop. It has been papered over once already at the runtime-tuning level in #3163 (source-watcher concurrency + CPU), but a bad etcd second during install still lands us here. Cozystack cannot afford to wait for an upstream release cycle to unblock every install. Fix PackageSourceReconciler.updateStatus now checks two invariants before falling back to copying the (potentially stuck) upstream Ready condition: 1. ArtifactGenerator.status.Inventory is non-empty 2. ArtifactGenerator.status.ObservedSourcesDigest is set Both fields are only written by source-watcher after the artifacts have actually been produced — status.Inventory carries the ExternalArtifact digests downstream Flux is already consuming. If BOTH are present AND the upstream Ready condition is still Unknown (or absent), the PackageSource Ready condition is synthesised as True with a distinctive reason `ArtifactsGeneratedAwaitingUpstreamStatus` and a message that names the upstream bug and the observable state it relied on. If the upstream Ready ever catches up (True or False) the Owns() watch re-fires the reconciler and the real condition overwrites the synthetic one — True stays True, False surfaces the real failure. Failures are NEVER masked: the synthesis path only triggers when Ready is Unknown or missing. Safety * Never synthesises True over a real False → upstream failure is surfaced unchanged. * Requires BOTH Inventory non-empty AND ObservedSourcesDigest set → a fresh ArtifactGenerator that has literally not started work cannot be prematurely marked Ready. * ObservedGeneration is stamped so a spec change re-invalidates the synthesis until the next observable-artifacts pass. * Reason string is greppable so operators can audit which PackageSources came up via the workaround. Tests Added TestArtifactGeneratorObservablyReady with 7 cases covering every branch of the predicate: - fresh generator: false - inventory only: false - digest only: false - both set + Unknown Ready: true (the actual bug) - both set + no Ready cond: true (same window pre-stamp) - both set + Ready=True: false (pass real condition) - both set + Ready=False: false (never mask failure) All pass; existing operator unit tests are unaffected. Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io> |
||
|
Aleksei Sviridkin
|
a44d11d2b6 |
fix(cozystack-basics): validate legacy Ingress hostnames against the tenant apex (#3200)
## What this PR does The Gateway (`cozystack-gateway-hostname-policy`) and Route (`cozystack-route-hostname-policy`) admission policies constrain tenant hostnames to the tenant apex on the Gateway API path. Gateway API is opt-in and off by default, so in the default configuration tenant applications publish through a legacy `networking.k8s.io/Ingress` on the shared ingress-nginx, which had no equivalent policy. This adds `cozystack-ingress-hostname-policy`, a fail-closed `ValidatingAdmissionPolicy` + binding for `Ingress` in `tenant-*` namespaces — the legacy-Ingress counterpart of the existing Gateway/Route hostname policies, and a new Layer 8 in the gateway security model. A hostname on `spec.rules[].host` or `spec.tls[].hosts[]` is allowed when it is within the namespace's own apex (`namespace.cozystack.io/host`), or when it is a concrete (non-wildcard) name entirely outside the platform root apex (`_cluster.root-host`). The second case keeps the `kubernetes` app's Proxied `exposeMethod` working, since it routes a user-supplied external domain to a nested cluster. A hostname under the platform root apex but outside the namespace's own apex, a wildcard taken through the outside-root path (which could otherwise match the platform apex itself), a rule with no host (absent or empty — an unbounded catch-all), and `spec.defaultBackend` are rejected. The policy is fail-closed: a `tenant-*` namespace missing its own apex label is denied, and the policy renders only when `_cluster.root-host` is set, so it is never shipped in a state that would admit every hostname. `cozy-*` namespaces are cluster-admin-managed and not gated. This is defense-in-depth in the same family as the Gateway/Route policies — by design tenants hold only read access to `Ingress`. The change ships helm-unittest coverage (render, plus the root-host, empty-host, and wildcard guards) and behavioral admission tests against the live apiserver in `hack/e2e-apps/gateway.bats` (in-apex allow, external-domain allow, under-root-outside-apex deny, wildcard-matching-apex deny, hostless deny, empty-string-host deny, defaultBackend deny), and documents the new layer in `packages/extra/gateway/README.md`. ### Screenshots N/A — no UI change. ### Release note ```release-note fix(cozystack-basics): constrain legacy Ingress hostnames in tenant namespaces to the tenant apex ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added **fail-closed** defense-in-depth Layer 8 (`cozystack-ingress-hostname-policy`) to enforce allowed legacy Ingress rule and TLS hostnames for tenant namespaces, while blocking unsafe catch-all configurations (including missing/empty hosts and `spec.defaultBackend`) when the platform root host is configured. * **Tests** * Added end-to-end and render/deny validation for permitted vs denied hostname patterns, wildcard behavior, required namespace host labeling, and correct deny/fail-closed rendering. * **Documentation** * Updated the gateway security model to document Layer 8 behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
IvanHunters
|
a1c779854b |
fix(vertical-pod-autoscaler): migrate admission-controller TLS to cert-manager to stop reconcile-driven ReplicaSet churn (#3178)
## Summary
The VPA admission-controller webhook TLS Secret is currently rendered by
an in-chart `genCA` + `genSignedCert` template. Those functions are not
idempotent across `helm template` invocations — every Flux HelmRelease
reconcile produces a fresh CA + cert. A `checksum/tls-secret`
podAnnotation on the admission-controller Deployment then flips its
sha, materialising a new ReplicaSet on every reconcile.
Under load — the E2E install phase where cert-manager / CDI / LINSTOR
cascade races repeatedly refire the VPA HelmRelease — this produces
10+ admission-controller ReplicaSets in minutes and blocks the whole
cozystack-platform HR: the admission-controller pod livelocks in
startup (waits on its own election lease which its previous
ReplicaSet is still holding, hits livenessProbe timeout,
CrashLoopBackOffs), VPA never becomes Ready, PackageSources stay
InProgress, and cozystack-platform aborts at its 15-minute install
timeout.
Fix: replace the source of the TLS Secret with a namespace-scoped
cert-manager `Issuer` + `Certificate`. cert-manager only rewrites the
Secret when the certificate is close to expiry (`renewBefore`), so
reconcile-driven cert churn stops. Pod rotation is delegated to a
runtime-lookup-based `checksum/tls-cert` podAnnotation that only
changes when the Secret's `tls.crt` content actually changes — not on
every render, not on every reconcile.
## What
-
`charts/vertical-pod-autoscaler/templates/admission-controller/certificate.yaml`
(new): namespace-scoped selfSigned Issuer + Certificate. The
Certificate mirrors the legacy `genSignedCert` shape (single DNS SAN
`vpa-webhook.<ns>.svc`, 365d validity, matching Secret name), so
cert-manager on the first upgrade adopts the same Secret name the
legacy path wrote — no rename cascade in the Deployment volume mount.
Rendered only when `admissionController.tls.certManagerEnabled=true`
AND no `existingSecret` is supplied.
-
`charts/vertical-pod-autoscaler/templates/admission-controller/tls-secret.yaml`
(modified): wrapped in an extra `certManagerEnabled=false` guard so
the legacy genCA path remains available as an explicit opt-out.
-
`charts/vertical-pod-autoscaler/templates/admission-controller/deployment.yaml`
(modified): the legacy `checksum/tls-secret` annotation is gated
behind `certManagerEnabled=false`. Under the cert-manager path the
Deployment carries a `checksum/tls-cert` annotation computed from
`lookup`-ing the runtime Secret's `tls.crt` — see the template
comment for the full timing analysis and the day-350 renewal
trade-off it documents.
- `charts/vertical-pod-autoscaler/values.yaml` (modified): new
`admissionController.tls.certManagerEnabled` value defaulting to
`true`. Values-block header documents the switch as effectively
one-way (cert-manager writes `type: kubernetes.io/tls`;
`Secret.type` is immutable, so falling back requires `kubectl
delete` first).
- `patches/cert-manager-admission-tls.diff` (new) +
`Makefile update` extended: the four subchart edits above live as a
single reviewable diff that Makefile re-applies over the freshly
vendored chart on every `make update`, matching the fluxcd-operator
package convention. Prevents a bump from silently reverting the
fix.
- `packages/core/platform/sources/vertical-pod-autoscaler.yaml`
(modified): VPA PackageSource now depends on `cozystack.cert-manager`.
Non-circular: cert-manager depends on `cozystack.networking` +
`cozystack.gateway-api-crds`, neither reaches VPA.
- `tests/vpa_test.yaml` (modified): +11 helm-unittest cases (15 total,
4 pre-existing) covering `certManagerEnabled={true,false}`,
`existingSecret` bypass, disabled admission-controller, and
the offline-render behaviour of the `checksum/tls-cert` lookup.
## Why (evidence)
Snapshot from CI run 28587656430 (this branch, pre-workaround-review):
```
admission-controller ReplicaSets after first-install churn window:
vertical-pod-autoscaler-admission-controller-55d888d6d4
vertical-pod-autoscaler-admission-controller-56f8d4c747
vertical-pod-autoscaler-admission-controller-5775dbc9b7
vertical-pod-autoscaler-admission-controller-585b7c56c9
vertical-pod-autoscaler-admission-controller-6fc44944fb
vertical-pod-autoscaler-admission-controller-74bccf4849
vertical-pod-autoscaler-admission-controller-84df98f98b
vertical-pod-autoscaler-admission-controller-8666dffb85
vertical-pod-autoscaler-admission-controller-86b469457c
vertical-pod-autoscaler-admission-controller-9b979f9d6
→ 10 ReplicaSets, one per Flux reconcile, each with a different
genCA-rooted TLS Secret sha in checksum/tls-secret.
```
## Migration for existing clusters
On upgrade the legacy Opaque Secret disappears from the release
manifest, Helm garbage-collects it, and cert-manager immediately
writes a fresh `kubernetes.io/tls` Secret of the same name via the
new Certificate. `Secret.type` is immutable, so in-place adoption is
impossible — the delete/create pass is the correct path, and the
seconds-long window it produces is safe: the `vpa-webhook-config`
MutatingWebhookConfiguration ships with `failurePolicy: Ignore`
(verified on cozy-dev3), so Pod admissions in that window skip VPA
mutation but never get rejected — no cluster-wide disruption.
The Deployment rolls in the same upgrade because the new
`checksum/tls-cert` annotation differs from anything the pod had
before, so the admission-controller re-registers `vpa-webhook-config`
with the fresh caBundle on the same pass.
## Test plan
- [x] `helm unittest packages/system/vertical-pod-autoscaler` — 15/15
pass.
- [x] `helm template` diff between main and this branch is scoped
exactly to the cert-manager path (new Certificate + Issuer,
new `checksum/tls-cert` annotation, guarded legacy Secret, new
values key, patch file + Makefile step, PackageSource
dependsOn). No unrelated lines change.
- [x] `make update`-style simulation (rm -rf charts && helm pull &&
patch) produces a byte-identical subchart to what git tracks —
the patch stays clean across future upstream bumps until it
genuinely conflicts.
- [ ] Live migration on cozy-dev3: `helm upgrade` VPA release from
pre-fix version → observe cert-manager write the new tls Secret
→ verify `checksum/tls-cert` on the new pod's podTemplate →
verify MutatingWebhookConfiguration `vpa-webhook-config`
caBundle updated in place → confirm no ReplicaSet churn on
subsequent Flux reconciles. Blocking the draft's removal.
- [ ] Full E2E on this branch — verify only 1 admission-controller
ReplicaSet after install, VPA HR reaches Ready inside
`cozystack-platform`'s 15m install timeout, and
`kubernetes-latest.bats` / `kubernetes-previous.bats` no longer
time out on VPA-attributable resource churn.
## Review pass — kvaps' CHANGES_REQUESTED addressed
- ✅ Vendored chart mods now carried as `patches/*.diff` re-applied by
`Makefile update` (matches fluxcd-operator convention).
- ✅ Retired the tls-migrator hook — kvaps correctly showed it was
a no-op: Helm's delete decision is driven by the stored release
manifest not by live labels (`ResourcePolicyAnno` is the only opt-
out), and `Secret.type` is immutable so cert-manager could not have
adopted the legacy Opaque Secret in place anyway. ~200 lines
removed.
- ✅ PR description rewritten to match the actual diff (no more
reference to the removed `tls-rollout.yaml` post-upgrade hook, real
test count, dev3 verification honestly listed as pending).
- ✅ `checksum/tls-cert` timing comment corrected: `lookup` runs on
Helm actions only, day-350 renewal trade-off spelled out.
- ✅ `certManagerEnabled=false` documented as effectively one-way
(Secret.type immutable).
- ✅ Dropped the `secretTemplate` labels — `app.kubernetes.io/managed-
by: Helm` on a cert-manager-owned Secret was misleading.
- ✅ Banner comment indentation restored in `vpa_test.yaml`.
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **New Features**
* VPA admission-controller webhook TLS now uses cert-manager by default,
with options to disable cert-manager or supply an existing TLS secret.
* Webhook pod restarts are now tied to cert-manager Secret contents via
a content-based checksum.
* **Bug Fixes**
* Improved webhook TLS provisioning reliability when TLS readiness is
delayed.
* Corrected conditional rendering/rollout behavior to avoid mismatched
TLS ownership.
* **Tests**
* Added rendering-invariant tests covering cert-manager, legacy TLS,
existing-secret bypass, and admission-controller disabled behavior.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
Aleksei Sviridkin
|
d89ad70698 |
fix(cozystack-basics): validate legacy Ingress hostnames against the tenant apex
The Gateway (cozystack-gateway-hostname-policy) and Route (cozystack-route-hostname-policy) admission policies constrain tenant hostnames on the Gateway API path. Gateway API is opt-in and off by default, so in the default configuration tenant applications publish through a legacy networking.k8s.io/Ingress on the shared ingress-nginx, which had no equivalent policy. Add a fail-closed ValidatingAdmissionPolicy + Binding for Ingress in tenant-* namespaces. A hostname on spec.rules[].host or spec.tls[].hosts[] is allowed when it is within the namespace's own apex (namespace.cozystack.io/host), or when it is a concrete (non-wildcard) name entirely outside the platform root apex (_cluster.root-host); a hostname under the platform root apex but outside the namespace's own apex, and a wildcard taken through the outside-root path (which could match the platform apex itself), are rejected. The outside-root branch keeps the kubernetes app's Proxied exposeMethod working, since it routes a user-supplied external domain to a nested cluster. A rule with no host (absent or empty, an unbounded catch-all) and spec.defaultBackend are also rejected. The policy templates the platform root apex into its CEL and renders only when _cluster.root-host is set, so it is never shipped in a state that would admit every hostname. A tenant-* namespace missing its own apex label is denied. Cover the policy with helm-unittest (render, including the root-host, empty-host, and wildcard guards) and behavioral admission tests against the live apiserver (in-apex allow, external-domain allow, own-apex-wildcard allow, external-tls allow, under-root-outside-apex deny, prefix-adjacent-under-root deny, wildcard-matching-apex deny, hostless deny, empty-string-host deny, defaultBackend deny), and document it as an additional layer in the gateway security model. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
2cfcdc265c |
fix(cert-manager): raise cainjector memory limit to unblock caBundle injection (#3199)
## What this PR does cert-manager's cainjector loads every ValidatingWebhookConfiguration, MutatingWebhookConfiguration, APIService and CRD into its informer caches at startup. On a full cozystack install that working set exceeds the 128Mi memory limit the package sets today, so the single leader-elected cainjector Pod is OOMKilled (exit 137) during cache population and enters CrashLoopBackOff before it ever runs a reconcile. Because cainjector is the component that injects the issuing CA into webhook `clientConfig.caBundle` (via `cert-manager.io/inject-ca-from`), a cainjector that never completes a reconcile leaves those caBundles empty. With `failurePolicy: Fail`, the apiserver then rejects every call to an affected webhook with `x509: certificate signed by unknown authority` — even though the cert-manager PKI chain is healthy and the webhook Pods serve a valid cert. In practice this intermittently blocks `Ingress` creation in tenant namespaces (the ingress-nginx admission webhook), which stalls dependent HelmReleases and fails installs. It behaves as a flake because it is a memory race: on a lighter run cainjector fits under 128Mi, injects the caBundle within seconds and everything works; on a heavier run (more CRDs/webhooks resident, slower leader election) peak startup memory crosses 128Mi and it is OOMKilled before the first injection, stalling CA injection cluster-wide for as long as it CrashLoops. Fix: raise the cainjector memory limit to 512Mi (matching the cert-manager controller in the same values file) and lift the request from 32Mi to 128Mi so injection completes deterministically instead of racing the OOM floor. A helm-unittest pins the sizing so a blanket resource change or a `make update` cannot silently lower it again. The webhook and controller resources are left unchanged — only cainjector exhibited the OOM. ### Screenshots N/A — no UI changes. ### Release note ```release-note fix(cert-manager): raise cainjector memory limit so CA injection into webhook caBundles no longer fails under memory pressure (previously surfaced as intermittent "x509: certificate signed by unknown authority" admission errors) ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Increased cainjector memory settings for cert-manager, helping it run more reliably during startup and cache population. * Added automated chart tests to verify the cainjector resource settings stay within the expected range. * **Bug Fixes** * Reduced the risk of cainjector being OOM-killed, improving stability and consistency of CA injection. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Ivan Okhotnikov
|
223c0ef4fb |
fix(cert-manager): raise cainjector memory limit to unblock caBundle injection
cainjector loads all ValidatingWebhookConfigurations, APIServices and CRDs into its informer caches at startup. On a full cozystack install that working set exceeds the 128Mi limit, so the single leader-elected cainjector Pod is OOMKilled during cache population and CrashLoops before it injects any caBundle. Webhook ValidatingWebhookConfigurations then keep an empty caBundle and every admission call fails with "x509: certificate signed by unknown authority" (failurePolicy: Fail), intermittently blocking Ingress creation and stalling tenant installs. Raise the cainjector memory limit to 512Mi (matching the controller) and the request to 128Mi, and add a helm-unittest that pins the sizing so a blanket resource change or a `make update` cannot silently lower it again. Signed-off-by: Ivan Okhotnikov <xorokhotnikov@gmail.com> |
||
|
Aleksei Sviridkin
|
10f8e2de1f |
feat(kamaji): default to 2 replicas, drop telemetry webhook (#2671)
## What this PR does
`kamaji-controller-manager` runs **9 sub-controllers per
TenantControlPlane** (bootstraptoken, coredns, konnectivity, kubeadm*,
kubelet, kubeproxy, …) inside a single Go process, **and** serves the
admission webhooks for `tenantcontrolplanes` + `datastores` from the
same pod via `kamaji-webhook-service`. With the upstream default
`replicaCount=1`, every webhook round-trip from the apiserver shares CPU
with the reconciler goroutines. On a multi-tenant cluster this pushes
the apiserver-observed p99 of `mtenantcontrolplane.kb.io` and
`vtenantcontrolplane.kb.io` from the kamaji-side `~10ms` (per
`controller_runtime_webhook_latency_seconds_count/sum`) up to `85–95ms`
— purely due to CPU contention.
Three safe defaults are changed:
- **`kamaji.replicaCount: 2`** — HA-safe baseline. `--leader-elect` is
already enabled upstream, so only one replica reconciles tenants; the
follower's CPU is fully available for webhook handlers. On a single-node
cluster both pods still schedule (just colocated).
- **`kamaji.telemetry.disabled: true`** — drops the
`--disable-telemetry` handler **and** removes the
`telemetry.kamaji.clastix.io` entry from the
`ValidatingWebhookConfiguration`. The latter requires `patches/1.diff`
to wrap the telemetry block in `{{- if not .Values.telemetry.disabled }}
… {{- end }}` inside the vendored
`controller-gen/validating-webhook.yaml`. Without the webhook entry, the
apiserver no longer round-trips to a no-op handler on every
TenantControlPlane CREATE/UPDATE/DELETE.
- **`kamaji.affinity` (soft `podAntiAffinity`)** — spread the two
replicas across nodes when possible, while keeping single-node clusters
working (`preferredDuringScheduling`, not required).
The `patches/1.diff` approach (re-applied by `make update`) mirrors the
pattern already used in `packages/system/fluxcd-operator/Makefile` and
`packages/system/ingress-nginx/Makefile`, and survives the next
`clastix/kamaji` chart bump.
## Measured impact
Hikube prod, 22 TenantControlPlanes, before/after a manual rollout of
the same configuration:
| webhook | p99 before | p99 after | Δ |
| --- | --- | --- | --- |
| `mtenantcontrolplane.kb.io` | 85 ms | 25 ms | **-71%** |
| `vtenantcontrolplane.kb.io` | 92 ms | 25 ms | **-73%** |
| `telemetry.kamaji.clastix.io` | 94 ms | _webhook removed_ | **-100%**
|
Cumulative Kamaji admission **impact** (`p99 × calls/s`) dropped from
**830 → 140 (-83%)**. The Kamaji webhooks moved from the top three of
the `apiserver_admission_webhook_admission_duration_seconds_*` impact
ranking down to positions #3-#4.
## Release note
```release-note
feat(kamaji): default to 2 replicas with soft pod anti-affinity and disable telemetry (handler + webhook entry). Reduces apiserver admission p99 for tenantcontrolplane mutating/validating webhooks by ~70% on multi-tenant clusters.
```
## Test plan
- [x] `helm template packages/system/kamaji` renders `replicas: 2`,
`--disable-telemetry`, the soft anti-affinity, and **no**
`telemetry.kamaji.clastix.io` entry in the
`ValidatingWebhookConfiguration`.
- [x] `patches/1.diff` applies cleanly against the freshly extracted
upstream chart (`patch --dry-run -p1 < patches/1.diff`).
- [x] `make update` re-applies the patch automatically.
- [x] Live deploy on a 22-TCP cluster: 3 pods Running (when overriden to
`replicaCount: 3` via cozystack-values), zero pod restarts, leader
election visible, `kamaji-webhook-service` Endpoints serving N IPs.
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **New Features**
* Telemetry can be disabled via configuration.
* Controller configured for high availability with 2 replicas.
* Pod anti-affinity added to spread replicas across nodes for better
resilience.
* **Bug Fixes**
* Validating webhook is now omitted when telemetry is disabled to avoid
unintended behavior.
* **Chores**
* Update process improved to re-apply chart patches after extraction for
reproducible updates.
<!-- review_stack_entry_start -->
[](https://app.coderabbit.ai/change-stack/cozystack/cozystack/pull/2671?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack)
<!-- review_stack_entry_end -->
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
Aleksei Sviridkin
|
809a04a220 |
feat(kubevirt-cdi): make cloneStrategyOverride configurable (#2926)
## What this PR does - Exposes the CDI `cloneStrategyOverride` as a Helm value in `packages/system/kubevirt-cdi` instead of hardcoding it in the CR template. - Default stays `csi-clone`, so **behavior is unchanged** for everyone. - It can now be overridden via a cozystack `Package` (e.g. `cloneStrategyOverride: copy`), the same way `uploadProxyURL` already is — no fork needed. ### Why The value was hardcoded, so the only way to change the clone strategy was to fork the package. On some storage backends `csi-clone` is problematic: with LINSTOR/DRBD a CSI clone is created co-located with the source golden image and then relocated to optimal nodes asynchronously; that async relocation (`migrate-disk`) can race the post-clone resize CDI performs, occasionally leaving a volume whose DRBD device is smaller than its PVC capacity. Being able to select `snapshot` or `copy` per cluster, without maintaining a fork, is a useful escape hatch. This only adds a knob; it does not change the default or recommend a strategy. ## Release note ```release-note feat(kubevirt-cdi): make CDI cloneStrategyOverride configurable via values (default csi-clone) ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * The KubeVirt CDI clone strategy is now configurable via Helm chart values, allowing selection of `csi-clone`, `snapshot`, or `copy` for DataVolume cloning. * The default remains `csi-clone` to preserve existing behavior and ensure backward compatibility. * **Documentation** * Added the `cloneStrategyOverride` Helm value description and allowed options to the chart’s configuration. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
901bb981ae |
fix(seaweedfs): configure postgres2 connection pool for the filer (#2906)
## What this PR does - Adds `WEED_POSTGRES2_CONNECTION_MAX_IDLE=20`, `WEED_POSTGRES2_CONNECTION_MAX_OPEN=40` and `WEED_POSTGRES2_CONNECTION_MAX_LIFETIME_SECONDS=600` to the filer `extraEnvironmentVars` in `packages/system/seaweedfs/values.yaml`. The vendored seaweedfs chart only ships connection pool settings for the **mysql** store (`WEED_MYSQL_CONNECTION_MAX_*`), which cozystack does not use. The **postgres2** store, which cozystack enables, has no pool settings at all. As a result the Go `database/sql` pool runs with `SetMaxIdleConns(0)`: every filer metadata lookup opens a fresh PostgreSQL connection (TCP + TLS + SCRAM, ~300 ms each). With ~5 path-component lookups per S3 operation, this adds roughly **2 seconds of latency to every S3 request**, even on an idle cluster. ### Measurements (production cluster, 12 nodes, 2 filers, CNPG backend) Before / after applying these three variables, same test (sequential ops through the S3 gateway): | Operation | Before | After | |---|---|---| | PUT 4 KiB (p50) | 10.0 s | 0.28 s | | GET 4 KiB (p50) | 3.0 s | 0.14 s | | HEAD (p50) | 2.6 s | 0.16 s | | Bounded LIST, 100 keys (p50) | 3.8 s | 0.37 s | `pg_stat_activity` confirmed the root cause: before the fix every backend was younger than ~200 ms (permanent connection churn); after the fix connections are reused for minutes. Real-traffic p95 of `SeaweedFS_filerStore_request_seconds` dropped from 1.3–6.5 s to ~0.18 s. `max_open` is capped at 40 so that the default two filer replicas stay below the CNPG default `max_connections` of 100. ## Release note ```release-note fix(seaweedfs): configure postgres2 connection pool for the filer, removing ~2s of per-request S3 latency caused by PostgreSQL connection churn ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Optimized PostgreSQL connection pool configuration in SeaweedFS to reduce latency and improve performance during metadata operations. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
ec9c29948b |
[reloader] Set resources requests/limits on reloader container (#2631)
## Summary
The upstream Stakater reloader chart ships with `resources: {}`, which
triggers a `no CPU limit` conformance warning on the `reloader`
Deployment.
Override `packages/system/reloader/values.yaml` at
`reloader.reloader.deployment.resources` with the exact values from the
chart's own commented example:
- `limits` : `cpu: 100m`, `memory: 512Mi`
- `requests` : `cpu: 10m`, `memory: 128Mi`
Reloader is a light controller that watches ConfigMap / Secret changes
and triggers workload restarts — small CPU envelope with enough memory
to track many objects in clusters with lots of namespaces.
## Test plan
- [ ] `helm template . --namespace cozy-reloader` from
`packages/system/reloader/` renders a `resources` block on the
`reloader-reloader` container of the Deployment (verified locally)
- [ ] Re-run the conformance/lint tool — the `reloader-reloader has no
CPU limit` warning should be gone
- [ ] CI passes
### Release note
\`\`\`release-note
Set CPU/memory requests and limits on the reloader Deployment to clear
the "no CPU limit" conformance warning. Values follow the upstream
chart's commented example (limits 100m/512Mi, requests 10m/128Mi).
\`\`\`
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Chores**
* Updated resource allocation configuration to define CPU and memory
parameters for system component deployments, enhancing infrastructure
stability and resource optimization.
[](https://app.coderabbit.ai/change-stack/cozystack/cozystack/pull/2631)
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
Aleksei Sviridkin
|
f2d66579f1 |
[metrics-server] Set resources requests/limits on metrics-server container (#2630)
## Summary The upstream metrics-server chart ships with `limits` commented out (only `requests cpu 100m / memory 200Mi` are set), which triggers a `no CPU limit` conformance warning on the `metrics-server` Deployment. Override `packages/system/metrics-server/values.yaml` to keep the upstream-recommended baseline `requests` and add `limits`: - `limits` : `cpu: 500m`, `memory: 500Mi` - `requests` : `cpu: 100m`, `memory: 200Mi` Comfortable headroom on top of the [official sizing formula](https://github.com/kubernetes-sigs/metrics-server#configuration) (`100m + 1m/node`, `200Mi + 2Mi/node`), covering clusters up to ~150 nodes without needing to revisit. Larger clusters can override these values further. ## Test plan - [ ] `helm template . --namespace cozy-monitoring` from `packages/system/metrics-server/` renders a `resources` block on the `metrics-server` container of the Deployment (verified locally) - [ ] Re-run the conformance/lint tool — the `metrics-server has no CPU limit` warning should be gone - [ ] CI passes ### Release note \`\`\`release-note Set CPU/memory requests and limits on the metrics-server Deployment to clear the "no CPU limit" conformance warning. Values follow the upstream sizing recommendation (requests 100m/200Mi) with a comfortable limits cap (500m/500Mi). \`\`\` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Configured explicit resource requests and limits for the metrics-server service (CPU: 100m-500m, Memory: 200Mi-500Mi) to ensure consistent resource allocation. [](https://app.coderabbit.ai/change-stack/cozystack/cozystack/pull/2630) <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
4952909e4d |
[external-dns] Set resources requests/limits on external-dns container (#2629)
## Summary
The upstream external-dns chart ships with `resources: {}`, which
triggers a `no CPU limit` conformance warning on the `external-dns`
Deployment.
Override `packages/system/external-dns/values.yaml` to set:
- `limits` : `cpu: 100m`, `memory: 128Mi`
- `requests` : `cpu: 10m`, `memory: 64Mi`
Modest sizing for a controller that just watches Service / Ingress
objects and pushes DNS records to the provider.
## Test plan
- [ ] `helm template . --namespace cozy-external-dns` from
`packages/system/external-dns/` renders a `resources` block on the
`external-dns` container of the Deployment (verified locally)
- [ ] Re-run the conformance/lint tool — the `external-dns has no CPU
limit` warning should be gone
- [ ] CI passes
### Release note
\`\`\`release-note
Set CPU/memory requests and limits on the external-dns Deployment to
clear the "no CPU limit" conformance warning.
\`\`\`
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Chores**
* Updated ExternalDNS resource allocation with optimized CPU and memory
limits and requests for improved resource efficiency.
[](https://app.coderabbit.ai/change-stack/cozystack/cozystack/pull/2629)
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
Aleksei Sviridkin
|
24e3bf98b5 |
[bootbox] Add resources requests/limits to matchbox container (#2625)
## Summary The `matchbox` container in `packages/extra/bootbox/templates/matchbox/deployment.yaml` had no `resources` block, triggering a `no CPU limit` conformance warning on the `bootbox-matchbox` Deployment (PXE/iPXE provisioning service). Bootbox is a standalone chart (no vendored upstream), so the values are set directly in the template — same pattern as #2614 (csi-resizer) and #2624 (s3manager). - `limits` : `cpu: 200m`, `memory: 256Mi` - `requests` : `cpu: 50m`, `memory: 128Mi` Modest sizing since matchbox is a lightweight HTTP server serving iPXE profiles and boot assets from ConfigMaps. ## Test plan - [ ] `helm template bootbox . --namespace tenant-root --show-only templates/matchbox/deployment.yaml --set _namespace.host=example.com --set _cluster.solver=letsencrypt` from `packages/extra/bootbox/` renders the `resources` block on the `matchbox` container (verified locally) - [ ] Re-run the conformance/lint tool — the `matchbox has no CPU limit` warning should be gone - [ ] CI passes ### Release note \`\`\`release-note Add CPU and memory requests/limits to the matchbox container of the bootbox Deployment to clear the "no CPU limit" conformance warning. \`\`\` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Added Kubernetes resource limits and requests configuration to optimize container resource allocation and improve deployment stability. [](https://app.coderabbit.ai/change-stack/cozystack/cozystack/pull/2625) <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
04a05f623c |
[bucket] Add resources requests/limits to s3manager container (#2624)
## Summary The `s3manager` container in `packages/system/bucket/templates/deployment.yaml` had no `resources` block, triggering a `no CPU limit` conformance warning on the bucket UI Deployment. The bucket package is a standalone chart (no vendored upstream), so the values are set directly in the template — same pattern as the csi-resizer fix in #2614 and the existing `capi-providers-cpprovider` template. - `limits` : `cpu: 500m`, `memory: 512Mi` - `requests` : `cpu: 100m`, `memory: 128Mi` Comfortable headroom for the lightweight S3 browser UI while keeping a small baseline request. ## Test plan - [ ] `helm template . --show-only templates/deployment.yaml` from `packages/system/bucket/` renders the `resources` block on the `s3manager` container (verified locally) - [ ] Re-run the conformance/lint tool — the `s3manager has no CPU limit` warning should be gone - [ ] CI passes ### Release note \`\`\`release-note Add CPU and memory requests/limits to the s3manager container of the bucket UI Deployment to clear the "no CPU limit" conformance warning. \`\`\` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Improved deployment configuration with explicit resource allocation settings to ensure optimal service stability and resource management. [](https://app.coderabbit.ai/change-stack/cozystack/cozystack/pull/2624) <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
6762d2d95c |
[goldpinger] Set resources requests/limits on goldpinger DaemonSet (#2623)
## Summary
The upstream Bloomberg goldpinger chart ships with `resources: {}`,
which triggers a `no CPU limit` conformance warning on the
`goldpinger-daemon` container running on every node.
Override `packages/system/goldpinger/values.yaml` to set:
- `limits` : `cpu: 200m`, `memory: 256Mi`
- `requests` : `cpu: 50m`, `memory: 64Mi`
Comfortable margin for the ping-mesh fan-out on larger clusters while
keeping a small baseline request so the DaemonSet schedules everywhere.
## Test plan
- [ ] `helm template . --namespace cozy-goldpinger` from
`packages/system/goldpinger/` renders the `resources` block on the
`goldpinger-daemon` container of the DaemonSet (verified locally)
- [ ] Re-run the conformance/lint tool — the `goldpinger has no CPU
limit` warning should be gone
- [ ] CI passes
### Release note
\`\`\`release-note
Set CPU/memory requests and limits on the goldpinger DaemonSet to clear
the "no CPU limit" conformance warning.
\`\`\`
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Chores**
* Updated Kubernetes resource requirements for the goldpinger component,
defining CPU limits (200m), memory limits (256Mi), CPU requests (50m),
and memory requests (64Mi).
[](https://app.coderabbit.ai/change-stack/cozystack/cozystack/pull/2623)
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
Aleksei Sviridkin
|
1aa6436922 |
[cert-manager] Set resources requests/limits on cert-manager pods (#2616)
## Summary
The upstream cert-manager chart ships with `resources: {}` for all four
workloads, which triggers `no CPU limit` (and `no memory limit`)
conformance warnings on every pod.
Override `packages/system/cert-manager/values.yaml` (previously empty)
to set sane requests/limits sized to each component's role:
| Component | requests cpu/mem | limits cpu/mem | Notes |
|---|---|---|---|
| controller | 50m / 128Mi | 500m / 512Mi | Main workload — issues and
renews certs |
| webhook | 10m / 32Mi | 200m / 128Mi | Admission webhook, short-lived
requests |
| cainjector | 10m / 32Mi | 200m / 128Mi | CA bundle injection, light
watch loop |
| startupapicheck | 10m / 16Mi | 100m / 64Mi | One-shot Job at install
time |
Requests for `cainjector` (`10m` cpu / `32Mi` mem) match the example in
the upstream values.yaml comment.
## Test plan
- [ ] `helm template . --namespace cert-manager` from
`packages/system/cert-manager/` renders a `resources` block on all four
pods (verified locally for controller, webhook, cainjector and the
startupapicheck Job)
- [ ] Re-run the conformance/lint tool that originally flagged the
warnings on `cert-manager-cainjector` — should be clean on the other
three pods too
- [ ] CI passes
### Release note
\`\`\`release-note
Set CPU/memory requests and limits on cert-manager controller, webhook,
cainjector and startupapicheck pods to clear "no CPU limit" conformance
warnings.
\`\`\`
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Chores**
* Added resource allocation configuration for cert-manager deployment,
specifying CPU and memory requests and limits for improved system
stability and resource management across core components.
[](https://app.coderabbit.ai/change-stack/cozystack/cozystack/pull/2616)
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
Aleksei Sviridkin
|
b2d00af495 |
[kubernetes] Add CPU/memory limits to csi-resizer sidecar (#2614)
## Summary - The `csi-resizer` container in `packages/apps/kubernetes/templates/csi/deploy.yaml` was the only sidecar in the `kcsi-controller` Deployment without `resources.limits`, which triggers a "no CPU limit" conformance warning. - Aligned it with the other six CSI sidecars in the same Deployment: `limits` `cpu: 512m` / `memory: 512Mi` and `requests` `cpu: 125m` / `memory: 128Mi`. - No other files touched; vendored charts and other CSI drivers (`nfs-driver`, `piraeus-operator`) are out of scope for this PR. ## Test plan - [ ] `make -C packages/apps/kubernetes show NAME=test NAMESPACE=test` renders the Deployment with the new `limits` block - [ ] Re-run the conformance/lint tool that originally flagged the warning — `csi-resizer has no CPU limit` should be gone - [ ] CI passes ### Release note \`\`\`release-note Set CPU and memory limits on the csi-resizer sidecar of the kubevirt-csi controller to align with the other CSI sidecars and clear the "no CPU limit" conformance warning. \`\`\` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Updated resource allocation for the CSI resizer component, increasing CPU and memory requests to improve performance and reliability. [](https://app.coderabbit.ai/change-stack/cozystack/cozystack/pull/2614) <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
mattia-eleuteri
|
8bc62cca1c |
feat(kamaji): default to 2 replicas, drop telemetry webhook
The kamaji-controller-manager runs ~9 sub-controllers per
TenantControlPlane (bootstraptoken, coredns, konnectivity, kubeadm*,
kubelet, kubeproxy, etc.) inside a single Go process, AND serves the
admission webhooks for tenantcontrolplanes/datastores via the
kamaji-webhook-service ClusterIP. With the upstream default
replicaCount=1, every webhook RT shares CPU with the reconciler
goroutines. On a multi-tenant cluster (~20 TCPs), this pushes
apiserver-observed p99 for mtenantcontrolplane.kb.io and
vtenantcontrolplane.kb.io from the kamaji-side ~10ms to 85-95ms.
Three changes, each safe on small clusters and high-impact on large ones:
* replicaCount=2 — HA-safe baseline. --leader-elect is already enabled
upstream, so only one replica reconciles tenants; the follower's CPU
is fully available for webhook handlers, cutting p99 ~70% on busy
clusters. On a single-node cluster both pods schedule normally.
* telemetry.disabled=true — drops both the --disable-telemetry handler
and the telemetry.kamaji.clastix.io admission webhook entry from the
ValidatingWebhookConfiguration. The latter requires patches/1.diff to
wrap the telemetry block in a {{ if not .Values.telemetry.disabled }}
conditional inside the vendored controller-gen file. Without the
webhook entry, the apiserver no longer issues a TCP round-trip to a
no-op handler on every TenantControlPlane CREATE/UPDATE/DELETE.
* podAntiAffinity (preferred) — spread the two replicas across nodes
when possible. preferredDuringScheduling so single-node clusters
still schedule (both pods colocated, which is acceptable).
Measured on a 22-TCP production cluster (Hikube, before/after):
mtenantcontrolplane p99 85 → 25 ms (-71%), vtenantcontrolplane p99
92 → 25 ms (-73%), telemetry webhook impact 291 → 0. Total Kamaji
admission impact -83% (830 → 140).
The patches/1.diff approach (re-applied by `make update`) mirrors the
pattern already used in fluxcd-operator and ingress-nginx and survives
the next clastix/kamaji chart bump.
Signed-off-by: mattia-eleuteri <mattia@hidora.io>
|
||
|
mattia-eleuteriandClaude Opus 4.8
|
c47bb6edb0 |
feat(kubevirt-cdi): make cloneStrategyOverride configurable
The CDI cloneStrategyOverride was hardcoded to csi-clone in the CR template, so operators could only change it by forking the package. Expose it as a Helm value (default csi-clone, behavior unchanged) so it can be set via a cozystack Package override, the same way uploadProxyURL already is. This is useful where the csi-clone path is problematic on a given storage backend (e.g. LINSTOR/DRBD, where a clone is co-located with the source and relocated afterwards; the async relocation can race the post-clone resize), letting operators select snapshot or copy without maintaining a fork. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: mattia-eleuteri <mattia@hidora.io> |
||
|
mattia-eleuteri
|
49cbea8c73 |
fix(seaweedfs): configure postgres2 connection pool for the filer
The vendored seaweedfs chart ships connection pool settings only for the mysql store, which cozystack does not use. The postgres2 store, which is enabled, has no pool settings, so the Go sql pool keeps zero idle connections and every filer metadata lookup opens a fresh PostgreSQL connection (TCP + TLS + SCRAM, ~300ms each). With ~5 path lookups per S3 operation this adds roughly 2 seconds of latency to every S3 request, regardless of cluster load. Measured on a production cluster (12 nodes, 2 filers): PUT of a 4KiB object went from 10s (p50) to 0.28s, HEAD from 2.6s to 0.16s after setting these values; pg_stat_activity confirmed connections were previously recycled every ~200ms and are now reused. max_open is capped at 40 so that two filer replicas stay below the CNPG default max_connections of 100. Signed-off-by: mattia-eleuteri <mattia@hidora.io> |
||
|
Matthieu
|
adb307cbae |
[goldpinger] Set resources requests/limits on goldpinger DaemonSet
The Bloomberg goldpinger chart ships with resources: {}, which triggers
a 'no CPU limit' conformance warning on the goldpinger-daemon container
deployed on every node. Override the package values.yaml to set
limits 200m/256Mi and requests 50m/64Mi — comfortable margin for the
ping-mesh fan-out on larger clusters while keeping a small baseline
request.
Signed-off-by: Matthieu <matthieu@hidora.com>
|
||
|
Matthieu
|
bf54945c08 |
[reloader] Set resources requests/limits on reloader container
The upstream Stakater reloader chart ships with resources: {}, which
triggers a 'no CPU limit' conformance warning on the reloader
Deployment. Apply the exact values from the chart's own commented
example (limits 100m/512Mi, requests 10m/128Mi) — reloader is a light
controller that watches ConfigMap/Secret changes and triggers restarts,
so a small CPU envelope with enough memory to track many objects fits.
Signed-off-by: Matthieu <matthieu@hidora.com>
|
||
|
Matthieu
|
3c2133ce4b |
[metrics-server] Set resources requests/limits on metrics-server container
The upstream metrics-server chart ships with limits commented out (requests cpu 100m / memory 200Mi only), which triggers a 'no CPU limit' conformance warning. Override the package values.yaml to keep the upstream-recommended baseline requests and add limits 500m/500Mi — comfortable headroom on top of the official sizing formula (100m + 1m/node, 200Mi + 2Mi/node), covering clusters up to ~150 nodes without needing to revisit. Signed-off-by: Matthieu <matthieu@hidora.com> |
||
|
Matthieu
|
7f72e7bbe8 |
[external-dns] Set resources requests/limits on external-dns container
The upstream external-dns chart ships with resources: {}, which triggers
a 'no CPU limit' conformance warning on the external-dns Deployment.
Override the package values.yaml to set limits 100m/128Mi and requests
10m/64Mi — modest sizing for a controller that just watches Service /
Ingress objects and pushes DNS records to the provider.
Signed-off-by: Matthieu <matthieu@hidora.com>
|
||
|
Matthieu
|
0ea94e7d2f |
[bootbox] Add resources requests/limits to matchbox container
The matchbox Deployment in the bootbox package had no resources block, triggering a 'no CPU limit' conformance warning. Set limits 200m/256Mi and requests 50m/128Mi — modest sizing since matchbox is a lightweight HTTP server serving iPXE profiles and boot assets from ConfigMaps. Bootbox is a standalone chart (no vendored upstream), so the values are set directly in the template, matching the existing pattern in csi-resizer and s3manager. Signed-off-by: Matthieu <matthieu@hidora.com> |
||
|
Matthieu
|
c8b180d91a |
[bucket] Add resources requests/limits to s3manager container
The s3manager Deployment in the bucket package had no resources block, triggering a 'no CPU limit' conformance warning. Set limits 500m/512Mi and requests 100m/128Mi — comfortable headroom for the lightweight S3 browser UI. Bucket is a standalone chart (no vendored upstream), so the values are set directly in the template, matching the existing pattern in csi-resizer and capi-providers. Signed-off-by: Matthieu <matthieu@hidora.com> |
||
|
Matthieu
|
cd95040c58 |
[cert-manager] Set resources requests/limits on cert-manager pods
The upstream cert-manager chart ships with resources: {} for all four
workloads (controller, webhook, cainjector, startupapicheck), which
triggers 'no CPU limit' conformance warnings on every pod. Override the
package values.yaml to set sane requests and limits sized to each
component's role: controller is the most loaded, webhook and cainjector
handle short-lived admission/injection work, and startupapicheck is a
one-shot Job.
Signed-off-by: Matthieu <matthieu@hidora.com>
|
||
|
Matthieu
|
50c4080ba3 |
[kubernetes] Add CPU/memory limits to csi-resizer sidecar
The csi-resizer container was the only sidecar in the kcsi-controller Deployment without resources.limits, triggering a 'no CPU limit' conformance warning. Align it with the other six CSI sidecars in the same Deployment (limits 512m/512Mi, requests 125m/128Mi) so capacity planning and audits stay uniform. Signed-off-by: Matthieu <matthieu@hidora.com> |
||
|
Aleksei Sviridkin
|
58d61d5652 |
fix(ci): build api-gate detector from base tip when merge base predates it (#3193)
## What this PR does The `API Review Gate` required check builds the `cmd/api-gate` detector from the PR's merge-base checkout so a PR cannot neuter the detector by editing its own code. When a PR's merge base predates the commit that introduced the gate (`fa871249c`), `cmd/api-gate` is absent from both the merge base and the equally old head, so the head fallback also fails to build and the check errors out before evaluating any API content. This red-blocks every open PR whose merge base is older than the gate, and neither a re-run nor an approving review clears it. This adds the base branch tip (`origin/<base>`) as an intermediate build source: prefer the merge-base checkout, fall back to the base tip when the merge base lacks the tool, and bootstrap from the head only while the tool exists nowhere on the base branch yet. The base tip is a protected, non-PR-authored branch, so it preserves the "analyze the head as data, not code" property. The API surface is still diffed against the merge base; only where the detector binary is compiled changes. This unblocks the pre-gate backlog without forcing every affected PR to rebase. The selection is kept inline in the workflow rather than in a helper script: a helper would be read from the untrusted PR head, which both reintroduces the failure for heads that predate the helper and lets a PR steer the selection away from the trusted build. Closes #3192 ### Screenshots N/A — CI-only change, no user-facing UI. ### Release note ```release-note fix(ci): API Review Gate now builds its detector from the base branch tip when the PR merge base predates the gate, so pre-gate PRs stop failing the check without needing a rebase. ``` |
||
|
Aleksei Sviridkin
|
361748ea6a |
fix(ci): build api-gate detector from base tip when merge base predates it
The API Review Gate builds cmd/api-gate from the PR's merge-base checkout so a PR cannot neuter the detector by editing its own code. When the merge base predates the commit that introduced the gate, cmd/api-gate is absent from both the merge base and the equally old head, so the head fallback also fails to build and the required check errors out before evaluating any API content. This blocks every open PR whose merge base is older than the gate, and neither a re-run nor an approving review clears it. Add the base branch tip as an intermediate build source: prefer the merge-base checkout, fall back to the base tip when the merge base lacks the tool, and bootstrap from head only while the tool exists nowhere on the base branch yet. The base tip is a protected branch, not PR-authored, so it preserves the "analyze head as data, not code" property. The API surface is still diffed against the merge base; only where the detector binary is compiled changes. This unblocks the backlog without forcing every affected PR to rebase. The selection stays inline in the workflow rather than in a helper script: a helper would be read from the untrusted PR head, which both reintroduces the failure for heads that predate the helper and lets a PR steer the selection away from the trusted build. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
IvanHunters
|
1baf126b1a |
fix(tenant): inherit full ancestor label chain from parent namespace (#2912)
## Summary
`packages/apps/tenant/templates/namespace.yaml` labels every tenant
namespace
with `tenant.cozystack.io/<ancestor>` for each of its ancestors. These
labels
drive the `<ancestor>-egress` `CiliumClusterwideNetworkPolicy`
(`networkpolicy.yaml`): an ancestor tenant's pods may reach a namespace
only if
that namespace carries `tenant.cozystack.io/<ancestor>`.
The chain was derived by splitting `.Release.Namespace`, which only ever
reached
one level up. A direct child of `tenant-root` got `tenant-root` (its
parent
namespace *is* `tenant-root`), but a grandchild — `tenant-ktj-htdev`
created
under `tenant-ktj` — got only `tenant-ktj` and dropped `tenant-root`
entirely.
Without `tenant.cozystack.io/tenant-root` on a nested namespace, the
root
ingress controller cannot reach the proxy service in a nested
`kubernetes`
tenant, surfacing as `upstream timed out (110) while connecting to
upstream` in
the nginx logs.
## Fix
Derive the ancestor chain deterministically from the namespace name
instead of
from any cluster lookup. Every tenant descends from `tenant-root`, so
that label
is emitted unconditionally; the remaining ancestors are encoded in the
namespace
name itself, because `tenant.name` builds each child as
`<parent-namespace>-<word>` — so every progressive dash-prefix of the
name is a
real ancestor namespace (`tenant-ktj-htdev` yields `tenant-ktj` and
`tenant-ktj-htdev`). The logic lives in a new
`tenant.ancestorTenantLabels`
helper:
```yaml
labels:
{{- include "tenant.ancestorTenantLabels" $tenantName | nindent 4 }}
```
This needs no cluster state, renders identically offline (so it is
unit-testable), and converges on every reconcile regardless of the order
in
which parent and child `HelmRelease`s reconcile — unlike a
parent-namespace
`lookup`, which helm-controller does not re-render on and which is
ordering-dependent. The `ownerReference` `lookup` is kept but made
non-fatal:
during a real reconcile the parent namespace always exists (the tenant
`HelmRelease` and its `cozystack-values` Secret live in it), so the
`ownerReference` is always emitted; when the lookup is empty (offline /
unit
test) it is simply omitted so the labels still render.
Migration `49` backfills the labels on existing clusters so they heal on
upgrade
regardless of per-tenant `HelmRelease` state (e.g. suspended or failed
releases), and `migrations.targetVersion` is bumped to `50` so the
runner
iterates it. The migration derives the same chain from each namespace
name,
is idempotent (`kubectl label --overwrite`) and best-effort
(per-namespace
failures are tolerated so a transient apiserver error cannot abort the
upgrade).
## Test plan
- [x] `helm template packages/apps/tenant` at depth 1/2/3 — emits
exactly the
expected ancestor set at each depth: depth-1 `{tenant-root,
tenant-ktj}`,
depth-2 adds `tenant-ktj-htdev`, depth-3 adds `tenant-ktj-htdev-foo`. No
missing, no stray labels.
- [x] `helm unittest packages/apps/tenant` — new
`tests/namespace_ancestor_labels_test.yaml` pins the chain at depths
1/2/3
plus a negative assertion; whole suite green.
- [x] `bats hack/cozystack-version-stamp.bats` — migration `49` stamps
the
version through the shared `stamp_cozystack_version` helper (all 10
tests
pass, including the guard that migrations >= 42 must use the helper).
- [x] Migration `49` ancestor derivation verified under `/bin/sh` to
match the
helm output byte-for-byte, and dry-runs cleanly under `set -euo
pipefail`.
- [ ] Live cluster: create a 3-level tenant (`root -> ktj -> htdev`),
verify
`tenant-ktj-htdev` has all three `tenant.cozystack.io/*` labels.
- [ ] Live cluster: create a `kubernetes` tenant under `htdev` with
`addons.ingressNginx.hosts`, verify an external HTTP request reaches the
tenant ingress controller via the root-ingress proxy (no `upstream timed
out`).
## Closes
- Closes #2810
- Closes #2171
```release-note
fix(tenant): tenant namespaces now carry the full `tenant.cozystack.io/<ancestor>` label chain (including `tenant-root`) at any nesting depth, restoring root-ingress reachability to nested tenants; a migration backfills the labels on existing clusters
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Bug Fixes**
* Improved tenant namespace label inheritance across multi-level tenant
hierarchies, ensuring all ancestor tenant labels are consistently
applied.
* **New Features**
* Standardized ancestor label generation to reliably include the full
tenant chain for derived tenant namespaces.
* **Tests**
* Added regression coverage for ancestor label presence and absence at
multiple hierarchy depths.
* **Chores**
* Added a migration to backfill missing ancestor labels for existing
tenant namespaces during platform upgrades.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
Ivan Okhotnikov
|
2ac269a04b |
docs(vertical-pod-autoscaler): fix stale ~350d in checksum/tls-cert ascii timeline
Second self-review pass caught a remaining reference to the old rotation math in the checksum/tls-cert doc block's ascii timeline (`renewal (~350d)` — from `renewBefore: 360h` when 365 - 15 = 350). With the widened `renewBefore: 2160h` rotation now lands at day ~275, matching the bullet-list narrative directly below. Update the number in place and regenerate the vendored-chart patch. No functional change; one-line doc fix. Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io> |
||
|
Ivan Okhotnikov
|
d6e57d67e7 |
docs(vertical-pod-autoscaler): sync deployment.yaml trade-off narrative with widened renewBefore
Self-review caught a stale day-math block in deployment.yaml that still
described the previous `renewBefore: 360h` behaviour ("cert-manager
renews at day ~350 and NO Helm action happens in the following ~15
days"). After bumping `renewBefore` to 2160h in the previous commit,
rotation now lands at day ~275 with a ~90-day window before expiry.
Update the numbers in place and regenerate the vendored-chart patch so
`make update` re-applies the corrected text instead of silently
reverting to the stale description on the next chart refresh.
No functional change — cert rotation math already lives in
`certificate.yaml`; this only fixes the parallel rationale block a
future reader would read alongside it.
Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
|
||
|
Ivan Okhotnikov
|
79ce69a3b4 |
fix(vertical-pod-autoscaler): widen renewBefore + document selfSigned checksum, add existingSecret belt-and-suspenders assertion
Address the three non-blocking follow-ups on myasnikovdaniil's approval: * renewBefore: 360h → 2160h (15d → 90d). With webhook `failurePolicy: Ignore`, an expired in-memory cert silently drops admission traffic until a Helm action rolls the ReplicaSet. helm-controller does not re-render on reconcile ticks after the release converges, only on install/upgrade/rollback — a 15-day window ties admission-webhook liveness to release cadence. 90 days survives realistic release schedules on customer clusters. Rotate-and-restart requirement is unchanged; the window is widened. * Document on deployment.yaml why hashing only `tls.crt` is safe: the Issuer is `selfSigned`, so `ca.crt == tls.crt` and every rotation re-issues `tls.crt`. If a future refactor ever swaps the Issuer to a CA-backed one (external CA, ACME, Vault), the checksum MUST be extended to also cover `ca.crt` — otherwise a CA-only rotation would leave the self-registered MutatingWebhookConfiguration serving a stale caBundle until the next leaf rotation. * Add a belt-and-suspenders helm-unittest assertion pinning that under `existingSecret` the admission-controller pod-template carries NEITHER `checksum/tls-secret` (legacy amplifier) NOR `checksum/tls-cert` (cert-manager runtime lookup). Templates are already structurally gated; this catches a future refactor that accidentally moves either checksum outside its `if` gate. Regenerated patches/cert-manager-admission-tls.diff against upstream via `make update` flow so the vendored chart edits stay in sync with the patch file (avoids silent revert on the next update). Verified: 16/16 helm-unittest cases pass, `helm lint` clean, `helm template ... certManagerEnabled=true` renders `renewBefore: 2160h`, and re-applying the regenerated patch onto a fresh upstream pull produces a byte-identical chart tree. Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io> |
||
|
myasnikovdaniil
|
c5ff2178e7 |
docs(release): add changelog for v1.4.5 (#3083)
This PR adds the changelog for release `v1.4.5`.
✅ Changelog has been automatically generated in
`docs/changelogs/v1.4.5.md`.
|
||
|
myasnikovdaniil
|
6f39beffb6 |
docs(release): add changelog for v1.5.2 (#3162)
This PR adds the changelog for release `v1.5.2`.
✅ Changelog has been automatically generated in
`docs/changelogs/v1.5.2.md`.
|
