mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-03 10:37:21 +09:00
docs/system-memory-limits
1235
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Aleksei Sviridkin
|
9dc49de1c9 |
fix(apps): make the post-delete cleanup hooks report failure (#3593)
## What this PR does The qdrant and harbor post-delete cleanup hooks could not fail. Every delete was guarded by `|| echo "WARNING: ..." >&2`, neither script set `-e`, and both ended on an unconditional completion message, so the container exited 0 on every path. Helm read that as a successful hook, `helm.sh/hook-delete-policy: hook-succeeded` removed the Job together with the pod that held the warning, and an uninstall that reclaimed nothing looked exactly like one that had nothing to reclaim. Written up in #3566. Both hooks now count failures and set the exit code at the end. `set -e` would abort at the first failed delete and skip the rest of the sweep, which is the opposite of what is wanted: every remaining object should still be attempted and the outcome reported once. Already-gone objects stay a success, since every delete keeps `--ignore-not-found`. Each failure message names the object it left behind. `packages/apps/mariadb` already uses this shape. Both hooks also consumed `kubectl get` output directly: harbor by iterating `$(kubectl get ...)`, qdrant by piping it into `grep`, whose status the pipeline then reports. A failed listing arrived as an empty list, and the sweep reported success without having looked. Each listing is now captured before it is used. #3566 attributes that shape to harbor's two loops alone. qdrant has it too: `pvcs=$(kubectl get pvc ... | grep -E ... || true)` reports grep's status, so a `kubectl get` that failed left `$pvcs` empty and the hook announced "no data PVCs found; nothing to delete". Both instances are fixed here. ### The cost A hook that exits non-zero keeps its Job and its pods, because `hook-succeeded` does not fire for it. That is the point, the pods hold the log, but it means a failed uninstall now leaves objects behind where it previously left none. Nothing collects them on its own: `before-hook-creation` only fires when the hook runs again, and a `post-delete` hook runs only on an uninstall, so they stay until a release of that name is installed and uninstalled again, or until the namespace goes. The release itself is not stranded. Helm appends the post-delete hook error to its error list instead of returning on it, marks the release uninstalled, purges it from storage anyway, and only then reports `uninstallation completed with N error(s)`. The failure surfaces once and the app still goes away. Both Jobs get one retry instead of the Kubernetes default of six, and instead of none. Every delete is idempotent, so a second attempt re-issues an API call rather than re-running any logic, and one short backoff separates a transient apiserver error from a real one. These failures become visible for the first time with this change, and a signal that fires on noise stops being read, including on the day it is right. Six is the far end of the same mistake: ten minutes of backoff across seven pods outlasts the hook timeout, so Helm reports that timeout instead of the hook's own error. `packages/apps/clickhouse` and `packages/apps/mariadb` already exit non-zero from their cleanup hooks and set no retry limit, so a real failure in either spends the full backoff and reaches the operator as a timeout rather than as the error the hook wrote. Adding the exit code without the cap reproduces that. One limitation is deliberate. harbor's ACME sweep cannot tell "this kind is not served" from "I could not look". If cert-manager is torn down before the release, `kubectl get challenge` fails and the uninstall is reported failed with nothing to sweep. cert-manager is a hard dependency of the bundle harbor ships in, so the window is narrow, and a CRD-existence probe would add a second silent-skip path of its own: it would swallow a discovery failure the way this change stops swallowing a listing failure. ### Tests `hack/app-cleanup-hooks_test.bats` renders each hook's script out of its chart and runs it against a fake `kubectl`, because the property under test is the container's exit code and matching against manifest text cannot establish it. There are eight failure paths across the two hooks, six in harbor and two in qdrant, and each has a case that goes red when that one path alone is reverted to a swallowed warning. Also covered: a clean delete, and an object that is already gone in both its shapes, an empty listing and a `NotFound` on the delete itself. The fake `kubectl` models `--ignore-not-found` instead of exiting 0 unconditionally, so dropping that flag from a hook turns the already-gone cases red. Message assertions are anchored on the hook's own `ERROR: ` prefix, because the fake names the object in its own diagnostic too, and an unanchored match is satisfied by the fixture. Two manifest fields are pinned in each chart's existing helm-unittest suite, where manifest fields belong. The retry cap is one. The other is `helm.sh/hook-delete-policy`: adding `hook-failed` to that annotation hands the failed Job straight back to Helm to delete, which undoes the whole change. I checked that before adding the assert, and the reversal left both suites fully green. The qdrant Chainsaw suite explained its post-delete pod collector by quoting the guard string this change removes, and concluded that Helm always collects the pod away. That conclusion inverts here, so the note is rewritten, along with the sentence in the harbor suite that points at it. ### Screenshots Not a UI change. ### Downstream repositories I walked the trigger map in `docs/agents/contributing.md` against the diff, file by file. It touches two app chart templates and adds one file under `hack/`. No package added, renamed or removed, no `values.schema.json`, no `values.yaml` default, no version enum, no `ApplicationDefinition`, no namespace, no node prerequisite, and neither `hack/package.mk` nor `hack/update-crd.sh`. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note fix(apps): the qdrant and harbor post-delete cleanup hooks now exit non-zero when a delete or a listing fails, instead of reporting success on every path. `helm uninstall` reports the failure rather than returning clean, and the hook Job and its pods stay in the namespace so the log survives to be read. Nothing collects them on its own: they remain until a release of that name is installed and uninstalled again, or until the namespace is removed. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved Harbor and Qdrant cleanup reliability by detecting listing and deletion failures. - Cleanup jobs now retry once and report failures with a non-zero status. - Cleanup continues processing remaining resources after individual failures. - Successful cleanup remains safe and repeatable, including when resources are already missing. - Harbor cleanup remains limited to resources associated with the configured Harbor host. - **Tests** - Added comprehensive coverage for successful, repeated, partial, and failed cleanup scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
3d4e8c07e9 |
test(tenant): anchor the operator-gate absence and refresh a spent reason (#3661)
## What this PR does Two tests in `packages/apps/tenant/tests/vmo_webhook_dependson_test.yaml` say the seaweedfs and gateway HelmReleases deliberately carry no operator gate, and assert that with `notExists: spec.dependsOn` and nothing else. A bare absence assert also passes when the template renders no document at all, so both stayed green with those two releases switched off entirely. With `seaweedfs: false, gateway: false` the suite still reported 5 of 5 passing; pinning `metadata.name` next to the absence turns the same run into 2 failures. An earlier check on these asserts injected a `dependsOn` and watched them redden. That proves they catch an added gate, which is the other direction. For a negative assert the direction that matters is the document going away, and that one was never covered. Pairing `equal: metadata.name` with the absence is not a new idiom introduced here. `no_upgrade_force_test.yaml`, in the same directory, already anchors all seven of its releases that way, and this brings the last two suites in the chart into line with it. Both suite headers also carried a reason for their `templates:` list that has expired: that rendering the whole chart fails because `keycloakgroups.yaml` indexes `.Values._cluster` unguarded. #3636 added the guard, so a no-values render of the chart now succeeds. The list is still load-bearing, for the reason `no_upgrade_force_test.yaml` already records — it keeps an unrelated template breaking out of this suite's result. Checked by adding a nil dereference to a template neither suite asserts on: both suites stay green with their lists and error without them. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map against the diff. It touches two files, both under `packages/apps/tenant/tests/`, and no chart, template, values file or tooling. Nothing in the map keys on test files. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note NONE ``` |
||
|
Aleksei Sviridkin
|
c681ae1243 |
test(tenant): anchor the operator-gate absence and refresh a spent reason
The two "carries no operator gate" tests asserted only `notExists: spec.dependsOn`. A bare absence assert passes on a template that renders no document, so toggling `seaweedfs` and `gateway` off left both green while the releases they guard were gone. Measured: all 5 tests passed with the templates rendering nothing, and 2 of them fail once the name is pinned alongside the absence. An earlier check on these two asserts injected a `dependsOn` and watched them redden, which proves only that they catch an added gate. That is the other direction; the one that matters for a negative assert is the document disappearing. Both suite headers also justified their `templates:` list with a reason that has expired: keycloakgroups.yaml now guards its `.Values._cluster` index, so a no-values render of the whole chart succeeds. The list is still load-bearing, for the reason the sibling suite already records. It keeps an unrelated template out of this suite's result: with a nil dereference added outside the guard of a template neither suite asserts on, both suites stay green with their list and error without it. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
e9411d6b8c |
fix(apps): make the post-delete cleanup hooks report failure
The qdrant and harbor post-delete hooks guarded every delete with `|| echo "WARNING: ..." >&2`, set no `-e`, and ended on an unconditional completion message, so the container exited 0 on every path. Helm read that as a successful hook and the hook-succeeded delete policy removed the Job together with the pod that carried the warning, leaving an uninstall that reclaimed nothing indistinguishable from one that had nothing to reclaim. Count the failures instead and set the exit code at the end, so a failed delete does not skip the deletes that follow it. An object that is already gone stays a success: every delete keeps --ignore-not-found, which exits 0 in that case. This is the shape packages/apps/mariadb already uses for the same problem. Both hooks also consumed the output of `kubectl get` directly -- harbor by iterating it, qdrant by piping it into grep, whose status the pipeline then reports. A listing that failed therefore arrived as an empty list and the sweep was skipped rather than reported. Each listing is now captured before it is used, so a listing that failed and one that returned nothing are different events. A hook that exits non-zero keeps its Job and pods, which is the point: they hold the log. Give the Job one retry rather than the Kubernetes default of six or none at all. Every delete is idempotent, so a second attempt re-issues an API call rather than re-running any logic, and one short backoff is what separates a transient apiserver error from a real one -- these failures become visible for the first time here, and a signal that fires on noise stops being read, including on the day it is right. Six is the far end of the same mistake: ten minutes of backoff across seven pods outlasts the hook timeout, and Helm reports that timeout instead of the hook's own error. The qdrant Chainsaw suite explained its post-delete pod collector by quoting the guard string removed here and concluding the pod is always collected away. That is now backwards: a failed cleanup is exactly when the pod survives and the collector is the evidence. Rewrite the note, and the sentence in the harbor suite pointing at it. hack/app-cleanup-hooks_test.bats renders both scripts and runs them against a fake kubectl. There are eight failure paths across the two hooks -- six in harbor, two in qdrant -- and each has a case that goes red when that one path alone is reverted to a swallowed warning. Message assertions are anchored on the hook's own `ERROR: ` prefix, because the fake kubectl names the object in its own diagnostic too and an unanchored match is satisfied by the fixture. The helm-unittest suites pin the retry limit and the hook delete policy: adding hook-failed to that annotation hands the Job back to Helm to delete on failure and destroys the evidence this change exists to keep, and until these asserts that reversal left both suites green. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Andrey KolkovandClaude Opus 4.8
|
dff56aa998 |
feat(backups): add a native MongoDB (psmdb) backup-strategy driver
Adds a MongoDB (Percona Server for MongoDB / psmdb) backup-strategy driver so `apps.cozystack.io/MongoDB` reaches feature parity with the CNPG, MariaDB and ClickHouse native drivers — logical backups, restore-to-differently-named instances, and point-in-time recovery through the unified BackupJob/RestoreJob/BackupClass/Plan interface (closes #3545). The driver orchestrates the operator's PerconaServerMongoDBBackup / PerconaServerMongoDBRestore CRDs: - BackupJob: creates a one-shot PerconaServerMongoDBBackup, polls status.state, and persists the S3 destination + storage descriptor (by-reference, no credentials) onto the Cozystack Backup artifact. - RestoreJob: drives PerconaServerMongoDBRestore via backupSource (read back from the source backup status, snapshot fallback) so the same artifact restores in-place or into a differently-named instance; PITR via spec.options.pitr (date/latest). Because psmdb resolves the backup storage by name against the cluster's spec.backup.storages, the driver names the chart's storage (s3-storage) via the cozy-default-mongodb Strategy rather than injecting it, and surfaces a clear Ready=False precondition when a target has backups disabled. Wiring: - api/backups/strategy/v1alpha1: MongoDB strategy type + generated CRD/deepcopy. - internal/backupcontroller: psmdbstrategy_controller + psmdbtypes/mongodbapp partial-schema mirrors; dispatch, supported-kinds and scheme registration. - backupstrategy-controller: cozy-default MongoDB route, cozy-default-mongodb Strategy CR, and psmdb.percona.com RBAC. - apps/mongodb: backup.insecureSkipTLSVerify knob (psmdb s3 storage has no CA-bundle field) so backups can target Cozystack's self-signed seaweedfs. Tests + e2e: - unit tests for the driver (validation, ensure-idempotency, precondition, backupSource resolution, snapshot round-trip, PITR option parsing). - examples/backups/mongodb + a mongodb-2-backup-roundtrip Chainsaw suite that drives run-all.sh end-to-end (sentinel document round-trips through S3 into a restored copy while the source stays untouched), per docs/agents/e2e-testing.md. - docs/operations/backup-classes.md updated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Alexey Artamonov
|
2b5c85cde7 |
fix(clickhouse): assign DB_LIST so backup retention cleanup runs
The legacy per-instance backup script printed `echo DB_LIST=$(...)` instead of assigning DB_LIST, leaving it unset. The cleanup loop `for db in $DB_LIST` then ran zero iterations, so `restic forget` and `restic prune` never executed and snapshots accumulated in object storage indefinitely despite the configured cleanupStrategy. The Job still reported success, hiding the failure. Assign DB_LIST from the shuffled table list so the retention cleanup loop runs. Add a helm-unittest regression asserting the rendered backup.sh assigns DB_LIST and no longer contains the `echo DB_LIST=` typo. Fixes #3758 Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Andrey Kolkov
|
d0990299ed |
feat(clickhouse): let the backup sidecar trust a private S3 CA, unblocking the backup e2e round-trip (#3385)
## What this PR does Gives the ClickHouse `clickhouse-backup` sidecar a way to trust an S3 endpoint whose certificate is signed by a private CA, and uses that to make the ClickHouse backup/restore e2e round-trip actually run in CI. This actualizes #2600, whose feature code and example scripts have since landed on `main` independently, and whose remaining contribution — the e2e test — was still in the retired BATS format. #2600 can be closed once this merges. ### 1. `backup.endpointCA` on the ClickHouse chart The sidecar had **no TLS/CA surface at all**: bucket coordinates and credentials arrive as `S3_*` env, and nothing let it verify an endpoint signed by a private CA. Cozystack's own in-cluster SeaweedFS is exactly that case — it serves `:8333` behind the self-signed "SeaweedFS CA" — so a ClickHouse release could only ever back up to a publicly-trusted endpoint. MariaDB (`storage.s3.tls.caSecretKeyRef`) and Postgres (barman `endpointCA`) have had a CA field all along; this closes the gap for ClickHouse. `backup.endpointCA` takes `{name, key}`. When set, the chart mounts that Secret into the sidecar and points `SSL_CERT_DIR` at it. Empty — the default — renders no env, no mount and no volume, so existing releases are byte-for-byte unchanged. **Why `SSL_CERT_DIR` and not `AWS_CA_BUNDLE`.** Both are honoured by `altinity/clickhouse-backup:2.7.4`; I measured them against a self-signed endpoint and public AWS S3 in turn. Go reads `SSL_CERT_DIR` *in addition to* the system bundle, so both endpoints verify. `AWS_CA_BUNDLE` *replaces* the SDK's trust pool: the private endpoint verifies, and public S3 then fails `x509: certificate signed by unknown authority` — it would silently break a release that also talks to a public endpoint. `S3_DISABLE_CERT_VERIFICATION` was rejected outright, since it drops verification rather than trusting one more CA. ### 2. The e2e suite, and what it was really testing Two Tests added to the existing `hack/e2e-chainsaw/clickhouse/` suite (in name order, mirroring the `etcd`/`mariadb`/`postgres` shape — see the layout rationale below for why they live here rather than in a new directory): - **`clickhouse-1-backup-contracts`** (CI, `tenant-test`, ~98s): a backup-enabled ClickHouse asserted against the *running* cluster — the `clickhouse-backup` sidecar on the `ch-backup-api` `:7171` port, and the `<release>-backup-api-auth` Secret whose keys the sidecar's `API_USERNAME`/`API_PASSWORD` actually resolve `valueFrom`. Each is a silent break if a chart bump renames it. Needs no object store. - **`clickhouse-2-backup-roundtrip`** (CI, `tenant-root`): `Bucket` → `Altinity` strategy + `BackupClass` → source ClickHouse with a sentinel row → `BackupJob` → in-place `RestoreJob` → to-copy `RestoreJob` into a second instance, driving `examples/backups/clickhouse/run-all.sh` so test and docs cannot drift. The round-trip was originally gated behind `CLICKHOUSE_E2E_S3_ROUNDTRIP` and **skipped in CI** — its `PASS (1.01s)` meant the gate fired, not that a backup happened. `backup.endpointCA` removes the reason for the gate, so the suite now adopts the same three preconditions mariadb/postgres rely on: run in `tenant-root` (an isolated tenant's Cilium egress allowlist blocks its Pods from reaching `tenant-root`'s SeaweedFS), target the in-cluster endpoint instead of the `Bucket`'s external ingress URL (an unroutable placeholder in CI), and trust its certificate via the copied CA. It also pre-cleans with `cleanup.sh`, because `tenant-root` is shared and a leftover `Succeeded` `BackupJob` would satisfy the harness's wait without a backup ever running. Both backup Tests live in the existing `hack/e2e-chainsaw/clickhouse/` suite rather than a new suite directory, so no selector mapping is added: `hack/select-e2e.sh` already derives `clickhouse` both from `packages/apps/clickhouse/**` (the `*-application` rule) and from `examples/backups/clickhouse/**` (the harness path), and both paths therefore select the suite that actually runs the harness. That alignment is the reason for the layout — a separate `clickhouse-backup/` directory would have made an edit to the harness select a suite that cannot exercise it. `hack/select-e2e.sh` and `hack/select-install.sh` are otherwise untouched (the only change is a comment recording that invariant), and the existing `backup example harness edit selects its app suite` case in `hack/select-e2e_test.bats` covers the mapping. ### Verification - `helm unittest` — 17 pass, including 5 new `endpointCA` cases: the unset default renders nothing, the env/mount/volume wiring, a custom key still projected as `ca.crt`, composition with `useSystemBucket`, and `backup.enabled=false` renders nothing. - The CA-trust behaviour was measured against the real image, not assumed: without a CA knob the sidecar reports `x509: certificate signed by unknown authority`; with `SSL_CERT_DIR` the handshake succeeds. - `hack/select-e2e_test.bats` (16) and `hack/select-install_test.bats` (17) pass; `select-install.sh --validate` reports graph OK. - `make generate` is idempotent, so the pre-commit job's `git diff --exit-code` stays clean. - The contracts Test passed in CI on earlier runs of this branch. The round-trip only became CI-live with this work, so it still needs a green E2E run before merge — the branch has since been rebased onto current `main` to get one. ### Screenshots Not applicable — no UI changes. ### Downstream repositories Walked the trigger map against the diff, file by file. The load-bearing change is the new `backup.endpointCA` field in `packages/apps/clickhouse/values.yaml` / `values.schema.json`; everything else is `hack/e2e-chainsaw/`, `examples/backups/`, one docs paragraph, and regenerated artifacts, none of which appear in the map. - [ ] No downstream repository is affected by this change - [x] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: https://github.com/cozystack/terraform-provider-cozystack/issues/18 — "add, remove or rename a field in an app's `values.schema.json`" reaches the provider's hand-maintained schema, model and expand/flatten pair, so the new nested object is not surfaced until it is added there. Opened as an issue rather than a PR because the natural moment is alongside that repo's next `api/apps/v1alpha1` pin bump, which is a maintainer call. Not ticked, with reasons rather than silence: **cozystack/website** — the trigger is *adding, renaming or removing a package*, and this adds a field to an existing one. `clickhouse` is already in the website `Makefile`'s app list and its reference page is generated from this package's `README.md`, so the regenerated `backup.endpointCA` rows are carried by the existing release-tag automation with no manual PR. **cozystack/external-apps-example** — vendors `hack/package.mk` and `update-crd.sh`, neither of which this PR touches. **cozystack/ansible-cozystack**, **ccp**, **talm**, **cozyhr**, **cozy-proxy**, **cozystack-telemetry-server**, **examples** — no installer values, `hack/` layout, namespace, variant, node-prerequisite, annotation, label or metric in their trigger rows is touched here. ### Release note ```release-note feat(clickhouse): add `backup.endpointCA` so the clickhouse-backup sidecar can verify an S3 endpoint whose certificate is signed by a private CA, such as Cozystack's in-cluster SeaweedFS. Leave it unset for publicly-trusted endpoints; existing releases are unaffected. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added optional private S3 endpoint CA support for ClickHouse backup/restore sidecars via `backup.endpointCA`, including configurable Secret name and CA bundle key (default `ca.crt`). * Sidecars mount the CA and extend trust while leaving public endpoints unchanged. * **Documentation** * Updated ClickHouse chart docs, operational backup guidance, and backup/restore examples to cover CA setup, overrides, and cleanup. * **Tests** * Added helm-unittest and end-to-end coverage validating CA rendering/mounting behavior and ClickHouse backup/restore workflows. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
3ae1e10c92 |
docs(kubernetes): correct the registryMirrors field and README claims
State in the README that in-guest registry pulls are routable again via talos.registryMirrors, which knob covers which artifact (the mirror routes registry pulls; the worker OS disk image arrives as a raw HTTP artifact under talos.imageFactoryURL and no registry mirror touches it), and the limits: skipFallback is needed for air-gap, machine.registries.config is not passed through, and existing nodes keep their boot-time config until replaced. Trim the field doc comment. The regenerated schema, README tables and cozyrds openAPISchema follow from those sources. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
3a1292d682 |
fix(kubernetes): escape tenant values in the worker reconcile heredoc
The worker TalosConfigTemplate is applied from an unquoted `cat <<EOF | kubectl apply` heredoc, so every free-form value interpolated into it is subject to shell parameter expansion and command substitution at Job runtime. Escape backslash, dollar and backtick on talosVersion, installerRepository, schematicID and the registryMirrors block so each renders as a literal, and record the invariant above the data block in both charts: a new free-form field here must be escaped or render-time pattern-validated, or it reopens command injection into a host-cluster Pod. Escaping covers every byte by construction; the chain is a no-op for ordinary values, so the default render and its content-hash Job name are unchanged. Tests render both charts and run the heredoc through a real shell with hostile values, which a rendered-string regex cannot do, and the default-omits-mirrors assertion selects the Job by kind rather than by document index so it cannot pass against the wrong document. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
IvanHunters
|
58910c0edc |
feat(kubernetes): route tenant worker image pulls through a registry mirror
Add a talos.registryMirrors passthrough to the kubernetes and kubernetes-nodes
charts that renders into the worker TalosConfigTemplate machine.registries.mirrors,
so an operator can point a registry such as ghcr.io at an in-cluster pull-through
mirror and a worker boot no longer depends on live public egress. Default {}
keeps the rendered worker reconcile Job byte-identical to before, content-hash
Job name included. Free-form mirror values are shell-escaped for the unquoted
apply heredoc, covered by an execution-level bats test.
Also add the e2e ghcr-mirror (registry:2 pull-through plus a Cilium egress
policy) and wire it into the kubernetes chainsaw suites, so CI exercises the
mirror path and takes the flaky public ghcr.io kubelet-image pull off live
egress.
Refs #3513 (addresses the egress variant of the tenant-worker node-join
failures; the dominant variant is CI-runner CPU starvation, tracked in the
issue).
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
|
||
|
Aleksei Sviridkin
|
92ec49e64e |
fix(tenant): retry failed monitoring and gateway installs in place (#3633)
<!-- 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 The tenant chart renders monitoring and gateway with no install strategy, so they get Flux's default, and install remediation is an uninstall in every case Flux offers. Neither sets `spec.uninstall.disableHooks`, which defaults to false. So a release that missed nothing but a readiness budget gets uninstalled, and the app chart's `post-delete` cleanup Job runs against it. Those Jobs delete on purpose, and they are right to. The monitoring one removes PVCs labelled `apps.cozystack.io/application.name=<release>-system`. That label is not incidental either: the metrics and logs claim templates stamp it so the operator copies it onto the storage PVCs, and the Job selects on it to stay release-scoped. Both ends were built to meet. The result is that a slow first install deletes the tenant's metrics and logs, and nothing reconstructs them. The gateway Job deletes the `cozystack-acme-account` Secret. That key regenerates, so this one costs no data. It costs a re-registration against the ACME provider, and repeated cycles run into its rate limits, which can hold certificate issuance for the tenant down for hours. Smaller stake, same mechanism, and the two are not one class: only one of them owns storage. `RetryOnFailure` keeps the manifests applied and retries the failed release, so no uninstall happens and neither Job runs. Both actions carry the strategy because the controller performs that retry as an upgrade, so with install alone the retry's own failure falls through to upgrade remediation. The remediation blocks stay: an absent release consults install, an out-of-sync one consults upgrade, both by retry count rather than by strategy. Two things are traded away. A real upgrade failure now retries instead of rolling back, which gives up no stable safety net, since with `retries: -1` the default strategy was already rolling back and re-upgrading without end. And the uninstall was the only automatic clean slate, so an install wedged on a bad resource state retries against that state until an operator removes the release. For a release that owns data that is the right exchange. This removes the consequence rather than the trigger, and for monitoring the trigger is worth naming separately: the parent HelmRelease here waits `10m`, while the child `monitoring-system` release sets `15m` deliberately, with a comment explaining that 10m is too short for the OIDC users-Job inside its `activeDeadlineSeconds: 900`. The parent's wait covers the child, so the parent can fail while the child is behaving exactly as designed. That mismatch is untouched here and wants its own change. The chart's etcd and seaweedfs releases lose storage the same way and are outside this change. Beyond those four, computeplane, ingress and info stay on the default strategy, but their charts ship no destructive `post-delete` hook, so remediation costs them a wasteful teardown rather than data. Same shape as #3552 and #3581. Relates to #3624. ### Screenshots <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note fix(tenant): retry a failed monitoring or gateway install instead of uninstalling the release, so a slow first install no longer runs the cleanup hooks that delete the tenant's metrics and logs volumes or its ACME account key ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Reliability Improvements** * Monitoring and gateway deployments now automatically retry failed installs and upgrades every 30 seconds. * Existing unlimited remediation retries remain available, improving recovery from deployment issues. * **Validation** * Added automated coverage to verify retry behavior for both monitoring and gateway deployments. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
36f83fa9b7 |
fix(tenant): retry failed etcd and seaweedfs installs in place (#3630)
<!-- 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 The tenant chart renders the etcd and seaweedfs releases with no install strategy, so they get Flux's default, and install remediation is an uninstall in every case Flux offers. Neither release sets `spec.uninstall.disableHooks`, which defaults to false, so that uninstall runs the app chart's `post-delete` cleanup Job. Nothing unusual is configured anywhere along that chain: fail early, uninstall, hooks. Those Jobs delete data on purpose. The etcd one removes the `data-etcd-<ordinal>` PVCs by name, because the etcd-operator does not reclaim them when the EtcdCluster goes. The seaweedfs one removes the volume PVCs by label and explicitly reclaims the keep-protected CNPG Cluster holding the filer metadata, because nothing else would. Both are right for a real uninstall, and neither hook is at fault here. Remediation is what calls them, on a release that is only starting slowly. So an install that misses its budget does not restart. It destroys the datastore the tenant control planes keep their state in, or the object store's data together with the metadata database that indexes it, and the next attempt begins from nothing. That budget is not a comfortable margin. The seaweedfs release has `timeout: 10m`, and `hack/e2e-install-cozystack.bats` already records that on a loaded runner the tenant stack only starts creating pods around 9 to 10 minutes in. The window CI observes and the window that triggers this are the same window. `RetryOnFailure` keeps the manifests applied and retries the failed release as an upgrade. No uninstall happens, so neither cleanup Job runs and the install already under way carries on instead of starting over. Both actions carry the strategy. The controller runs the retry of a failed install as an upgrade, so with install alone the retry's own failure falls through to upgrade remediation. The remediation blocks stay: an absent release consults install, an out-of-sync one consults upgrade, both by retry count rather than by strategy. One trade is that a real upgrade failure retries instead of rolling back. That costs no stable safety net. With `retries: -1` and the default strategy, upgrade remediation was already rolling back and retrying without end, so the release never rested in the rolled-back state either. The other side of the trade is that the uninstall was also the only automatic clean slate. On a tenant holding no data yet, an install that wedges on a bad resource state now retries against that state until an operator removes the release. For a release that owns data that is the right exchange, because lost data does not come back and a wedged install does. This covers two of the tenant's data-owning releases. The monitoring release carries the same default and a cleanup Job that deletes its PVCs as well, and is left for its own change, as is the rest of the catalog, whose nested releases want the question asked one at a time. Same shape as #3552 and #3581. Relates to #3555 and #3576. ### Screenshots <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note fix(tenant): retry a failed etcd or seaweedfs install instead of uninstalling the release, so a slow first install no longer runs the cleanup hooks that delete the etcd data volumes, the SeaweedFS volumes and the filer metadata database ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Reliability Improvements** * Improved installation and upgrade recovery for etcd and SeaweedFS by automatically retrying failed operations every 30 seconds. * Preserved ongoing remediation attempts to help services recover without uninstalling or rolling back after failures. * **Tests** * Added coverage to verify retry behavior and remediation settings for both services. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
79ceaf1dc2 |
fix(harbor): retry the nested system install instead of uninstalling it (#3621)
<!-- 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 Harbor's app chart renders a nested HelmRelease that owns the whole workload: the Deployments, the inline RedisFailover and its volume. It had no install strategy, so it got Flux's default, and install remediation is an uninstall in every case Flux offers. A first install that misses its budget gets torn down, and the next one provisions the redis volume and runs the master election again from scratch. That happens on a loaded runner. harbor-core waits on redis, redis waits on its volume and the master election, and the nginx Deployment hits the default `progressDeadlineSeconds` of 600s. Flux reads that as a stalled resource and fails the install early. The e2e wait budget is 10m, the same as the deadline, so after a teardown the second attempt has less time than a first install needs. `RetryOnFailure` keeps the manifests applied and retries as an upgrade. Nothing is deleted, so the volume, the elected master and harbor-core's startup all survive, and the retry costs 30s instead of a full install. A Deployment clears `ProgressDeadlineExceeded` by itself once its pods go ready, so the retry lands as soon as the stack converges. This drops the teardown, not the deadline. The suite applies the release in one step and waits for it in later ones, so part of the budget is still left when the fail-early hits. That leftover is useless to a fresh install and usable to a retry that resumes. Whether the deadline or the budget also needs moving is a separate question, and an early deadline only becomes safe once a failed install stops being torn down. In production the win holds without any of that: a slow first install no longer deletes the Redis volume and the database. Both actions carry the strategy. The controller runs the retry of a failed install as an upgrade, so with install alone the retry's own failure falls through to upgrade remediation. The remediation blocks stay: an absent release consults install, an out-of-sync one consults upgrade, both by retry count rather than by strategy. It goes on the nested release, not the parent. The parent is built from the Application by the apiserver, which already stamps this strategy on both actions. The nested one owns the workload and the volume a remediation deletes. Same shape as the tenant CNI and CSI releases in #3552 and #3581, and the unit test follows theirs. Relates to #3445. ### Screenshots <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note fix(harbor): retry a failed Harbor install instead of uninstalling the release, so a slow first install no longer deletes the Redis volume and starts over ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Harbor installations and upgrades now automatically retry every 30 seconds when failures occur. * Existing unlimited remediation retries remain available for both operations. * **Tests** * Added automated coverage to verify Harbor retry behavior and remediation settings. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
d956536461 |
fix(tenant): render the chart without the platform's injected values (#3636)
<!-- 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
`helm lint packages/apps/tenant` fails on main with three errors. This
removes two of the three; the branch name notwithstanding, lint still
exits 1 afterwards on the third, which is a repository-wide convention
question rather than a defect in this chart.
**The chart cannot be rendered outside a cluster at all.**
`templates/keycloakgroups.yaml` reads `index .Values._cluster
"oidc-enabled"`, and `_cluster` is injected by the platform, so it is
absent whenever the chart is rendered on its own. `index` on a nil map
is a hard error, not an empty result, so both `helm lint` and a plain
`helm template` die on line 1.
The idiom the neighbouring charts use for the same lookup does not
rescue this on its own, which is worth stating because it looks exactly
like a guard. `(index .Values._cluster "x") | default "y"` still fails:
`index` is evaluated first and errors on the nil map, so `default` never
receives a result to substitute; it only ever covered a key missing from
a map that exists.
The spelling alone does not decide whether a chart breaks, though.
`packages/system/cozystack-basics` uses it and lints clean, because line
1 of its `values.yaml` is `_cluster: {}`, which makes the map non-nil
before `index` runs. A chart is broken only when it neither declares the
empty map nor guards at the use site, and a lint sweep across the
catalog found 27 in that state. Guarding at the use site is the choice
here because it makes the template correct whatever `values.yaml`
contains, and because `_cluster` is platform-internal while this chart's
`values.yaml`, schema and README are generated together from
`api/apps/v1alpha1/tenant/types.go` as user-facing output. The guard has
to sit on the map rather than on the value it returns:
```
{{- $oidcEnabled := index (.Values._cluster | default dict) "oidc-enabled" }}
```
**This is behaviour-preserving, and that is checked rather than
asserted.** With `_cluster` present, `| default dict` returns it
untouched and the lookup is what it was; with it absent the lookup is
nil, the `eq` below is false, and the block stays unrendered, which is
what an unconfigured tenant should produce. Rendering before and after
gives byte-identical output:
| values | before | after |
| --- | --- | --- |
| `_cluster.oidc-enabled: "true"` | 4 KeycloakRealmGroups |
byte-identical |
| `_cluster.oidc-enabled: "false"` | no groups | byte-identical |
| no values at all | hard error | renders, no groups |
Worth recording how that comparison has to be run, because the obvious
way proves nothing. The KeycloakRealmGroup block sits behind
`.Capabilities.APIVersions.Has "v1.edp.epam.com/v1"`, which is false
under a plain `helm template`, so a first pass rendered the same output
in *both* the enabled and disabled cases, and a diff of those two would
have compared one empty result against another. The renders above pass
`--api-versions v1.edp.epam.com/v1` so the guarded branch actually
executes. Anyone proving neutrality on a capability-gated template needs
that flag or the proof is vacuous.
**`Chart.yaml` does not declare `cozy-lib`**, which is present as a
symlink under `charts/`, so lint reports missing chart metadata. It is
declared the way `packages/apps/qdrant` does it, that being the one
chart in the tree which declares it and does not carry this error.
Rendering is unchanged by the declaration, checked the same way as
above.
Two things that could plausibly have broken and did not, checked rather
than assumed: rendering from a packaged `helm package` artifact is
byte-identical, and the `charts/cozy-lib` symlink is still expanded into
the tgz with the dependency declared. And neither `hack/update-crd.sh`
nor the `fix-charts` target in `packages/apps/Makefile` reads the
`dependencies:` block, so nothing needs regenerating.
**The icon error is left in place on purpose, and the relative path is
load-bearing.** `helm lint` wants a URL with a scheme, and
`/logos/tenant.svg` has none. But `hack/update-crd.sh` consumes exactly
this form: it rewrites a leading slash to `./`, then reads that file off
disk and base64-encodes it into `.spec.dashboard.icon` of the generated
ApplicationDefinition. `packages/apps/tenant/logos/tenant.svg` is that
file. Give the chart a URL with a scheme and the generator's `[[ ! -f
"$ICON_PATH" ]]` check no longer finds anything to encode. So the
relative form is a requirement of the pipeline rather than a stylistic
convention, all 31 icon-carrying charts use it, and satisfying helm lint
here means changing the generator, not the charts. That is a
repository-wide decision and not this PR's.
`tests/keycloakgroups_oidc_test.yaml` pins the gate in all three
directions: four realm groups when `oidc-enabled` is `"true"`, none when
it is `"false"`, and none when the platform injected no `_cluster` at
all. The last case is the one the guard exists for, and it needs no
mocking, since `_cluster` is absent from `values.yaml` and "not
injected" is just the chart's default state. Against the unguarded
template that suite fails with `index of untyped nil`. The chart's other
seven suites pass identically with and without the fix, which is
precisely why the change needed its own.
Two limits of the new suite, stated rather than left to be found. It
pins how many documents render, not which: renaming the four groups or
breaking a `realmRef` would pass. And the template's second gate,
`.Capabilities.APIVersions.Has "v1.edp.epam.com/v1"`, is enabled for the
whole suite, so no case covers its absence. Both are worth closing on
the next touch of this file; neither weakens what the suite does pin,
which is that the chart renders at all when the platform injected
nothing.
A trap for anyone reproducing this by hand: the gate compares against
the string `"true"`, so `--set _cluster.oidc-enabled=true` passes a YAML
boolean and renders nothing, while `--set-string` renders the four
groups. The platform writes a genuine string, and it survives the
round-trip through a Secret because `toYaml` quotes it. Note also that
this chart's helpers reject a release name that does not begin with
`tenant-`, which will silently render nothing for either form.
One note in case a future change teaches the linter to inject the
platform's values, which would hide this class of error without fixing
it: the guard is still worth having, because it is what lets `helm
template packages/apps/tenant` succeed with no values file at all. A
chart that renders standalone can be inspected, diffed and unit-tested
without reproducing the platform around it.
### Screenshots
<!-- REQUIRED for UI changes: attach screenshots or screen recordings
demonstrating
the visual impact of your changes. PRs with UI changes without
screenshots will not be merged. -->
### Downstream repositories
<!--
Cozystack is upstream for repositories that are NOT kept in sync with it
automatically, and nothing in CI compares the two sides, so a change
here can break them silently.
Which change forces what, and which file to touch, is the trigger map in
docs/agents/contributing.md under "Downstream Repositories". Walk it
before you tick anything.
IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM
TO FILL IN:
1. Walk the trigger map against the actual diff of this PR, file by
file. Do not decide from the PR title or from your memory of what you
changed.
2. For every repository you tick, open the follow-up PR there and link
it on the same line. A ticked box with no link claims work that does not
exist, which is worse than an empty box. Search that repository for an
open PR or issue covering it first, and link that instead of filing a
duplicate.
3. When the follow-up is out of scope, or needs a decision that is not
yours to make, open an issue in that repository instead, link that, and
say so in the PR body. Do not push a speculative PR into another
repository just to fill a line here.
4. Do not tick "No downstream repository is affected" to make the
checklist go away. If you are unsure, leave every box empty and say so
in the PR body, so a human decides.
-->
- [x] No downstream repository is affected by this change
- [ ] [cozystack/website](https://github.com/cozystack/website) -
follow-up:
- [ ]
[cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack)
- follow-up:
- [ ]
[cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack)
- follow-up:
- [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up:
- [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up:
- [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) -
follow-up:
- [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) -
follow-up:
- [ ]
[cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server)
- follow-up:
- [ ]
[cozystack/external-apps-example](https://github.com/cozystack/external-apps-example)
- follow-up:
- [ ] [cozystack/examples](https://github.com/cozystack/examples) -
follow-up:
### Release note
<!-- Write a release note:
- Explain what has changed internally and for users.
- Start with the same `type(scope):` prefix as in the PR title
- Follow the guidelines at
https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md.
-->
```release-note
fix(tenant): the tenant chart now renders without the values the platform injects, so `helm template` works on it standalone, and its `cozy-lib` dependency is declared in Chart.yaml; this clears two of the three `helm lint` errors it reported, the remaining one being the repository-wide relative icon URL
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
- **New Features**
- Added support for using the shared Cozy library in tenant deployments.
- **Bug Fixes**
- Improved tenant configuration rendering when cluster settings are
unavailable.
- OIDC-related Keycloak groups now render reliably when OIDC is enabled
and required platform capabilities are present.
- Preserved expected behavior when OIDC is disabled or configuration is
omitted.
- **Tests**
- Added coverage for enabled, disabled, and missing cluster
configuration scenarios.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
Aleksei Sviridkin
|
c1071dab9b |
feat(kubernetes): let a node group log the guest serial console (#3637)
## What this PR does Both tenant Kubernetes e2e suites keep missing the node-Ready budget with zero Nodes registered, and the time goes inside the guest, between the worker VMI reaching Running and Talos asking for its apid certificate. Nothing collected today reaches that window. The in-guest capture the harness already has drives `talosctl`, so it needs the apid the worker never got to, and the management side has neither a Node nor a certificate request to read. Four attempts at this failure have been designed from outside the guest; the most recent raised the budget from 12m to 18m and the same fingerprint came back at the new deadline. KubeVirt can stream a VM's guest serial console into a `guest-console-log` container beside virt-launcher, readable with `kubectl logs` and needing nothing from inside the guest — no talosctl, no client certificate, no helper Pod, no reachable apid. That is the one property this failure class demands, so this turns it on for the suite's worker node group and collects it into the diagnostic bundle when node-join fails, ahead of the `talosctl` capture that cannot describe the same runs. Reads start at the beginning of the stream rather than tailing it: a guest that repaints its console would push the boot output out of any `--tail` window, while `--limit-bytes` truncates from the far end instead. Every read is bounded, and the walk stops at a cap that records both counts, so the collector cannot become an unbounded term in a failure path that already runs several others under one deadline. A read that returns no console is not filed as a quiet guest. Three outcomes get three labels, because only one of them says anything about the guest: the read failed and returned nothing, the read succeeded and returned nothing, or it broke off part way. Each read's stderr is kept beside the capture rather than inside it — kubectl's own `container guest-console-log is not valid for pod` would otherwise fill the file, and a console that never started would read as one that had spoken. That case is the headline one here, so the Pod state and events that explain it are collected alongside, once for the whole selector. The switch is a new opt-in `logSerialConsole` on a node group, off by default. It renders nothing unless set to true, including when set to false, because the `KubevirtMachineTemplate` is content-hash named and a byte emitted for a group that did not ask for it renames the template and rolls every live worker VM on upgrade. I verified that by rendering the base tree and this branch: absent and explicit-false produce a byte-identical template name on both charts. Worker VMs come from two charts, `kubernetes` and `kubernetes-nodes`; both carry the field, and the golden-parity check between them gains a case that keeps their output identical. The passing path checks separately that the container actually attached, and runs that check last, after everything the suite exists to prove — a debugging aid should not preempt the assertions that cover the cluster. Without it the suite would first learn the setting was inert in the one run that needed the console, when it can no longer be recovered. Relates to #3513. ### The risk this carries, stated up front The platform turns serial console logging off cluster-wide in the KubeVirt CR. That setting is a mitigation added in February 2026 for kubevirt/kubevirt#15989, where the `guest-console-log` container kept virt-launcher from starting and left VMs in `PodInitializing`. A per-VM `logSerialConsole` takes precedence over it — `isSerialConsoleLogEnabled` reads the VMI field first and only then falls back to the cluster setting — which is what makes this work, and also makes this the first thing on the platform to go through that mitigation. The mitigation was written against KubeVirt v1.6.x; the platform now ships v1.8.4, and the upstream issue is still open with no reproduction reported either way on the newer version. So the first run of these suites is also the experiment, and it doubles as a revalidation of that February mitigation on the version actually shipping, which nobody has done. The two outcomes are distinguishable rather than ambiguous: kubevirt/kubevirt#15989 shows up as virt-launcher wedged in `PodInitializing` with the VMI never reaching Running, while the failure this instruments shows the VMI Running and zero certificate requests. A red run with the first signature is an answer, not noise — and the collector is built so that it says which one happened instead of reporting silence. The blast radius is small: the product field is absent by default and only the e2e node group sets it, so backing this out is one line. The revert criterion, stated before the run rather than after it. If worker virt-launcher Pods sit in `Init` on `guest-console-log` and no Node registers, kubevirt/kubevirt#15989 still reproduces on v1.8.4: drop the `logSerialConsole: true` line from the e2e node group, keep the chart field, and the upstream issue gains the reproduction on a current version that its thread does not have. If the Pods reach Running and the console logs carry boot output, the mitigation is a candidate for removal platform-wide, which is its own change. Either way the run answers something that was open before it. Dropping `virtualMachineOptions.disableSerialConsoleLog` outright was the alternative. It is a smaller diff and needs no values key, but it exposes every VM on the platform to a race nobody has re-tested, instead of one node group in one test. That is its own revalidation exercise and belongs in its own change, on evidence this one produces. ### Verification `make unit-tests` passes. The new bats suite runs green under both `bash` and a `dash` `/bin/sh`, which matters because `hack/cozytest.sh` is `#!/bin/sh` and an earlier revision of this branch carried a bash-only construct that failed silently there. Every test in it is killed by a specific mutation of the code it covers. Both charts' helm-unittest suites pass, the golden-parity script reports byte-identical output across all four cases including the new one, and `make generate` is idempotent at the repo root. Reviewed by Claude across several rounds and by an independent second reader; the review loop found and fixed real defects in this branch, including a green-path check that had become unable to report the one thing it existed for. ### Screenshots Not a UI change. ### Downstream repositories - [ ] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [x] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: https://github.com/cozystack/terraform-provider-cozystack/issues/26 - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note feat(kubernetes): worker node groups accept `logSerialConsole`, which streams a worker VM's guest serial console into a `guest-console-log` container readable with `kubectl logs`. Off by default. It is the only way to observe a worker that stalls during boot before Talos apid answers, when no Node and no certificate request exist to diagnose from. Enabling it rolls the node group and overrides the platform's cluster-wide `disableSerialConsoleLog`. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added an optional `logSerialConsole` setting for Kubernetes worker node groups. * Streams worker VM guest serial-console output to a dedicated log container when enabled. * Configuration is disabled by default and available through Kubernetes application settings. * **Bug Fixes** * Improved worker failure diagnostics with bounded serial-console output and pod-state information. * Validates that enabled worker pools attach the expected console-log container. * **Documentation** * Documented configuration behavior, rollout effects, defaults, and timeout considerations. * **Tests** * Added coverage for rendering, attachment validation, capture limits, and failure scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
7b36a5d2f2 |
test(tenant): give these template lists their real reason (#3651)
## What this PR does Two helm-unittest suites in `packages/apps/tenant/tests/` said they restrict rendering because `namespace.yaml`'s `lookup` errors under helm-unittest. It does not. The lookup returns empty offline and the template renders, which `namespace.yaml`'s own header states and which `tests/namespace_ancestor_labels_test.yaml` depends on. I measured what each list actually does by removing it, and the two suites turned out to be different cases rather than two copies of one. In `cleanup_role_watch_test.yaml` the list is load-bearing, for a template the comment never mentioned. That suite sets no values, and `keycloakgroups.yaml` indexes `.Values._cluster` on its first line, so the whole-chart render fails with `index of untyped nil` and all 8 tests error. The comment now says that, and says it conditionally: a guard on that index is already in flight, and once it lands the list stops being load-bearing. I checked that too, by applying the guard and re-running with the list removed, which passes 8/8. In `vmo_webhook_dependson_test.yaml` the list is not load-bearing at all. That suite sets `_cluster`, the chart renders without the list, and all 5 tests pass. It stays only to scope the suite, and the comment says so instead of inventing a technical reason. Each comment now states what holds for its own file and nothing wider. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map against the diff. It touches two files, both under `packages/apps/tenant/tests/`, and no chart, template, values file or tooling. Nothing in the map keys on test files. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Clarified test-rendering rationale and suite-scoping comments. * Documented that rendering requires `_cluster` values and that template lists control test scope. * **Tests** * Updated explanatory comments only; test behavior and configuration remain unchanged. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
50c5e2c14c |
test(e2e): capture the tenant worker guest serial console
The tenant Kubernetes suites fail the node-Ready budget with zero Nodes registered, and the time is lost inside the guest between the worker VMI reaching Running and Talos requesting its apid certificate. Nothing collected today covers that window: the existing in-guest capture drives talosctl, so it needs the very apid the worker never reached, and on the management side there is neither a Node nor a certificate request to read. Turn on the guest console log for the suite's node group and collect each virt-launcher's guest-console-log container from the management cluster. Reads start at the beginning of the stream rather than tailing it, so a guest that repaints its console cannot push the boot output out of the window; every read is bounded, and the walk stops at a cap that records both counts, so the collector cannot become the unbounded term in a failure path that already runs several others under one deadline. A read that returns no console is not filed as a quiet guest. Three outcomes get three labels, because only one of them says anything about the guest: the read failed and returned nothing, the read succeeded and returned nothing, or it broke off part way. Each read's stderr is kept beside the capture rather than inside it, since kubectl's own "container is not valid for pod" would otherwise fill the file and a console that never started would read as one that had spoken. That case is the headline one here, so the Pod state and events that explain it are collected alongside, once for the whole selector. Enabling the console overrides a cluster-wide setting the platform holds for an open upstream defect, so the run is also an experiment, and its own failure mode is the one that hides best: the container wedges virt-launcher in Init, no worker boots, and the suite reports "fewer than 2 tenant nodes Ready" -- the same sentence the failure under study produces. Read as the known signature, the experiment's answer would be filed as the flake it was meant to explain. So the failure path names that case first, before any other diagnostic, and only when it is true. The passing path checks separately that the container attached at all, and reports the ratio it saw rather than a bare success, since the check is deliberately satisfied by any one Pod. It keeps three outcomes apart too: a read that did not answer, a selector that matched no Pod, and Pods that matched and carry nothing. It reads the Pod name alongside the container names so the last two cannot collapse, and both container lists so a future move of the container cannot fail a run over a diagnostic. It runs last, after everything the suite exists to prove, because a debugging aid must not preempt the assertions that cover the cluster. The capture runs before the talosctl one because it is the only of the two that can describe a worker which never reached apid, the shape this failure keeps taking. kubectl reports a warning and an empty result alike, on stderr with a zero status, so the exit status decides which artifact its output lands in. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
1cb51c92aa |
test(tenant): give these template lists their real reason
Both suites here said they restrict rendering because namespace.yaml's `lookup` errors under helm-unittest. It does not. The lookup returns empty offline and the template renders, which namespace.yaml's own header states and which tests/namespace_ancestor_labels_test.yaml depends on. Measured what each list actually does by removing it. In cleanup_role_watch_test.yaml the list is load-bearing, for a template the comment never mentioned: the suite sets no values and keycloakgroups.yaml indexes `.Values._cluster` on its first line, so the whole-chart render fails with "index of untyped nil". Stated conditionally, because a guard on that index is already in flight and would end it. In vmo_webhook_dependson_test.yaml the list is not load-bearing at all, because that suite sets `_cluster` and the chart renders without it. It stays only to scope the suite. Each comment now states what holds for its own file and nothing wider. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
a3c1408d08 |
test(harbor): select hook and release documents by kind, not by index
Both suites addressed their subject by document index. An index is a position, so a template that gains a document ahead of the one under test moves every later assertion onto a different object, and what happens then depends on the assertion: measured by inserting one document at the head of each template, the cleanup suite reddens outright, while two of the four jobservice tests keep passing because a `notExists` against an object that never had the path is satisfied for the wrong reason. Indices are also easy to get wrong by hand. helm-unittest numbers documents in template source order, while `helm template` prints them sorted by kind, so the rendered output is not the order being counted. Every document these suites assert on is unique by kind within its template, so each test now names its subject. A selector that matches nothing fails, including on the `notExists` tests, so the anchor holds in the direction that was silent before. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
eac7e1ea15 |
test(tenant): anchor the upgrade.force guard to its HelmRelease
Each test asserted only `notExists: spec.upgrade.force`, which passes on a template that renders no document at all. Renaming the value that gates one of these releases makes the template render nothing and leaves its test green: measured on etcd.yaml, where the guard stayed green while a sibling suite's `equal` was the only thing that noticed the release had vanished. Pin each HelmRelease name alongside the absence so the assertion has a document to be about. Cover computeplane.yaml. Seven templates in this chart emit a HelmRelease and the list held six; the module arrived after the suite was written. The invariant holds there today, so this closes an unguarded template rather than a live regression — which is the same gap that produced this suite, whose first pass missed gateway.yaml. Correct two claims in the header. The template restriction was said to be there because namespace.yaml's `lookup` errors under helm-unittest; it does not, the lookup returns empty offline and the template renders, which is what its own header says and what namespace_ancestor_labels_test.yaml relies on. The restriction stays, for the reason that actually applies: it keeps an unrelated template breaking out of this suite's result. helm-controller also does not deprecate upgrade.force. The CRD vendored at internal/fluxinstall/manifests/fluxcd.yaml, which is what the pinned Flux 2.8.x ships, describes it as a replacement strategy and carries no deprecation marker; `recreate` is the neighbouring field that does. Later upstream documentation adds that the field is ignored under server-side apply, and that sentence is not in the pinned CRD, so the comment says which is which. The reason to keep the flag off is client-side apply, where it replaces resources instead of patching them. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
6d403c3015 |
fix(tenant): retry failed etcd and seaweedfs installs in place
The tenant chart renders both releases with Flux's default install strategy, and install remediation is an uninstall in every case Flux offers. Neither release disables hooks on it, so a remediated install runs each chart's post-delete cleanup Job, and those Jobs delete data on purpose. The etcd one removes the data-etcd-<ordinal> PVCs by name, because the etcd-operator does not reclaim them when the EtcdCluster goes. The seaweedfs one removes the volume PVCs by label and explicitly reclaims the keep-protected CNPG Cluster holding the filer metadata, precisely because nothing else removes it. Both are correct for a real uninstall. Remediation is what calls them, on a release that is only starting slowly. So an install that misses its budget does not merely restart. It destroys the datastore the tenant control planes keep their state in, or the object store's data together with the metadata database that indexes it, and the next attempt begins from nothing. RetryOnFailure keeps the manifests applied and retries the failed release, so no uninstall happens and neither cleanup Job runs. Both actions carry the strategy because the controller performs that retry as an upgrade, so on install alone the retry's own failure falls through to upgrade remediation. The cost is that a real upgrade failure retries rather than rolls back, which gives up no stable safety net: with retries: -1 and the default strategy, upgrade remediation already rolled back and retried without end. The remediation blocks stay because an absent release consults install and an out-of-sync one consults upgrade, both by retry count rather than by strategy. The other side of the trade is that the uninstall was also the only automatic clean slate. On a tenant holding no data yet, an install that wedges on a bad resource state now retries against that state until an operator removes the release. For a release that owns data this is the right exchange, because lost data does not come back and a wedged install does. This covers two of the tenant's data-owning releases. The chart's monitoring release destroys its storage on a remediated install as well and is outside this change, as is the rest of the catalog, whose nested releases want the question asked one at a time. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
bd3f3b93d0 |
fix(tenant): render the chart without the platform's injected values
`helm lint packages/apps/tenant` fails, and two of its three errors are
the chart's own.
`templates/keycloakgroups.yaml` reads `index .Values._cluster
"oidc-enabled"`. `_cluster` is injected by the platform, so it is absent
whenever the chart is rendered on its own, and `index` on a nil map is a
hard error: `index of untyped nil`. The chart cannot be rendered or
linted outside a cluster at all.
The idiom the neighbouring charts use for the same lookup does not
rescue this on its own, which is worth stating because it looks like it
does. `(index .Values._cluster "x") | default "y"` still errors: `index`
evaluates first and fails on the nil map, so `default` never sees a
result to substitute; it only ever covered a key missing from a map that
exists.
The spelling alone does not decide whether a chart breaks, though.
`packages/system/cozystack-basics` uses it and lints clean, because line
1 of its values.yaml is `_cluster: {}`, which makes the map non-nil
before `index` runs. So a chart is broken only when it neither declares
the empty map nor guards at the use site. Guarding at the use site is
preferred here because it makes the template correct whatever
values.yaml contains, which is what lets the chart render standalone;
and `_cluster` is platform-internal, while this chart's values.yaml,
schema and README are generated together from
api/apps/v1alpha1/tenant/types.go as user-facing output.
The guard has to sit on the map rather than on the value it returns,
hence `index (.Values._cluster | default dict) "oidc-enabled"`.
That is behaviour-preserving rather than merely quieter. With `_cluster`
present, `| default dict` returns it untouched and the lookup is what it
was. With it absent the lookup is nil, the `eq` below is false, and the
block stays unrendered, which is what an unconfigured tenant should
produce. Rendered both ways before and after, with
`--api-versions v1.edp.epam.com/v1` so the capability-gated block
actually executes, the output is identical byte for byte: four
KeycloakRealmGroups when `oidc-enabled` is `"true"`, none when it is
`"false"`. Rendering with no values at all goes from a hard error to a
clean render with no groups.
`Chart.yaml` also does not declare `cozy-lib`, which is present as a
symlink under `charts/`, so lint reports missing chart metadata. Declare
it the way `packages/apps/qdrant` does, that being the one chart in the
tree that declares it and does not carry this error. Rendering is
unchanged by the declaration, checked the same way.
`tests/keycloakgroups_oidc_test.yaml` pins all three branches of the
gate: four realm groups when `oidc-enabled` is `"true"`, none when it is
`"false"`, and none when the platform injected no `_cluster` at all.
That third case is the one the guard exists for, and it needs no mock,
because `_cluster` is absent from values.yaml and so "not injected" is
simply the chart's own default state. Against the unguarded template the
suite fails with `index of untyped nil`; the chart's other seven suites
pass identically either way, which is what left the change uncovered.
The third error, `invalid icon URL '/logos/tenant.svg'`, is deliberately
left. Lint wants a URL with a scheme, and all 31 charts in the tree that
carry an icon use a relative path, none an absolute one. Changing this
one would make it the only chart with a different convention, and
choosing the URL a relative path should become is a decision about where
those assets are served from, not about lint.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
|
||
|
Aleksei Sviridkin
|
a27c341843 |
fix(tenant): retry failed monitoring and gateway installs in place
Both releases carry Flux's default install strategy, and install remediation is an uninstall in every case Flux offers. Neither disables hooks on it, so a remediated install runs the app chart's post-delete cleanup Job against a release whose only fault was missing a readiness budget. The monitoring Job deletes PVCs labelled apps.cozystack.io/application.name=<release>-system. That label is not incidental: the metrics and logs claim templates stamp it precisely so the operator copies it onto the storage PVCs, and the cleanup Job selects on it to stay release-scoped. A slow first install therefore deletes the tenant's metrics and logs, and nothing reconstructs them. The gateway Job deletes the cozystack-acme-account Secret. That key regenerates on the next install, so this one costs no data. It costs a re-registration against the ACME provider, and repeated cycles run into its rate limits, which can hold certificate issuance for the tenant down for hours. RetryOnFailure keeps the manifests applied and retries the failed release, so no uninstall happens and neither cleanup Job runs. Both actions carry the strategy because the controller performs that retry as an upgrade, so on install alone the retry's own failure falls through to upgrade remediation. Two things are traded away. A real upgrade failure now retries rather than rolls back, which gives up no stable safety net, since with retries: -1 and the default strategy upgrade remediation already rolled back and retried without end. And the uninstall was the only automatic clean slate, so an install wedged on a bad resource state retries against that state until an operator removes the release. The remediation blocks stay because an absent release consults install and an out-of-sync one consults upgrade, both by retry count rather than by strategy. The chart's etcd and seaweedfs releases lose storage the same way and are outside this change. Beyond those four, computeplane, ingress and info remain on the default strategy, but their charts ship no destructive post-delete hook, so remediation costs them a wasteful teardown rather than data. That is a different question from this one. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
c20d47a245 |
feat(kubernetes): let a node group log the guest serial console
A worker VM that stalls during boot before Talos apid answers is invisible to every diagnostic the platform has: no Node registers, no certificate request arrives, and talosctl needs the apid that never came up. The guest serial console does show that window, but KubeVirt attaches its guest-console-log container only when the VMI asks for it, and the platform turns console logging off cluster-wide. Add an opt-in per-node-group logSerialConsole that sets the KubeVirt field, which takes precedence over the cluster-wide setting. The key renders nothing unless true -- including when set to false -- because the KubevirtMachineTemplate is content-hash named, so a byte emitted for a group that did not ask for it renames the template and rolls every live worker VM. The description carries the two costs the field name does not show. Changing the value rolls the group it is set on, so the stuck VM whose console was wanted is replaced along with the rest and the setting buys visibility into the next boot rather than the current one. And the cluster-wide default it overrides exists because that same container has been seen holding virt-launcher in PodInitializing, which would take a group down rather than explain it. Worker VMs are emitted by two charts. Both carry the field, and the golden-parity check gains a case that keeps their output identical. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
f4dc5d4d3c |
fix(harbor): retry the nested system install instead of uninstalling it
The harbor app chart renders a nested HelmRelease that owns the whole workload: the harbor Deployments, the inline RedisFailover and its volume. It carried Flux's default install strategy, and install remediation is an uninstall in every case Flux offers, so a first install that went over budget was torn down between attempts and the next one re-provisioned the redis volume and re-ran the master election from zero. That is reachable on a loaded runner. harbor-core blocks on redis while redis waits for its volume and its master election, and the nginx Deployment reaches the default progressDeadlineSeconds of 600s, which Flux reads as a stalled resource and fails the install early. The e2e wait budget for the release is the same 600s, so one teardown cycle consumes all of it and the second attempt cannot finish however fast it converges. RetryOnFailure keeps the manifests applied and retries the failed release, so the convergence already under way continues instead of restarting. Both actions carry the strategy because the controller performs the retry of a failed install as an upgrade, so on install alone the retry's own failure falls through to upgrade remediation. The cost is that a real upgrade failure retries rather than rolls back, which gives up no stable safety net: with retries: -1 and the default strategy, upgrade remediation already rolled back and retried without end. The remediation blocks stay because an absent release consults install and an out-of-sync one consults upgrade, both by retry count rather than by strategy. The change lands on the nested release rather than on the parent: the parent is built from the Application by the apiserver, which already stamps this strategy on both actions, while the nested release is the one that owns the workload and the volume a remediation deletes. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
0ba567dd73 |
fix(tenant): bound the pre-delete cleanup waits and the hook Job (#3598)
## What this PR does The tenant pre-delete hook deletes the tenant's HelmReleases in two waves with `kubectl delete --wait=true` and no `--timeout`. kubectl reads `--timeout=0` as "wait forever" and substitutes a week, so a wave hangs for as long as any child HelmRelease cannot finalize. helm then returns from the uninstall without deleting anything: the release stays in storage, flux keeps the finalizer, and the same teardown gets retried. The delete policy carries no `hook-failed`, so the waiting Job survives between attempts too. Each wave now gets 90s. Neither is guarded, on purpose. Nothing stands behind these deletes, so a wave that did not finish ends the run instead of reporting a cleanup that never happened. That also keeps the order the two waves exist for, since the tenant modules are what the applications run on. Whatever got deleted stays deleted, so the next attempt resumes from there. The Job gets `activeDeadlineSeconds: 270`. It bounds what the waits do not, since the DELETE calls carry kubectl's default `--request-timeout` of 0, and it sits under the 300s uninstall budget flux applies when the HelmRelease has no timeout of its own. That is the budget for tenants cozystack-api generates; `tenant-root` is the exception, a static release in `cozystack-basics` carrying `timeout: 15m0s`, and the ordering holds there too since 280 is under 900. The 30s gap is not cosmetic. The deadline runs from the Job's `.status.startTime` while helm's budget runs from before the Job is created, so at equal values helm gives up first, on a Job that is still running. That is the state this change is meant to end, and it would look fixed, because a number would be there. `terminationGracePeriodSeconds: 10` is part of the same arithmetic. Since 1.31 the Job controller holds back the terminal condition until every pod has finished terminating, so what helm waits for is the deadline plus the termination. At the 30s default that lands on 270+30, level with the budget, which is the gap the deadline was set to buy. That period gets spent in full, every time. The SIGTERM goes to PID 1, which is the shell: the script runs several commands, so the shell never execs into kubectl and stays PID 1 itself. A signal carrying its default disposition is discarded by the kernel when the target is PID 1, and a non-interactive shell installs no SIGTERM handler, so nothing acts on it. kubectl is a child and was not signalled at all. The pod ends on SIGKILL. Nothing in that script needs to finish gracefully, the DELETE calls are already sent and the process is only watching. The pod also gets resource requests, matching the sibling hook in `packages/apps/kubernetes`. Without them it is BestEffort and first to go under node pressure, which mattered less when a retry inside the Job absorbed an eviction. With a single attempt it does not. `backoffLimit: 0` and `restartPolicy: Never` go together, and neither implies the other. At the default limit of 6 the Job does not reach `Failed` inside the budget, so helm falls back on its own timeout with the same nameless "timed out waiting for the condition" as before. Under `OnFailure` the pod survives its own failure and the kubelet re-runs the container in place, and the Job controller finds out afterwards by summing `restartCount`. The script starts over, re-enters the applications wave that just expired, and gets cut down mid-run when the controller catches up. `Never` puts a failed pod in front of the controller directly. ## What this PR does not do No remedy behind the bounded waits. The `packages/apps/kubernetes` hook force-clears child finalizers when its own wait expires. That needs `patch` on HelmReleases in the Role, and it is only safe there because the whole tenant control plane is going away in the same run. Here a wave that expires ends the attempt. `packages/apps/kubernetes/templates/delete.yaml` is untouched. It does not make a doomed teardown converge. A tenant whose children cannot finalize still gets its uninstall retried. What changes is that each attempt ends in bounded time, instead of eating the whole uninstall budget and leaving a pod behind. 90s is a guess, and it is tight. The first wave deletes every application HelmRelease in the namespace, including kinds that are slow to tear down: a nested Tenant runs this same hook one level down before its release finalizes, and an app of kind Kubernetes tears a control plane down. Either one can outrun 90s. The bound is what is left once the Job deadline sits under helm's budget instead of level with it, split between the two waves. The 90s the deadline keeps back for scheduling and the image pull is not calibrated either, and I would rather say so than imply it was. `docker.io/clastix/kubectl:v1.32` comes from Docker Hub with no mirror in front of it, and this repo has measured Docker Hub pulls of a smaller image at about 1m42s in CI. So that remainder is roughly the observed worst case rather than comfortably above it. A cold pull that slow lands on the `activeDeadlineSeconds` path, which is the one that deletes the pod and leaves nothing to read. There is a band that pays for this, and I would rather name it than round it off. Neither shape above fits inside 300s today, so for them the bound costs nothing that works. But every generated child HelmRelease carries a wait strategy, so a wave inherits its slowest child, and a wave that settles in something like 95s does complete inside the budget today. After this change it expires instead, and that tenant trades one clean attempt for an extra flux retry. In wall clock that retry is about five minutes, since `helmrelease-interval` defaults to `5m`, so a tenant in that band takes noticeably longer to delete than it does now. I think that is the right side of the trade: an unbounded wave does not fail, it spends the whole budget and leaves helm reporting a timeout that names nothing. If the band turns out to be populated, the waits and the deadline move together, and the bats suite pins their ordering rather than their values. One caveat on the diagnostics, since the comments in the chart used to promise more than they deliver. The failing wave leaves its output in the failed pod, which `ttlSecondsAfterFinished: 300` then reaps while flux's uninstall backoff is still climbing. And on the `activeDeadlineSeconds` path there is no log at all, because the Job controller deletes the active pods when the deadline expires. That path is the backstop for a kubectl that never returns; the expected path is the wave's own `--timeout`, where the script exits on its own and the pod survives to be read. ## Tests `hack/tenant-pre-delete-hook.bats` renders the chart, pulls the hook script out of the Job and runs it against a stub `kubectl` on `PATH`. It covers a clean run reporting success, every delete reaching kubectl with a non-zero bound, the ordering of the four numbers, and a wave that gave up ending the run without going on to the next one. `--timeout` is implemented inside kubectl, so no stub can make that deadline actually elapse. The tests read the arguments that reached kubectl and inject the non-zero exit an expired delete produces. No EXIT traps. The ordering test reads the numbers out of the rendered Job instead of pinning them, so moving a value keeps it honest while a value that breaks the ordering still fails. An absent deadline or termination period is rejected explicitly, otherwise `null` satisfies the numeric comparison and a Job with no bound at all passes. The Job's own shape is pinned in the chart's helm-unittest suite, since the script cannot observe that about itself. Both suites are green locally, and I mutated each pinned value to confirm they fail for the right reason: dropping either `--timeout`, dropping `activeDeadlineSeconds`, dropping `terminationGracePeriodSeconds`, dropping the requests, `backoffLimit` back to 6, `restartPolicy` back to `OnFailure`, and a `|| echo` appended to the applications wave. Each one goes red in the test that claims that ground. Worth flagging so the check marks are not read as more than they are: `make bats-unit-tests` is red at the merge base, unrelated to this branch, and the loop it runs exits on the first failing file. `hack/cozyreport.bats` sorts ahead of this suite, so on CI the new bats file is not reached at all right now. It is not failing, it is not running. The helm-unittest cases are unaffected and do execute. ### Screenshots None. This changes a Job manifest and its tests, with no user-visible surface. ### Downstream repositories Walked the trigger map against the diff. The diff is one Job manifest inside the tenant chart, its helm-unittest suite, and a new `hack/*.bats` file. No package is added, renamed or removed; no `values.schema.json`, `values.yaml` default or version enum changes; no `ApplicationDefinition` semantics, namespace, variant or release asset changes; nothing under `hack/` is moved or renamed and no make target changes behaviour, since `bats-unit-tests` already globs `hack/*.bats`. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note fix(tenant): bound the pre-delete cleanup hook's HelmRelease deletions and give its Job a wall-clock deadline, so a tenant whose children cannot finalize no longer leaves the hook waiting indefinitely and blocking the uninstall ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Reliability Improvements** * Tenant cleanup jobs now enforce execution deadlines and terminate gracefully. * Cleanup operations have bounded deletion timeouts and no automatic retries. * Failed cleanup steps now stop subsequent operations and correctly report failure. * Cleanup jobs now specify CPU and memory resource requirements. * **Tests** * Added comprehensive coverage for cleanup behavior, timeouts, failure handling, and job lifecycle settings. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
0b7a8f67d1 |
fix(apps/kubernetes): report the pre-delete cleanup outcome honestly (#3592)
## What this PR does The pre-delete cleanup Job for the `kubernetes` app ended with an unconditional `echo "Cleanup completed successfully"`. Several of its steps swallow a failure behind `|| echo "WARNING: ..."` so that `set -e` cannot end the run, which means that line was printed in exactly the situation those warnings exist to report: child HelmReleases still holding Flux finalizers, or a Kamaji datastore Secret still pinning its namespace. Someone reading the Job log to find out why a namespace will not delete was told the cleanup had worked. The Job now records what a step left behind and closes with either the success line or the list of steps that left something. Only residue gets recorded. A bounded delete that times out has a remedy right behind it (Step 2 falls through to force-clearing finalizers, Step 4 falls through to Step 4b), and whether anything survived is decided by that remedy, which reports its own outcome. A tenant with no working nodes takes that route as a matter of course, which is the reason the wait is bounded at all, so recording the timeout itself would report a finished teardown as incomplete on the commonest path there is and send an operator hunting for objects that are not there. That is the same defect as the one being fixed, pointing the other way. A read that fails looks exactly like one that found nothing. An empty list and a failed list both give a loop nothing to iterate; an absent object and an unreachable apiserver both make a probe non-zero. So the whole script was walked for that shape instead of patching the instances as they were reported. Four reads reach the closing line. Step 2's force-clear re-list now keeps its status and reports when it cannot list what it was about to clear. Step 4b's two probes became three-way, present, absent or unknown, so a Secret whose existence could not be established is no longer announced as absent. Step 1's list is the one left swallowing its status, and that is deliberate: Step 2 deletes by label rather than from that list, so a suspend that never happened leaves nothing of its own behind, and reporting it would name residue that does not exist. Making the TenantControlPlane probe three-way also repairs the gate above it, and that is intended. The gate is there so the datastore Secret's finalizer is not stripped while Kamaji may still own it, and a two-way probe defeated it in the one direction that does damage: any error read as "confirmed absent" and the strip went ahead. Its likeliest cause, a throttled or briefly unreachable apiserver, is also likeliest exactly when a broken tenant is being torn down. Step 4b declining to strip a finalizer while the TenantControlPlane is still alive is the one residue that no failing command marks: every call in that branch succeeds, and the namespace stays blocked regardless. It now reports, and names the TenantControlPlane as the thing holding it up. What this PR does not do: the hook still exits zero, even with every backstop engaged. That is the difference between a main path and a backstop. `packages/apps/tenant/templates/cleanup-job.yaml` is the same kind of hook and handles it the opposite way, correctly: deleting the HelmReleases is that hook's whole purpose, so a failure there means the cleanup did not happen, and `set -e` ending the run before the success line is honest. The swallowed sites here are backstops. They run only after the normal deletion has already timed out, and they exist to push teardown further along. Failing there aborts the Helm uninstall before the release is removed, so the release stays in storage and the same doomed teardown is retried forever, at precisely the moment the backstop was written for. The steps that are not backstopped still end the run under `set -e` and never reach the closing message at all. This makes the message truthful. It does not make it reachable. The Job carries `helm.sh/hook-delete-policy: hook-succeeded`, and the hook exits zero by design on every backstopped run, so Helm reaps the Job, and its pod log, right after the hook completes. Whether the closing line survives long enough to be read is a question about the delivery channel, not about its wording, and changing that channel (an Event on the CR, or keeping the Job when something was left behind) has a different blast radius. Left for a separate change. The step comment and the runtime warning both cited the tracking issue for the Kamaji datastore-secret leak as though it were live. It is closed, and closed by this step rather than by a fix in Kamaji, so following the pointer showed a resolved report and no reason for the code to exist. The comment now says the ordering it covers is still reachable, and the warning names the finalizer and the object to clear by hand instead of an issue number. The workaround itself is untouched: nothing indicates the upstream cause has gone away. `hack/kubernetes-pre-delete-hook.bats` renders the Job, extracts the script and runs it against a stub `kubectl`, covering a clean run, a fallback route that succeeded, a TenantControlPlane outliving Step 4, each probe that could not answer, every remedy failing at once, an unbackstopped step ending the run, and the zero-exit invariant. The helm-unittest cases match the script's source text and cannot tell whether the closing message is reachable. Every case was checked by mutation, including reinstating the unconditional success line and reinstating the over-reporting the fallback case guards against. ### Screenshots Not a UI change. ### Downstream repositories - [x] No downstream repository is affected by this change ### Release note ```release-note fix(kubernetes): the pre-delete cleanup hook no longer reports success when a cleanup step left something behind, and no longer reports a completed teardown as incomplete when it merely fell back to force-clearing finalizers. The Job log now names the steps that left residue. The hook still exits zero so the Helm uninstall can remove the release. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved Kubernetes cleanup handling during application deletion. * Cleanup now distinguishes unavailable resource checks from confirmed absence, avoiding unsafe finalizer removal. * Reports leftover resources when recoverable cleanup steps fail while still completing the deletion process. * Improved handling of child releases, control-plane resources, and datastore secrets. * **Tests** * Added comprehensive coverage for successful cleanup, partial failures, resource residue, probe errors, and unrecoverable steps. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
ab8675161c |
fix(kubernetes): bound the remaining pre-delete hook deletions (#3604)
## What this PR does Three of the six steps in the `kubernetes` pre-delete hook call `kubectl delete` with neither `--wait=false` nor `--timeout`. `--wait` defaults to true and `--timeout=0` is implemented as a week, so the KamajiControlPlane delete in step 3, the DataVolume delete in step 5 and the LoadBalancer Service delete in step 6 each park the hook for 168 hours on one stuck finalizer. A DataVolume held by a CDI importer or a PVC finalizer is the reachable case. The waits in steps 2 and 4 were bounded earlier, in response to a report that named the child-HelmRelease wait and nothing else. The fix stopped where the report did, so the shape it described stayed in the three steps nobody had looked at. None of the three waits now. Nothing downstream reads the result of steps 5 and 6. What matters after step 3 is the TenantControlPlane, and step 4 deletes that one directly and waits for it, so blocking on the KamajiControlPlane first only adds a second place for a finalizer to stall. An expired wait and `--wait=false` leave the same cluster state, with the same objects finalizing in the background. Not waiting also keeps these steps under `set -e` with no guard, which matters more than it looks. A guard cannot tell an expired wait from a rejected request, so it would report a failed delete as cleanup that happened. With no timeout to expire, every non-zero exit left is a real failure. The `watch` verb goes away for those three resources. kubectl opens a watch only on the wait path, so the grant was unexercised. `helmreleases` and `tenantcontrolplanes` keep it because steps 2 and 4 still wait, and there the verb is load-bearing in a way that is easy to miss: removing it does not produce an error, it hangs the delete. That is the failure this repo already hit once at the tenant level, so the whole rule set is pinned rather than left to the next reviewer of this Role. `backoffLimit` goes to 0, against a default of 6 that `restartPolicy: Never` turns into seven pods in series, each restarting the script from step 1 and each paying its own image pull. ## What this PR does not do It does not add `activeDeadlineSeconds`, and that is arithmetic rather than oversight. A deadline has to sit above the sum of the remaining waits, or it cuts runs that were going to finish, and far enough under helm-controller's uninstall budget to be the thing helm reports rather than something helm waits through. Those two bounds do not currently leave a gap, and the image pull comes from Docker Hub with no mirror in front of it. Room comes from raising `spec.uninstall.timeout` on the generated HelmRelease first; squeezing the waits further is the wrong direction. Worth stating plainly, because it is the one change here that is not purely a tightening: with `backoffLimit: 0` a pod eviction now fails the whole uninstall attempt, where six retries previously absorbed it. That is deliberate. Both remaining waits are guarded, so an attempt can spend its entire budget without failing, and a retry would restart from step 1 with the clock already there; on a nodeless tenant it would also re-pay step 4, since the first attempt leaves the TenantControlPlane still terminating. The failure a retry would cover is an eviction, which is not bounded in time and costs most when it arrives late. Flux still repeats the uninstall on its own interval, so the end state is preserved and the trade is wall-clock for predictability. It does not bound the requests. `--wait=false` and `--timeout` bound the waiting, so an apiserver that accepts the connection and then stops answering hangs this hook exactly as before. That is a different failure from the stuck finalizer, it is not made worse here, and it wants one answer across every call rather than a flag on the deletes. ## Tests `packages/apps/kubernetes/tests/delete_hook_test.yaml` gains cases for the bounds and for the Role. Every delete in the hook is asserted to carry one, the Job is pinned to a single attempt, and the rule set is pinned whole. The regex patterns walk only continuation lines, the ones ending in a backslash, so each stays inside the command it names. A cross-line wildcard reaches a flag belonging to the next command instead, and RE2 has no lookahead to exclude the `echo` in between. The Role assertions use whole-rule `contains` plus `lengthEqual` rather than per-index verb checks. `notContains` passes when its path does not resolve, so an index-addressed "and nowhere else" goes quiet the moment the rules list is reordered or shortened, which is exactly the edit it exists to catch. Matching the entire rule pins the verb set in both directions and names the resource when it fails. Every pinned value was mutation-tested: dropping `--wait=false` from each of the three deletes, dropping `--timeout` from either of the two that still wait, `backoffLimit` in both directions, adding `watch` back, removing `watch` from a resource that waits, appending a rule past everything asserted, and reordering the rules list. Each one fails the assertion that claims that ground and no other. ### Screenshots None. This changes a Job manifest and its tests, with no user-visible surface. ### Downstream repositories Walked the trigger map against the diff. The diff is one Job manifest inside the kubernetes chart and its helm-unittest suite. No package is added, renamed or removed; no `values.schema.json`, `values.yaml` default or version enum changes; no `ApplicationDefinition` semantics, namespace, variant or release asset changes; nothing under `hack/` is touched and no make target changes behaviour. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note fix(kubernetes): bound the pre-delete hook's remaining unbounded deletions and limit its Job to a single attempt, so a stuck finalizer on a DataVolume, LoadBalancer Service or KamajiControlPlane no longer parks the hook and blocks the uninstall ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved Kubernetes resource cleanup during deletion to avoid indefinite waits. * Cleanup operations now complete with bounded waiting and fewer retry attempts, improving reliability and responsiveness. * Removed unnecessary access permissions from cleanup operations, strengthening security. * **Tests** * Added coverage to verify deletion timeouts, retry behavior, and required permissions across supported resources. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
4e3a679e7b |
test(apps): anchor index-addressed negative asserts to what they name
notContains passes whenever its path resolves to something that simply lacks the forbidden content, and a different element at the same index qualifies. These guards would go green if the Role's rule order changed or a nodePool were added ahead of the one they name, while the resource they forbid came back on the element they meant. Assert the addressed element before asserting what it must not have. The Kafka Role rules have no identifier, so the anchor is the in-cluster bootstrap Service that rule must keep. The OpenSearch pool does have one, `component`, and that is what is pinned: roles are shared between pools, so a role-based anchor would be satisfied by a second data-bearing pool inserted ahead of this one. Each anchor was checked against the shift it exists to catch, and against the same shift with the anchors removed: without them the blocks stay green while the forbidden content is back on the element they name. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
b60c08f110 |
fix(kubernetes): bound the remaining pre-delete hook deletions
Three of the six steps in the pre-delete hook call kubectl delete with neither --wait=false nor --timeout. kubectl defaults --wait to true and reads --timeout=0 as "wait forever", which it implements as a week, so the KamajiControlPlane delete in step 3, the DataVolume delete in step 5 and the LoadBalancer Service delete in step 6 each park the hook for 168 hours on a single stuck finalizer. A DataVolume held by a CDI importer or a PVC finalizer is the reachable case. None of the three waits now. Nothing downstream reads the result of steps 5 and 6, and what matters after step 3 is the TenantControlPlane, which step 4 deletes and waits for directly; blocking on the KamajiControlPlane first only adds a second place for a CAPI finalizer to stall. An expired wait and --wait=false leave the same cluster state either way, with the same objects finalizing in the background once the hook has moved on. Not waiting also keeps these steps under `set -e` with no guard. A guard cannot tell an expired wait from a rejected request, so it would report a failed delete as cleanup that happened; with no timeout to expire, every non-zero exit left is a real failure. Drop the watch verb from the three resources those steps delete, and pin the whole rule set. kubectl opens a watch only on the wait path, so the grant was unexercised for the three; for helmreleases and tenantcontrolplanes its absence would not error but hang, which is the failure mode hardest to read from the outside. Set backoffLimit to 0, against a default of 6 that restartPolicy Never turns into seven pods in series. Zero rather than one because both remaining waits are guarded, so an attempt can spend its whole budget without failing and a retry would start from step 1 with the clock already there; on a nodeless tenant it would also re-pay step 4, since attempt one leaves the TenantControlPlane still terminating. The failure a retry would cover is an eviction, which is not bounded in time and is worst exactly when it arrives late. The price is that such an eviction fails the whole attempt and flux repeats it on its own interval. No activeDeadlineSeconds: the waits and helm-controller's uninstall budget leave no gap a deadline could sit in, once a Docker Hub image pull is counted. That needs the budget raised first, through spec.uninstall.timeout on the generated HelmRelease, and is left out rather than set to a number that would cut runs which were going to finish. These bounds cover the waiting, not the request: --request-timeout is still 0 on every call in the hook, so an unresponsive apiserver hangs it as before. That is a different failure from the stuck finalizer, and it wants one answer across every call rather than a flag on the deletes. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
cf85716444 |
fix(tenant): bound the pre-delete cleanup waits and the hook Job
kubectl reads --timeout=0 as "wait forever" and substitutes a week, so the two `kubectl delete helmreleases --wait=true` calls in the tenant pre-delete hook hang for as long as any child HelmRelease cannot finalize. helm returns from Uninstall.Run before deleting anything, so the release stays in storage and flux retries the same teardown; helm.sh/hook-delete-policy carries no hook-failed, so the waiting Job survives between attempts as well. Bound each wave at 90s. Neither is guarded: nothing stands behind these deletes, so a wave that did not finish ends the run rather than report a cleanup that did not happen, and ending it keeps the tenant modules from being torn out from under applications that are still deleting. Whatever was deleted stays deleted, so the next attempt resumes from there. The bound has a price and it is not zero. A nested Tenant, or an app of kind Kubernetes whose own hook spends two 120s waits first, misses 90s and converges across uninstall retries instead; neither fits the 300s budget as it stands either. Between those and the fast case sits a band that does pay: a wave inherits its slowest child, so one that settles around 95s completes inside the budget today and expires here instead, trading a clean attempt for one more retry. An unbounded wave does not fail, it spends the whole budget and reports a timeout naming nothing, so the trade is worth taking. Give the Job an activeDeadlineSeconds of 270. It bounds what the waits do not — the DELETE calls carry kubectl's default --request-timeout of 0 — and sits under the 300s uninstall budget flux applies when the HelmRelease carries no timeout of its own, because the deadline runs from the Job's startTime while that budget runs from just before the Job is created. At equal values helm gives up first, on a Job still running. Cap the pod's termination at 10s, which is part of that arithmetic rather than a detail of it. Since 1.31 the Job controller withholds the terminal condition until every pod has finished terminating, so helm learns of the failure at the deadline plus this period; the 30s default would land it at 270+30, level with the budget the deadline is set under. The period is spent in full: the SIGTERM goes to PID 1, which is a shell running a multi-command script, and a default disposition sent to PID 1 is discarded by the kernel with no handler installed to catch it. kubectl is a child and is never signalled. Set backoffLimit to 0 and restartPolicy to Never. The limit is needed under either policy, since the default of 6 keeps the failure from reaching helm inside the budget. The policy matters because under OnFailure the pod survives its own failure and the kubelet re-runs the container in place, with the controller applying the cap only afterwards by summing restartCount: the script re-enters the wave that just expired and is cut down mid-run. Never leaves a failed pod the controller sees directly. Add resource requests to match the sibling hook in packages/apps/kubernetes. Without them the pod is BestEffort and first out under node pressure, which a retry inside the Job used to absorb and a single attempt does not. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
0827bcb300 |
fix(apps/kubernetes): report the pre-delete cleanup outcome honestly
The pre-delete cleanup Job ended with an unconditional "Cleanup completed successfully". Several of its steps swallow a failure behind `|| echo "WARNING: ..."` so that `set -e` cannot end the run, so that line was printed in exactly the situation the warnings exist to report: child HelmReleases still holding finalizers, or a Kamaji datastore Secret still pinning its namespace in Terminating. Record what a step leaves behind and close with either the success line or the list of steps that left something. Residue is the thing worth recording, not every fallback: a bounded delete that times out has a remedy right behind it -- Step 2 falls through to force-clearing finalizers, Step 4 falls through to Step 4b -- and whether anything survived is decided by that remedy, which reports its own outcome. A tenant with no working nodes takes that route as a matter of course, which is why the wait is bounded in the first place, and calling such a run incomplete would send an operator hunting for objects that are not there. A read that fails looks exactly like one that found nothing: an empty list and a failed list both give a loop nothing to iterate, and an absent object and an unreachable apiserver both make a probe non-zero. Every read the summary rests on now keeps its status. Step 2's force-clear re-list reports when it cannot list what it was about to clear, because everything it could not return is still holding its finalizer. Step 4b's two probes became three-way -- present, absent or unknown -- so a Secret whose existence could not be established is no longer announced as absent. Step 1's list is the one read still swallowing its status, and that is a decision rather than an omission: Step 2 deletes by label rather than from that list, so a suspend that never happened leaves nothing of its own behind and reporting it would name residue that does not exist. Making the TenantControlPlane probe three-way also repairs the gate above it, and that is intended rather than incidental. The gate exists so the datastore Secret's finalizer is not stripped while Kamaji may still own it, and a two-way probe defeated it in the one direction that does damage: any error reads as "confirmed absent" and the strip goes ahead. Its likeliest cause, a throttled or briefly unreachable apiserver, is also likeliest exactly when a broken tenant is being torn down. Step 4b declining to strip a finalizer while the TenantControlPlane is still alive is the one residue that no failing command marks: every call in that branch succeeds and the namespace stays in Terminating regardless. It now reports, and names the TenantControlPlane as the thing holding the namespace up. The exit code is deliberately unchanged and still zero. These are backstops, not the main path: they run after the normal deletion has already timed out, and they exist to push teardown further along, so failing there would abort the Helm uninstall at the very moment the backstop was written for, leaving the release in storage and the same teardown retried forever. That makes the closing message the only outcome report an operator gets, which is why it has to be accurate. The steps that are not backstopped still end the run under `set -e` and never reach that message. Rework the runtime warning and the RBAC comment that pointed at the tracking issue for the Kamaji datastore-secret leak. That issue is closed, and closed by this very step rather than by a fix in Kamaji, so a reader who followed the pointer found a resolved report and no reason for the code to exist. The step comment now says the ordering it covers is still reachable, and the warning names the finalizer and the object to clear by hand. Cover the behaviour with a bats file that renders the Job, extracts the script and runs it against a stub kubectl, since the helm-unittest cases match the script's source text and cannot tell whether the closing message is reachable. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
b4e203164a |
fix(kubernetes): retry the tenant CSI install instead of uninstalling it (#3581)
## What this PR does The tenant CSI HelmRelease carried `remediation.retries: -1` with no strategy, so Flux applied its default, `RemediateOnFailure`. Install remediation is an uninstall in every case the API offers, so an install that missed its budget was torn down between attempts: the tenant lost its CSI node plugin and the StorageClasses this release propagates until a later attempt applied them again, and that attempt restarted the DaemonSet rollout instead of resuming one already under way. That teardown races the rollout it exists to recover, and the run recorded in #3576 shows how narrowly. Both DaemonSet pods were scheduled at the same second on both workers, so nothing was waiting on capacity; the ten minutes went to image pulls. The slower worker started its last container six seconds after the install timeout fired, and the uninstall removed it one second after that. `RetryOnFailure` leaves the manifests applied and retries the failed release. Both actions carry it because the controller picks the active retry from `.status.lastAttemptedReleaseAction` and performs that retry as an upgrade: set on install alone, the retry still happens, but its own failure is then handled by upgrade remediation, whose default is a rollback with no previous release to roll back to. The `remediation` blocks stay, because an absent release consults `install.remediation` and an out-of-sync one consults `upgrade`, both by retry count rather than by strategy. This is the change #3552 makes for the tenant CNI, and the reason for putting the strategy on both actions is the one given there. It is also the shape upstream prescribes: the "Recommended settings" block in the [HelmRelease spec shipped with the helm-controller this installs](https://github.com/fluxcd/helm-controller/blob/v1.5.1/docs/spec/v2/helmreleases.md#recommended-settings) puts `strategy.name: RetryOnFailure` under both `install` and `upgrade` for production deployments. Nothing changes on the test side, and that is deliberate rather than deferred. Because no manifest is removed at the attempt boundary, the rollout continues across it and the kubelet finishes what it started, while the retry re-applies the same manifests every 30s. The release therefore reports Ready once the rollout completes rather than once a whole attempt has fit inside the timeout, so what the existing e2e wait has to outlast is the rollout and no longer an attempt boundary that discards it. That widens what the wait tolerates without making it unbounded: a rollout slower than the wait itself still fails, and this does not claim otherwise. Scope. #3576 records more than this fixes, so this does not close it. The other tenant addon HelmReleases this chart renders keep the remediation-only shape; CSI is the one this changes. ### Screenshots Not a UI change. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: The trigger map was walked against the diff. The change is a HelmRelease template and a chart test, not a `values.schema.json` field, a `values.yaml` default, a version enum, a `release.prefix`, an `ApplicationDefinition`, a chart source kind or a namespace. ### Release note ```release-note fix(kubernetes): the tenant CSI HelmRelease now retries a failed install instead of being uninstalled between attempts, so a failed install no longer takes the tenant's CSI node plugin and its propagated StorageClasses away while it retries. The same strategy applies to the upgrade action, which it has to: a retried install runs as an upgrade. One consequence is operator-visible — a failed CSI upgrade in a tenant cluster now retries the new manifests every 30s instead of rolling back to the previous release. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved reliability when installing or upgrading the CSI component by automatically retrying failed actions every 30 seconds. * Retains unlimited remediation retries to help recover from transient failures. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
0c4c11c845 |
fix(kubernetes): retry the tenant CSI install instead of uninstalling it
Flux defaults a HelmRelease to the RemediateOnFailure strategy, whose install remediation is always an uninstall. A tenant CSI install that misses its timeout was therefore torn down between attempts, taking the CSI node plugin and the StorageClasses this release propagates with it, and the next attempt restarted the DaemonSet rollout from nothing instead of resuming one already under way. That teardown races the rollout it is meant to recover. In one observed run both DaemonSet pods were scheduled immediately and spent the whole budget pulling images; the slower node started its last container six seconds after the install timeout fired, and the uninstall removed it one second later. Retrying in place keeps that work. RetryOnFailure leaves the manifests applied and retries the failed release. Both actions carry it because the controller picks the active retry by .status.lastAttemptedReleaseAction and runs that retry as an upgrade: set on install alone, the retry happens but its own failure falls through to upgrade remediation, which rolls back by default with no previous release to roll back to. The cost is that a real upgrade failure now retries rather than rolls back, and that gives up no stable safety net: with retries: -1 and the default rollback strategy, upgrade remediation already rolled back and retried without end. The remediation blocks stay for a separate reason. An absent release consults install.remediation and an out-of-sync one consults upgrade, both by retry count rather than by strategy, so retries: -1 is what keeps those two branches acting. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Andrey Kolkov
|
1b1a6b2f1d |
fix(clickhouse): address review — trim template docs, harden backup demo CA secret
Review feedback on #3385: - Move the endpointCA rationale out of the Helm template (lllamnyp): the SSL_CERT_DIR and CA-volume blocks were multi-paragraph essays. Trim each to a few lines stating only the non-obvious decision (SSL_CERT_DIR vs AWS_CA_BUNDLE; the deliberate non-optional mount) and point at the values.yaml endpointCA field for the full description. - wait_deleted (00-helpers.sh): poll with 'get --ignore-not-found' and treat only an empty result as deletion, so an RBAC/API-server/transport failure is no longer read as success and cleanup can't settle on a false positive. - Backup demo CA Secret: scope the default name to CLICKHOUSE_NAME, stamp an ownership label on the copy, refuse to overwrite a Secret the demo doesn't own, and delete on cleanup only when that label is present — so a foreign Secret sharing the name is never clobbered or removed. - README: document CH_BACKUP_CA_SECRET and the release-scoped, demo-owned CA Secret; spell out the admin-copy path (export CH_BACKUP_CA_SECRET before steps 04 and 07). Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey KolkovandClaude Opus 4.8
|
419c55695d |
docs(clickhouse): clarify shipped sidecar image is upstream 2.7.4 in SSL_CERT_DIR note
The SSL_CERT_DIR rationale said the behaviour was "verified against altinity/clickhouse-backup:2.7.4" while the chart ships altinity-clickhouse-backup:v1.6.0, which reads as a version mismatch. They are the same binary: the cozystack image is built FROM upstream altinity/clickhouse-backup:2.7.4. Say so, and point at the Dockerfile, so the measurement provably applies to the shipped image. Comment-only change inside a Go-template comment; render is unchanged and helm unittest stays 17/17. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Aleksei Sviridkin
|
80a2090444 |
fix(kubernetes): retry the tenant CNI install instead of uninstalling it
The tenant cilium HelmRelease carried no install strategy, so Flux applied its default, RemediateOnFailure, which uninstalls the release between attempts. Cilium is the tenant cluster's only CNI, so every cycle takes it away and gives it back when the next install applies the manifests again, and retries: -1 lets that repeat without end. What sets a cycle off is an install that outruns its budget, which can happen while the tenant still has no registered node, since the operator Deployment cannot become Available until one exists. Remediation neither causes that delay nor is fixed by this change; what it adds is a repeated teardown of the CNI on top of a delay that resolves itself once the node arrives. This removes the amplifier, not the cause. RetryOnFailure keeps the applied manifests in place and retries a failed install as an upgrade, so the rollout completes on its own once a worker registers. Both actions carry the strategy because the retry of a failed install runs as an upgrade, which would otherwise fall back to upgrade remediation. It is the strategy and retry interval cozystack-api and cozystack-operator already set on every HelmRelease they generate, except that they leave remediation nil where this release keeps it. The other tenant addons the chart renders still carry the remediation-only shape; this one is where the teardown removes the cluster's only CNI rather than churning a single component. The cost is that a genuine upgrade failure no longer rolls back either, it retries on the failed manifests instead. That is forced, since the controller cannot tell a retried install from a real upgrade, and what it replaces was an unbounded rollback-and-retry flap rather than a stable safety net. The remediation blocks stay, since they cover the branches no strategy reaches: an absent release, which consults the install block, and an out-of-sync one, which consults upgrade. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Myasnikov Daniil
|
4e9c5ce8ca |
fix(kubernetes): render the talos-reconcile Job for the default md0 group
The Job in templates/talos/talos-reconcile-job.yaml is the only producer of the TalosConfigTemplate that each worker MachineDeployment names in spec.template.spec.bootstrap.configRef; cluster.yaml renders the reference but deliberately not the object. Its loop read .Values.nodeGroups directly, while the MachineDeployment loop reads the effective set through the kubernetes.nodeGroups helper. For a cluster that supplies no nodeGroups the two disagree: the helper's else-branch emits the built-in md0 group, so the md0 MachineDeployment renders, but the raw map is empty and no Job renders at all. Every Machine the cluster-autoscaler adds to md0 then blocks indefinitely on a TalosConfigTemplate that nothing will ever create, and the KamajiControlPlane spec.network.certSANs patch the same Job performs is skipped too. Range over the helper so the Job set tracks the MachineDeployment set exactly. No change for a cluster that declares its own groups, and md0 stays removable: the helper's if-branch keeps a user-supplied map authoritative. The gap survived because every helm-unittest fixture and e2e suite declares md0 explicitly. tests/nodegroups_default_test.yaml does render the empty-nodeGroups case but lists only templates/cluster.yaml, so it never looked at the Job. The new suite pins both halves of the helper contract and fails without this fix. Fixes #3504 Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Andrey KolkovandClaude Opus 4.8
|
9379729b31 |
fix(clickhouse): address review notes on endpointCA and the CA copy
Review notes from IvanHunters on #3385 (LGTM with non-blocking findings). 1. The backup-s3-ca volume carries no `optional: true` while the adjacent useSystemBucket credential refs deliberately do, and the asymmetry was undocumented — an operator naming a Secret that does not exist gets the whole ClickHouse Pod stuck in ContainerCreating, not just the sidecar. Kept the fail-closed behaviour and wrote down why the two cases differ: credentials are projected by the platform after install, so a strict ref would wedge every fresh release until the first BackupJob, whereas this CA is named by the tenant, must already exist, and is trust material — optional would mount an empty dir, the Pod would look healthy, and the first backup would fail deep inside clickhouse-backup with an x509 error naming neither the Secret nor the chart. The comment also states the blast radius plainly. 2. The CA probe swallowed stderr, so an RBAC denial on the cross-namespace read was reported as "CA not found" followed by advice (set S3_CA_SECRET) that cannot fix a permissions problem. It now separates present / absent / not-allowed-to-look and says so, and the README no longer labels step 03 simply "tenant": copying the CA reads a Secret in tenant-root, which is a platform capability. Added a section spelling out the three ways to satisfy that (admin credentials, a one-off admin copy, or skipping the CA entirely on a publicly-trusted endpoint). 3. Caveat, and a usability regression this branch introduced: on a cluster with a publicly-trusted endpoint and no seaweedfs, step 03 hard-exited on a default the user never chose. It now distinguishes an explicitly named Secret (still an error when missing — a typo should not be guessed past) from the unset default, where it warns and continues with backup.endpointCA unset. Verified all four paths against a fake kubectl: Forbidden, absent+default, absent+explicit, and present. 4. Caveat: endpointCA reaches the clickhouse-backup sidecar only. The legacy `schedule` CronJob writes to S3 through restic and does not consume it, so a private-CA endpoint combined with `schedule` still fails TLS. Documented on the field itself, so it lands in values.schema.json, the README and the cozyrds openAPISchema. The PR body's claim about select-e2e.sh/select-install.sh mappings was stale after the restructure that removed them; corrected there rather than in code. 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
|
0c14d6cf43 |
test(e2e): sharpen round-trip diagnostics and make teardown settle
Recommendations from the third review pass (which returned LGTM; none of these blocked, all three are cheap and make the failure paths honest). 1. The round-trip's catch named nothing. An unnamed `describe HelmRelease` in tenant-root dumps every release on the root tenant of a live cluster and buries the two that matter, so name them (mariadb-2-backup-roundtrip names its release for the same reason). Raised podLogs tail 100 -> 200, since the sidecar logs every action and an S3 error sits well back from the tail by the time a step fails, and added the Keeper Pods: chkeeper.yaml labels them `app: <release>-keeper`, so the existing app.kubernetes.io/instance selectors missed them entirely — and a source ClickHouse that never reaches a ready replica is usually Keeper failing to form a quorum, which explains itself only there. 2. SSL_CERT_DIR is now `/etc/ssl/cozy-s3-ca:/etc/ssl/certs`. The single-directory form works — Go reads the default bundle FILE regardless, which is what the earlier measurement showed — but that left the public-CA half of the contract resting on a file this image happens to ship. Naming the system directory costs nothing and survives a base-image bump that moves the bundle; verified against public AWS S3 that both forms still verify (403 auth, no x509). The mount path stays the bare directory, and the unit test now pins both halves separately — it caught the over-broad edit that had briefly made mountPath a colon list. 3. cleanup.sh returned while the uninstalls it triggered were still running, so the next suite (etcd) could start with helm-controller still draining this one, and the pre-clean could hand step 04 an application whose predecessor was still going away. It now waits for both ClickHouse releases and the Bucket to actually disappear, and — since the script deliberately runs without `set -e` so one absent resource cannot abandon the rest — records those failures and exits non-zero, rather than reporting a teardown that did not settle as success. wait_deleted polls `get` instead of using `kubectl wait --for=delete`, which errors on an already-absent resource: the normal case here, and indistinguishable from a real failure. Sized the two cleanup step budgets to 12m for the same reason the run step is 60m: cleanup.sh's waits now sum to 540s, which the previous 8m did not clear. The Keeper-sized-from-the-server-resources bug the review found outside this diff is filed as #3487. 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
|
2687e095f4 |
feat(clickhouse): let the backup sidecar trust a private S3 CA
The clickhouse-backup sidecar had no TLS/CA surface at all: bucket coordinates
and credentials arrive as S3_* env, and nothing let it verify an endpoint whose
certificate is signed by a private CA. Cozystack's own in-cluster SeaweedFS is
exactly that case — it serves :8333 behind the self-signed "SeaweedFS CA" — so a
ClickHouse release could only ever back up to a publicly-trusted endpoint, while
MariaDB (storage.s3.tls.caSecretKeyRef) and Postgres (barman endpointCA) have had
a CA field all along.
Add backup.endpointCA {name, key}. When set, the chart mounts that Secret into
the sidecar and points SSL_CERT_DIR at it. Empty (the default) renders no env, no
mount and no volume, so existing releases are untouched.
SSL_CERT_DIR rather than AWS_CA_BUNDLE, measured against
altinity/clickhouse-backup:2.7.4 with a self-signed endpoint and public AWS S3 in
turn: Go reads SSL_CERT_DIR in addition to the system bundle, so both verify;
AWS_CA_BUNDLE replaces the SDK trust pool, so the private endpoint verifies and
public S3 starts failing `x509: certificate signed by unknown authority`. That
would silently break a release that also talks to a public endpoint.
S3_DISABLE_CERT_VERIFICATION was rejected outright — it drops verification
instead of trusting one more CA.
Covered by helm-unittest: the unset default, the mount/env/volume wiring, a
custom key still projected as ca.crt, composition with useSystemBucket, and that
backup.enabled=false renders nothing.
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>
|
||
|
Andrei Kvapil
|
2ff9c30d04 |
docs(kubernetes): note the etcd gate in the Proxied Service comment
The Proxied ingress Service is gated on _namespace.etcd as well as the addon toggle and exposeMethod, but the comment above it still described only the addon+exposeMethod gate — comment drift from the etcd-gate change. State both gates so the comment matches the condition it sits on. Assisted-By: Claude Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io> |
||
|
Andrei Kvapil
|
8fc8d23dfd |
fix(computeplane): address review round three
Gate the Proxied ingress Service on _namespace.etcd like the addon HelmRelease it fronts, so an etcd-less tenant with the addon pinned on does not get a selector-less Service (+ test). Correct the secrets framing: the tenant-visible surface is an allowlist (include: [] withholds everything), so drop the never-load-bearing exclude entry, the bats cross-check and the rename-coupling comments, and state the real mechanism. Fix the etcd prerequisite docs (an ancestor's etcd satisfies the wrapped chart; the awaiting-etcd beacon is the diagnostic), reword the tenant toggle to stop advertising the not-yet-shipped placement routing, require maxReplicas >= 1 on the ingress-role node group, and fix comment drift (Service name after the release rename, placement-hook label, package-guard lookup semantics). Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io> |
||
|
Andrei Kvapil
|
366268d9f5 |
test(kubernetes): pin that a disabled ingress addon renders nothing with hosts set
hosts with the addon disabled used to render the Ingress; the addon gate makes 'the addon is off' mean nothing renders. Pin the combination so the behaviour change reads as a decision. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io> |
||
|
Andrei Kvapil
|
6dcf5373e7 |
fix(computeplane): let Flux derive both release names
The aggregated API cannot represent spec.releaseName: the conversion rebuilds HelmRelease specs without it, so the first tenant edit through the API dropped the module's explicit computeplane-module release name, let it default to the object name computeplane, and collided with the Helm storage record the cluster release owned — with the tenant chart writing the old name back every interval. Drop releaseName from both HelmReleases and derive names from object names, like every other tenant module. Kamaji now writes the admin kubeconfig to computeplane-cluster-admin-kubeconfig; the computeplane-rd secrets.exclude entry follows in the same change, and a bats test pins the exclusion to the name the cluster release actually produces so a future rename cannot silently re-expose the credential. Also add the check-release-name guard the other extra modules carry, and pin the fluxcd addon's distribution artifact to the flux-operator version vendored in system/fluxcd-operator instead of :latest. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io> |
||
|
Andrei Kvapil
|
76374f6472 |
fix(tenant): fail loudly when computeplane is enabled without its package
The computeplane-application package ships in the iaas bundle while the toggle arrives with tenant-rd in the system bundle, and bundles are independent booleans. Refuse the render with a clear diagnostic instead of producing a HelmRelease that retries a nonexistent artifact forever. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io> |
||
|
Andrei Kvapil
|
bca5b3f9fb |
fix(kubernetes): render the Proxied ingress Service only when the addon is enabled
exposeMethod defaults to Proxied while the addon defaults to off, so gating the Service on exposeMethod alone emitted a selector-less Service — surfaced in the tenant dashboard — on every cluster that runs no ingress-nginx at all. Gate on the addon instead; ComputePlane sets enabled: true explicitly, and the Service still renders without a hosts list. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io> |
||
|
Andrei Kvapil
|
9cb4f2922d |
fix(kubernetes): create the Proxied ingress Service independently of hosts
The proxy Service was only rendered when addons.ingressNginx.hosts was non-empty, but external consumers — e.g. a ComputePlane app's per-app management Ingress — need to route to the tenant cluster's ingress-nginx without the module knowing every app host in advance. Render the Service whenever exposeMethod is Proxied; the Ingress still requires hosts. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io> |
||
|
Andrei Kvapil
|
b2232904b6 |
feat(tenant): add the computeplane module toggle
Wire the computeplane module into the tenant chart as a bool toggle, matching the other tenant modules. The module's own Helm release takes the -module suffix so the canonical release name computeplane (and with it the computeplane-admin-kubeconfig Secret) stays reserved for the cluster HelmRelease the module renders. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io> |
||
|
Andrei Kvapil
|
8b461f0c0b |
feat(monitoring): drop the Grafana rebuild for the upstream image + catalog plugins (#3378)
## What this PR does Cozystack builds and publishes `ghcr.io/cozystack/cozystack/grafana` — a rebuilt Grafana image — and uses it by default in the monitoring stack. Grafana is AGPL-3.0, so publishing a rebuilt binary makes the project a redistributor of an AGPL Grafana build. This is a governance concern for CNCF Incubation (there is no precedent for a CNCF project redistributing an AGPL binary). The only reason the image was ever rebuilt was to bundle the VictoriaLogs datasource plugin, which was not in the official Grafana plugin catalog at the time. It is now published there as a signed plugin (`victoriametrics-logs-datasource`), so the rebuild is no longer necessary. This PR moves the deployment to the upstream `grafana/grafana` image (digest-pinned, routed through `cozy-lib.image` so mirrored-registry installs keep working) and installs the plugins at container startup via `GF_INSTALL_PLUGINS`, with versions pinned: - `victoriametrics-logs-datasource 0.29.0` — the VictoriaLogs datasource (signed; the reason the image was rebuilt) - `natel-discrete-panel 0.1.1`, `grafana-worldmap-panel 1.0.6`, `marcusolsson-dynamictext-panel 6.2.0` — reproduce the panel set the removed Dockerfile vendored **Air-gapped installs keep working.** The plugin zips are not downloaded from the internet at pod startup: they are mirrored from the official catalog into the existing `grafana-dashboards` image at build time and served by that in-cluster static file server, next to the dashboards it already serves. `GF_INSTALL_PLUGINS` points at `http://grafana-dashboards.cozy-grafana-operator.svc/plugins/…`, so the runtime dependency stays inside the cluster and the artifact is mirrored into private registries together with every other first-party image. A new `allow-to-grafana-dashboards` CiliumNetworkPolicy (mirroring `allow-to-keycloak`) opens tenant egress to that server, since tenant egress is default-deny. As a result, `ghcr.io/cozystack/cozystack/grafana` is no longer built or published: the Dockerfile, the pinned image tag, the `image` targets in both monitoring Makefiles, and the build step in the root Makefile are removed. Notes: - The AGPL-3.0 Grafana binary is no longer redistributed; the mirrored plugin zips are Apache-2.0 licensed and are served unmodified from the official catalog builds (signatures intact). - `allow_loading_unsigned_plugins` and the custom `/var/lib/grafana-plugins` path are dropped: the catalog datasource is signed, and plugins install into the writable default `GF_PATHS_PLUGINS` (`/var/lib/grafana/plugins`). - The VictoriaLogs datasource provisioning (the `GrafanaDatasource` CRs pointing at `vlselect`) is unchanged. - Plugins are re-installed on every pod start from the in-cluster server (no PVC involved, ~77 MB per start per replica over the cluster network); a failed download fails the pod loudly instead of silently dropping the datasource. - `natel-discrete-panel` and `grafana-worldmap-panel` are Angular plugins; Angular is disabled by default in Grafana 11, so they do not load — this is unchanged from the current rebuilt image (same Grafana 11.6.15, same config). They are kept for parity and can be dropped as a follow-up. ### How this was verified - `helm unittest` across the repo passes, including new regression tests: the monitoring chart asserts the exact upstream image reference (with and without `images-registry` set) and the exact `GF_INSTALL_PLUGINS` value; the tenant chart asserts the new egress policy. - Local end-to-end of the full chain in Docker: built the modified `grafana-dashboards` image, served it on a shared network, and started upstream `grafana/grafana:11.6.15` with the rendered `GF_INSTALL_PLUGINS` value. All four plugins download from the in-cluster-style URL (no internet egress from Grafana), and `GET /api/plugins` reports `victoriametrics-logs-datasource` v0.29.0 as `enabled: true` with `signature: valid`. ### Screenshots Not applicable — no UI change. Grafana renders the same datasources and panels; only the image source and plugin delivery mechanism change. ### Downstream repositories Walked the trigger map against the diff. This is an internal change to how the monitoring stack sources the Grafana image and its plugins (`packages/system/monitoring`, `packages/system/grafana-operator`, `packages/apps/tenant` network policy). No package is added, renamed or removed; no `values.schema.json`, `ApplicationDefinition`, platform values, release asset, shared `hack/` tooling or `package.mk` contract changes; the added `grafana.image` key is an internal system-chart value, not a user-facing app field. No downstream repository is affected. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note feat(monitoring): Grafana now runs on the upstream `grafana/grafana` image; the VictoriaLogs datasource and panel plugins are mirrored from the official Grafana catalog into the in-cluster `grafana-dashboards` server and installed at startup, so air-gapped installs keep working. Cozystack no longer builds or publishes `ghcr.io/cozystack/cozystack/grafana`. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Improvements** * Grafana in monitoring is now based on a digest-pinned upstream image for more consistent deployments. * Required Grafana plugins are installed automatically at startup via `GF_INSTALL_PLUGINS`; unsigned-plugin configuration was removed. * Tenant networking now includes a new egress rule specifically for Grafana dashboards. * Grafana dashboards image now includes required plugin assets prepared during image build. * **Tests** * Added/updated checks covering digest pinning, mirrored-registry behavior, plugin-installation behavior, and the new Grafana dashboards network policy. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |