mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-09 14:02:56 +09:00
8f4c19daaeb00a2aeccf761cd96879867a4d86ce
5128
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Myasnikov DaniilandClaude Opus 4.8
|
4777273dbd |
feat(release): add nightly OCIR->GHCR image mirror script
hack/nightly-mirror.sh copies the latest main build's cozystack-owned component images from the CI build registry (OCIR) to the public release registry (GHCR) by digest — no rebuild — then rewrites the baked package tree's image hosts so the GHCR install closure is self-contained. It reuses promote-retag.sh's three-shape ref enumeration, filters to cozystack-owned refs, and deliberately excludes the cozystack-packages artifact (rebuilt downstream from the rewritten tree). Dry-run bats test under the cozytest harness guards the ref selection, the cozystack-packages exclusion, third-party filtering, and the host rewrite plan. shellcheck clean. Assisted-By: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov DaniilandClaude Opus 4.8
|
a49ee8c42e |
fix(release): pin promote-retag dry-run test to the default REGISTRY
The CI workflow exports REGISTRY=<OCIR build registry> for every job (.github/workflows/pull-requests.yaml). The dry-run test inherited it, so promote-retag.sh filtered the tree's digest refs by the OCIR registry — but the committed release tree vendors them under ghcr.io/cozystack/ cozystack (the script default), so the selector matched nothing and aborted with 'No cozystack-owned digest-pinned image refs found', failing the Unit & controller tests job. Run the script with 'env -u REGISTRY' so it uses its default registry — the one the tree's refs and this test's assertions actually use — and add a slim self-diagnosing fallback that dumps the script's stderr and the yq build on any unexpected non-zero exit. Assisted-By: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov DaniilandClaude Opus 4.8
|
20c0806325 |
test(release): port promote-retag bats to the cozytest harness
hack/promote-retag_test.bats was written for upstream bats (run, $status, $output, setup(), skip), but CI runs hack/*.bats through hack/cozytest.sh, a pure-shell runner that has none of those. The first 'run' therefore died with 'cozytest.sh: run: not found', failing the Unit & controller tests job. Rewrite both cases in the cozytest idiom already used by select-e2e_test.bats and check-host-runtime.bats: redirect to temp files, capture the exit code with '|| rc=$?' so the harness's set -e does not abort the expected-failure case, and assert with plain shell. The selector logic under test is unchanged. Assisted-By: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
6a2f392b41 |
docs(release): rewrite for the write-once + rc-promotion model
The release doc still described the removed force-retag flow and auto-release.yaml cron. Update it to the model this PR ships: stable tags are created write-once at the PR merge commit (never force-moved), the maintenance branch is fast-forward-only, stable patches come only from rc promotion (nightly-rc.yaml + promote-rc.yaml + retention.yaml), and the api/apps/v1alpha1 Go-module tag is cut only for stable. Replaces the "Automated patch tag" and "Force-retagging" sections (the latter becomes "Tag immutability") and refreshes the See-also list. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
22a9437aa6 |
test(installer): cover COZYSTACK_VERSION env rendering
The COZYSTACK_VERSION env is what lets a release-candidate image be promoted to stable by retagging alone, and it is interleaved with the per-variant endpoint env under one conditional. Add a helm-unittest (and the package test target so `make unit-tests` runs it) pinning both axes: talos/hosted x platformVersion set/unset, asserting COZYSTACK_VERSION presence and the variant endpoint block. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
bb6e261163 |
fix(ci): escape rc version in promote sed; idempotent stable branch
The rc->stable substring rewrite ran `sed s/${RC_VERSION}/.../` with the dots
unescaped, so the rc version matched as a regex. Escape regex metacharacters so
the match is literal. Use `git checkout -B` for the stable staging branch so a
re-run resets the local branch instead of failing on "already exists".
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
|
||
|
Myasnikov Daniil
|
44a16d51dc |
fix(ci): scope nightly targets and gate api-tag/release-PR to stable
nightly-rc.yaml's `release-[0-9]*.[0-9]*` glob also matched the per-tag staging branches tags.yaml creates (release-1.4.0, release-1.4.0-rc.2, release-1.5.0-nightly.<date>), cutting spurious nightly tags. Anchor the target selection with a `^release-[0-9]+\.[0-9]+$` regex. tags.yaml now exposes an is_stable output and gates the write-once api/apps/v1alpha1 Go-module tag and the auto-opened release PR on it, so an rc/beta/alpha (is_stable=false) or a nightly (is_stable unset) never pollutes the module proxy or opens a release PR (#2677 goal 3). Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
27add81772 |
fix(release): stamp openapi.json with COZYSTACK_VERSION on promotion
The openapi-json target used `git describe`, so at promote time (checked out on the rc staging branch, before the stable tag exists) the stable release's openapi.json reported the rc version. Honor COZYSTACK_VERSION when set, mirroring the v-prefixed convention the cozypkg/check-readiness targets already use, and fall back to git describe only when it is unset. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
31ea4d4552 |
fix(release): retag only cozystack-owned images on promotion
collect_refs scraped every @sha256 ref from the package values.yaml, including third-party images (docker.io/clastix/kubectl, ghcr.io/kvaps/...), bare upstream tags (kube-ovn/keycloak/kilo) and a "--migrate-image=..." arg string. The first skopeo copy to a registry CI cannot push to aborted the whole promotion under set -e, so stable promotion could not complete. Filter the canonical refs to those under $REGISTRY (default ghcr.io/cozystack/cozystack, matching common-envs.mk; overridable for forks). Also require skopeo only when not --dry-run, so the selection is testable without it. Add hack/promote-retag_test.bats: a --dry-run over the real packages tree must emit only cozystack-owned refs (zero third-party), and a REGISTRY override that matches nothing must fail rather than promote the wrong set. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
c3fb1e21d8 |
fix(build): guard empty COZYSTACK_VERSION in operator render
Address review feedback from gemini-code-assist on Makefile:56,64,73: when COZYSTACK_VERSION is empty, v$(COZYSTACK_VERSION) expanded to a bare 'v', which is truthy and made the installer template render COZYSTACK_VERSION="v", overriding the operator's baked-in version. Guard each --set with $(if ...) so an empty version passes an empty value and the operator falls back to its build-time version. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
020e76a842 |
fix(hack): verify mikefarah yq in promote-retag
Address review feedback from gemini-code-assist on hack/promote-retag.sh:27: command -v yq only confirms some yq is on PATH; the script's queries use mikefarah syntax, so reject python-yq and other variants by checking the version string, mirroring the Makefile build-deps check. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
328394a873 |
fix(hack): make promote-retag ref_repo port-safe
Address review feedback from gemini-code-assist on hack/promote-retag.sh:48: ref_repo now strips the :tag from the last path component only, so a registry host carrying a :port (e.g. localhost:5000/cozystack/operator) on a no-tag ref is preserved instead of truncated to the host. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov DaniilandClaude Opus 4.8
|
58dc6eb2e0 |
ci(release): add nightly-rc and nightly retention workflows
- nightly-rc.yaml: daily cron that cuts a write-once vX.Y.Z-nightly.<date> tag for each tracked target (main + active release-X.Y branches) when its HEAD moved since the last nightly. The tag drives the normal rc build/e2e path. Gated by the NIGHTLY_RC_ENABLED repo variable so it can merge dark and be switched on deliberately. - retention.yaml: daily cron that keeps the newest 14 nightlies per line and deletes older nightly tags + their releases. Delete-only, scoped strictly to the *-nightly.* namespace (rc/stable untouched). Dry-run by default; flip the `apply` input or the RETENTION_APPLY repo variable to delete. Registry image pruning is left as a tracked TODO (no OCIR delete parity). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov DaniilandClaude Opus 4.8
|
9336641b95 |
ci(release): promote rc to stable by retag, no rebuild
Add the rc->stable promotion path. A stable release is a renamed release-candidate: the bytes shipped as vX.Y.Z are bit-for-bit the bytes built and e2e-tested as vX.Y.Z-rc.N. - hack/promote-retag.sh: reads the digest-pinned image refs the rc baked into packages/*/*/values.yaml (single-string, split repository/tag/digest, and the cozystack-packages OCI artifact) and `skopeo copy`s each — by digest — to the stable tag and :latest. Copying by digest guarantees the stable image is identical to the rc; the result is verified with `skopeo inspect`. Validated offline with `sh -n` and the yq extraction against the current tree; the skopeo path is CI-validated only. - promote-rc.yaml: workflow_dispatch(rc_tag). Validates the rc exists and the stable tag does not, retags via the script, rewrites the cosmetic -rc.N substring in the vendored tags to the stable version (the @sha256 wins regardless), restamps the version-stamped assets (operator manifests, cozypkg, openapi; the heavy Talos assets are copied from the rc draft), and opens a release-X.Y.Z PR. Merging it reuses the existing release-PR e2e + pull-requests-release.yaml finalize to cut the write-once stable tag. Squash is disallowed on the PR (the stable tag needs a real merge commit). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov DaniilandClaude Opus 4.8
|
9a2afa83a1 |
ci(release): make release tags write-once (drop force-retag)
Replace the force-retag operations in the release flow with write-once / compare-before-force guards, so published tags are never moved (the cache poisoning on api/apps/v1alpha1 via the Go module proxy, plus SBOM/scanner drift, all stem from moved tags). - tags.yaml: the api/apps/v1alpha1 submodule tag is now write-once (create if absent, no-op if unchanged, fail loudly if it would move). The release-X.Y.Z staging branch uses a compare-before-force guard (skip when unchanged; force+log only when the staging branch genuinely moves — it is a mutable staging ref, not an artifact). The build is now skipped when a release (draft OR published) already exists, so a stable tag prepared by promotion is never rebuilt here. - pull-requests-release.yaml: the stable tag is created at the PR merge commit with a write-once guard instead of `git tag -f`/`git push -f` — the merge commit did not exist before the PR opened, so force is impossible by construction. The maintenance branch is fast-forward-only (no `updateRef force:true`); a non-FF update warns and is left for a maintainer. - auto-release.yaml: removed. Stable patch tags were auto-bumped nightly via delete-recreate; stable now appears only through an explicit rc promotion. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov DaniilandClaude Opus 4.8
|
fd7fcbda4e |
refactor(operator): read platform version from env (decouple from build)
The operator's platform version was baked into the image at build time
(`-ldflags -X pkg/version.Version` from `git describe`), so a
`vX.Y.Z-rc.N` image literally carried the rc string. That blocks promoting
a release-candidate to stable by retag: the bits would still self-report
the rc version.
Resolve the version from the COZYSTACK_VERSION env var at runtime, falling
back to the build-time value when unset (local builds still work). The
installer chart threads `cozystackOperator.platformVersion` into the
operator Deployment as that env var, and `make manifests` stamps it into
the kubectl-apply release artifacts. The same image bits can now report any
release name, so promotion needs no rebuild.
The only behavioral reader is the telemetry metric
`cozy_cluster_info{cozystack_version=...}`; cozypkg/check-readiness keep
their own build-time Version and are unaffected.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
|
||
|
myasnikovdaniil
|
35025cf5f0 |
test(e2e): capture LINSTOR satellite ErrorReports in cozyreport (#3209)
## What this PR does Extends `hack/cozyreport.sh` linstor module to copy `/var/log/linstor-satellite/ErrorReport-*.log` off every `linstor-satellite` pod into `linstor/error-reports/<node>.tgz` in the cozyreport archive. ### Why The linstor-Satellite pod stdout log references its error reports only by ID: ``` [DeviceManager] ERROR LINSTOR/Satellite/... SYSTEM - Failed to create zfsvolume [Report number 6A4A394E-E4EA0-000000] ``` The report body (actual `zfs create -V …` stderr, DRBD adjust output, storage-driver stack trace, exit code) lives on the satellite pod's ephemeral filesystem at `/var/log/linstor-satellite/ErrorReport-*.log`. crust-gather only captures pod stdout, so the report body is lost when the e2e sandbox is torn down at end-of-job. Consequence for post-mortem of `ResourceExhausted` / `Not enough available nodes` CreateVolume retry loops (see e.g. PR #3044 CI history): cozyreport shows only the driver-level symptom. Root cause distinction (out-of-space vs dataset name conflict vs ZFS module error vs kernel signal) is not possible from the artefact alone, and the sandbox is unrecoverable by that point. ### Scope `hack/cozyreport.sh` only, +32 lines under the existing `linstor module` block. Guarded so: - a satellite with zero reports (healthy case) leaves nothing in the tree - a transient exec / tar failure never fails the surrounding cozyreport run - empty parent directory is pruned ### Verified on a live cluster - Label selector `app.kubernetes.io/component=linstor-satellite` matches all 3 satellite pods on the dev cluster used for verification. - Bundle for one active satellite comes out at ~60 KiB (dozens of historical reports tarred). No product-side change, no dependency on capture happening during a specific test phase; runs as part of the existing end-of-job cozyreport collection. ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Enhanced offline LINSTOR diagnostics collection by adding an error-report index and bundling controller and satellite error-report logs into downloadable archives. * Generates per-node satellite bundles using stable node naming for clearer organization of collected diagnostics. * **Bug Fixes** * Avoids creating empty or invalid error-report archives and automatically cleans up unused bundles to prevent clutter in the collected output. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
myasnikovdaniil
|
6b6fad03fc |
fix(operator): force source-watcher drift on stuck ArtifactGenerator status (#3182)
## Summary `cozystack-platform` HelmRelease times out at 15m on fresh install when a PackageSource's derived ArtifactGenerator stalls in `Ready: Unknown` even though its ExternalArtifacts are produced and consumed successfully. Root cause: fluxcd source-watcher patches `.status` and `.status.conditions` as separate apiserver requests via `patch.Helper`, so an etcd timeout under first-install load can leave `Inventory` + `ObservedSourcesDigest` persisted while the Ready-condition write is lost. Upstream tracker: [fluxcd/pkg#934 — `patch.Helper` creates race conditions in downstream controllers](https://github.com/fluxcd/pkg/issues/934). The bug is present in the currently-deployed v2.1.0 (pinned at `internal/fluxinstall/manifests/fluxcd.yaml:8230`), and a source-watcher version bump alone does not fix it because the race lives in the shared `fluxcd/pkg` runtime. ## What this PR does `PackageSourceReconciler` now detects the stuck ArtifactGenerator signature (Inventory populated, ObservedSourcesDigest set, Ready=Unknown on the current generation, LastTransitionTime older than a 30s grace period) and forces source-watcher onto its drifted-branch reconcile by writing **both** signals it keys on: 1. **`Ready=False` on the AG's status subresource.** source-watcher's `detectDrift` (v2.1.0 `artifactgenerator_drift.go:52`) treats `IsFalse(Ready)` as `NotReady` drift, promoting the reconcile past the no-drift early-return at `artifactgenerator_controller.go:164`. 2. **`reconcile.fluxcd.io/requestedAt` bump on the AG's metadata.** source-watcher's ArtifactGenerator watch is registered with `predicate.Or(GenerationChangedPredicate, ReconcileRequestedPredicate)` — the annotation is what enqueues the reconcile in the first place. Without it a status-only change never fires the watch and the AG would sit until the periodic self-requeue (hardcoded to 1h in `GetRequeueAfter`), well outside the 15m HR install timeout. Both signals are load-bearing — either alone is a no-op. The status write triggers the drifted branch inside source-watcher's reconcile; the annotation write triggers the reconcile that inspects that drift. On successful rebuild source-watcher writes `Ready=True` at `controller.go:254`; on a genuine rebuild failure it writes `Ready=False` with a real reason. Either way an honest condition from upstream replaces our synthetic False, and the `Owns(&ArtifactGenerator{})` watch fires this reconciler to copy the resolved condition onto the PackageSource. Bounded-retry harness sizes the total budget at ~11.5m (`30s + 60s + 2m + 4m + 4m`) safely inside the 15m HR install timeout; after `maxRecoveryAttempts` fruitless force-drift attempts the driver surfaces `PackageSource.Ready=False, reason=SourceWatcherStalled` so an operator can intervene, rather than lying about the state. Retry state (attempt count + last-force timestamp) lives on the AG as `cozystack.io/source-watcher-recovery-*` annotations so it survives operator restarts. Counter reset is via the canonical `clearRecoveryTracking` on the not-stuck path (Ready=True from source-watcher, or Ready=False with an upstream reason), NOT keyed on `LastTransitionTime` — source-watcher's Progressing write during rebuild always advances that timestamp, and a heuristic reset there would make `SourceWatcherStalled` unreachable under the very race this driver targets. ## Owns()-re-entrancy handling `Owns(&ArtifactGenerator{})` re-fires this reconciler on the force-drift's own writes (both the status patch and the metadata patch). A unified gate in `updateStatus` routes three overlapping signals into the same state machine: - **Our own marker reflecting back** — `Ready.Reason` carries `SourceWatcherRecoveryForced` (written only by `forceArtifactGeneratorDrift`). - **source-watcher's Progressing during rebuild** — Ready=Unknown on the current generation, tracking annotations already present. - **Classic first-time stuck detection** — the fluxcd/pkg#934 signature past the grace period. All three route to `maybeRecoverArtifactGenerator`, whose `decideRecovery` state machine is the single arbiter of wait vs re-force vs give-up. The alternative — a separate "wait for source-watcher" helper — was tried and rejected: if source-watcher never responds (missing predicate, network partition, dead pod), a wait-only path tight-loops without ever advancing to SourceWatcherStalled. The unified gate lets the state machine's own backoff schedule bound both the wait and the eventual give-up. ## Contract `PackageSource.status.conditions[Ready]` contract is **unchanged**: - `True` still means "source-watcher processed the current revision" (copied through from AG on the fast path). - `False` from a genuine upstream failure still passes through unchanged. - `False, reason=SourceWatcherStalled` is new — surfaces after `maxRecoveryAttempts` fruitless force-drift attempts, an honest operator-actionable signal rather than a false-positive True. - During force-drift and backoff windows, `Ready=Unknown, reason=AwaitingSourceWatcherRecovery` describes what the operator is waiting on. - The synthetic `SourceWatcherRecoveryForced` marker is an operator-internal signal on the AG only; it never leaks to the PackageSource. ## Retirement Once `fluxcd/pkg#934` lands and source-watcher consumes a `patch.Helper` that either serialises or transactionally combines the `.status` / `.status.conditions` writes, this whole recovery driver can be deleted and `updateStatus` can copy the AG's Ready condition through unconditionally. A `TODO(remove once fluxcd/pkg#934 lands and is rolled out)` marker on the driver points at the upstream issue. ## HA note The recovery driver stores its state as annotations on the ArtifactGenerator, so it races on write when two cozystack-operator replicas both see the same stuck target. Deploy the operator with `--leader-elect=true` (or `replicas: 1`, which is the shipped default in `packages/core/installer/templates/cozystack-operator.yaml`). ## Testing **Unit tests** cover: stuck-detection predicate with grace-period cases, bounded-retry state machine (Force / Wait / GiveUp branches), backoff schedule, tracking-annotation parser hardening, mutation contract on the two-subresource force-drift shape (success, status-patch failure rollback, metadata-patch failure with Ready=False persisted and annotations rolled back, rollback-restores-prior-bump), no-op clear-tracking, Reconcile early-return on DeletionTimestamp, follow-up scheduling on Unknown-within-grace to avoid the 10h informer resync dormancy, Owns()-re-entrancy for both our own marker and Progressing during recovery, canonical reset on Ready=True, and the source-watcher-unresponsive path (stale own-marker triggers retry through Force, exhaustion surfaces as SourceWatcherStalled). All pass with `-race`; `go vet` and `golangci-lint` clean. **E2E on dev3** — pending on the current tip. An earlier e2e ran on `97b054bbb`, which observed a stuck AG recovering after our force-drift but also had a dormancy bug (fixed since) and the requestedAt bump missing (fixed since). Both of the current shape's load-bearing mechanisms — requestedAt trigger + status-condition drift — need verification against a live source-watcher on a fresh e2e run before merge. |
||
|
Andrei Kvapil
|
59f035ff53 |
fix(kubevirt-instancetypes): restore persistent EFI/TPM state
Reverts #3006. Dropping persistent EFI/TPM was motivated by a live-migration concern that rested on an outdated assumption — that an RWO Filesystem backend-storage PVC pins the VM to its node. KubeVirt has migrated RWO-Filesystem backend storage since v1.4 (kubevirt/kubevirt#12629): on migration it creates a fresh target state PVC and copies the small state blob, so persistent-EFI/TPM VMs on the default replicated storage live-migrate fine (verified: the VM reports LiveMigratable=True and the copy-on-target PVC is created on migration). Restore upstream's persistent firmware state for the windows.* preferences. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io> |
||
|
myasnikovdaniil
|
e27c586a5d |
test(e2e): wait for LINSTOR pool free capacity between tenant Kubernetes tests (#3210)
## What this PR does Adds a companion `cozy_wait_linstor_pool_free` helper to `hack/e2e-apps/run-kubernetes.sh` and calls it from `cozy_cleanup` right after `cozy_wait_tenant_drained`. The helper polls LINSTOR for the smallest ZFS pool `FreeCapacity` across every satellite and blocks until it reaches a per-satellite threshold (default 90 GiB), bounded to 300 s with a loud warning on timeout. ### Root cause (proven) `kubernetes-previous.bats` intermittently failed with `zfs create -V 20976048KB data/pvc-... returned with exitcode 1: cannot create '...': out of space` on a satellite that measured **132 GiB free 35 minutes later** — so this is not an absolute capacity issue but a timing race. Root cause is captured in the cozyreport artefact of run 28751310913 (PR #3044), satellite ErrorReport `6A4AADFD-349B2-000000`. The previous tenant Kubernetes test's `cozy_wait_tenant_drained` gate returned as soon as its PVCs disappeared from the API, but the satellite-side ZFS `zvol destroy` for the just-deleted replicated (autoPlace=3, DRBD) 20 GiB worker disks lagged tens of seconds behind that signal as DRBD adjusted, unref counts drained, and ZFS batch-destroyed the datasets. The next tenant test's `CreateVolume` landed inside that window, LINSTOR-CSI retried autoplace against the still-being-torn-down previous placement (`Do not place with resource: pvc-<same-uid>` implicit anti-collocation, no CSI bug), and worker Machine bringup stretched past the MHC `nodeStartupTimeout` budget with no cluster-side fault. The proximate observation (`zfs create out of space`) surfaced only because of PR #3209 which teaches `hack/cozyreport.sh` to bundle satellite ErrorReports into the artefact. Without that, the root cause was ambiguous between several plausible LINSTOR internal races and I nearly opened a wrong upstream issue. ### Threshold rationale Two 20 GiB replicated worker targets = 60 GiB per satellite (autoPlace=3 places one replica per node) plus two 21 GiB CDI scratch PVCs (worst case both landing on the same node via the `local` storageClass) = ~82 GiB per-satellite peak footprint. 90 GiB default threshold covers that with margin. ### Scope `hack/e2e-apps/run-kubernetes.sh` only. E2E-scope, no product or chart change. Threshold and timeout are function args with sensible defaults; caller can override. ### Verified `jq` expression against a live LINSTOR 1.33.2 cluster: ``` $ kubectl -n cozy-linstor exec deploy/linstor-controller -- sh -c 'linstor --machine-readable sp l 2>/dev/null | jq -r "first | .[] | select((.provider_kind | test(\"^ZFS\")) and .free_capacity != null) | \"\(.free_capacity):\(.node_name)\"" | sort -n' 3088864:talos-5jt-iq3 42483036:talos-te2-p24 197991872:talos-6cn-cgg ``` Matches `linstor sp l` output exactly (2.95 GiB / 40.51 GiB / 188.82 GiB). ZFS-only filter excludes DISKLESS `Long.MAX_VALUE` sentinel; `and .free_capacity != null` guard prevents OFFLINE satellite pool objects (whose `free_capacity` is optional per REST schema) from ranking as `null` in the sort and hanging the loop. Contract mirrors `cozy_wait_tenant_drained`: bounded deadline, `|| true` at the call site, loud timeout warning that includes the smallest observed pool and its satellite. Timeout can never wedge the suite worse than the status quo. ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved cleanup reliability by waiting for storage capacity to be released before finishing teardown, reducing cases where resources appear deleted but storage is still unavailable. * Made teardown more resilient with a configurable timeout to avoid premature completion. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Ivan Okhotnikov
|
287dbca600 |
test(e2e): wait for LINSTOR pool free capacity between tenant Kubernetes tests
kubernetes-previous.bats intermittently fails with `zfs create -V ... returned with exitcode 1: cannot create '...': out of space` on a satellite even though the LINSTOR pool measured 30+ minutes later shows 100+ GiB free per satellite. Root cause proven from a satellite ErrorReport captured in the cozyreport artefact (PR #3044 run 28751310913, report id 6A4AADFD-349B2-000000): the previous tenant Kubernetes test's cozy_wait_tenant_drained gate returned as soon as its PVCs disappeared from the API, but the satellite-side ZFS `zvol destroy` for the just-deleted replicated (autoPlace=3, DRBD) 20 GiB worker disks lagged tens of seconds behind that signal as DRBD adjusted, unref counts drained, and ZFS batch-destroyed the datasets. The next tenant test's CreateVolume then landed inside that window against a pool that had not yet reclaimed the space, LINSTOR-CSI retried autoplace against its own still-being-torn-down previous placement, and worker Machine bringup stretched past the MHC nodeStartupTimeout budget with no cluster-side fault. Add a companion `cozy_wait_linstor_pool_free` helper that polls the LINSTOR controller for the smallest FreeCapacity across every ZFS satellite pool and blocks until it reaches a per-satellite threshold (default 90 GiB — covers the ~82 GiB worst-case footprint of two 20 GiB replicated worker targets across all three satellites plus two 21 GiB CDI scratch PVCs landing on the same node via the local storageClass). Called from cozy_cleanup immediately after cozy_wait_tenant_drained. Bounded and best-effort: 300 s timeout, `|| true` at the call site, warning printed with the observed pool state if the threshold is not reached in budget so the next test does not misdiagnose the pressure. The threshold is a floor for the concrete failure mode observed in the E2E suite; the surrounding kubernetes-latest -> kubernetes-previous handoff is the only run in the current suite that immediately follows a heavy tenant teardown with another tenant provisioning, so this gate is the natural place for the check. E2E-scope only, no product or chart change. The proximate observation (`zfs create out of space`) surfaced because of PR #3209 which taught hack/cozyreport.sh to bundle satellite ErrorReports; without that artefact the root cause was ambiguous between multiple plausible LINSTOR internal races. Ships alongside that PR. Signed-off-by: Ivan Okhotnikov <xorokhotnikov@gmail.com> |
||
|
Ivan Okhotnikov
|
d5098d9430 |
test(e2e): capture LINSTOR satellite ErrorReports in cozyreport
The linstor-Satellite pod stdout log references its error reports
only by ID:
[DeviceManager] ERROR LINSTOR/Satellite/... SYSTEM - Failed to
create zfsvolume [Report number 6A4A394E-E4EA0-000000]
The body of that report (which carries the actual `zfs create -V …`
stderr, the DRBD adjust output, the storage-driver stack trace, and
the exit code) lives at `/var/log/linstor-satellite/ErrorReport-*.log`
on the satellite pod. crust-gather only captures pod stdout, so the
report body stays on the pod's ephemeral filesystem and is lost when
the e2e sandbox is torn down at end-of-job.
Consequence for post-mortem: a run that failed on a
`ResourceExhausted` / `Not enough available nodes` CreateVolume retry
loop shows only the driver-level symptom in cozyreport. Root cause
attribution (out-of-space, dataset name conflict, ZFS module error,
kernel signal) is not distinguishable without the report body, and
without the sandbox is unrecoverable.
Extend the linstor module in `hack/cozyreport.sh` to tar the
`ErrorReport-*.log` set off every `linstor-satellite` pod into
`linstor/error-reports/<node>.tgz`. Guarded so a satellite with zero
reports (the healthy case) leaves nothing in the tree, and a
transient exec / tar failure never fails the surrounding cozyreport
run.
Verified against a live dev cluster: label selector
`app.kubernetes.io/component=linstor-satellite` matches all three
satellite pods, and the tar bundle for one active satellite comes
out at ~60 KiB. No product-side change.
Signed-off-by: Ivan Okhotnikov <xorokhotnikov@gmail.com>
|
||
|
IvanHunters
|
b6ac249faa |
test(e2e): bump sandbox tenant storage quota 100Gi -> 200Gi (#3206)
## What this PR does Raises the E2E sandbox tenant's `resourceQuotas.storage` from 100Gi to 200Gi so back-to-back tenant Kubernetes tests (`kubernetes-latest.bats` -> `kubernetes-previous.bats`) do not collide with tenant quota during the second cluster's CDI import. The 100Gi default in `packages/apps/tenant` is unchanged — this touches only the E2E test tenant CR in `hack/e2e-install-cozystack.bats`. ### Root cause Both kubernetes-*.bats tests provision a 2-worker tenant control plane whose worker root disks are populated by CDI. CDI's second-phase importer allocates a scratch PVC of the same 20Gi as the target disk before the qcow2 unpack, so per-tenant peak footprint is: ``` 2 × 20Gi target + 2 × 21Gi scratch = ~82Gi during import ``` collapsing to 40Gi at rest. `kubernetes-latest`'s `cozy_cleanup` deletes the Kubernetes CR and blocks in `cozy_wait_tenant_drained` (300s) waiting for KubeVirt VMs / VMIs / namespace PVCs to disappear. The API-level PVC delete returns quickly, but the LINSTOR CSI detach + DRBD resource release on the underlying nodes lags past that budget. During the overlap window the deleted PVCs still count against `ResourceQuota.status.used.requests.storage` because the storage provisioner has not finished releasing the volume. `kubernetes-previous` then starts its own CDI import with 40Gi of stale `kubernetes-latest` quota still charged, and CDI's scratch PVC create for the second worker (used 80Gi + scratch 21Gi = 101Gi) is rejected by the tenant-quota admission check at 100Gi. The second worker's DataVolume stalls in `ImportInProgress`, the tenant node never joins, and the 12-minute node-join deadline in `run-kubernetes.sh` fails `kubernetes-previous.bats`. ### Evidence `cozyreport.tgz` from the failing E2E run of PR #3044 (run 28730757213): ``` cdi-deployment log: scratch PVC API create errored: persistentvolumeclaims "…-qxl8r-disk-system-scratch" is forbidden: exceeded quota: tenant-quota, requested: requests.storage=22763536384 (21Gi), used: requests.storage=85899345920 (80Gi), limited: requests.storage=100Gi ``` The tenant-quota `status.used.requests.storage` in the snapshot decays back to 40Gi by end-of-run — confirming the leak is a transient window during the second test's start-up, not a permanent leak. ### Relation to prior work Overlaps in scope with #2947 (`ci(e2e): bump tenant storage quota 100Gi -> 200Gi`), which bundles this same bump alongside three other e2e stabilization changes (crust-gather tag, harbor BucketClaim timeout, cilium IP-conflict healer). #2947 has been stale since 2026-06-17 with the maintainer suggesting it be closed. This PR isolates just the quota-bump so it can land on its own scope; the other three changes are orthogonal and can ship separately if #2947 stays dormant. ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Updated end-to-end coverage for tenant creation with isolated mode to reflect higher storage quota expectations. * Improved quota validation in test scenarios to better match supported storage behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Ivan Okhotnikov
|
3ab9f95625 |
test(e2e): bump sandbox tenant storage quota to 200Gi
The E2E sandbox tenant (tenant-test) provisions back-to-back tenant
Kubernetes control planes via kubernetes-latest.bats and
kubernetes-previous.bats. Each control plane brings up two worker VMs
whose 20Gi root disks are populated by CDI, and CDI's second-phase
importer allocates a scratch PVC of the same size before the qcow2
unpack. Peak per-tenant footprint is therefore ~82Gi during import
(2 × 20Gi target + 2 × 21Gi scratch), collapsing to 40Gi at rest.
kubernetes-latest teardown deletes the tenant Kubernetes CR and waits
via cozy_wait_tenant_drained for the KubeVirt VMs, VMIs and namespace
PVCs to disappear. The API-level PVC delete returns quickly, but the
LINSTOR CSI detach + DRBD resource release on the underlying nodes
lags past the 300s drain budget. During that window, the deleted
PVCs still count against ResourceQuota's requests.storage because
the storage-provisioner has not finished releasing the volume — so
kubernetes-previous starts its own CDI import with 40Gi of stale
kubernetes-latest quota still charged, and CDI's scratch PVC create
for the second worker (used=80Gi + scratch=21Gi = 101Gi) is
rejected by the tenant-quota admission check at 100Gi. The second
worker's DataVolume never leaves ImportInProgress, the tenant node
never joins, and the 12-minute node-join deadline in
run-kubernetes.sh trips kubernetes-previous.bats.
Raise the sandbox tenant-quota storage ceiling to 200Gi so the
overlap between one test's slow storage cleanup and the next test's
CDI import fits inside the quota. The failing-run diagnostic is
cozyreport artifact from run 28730757213:
cdi-deployment log:
scratch PVC API create errored: … forbidden: exceeded quota:
tenant-quota, requested: requests.storage=22763536384,
used: requests.storage=85899345920,
limited: requests.storage=100Gi
This is a test-infrastructure fix; the 100Gi default in the tenant
chart itself is unchanged. The assertion below the CR update is
lifted from 100Gi to 200Gi in lockstep.
Signed-off-by: Ivan Okhotnikov <xorokhotnikov@gmail.com>
|
||
|
Ivan Okhotnikov
|
0d98e27981 |
fix(operator): hold final backoff window before give-up, note upstream version skew
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> |
||
|
Ivan Okhotnikov
|
37421d2fec |
docs(operator): honest JSON-merge-patch tradeoff, clarify asymmetric OG check, warn on partial force-drift
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> |
||
|
Ivan Okhotnikov
|
596427deb3 |
docs(operator): scrub stale awaitSourceWatcherResponse references from comments and test names
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> |
||
|
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 --> |
||
|
Aleksei Sviridkin
|
9ab046eb9e |
fix(seaweedfs): soften master anti-affinity for small clusters
The vendored subchart pins the master to a hard hostname anti-affinity (requiredDuringSchedulingIgnoredDuringExecution). With the master StatefulSet at three replicas, the surplus Pods stay Pending forever on clusters with fewer than three nodes, so `helm install --wait` never returns and the S3 stack never comes up. Soften the term to a strong preference (weight 100) so the masters still spread across distinct nodes when the cluster has them, while single- and dual-node clusters can bring the master up. Mirrors the posture already used for the s3 component. Selector and topologyKey are unchanged; only required -> preferred. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
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
|
c4d023d979 |
docs(vm-instance): document attaching a VM to an external VLAN
Add a guide for bridging a vm-instance VM onto a physically-routed VLAN via a Linux bridge and a bridge-type NetworkAttachmentDefinition. The overlay-only default does not cover the case where a VM must share a broadcast domain with external hardware and take an address from that VLAN's subnet. Explain why macvlan is incompatible with KubeVirt bridge binding (macvlan demultiplexes ingress by the child MAC while bridge binding puts the guest MAC on the wire, so replies are dropped) and why a Linux bridge works, then walk through the host bridge, the NAD, and assigning a static guest address through cloud-init. Cover the dual-homed NIC layout, the absence of networkData, and the post-recreate ARP staleness. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
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 -->
|
