Add first-class fields so a package/app can declare the Flux helm-controller
v1.5.0 kstatus health gate for the custom resource it wraps, so
HelmRelease.Ready means "the CR is actually healthy" instead of "helm applied
the CR" (#2642). API-owner approved in #3263.
- api: WaitStrategy (scalar, maps to HelmReleaseSpec.WaitStrategy.Name — a
deliberate simplification) + HealthCheckExprs ([]kustomize.CustomHealthCheck,
the upstream type verbatim) on both ApplicationDefinition.spec.release (the
apiserver path) and ComponentInstall (the operator path).
- config.ResolveWaitStrategy: shared by both HelmRelease builders. Couples the
default — healthCheckExprs imply the poller strategy when none is set, since
exprs are only evaluated under poller. This removes the "must also set
waitStrategy: poller" footgun and does not depend on the controller's global
default.
- thread through buildHelmReleaseSpec (operator) and
convertApplicationToHelmRelease (apiserver); the apiserver loads the typed
spec.release fields in start.go.
- regenerate deepcopy, the ApplicationDefinition and PackageSource CRDs, and
openapi. The backupstrategy CRDs also regenerate — benign k8s-0.35 PodSpec
description drift, not a functional change.
disableWait is intentionally not added (YAGNI): the only known need (the
kubernetes tenant-addon deadlock) was resolved child-side, and waitStrategy:
legacy is not equivalent (legacy still waits Helm-v3-style; disableWait skips
waiting entirely). Add it later when a concrete leaf needs a true no-gate
escape hatch.
Refs #2642
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Address lexfrei's non-blocking follow-ups on the LGTM approval:
Follow-up #1 — give-up preempted the final backoff. `decideRecovery`
returned `recoveryActionGiveUp` as soon as `attempts >= maxRecoveryAttempts`,
without checking whether the final-force backoff window had elapsed. The
Owns()-watch re-fire from the N-th force-drift's own writes triggered a
reconcile milliseconds after the force, so the effective time-to-give-up
was ~7.5m (30s+60s+2m+4m) — not the ~11.5m documented on the constants
block. The final 4-minute response window the last nudge was supposed to
grant source-watcher was never actually granted.
Fix: extend the exhaustion branch to hold `Wait` until
`now - lastRecoveryAt >= backoffFor(maxRecoveryAttempts)` before switching
to GiveUp. Now the documented ~11.5m budget matches the observed
behaviour, and the last force gets a real chance to succeed (Owns
watch → source-watcher Ready=True copy-through) before we surface
SourceWatcherStalled.
Tests:
* `TestDecideRecovery` gains a case pinning `Wait` when
attempts=maxRecoveryAttempts but the final backoff has not yet
elapsed (the exact re-fire shape the previous behaviour tripped on).
* `TestUpdateStatus_OwnMarkerAttemptsExhaustedButFinalBackoffPending`
drives the full updateStatus path against the same shape, asserting
the PackageSource stays in Unknown/AwaitingSourceWatcherRecovery and
the RequeueAfter equals the remaining backoff, NOT zero (GiveUp).
Follow-up #3 — upstream version-skew note. The Go module dependency
tracks `fluxcd/source-watcher/api/v2 v2.0.3` (CRD types package only),
but the deployed binary this workaround targets is pinned to v2.1.0
at `internal/fluxinstall/manifests/fluxcd.yaml`. Comments referencing
`drift.go:52` / `controller.go:164/254` describe the deployed binary,
not the compiled API package. Documented on the constants block so a
future flux bump re-verifies the anchors AND the watch predicate /
drift-detection heuristics, which are the load-bearing upstream
assumptions.
Follow-up #2 (envtest over full Reconcile→Owns→updateStatus cycle) is
architectural and out of scope for this PR — noted for a follow-up.
Signed-off-by: Ivan Okhotnikov <xorokhotnikov@gmail.com>
Self-review nits raised in a skeptical pass over the current tip:
* The status-patch docstring in forceArtifactGeneratorDrift described
the JSON-merge-patch full-array-replace on `status.conditions[]` as
"net-neutral". That was too optimistic: if source-watcher writes a
concurrent Reconciling condition between our Get and our Patch, our
patch does transiently wipe it. source-watcher restores it on its
next reconcile (which our requestedAt bump is about to trigger), but
downstream tooling watching for Reconciling can observe the momentary
absence. Rewrite the comment to name that trade-off explicitly and
point at server-side apply as the fuller fix if the operator ever
migrates to SSA globally.
* The unified own-marker/progressing/stuck gate in updateStatus is
asymmetric on ObservedGeneration — isProgressingInRecovery checks it,
isOwnMarker does not. That asymmetry is deliberate (a stale own-marker
after a spec edit must still route to the state machine to avoid
leaking the synthetic reason to the PackageSource — exactly the B2
regression), not an oversight. Add a comment explaining the
reasoning so a future edit doesn't "symmetrise" the check and re-open
B2.
* forceArtifactGeneratorDrift's step-2 (metadata patch) failure was
silent when it happened after a successful step-1 status patch. In
that state source-watcher has been nudged, but the tracking
annotations never persisted, so the next reconcile reads attempts=0
and re-forces from scratch instead of accumulating toward
SourceWatcherStalled — a persistent metadata-write-only apiserver
malfunction would loop forever. Log the failure loudly with the
intended attempt count so operators can spot this specific mode
from cozystack-operator logs.
Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
Self-review after the previous commit removed the awaitSourceWatcherResponse
helper (routing unified through maybeRecoverArtifactGenerator's state machine
via a single own-marker / progressing-in-recovery / stuck gate). Four
comments and two test names still referenced the now-deleted symbol —
misleading anyone tracing the architecture.
* maybeRecoverArtifactGenerator docstring: rewrote the trade-off paragraph
to describe the unified gate and Wait branch instead of the removed
helper.
* TestMaybeRecoverArtifactGenerator_DoesNotResetCounterOnFreshTransition
comment: same rewrite.
* TestUpdateStatus_OwnMarkerRoutesToAwait renamed to
TestUpdateStatus_OwnMarkerHoldsTrackingViaStateMachine + comment
rewritten to describe the state-machine Wait branch.
* TestUpdateStatus_ProgressingDuringRecoveryRoutesToAwait renamed to
TestUpdateStatus_ProgressingDuringRecoveryHoldsTracking + comment
rewritten in parallel.
Pure doc / naming; no code or test-body changes. Tests still pass with
-race, vet clean, golangci-lint 0 issues.
Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
The XValidation justification was factually wrong: the deployed
helm-controller CRD has no rule rejecting a Strategy alongside a
Remediation entry (only .retryInterval is gated on Strategy.Name), so
the claimed "silently break every HR" safety net does not exist.
Reword the comment in all three test files to the real reason
Remediation stays nil: retries are driven by a single
Strategy.RetryInterval path, matching cozystack-operator's
PackageReconciler.buildHelmReleaseSpec.
Address review feedback from lexfrei on rest_helmrelease_spec_test.go,
rest_timeout_test.go and internal/operator/package_reconciler_test.go.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
helm-controller's XValidation rule rejects Strategy.Name=RetryOnFailure
with RetryInterval set alongside a Remediation entry, so a future
"for safety" reintroduction of Remediation{Retries: -1} would silently
break every HR. Pin the deliberate omission.
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Avoid pointer aliasing across all generated HelmReleases. r.HelmReleaseMaxHistory
is set once at startup today, but a future mutation through the field would
silently affect every in-flight HelmReleaseSpec.
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Chart template now uses `ne (toString …) ""` for the five helmRelease*
knobs, so integer 0 (notably MaxHistory=0 = unlimited history per Helm
semantics, documented in values.yaml) is no longer silently dropped.
Extract buildHelmReleaseSpec from Reconcile and add unit tests
asserting Interval, MaxHistory, Install/Upgrade timeouts,
Strategy=RetryOnFailure, RetryInterval, and ChartRef propagate from
PackageReconciler config to the 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>
Add an opt-in UpgradeCRDs field to ComponentInstall that maps to
HelmRelease.Spec.Upgrade.CRDs, allowing a PackageSource component to
declare how Flux should handle CRDs from the chart's crds/ directory
on upgrade.
The helm-controller default on upgrade is Skip, which means new CRDs
added between chart versions never reach existing clusters and must be
applied manually. Setting upgradeCRDs: CreateReplace makes Flux apply
new CRDs declaratively with the chart.
Allowed values are restricted to Skip, Create, CreateReplace via a
kubebuilder enum marker. Empty / unset preserves the existing Flux
default, so all existing PackageSource resources keep working.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Instead of using per-Package SSA field owners (which is a workaround relying
on SSA mechanics), properly resolve whether a namespace should be privileged
by iterating all PackageSources and their active Packages. A namespace gets
the privileged PodSecurity label if ANY Package has a component with
privileged: true installed in it.
This fixes the race condition where Packages sharing a namespace (e.g.
linstor and linstor-scheduler in cozy-linstor) would overwrite each other's
labels depending on reconciliation order.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
When multiple Packages share a namespace (e.g. cozystack.linstor and
cozystack.linstor-scheduler both use cozy-linstor), the shared SSA field
owner "cozystack-package-controller" caused a race condition: the last
Package to reconcile would overwrite the namespace labels set by others.
This meant that if cozystack.linstor set pod-security.kubernetes.io/enforce=
privileged and then cozystack.linstor-scheduler reconciled without the
privileged flag, SSA would remove the label. On Talos clusters (which
default to "baseline" PodSecurity enforcement), this caused privileged
satellite pods to be rejected with PodSecurity violations.
Fix by using per-Package field owners (cozystack-package-{name}), so each
Package independently manages its own namespace labels without interfering
with other Packages sharing the same namespace.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Add dedicated flux-tenants controller with label selector
--watch-label-selector=sharding.fluxcd.io/key=tenants to handle
tenant workloads separately from platform components.
Update all kubernetes app HelmReleases to:
- Use chartRef with ExternalArtifact instead of OCIRepository sourceRef
- Add sharding.fluxcd.io/key=tenants label
- Add cozystack.io/target-cluster-name label
Update fluxinstall to parse multiple YAML manifest files and
use flux service for storage-adv-addr.
Add cozystack-basics package for core tenant/namespace setup.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Add automatic injection of cozystack-values secret reference into
HelmReleases created by Package reconciler. This enables charts to
access cluster and namespace configuration via .Values._cluster and
.Values._namespace.
Add annotation operator.cozystack.io/skip-cozystack-values to disable
injection for specific PackageSources (used for platform PackageSource
to avoid circular dependency).
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Myasnikov Daniil