mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-03 18:47:04 +09:00
bisect/nightly-mid
661
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Aleksei Sviridkin
|
6ecb36ffc7 |
feat(cozystack-controller): extract application CA into a key-free tenant Secret (#3407)
## What this PR does Adds an engine-agnostic controller that publishes a key-free `<release>.tenant-ca` Secret (only `ca.crt`) for a managed application, so a tenant can verify the application's TLS without ever seeing a private key. The source is declared, not guessed: a chart renders a namespaced `TenantProjection` sentinel (group `internal.cozystack.io`) naming the CA Secret to lift; the controller watches the sentinel, extracts `ca.crt`, and writes the projection owner-referenced to the sentinel, so garbage collection is native. The projection carries the `internal.cozystack.io/tenant-ca` label the lineage webhook turns into tenant visibility. Postgres is the first consumer. The trust boundary is RBAC: no tenant role grants any verb on `internal.cozystack.io`, so a tenant cannot forge a sentinel. A companion `ValidatingAdmissionPolicy` that pins the writer to helm-controller ships separately as defence in depth. This is the controller-and-CRD slice, split out from #3299 for focused review. It is the only piece required for the feature to work; the render-time helper, the writer policy, the capability-gate, and the release-prefix validation land as their own PRs. ### Downstream repositories Walked the trigger map against the diff. This adds an internal CRD and controller and a sentinel to the Postgres chart; no app is added or renamed and no `values.schema.json` changes, so the typed provider and Ansible are unaffected. The one to confirm is the Postgres README edit — if the website mirrors it, it needs a refresh. - [ ] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: ### Release note ```release-note feat(cozystack-controller): publish a key-free per-application CA trust anchor (`<release>.tenant-ca`) that tenants read to verify managed-application TLS, declared through a new `TenantProjection` CRD. Postgres is the first consumer. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added tenant-facing CA certificate projections for PostgreSQL deployments. * Tenant secrets now provide only the CA certificate, excluding private keys. * Added support for configuring and monitoring tenant CA projections. * **Documentation** * Updated PostgreSQL TLS instructions to retrieve certificates through tenant secrets. * **Bug Fixes** * Improved protection against invalid certificates, secret collisions, and accidental exposure of private key material. * Added validation to ensure projected certificates remain synchronized with their sources. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Andrey Kolkov
|
8aeedde088 |
feat(backups): support PostgreSQL point-in-time recovery (PITR) (#3383)
## What this PR does Makes PostgreSQL point-in-time recovery (PITR) a genuinely supported, tested capability for the new `backups.cozystack.io` API. Fixes #2774. Was **stacked on #3313** (`fix/3300-cnpg-barman-plugin`) for the `recoveryTime` → CNPG `bootstrap.recovery.recoveryTarget.targetTime` plumbing; #3313 has since merged and this branch is rebased on `main`. This PR adds the reliability + verification layer #2774 asked for. ### Changes - **Precise failure reason for an unreachable target.** The restore deadline (`spec.options.restoreTimeoutSeconds`, default 30m) stays the sole authority for "this restore is stuck" — the client waits the full window either way. What changes is the *diagnosis*: once the deadline has elapsed, if `recoveryTime` was set and the recovery Cluster never went healthy, the CNPG driver reads the `full-recovery` pod logs and — when several distinct attempts exited with PostgreSQL's `FATAL: recovery ended before configured recovery target was reached` — fails the RestoreJob with `RecoveryConverged=False` / `Ready=False` (reason `RecoveryTargetUnreachable`) and an actionable message, instead of a bare generic timeout. Timing is unchanged; only the reason/message differ. (For a faster rejection, set a shorter `restoreTimeoutSeconds`.) Design points: - **We deliberately do NOT trip early off the FATAL.** A valid near-now target emits that same FATAL transiently — repeatedly, for many attempts, on a slow archiver — before it converges, so any earlier trip would wrongly reject a recoverable restore (observed twice in CI). The FATAL is used only to *explain* a failure the deadline has already declared. - **Health wins unconditionally** — the deadline/classification check precedes the healthy check, but a recovery that converges within the window reaches success and is never classified as failed (a live dev7 restore did exactly this). - **Counting distinct FATAL-ing attempts** (not pod failures) tolerates a one-off archive-lag miss on a near-now target and ignores transient recovery-pod crashes (a node blip / brief API-server unreachability never emits that FATAL). - Adds the `pods/log` RBAC grant the log read needs (distinct subresource from `pods`); a `Forbidden` is logged loudly and surfaced in the failure message rather than silently disabling the classification. - **e2e** (`examples/backups/postgres/run-all.sh`, wired into the Chainsaw `postgres-2-backup-roundtrip` suite): - Step 45 — write `before`, capture server timestamp (µs precision), write `after`, restore to that timestamp, assert `before` present and `after` absent. - Step 46 — a `recoveryTime` an hour ahead must fail with reason `RecoveryTargetUnreachable` once the deadline elapses (asserts the *reason*, so it proves the classification fired, not just that a timeout occurred). - Chainsaw budget raised 25m → 40m to cover three sequential restores + the post-deadline classification. - **Docs + example**: uncommented `45-restorejob-pitr.yaml`; a `docs/operations/backup-classes.md` PITR section (recoverable window, discovering earliest/latest restorable time from the backup catalog since `firstRecoverabilityPoint` is unreliable under the plugin, archive-lag tolerance, idempotency under GitOps). ### Verification - `go build` / `go vet` clean; unit tests pass (`logIndicatesRecoveryTargetUnreachable`, `recoveryUnreachableFromLogs`, `recoveryPodsToInspect`, nil-Clientset guard, a reconcile-level success-path test asserting `Succeeded` + `RecoveryConverged=True`, and — added per review — `TestReconcileCNPGRestore_DeadlineClassifiesRecoveryTargetUnreachable`, a reconcile-level test driving the deadline branch through all three sub-paths: FATAL found → reason `RecoveryTargetUnreachable`, FATAL absent → generic timeout, `Forbidden` log read → generic timeout + missing-`pods/log`-RBAC note. It fails if the call-site reason is reverted to the generic one). - Reviewed via adversarial branch-review; fixes include a critical missing-RBAC dead-on-arrival, a classification false-positive on transient recovery-pod crashes, and a sub-second e2e flake. - **dev7**: backup + WAL archiving + recovery convergence verified live on the #3313 build. The load-bearing assumption of the classification — CNPG **accumulates** failed `full-recovery` pods rather than reusing one — was confirmed (observed 7 simultaneous failed recovery pods). A fully-green live step-46 run was blocked by intermittent COSI bucket-provisioning flakiness on dev7; the negative path is covered by the CI Chainsaw e2e. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff. The only user-facing surface is the new `docs/operations/backup-classes.md` PITR section; the website keeps a hand-mirrored copy per docs version. - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: PITR docs mirror warranted (not yet opened; same pending mirror as #3313's backup-classes changes) - [x] No other downstream repository is affected by this change ```release-note feat(backups): PostgreSQL RestoreJob now supports point-in-time recovery (PITR). Set spec.options.recoveryTime (RFC3339) to restore to an exact instant. A recoveryTime past the latest archived WAL still runs to the restore deadline (spec.options.restoreTimeoutSeconds, default 30m), but the failure is now reported with the precise reason RecoveryTargetUnreachable and an actionable message instead of a generic timeout; set a shorter restoreTimeoutSeconds for a faster rejection. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added PostgreSQL point-in-time recovery (PITR) support via RFC3339 `recoveryTime`. * Introduced a PITR RestoreJob example, plus helper variables and an unreachable-target scenario. * **Bug Fixes** * Improved restore deadline handling with deterministic, log-based fail-fast classification for unreachable recovery targets, including clearer conditions. * **Documentation** * Added detailed PITR operational documentation (recovery windows, failure behavior, and GitOps/idempotent behavior). * Expanded README and example comments for PITR verification and failure expectations. * **Tests** * Extended unit tests and end-to-end chainsaw workflow to cover PITR success and unreachable failures. * **Chores** * Updated RBAC to allow pod log reads; refined cleanup and run scripts for additional RestoreJob artifacts. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Myasnikov Daniil
|
6a41d65f16 |
test(e2e): avoid ripgrep dependency in diagnostics tests
Assisted-By: GPT-5 <noreply@openai.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
4e0cfd994d |
test(e2e): render COSI catch output as YAML
Assisted-By: GPT-5 <noreply@openai.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
ad1adbf785 |
test(e2e): fix diagnostic collector blind spots
Assisted-By: GPT-5 <noreply@openai.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
myasnikovdaniil
|
55195ded0c |
test(e2e): capture previous container logs and stop killing the snapshot (#3346)
## What this PR does Root-causing a recent E2E failure was nearly blocked because the diagnostics captured on failure had collected almost nothing. Two independent mechanisms were responsible, and both had been silently in place for a long time. **The failing container's previous logs were never captured.** The pod had crash-looped, so the decisive evidence lived in the first container instance, reachable only through `kubectl logs --previous`. Chainsaw's `podLogs` action has no `previous` field, so this could not be expressed per-suite; the conclusion had to be reconstructed from indirect evidence instead. A new capture runs from the global catch and from the BATS failure hook, selecting containers at runtime by `restartCount > 0` across both init and regular containers, so a healthy run stays quiet and future suites are covered without per-suite wiring. **The catch was structurally guaranteed to be killed.** A Chainsaw `script` op is bounded by `timeouts.exec`, which is 5m, while the catch's own inner budgets summed to roughly 960s. It reported `SCRIPT | ERROR | signal: killed` and produced nothing — and its second leg, the data-plane capture, had therefore never run at all. The kill signature was itself misleading: `timeout` exits 124 and cannot produce that message, which comes from Go's `exec.Cmd` killing the process group. Each catch op now carries an explicit timeout above the budgets it contains, so the inner one fires and leaves a partial, usable artifact instead of losing everything. **The snapshot was truncated at 60 seconds.** `crust-gather --duration` defaults to 60s and was never passed, so every snapshot ever taken was cut short, with nothing in the artifact saying so. The API discovery it performs first is not covered by that budget at all, which is where the outer wall-clock was going on a cluster with a degraded aggregated APIService — precisely the state a failed run is in. Both budgets are now set explicitly, and the collector's output goes to a log inside the snapshot with a completeness verdict, so "is this snapshot complete?" is answerable from the artifact. Failure paths are reported rather than swallowed: an unreachable API says so instead of implying nothing had restarted, a truncated capture is kept and marked, and every dropped item is logged, since silent truncation reads as full coverage. ### What is not verified here The mechanism claims are read from the pinned versions of Chainsaw and crust-gather. Everything cluster-facing is not verified by this PR: the go-template's output shape against a live API server, `--previous` returning the first instance for a real crash-looper, and whether the new collection budgets suffice on a degraded cluster. The bats suite covers the selection helpers only, and those cases are mutation-tested. ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Diagnostics** * Improved end-to-end failure evidence by capturing “previous instance” logs for crash-looping pods (including restarted init containers) and prioritizing relevant namespaces. * Added time-bounded diagnostics collection with explicit outer timeouts; collector output is saved to per-snapshot log files. * Classifies collector outcomes as complete, timeout-truncated, or failed, while preserving the original test failure signal. * **Documentation** * Expanded E2E failure-diagnostics guidance with new timeout and evidence-capture conventions. * **Tests** * Added unit-style tests for previous-log selection, prioritization, row limiting, and deterministic log artifact naming. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
myasnikovdaniil
|
d0c0f399f8 |
fix(ci): cover every image-ref storage shape in promote, retag and mirror (#3404)
## The bug Promotion rewrites the rc version substring to the stable version across the vendored image references. That rewrite lived inline in `promote-rc.yaml` and globbed the depth-2 package `values.yaml` plus `packages/apps/kubernetes/images/*.tag` **only**, on the premise that the kubernetes app was the only one whose `.tag` files carry the cozystack version. Nine other `.tag` files do, and `system/multus` seds its reference straight into a vendored upstream daemonset manifest. #3397 (promote `v1.6.0-rc.4` → `v1.6.0`) is therefore staged with **33 of 55 first-party references still reading `v1.6.0-rc.4`**. `hack/promote-retag.sh` and `hack/nightly-mirror.sh` had the same blind spot from the other direction — both scanned the depth-2 `values.yaml` alone, so those images were never retagged to the stable version (**30 references selected before, 42 after**) and never mirrored to the public registry for a nightly. A retag miss leaves an image short of a tag, since the digest still resolves inside one registry. A mirror miss is worse: the host rewrite walks the same file list, so a reference that is neither mirrored nor rewritten leaves the published tree pointing at the private build registry. `v1.6.0` is the first release to go through promotion — every earlier release was a full rebuild from the tag, which stamped every file uniformly. The glob has always been wrong; the old path just made it unobservable. ## Why it could not be caught before The rewrite was workflow-inline, so there was nothing to unit test. The only way to observe the miss was to cut a release. It is a script now — `hack/promote-rewrite-tags.sh` — and `hack/promote-rewrite-tags_test.bats` round-trips it against the real tree under `make unit-tests`, with no cluster, registry or release involved. ## The fix The three call sites each carried their own idea of where a reference can live, which is how they drifted apart. That knowledge moves into `hack/lib/image-refs.sh` as the single enumeration — `image_ref_files()` for the files, `collect_image_refs()` for the references inside them — and all three source it, so a new storage location is declared once and reaches every consumer. The rewrite also asserts its own postcondition: it scans **wider** than it rewrites and fails the promotion when a reference survives in a location the enumeration does not know about, rather than shipping it quietly. The wide scan is shape-filtered to image-reference position, because `X.Y.Z-rc.N` is an ordinary version string that other things legitimately contain — `kubevirt-csi-driver/go.sum` pins `github.com/golang/protobuf v1.4.0-rc.2`, and `v1.4.0-rc.2` is a cozystack rc that was actually cut. Narrowing it to `${RC}@sha256:` instead would have been wrong: `system/cilium/values.yaml` keeps `tag:` and `digest:` under separate keys. `docs/agents/image-refs.md` records the contract — three tag classes, three storage shapes, the invariants, and which consumer does what. ## Not a bug: kamaji `cluster-api-control-plane-provider-kamaji` stays at `v0.19.0-cozystack.0` across the promotion. It is a first-party rebuild of an upstream component, versioned by that component rather than by the cozystack version line, so no rewrite is owed. A test pins this so a future fix here cannot over-reach. ## Documented known gaps, deliberately not fixed here Both are pre-existing and in the **host** dimension; the version dimension this PR owns is complete, because the version always lives in a `tag` key regardless of where the host sits. - **The nightly host rewrite cannot reach a split host.** It is a literal `<src-registry>/` substring replace, so `keycloak-operator` (host in a sibling `registry:` key) and `kubeovn` (`global.registry.address`, no trailing slash) are mirrored but keep pointing at the build registry in the published tree. Fixing it needs structure-aware rewriting. - **A reference inside a gzip is invisible.** `capi-providers-cpprovider` ships `files/components.gz`, and the promote postcondition cannot catch this class at all since `grep -rIl` skips binaries. Nothing is owed today — kamaji is component-versioned and the image is still mirrored and retagged via its `.tag` — so the residual is that a nightly's kamaji ConfigMap keeps the build-registry host. ## Testing - 17 tests in the new suite; `make bats-unit-tests` green (312 tests, exit 0); `shellcheck -x` clean; `zizmor` clean on the workflow. - Verified against four real rc versions (`1.4.0-rc.2`, `1.4.0-rc.4`, `1.5.0-rc.2`, `1.6.0-rc.4`) in both directions — no false positives, no false negatives. - Every test is mutation-proven. Reverting the enumeration to the old glob, deleting any of the five YAML shapes, corrupting emitted digests, reverting extras to parse-only, dropping the unreadable-file guard, broadening shape 4 beyond `global.images`, or planting an unenumerated first-party reference in any storage shape each fail a named test. ## Reviewer note, out of scope `platform-migrations` is pinned at two different digests — `core/platform/values.yaml` at `v1.5.0` and `backupstrategy-controller/values.yaml` at `v1.4.0-rc.2` (an unowned pin; no Makefile writes that key, tracked in #3143). A `promote-retag.sh` dry-run emits **two copies to the same destination tag**, so the write-once guard will fail the promotion. This is independent of this PR and needs a decision about the backup-client image version, so it is left alone here — but it blocks the 1.6.0 promote and wants fixing before the re-cut. ## Release note ```release-note Promotion, retagging and nightly mirroring now cover every image reference the build stamps, including those stored in `images/*.tag` files and in vendored manifests. Previously a promoted release could ship images still tagged with the release-candidate version, and those images received no stable tag in the registry. ``` Fixes the promote half of #3143. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Image promotion and nightly mirroring now detect digest-pinned image references across values files, standalone tag files, and supported templates. - Stable release preparation supports RC→stable tag rewriting via a dedicated promotion script, with broader verification to ensure no RC references remain. - **Bug Fixes** - Improved completeness of ref discovery and scanning, including previously missed template/tag locations. - **Documentation** - Added and linked guidance documenting image-reference invariants and how tooling enumerates supported storage shapes. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
myasnikovdaniil
|
bbfe9f9a9d |
fix(backupstrategy-controller): give chBackupClientImage a producer (#3405)
## This is a workaround `backupstrategy-controller` should not be running the platform **migrations** image. This PR does not fix that — it makes the existing reuse *safe* so the v1.6.0 promote can proceed, and documents in the tree exactly what is wrong and what should replace it. Treat it as a stopgap with a TODO, not as an endorsement of the design. ## What is actually wrong The Altinity strategy Pod drives clickhouse-backup's HTTP API. Its script uses exactly four binaries — `curl`, `jq`, `sleep`, `date`. For that it currently pulls `platform-migrations`: | | | | --- | --- | | Size | **349 MB** (a dedicated `alpine + curl + jq + ca-certificates` measures **23 MB**) | | Also contains | `kubectl`, `helm`, `git`, `cozyhr`, the `etcd-migrate` binary, every migration script, vendored etcd CRDs | | `ENTRYPOINT` | `run-migrations.sh` | Two consequences worth naming. A tenant-adjacent backup Pod carries cluster tooling it has no use for. And because the image's own entrypoint is the migration runner, the `command:` override in the strategy template is load-bearing — drop that one line and the Pod runs platform migrations with `kubectl` and `helm` already on the PATH. It also creates the coupling that caused the bug this PR fixes: bumping `ETCD_OPERATOR_VERSION` or editing any migration script re-digests the image, forcing the backup client's pin to move for reasons that have nothing to do with backups. ## The release blocker `platform-migrations` was pinned at two different digests in the committed tree: | Location | Pin | Producer | | --- | --- | --- | | `packages/core/platform/values.yaml` `.migrations.image` | `v1.5.0@sha256:8bf61f17…` | `packages/core/platform/Makefile` | | `backupstrategy-controller/values.yaml` `.chBackupClientImage` | `v1.4.0-rc.2@sha256:17390197…` | **none** | Promotion retags **by digest**: `hack/promote-retag.sh` copies every collected `<repo>@<digest>` to `<repo>:<stable-version>`. Two digests under one repository produce two copies competing for the same destination tag — the first wins, the second hits the write-once guard, and the promotion fails. A dry run against `main` emits exactly that pair: ``` ▸ ghcr.io/cozystack/cozystack/platform-migrations sha256:17390197… → docker://ghcr.io/cozystack/cozystack/platform-migrations:v9.9.9 ▸ ghcr.io/cozystack/cozystack/platform-migrations sha256:8bf61f17… → docker://ghcr.io/cozystack/cozystack/platform-migrations:v9.9.9 ``` `v1.6.0` is the first release cut through promotion rather than a full rebuild. Every earlier release ran `make build`, which restamped both copies as a side effect and hid the missing producer. ## What this PR does `packages/core/platform/Makefile` now stamps both keys with the same ref, so they move in lockstep by construction rather than by instruction, and the committed value is aligned to the digest `core/platform` already carried. Moving the runner forward two minors is safe: it is used only as a curl + jq container, and the image built at `v1.5.0` carries them (`git show v1.5.0:…/migrations/Dockerfile` → `apk add … jq ca-certificates bash curl`). ## What should replace it A dedicated `ch-backup-client` image built by **this package's own Makefile** — measured at 23 MB, 15× smaller, with none of the cluster tooling and none of the entrypoint hazard. That also deletes the cross-package stamp this PR adds, because the pin would be produced by the package that owns it. The objection that has kept this reuse in place — "a second tag release CI would have to re-digest each cut" — does not survive contact with the actual defect. Every other first-party image already pays that cost, automatically. What broke here was not an extra tag; it was **a pin with no producer**, which a package-owned image cannot reproduce. I am deliberately not doing that here: it means a new image, a new build target and a new digest introduced during a release cut. It belongs in a follow-up. ## The guard `hack/image-pin-consistency.bats` asserts the general invariant — no repository pinned at more than one digest — driven through the real promotion selector so it tracks whatever set promotion actually acts on, rather than a reimplementation that could drift from it. Plus a specific lockstep assertion so a recurrence names its cause. This check could not have been added before now: it fails on the very duplicate this PR removes. ## Testing - `hack/image-pin-consistency.bats` — both tests pass; verified they fail when the drifted pin is restored. - `make bats-unit-tests` green (294 tests, exit 0). - `make -C packages/system/backupstrategy-controller test` — 11 tests, 3 suites, green. - `helm template … --set backupStorage.bucketNameOverride=test-bucket` renders the strategy Pod with the new ref. - The Makefile's `yq` step was simulated directly to confirm the relative path and key path resolve and that comments survive the in-place edit. - Merged locally with #3404 onto `main` and re-run: the expanded ref collector in that PR surfaces no additional duplicate pins (314 tests, exit 0). ## Relationship to other work Relates to #3143 — this is one instance of its unowned-pin class, the one its discussion flagged as having no producer. The four never-built packages there remain untouched. Independent of #3404 (promote/retag/mirror storage-shape coverage); the two touch no common files and can merge in either order. Both are needed before the `v1.6.0` promote is re-cut. ## Release note ```release-note Fixed a release-blocking duplicate pin: the ClickHouse backup client image is now stamped from the same source as the platform migrations image it reuses, so both carry one digest. Previously they could drift, which made promotion attempt to retag two digests to the same stable tag and fail. Reusing the migrations image here remains a documented workaround pending a dedicated backup-client image. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Ensured the platform migrations image digest is pinned consistently between the platform and backup strategy components. * Updated the backup strategy component to reference the new pinned `platform-migrations` digest. * **Tests** * Added promotion/retag consistency checks to fail on conflicting source digests for the same stable tag. * Added drift detection to ensure `platform-migrations` image references match across consumer configuration files. * **Documentation** * Expanded in-chart comments and values documentation clarifying the coupling and workaround behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Andrey KolkovandClaude Opus 4.8
|
4605bbf3db |
test(mariadb): pre-clean the backup roundtrip against stale tenant-root state
Address review feedback on #3384. The TIA rule for
examples/backups/<app>/ that an earlier revision of this commit added
has since landed on main independently (
|
||
|
Andrey KolkovandClaude Opus 4.8
|
d1f2822018 |
test(mariadb): add backup/restore round-trip e2e (chainsaw) via example harness
Reworks the never-merged backup-mariadb-e2e branch onto the modern Chainsaw approach (docs/agents/e2e-testing.md). The bats port that branch carried (hack/e2e-apps/backup-mariadb.bats) is dropped — the whole hack/e2e-apps/*.bats app suite has been replaced by hack/e2e-chainsaw/, and the mariadb strategy controller + example manifests it depended on have since landed on main independently. Only the e2e coverage was still missing. Convert examples/backups/mariadb/ from static numbered YAML into the same executable harness the other backup demos use (00-helpers.sh, run-all.sh, cleanup.sh), keeping the numbered manifests as the human-readable flow that run-all.sh drives — so the documented flow and the automated test can never drift. run-all.sh provisions a Bucket, derives the two per-app Secrets the strategy references from BucketInfo + the seaweedfs CA, deploys a source MariaDB with a sentinel row, fires an ad-hoc BackupJob, restores to a copy with a to-copy RestoreJob, and asserts the sentinel round-tripped through S3 into the copy while the source is left untouched. Add a second Chainsaw Test (mariadb-2-backup-roundtrip) to hack/e2e-chainsaw/mariadb/ that drives run-all.sh + cleanup.sh, mirroring postgres-2-backup-roundtrip: runs in tenant-root (the isolated e2e tenant cannot reach tenant-root's seaweedfs across the Cilium egress policy), skipDelete since the harness (not Chainsaw) owns the resources, fail-fast on terminal BackupJob/RestoreJob phases, and scoped catch diagnostics. Cleanup also prunes the operator-side k8s.mariadb.com Backup/Restore CRs the Cozystack jobs own only by label. Assisted-By: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey KolkovandClaude Opus 4.8
|
6a1e2d0d06 |
feat(backups): support PostgreSQL point-in-time recovery (PITR)
Makes RestoreJob.spec.options.recoveryTime a genuinely supported, tested capability for the new backups.cozystack.io API (issue #2774). The recoveryTime -> CNPG bootstrap.recovery.recoveryTarget.targetTime plumbing already existed on the barman-cloud-plugin branch; this adds the reliability and verification layer the issue asked for: - Fail-fast for an unreachable target: when a RestoreJob has recoveryTime set and the recovery Cluster is not healthy, the driver reads the CNPG full-recovery pod logs and, once several distinct attempts have exited with PostgreSQL's "recovery ended before configured recovery target was reached" FATAL, fails the RestoreJob with conditions RecoveryConverged=False / Ready=False (reason RecoveryTargetUnreachable) instead of hanging to the restore deadline. Health is checked first, so a converged recovery always wins; counting distinct FATAL-ing attempts (not pod failures) tolerates a transient archive-lag miss on a near-now target and ignores transient recovery-pod crashes. Adds the pods/log RBAC grant the log read requires. - e2e (examples/backups/postgres/run-all.sh, wired into the Chainsaw postgres-2-backup-roundtrip suite): step 45 writes a before marker, captures the server timestamp, writes an after marker, restores to that timestamp and asserts the before row survived and the after row did not; step 46 submits a recoveryTime an hour ahead and asserts it fails fast with reason RecoveryTargetUnreachable. - Working, uncommented example (45-restorejob-pitr.yaml) and a docs section (docs/operations/backup-classes.md) covering the recoverable window, how to discover the earliest/latest restorable time from the backup catalog, the archive-lag tolerance, and idempotency under GitOps. Refs #2774 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Aleksei Sviridkin
|
99f04d99bd |
test(postgres): verify the tenant CA projection end to end
Assert the live controller publishes <release>.tenant-ca carrying only ca.crt, that its certificate matches CloudNativePG's CA while the private key never crosses over, and that the anchor is reachable through the tenantsecrets API while the key-bearing source Secret is not. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
37a91f98b8 |
feat(cozystack-controller): extract application CA into a key-free tenant Secret
Every catalog engine issues a per-release CA, but the object holding ca.crt usually holds a private key beside it (cert-manager's tls.key, CloudNativePG's ca.key), so granting a tenant read on it hands over key material too. Add a controller that publishes the trust anchor as one canonical, key-free Secret per release, "<release>.tenant-ca", holding only ca.crt. A chart declares the anchor by rendering a namespaced TenantProjection sentinel (internal.cozystack.io/v1alpha1) that names the source Secret and key. The controller rebuilds ca.crt from the parsed certificate DER rather than copying the source, refuses any value carrying private key material, and owns the projection by owner reference so a foreign Secret at the same name is never overwritten. No tenant RBAC grants any verb on internal.cozystack.io; a ValidatingAdmissionPolicy pins writes to helm-controller as defence in depth. The controller does not grant tenant read access — the lineage webhook does, from the ApplicationDefinition's spec.secrets. This side stamps the engine-agnostic internal.cozystack.io/tenant-ca label and forces a re-admission when those selectors change. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Myasnikov Daniil
|
427ed68761 |
fix(backupstrategy-controller): give chBackupClientImage a producer
platform-migrations was pinned at two different digests: v1.5.0 in packages/core/platform/values.yaml, stamped by its Makefile, and v1.4.0-rc.2 as .backupStrategyController.chBackupClientImage, stamped by nothing at all. The second is the Altinity strategy Pod's curl+jq runner, deliberately reusing the migrations image rather than shipping a second one-binary tag that release CI would have to re-digest every cut. That reuse only holds while the two move together, and the values.yaml comment said so -- but a comment was the only thing enforcing it, so the copy with no producer froze two minors back. This is a release blocker, not just staleness. Promotion retags by digest, copying every collected <repo>@<digest> to <repo>:<stable>. Two digests under one repository produce two copies competing for one destination tag: the first wins and the second hits the write-once guard in hack/promote-retag.sh, which fails the promotion. A dry run against the tree emitted exactly that pair. v1.6.0 is the first release cut through promotion rather than a full rebuild, so it would have been the first to hit it -- every earlier release ran `make build`, which restamped both copies as a side effect and hid the missing producer. packages/core/platform/Makefile now stamps both keys with the same ref, so they are in lockstep by construction rather than by instruction, and the committed value is aligned to the digest core/platform already carried. The image is used only as a curl + jq + ca-certificates runner, so moving it forward two minors changes no behaviour the strategy depends on; it renders and the chart's helm-unittest suite passes. hack/image-pin-consistency.bats adds the general guard -- no repository pinned at more than one digest, driven through the real promotion selector so it tracks whatever set promotion acts on -- plus a specific lockstep assertion so a recurrence names its cause. The general check could not be added before now: it fails on the very duplicate this commit removes. Relates to #3143 (the unowned-pin class) and unblocks the v1.6.0 promote. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
91c076137f |
test(ci): pin shape 3's per-map digest binding
The shape-3 fixture held a single repository/digest pair, so a rule that binds one digest per file and reuses it for every repository satisfied it -- and passed all three suites. That is not a theoretical mutation: against the real linstor values it hands piraeus-server's digest to linstor-csi, which retags and mirrors the wrong image under the right name. Silently shipping wrong bytes is worse than the missing-tag bug this branch started from. The fixture now carries two shape-3 maps with distinct digests in one file and asserts both exact canonical refs, so the digest must be bound per map rather than per file. Assisted-By: Claude <noreply@anthropic.com> Assisted-By: GPT-5 <noreply@openai.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
a16f218875 |
test(ci): correct a stale comment and isolate the fail-open fixtures
Two review nits, both comment- or fixture-level. The completeness oracle's comment still said the per-shape test "matches on the repository substring, so a shape that loses its registry rejoin still satisfies it". That stopped being true in the same commit that introduced it -- the substring match was replaced with an exact canonical-ref assertion, which kills both the shape-2 and shape-3 rejoin mutations. The comment understated coverage. The fail-open test copied the tree once and looped three rc versions over it, so the second and third iterations ran against a tree the first rewrite had already mutated. Each now gets a fresh copy, so each tests the input it names. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
539a7b8e0b |
test(ci): catch shape-4 stapling inside global, and spaced marker keys
Round-5 review. The shape-4 negative fixture placed its decoy outside `global` entirely, so it only caught a document-wide broadening. Mutating the rule to `.global | ..` instead passed the per-shape test and all three suites while still stapling the document-level registry onto unrelated maps inside `global` -- manufacturing first-party refs that promotion would then retag and mirror to destinations nothing ever built. The fixture now carries two decoys at different depths, one inside `global` but outside `global.images` and one outside `global`, and the mutation is caught. Markers also required the key to be immediately followed by its colon, so `registry : ghcr.io/...` was missed. No producer writes that spelling and it is valid YAML rather than anything in the tree, so this is theoretical -- but tolerating whitespace costs one character class and keeps the comment's exhaustiveness claim true lexically as well as structurally. Marker count is unchanged at 43 files with the same two allowlist entries. Assisted-By: Claude <noreply@anthropic.com> Assisted-By: GPT-5 <noreply@openai.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
fa4ae65d04 |
test(ci): assert usable refs, quoted keys, and shape-4 over-collection
Round-4 review. Both reviewers independently found the declared-extra test was passing while the thing it guards was broken, and each found one more. That test asserted `grep -q "@sha256:<digest>"`, which any token containing the digest satisfies. Its fixture was a split-host ref, which a textual scrape can never reconstruct -- only the contiguous tag value is recoverable, so ref_repo() reduces it to `latest` and every consumer drops it at the ownership filter. The fixture was therefore enumerated-but-uncollected, which is exactly the conflation the test was added to catch, and gutting the scrape to emit a bare digest with no repository left the whole suite green. The fixture is now a contiguous ref in an unparseable template (single-quoted, so the scrape's new quote-tolerance is exercised) and the assertion pins the exact canonical ref. The limitation that a split-host ref in an unparseable file is recoverable by neither route is recorded rather than implied away: the file-level marker check is what covers that case. The scrape also captured a leading quote, so any quoted ref in a .tag file or declared extra yielded a token every consumer silently discards. Nothing in the tree is affected today, but it meant the double-pass rescued only unquoted refs. Markers missed quoted YAML keys: an unenumerated template using "registry": / "repository": passed the entire suite. They now tolerate quoting on the key as well as the value, still matching 43 files with no new allowlist entries. Shape 4 could be broadened from .global.images[] to every map in the document and pass everything, because asserting that the expected ref appears cannot see an unexpected one. That scoping is load-bearing -- the host is a document-level key, so a global binding staples kubeovn's registry onto unrelated repositories and manufactures owned refs that were never built, which promotion would then retag and mirror. The shape-4 fixture now carries a digest-pinned sibling outside global.images and the test asserts its absence. Two per-shape fixture gaps closed while there: shape 2 had no sibling `registry:`, so it losing its rejoin was invisible here and caught only by one nightly-mirror fixture; and shape 1 had no tagless case, so narrowing it to require a :tag before the digest went undetected -- a digest-only pin is an ordinary Helm spelling. Also corrected the residual-limit comment on the markers. Coverage is exhaustive, not merely broad: the collector joins registry + "/" + repository, so a split can only fall on a path-separator boundary and markers 1-3 cover all four possibilities. A split elsewhere does not need covering because it does not produce a first-party ref. Assisted-By: Claude <noreply@anthropic.com> Assisted-By: GPT-5 <noreply@openai.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
8ad25b970f |
test(ci): pin digests exactly and generalize the first-party markers
Round-3 review, both reviewers again converging on the same defects. The file-level marker check was keyed to the exact ref spellings present in the tree rather than to the join semantics _collect_yaml_shapes implements, and three realistic first-party refs evaded it: `registry: ghcr.io/cozystack/cozystack` + `repository: <name>` (kubeovn's layout under the key name keycloak-operator and ingress-nginx use, which the old marker missed because it required a trailing slash a complete-value host does not have), a host+org split with precedent at fluxcd's values.yaml, and a single-quoted `repository:` where only double quotes were admitted. All three carried a version-free latest@sha256: tag -- exactly the profile the check exists for -- and passed the whole suite. The markers now follow the collector's join semantics; this matches 43 files and needs no new allowlist entries. The residual limit (a host split at an arbitrary point inside cozystack/cozystack) is stated rather than implied, along with why widening further is not worth it: a bare cozystack/cozystack marker matches 65 files and would need 23 allowlist entries for Makefiles and test fixtures. More seriously, making the completeness oracle share shape logic introduced a regression the previous independent oracle would have caught: corrupting the last hex digit of every emitted digest left the rewrite, retag and mirror suites entirely green, because both sides of the comparison move together and the per-shape test matched only a repository substring. The digest is the only part deciding which bytes get retagged, so the per-shape test now asserts the exact canonical repo@sha256:digest for every shape. Declared extras were also narrowed by the refactor: parsing alone means a ref inside a block scalar yields the enclosing block (which the ownership filter then discards) and an extra that stops parsing after `make update` yields nothing at all. Both are silent skips, in the one storage shape defined as a manifest vendored verbatim from upstream. Extras get the textual scrape again alongside parsing, with the callers' dedup absorbing the overlap. Finally, enumerating a path only proves the path is visited: a declared extra whose ref the parser cannot reach stayed enumerated but uncollected while both completeness checks passed. Added a test asserting a declared extra yields its ref, using an unparseable Helm template with a single-quoted split-host ref -- the shape that defeated both guards. Also corrected the claim that the per-shape test compensates for the shared oracle: it catches a shape being DELETED, not narrowed. A shape losing its registry rejoin is caught by promote-retag and nightly-mirror, which assert named packages, and the comment now says so. Assisted-By: Claude <noreply@anthropic.com> Assisted-By: GPT-5 <noreply@openai.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Aleksei Sviridkin
|
f3a0f2d323 |
test(e2e): avoid head -n 0 in prevlog_cap for BSD portability
A zero container cap reached head -n 0, which GNU head treats as an empty
result but BSD/macOS head rejects ("illegal line count -- 0"), aborting a
local run. Special-case the zero cap to consume stdin instead, so the
helper behaves the same on both coreutils flavours.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
|
||
|
Myasnikov Daniil
|
648eb42cae |
test(ci): make the image-ref guards see split-host and unparseable files
Both round-2 reviewers independently landed on the same two test-side
holes, and both are the guard being blind in exactly the way the bug it
guards was.
The completeness oracle derived its expected set from a grep for a
contiguous ghcr.io/cozystack/cozystack/...@sha256: string, so it could
not see any split-host shape: cilium, keycloak-operator, kubeovn, the OCI
artifact and seven shape-3 refs, 12 of 43 first-party refs in all. That
includes keycloak-operator, which the test's own rationale names as the
motivating profile. The fixture-discovery grep in the same file had
already been widened for this exact reason; the fix was never carried to
the oracle.
The oracle now applies the same shape knowledge to an independently
discovered file list, which is the axis that actually drifts -- what each
consumer disagreed about historically is which FILES it visits, not how a
ref is spelled inside one. To keep that sharing safe, shape coverage is
now pinned by its own test, so deleting a shape still fails something
even though both sides of the comparison would move together.
Sharing required splitting collect_refs_from_file out of
collect_image_refs. Behaviour is unchanged: same 67 refs collected before
and after, multus and the packages artifact both still present.
A YAML oracle cannot cover everything, though. A reviewer planted a
split-host ref in a Helm template, which yq cannot parse at all, and it
survived every test. A file-level marker check now backstops that: any
file carrying a first-party marker must be enumerated or explicitly
allowlisted. Two files are allowlisted today, each with its reason -- the
talos-csr-signer Dockerfile placeholder, and the documented capi gzip
gap. This also closes the secondary weakness that comparing on repo@digest
masks an unenumerated occurrence when the same image sits at an
enumerated path, which is live for that capi file.
Second hole: the postcondition's key-position branch had no coverage.
Replacing the whole pattern with the naive "${RC}@sha256:" -- the
narrowing rejected because cilium keeps tag: and digest: under separate
keys -- left all 22 tests green, so the branch that makes the split shape
visible was asserted by nothing. Added a cilium-shaped fixture at an
unenumerated path.
Both previously-surviving mutations are now caught. The oracle was also
pre-filtered by registry marker before parsing, cutting it from 32s to 5s.
Assisted-By: Claude <noreply@anthropic.com>
Assisted-By: GPT-5 <noreply@openai.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
|
||
|
Aleksei Sviridkin
|
76f7267ff2 |
test(e2e): describe --previous as the preceding instance, not the first
kubectl logs --previous returns the immediately preceding terminated container instance, not the original first one — after multiple restarts the first instance is already gone. Reword the helper, the BATS and Chainsaw failure handlers, and the diagnostics doc to match the actual --previous semantics. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
1ff83f5c06 |
test(e2e): fix false claim that a job timeout preserves the report
The catch-timeout comment justified the raised ceiling by asserting that if: always() upload survives a job timeout. It does not: a 180-minute cap ends the job cancelled, and the post-cancellation grace window is consumed by the earlier report-collection step, so both cozyreport.tgz and the job log are lost. Correct the comment to describe the real failure mode. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Myasnikov Daniil
|
9bd8c17971 |
fix(ci): shape-filter the promote postcondition and close test gaps
Review of the previous commit found the postcondition itself could abort
a release. It grepped the bare "X.Y.Z-rc.N" substring across the tree, and
three live files carry such strings without being image refs:
kubevirt-csi-driver's go.sum pins github.com/golang/protobuf v1.4.0-rc.2
-- a version cozystack actually cut, so promoting v1.4.0 would have
failed on a Go checksum line -- metallb_test.yaml names v1.5.0-rc.2 in a
comment, and the console pnpm-lock.yaml carries rolldown@1.0.0-rc.15. The
remediation the error message gave would have made things worse: declaring
go.sum as a ref source would sed a checksum file.
Match the version only in image-reference position now: after an
image/repository/tag key with no intervening '#', or immediately before an
@sha256 digest. Narrowing it to "${RC}@sha256:" alone would have been
wrong -- cilium keeps `tag: v1.5.0` and `digest:` under separate keys, so
the version is not always adjacent to the digest. Verified against four
real rc versions that previously aborted.
The suite also only tested the fail-closed direction, which is why that
defect survived, and the collector had no completeness guard at all: a
first-party ref added at an unenumerated path on a `latest@` tag was
mirrored by nothing and passed all eighteen tests. Since the mirror's
failure mode is the worst of the three -- the published nightly points at
the private build registry -- that gap mattered most where coverage was
thinnest.
Added: a fail-open test using the three real rc versions above; a
collector completeness diff that discovers refs by content and fails on
any the enumeration misses; coverage for the OCI-artifact shape, whose
deletion previously left every suite green; and an unreadable-file test,
since a read error was being treated as "no match" and silently skipped.
The component-versioned test was vacuous -- its rc version shared no
substring with the kamaji tag it claimed to protect, so it passed by
construction. It now uses an rc version sharing kamaji's 0.19.0 prefix,
which a loosely-anchored implementation would corrupt.
Round-trip fixture discovery matched a combined 'ghcr.io/cozystack/
cozystack/' prefix, which skips keycloak-operator (host in a sibling
registry: key) and kubeovn (host in global.registry.address). It now
matches 'cozystack/cozystack' and stamps split `tag:` keys too.
Also documented two pre-existing gaps rather than half-fixing them: the
nightly host rewrite is a contiguous-substring replace and cannot reach a
split host, and a ref inside a gzip is invisible to every tool including
the postcondition, which skips binaries. Both are host-dimension defects;
the version dimension this change owns is unaffected, because the version
always lives in a tag key regardless of where the host sits.
Assisted-By: Claude <noreply@anthropic.com>
Assisted-By: GPT-5 <noreply@openai.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
|
||
|
Aleksei Sviridkin
|
07c67a8913 |
test(e2e): capture previous container logs and stop killing the snapshot
Root-causing a crash-loop failure was blocked twice over by the diagnostics collected on failure. `kubectl logs` shows only the current container instance, so for a crash-looping pod the suites' `podLogs` collector captured the Nth restart -- a fresh replay of the same startup path -- while the evidence that explained the failure sat in the first, interrupted instance. Chainsaw's podLogs collector has no `previous` option, so add e2e-capture-previous-logs.sh and drive it from the global catch and from the BATS failure hook. It discovers containers with restartCount > 0 at runtime (init containers included, where an interrupted bootstrap lives), orders the app suites' namespace first, and dumps each to the job log and the snapshot tree. Containers that never restarted are skipped, so a healthy run emits one line rather than a page of "previous terminated container not found". Every way that capture can come up short is named rather than inferred, because kubectl exits 1 for a garbage-collected container, an RBAC denial and an unreachable node alike: the container cap, a pod list that never returned (which is not the same as nothing having restarted), a read that timed out versus one kubectl refused and the reason it gave, and a previous instance that produced no output. A partial capture is kept and marked truncated rather than deleted -- it is the only copy of a log already gone from the cluster. The crust-gather snapshot was killed by the Chainsaw op containing it. A script op is bounded by timeouts.exec (5m), and the catch's own wall-clock wrappers summed to 960s, so the script was SIGKILLed partway through -- taking out the process group mid-write and skipping the data-plane capture entirely. Give each catch op an explicit timeout above the budgets it contains, so an inner timeout fires instead and leaves a partial, still-servable snapshot behind. The data-plane leg drops from 600s to 300s at that call site so both legs fit one envelope; the BATS hook keeps the wider budget, having no containing op. Set crust-gather --duration explicitly as well. It is the collection phase's own budget, it defaults to 60s, and on elapse it abandons the collection and skips its finish step, leaving a truncated tree with no indication anything is missing. The outer wall-clock has to exceed it because the API discovery that runs first is not covered by that budget at all -- with an unhealthy aggregated APIService, the state a failed run is usually in, discovery alone can consume the whole timeout before a single object is written. Send each collector's output to a log beside its snapshot instead of /dev/null, with a complete/truncated/failed verdict, so that question is answerable from the artifact. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Myasnikov Daniil
|
09323fff13 |
fix(ci): cover every image-ref storage shape in promote, retag and mirror
Promotion rewrote the rc version substring across the depth-2 package values.yaml plus packages/apps/kubernetes/images/*.tag alone, on the premise that the kubernetes app was the only one whose .tag files carry the cozystack version. Nine other .tag files do, and system/multus seds its ref straight into a vendored upstream manifest. v1.6.0 was therefore staged with 33 of 55 first-party refs still reading v1.6.0-rc.4. promote-retag.sh and nightly-mirror.sh had the same blind spot from the other direction: both scanned only the depth-2 values.yaml, so those images were never retagged to the stable version (30 refs selected before, 42 after) and never mirrored to the public registry for a nightly. A retag miss merely leaves an image short of a tag, since the digest still resolves inside one registry; a mirror miss is worse, because the host rewrite walks the same file list and a ref that is neither mirrored nor rewritten leaves the published tree pointing at the private build registry. The three call sites each carried their own idea of where a ref can live, which is how they drifted apart. Move that knowledge into hack/lib/image-refs.sh as the single enumeration -- image_ref_files() for the files, collect_image_refs() for the refs inside them -- and have all three source it, so a new storage location is declared once and reaches every consumer. The rewrite itself moves out of promote-rc.yaml into hack/promote-rewrite-tags.sh. Being workflow-inline is why this was only observable by cutting a release: there was nothing to unit test. It now also asserts its own postcondition, scanning wider than it rewrites and failing the promotion when any ref survives in a location the enumeration does not know about, rather than shipping it quietly. Component-versioned images (kamaji's v0.19.0-cozystack.N) and third-party pass-through images do not ride the cozystack version line and are deliberately left alone; a test pins that so a future fix here cannot over-reach. capi-providers-cpprovider is a documented known gap: its ref is also embedded in a gzipped copy that the chart actually ships, and rewriting only the readable copy would diverge the two. docs/agents/image-refs.md records the contract -- three tag classes, three storage shapes, the invariants, and which consumer does what. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
myasnikovdaniil
|
960d853706 |
feat(api): typed waitStrategy/healthCheckExprs for kstatus readiness (#2642) (#3273)
## What Adds typed `waitStrategy` + `healthCheckExprs` fields so a package/app can declare the Flux helm-controller v1.5.0 kstatus health gate for the custom resource it wraps — making `HelmRelease.Ready` mean "the CR is actually healthy" instead of "helm applied the CR". This is the operator/apiserver foundation of the kstatus epic (#2642); the API surface was reviewed and approved by the API owner in #3263. Commits (per the review's "isolated step-0" request): 1. `build(deps)` — the dependency bump (see below). 2. `feat(api)` — the fields, plumbing, tests, and regenerated code. 3. `fix(api)` — match the openapi code-generator pin to the 0.35 runtime (fixes an aggregated-apiserver crash the bump surfaced). 4. `fix(api)` — declare `OpenAPIModelName` on the apps types so SSA resolves catalog kinds. 5. `docs(api)` — note that `disableWait` silently defeats `healthCheckExprs`. ## API surface Typed fields on **both** HelmRelease-building paths: - `ApplicationDefinition.spec.release` (apiserver / catalog apps → `convertApplicationToHelmRelease`) - `ComponentInstall` (operator / platform packages → `buildHelmReleaseSpec`) ```go // WaitStrategy maps to HelmReleaseSpec.WaitStrategy.Name — a deliberate scalar // simplification of the upstream {name} object. One of poller|legacy. WaitStrategy string // HealthCheckExprs maps to HelmReleaseSpec.HealthCheckExprs (the upstream // kustomize.CustomHealthCheck type verbatim). HealthCheckExprs []kustomize.CustomHealthCheck ``` `config.ResolveWaitStrategy` is shared by both builders and **couples the default**: `healthCheckExprs` are only evaluated under the `poller` strategy, so when expressions are set and no strategy is given, the generated HelmRelease defaults to `poller`. A package that sets only `healthCheckExprs` is therefore self-contained, independent of the controller's global default. ## The dependency bump (commit 1) The `WaitStrategy` / `kustomize.CustomHealthCheck` / `HelmReleaseSpec.HealthCheckExprs` types do not exist below `helm-controller/api` v1.5.0 — the runtime is already v1.5.0 (shipped by #2602), so this closes a Go-types-vs-runtime skew. It is **not** an isolated bump: v1.5.x requires **k8s 0.35 + controller-runtime 0.23**, cascading the k8s stack 0.34 → 0.35 (and the go directive to 1.26). cozystack replaces `k8s.io/apimachinery` with a fork carrying the still-open upstream `ConvertToVersion` patch (k/k#135537). That single commit rebases cleanly onto apimachinery v0.35.0; published at `cozystack/apimachinery@release-1.35-pr135537`, and `go.mod` pins its pseudo-version. **The repo builds with zero source changes under the new stack.** > Maintainer note: please confirm the `release-1.35-pr135537` fork branch naming, or fold it into the `cozystack` branch as you prefer — the pin can be updated to match. ## Not included (conscious YAGNI) `disableWait` is intentionally omitted. The only known need (the kubernetes tenant-addon deadlock) was resolved child-side, and `waitStrategy: legacy` is not equivalent (legacy still waits Helm-v3-style; `disableWait` skips waiting entirely). Add it later when a concrete leaf needs a true no-gate escape hatch. ## Tests - `config.TestResolveWaitStrategy` — the couple-the-default logic (unset+exprs→poller; explicit legacy honored; unset+no-exprs→nil). - Operator + apiserver cases mirror each other (`TestBuildHelmReleaseSpecHealthCheckExprs` / `TestConvertApplicationToHelmRelease_HealthCheckExprs`). - `go build ./...` clean, `go vet` clean, `gofmt` clean, codegen regenerated and committed. The 4 backupstrategy CRDs regenerate with benign k8s-0.35 PodSpec description drift (DynamicResourceAllocation alpha→stable, toleration Lt/Gt, resizePolicy note) — no functional change. ## Follow-up Phase 3 (per-leaf adoption — postgres/kafka/mongodb/etc. declaring their CEL via these fields) builds on this. The full CRD→CEL catalog is maintained in the epic's readiness audit. Refs #2642 ```release-note feat(api): add `waitStrategy` and `healthCheckExprs` to ApplicationDefinition and ComponentInstall, so a HelmRelease reports Ready only once the custom resource it wraps is actually healthy ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added configurable CEL health-check expressions for application releases and package components. * Added `waitStrategy` support (`poller` and `legacy`) with automatic `poller` selection when health checks are configured and no strategy is set. * Propagated these settings into generated Helm releases, affecting readiness/wait behavior. * **Documentation** * Updated Application and PackageSource schemas to include `healthCheckExprs` and `waitStrategy`. * Extended related Kubernetes CRD documentation, including projected pod certificate annotations and workload reference fields. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Andrey Kolkov
|
e73ad68a85 |
fix(backups): support cnpg-operator with barman plugin (#3313)
<!-- Thank you for making a contribution! Here are some tips for you:
- Use Conventional Commits for the PR title: `type(scope): description`
- Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore
- Scopes are not an exhaustive list — pick the most specific scope for
the change and extend the list when a genuinely new area appears.
Examples:
- System components: dashboard, platform, operator, cilium, kube-ovn,
linstor, fluxcd, cluster-api
- Managed apps: postgres, mariadb, redis, kafka, clickhouse,
virtual-machine, kubernetes
- Development and maintenance: api, hack, tests, ci, docs, maintenance
- Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or
add a `BREAKING CHANGE:` footer
- If it's a work in progress, consider creating this PR as a draft.
- Don't hesistate to ask for opinion and review in the community chats,
even if it's still a draft.
- Add the label `backport` if it's a bugfix that needs to be backported
to a previous version.
-->
## What this PR does
Fixes #3300. Migrates CNPG Postgres backups off the deprecated native
`spec.backup.barmanObjectStore` (removed in CNPG 1.29; the
`barman-cloud-*` binaries it needs are absent from the `standard` image
variant keycloak-db pins, and the bare system-flavor tags apps/postgres
pins are deprecated upstream) onto the [CloudNativePG Barman Cloud
plugin](https://github.com/cloudnative-pg/plugin-barman-cloud), across
every Postgres backup surface: `apps/postgres`, `system/keycloak`, and
the `cozy-default-cnpg` BackupClass strategy driver. The chart renders
`spec.plugins` + a `barmancloud.cnpg.io/ObjectStore` CR; the
backup-controller driver SSA-applies an `ObjectStore` and patches
`spec.plugins`; `Backup`/`ScheduledBackup` use `method: plugin`. The
`postgres-operator` chart vendors and co-locates the
`plugin-barman-cloud` operator (with its cert-manager-issued mTLS) so
CNPG can discover it.
One platform-level change is required for the plugin to run and ships
with this PR (see the upgrade notes in
`docs/operations/backup-classes.md` and the release note below): the
CloudNativePG operator is bumped to 1.28.1 (also fixing an operator/CRD
version skew that broke all backups). The other prerequisite — the
LINSTOR scheduler admission webhook stripping
`initContainers[].restartPolicy` cluster-wide, which broke the plugin's
restartable sidecar — is already fixed on `main` (
|
||
|
Myasnikov Daniil
|
292f8ec65a |
fix(ci): type-guard the concatenated keys in the ref selectors
Address non-blocking review feedback from lexfrei on hack/nightly-mirror.sh:81 and hack/promote-retag.sh:84. `registry`, `repository` and `digest` are joined into a ref by string concatenation with no type guard, so a non-string value could abort the whole per-file expression. Because collect_refs swallows both stderr and status, that abort silently discards every co-located owned ref — the same per-file blast radius the `tag` guard already prevents. The mechanism differs from `tag`'s, though, and the narrower reading is worth recording. These keys are never fed to test(), so the "cannot match with !!int" abort does not apply to them: yq coerces int, bool, null and seq on `+`, and each of those merely yields a ref the ownership filter discards. Only a !!map actually aborts, raising "!!str () cannot be added to a !!map". The guard is therefore narrow insurance against a nested-map typo in a vendored chart, not protection against unquoted scalars. Implemented as select() rather than a test(), so an offending map drops only itself and its neighbours in the same file still resolve. The nightly fixture gains a file pairing a map-typed `repository` with an owned ref, asserting the latter is still mirrored; removing the guard turns it red. Selection over the committed tree is unchanged at 30 repositories. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
acd88bdd00 |
fix(ci): collect image refs whose host is outside repository
Address review feedback from lexfrei and IvanHunters on
hack/promote-retag.sh:84 and hack/nightly-mirror.sh:81.
The shape-3 rule assumed the registry host lives inside `repository`.
Two cozystack-built images do not satisfy that and were emitted
host-less, so the ownership filter dropped them — the same silent
emit-then-drop this PR fixes for the other shapes:
- keycloak-operator splits the host into a sibling `registry` key
({registry: ghcr.io, repository: cozystack/cozystack/keycloak-
operator, tag: latest@sha256:...}), and its own Makefile writes
exactly that layout.
- kubeovn keeps the host in the document-level
global.registry.address with a bare `repository: kubeovn`. It is
built and pushed to $REGISTRY by the cozystack/kubeovn-chart
wrapper, whose `make image` writes all three keys.
Neither has received a 1.x release tag: on GHCR keycloak-operator
carries only `latest`, and kubeovn's newest cozystack-versioned tag is
v1.14.11-v0.38.8, from before the 1.0 versioning switch — while cilium,
which uses an already-covered shape, has v1.5.0 through v1.6.0-rc.2.
Shapes 2 and 3 now rejoin an optional `registry` sibling, and a new
shape-4 rule handles the chart-global layout. An audit of every
@sha256: digest under packages/*/*/values.yaml against the selector
output confirms the remaining five misses are third-party or sit
outside $REGISTRY. Dry-run over the committed tree: 28 -> 30
repositories, exactly these two gained, nothing lost.
Also correct two comments that overstated coverage. The header no
longer presents its list as the shapes the build is limited to — the
list is empirical and charts are vendored — and the ownership-filter
comment no longer names kilo, kube-ovn and keycloak-operator as
third parties not pushed to $REGISTRY. All three are pushed there, and
that wrong comment is part of why the missing tags went unnoticed.
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
|
||
|
Andrey KolkovandClaude Fable 5
|
b897b53107 |
test(postgres): fix S3 CA secret name in backup roundtrip e2e
The backup roundtrip e2e copied the seaweedfs S3 CA from a Secret named seaweedfs-system-ca-cert in tenant-root, a name captured off a dev cluster. On current main the seaweedfs chart pins fullnameOverride: seaweedfs, so the cert-manager CA Certificate's secret is seaweedfs-ca-cert; the old name is absent and the first CI run of the e2e failed at step 05a with 'secrets "seaweedfs-system-ca-cert" not found'. Default S3_CA_SECRET to seaweedfs-ca-cert and, when that Secret is absent, auto-discover the seaweedfs CA Certificate (the seaweedfs-labelled one with spec.isCA=true) and read its secretName, so a future fullnameOverride change does not silently break the copy again. Fail with a clear message when neither resolves. README and chainsaw comment updated to match. Assisted-By: Claude <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Myasnikov Daniil
|
770314b3cb |
fix(ci): retag images whose digest is embedded in the values tag
promote-retag.sh carries the same collect_refs blind spot the previous commit fixes in nightly-mirror.sh, and nightly-mirror.sh's own comment asserted the two were "identical to" each other -- which is how the gap stayed invisible once one of them was fixed. Measured against the committed tree, the dry-run selects 19 repositories before this change and 27 after. The eight it was missing are exactly the set that broke the nightly: kamaji, kilo, linstor-csi, piraeus-server, linstor-gui, metallb-controller, metallb-speaker, redis-operator. Nothing is lost from the previous selection. The consequence differs from the nightly case and is milder. This retag is within a single registry, so the digests already exist and every digest-pinned install still resolves -- there is no dangling reference and no 404. What is missing is the :<version> tag (and :latest under MOVE_LATEST=1) for those eight images, so a promoted release does not carry the tags it claims to, and anything resolving a cozystack component by version tag rather than by digest does not find it. Nothing in the promotion detects this: the script reports success having simply never seen those refs. The existing test could not catch it. It asserts "at least one cozystack-owned image is selected" and that nothing third-party leaks in -- both satisfied by the shapes that already worked, which is why eight omissions sat under a green suite. It now names concrete shape-3 packages, since a count or a generic pattern would drift back to proving nothing. Verified the assertion fails against the unfixed script. Same type guard on `.tag` as the previous commit, for the same reason: without it an unquoted numeric tag anywhere in a file silently discards every shape-3 ref in that file, and the swallowed stderr leaves no trace. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
06278ac18b |
fix(ci): mirror images whose digest is embedded in the values tag
The nightly's full-suite e2e failed at install: the e2e-image-prepuller
DaemonSet never became ready, because
ghcr.io/cozystack/cozystack/linstor-csi@sha256:2f314cb9... and
.../piraeus-server@sha256:39fdacec... both 404 on GHCR. They are not there
because nightly-mirror.sh never copied them.
collect_refs handled three values shapes, and the one most package Makefiles
actually write is not among them: a split map whose digest is embedded in the
tag, {repository, tag: <tag>@sha256:<digest>}, produced by a single yq call
setting .image.tag to "$(IMAGE_TAG)@$(digest)". It matches neither the
{repository, tag, digest} rule (no `digest` key) nor, usefully, the
single-string rule -- that one matches the bare tag value, which carries no
repository, so ref_repo() reduces "main@sha256:..." to the repository "main"
and the SRC_REGISTRY case drops it.
Being dropped there is silent, and the host rewrite at the end of the script
runs over every depth-2 values.yaml regardless of what was mirrored. So the
image was never copied AND its repository was still repointed at
DST_REGISTRY, leaving a reference to an image that was never pushed there --
exactly the dangling-ref failure the comment above that rewrite warns about,
reached by a path it did not anticipate. The 404 then surfaces at pull time,
one job later, as a DaemonSet rollout timeout that names no image.
This is not limited to linstor. Checking the :nightly tag on GHCR, the two
packages using a recognised shape (cilium, cozystack-operator) are present
and eight images across six packages using this shape are absent: kamaji,
kilo, linstor-csi, piraeus-server, linstor-gui, metallb-controller,
metallb-speaker, redis-operator. The prepuller only templates
kubeovn/linstor/cert-manager, so it surfaced two of the eight; the rest would
have failed later at HelmRelease install.
Latent since the script was added in
|
||
|
Myasnikov Daniil
|
bd770b2288 |
fix(hack): make the seaweedfs naming audit POSIX sh
hack/seaweedfs-naming-audit.bats sources the audit script at top level,
and cozytest.sh sources the converted test file into its own /bin/sh --
dash 0.5.12 on the Ubuntu CI runner. dash has no `pipefail`, so
`set -uo pipefail` aborted the runner before the first @test and took
the whole file down with it:
hack/cozytest.sh: 48: set: Illegal option -o pipefail
The bash shebang could not prevent this: `.` executes the file's
contents in the caller's shell, so the shebang is only a comment there.
The script was already POSIX apart from those two lines -- no arrays,
no [[, no local, no bashisms at all -- so the script becomes POSIX sh
rather than the runner becoming bash. That keeps the shell under test
and the shell at run time the same one, instead of testing under dash
while shipping under bash.
Dropping `pipefail` changes no behaviour: the script never sets -e and
never reads a pipeline's exit status, reporting failure as empty output
checked with [ -n ... ].
Verified under dash (ubuntu:24.04): 11/11 tests pass, and the script
runs clean against a live cluster, classifying five seaweedfs tenants
with the revision-1 scheme and the PV ages agreeing on every MIXED
case. shellcheck -s sh reports nothing.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
|
||
|
Myasnikov Daniil
|
9b82d1bf36 |
test(seaweedfs): run the db-adopt migrations in the shell they ship with
Address review feedback from IvanHunters on packages/core/platform/images/migrations/migrations/43:23: The suite invoked the migrations as `sh "$MIG_DIR/<n>"`. On the CI runner /bin/sh is dash, which has no `set -o pipefail`, so both scripts aborted on their `set` line with "set: Illegal option -o pipefail" (exit 2) before doing any work, and the job went red. Production was unaffected — the migrations image is FROM alpine, so /bin/sh there is busybox ash — which is exactly why checking the runtime image missed it. Rather than change a shebang or drop pipefail, run the migrations the way run-migrations.sh does: by path, inside the image's own pinned alpine base, read out of the migrations Dockerfile so the interpreter under test cannot drift from the one they ship on. pipefail is load-bearing here — lib/cozystack-version.sh pipes the rendered manifest into `kubectl apply`, so without it a failed render stamps the version from empty input — and a shell that lacks it can prove nothing about that path. run_migration() returns the container's status explicitly: cozytest.sh's awk generator rewrites every bare `}` in column 0 into `return 0` + `}`, so a helper that falls off its own end reports success no matter what it ran, and every fail-closed assertion would have passed vacuously. 15/15 pass. Verified the interpreter under test is BusyBox v1.37.0 ash with pipefail supported, and that a fail-open mutation in lib/seaweedfs-db-adopt.sh turns the suite red. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
24bcb0bb69 |
fix(seaweedfs): decide MIXED direction by relative PV vintage, not a clock window
The audit named the candidate duplicate by measuring each generation's oldest PV against the release's first_deployed, with an absolute 120-second window for the mid-rebind case. The shipped StorageClasses are WaitForFirstConsumer, so PVs are provisioned at pod-SCHEDULE time — on a cold cluster minutes after first_deployed — and a tenant interrupted mid-way through Step 2's re-bind (both generations sitting on original-vintage PVs) fell out of the window and into a branch decided by which volume index happened to provision first. The advice printed there routes the operator to a step that deletes the claims Step 2 had not yet re-bound. Replace the window with the relative rule runbook Step 2a already enforces: a generation is the candidate duplicate only when EVERY one of its bound PVs is strictly newer than every bound PV of the other generation. Overlapping or tying vintages name no candidate — finish Step 2 or escalate. first_deployed is still printed, as context only. The direction decision is now a pure function (classify_mixed_direction) so the bats suite drives it directly: clean duplicate both ways, interrupted re-bind, tie, and a no-clock case pinning the WaitForFirstConsumer regression. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
601f164c5f |
fix(seaweedfs): stop the naming guard guessing which volume generation holds the data
The guard told a class-S tenant from a D tenant by comparing PVC creationTimestamps,
on the premise that "PVCs are never recreated in place, so the older generation is
where the data was born". The premise is false, and this runbook is the counter-
example: Step 2's re-bind DELETES each release-named claim and recreates it under
the chart name against the same PV.
So a tenant interrupted part-way through Step 2 — after volume-0, before volume-1 —
has data1-seaweedfs-volume-0 created SECONDS ago holding real data, beside
data1-seaweedfs-system-volume-1 with the original timestamp also holding real data.
That is the exact inversion of the rule. Reproduced against the old template:
keeps its data on the release-named volume PVCs (created 2026-06-20T07:01:59Z,
OLDER than the chart-named data1-seaweedfs-volume-* claims created
2026-07-17T09:30:00Z) ... Remove the empty chart-named set ... (Step 2a, then Step 2)
Step 2a then runs `grep -E '^data1-seaweedfs-volume' | xargs kubectl delete pvc` —
deleting the claim Step 2 had just re-bound — and Step 2 had already restored the
PV's original reclaim policy, so on any Delete-policy StorageClass the volume and
the bytes go with it. The guard's own recovery path was a data-loss instruction.
Two more holes in the same classifier:
- The S-damaged branch was gated on $systemPVC, which requires the PVC name to
contain "seaweedfs". seaweedfs.componentName truncates the fullname to 56 chars
before appending -volume, so an instance name >= ~40 chars drops the chart name
off the PVC name — the very case fullname_override_named_test.yaml exists for.
Probed with `archive-of-quarterly-financial-statements-x1`: damaged + renamed set
Ready fell through to the D-split branch ("is running two live SeaweedFS sets ...
Stop the split"), sending the operator to quiesce the set holding ALL the data —
the exact misclassification the parent commit exists to remove. Damaged + not
Ready fired no branch at all and rendered through unguarded.
- readyReplicas == 0 was read as proof the duplicate never served. It is a
snapshot: a duplicate that crashed, was scaled down, or lost readiness AFTER
serving writes reads identically, and adoption strands whatever it wrote.
There is no sound signal here. Claim timestamps are mutable, StatefulSets are
recreated by the adoption hook, and liveness is a snapshot — nothing durable in the
object graph distinguishes "empty set a bad upgrade created" from "the data,
mid-re-bind". Rather than add a fourth heuristic, the guard now decides only what
is decidable:
legacy generation only (or nothing) -> render; adoption is correct
release-named generation only -> refuse, class S (Step 2)
BOTH generations -> refuse; an operator classifies
Both generation flags now OR the PVC and StatefulSet evidence, so the truncated
long-name tenant is seen via its label-matched StatefulSet.
The cost is bounded and lands only where it should. A duplicate exists only on a
tenant that passed through the broken 4.31 rename in 1.5.x — already damaged,
already needing an operator. A 1.4.x tenant upgrading straight to 1.6 never
renames, so it has one generation and is untouched. Confirmed on the stand:
tenant-l (1.4.5-era, never upgraded) is legacy-only and dry-runs clean; the four
both-generation tenants all already need an operator. What is lost is D-wedged
auto-adoption, which was resting on the readyReplicas assumption above.
Classification moves to the runbook, where a human has signals a template does not:
Step 1 now reads the ages of the PVs (durable — they survive the re-bind) via
.spec.volumeName instead of the claims, reports MIXED instead of picking, and walks
the operator through interrupted-Step-2 first, then strict age separation, then
verifying emptiness directly, then escalating rather than guessing. Step 2 stashes
the PV's original reclaim policy ON THE PV and leaves it Retain until Step 5
verifies the tenant, so a mis-step during recovery is survivable. Step 2a grows a
precondition that refuses unless every chart-named claim is bound to a strictly
newer PV.
Tests: 7 of 14 in the enforcing suite fail against the old template, including the
interrupted-Step-2 tenant and both truncated-long-name cases. hack/seaweedfs-guard-
parity.bats pins the two copies (system/ enforcing, extra/ sibling) against silent
drift — they cannot share a library, neither chart depends on cozy-lib, and "keep
these in sync" as a comment is the failure class this whole branch is about; it
also pins that creationTimestamp and readyReplicas never come back.
`make test` in both charts now runs helm template --is-upgrade and greps for the
refusal. The fail-closed canary — the headline safety property — had ZERO automated
coverage: guard_fail_closed_test.yaml passes with AND without the fix, because
helm-unittest 1.0.3 ignores release.isUpgrade. Verified the new target fails when
the canary is removed.
Review round 3 — the refusal was right; the justification was false and the
procedure that replaced the chart's judgement was broken on both axes.
THE COMMENT WAS WRONG. It said "nothing durable in the object graph distinguishes
the empty generation from the data mid-re-bind". A durable signal does exist, and
the reviewers were right to call that out: the Helm release history. Verified on
the stand — sh.helm.release.v1.<name>-system.v1 records the scheme the tenant was
BORN under (tenant-root/l/dsplit/named rev 1 render seaweedfs-master => pre-4.31;
tenant-fresh renders seaweedfs-system-master => 1.5.x), and info.first_deployed is
on every retained revision, so anchoring PV creationTimestamps against it survives
history pruning and the Step 2 re-bind (tenant-root: chart-named +10s vs
release-named +2084s; tenant-fresh: +5225s vs +6s).
BUT IT DOES NOT UNLOCK AUTO-ADOPTION, and that is the substantive disagreement
with the round-3 direction. Birth order answers "which generation is ORIGINAL".
Adoption needs "is the OTHER one EMPTY". Those come apart exactly where it
matters: D-wedged (duplicate never scheduled, safe to adopt) and D-split
(duplicate served writes, adopting strands them) are BOTH born pre-4.31 and are
therefore identical on every durable signal. Verified: tenant-root and
tenant-dsplit have the same rev-1 scheme and the same first_deployed deltas, yet
one duplicate served and the other did not. The only thing that ever separated
them was readyReplicas, which is a snapshot and not evidence. So the render keeps
refusing on presence, and the comment now says why — naming the signal, so nobody
re-derives the broken one.
The signal IS used where it belongs, in the audit.
- naming-guard.yaml / seaweedfs.yaml: match the release-named generation by
RECONSTRUCTING the name 4.31 gives this release's volume component
(templates/_naming.tpl, replaying seaweedfs.fullname + componentName), tested
FIRST because the prefixes nest. Fixes a blocker: an instance legitimately named
`seaweedfs-volume` renders seaweedfs-volume-system-volume and
data1-seaweedfs-volume-system-volume-0, which BOTH satisfy the chart-named
prefixes — a prefix-only guard read its live storage as legacy and would render
onto the empty chart-named claims, stranding the real generation. The parity
bats passed straight through it, because parity pins sameness, not correctness.
Reconstruction also removes the name-match blind spot at source: a truncated long
instance name now matches by PVC, not only by StatefulSet.
- hack/seaweedfs-naming-audit.sh + .bats: the classifier is a tested script now.
Two revisions of it shipped as an untested snippet inside the runbook and both
were wrong in ways that routed a live tenant into the step that strands its data:
a selector `^data1-(.*seaweedfs.*)-volume` that also matched the chart-named
claims, so the release-named range spanned BOTH generations, every tenant read as
"overlapping / mid-rebind", and a genuine S-damaged tenant was routed AWAY from
Step 2a into Step 2 — which deletes its data claim, fails to recreate it, then
deletes its StatefulSets; and a `grep seaweedfs` name match that reported
"L — nothing to do" for a long-named tenant the chart refuses. Both are covered.
The audit reports L/S/MIXED matching the guard, names the original generation
from both durable signals, detects MID-REBIND (both generations at
first_deployed), and states plainly that "original" is not "empty" and that
emptiness must be established from the duplicate's volume files before deleting.
A bats case renders _naming.tpl through helm and compares it to the script's
reconstruction, so the two implementations cannot drift.
- Runbook: Step 1 calls the script. Steps 3/4 were circular — Step 3 ended in an
upgrade the presence-gated guard refuses, and Step 4 told the operator to clear
duplicates AFTER an upgrade that cannot run until they are gone. Step 3 now
removes the duplicate BEFORE the upgrade and re-runs the audit to prove it; Step 4
starts from "every tenant reads L or S". D-wedged no longer claims to need
nothing. Step 3's "enumerate fids that resolve only on the duplicate" was not an
executable procedure and is now an explicit escalation: two masters allocate
volume IDs from independent sequences into one shared seaweedfs-db, so there is
no supported tool that reconciles two volume-ID spaces against one metadata
store. Step 2's re-bind restores the cleanup hook's labels on the recreated
claims, without which a recovered tenant leaks its volumes on a later delete.
Fixture corrected: data1-...-x1-system-volume-0 is not a name the chart renders;
componentName gives ...-x1-system-seaw-volume-0.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
|
||
|
Myasnikov Daniil
|
47e24f01d1 |
fix(platform): hand seaweedfs-db over for every instance name, not just the default
Migration 43 performs the v1.5.0 db-split hand-over: it re-owns Cluster/seaweedfs-db from <name>-system to the new <name>-db release and stamps helm.sh/resource-policy: keep, so the <name>-system upgrade — whose post-split chart no longer renders the Cluster — does not prune it as a removed resource. It compared the owning release name against the literal "seaweedfs-system". SeaweedFS is a user-creatable kind, so an instance named `foo` is owned by `foo-system` and was silently skipped: no re-own, no keep. The <name>-system upgrade then deleted the Cluster, CNPG took its PVC with it, and the filer metadata — the index for every object in that tenant's S3 — was destroyed. Data loss, not an outage: the volume PVCs still hold the bytes, nothing can find them. Verified live on the upgrade stand, which is a natural control group. All five tenants sit in the SAME failed-upgrade loop (<name>-system last-deployed is rev 1, a pre-split revision whose manifest still contains the Cluster, so every retry recomputes the same deletion): tenant-root/dsplit/l instance `seaweedfs` owner=seaweedfs-db keep Cluster alive tenant-named instance `foo` no Cluster at all Cluster GONE The only difference is that migration 43 fired for the default-named instances and skipped `foo`. That also answers whether `keep` is sufficient: three tenants survive the identical prune loop with it. Correction to the reported mechanism: this is the 1.4->1.5 db split (PR #2601, v1.5.0), not a 1.5->1.6 change — v1.5.3 and main are identical on every db-split file. And the split does protect the object; it just only protected the instances someone happened to name `seaweedfs`. - lib/seaweedfs-db-adopt.sh: shared hand-over matching the `-system` SUFFIX, so `foo-system` -> `foo-db` exactly as `seaweedfs-system` -> `seaweedfs-db`. Idempotent, and refuses a release named literally `-system` rather than annotating an owner of `-db` that no release would claim. - Migration 43 sources it: covers clusters upgrading from before 43 (1.4.x). - Migration 53 (new) re-runs it: migrations never re-run, so any cluster already at >= 44 ran the hardcoded version and is still exposed. As a pre-upgrade hook it lands before <name>-system re-renders, closing the window on THIS upgrade. This is the one that matters for 1.5.x -> 1.6. - targetVersion 53 -> 54. Tests fail on unfixed code: the non-default-name case produces no ANNOTATE at all against the hardcoded 43, while the default-name case passes — which is precisely the bug's shape. They drive the real migration scripts against a fake kubectl, following hack/migration-50-etcd-adopt.bats. LIMIT, stated plainly: a Cluster that is already deleted cannot be recovered by either migration — there is nothing left to re-annotate and the PVC went with it. Such a tenant needs seaweedfs-db restored from a backup. Runbook Step 0 adds the ownership audit that distinguishes at-risk from already-lost, and records that <name>-db can report Ready while its Cluster is gone (it rendered fine; a later <name>-system prune removed it and Flux has not re-checked) — so the audit trusts `kubectl get cluster`, not the HelmRelease status. Review round 2 — two ways this still lost the database: - OWNERSHIP IS NOT SAFETY. The helper skipped any Cluster already owned by <name>-db, treating ownership as proof the hand-over was done. It is not: where the hand-over was skipped, <name>-system prunes the Cluster and <name>-db RECREATES it under its own ownership with NO keep, while <name>-system's prune baseline still lists it — so the next reconcile deletes it again. That is the observed delete/recreate loop, and the skip walked straight past it. Verified on the stand: tenant-l and tenant-root are seaweedfs-db-owned AND their <name>-system deployed revision (rev 1) still contains the Cluster, so only keep saves them; tenant-fresh is seaweedfs-db-owned without keep but was installed after the split, so its baseline never had one. Telling those apart needs the release's deployed manifest, which this script cannot read cheaply or reliably. The costs are asymmetric — a needless keep leaves an orphan on delete (now reclaimed by the cleanup hook), a missing one loses the database — so keep is stamped on every Cluster owned by either side of the split. - FAIL OPEN, THEN STAMP. `set -euo pipefail` does not abort on a failing command substitution in a `for` word-list: the loop ran zero times, the script continued, and stamp_cozystack_version ran anyway. Migrations never re-run, so one throttle, RBAC hiccup, or not-yet-established CNPG apiservice behind the pre-upgrade hook permanently left every at-risk tenant exposed with no later migration to catch them. Reproduced: against the old helper a failing fleet scan still logged STAMP 54. Now every kubectl failure is fatal EXCEPT the two that genuinely mean "nothing to do" — the CNPG resource type not being served (the fail-open that IS load-bearing: a cluster without CNPG must still upgrade) and a Cluster vanishing between scan and read. An unreadable annotation is no longer indistinguishable from an absent one, and the unowned case now warns instead of passing silently. - ORPHAN ON DELETE. `keep` is permanent (removal deferred to the 1.7 batch migrations) and also survives the <name>-db release's own uninstall, so `kubectl delete seaweedfs <name>` left the CNPG Cluster, its Postgres pods and its PVCs behind — the cleanup hook selects app.kubernetes.io/instance=<name>-system, which the db PVCs (cnpg.io/cluster=seaweedfs-db) do not carry. Migration 53 widened that from pre-1.5 default-named tenants to every instance name, so the hook now deletes the Cluster explicitly; its PVCs carry ownerReferences to it and follow. - Tests assert the stamped VERSION (44 / 54), not a bare "STAMP": a wrong number would loop run-migrations.sh forever and the old assertion could not tell. Review round 3 — the reclaim introduced a data-loss path of its own: - DELETING ONE APP DELETED ANOTHER APP'S DATABASE. The db chart hardcodes the Cluster name `seaweedfs-db` for every instance, and the reclaim was scoped by that name in the namespace rather than by what the release owns. templates/seaweedfs-db.yaml gates the <name>-db HelmRelease on `topology != "Client"`; the cleanup hook had no such gate. Verified by render: `helm template s3-remote . --set topology=Client` emits ZERO s3-remote-db HelmReleases yet still emitted the Role granting delete on clusters/seaweedfs-db AND `kubectl delete cluster.postgresql.cnpg.io -n tenant-root seaweedfs-db`. So in a namespace running a server instance plus a Client, deleting the CLIENT destroyed the SERVER's filer metadata — CNPG takes the PVC via ownerReferences — for an app the operator never touched. Reachable the same way via a second Simple instance, whose <name>-db loses Helm's ownership check but which still installs and still carries the hook. main's hook was instance-scoped; this was new, and mine. Now gated exactly as seaweedfs-db.yaml is, and the Job re-checks meta.helm.sh/release-name == <release>-db at run time before deleting. The gate stops the permission being granted where it can never be legitimate; the runtime check stops the delete where it can. - `|| echo` turned any API/authz/admission failure into exit zero, so backoffLimit never retried and hook-delete-policy removed the only cleanup Job while the keep-protected Cluster stayed orphaned. A failed reclaim now fails the Job. - TEST SHELL. The migrations run under /bin/sh = busybox ash (the image is FROM alpine, run-migrations.sh is #!/bin/sh), but the tests drove them with bash — so the fail-closed guarantees were asserted in a shell that never runs them, which matters more than usual given the bug they cover was a fail-open. The scripts are now invoked via `sh` and the fake kubectl is POSIX sh. Verified directly against the production base image (busybox 1.37.0 / alpine:3.24): `set -o pipefail` is supported, every script passes `ash -n`, and the failing-fleet-scan path exits non-zero without stamping under real ash. The recipe is in the bats header. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
myasnikovdaniil
|
6338b0b2fd |
fix(migrations): derive the etcd-adoption snapshot target from the projected bucket creds (#3335)
## What this PR does
Migration 50 takes a mandatory S3 safety snapshot before adopting live
etcd onto the v1alpha2 operator. It is a **`pre-upgrade` hook**, so
every chart-rendered resource it reads still belongs to the version
being upgraded **from**. It resolved the snapshot target from the live
`cozy-default-etcd` Strategy CR — and on a v1.5.x cluster that CR cannot
be trusted to describe a reachable target. The platform upgrade is gated
behind the snapshot, so nothing can self-heal first: the corrected
`backupstrategy-controller.endpoint` helper only reconciles **after**
the upgrade this hook blocks.
### Two failure modes, both a hard block
**1. The Strategy CR does not exist.** `strategy-etcd-default.yaml` is
guarded by `{{- if $bucketName -}}`, whose helper `lookup`s the
BucketClaim's `.status.bucketName`. That BucketClaim is created by the
*same chart*, so on first render the lookup is empty and the Strategy is
silently skipped. Helm `lookup` only runs at install/upgrade time, so a
cluster that reached 1.5.x in one hop never grows the CR on reconcile —
a forced reconcile does not produce it.
**2. The Strategy CR exists carrying
`http://seaweedfs-s3.tenant-root.svc.cozy.local:8333`** (the static
v1.5.x `backupStorage.endpoint` default). Cozystack ships SeaweedFS with
`enableSecurity=true`, so `:8333` is a **TLS listener** — plaintext
against it fails the handshake and every snapshot dies. Fixing the
*scheme* is not sufficient: as `backupstrategy-controller.endpoint`
documents, the Etcd Strategy's S3 schema has no
`caCert`/`insecureSkipVerify`, so the self-signed in-cluster endpoint is
unusable at **any** scheme. A trusted-cert endpoint is required.
## The fix
Take the coordinates from `cozy-backups-creds` instead. The projector
writes it from the **COSI-provisioned bucket's system credentials** —
bucket provisioning, not chart values — so it describes the live bucket
regardless of which version rendered the charts. It carries `endpoint`,
`bucketName`, `region` and `forcePathStyle` alongside the AWS keys, and
`resolve_platform_backup_args` already read this Secret to validate
those keys, so preferring it adds no new dependency.
Both modes disappear: mode 1 because the CR is no longer needed, mode 2
because its endpoint is ignored.
### Classifying COSI vs. external S3
The projected endpoint is a bare host fronted by the always-TLS S3
ingress, so on the platform-managed path we force `https://` — exactly
what `backupstrategy-controller.endpoint` does. But the admin-managed
external-S3 case (`provisionBucket: false`) must keep
`.Values.backupStorage.endpoint` verbatim, since it may legitimately be
plaintext against a private store. The two therefore have to be told
apart.
**Endpoint presence cannot do that.** `ProjectBackupCredentials`
substitutes `BACKUP_STORAGE_ENDPOINT` when the source Secret is silent
and then *fails loud* rather than projecting an endpoint-less Secret, so
a projected `endpoint` key is **always** present and **always**
scheme-stripped — for external S3 too. Classifying on its absence would
force `https://` onto a plaintext external store and never consult the
CR, which is a regression against a supported, documented configuration.
Instead the hook asks whether the bucket the credentials name is one
**COSI actually provisioned**: it matches the projected `bucketName`
against `.status.bucketName` of any BucketClaim. That is keyed on data
rather than names, so it is independent of `backupStorage.namespace` /
`.bucketName` / `bucketNameOverride` — all supported Package-CR
overrides the hook cannot see — and a claim-name lookup would
misclassify a renamed bucket as external.
Resolution is then:
| cluster state | endpoint | other coordinates |
|---|---|---|
| a **live** BucketClaim claims the bucket | projected host, forced
`https://` | projected Secret |
| **no** claim claims the bucket (external S3) | Strategy CR, scheme
verbatim | Strategy CR |
| only a **Terminating** claim claims it | refuse | — |
| BucketClaim API unreadable | refuse | — |
On the external path every coordinate comes from the CR, never a
Secret/CR hybrid: `strategy-etcd-default.yaml` pairs `bucket` with
`credentialsSecretRef: cozy-backups-creds`, so CR coordinates plus the
projected credentials is exactly the pairing the cluster's real etcd
BackupJobs already use.
### Deliberate behaviour changes
Two states now refuse where `main` proceeded. Both are cases where the
classifier cannot read its input, and adopting live etcd is
irreversible, so it fails closed with an actionable message rather than
guessing:
- **A Terminating BucketClaim on the projected bucket.** Reachable via
COSI → external reusing a retained bucket name, where the
`bucketclaim-protection` finalizer wedges the corpse permanently. Both
readings are reachable and guessing wrong in either direction also ends
in a block — just with a confusing TLS/handshake error instead of a
precise one. Remedy: let COSI reap the claim, or clear its finalizer,
then re-run (migration 50 is idempotent).
- **A sustained BucketClaim API read failure** on a cluster that has
COSI installed. A genuinely absent CRD is still a definitive, zero-cost
"external" answer and costs no retries.
### The escape hatch is now reachable
The script has always supported `ETCD_ADOPT_SKIP_BACKUP=1` for clusters
that intentionally have no backup storage, but `migration-hook.yaml`
passed no such variable, so the failure paths advised an action the
platform made impossible. It is now plumbed as
`migrations.etcdAdoptSkipBackup`, an explicit opt-in defaulting to off,
and the operator-facing messages name that knob rather than a variable
nobody can set. It also unblocks the two refuse states above.
The template normalises to the exact string the script matches: a bare
bool renders `true`, which the script's `= "1"` test would accept into
the Job spec and then silently ignore — the worst outcome for a valve
reached for under duress. Anything unrecognised resolves to `"0"`, so
taking the hatch requires an affirmative spelling and never a typo. A
Platform Package predating the key renders cleanly as off.
The script's own check was widened alongside it to accept `1|true|yes`
case-insensitively, so a hand-run image honours what an operator
actually types. That is a behaviour change beyond plumbing, and it has a
cost worth naming: if the template's normalisation were ever dropped, a
raw bool would previously have failed safe (snapshot taken) and now
fails unsafe (snapshot skipped). The rendered value is pinned by a unit
test to prevent that.
`migration-hook.yaml` had no test coverage at all before this: the Job
renders only when a `lookup` of `cozystack-version` reports a version
below target, and `lookup` returns nil under `helm template`, so lint
and CI never rendered the Job. That is structurally why an unreachable
escape hatch shipped unnoticed. The new suite mocks the ConfigMap so the
Job is rendered and its env asserted by value.
## Tests
The bats fixture modelled `cozy-backups-creds` as carrying only the AWS
keys, and test 1 asserted the broken plaintext endpoint **as expected
output** — written against the implementation rather than the
requirement, against a mock that did not match a real cluster, which is
why this shipped green. The fixture now models what the projector and
COSI really produce.
Added, each verified to fail before the corresponding fix and pass
after:
- platform path derives `https://` from the projected coordinates, and
the in-cluster host never appears
- absent Strategy CR still resolves (mode 1)
- external S3 keeps the CR endpoint's scheme verbatim
- external S3 takes **every** coordinate from the CR, never a Secret/CR
hybrid
- overridden bucket coordinates still classify as COSI
- a large multi-tenant claim list still classifies correctly (the match
must not depend on list size or position)
- a Terminating claim on the projected bucket refuses, and never scales
or adopts
- an unreadable BucketClaim CRD / list refuses
- a live claim wins over an unrelated Terminating one
- COSI keeps the projected bucket even when the CR names another
- the skip-backup hatch is honoured, and an unrecognised value does not
skip
Suite: 15 → 28, plus a new helm-unittest suite covering the hook's
rendered env (chart: 81 → 88 tests).
## Validation
Reproduced end-to-end on a throwaway 3-node stand (fresh v1.4.5 → v1.5.3
→ this branch's build). Before: mode 1, version stamp stuck,
`cozy-backups` Bucket Ready, `backupstrategy-controller` Ready, creds
projected, **zero** Strategy CRs. After: the stamp advanced and the hook
completed, with endpoint, bucket, region and path-style all derived from
the projected Secret and no Strategy CR in existence. Failure was safe
in both cases — it refuses before any mutation: etcd stayed 3/3 Running,
the legacy CR un-adopted, no scale-down. The derived `https://` endpoint
reproduces the 1.6 chart helper's own output byte-for-byte, and the
projected `bucketName` equals the BucketClaim's `.status.bucketName` on
a live stand.
## Follow-ups this PR deliberately does not do
- **Fix the `if $bucketName` lookup race** in
`strategy-etcd-default.yaml`, so the Strategy exists at all on affected
clusters. This PR makes migration 50 independent of it, but the missing
CR likely means **default etcd backups are silently broken on 1.5.x
clusters**, upgrade or not — the backup e2e is `.disabled`, so CI cannot
see it. Possibly related to #3256. Worth its own issue.
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **New Features**
* Backup destination for migration 50 adoption can now derive from
projected credentials when available.
* Added `migrations.etcdAdoptSkipBackup` to control whether the
migration job skips pre-adoption snapshot safety checks.
* **Bug Fixes**
* Improved COSI vs external S3 classification and endpoint
normalization; clearer failures when destination can’t be resolved.
* `ETCD_ADOPT_SKIP_BACKUP` now honors true/false and rejects
unrecognized values (does not skip by accident).
* **Tests**
* Expanded end-to-end coverage for projected vs external S3 behavior,
precedence rules, and non-destructive failure scenarios.
* Added migration hook tests validating `ETCD_ADOPT_SKIP_BACKUP` env-var
mapping and defaulting.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
myasnikovdaniil
|
aa707f17b5 |
test(etcd): stop losing the metrics probe's output to the attach race (#3350)
## What this PR does The etcd suite's metrics contract failed in CI with `metrics endpoint did not return 200 over http on the 'metrics' port; got: If you don't see a command prompt, try pressing enter.` The probe runs a one-shot curl Pod and reads a `code=<status>` marker back off its output via `kubectl run --rm --restart=Never --attach`. `--attach` only establishes its connection once the Pod reaches Running, so a container that exits in milliseconds — which is what curl against a Pod IP does — writes its stdout into the void, and `--rm` then destroys the Pod carrying the log that still held it. The step reports the metrics endpoint as broken on a cluster that was serving it correctly throughout. The evidence that this was output loss rather than an endpoint failure is that the captured output contained no `code=` at all. A genuine connection failure would still have printed `code=000` from `-w`, plus curl's own error text from `-fsS`, and both were being captured. Meanwhile the probe Pod's events show a clean run, all three etcd members were up without restarts, and every preceding assert in the suite passed. The fix drops `--rm`, re-reads the Pod log when the attach stream carried no marker, and deletes the Pod explicitly afterwards. This is the same treatment `hack/e2e-chainsaw/_lib/talos-image-cache.sh` already applies to the identical race — that probe documents it inline; the etcd one was simply never given it. A `--pod-running-timeout` is set for the same reason it is there: the probe image is used nowhere else in the tree, so it pays a cold pull on every run and kubectl's one-minute default was a second way to end up with no marker. The recovery cannot turn a real failure green. The marker encodes the outcome, so a `code=503` recovered from the log still fails the step; an attach stream that already carried a marker is never second-guessed; and the pre-run delete blocks, so a stale Pod's log cannot be read in place of this run's. Verified by running the probe's exact curl invocation against a 503 and against a dead port: the first yields `code=503`, the second `code=000` plus curl's error, and both fail. The probe still runs exactly once — this is output recovery, not a retry. Helper extracted to `hack/e2e-chainsaw/_lib/etcd-probe.sh` with unit coverage in `hack/etcd-probe_test.bats`. ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved etcd metrics probe reliability by recovering probe results when attached output is incomplete. * Preserved meaningful probe and Kubernetes error details, including non-success status codes. * Improved probe pod timing and cleanup to reduce intermittent end-to-end test failures. * **Tests** * Added coverage for successful recovery, failed probes, missing output, and log retrieval errors. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
myasnikovdaniil
|
dc3929373b |
test(kubernetes): size the e2e tenant apiserver to the chart default (#3351)
## What this PR does The `kubernetes-previous` suite failed with `timed out waiting for the condition on helmreleases/kubernetes-test-previous-version-csi`, and the reported cause looked like a dependency problem — `csi` waiting on `vsnap-crd`, `coredns` waiting on `cilium`. It was neither. The cluster snapshot taken at failure shows the tenant `kube-apiserver` containers terminated with exit code 137 within about two minutes of each other: one killed for failing its liveness probe as reclaim stalled it, the other OOMKilled outright, both against `limits.memory: 512Mi`. The e2e fixture pinned the tenant control plane to the legacy `small` preset — 1 CPU and 512Mi — while the chart ships `c1.medium`, 2 CPU and 2Gi, as the default for this field. A two-node tenant cluster running the full addon set opens enough watches to cross 512Mi once the workers join, so the apiserver restarts, and every in-flight HelmRelease that is polling a DaemonSet rollout burns its entire ten-minute budget while the control plane is unavailable. That is why two unrelated releases, `csi` and later `ingress-nginx`, failed with the same signature while the Deployment-based releases that had installed earlier succeeded. The `DependencyNotReady` lines are aggregated first-occurrence events — `vsnap-crd` had in fact installed successfully nine minutes before cilium did. This is a defect in the fixture rather than in the product: users on defaults get four times the memory. The default was raised deliberately in v0.40.4, "to ensure more reliable operation under higher workloads and prevent resource constraints"; the fixture was left behind on an unexplained downward override. Setting it to the chart default makes the test exercise a control plane sized the way a real one is. No timeout is widened and no retry is added, since either would hide the cause rather than remove it. The sibling OIDC fixtures keep the smaller preset, correctly — they run a single replica with no workers, where this failure mode cannot occur. Suites are executed serially, so the additional memory applies to one tenant cluster at a time. What this does not prove is that 2Gi is empirically sufficient; that is the value the chart itself ships, so if it still proves too small, the finding belongs to the product default rather than to this fixture. ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Updated the Kubernetes test environment’s control plane resource allocation to improve reliability and reduce out-of-memory errors and timeouts. * **Documentation** * Added guidance explaining the resource selection and previous failure conditions. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Myasnikov Daniil
|
efa85a4f61 |
feat(platform): make the etcd-adoption skip-backup hatch reachable
Migration 50 has honoured ETCD_ADOPT_SKIP_BACKUP since it was written, and its failure paths tell operators to "re-run with ETCD_ADOPT_SKIP_BACKUP=1". But migration-hook.yaml passes only NAMESPACE/CURRENT_VERSION/ TARGET_VERSION, and the Job is a Helm hook the chart re-renders, so there has never been a way to set it. The script advertised an escape the platform made impossible. The endpoint-classification work landing alongside this makes that worse: it adds refuse-rather-than-guess paths, so more clusters can now legitimately stop here. Plumb it through migrations.etcdAdoptSkipBackup, default false. Skipping the pre-adoption snapshot rewires ownership of LIVE etcd storage with no way back, so it stays opt-in and deliberate — the flag is the last resort for a cluster that has nothing to fix, not the first response to a refusal. Render "1"/"0" rather than the bare bool. The script matches the literal string "1", so a bool would render "true", sail through the Job spec looking correct, and be ignored — a safety valve that is set but silently disregarded is worse than one that was never offered, because the operator believes they opted in. The value is normalised through a string, so a bool, a quoted "true" out of the Package CR's Values JSON, or a numeric 1 all land on the same answer, while nil or garbage resolves to "0": taking this hatch must require an affirmative spelling, never a typo. Resolved with `dig` so a Platform Package predating the key renders "0" instead of failing the render of the entire platform chart and bricking every upgrade. Widen the script's own check to accept 1/true/yes as well. The template guarantees the GitOps path, but an operator running this image by hand will reasonably type "true", and that is exactly the moment they are already stuck. The two layers cover independent failure modes. Emit the var unconditionally, including the "0" default, so the rendered Job states the cluster's safety posture outright rather than leaving an operator to diff values to find out whether a snapshot was skipped. Point the failure messages at the values path that actually exists, and describe it honestly: editing the Platform Package and letting Flux re-render is a real operation, not a one-liner, and the flag has to be reverted afterwards or the next etcd migration skips its snapshot too. Fixing the condition the migration names remains the recommended path — the migration is idempotent, so re-running after a fix is safe. templates/migration-hook.yaml had no coverage at all, because the Job only renders when a lookup of the cozystack-version ConfigMap reports a version below targetVersion, and lookup is nil under helm template. The new suite mocks that ConfigMap and asserts the RENDERED VALUE — enabled, disabled, key absent, stringified true, garbage, and the pre-existing three env vars — rather than merely that the variable exists, which is what let the trap above go unnoticed in the first place. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
0ba5337526 |
fix(migrations): key the etcd-adoption endpoint scheme off the claimed bucket
Deriving the safety-snapshot target from the projected cozy-backups-creds Secret is right, but detecting the admin-managed external-S3 case (provisionBucket: false) by "the projected endpoint is absent" is not: that state does not exist. The projector substitutes BACKUP_STORAGE_ENDPOINT and fails ReasonSourceMalformed when both sources are empty, so a projected Secret always carries a non-empty, scheme-stripped endpoint. So for provisionBucket: false against a plaintext external S3 the projected endpoint is a bare host, the script saw it non-empty, forced https:// and never consulted the Strategy CR — a regression against main, which read the CR verbatim and passed http:// through. Being a pre-upgrade hook with no ETCD_ADOPT_SKIP_BACKUP plumbed through migration-hook.yaml, that is a total upgrade block with no operator escape. Classify on data instead: match the projected bucketName against the .status.bucketName of any BucketClaim. The COSI driver assigns that name and the projector republishes exactly it (packages/system/bucket user-credentials.yaml reads BucketInfo spec.bucketName), so a match means the creds describe a COSI bucket => the host is the always-TLS S3 ingress => force https, as the 1.6 chart helper does. No match means external S3. Keying on data rather than on the claim's name and namespace matters twice. backupStorage.namespace/.bucketName are supported Package-CR overrides that this hook cannot see, so a name lookup would miss a renamed bucket and fall back to the v1.5.x plaintext CR — the original P0. And a BucketClaim wedged Terminating (its cosi bucketclaim-protection finalizer outlives an uninstalled COSI controller) keeps answering a name lookup while its status names some other bucket. Split the two paths by which source is authoritative. On COSI the projected coordinates win: the bucket is the COSI-assigned name the CR can only reproduce through a live lookup that may never have run. On external S3 the Strategy CR wins WHOLESALE, not just for the endpoint — the CR renders backupStorage.* and points at this same cozy-backups-creds, so CR coordinates plus these credentials is exactly what the cluster's own BackupJobs already use, and the bucket an operator will look in. Keeping the Secret's bucket while taking the CR's endpoint assembled a pairing nothing else produces: the projector copies bucketName straight from the admin-managed source Secret and never from backupStorage.bucketName, so the two can disagree outright and the snapshot could land in a bucket the platform never writes. Relying on the CR is safe on this path alone, because bucketName short-circuits to values before the BucketClaim lookup that races on the COSI path, so the CR is guaranteed present. Match with a single jq `any`, never `jq -r | grep -q`: grep exits on the first match and SIGPIPEs jq, and pipefail reports jq's 141 rather than grep's 0, so a MATCH returns as a failure and the cluster is misclassified external. It is a race on the list outgrowing one stdio write (~8KB) with only a last-line match safe, so it strikes exactly the big multi-tenant clusters that can least afford it, and backoffLimit 3 can decide differently on each retry. Measured with the match first, 20 runs each: 300 claims went 0/20 correct (rc=141 every time) and 2000 likewise, while `any` is 20/20 at 3, 300 and 2000. Refuse rather than guess when the answer is not knowable. Three states qualify: the BucketClaim API is unreadable; the list is not interpretable (no producer emits that, but without a shape gate it reads as "no claim" = external = the plaintext P0, silently); or the bucket is claimed ONLY by a Terminating claim. That last one is ambiguous — an admin who moved COSI -> external S3 reusing the bucket name, versus a still-COSI cluster whose claim is mid-recreate — and both readings fail as the same confusing handshake error when wrong, so a guess buys no reliability while reading it as "external" specifically resurrects the plaintext-:8333 P0. A live claim still wins over an unrelated Terminating one, so healthy clusters are unaffected. Each refusal names the condition and the operator action; the migration is idempotent, so re-running after resolving it is the remedy. Behaviour change worth noting: a cluster on external S3 that still has COSI installed now refuses on a sustained BucketClaim-API failure where main would have proceeded off the Strategy CR. That is the intended trade — main proceeded only because it never had to tell the two apart — but it does mean an apiserver outage can now block this hook where it previously did not. ETCD_ADOPT_SKIP_BACKUP is still not plumbed through migration-hook.yaml, so the messages deliberately point at resolving the condition rather than at an escape hatch the operator cannot reach. Force the scheme only onto a non-empty host: an absent endpoint must degrade to the CR as main did, not to a literal "https://" that would pass the final non-empty guard and reach etcd-migrate as a destination. The old test modelled external S3 by dropping the projected endpoint entirely — a state the projector cannot produce — so it green-lit the regression. It now models what the projector really writes, and the suite pins the stale-claim, overridden-coordinates, empty-endpoint, 300-claim-multi-tenant, Terminating-match, hybrid-coordinate and unreadable/uninterpretable API cases. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
9ceccd2b07 |
fix(migrations): derive the etcd-adoption snapshot target from the projected bucket creds
Migration 50 is a pre-upgrade hook, so every chart-rendered resource it reads still belongs to the version being upgraded FROM. It read the snapshot target from the live cozy-default-etcd Strategy CR, which on a v1.5.x cluster is broken in one of two ways — and the platform upgrade is gated behind this snapshot, so neither can self-heal first: 1. Absent. strategy-etcd-default.yaml is guarded by `if $bucketName`, whose helper looks up the BucketClaim status. That BucketClaim is created by the same chart, so on first render the lookup is empty and the Strategy is silently skipped. Helm lookup only runs at install/upgrade time, so a cluster that reached 1.5.x in one hop never grows the CR on reconcile. 2. Present, carrying the static v1.5.x default http://seaweedfs-s3.tenant-root.svc.cozy.local:8333. Cozystack ships SeaweedFS with enableSecurity=true, so :8333 is a TLS listener and plaintext against it fails the handshake. Fixing the scheme alone is not enough: the Etcd Strategy's S3 schema has no caCert/insecureSkipVerify, so the self-signed in-cluster endpoint is unusable at any scheme and a trusted-cert endpoint is required. Either way migration 50 hard-fails and blocks the upgrade, and the documented ETCD_ADOPT_SKIP_BACKUP escape hatch cannot be reached — migration-hook.yaml passes only NAMESPACE/CURRENT_VERSION/TARGET_VERSION. Take the coordinates from cozy-backups-creds instead. The projector writes it from the COSI-provisioned bucket's system credentials — bucket provisioning, not chart values — so it describes the live bucket regardless of which version rendered the charts, and it carries endpoint, bucketName, region and forcePathStyle alongside the AWS keys. resolve_platform_backup_args already read this Secret to validate those keys, so preferring it adds no new dependency. The endpoint is a bare host fronted by the always-TLS S3 ingress, so force https://, exactly as the 1.6 chart helper backupstrategy-controller.endpoint does. The Strategy CR remains the fallback for the admin-managed external-S3 case (provisionBucket: false), where .Values.backupStorage.endpoint is authoritative and may legitimately be plaintext against a private store; that path is detected by the absence of a projected endpoint and its scheme is preserved verbatim. The bats fixture modelled cozy-backups-creds as carrying only the AWS keys, and test 1 asserted the broken plaintext endpoint as expected output — the test was written against the implementation rather than the requirement, which is why the defect shipped green. The fixture now carries the coordinates the real projected Secret has, test 1 asserts the derived https endpoint, and two cases are added: an absent Strategy CR still resolving, and external S3 keeping the CR endpoint verbatim. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Aleksei Sviridkin
|
44b221d772 |
test(kubernetes): size the e2e tenant apiserver to the chart default
The tenant Kubernetes e2e pinned controlPlane.apiServer.resourcesPreset to the legacy "small" preset, capping the apiserver at 1 CPU / 512Mi -- a quarter of the memory the chart itself ships as the default (c1.medium, 2 CPU / 2Gi). 512Mi does not hold a two-node tenant cluster running the full addon set. Once the workers join, cilium, coredns, metrics-server, csi, ingress-nginx and VPA all establish watches at roughly the same moment, and the apiserver crosses the limit: one replica is killed for failing its liveness probe as reclaim stalls it, the other is OOMKilled outright. Both replicas restart within about two minutes of each other, so any tenant HelmRelease that is mid-install and waiting on a DaemonSet rollout observes 'InProgress' for its entire timeout and fails, then re-enters a fresh install on remediation. Which release loses is a matter of timing rather than of the release, which is why the failure moved between the csi, ingress-nginx and cilium releases and looked like an unrelated dependency problem each time. Use the chart default so the test exercises a control plane sized the way a real one is, instead of one held at a quarter of the shipped default. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
3a82ed43bc |
test(etcd): stop losing the metrics probe's output to the attach race
The metrics contract runs a one-shot curl Pod and reads a `code=<status>` marker back through `kubectl run --attach`. Attach connects only after the Pod reaches Running, so a container that exits before the connection is up writes its stdout into the void: the step then sees only kubectl's own "If you don't see a command prompt, try pressing enter." and reports the metrics endpoint as broken. curl against a Pod IP finishes in milliseconds, so losing that race is routine, and `--rm` compounded it by deleting the Pod the instant it exited, taking the log that still held the real output. The result was a false negative on a healthy cluster, non-deterministic in the way that reads as a flake because whether attach wins depends on Pod startup timing rather than on anything under test. Drop `--rm`, keep the Pod until its result is in hand, and re-read the log when the attach stream carried no marker. The recovery cannot mask a real failure: the marker encodes the outcome, so a recovered `code=503` stays a failure, and a stream that already carried a marker is never second-guessed. The pre-run delete stays blocking so the re-read cannot find a previous incarnation's marker, and the post-run delete is guarded so a transient API error cannot abort the step with the result in hand but unreported. Pin `--pod-running-timeout` at 90s rather than leaning on kubectl's 1m default. curlimages/curl is not otherwise used on these nodes, so the probe pays a cold pull, and a pull that outruns the budget produces the same markerless output this change exists to explain. The resolver is split into a sourced helper so the recovery is unit-tested without a cluster; the kubectl orchestration around it stays covered by the e2e run. The image-cache probe carries the same recovery inline for the same race. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
1e4a152291 |
test(mariadb): give first boot the readiness budget the probe grants
The suite allowed 3 minutes for a MariaDB endpoint address to appear, which was set when a replica had 40 seconds to finish its first boot. The startup probe now grants 310 seconds, so a healthy install can legitimately take longer than the gate allows and would be failed while the probe is still willing to wait. The endpoint assert is the gate that matters here. The chart hands the operator a MariaDB custom resource and ships no built-in workload, so helm has nothing to wait on and the HelmRelease reports Ready as soon as the manifests are applied, long before a server exists. An endpoint address, by contrast, appears only once a replica has passed its startup and then its readiness probe, so that assert is what actually covers bootstrap. Raise it to 10m in both the replicated and the single-replica test. One ready address satisfies the assert, so it has to clear a single startup budget rather than one per replica, plus the operator reconcile, PVC bind and image pull that precede it. The HelmRelease ceilings stay at 6m, since nothing about them gates on the server coming up. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Andrey KolkovandClaude Fable 5
|
8282c17d08 |
test(sdn): scope the new validate test's comment to what it proves
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey KolkovandClaude Fable 5
|
bbc0103949 |
test(sdn): pin the fail-closed suite-mapping contract in validate
Scrub the last "-" mention from the --validate usage doc (the escape no longer exists, so the header asserted the opposite of the behavior), and add the negative bats case the tightened branch was missing: a suite whose hardcoded mapping names a PackageSource absent from the graph must fail validation with the "maps to ..., not a PackageSource" error. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey KolkovandClaude Fable 5
|
4e9a40d811 |
test(sdn): drop the now-unreachable "-" mapping from suite_to_source
With the serviceexposure case gone no suite returns "-", and the only executable coverage of that branch was the bats test removed alongside it. Keeping an untestable dead branch for a hypothetical future suite is not worth it — re-adding the three lines with the next core-platform suite is cheaper, and validate/closure both simplify. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |