mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-09 05:25:14 +09:00
597d966d7d688b7fbaec27632b4e87f61ba6ca1f
224
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Aleksei Sviridkin
|
7002397e40 |
test(kafka): assert that uninstall leaves no node pool or broker behind
The Kafka suites passed while the node pools of the apps they had deleted, and the brokers those pools own, kept running into the suites that came after. Each suite now checks, after its apps are gone, that no KafkaNodePool and no pod labelled strimzi.io/cluster=<cluster> remains in the namespace. The kafka suite deletes its app explicitly so it can look afterwards. The metadata roundtrip's cleanup does not wait for the uninstalls, so its checks carry a budget sized from the pre-delete Job's deadline. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
scooby87
|
03e2da5805 |
feat(postgres,mariadb)!: drop plaintext user passwords (#4078)
## What this PR does Removes the plaintext `users[].password` field from the `apps/postgres` and `apps/mariadb` charts (and the mariadb `root` account), so managed-database passwords can no longer be pinned in tenant values or committed to git. Passwords are always chart-generated into the `<release>-credentials` Secret and preserved across reconciles via `lookup`, matching the model `redis`/`valkey` already use. This closes the "User passwords" item of the portal upstream epic (aenix-org/cozyportal#1060). The generated password never lands in the init-script Secret or the SQL it runs: the postgres init-job reads each user's password from the mounted `<release>-credentials` Secret at run time and applies it through a psql variable (`ALTER ROLE … WITH PASSWORD :'pw'`), so it does not linger in the init-script manifest a tenant can read. It does live in cleartext `stringData` in the `<release>-credentials` Secret itself, which is a chart manifest and therefore kept in Helm release history for `MaxHistory` revisions like any chart-generated secret — that residue is exactly why rotation cannot be built on the chart (see below). A `password` or a `passwordRotation` key left over in a tenant's values is accepted-but-inert and draws an admission warning, so an operator editing it is told it has no effect rather than getting a silent 200. Backup/restore into a copy keeps the target's chart-managed credentials working: the CNPG driver clears `bootstrap.enabled` once recovery is healthy so the init-job re-applies the generated passwords onto the recovered roles, and the MariaDB driver excludes `mysql.global_priv` from the dump (`ignoreGlobalPriv`) for backups captured from this release onward, so restoring one of those into a copy does not clobber the target's chart-managed users or `root`. Excluding the grant table has a flip side, and it applies to every MariaDB restore, not only a to-copy one: only accounts declared through `User` CRs (the chart's `users` map) are captured, so an account created out of band directly in MySQL is in no backup and a restore brings back its data and `mysql.db` grants but not the account itself. This is also a backup-side fix only: a restore from a backup taken **before** this change still replays the source's `mysql.global_priv` and overwrites the target's users and `root`, and `root` then needs an out-of-band reset (the operator applies `rootPasswordSecretKeyRef` only at datadir bootstrap, so there is no in-band path). See `docs/operations/backup-classes.md` for the convergence window, the out-of-band-account limitation, and this old-backup caveat. Password **rotation** is deliberately **not** in this PR. A chart-rendered credentials Secret keeps its cleartext in Helm release history for `MaxHistory` revisions, so a rotation built on it cannot revoke a leaked password; rotation is being reworked as a controller that owns the Secret — design proposal in cozystack/community#72. Migration is seamless: existing releases keep their current passwords on upgrade (the value already lives in `<release>-credentials`, which `lookup` re-reads and preserves); a `password` still present in a tenant's values is ignored, so no existing HelmRelease fails validation. Verification: `helm unittest` passes for both charts (postgres 87, mariadb 126, including render-time username guards that reject any name k8s would later reject as an invalid Secret key or User resource name); `go build ./...`, `go vet`, the backup-controller tests and the apps-apiserver tests are green; `make generate` was run with the CI-pinned `cozyvalues-gen` v1.6.0 so `values.schema.json`, the READMEs, the API types, and the `cozyrds` ApplicationDefinitions are all in sync (the branch was rebased onto `main`, which reconciled cleanly with the read-replica autoscaling feature that landed there). ### Screenshots N/A — no UI change. ### Downstream repositories - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: affected — `content/en/docs/next/cozystack-api/go-types.md` is hand-written and still shows `Password:` in the Go examples, and the managed-app reference pages regenerate from these chart READMEs, so both document `users[name].password` until refreshed (not yet filed). - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: affected — the provider hand-maintains the postgres/mariadb schemas and per-field defaults; this PR removes `users[].password`. A follow-up issue/PR is needed there (not yet filed). `terraform-provider-cozystack` and `website` are reached by this diff; both boxes are left unticked because the follow-ups are not yet filed. Same-repo future work this change suggests: the controller-owned password rotation (design in cozystack/community#72), and extending the "passwords only in the Secret" model to the other engines that still accept plaintext (`clickhouse`, `mongodb`, `nats`, `opensearch`, `rabbitmq`, `vpn`). ### Release note ```release-note feat(postgres,mariadb)!: passwords for managed PostgreSQL and MariaDB users can no longer be set via `users[].password` — they are always auto-generated and stored only in the `<release>-credentials` Secret, never rendered into the init-script or the SQL it runs. Existing releases keep their current passwords on upgrade; a `password` (or a `passwordRotation`) left in values is ignored with an admission warning. Password rotation is deferred to a follow-up controller design (cozystack/community#72). MariaDB backups now exclude the `mysql.global_priv` grant table (`ignoreGlobalPriv`), so a restore recreates only accounts declared through the chart's `users` map — an account created out of band directly in MySQL is not captured and must be recreated after a restore. ``` |
||
|
Aleksei Sviridkin
|
ab0c9f6e29 |
docs(e2e): document jq requirement for bucket suite (#4457)
<!-- 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 `kind/backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does Documents `jq` as a host prerequisite for the `bucket` Chainsaw suite, whose command steps use it to parse S3 responses. Fixes #4295 ### Screenshots N/A — documentation-only change. <!-- 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: - [ ] [cozystack/community](https://github.com/cozystack/community) - 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 docs(e2e): document jq as a host prerequisite for the bucket Chainsaw suite ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Updated the local-run prerequisites for the `bucket` suite to include `jq`, alongside `mc` and `nc`. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
nightcityblade
|
fdcbc5c83c |
docs(e2e): document jq requirement for bucket suite
Assisted-by: LLM Signed-off-by: nightcityblade <nightcityblade@gmail.com> |
||
|
Alexey Artamonov
|
f02ca8cbc2 |
test(backups): log in as an application user after each restore
The backup examples and e2e suites no longer set passwords. They read the generated ones from <release>-credentials with jq, since a username may contain a dot that jsonpath treats as a path step, and log in as an application user after each restore, so a restore that leaves the credentials unusable fails the run. The MariaDB run also checks that an account created out of band on the source is absent from the copy. The unreachable-target restore step asserts a terminal Failed attributed to the recovery window, since the reason itself is best effort. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Aleksei Sviridkin
|
a92e89e969 |
docs(kubernetes): describe the OIDC UID header from v1.33
The tenant OIDC guide, the version field description and the e2e helper comment still described a --feature-gates term rendered on v1.32. Left as they were, they would tell an operator that the chart switches that alpha gate on, which it no longer does. The guide now says what a v1.32 tenant has to write itself to get the UID, and which --emulated-version values fail the render, so an operator who meets that refusal finds it explained. The breaking-changes note in the README said the chart refuses the mixed gate spelling while OIDC is on; with that refusal gone it would promise a render error that no longer comes. The version description returns to its earlier wording in values.yaml and in the update-versions.sh template, because that template rewrites the line on every version bump and would otherwise bring the old text back. make generate carries it into the schema, README, API types and ApplicationDefinition. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Timofei Larkin
|
95ece888ba |
feat(kafka): add topic-data backup/restore example and e2e roundtrip (#3580)
## What this PR does
Adds a worked backup/restore example for a Cozystack-managed `Kafka`
application's **topic data**, built on the generic `Job` backup strategy
— the same pattern as `examples/backups/nats`, with no purpose-built
backup image. A stock Strimzi Kafka image (the `kafka-*.sh` CLI plus
`bash`, `curl` and `tar`) runs one shell script that branches on `{{
.Mode }}`: on backup it freezes each partition's end offset and drains
`[begin, end)` to a tarball it `PUT`s to S3; on restore it `GET`s the
tarball, recreates the topics with their original partition count and
replication factor, and replays every partition file. S3 access is `curl
--aws-sigv4`, verified against the endpoint's CA projected from the
`<app>-backup-s3` Secret, so no extra client image is needed.
The consistency model is a frozen-end-offset cut: `kafka-get-offsets
--time -1` is read once per partition at backup start and the consumer
drains only up to that offset, so anything produced during the run is
excluded and every partition of a topic is captured as of the same
instant — a cut by log position, independent of record timestamps.
This is deliberately a DATA-only backup. Topic configs, ACLs, SCRAM
users and **consumer-group offsets** are not captured, and re-produced
records receive fresh offsets — the README spells out the scope and
points at a volume-snapshot strategy for a faithful, offset-preserving
backup. Keyed records reproduce into their original partition because
the topic is recreated with the same partition count and the default
(murmur2) partitioner is deterministic.
The script fails closed rather than reporting a partial backup or a
lossy restore as success: it refuses an offset listing short of the
topic's partition count, a drain short of `end - begin`, a `topics`
value with a duplicate or a name outside Kafka's charset, a manifest
that disagrees with the data files, a target topic that already holds
records (checked before the replay, so a retried Pod cannot append a
second copy), a pre-existing topic with a different partition count or
replication factor, a replication factor below the target's effective
`min.insync.replicas`, and a post-replay count that does not match the
manifest. Each topic's replication factor is recorded in the manifest
and restored as captured; the `replicationFactor` BackupClass parameter
overrides it for restoring into a smaller cluster.
Also adds a `kafka-2-backup-roundtrip` Chainsaw test to
`hack/e2e-chainsaw/kafka`, which drives the example `run-all.sh` as its
harness (Bucket → two-broker source Kafka with a seeded 3-partition,
2-replica topic plus a decoy topic that the demo topic matches as a
regex but not as a literal → BackupJob → in-place restore with count,
content and replication-factor assertions → a second RestoreJob into the
populated topic that must be refused with the topic untouched →
two-broker target Kafka + to-copy restore + verify), mirroring
`mariadb-2-backup-roundtrip`. Two brokers, not one, because that is the
shape on which the chart sets `min.insync.replicas: 2` and a restore
with the wrong replication factor cannot land a record. It resolves the
Kafka image from the running operator so the backup Pod reuses the image
already cached on the nodes.
The strategy script itself is pinned by
`hack/kafka-topic-backup-strategy.bats`, on the shape of
`hack/kafka-backup-script.bats`: it renders the strategy through a stub
`kubectl` exactly as step 01 applies it, lifts the container body out,
and executes it against stubbed `kafka-*.sh`, a stubbed `curl` and a
directory standing in for S3, with fault injection for the broker
behaviours the guards exist for. Fourteen cases, one per guard; removing
the partition-listing guard reddens one of them.
Verified: `chainsaw/kafka-2-backup-roundtrip` passed in this PR's CI on
the two-broker shape (record count, byte-identical content dump and
replication factor 2 asserted after both restores, the refused
RestoreJob settled Failed with the topic untouched);
`hack/kafka-topic-backup-strategy.bats` passes under both
`hack/cozytest.sh` and bats.
Files:
- `examples/backups/kafka/` — 14 files (`00-helpers.sh`, `01`..`07`,
`run-all.sh`, `cleanup.sh`, `README.md`, `90`/`91`/`92` scenario
walk-throughs).
- `hack/kafka-topic-backup-strategy.bats` — hermetic test of the
strategy script.
- `hack/e2e-chainsaw/kafka/chainsaw-test.yaml` — new
`kafka-2-backup-roundtrip` Test appended.
- `hack/backup-access-preflight.bats` — the kafka example and Test join
the S3-preflight roster.
- `hack/select-e2e.sh`, `hack/e2e-chainsaw/.chainsaw.yaml` —
comment-only: their enumerations of preflight-gated suites are replaced
by a pointer.
- `.gitignore` — the example's `.source-dump.txt` snapshot.
No package sources (`values.yaml`, `values.schema.json`, `Chart.yaml`,
package `README.md`) are touched, so `make generate` produces no diff.
### Screenshots
N/A — no UI changes.
### Downstream repositories
Walked the trigger map in `docs/agents/contributing.md` file-by-file
against the diff. The change adds example scripts under
`examples/backups/kafka`, one unit test under `hack/`, and appends one
Chainsaw Test to an existing file under `hack/e2e-chainsaw/kafka` — it
adds no package under `packages/apps` or `packages/extra`, changes no
CRD, no `ApplicationDefinition`, no `values.schema.json`, no
platform/installer values, no `hack/*.mk` anchor or make-target
behaviour, and no node prerequisites in `hack/e2e-prepare-cluster.bats`.
None of the downstream triggers match.
- [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(kafka): add a topic-data backup/restore example and e2e round-trip on the generic Job backup strategy (data-only: topic records with a frozen-end-offset consistency cut and partition-preserving replay; consumer offsets, topic configs, ACLs and users are out of scope)
```
|
||
|
Aleksei Sviridkin
|
1f30ac4598 |
fix(tests): let the mariadb waits cover a first boot (#3769)
## What this PR does Closes #3741 Several waits for a database HelmRelease to become Ready gave up before the release could: the mariadb, mongodb, rabbitmq and clickhouse backup walkthroughs waited 300 seconds, and both mariadb Chainsaw suites gave their HelmRelease-ready assert 6 minutes. A generated release carries a 10-minute install timeout (`pkg/cmd/server/start.go:91`), so a failed install was reported as a bare timeout, because the release had not yet recorded why it failed, and for mariadb a first boot the startup probe was still willing to wait for failed the harness instead. A wait on such a release has two floors and has to beat the larger. The first is the app's own first boot: cozystack-api leaves `spec.waitStrategy` unset and helm-controller defaults it to `poller`, so the install waits through kstatus, and kstatus maps the operator CR's `Ready=False` onto `InProgress`. For mariadb that boot may take the chart's 310-second startup budget (`startupProbe.failureThreshold: 30` in `packages/apps/mariadb/templates/mariadb.yaml` with the operator's probe defaults of 20 and 10); mongodb, rabbitmq and clickhouse set no startup probe. The second is the install timeout. At or below it the waiter is the one that expires, because its clock starts when the step runs and the install's only once the Application has been reconciled and the chart artifact fetched. The install timeout binds in every case here, so the walkthrough waits go to 660 seconds and the mariadb asserts to 11 minutes. Raising a wait inside a walkthrough spends the Chainsaw op that runs the script, and an op that runs out SIGKILLs the process group and loses the script's error. Each op therefore grows by exactly what the waits it reaches gained: mariadb's round-trip from 30 to 42 minutes, rabbitmq's from 30 to 42, mongodb's from 30 to 36 (its round-trip runs with `SKIP_RESTORE`, so only the source wait counts), and clickhouse's from 60 to 72, which still sits above the sum of every inner ceiling. That bounds a failure at a single wait, not a run that is slow at every stage and then fails late; bounding that needs a phase budget, not a larger literal. postgres keeps its 360-second waits and every walkthrough keeps its 300-second bucket wait: whether a suite pays the extra minutes on every red run is a per-suite call. The mariadb endpoint asserts keep their job. A freshly applied CR carries no conditions and kstatus reads it as current, so the install wait can end before any pod is up; an endpoint address closes that window. That is also why the `wait_for_field` on the CR stays in the walkthrough. `wait_hr_ready` now dumps every condition and the release history on timeout and on `Stalled=True`, instead of the single Ready message a scheduled retry can overwrite before anyone reads it. It also stops printing one `(none recorded)` line for different answers: a release that never appeared, a release deleted during the wait, a lookup kubectl could not answer, a failed history read and an empty history each say so, with kubectl's own stderr kept, and a release seen earlier still gets its dump when only the last lookup failed. The change goes to every byte-identical copy of the helper, in `examples/backups/{clickhouse,mariadb,mongodb,postgres,rabbitmq}/00-helpers.sh`, and they stay identical. `hack/mariadb-first-boot-waits.bats` pins the mariadb budgets against the tree. It derives both floors from their sources instead of pinning literals, so raising `failureThreshold` past the install timeout, or changing the install-timeout default, reds it; each source has a test that refuses to proceed when it cannot be read, including the ApplicationDefinition, whose two override annotations would each invalidate the derivation. It holds the round-trip op at a fixed remainder over the ceilings up to and including the source wait, bucket stage and S3 preflight included, and at a fixed slack once both mariadb waits are paid, so raising a wait without the op reds. The extractors are also tested against fixtures, because the tree cannot tell a correct extractor from a broken one. Beyond mariadb, the file runs every exit branch of `wait_hr_ready` in each copy against a stubbed kubectl, and pins the copies byte-identical. The population is every definition in the tree found by grep, minus two named exceptions held to still differ, so a new copy joins without editing the test and an exception whose copy converges goes red: - `examples/backups/redis/00-helpers.sh` is written in the compact one-line style of the rest of that file, so converging it rewrites its shape rather than applying the shared body. - `examples/backups/kafka-metadata/00-helpers.sh` is a helper of its own with the same diagnostic gap on timeout (it prints no reason at all), and closing that is a change to kafka-metadata. `docs/agents/e2e-testing.md` replaces the reviewer checklist's 5-6m standard and its list of exceptions, which the tree had outgrown, with the floor a wait has to beat and the rule that a longer wait carries its reason in-line. The derivation gets a section of its own, opening with the rule in one line. For a release an app chart renders itself the floor is `spec.install.timeout`, then `spec.timeout`, then helm-controller's 5m default, and the nested `<name>-system` releases never take cozystack-api's 10m. The harbor comments name the floor each of their asserts sits against, and the opensearch comment stops citing the removed standard. Adding `hack/mariadb-first-boot-waits.bats` does not escalate the E2E selection: `hack/[^/]+\.bats$` sits in `inert_config_pattern` in `hack/select-e2e.sh`, and `full_suite_pattern` escalates only `hack/e2e-*.bats`. On this changeset `hack/select-e2e.sh` selects `clickhouse harbor mariadb mongodb opensearch postgres rabbitmq`. What this costs is paid only on a failing run; the raised numbers are ceilings. Summing the timeouts stated on each suite's own steps, mariadb goes from 92m to 114m, clickhouse from 116m to 128m, rabbitmq from 67m to 79m and mongodb from 95m to 101m, before the untimed steps that fall back to the defaults in `.chainsaw.yaml`. clickhouse is the most expensive suite by that sum. All of them compete for the 215m `timeout-minutes` of the whole `e2e` job, which also covers image download, sandbox preparation and the Cozystack install, and a job cancelled at the cap loses both its log and `cozyreport.tgz`. This does not fix the failure that led here. In the run that prompted the investigation the mariadb replica never opened port 3306 at all, spending its whole 310-second budget inside `mariadb-install-db` before being killed, and no wait budget would have saved it. That defect is #3744. What this fixes is a second, independent defect found while reading the same code. One thing here is argued rather than measured: the mechanism is checked against helm-controller and kstatus, and every number is derived from the tree, but no run ID pins that it is what any particular red run hit. Both floors justify 660s whatever the cause was, so being wrong about the cause leaves slack rather than breakage. ### Not in this PR - This PR retires the mariadb, mongodb, rabbitmq and clickhouse walkthrough waits and the two mariadb asserts. The rest is tracked in #4404: the other Chainsaw HR-Ready asserts under their floor, the harbor asserts, the 300-second bucket waits, the postgres waits, and the redis and kafka-metadata diagnostic gap. - The clickhouse round-trip comment says every wait fails fast, `wait_hr_ready` on `Stalled=True`, "so a genuinely broken backup reports in seconds". A release whose install is still running is neither Ready nor Stalled, so that holds for a stalled release and not for an install that has not finished. The sentence predates this change. - `hack/mariadb-first-boot-waits.bats` is large for what it guards (two floors, two ratchets on the op, and the helper identity pin), with many fixture tests on its own extractors. Trimming it to that core is a separate decision. - Two comments in that file state facts about the mariadb suite that are true today and that nothing checks: three script ops carry a minute timeout, and the only one ahead of the round-trip is the 8m pre-clean. - The mariadb op comment says a bound on the whole flow "needs a phase budget (docs/agents/e2e-testing.md)"; the doc describes it as bounding the collection phase as a phase (`COZY_DIAG_PHASE_BUDGET`), so the pointer would resolve faster if it named that bullet. - Cosmetic: the behavioural test's loop body is not indented under its `for`, one line of the bats header runs past the width of the rest, and the checklist's sentence under the example assert ("what this block demonstrates least") reads awkwardly. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff, file by file, for all eighteen files. The five `examples/backups/*/00-helpers.sh` files change the diagnostics of `wait_hr_ready`, and the mariadb, mongodb, rabbitmq and clickhouse walkthrough scripts change wait budgets; the manifests the walkthroughs apply and the flows they document are unchanged, so no website page restates anything that moved. Four Chainsaw suites (mariadb, clickhouse, mongodb, rabbitmq) change timeouts and comments, and the harbor and opensearch suites change comments only. `docs/agents/e2e-testing.md` changes the reviewer checklist entry and adds the sizing section. `hack/mariadb-first-boot-waits.bats` is a new file picked up by the existing `BATS_UNIT_FILES` wildcard. `hack/package.mk`, `hack/common-envs.mk` and `hack/update-crd.sh` are untouched, and no `values.schema.json`, `values.yaml` default, version enum, `ApplicationDefinition`, CRD, namespace or release asset is touched. - [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: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(tests): the MariaDB, MongoDB, RabbitMQ and ClickHouse backup examples and the MariaDB Chainsaw suites now wait for a HelmRelease past the release's own install timeout, so a failed install reports its own reason instead of a bare timeout, and the examples' HelmRelease wait now tells a release that never appeared, one deleted mid-wait and a kubectl that could not answer apart. ``` |
||
|
Andrey Kolkov
|
7ce85a2f7f |
test(kafka): pin the strategy script's guards with a hermetic bats suite
The in-Pod script had grown eight fail-closed guards across as many rounds, and the only thing exercising any of them was a 40-minute cluster test that drives the happy path plus one refusal. The partition listing guard in particular - the whole of the previous round's second fix - could be deleted with nothing in the tree going red. Add hack/kafka-topic-backup-strategy.bats on the shape of hack/kafka-backup-script.bats: render the strategy through a stub kubectl exactly as step 01 applies it, lift the container body out of it, and execute it against stubbed kafka-*.sh, a stubbed curl and a directory standing in for S3, with fault injection for the broker behaviours the guards exist for. Deleting that listing guard now fails one case; the script gains BIN/SCRATCH/CA_FILE overrides so it can run outside the Pod, each defaulting to the Pod's value. The same rounds' smaller findings: the manifest's topic name is held to Kafka's charset like the topics parameter, since it reaches unquoted expansions with globbing on; the shape check after --create compares the replication factor as well as the partition count, and refuses a replication factor below the topic's effective min.insync.replicas before replaying, because kafka-console-producer only logs a rejected acks=all write and the run would otherwise end at the count check with the cause unnamed; the manifest's replication factor must agree across a topic's lines; topic_records carries its own || return 1 on each command, as a function called as the operand of || runs with errexit off; the CA lookup in step 03 distinguishes a denied read from an absent Secret, and the README's persona column says the copy needs the admin; the e2e comment no longer claims the 2-label endpoint fails the serving cert's SANs, which include '*.tenant-root'. Rebased onto main, which moved the chart to KRaft: the demo's Kafka CRs drop the zookeeper block the chart no longer has. Signed-off-by: Andrey Kolkov <andrey.kolkov@aenix.io> Assisted-By: LLM |
||
|
Andrey Kolkov
|
5bd029c46a |
fix(kafka): restore with the captured replication factor and a complete listing
Record each topic's ReplicationFactor in the backup manifest, off the same --describe line the partition count already comes from, and recreate the topic with it on restore. The value used to be a BackupClass parameter defaulting to 1, and the Cozystack Kafka chart sets the broker-level min.insync.replicas to 2 as soon as kafka.replicas reaches 2: kafka-console-producer writes with acks=all, the broker rejects every write into a topic with fewer replicas than that, and the producer logs the rejection and exits 0. A restore onto any multi-broker cluster therefore landed zero records and failed the post-replay count with a message naming the wrong cause; in place, the documented delete had already taken the live topic with it. replicationFactor stays as an override for restoring into a smaller cluster, and a value the target cannot host now fails at --create in Kafka's own words. Hold the pre-replay emptiness check to the topic's partition count, as the backup side already does. kafka-get-offsets skips a partition it could not read and still exits 0, so a leader election in flight dropped the populated partitions from the listing, the surviving empty ones summed to zero, and the replay appended a full copy into a live topic - the exact outcome the check was moved ahead of the replay to prevent. Both restore-side counts now go through one helper that refuses a short listing. The data-file-versus-manifest check runs both ways before anything is created, and the replay walks the manifest topic by topic, announcing each one, so a retried Pod's log says which topics landed. The demo now runs two brokers with two-replica topics, which is the shape the chart puts min.insync.replicas=2 on; the restore steps assert the restored topic's replication factor, and step 06 submits a second RestoreJob against the populated topic and requires it to be refused with the topic untouched. Smaller findings from the same round: the topics parameter is held to Kafka's name charset (a literal "*" no longer path-expands) and refused on a repeated name, which produced a Succeeded backup no restore could balance; the strategy verifies S3 TLS against the seaweedfs CA projected into the <app>-backup-s3 Secret instead of curl -k, following the rabbitmq example, so the e2e endpoint moves to the .svc name the serving cert covers; the image lookup keeps digest-pinned references; the default namespace is tenant-root, the only one the demo ever ran in; step 05 caches the resolved Backup name for the restore steps; the decoy count subtracts begin offsets like the topic count; the bats test name carries its count again; and the scenario walk-throughs the sibling examples ship are added. Signed-off-by: Andrey Kolkov <andrey.kolkov@aenix.io> Assisted-By: LLM |
||
|
Andrey Kolkov
|
d405a36e3c |
fix(kafka): validate the backup's partition set against the topic
The drain guard only covers a short read within a partition it was told about. kafka-get-offsets lists just the partitions whose lookup succeeded - GetOffsetShell logs a per-partition KafkaException to stderr, skips it and still exits 0 - so a partition lost to a leader election got no manifest line, no data file and no drain check, and the tarball was uploaded with the Backup reporting Succeeded. The restore-side count check could not catch it either, since it sums the same short manifest, and a missing highest-numbered partition made restore recreate the topic with fewer partitions, re-placing every keyed record. Read the topic's own PartitionCount before the drain and fail closed when the offset listing is short. A short "begins" needs no separate check: its per-partition default of 0 can only overstate n, which the existing drain check then catches. Also run the shared BucketAccess preflight in the e2e - accessGranted does not prove the S3 server reloaded the new identity, and a fresh grant answering the SigV4 PUT with 403 reds the run as a backup defect - and state the .chainsaw.yaml namespace override as the rule it is rather than as a claim about which Tests match a name pattern. Signed-off-by: Andrey Kolkov <andrey.kolkov@aenix.io> Assisted-by: LLM |
||
|
Andrey Kolkov
|
7d8dee66a5 |
fix(kafka): verify restore landed and namespace-scope the S3 key
- Restore verification: kafka-console-producer, like the consumer on backup, exits 0 without every record being accepted, so a replay that dropped records still reported Succeeded. After the replay, compare each restored topic's record count (sum of end-begin over partitions) against the manifest and fail on any shortfall or overshoot - the latter being a restore into a non-empty topic, which this appending DATA restore does not support. - Namespace-scope the S3 object key (<namespace>/<app>/kafka-topics.tar) so two same-named apps in different namespaces sharing one bucket do not collide. The namespace is .Release.Namespace in both modes and always names the source, since a to-copy target shares the source's namespace (a TypedLocalObjectReference). - Warn instead of failing open when the Kafka image cannot be resolved from the operator: the pinned public literal is unreachable on an air-gapped cluster, so tell the caller to set KAFKA_IMAGE. - Word the .chainsaw.yaml namespace note as "most" (etcd-2-backup- roundtrip sets no namespace) and document the empty all-topics backup. Signed-off-by: Andrey Kolkov <andrey.kolkov@aenix.io> Assisted-by: LLM |
||
|
Andrey Kolkov
|
9673af6aca |
fix(kafka): guard short backup drains and settle example teardown
- Short-drain guard: kafka-console-consumer exits 0 after a timed-out short read, so a truncated drain was tarred and reported Succeeded. Compare the drained line count against end-begin per partition and fail the backup on any shortfall, making the README's "fails closed" promise real for compacted/transactional topics. - Settling teardown: cleanup.sh returned before the Kafka apps and Bucket had uninstalled and could not report failure, letting the neighbouring chainsaw/kafka Test contend on PVC reclaim. Add a wait_deleted helper and a teardown_rc gate that waits for the heavy objects and exits non-zero when the teardown does not settle. - Fully qualify the ambiguous delete kinds (backup/bucket/backupclass/ restorejob/backupjob) so a bare shortcut cannot target a foreign group and silently leave the Cozystack artifact behind. - Default KAFKA_IMAGE from the running operator (newest STRIMZI_KAFKA_ IMAGES entry), falling back to the literal only when the operator cannot be read; drop the duplicate resolution note. - Make the strategy and throwaway CLI Pods restricted-clean (runAsNonRoot + JSON-patch securityContext on the CLI Pods). - gitignore .source-dump.txt; replace the deprecated "small" preset with c1.small; capture the to-copy cluster in the e2e catch; document the all-topics risk; de-enumerate the decaying suite-list comments. Signed-off-by: Andrey Kolkov <andrey.kolkov@aenix.io> Assisted-by: LLM |
||
|
Andrey Kolkov
|
34a04e6b37 |
chore(kafka): document copy-safety caveats and harden all-topics loop
Document the caveats that make the example safe to copy - a null key round-trips as the literal string "null" (changing key and compaction identity), the drain stages the topic twice under /tmp with no ephemeral-storage bound, every backup of an app overwrites the same S3 key, and the Pods target warn-only Pod Security. Harden the all-topics partition-count loop: the final `[ ... ] && parts=...` returned non-zero under `set -e` when the last test was false, so use `if ... then ... fi`. Align the default KAFKA_IMAGE with the vendored operator (0.45.1-rc1) and list kafka-2- in the .chainsaw.yaml tenant-root header. Signed-off-by: Andrey Kolkov <andrey.kolkov@aenix.io> Assisted-by: LLM |
||
|
Andrey Kolkov
|
d38b538d98 |
test(kafka): fail fast on terminal backup phases and capture strategy Job logs
Two CI-quality fixes, both diagnostics/wall-clock only (neither can turn a run falsely green): - The chainsaw comment claimed run-all.sh fails fast on terminal BackupJob/RestoreJob phases and Stalled HelmReleases, but the kafka helpers never implemented it (carried over from mariadb). A BackupJob that settled phase=Failed was polled the full 600s and then mislabelled 'Timeout', burning ~10 of the 40 chainsaw minutes per already-red run. Port the mariadb variants: wait_for_field gains a terminal fail_value arg (call sites pass Failed), and a new wait_hr_ready fails fast on Stalled=True with an existence backstop. HR waits in steps 03/04/07 now use it; the Strimzi Kafka CR waits keep their genuine Ready condition. run-all.sh runs under set -e, so a non-zero step aborts the flow immediately, matching the comment. - The catch block only captured the Strimzi broker Pods (app.kubernetes.io/instance=kafka-kafka-test); the generic Job strategy runs the drain/replay in controller-created Pods labelled job.strategy.backups.cozystack.io/mode=backup|restore (never that instance label) and not TTL-reaped, so the actual S3/SigV4 error left no log in the report. Add a second podLogs selector keyed on that label. Signed-off-by: Andrey Kolkov <andrey.kolkov@aenix.io> Assisted-by: LLM |
||
|
Andrey Kolkov
|
00943392cc |
fix(kafka): resolve a single Kafka image for backup e2e harness
STRIMZI_KAFKA_IMAGES is a newline-separated version=image map with one entry per supported Kafka version. The old 'sed | tr -d [:space:]' collapsed every line into one string, producing an invalid multi-image reference (quay.io/...3.8.0quay.io/...3.8.1...) that failed 'kubectl run' with 'invalid reference format' and broke kafka-2-backup-roundtrip. Split on whitespace, keep image lines, strip the version prefix and take the last (newest) image so a single valid reference is exported. Signed-off-by: Andrey Kolkov <andrey.kolkov@aenix.io> Assisted-by: LLM |
||
|
Andrey Kolkov
|
7842b88676 |
feat(kafka): add topic-data backup/restore example and e2e roundtrip
Add examples/backups/kafka, a worked backup/restore of a Cozystack-managed Kafka application's topic data via the generic Job backup strategy - the same pattern as examples/backups/nats, with no purpose-built backup image: a stock Strimzi Kafka image (kafka-*.sh CLI + bash/curl/tar) runs one shell script that branches on .Mode. Consistency is a frozen-end-offset cut: kafka-get-offsets --time -1 is read once per partition at backup start and the consumer drains only up to that offset, so anything produced during the run is excluded and every partition is captured as of the same instant. Restore recreates each topic with its original partition count and replays every partition file; keyed records reproduce into their original partition via the default partitioner. This is deliberately a DATA-only backup: topic configs, ACLs, SCRAM users and consumer-group offsets are not captured, and re-produced records get fresh offsets. The README spells out the scope and points at a volume-snapshot strategy for faithful, offset-preserving backups. Add kafka-2-backup-roundtrip to hack/e2e-chainsaw/kafka, driving the example run-all.sh as the harness (Bucket -> source Kafka + seeded topic -> BackupJob -> in-place restore -> to-copy restore + verify) so the test and the documented flow cannot drift, mirroring mariadb-2-backup-roundtrip. The image is resolved from the running operator so the backup Pod reuses the cached Kafka image. Verified end to end on a live cluster: freeze cut orders:0 [0,8), orders:1 [0,11), orders:2 [0,11) = 30 records; in-place and to-copy restores both return 30 with the original 8/11/11 partition distribution. Signed-off-by: Andrey Kolkov <andrey.kolkov@aenix.io> Assisted-by: LLM |
||
|
Aleksei Sviridkin
|
b1519acfe7 |
fix(e2e): cap the bucket S3 reads so the verify step honors its timeout (#4281)
## What this PR does Refs #4268 — a plain reference, not `Fixes`: the issue asks for a `script` op to be cut off at its declared ceiling, and that is Chainsaw behaviour this repo cannot change. Chainsaw runs the op's shell under Go's `exec.CommandContext`, whose default cancel SIGKILLs the shell PID alone and then blocks on the inherited stdout/stderr pipe until an orphaned `mc` closes it, so the op returns only when `mc` exits on its own rather than at the ceiling. This PR mitigates that where it bit — per-command caps so no descendant outlives its own budget — but the op-timeout gap itself lives in Chainsaw, so the link stays a reference rather than a `Fixes`. The sibling step that produced the 606s, `verify-every-bucket-takes-a-write` in the `bucket-1-capacity` Test, is on `main` and already carries its own per-call caps; this PR applies the same shape to `verify-s3-access`. On run 34948279363 the bucket suite's `verify-every-bucket-takes-a-write` step declared `timeout: 5m` but ran 606s and ended on `mc`'s own non-zero status rather than being cut at the ceiling. `verify-s3-access` in `hack/e2e-chainsaw/bucket/chainsaw-test.yaml` is the same-shaped step in this suite. This PR wraps every S3 (`mc`) and apiserver (`kubectl`) read in `verify-s3-access` in its own `timeout`, so a hung read dies at a known budget instead of holding the step open, and corrects the two places (`hack/e2e-chainsaw/.chainsaw.yaml` and `docs/agents/e2e-testing.md`) that explained the op ceiling with a mechanism that does not hold. It caps `verify-s3-access` for sharing the shape, not for producing the 606s number. ### Which of the issue's two candidate mechanisms holds The issue named two candidates and stated the log could not separate them. The Chainsaw v0.2.15 source separates them, and it is the second one, with the exact refinement the issue proposed. **Candidate 1 — "an operation `timeout` does not bound a `script` op" — is refuted.** The op timeout is applied and does reach the process: `pkg/runner/operations/script.go:24` copies the step's `timeout` into `Timeouts.Exec`, and `:38` wraps the op context with `ctx, cancel := context.WithTimeout(ctx, tc.Timeouts().Exec)` before calling `op.Exec(ctx, …)`. So a 5m step produces a real 300s deadline on the context the command runs under. **Candidate 2 — "it bounds it and kills the shell, but Chainsaw waits on a pipe a grandchild (`mc`) still holds open" — is confirmed.** In `pkg/engine/operations/script/operation.go` the command is built with `exec.CommandContext(ctx, shell, …)` (`:105`), and in `execute` its stdout/stderr are set to `*bytes.Buffer` (`:128`–`:129`, the buffers are `internal.CommandOutput` in `pkg/engine/operations/internal/command_output.go:12`–`13`) and run via `cmd.Run()` (`:130`). Two Go `os/exec` properties then produce the overrun: `exec.CommandContext`'s default cancel is `Process.Kill()` on the shell PID alone — not a process-group kill, and the operation sets no `SysProcAttr.Setpgid` — and because stdout/stderr are not `*os.File`, `os/exec` allocates OS pipes and copier goroutines, so `cmd.Wait()` blocks until those pipes reach EOF. EOF needs every inherited write-end closed. When the shell forks `mc`, `mc` inherits those fds; at 300s the deadline SIGKILLs the shell but `mc` is orphaned, keeps retrying the S3 PUT, and holds the pipe, so the op does not return until `mc` exits on its own at 606s. `cmd.Run()` then returns `signal: killed`, which is stored in the `$error` binding and never printed — only the `($error == null): true` check failure is logged — which is why grepping the run for `signal: killed` found nothing. **Empirical confirmation.** A standalone Go program reproduces it exactly. `exec.CommandContext` with a 1s timeout, `*bytes.Buffer` stdout/stderr, running `sh -c 'sleep 2 & sleep 4'` (a descendant that inherits the pipe): `cmd.Run()` returns after ~4s, not 1s, with `err=signal: killed` and `ctx.Err()=context deadline exceeded` — the ceiling fired at 1s, control came back only when the descendant closed the pipe. Redirecting the descendants' stdout/stderr away from the pipe (`… >/dev/null 2>&1`) returns at 1.00s. That is the whole mechanism: the ceiling cuts the shell but not a descendant that still holds the pipe. ### The fix Per-command caps, the remedy the issue itself points to ("every read carries its own cap") and the shape already used elsewhere in the suite. The bucket step now defines a `k()` wrapper (`timeout 30 kubectl`) and an `mc()` wrapper (`timeout 15 mc`) for the reads, and calls `timeout 45 mc cp` directly for the two PUTs; a hung read is killed at its own budget, releasing the inherited pipe promptly, so even when the op ceiling fires the residual is one inner cap rather than `mc`'s full retry exhaustion. The terminal precedents for these budgets are the redis suite's terminal `k()` (30s, `redis/chainsaw-test.yaml:298`) and the sibling `verify-every-bucket-takes-a-write` step's terminal `mc cp` (45s, `:390`, sized for a volume grow); the 15s cap on `mc` in `hack/e2e-chainsaw/_lib/backup-access-preflight.sh` is not a precedent, because it sits inside a `while :;` retry loop where a 124 costs one attempt, whereas here under `set -e` a 124 ends the step. The inner budgets — 2×30 for the two `k` reads + 2×45 for the two `mc cp` PUTs + at most 7×15 for the other `mc` calls (eight other call sites, but the two cleanup `mc rm` are mutually exclusive) + the 30s port-forward readiness wait = at most 285s — sit under the 5m op ceiling, which stays as a backstop, per the "op above the sum of inner budgets" rule in `docs/agents/e2e-testing.md`. The long-lived `kubectl port-forward` keeps no cap by design (it must persist for the whole step, and its stdout/stderr are already redirected to `/dev/null`, so it cannot hold the step's pipe). The `mc()` wrapper is non-recursive because `timeout` execs its argument through a PATH lookup that bypasses shell functions — verified in `dash`, `bash` and `sh`. The `catch` block's diagnostic read gets the same treatment — `timeout 30 kubectl` under a 1m op `timeout` — for the same pipe reason. ### The readonly deny probe fails on a timeout The readonly-upload probe used to branch on `mc` directly (`if mc cp …; then`), which suspends `set -e`, so a `timeout` there (exit 124) was indistinguishable from the access-denied non-zero the probe expects: a 45s hang on the deny path would exit 124, take the false branch, reach cleanup and exit 0 — a long hang turned green, the exact window this PR exists to bound. This is the first `ro-user` write in the step, so no earlier call proved that path healthy. The probe now captures the status: exit 0 is the readonly regression, 124 is a timed-out probe, both fail; any other non-zero is the expected denial. ### No unit surface; why the evidence is the proof This is an e2e-config and doc-correctness change. The embedded Chainsaw script is not extracted into a unit harness anywhere in the tree, so there is no red-to-green unit test to add without inventing a bespoke script-linter (which would be a new checker standing in for the fix). The regression is instead pinned by the mechanism proof above: the source citations show the ceiling cannot cut a descendant that holds the pipe, and the caps remove the only way a descendant outlives its budget. The deny-probe status handling and the pipe-vs-file secret reads were exercised against stub exit codes (0 → regression-fail, 124 → timeout-fail, other non-zero → expected denial) and the whole script passes `sh -n` and `bash -n`. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff. The change touches only `hack/e2e-chainsaw/` suite content and `docs/agents/e2e-testing.md`; it moves or renames nothing under `hack/`, and touches no node-prerequisite or `package.mk` path. No downstream repository is reached. - [x] No downstream repository is affected by this change ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved end-to-end S3 access verification so stalled Kubernetes API and S3 operations fail promptly. - Correctly distinguishes timed-out access checks from expected permission denials. - Preserves partial command output and allows subsequent test steps to continue after individual timeouts. - Applies separate time limits to bucket reads, uploads, and access checks for more predictable test behavior. - **Documentation** - Clarified operation timeout behavior and recommended per-command limits within the overall operation limit. - Documented timeout handling for orphaned processes and updated worst-case timing guidance. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
ec706c49b1 |
docs(e2e): correct the Chainsaw Test-ordering claim in etcd and clickhouse suites (#4277)
Fixes #4269 ## What `hack/e2e-chainsaw/etcd/chainsaw-test.yaml` explained its layout with a comment claiming Chainsaw executes Tests in **name order, not file order**, and used that claim to justify why teardown lives inside each scenario. The clickhouse suite had picked up the same claim. This removes the ordering claim from both — it is false at the pinned version — and re-grounds the etcd teardown justification in a reason that does not depend on order. ## Why the claim was wrong (evidence, Chainsaw v0.2.15 — the pinned version) At v0.2.15 Chainsaw guarantees **no order between Tests at all**: not file, not document, not name. Restating "name order" as "document order" is wrong for the same reason. Verified against the pinned source: - `pkg/runner/runner.go` — the runner registers each Test through `testing.T.Run` and the per-Test body calls `t.Parallel()` whenever `spec.concurrent` is nil or true. Neither suite sets `concurrent`, so both take that path. Go pauses those subtests and releases them together once the parent returns; they then race for the `-test.parallel` semaphore. - `pkg/runner/flags/flags.go` — `execution.parallel` maps only to `-test.parallel`, a cap on how many Tests run at a time, not an order. - `hack/e2e-chainsaw/.chainsaw.yaml` sets `execution.parallel: 1`, so Tests are serialised — one at a time — but not ordered. The E2E run on this PR's own head demonstrates it: discovery loaded the five selected Tests in document order, then ran `clickhouse-2` before `clickhouse-1` with both etcd Tests interleaved between the clickhouse pair. Serial, non-overlapping, and in an order that is neither file, document, nor name. ## The fix The teardown decision the false claim justified is still correct, for a reason that does not touch order. Each etcd scenario shares the singleton `etcd` release and its cluster-scoped Kamaji `DataStore`, runs with `skipDelete: true` (opting out of Chainsaw auto-cleanup), and v0.2.15 has **no test-level `finally`** — so a scenario is reclaimed only if it reclaims itself, and that is true of whichever Test runs last regardless of order. The etcd comment now says exactly this, with the ordering clause dropped rather than restated. The clickhouse header's ordering sentence is removed outright: the three Tests use distinct releases and share no state, so nothing there needed an ordering claim. This is a comment-correctness change with no runtime surface, so there is no unit test to add — the load-bearing claim was verified empirically against the pinned Chainsaw source, as above. A repo-wide grep confirms no other ordering claim survives in `hack/e2e-chainsaw/` or `docs/agents/e2e-testing.md`. Per `docs/agents/e2e-testing.md`: comment-only, no new retries, no traps, no assertion or teardown-timing changes. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Corrected end-to-end test comments to drop an incorrect test-ordering claim; Chainsaw v0.2.15 guarantees no order between Tests (not file, not document, not name). * Clarified that teardown behavior is independent of test execution order. * Documented the relationship between ClickHouse backup tests and the corresponding backup test suite. * No test behavior or user-facing functionality changed. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
b96c822bde |
docs(agents): give the HR-Ready budget derivation its own section
The checklist entry had grown into a derivation and buried the rule it states. The rule stays in the checklist and the derivation moves to a section of its own. The derivation under-budgeted in two ways. It stated the 10m install timeout as universal, while releases an app chart renders itself take their template's spec.install.timeout, then its spec.timeout, or helm-controller's 5m where neither is set. And it restated which kinds kstatus gates on, which is easy to get wrong, so it now points at the upstream table instead. The harbor comments now name the floor each of their asserts actually sits against, and the opensearch comment stops citing a 5-6m standard the checklist no longer states. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <3811295@gmail.com> |
||
|
Aleksei Sviridkin
|
957ddd7f25 |
fix(tests): raise the mariadb suite HR asserts past the install timeout
At 6m the HelmRelease-ready asserts expired before the release's own 600s install timeout, so a failed install was reported as a bare Chainsaw timeout with no reason. 11m lets the release record why it failed first. The round-trip op grows by exactly what the two walkthrough waits gained. Otherwise a failure at either wait could run past the op, which SIGKILLs the process group and loses the script's error. That bounds one failure, not the whole flow: a run slow at every stage that fails late still reaches the op, and bounding that needs a phase budget rather than a larger literal. The endpoint asserts stay. A freshly applied CR has no conditions and kstatus reads it as current, so the HelmRelease wait can end before any pod is up; an endpoint address closes that window. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <3811295@gmail.com> |
||
|
Aleksei Sviridkin
|
f629280944 |
fix(tests): raise the mongodb, rabbitmq and clickhouse HR waits
Those walkthroughs gave their database HelmReleases 300s, under the 600s install timeout the generated releases carry, so a failed install surfaced as a bare timeout rather than its own reason. None of these charts sets a startup probe, so 660s clears both floors, as it does for mariadb. Each Chainsaw op that runs one of the scripts grows by exactly what the waits it reaches gained, so it keeps the margin it had. The clickhouse op still sits above the sum of every inner ceiling. The rabbitmq one covers a failed install at the source wait, while the ceilings up to its target wait add up to more than the op, as they did before. The mongodb round-trip runs with SKIP_RESTORE, so only its source wait counts there. postgres keeps its 360s: whether a suite pays the extra minutes on every red run is a per-suite call, and this one is not made here. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
1071802e3f |
test(e2e): pin the release-history read against the upstream type (#3607)
## What this PR does
`hack/remediation-guard.bats` carried a test called "status.history
extraction pins HR v2 status.history shape". It wrote a HelmRelease
document containing the field names it expected, then read that same
document back with the same jsonpath through `yq`. What it established
is that `yq` can read a fixture the test itself composed.
Its own comment named the failure it was there to catch: a future Flux
release renames the field, the jsonpath matches nothing, the guard
reports no remediation cycle, and real cycles pass the e2e assertion. A
rename moves neither the fixture nor the expression, so the test stays
green through exactly that event. The comment also named where the truth
lives, `github.com/fluxcd/helm-controller/api/v2`, and then pinned a
copy of the author's expectations instead.
`internal/fluxcontract` supplies neither half. The expression is read
out of `hack/e2e-chainsaw/_lib/remediation-guard.sh` at test time, so it
is the one that ships rather than a copy. The object is a
`helmv2.HelmRelease` built from the upstream type at the version
`go.mod` holds, serialized and matched through
`k8s.io/client-go/util/jsonpath`, which is the engine kubectl's `-o
jsonpath` uses. A renamed or retyped field is then a compile error or a
failed match, and a guard that stops reading history fails the lookup
with a message saying so.
That is one test. Two more exist so that it reads the expression kubectl
actually receives, because the expression now lives in a named
assignment the test sources and the caller expands: between the value
checked here and the value that runs there is a shell variable, and each
step of that indirection can fail while the suite stays green. The first
follows the caller's own call: it reads the local handed to
`helmrelease_has_remediation_cycle`, requires that local to be assigned
exactly once, and requires that assignment to take the expansion of the
shared name rather than the bare name, which matters because kubectl
echoes a brace-less template back verbatim, so a read that lost its `$`
returns its own text, clears the script's empty-history check, and
matches no status. Starting from the call rather than from the text is
what makes the pin describe the read that runs: asking only whether a
well formed read exists somewhere in the file is satisfied by a second
assignment overwriting the same local, and by a read filling a local the
guard never consumes. The second holds that no shell library but the one
that owns the name assigns it, which does not follow from the first: an
assignment beside the read satisfies the first and still replaces the
value after this file has approved the one the library exported. It
covers every library in that directory rather than the caller alone,
because the caller sources three of them and the owning one first, so an
assignment in any of the others lands later and wins.
Those two match text with a pattern, and each carries both halves of its
contract. A refuse table lists the forms the pattern must reject, and an
accept table lists the forms it must keep accepting, one case per
freedom the code claims: for the call, the `if` prefix, the negation, an
expansion with or without braces; for the read, the spelling of `-o`,
the quoting of the substitution, an expansion with or without braces,
and the local's name, which stays free because the call and the read are
renamed together. Without the accept side those freedoms survive only
until someone tightens the pattern, and nothing reddens when they go.
Two boundaries, named rather than left to be found. The pin holds the
shape of the read, not the vocabulary of the answer: the guard greps for
`failed` and `uninstalled`, and the test supplies those two strings
itself, so a release status renamed upstream leaves the jsonpath
returning strings, the test green because it wrote them, and the shell
grep silently matching nothing. Those values come from Helm's release
status rather than the Flux type, so pinning them reaches past what this
change binds, and there is more than one way to do it. Second, a literal
history read that fills a local the guard never consumes is not caught,
and on its own that is harmless: the guard still runs the shared
expression, and the moment it stops doing so the read filling the
consumed local no longer matches and the caller test fails.
The caller-side half is a heuristic and is meant to be read as one. It
decides whether a piece of text is the read by matching it, and a
matcher that reasons about shell is wrong at some spelling by
construction, which is the same objection the file raises against
reading the library's assignment as text instead of sourcing it.
Sourcing is available for the library and not for the caller, so what is
left is to make the imprecision run one way: the count is of
occurrences, so an extra one reddens and a missed one cannot pass, and a
form outside the accepted set fails with the pattern printed rather than
quietly stopping asserting.
That direction is the whole argument for keeping it. A false red is a
broken build on a legitimate rewrite, and the failure names what it did
not recognise, so it costs a minute. A silent green is a guard that
asserts nothing for as long as nobody looks, which is the state this
branch exists to end. The ways the caller can break without anyone
noticing are worth that trade. A read that lost its `$` hands kubectl a
template with no braces, which comes back as the variable's own name; a
hand-written path that is valid but names the wrong field comes back as
that field's values. Both are non-empty, so the script's own
empty-history backstop stays quiet and the cycle grep matches nothing. A
deleted call is quieter still, since there is then no assertion to fail.
None of the three is visible to the library-side pin, which judges the
value the library hands out and never learns whether it reached kubectl.
What does fail loudly is an expression kubectl cannot use at all, and
that is the one case needing no pin.
The shape the fixed test now has already exists in this tree, in
`hack/seaweedfs-naming-audit.bats`, under "the audit's reconstruction
agrees with the chart helper it mirrors". That one also writes a
heredoc, which is what makes the pair worth comparing: it copies the
real `_naming.tpl` out of the chart, renders it through `helm`, and
checks the audit script's output against the render. The heredoc is a
harness for calling the real helper, not a record of what the author
expected it to say, and the two sides have different authors. A test
that writes both sides proves only that it is self-consistent.
Nothing is lost by the deletion. The removed test's tail fed extracted
statuses into `helmrelease_has_remediation_cycle`, and the seven tests
remaining in the bats file `hack/remediation-guard.bats` still cover
that, including `uninstalled then deployed still returns detected`. The
extraction half is now pinned more strictly than before: the old test
asserted only that the output was non-empty and contained one line, the
new one asserts the exact two bare tokens, so dropping the `{"\n"}`
separator also goes red.
CI runs it. `make test-controllers` is its own step in
`.github/workflows/pull-requests.yaml` and runs `go test ./internal/...
-count=1`, which picks the new package up without registration.
### Screenshots
Not a UI change.
### Downstream repositories
Walked the trigger map against the diff. It adds a Go test package under
`internal/`, deletes one test from `hack/remediation-guard.bats`, adds a
variable to `hack/e2e-chainsaw/_lib/remediation-guard.sh`, points the
kubectl invocation in `hack/e2e-chainsaw/_lib/run-kubernetes.sh` at that
variable, adds a sentence to `docs/agents/e2e-testing.md`, and records
`make test-controllers` in `docs/agents/overview.md`. No package is
added, renamed or removed, and nothing touches a `values.schema.json`, a
version enum, a `values.yaml` default, an `ApplicationDefinition`, a
CRD, a namespace, an annotation or a telemetry metric.
`hack/e2e-prepare-cluster.bats` is untouched, so the node contract that
`talm` and `ansible-cozystack` restate by hand does not move.
The closest call is `ccp`, whose skills anchor on files under `hack/`.
Its trigger is a move, a rename, or a make target that does something
different. This changes the contents of three files, not their names,
and no target behaves differently; `hack/package.mk` and
`hack/common-envs.mk` are untouched.
- [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
|
97cb2781db |
test(e2e): pin the release-history read against the upstream type
The test that claimed to pin the HelmRelease v2 history shape wrote a HelmRelease document containing the field names it expected, then read that same document back with the same jsonpath. It established that yq can read a fixture the test itself composed. Its own comment described the failure it was meant to catch: a future Flux release renames the field, the jsonpath matches nothing, the guard reports no remediation cycle, and real cycles pass the e2e assertion. A rename moves neither the fixture nor the expression, so the test stays green through exactly that event. The comment also named where the truth lives, github.com/fluxcd/helm-controller/api/v2, and pinned a copy of the author's expectations instead. Replace it with a Go test that supplies neither half. The expression is read out of the shell library that ships it, and the object is built from the upstream type at the version go.mod holds, then serialized and matched the way kubectl does. A renamed or retyped field is a compile error or a failed match; a guard that stops reading history fails the lookup with a message saying so. Both directions were confirmed by mutation. Locating the expression by searching the file is not enough on its own. Inline in a long script, locating it means deciding which occurrences are code and which are prose, and every way of getting that wrong fails green: an example quoted near a broken read satisfies the pin while the guard reads nothing. So the expression moves into a named assignment in remediation-guard.sh, which the guard already sources, and the test sources that library and takes the value the shell is left holding. Deciding which text is the assignment is then not the test's problem: an `export`, an indent, a second assignment later in the file all resolve the way the guard will resolve them, because the thing resolving them is the shell. Sourcing borrows the environment along with the shell, so the snippet unsets the name before it sources. `go test` hands the child its own environment, and a shell that had exported this name would answer in place of a library that stopped assigning it: the value comes back non-empty, the empty check stays quiet, and the pin passes over a guard that reads nothing. That is this file's own subject reaching it through the environment rather than through the source. Moving the expression out of the call site puts a step between what is pinned and what runs, so the call site is checked too, and the check starts from the call rather than from the text: the local the guard is handed must be assigned exactly once, by an anchored assignment whose jsonpath is the expansion of the shared name. That a well-formed read exists somewhere in the file is a weaker claim than it reads as, since a second assignment to the same local, or a read filling another one, satisfies it while the guard runs a literal nothing here pins. The expansion and not the name: kubectl echoes a braceless template back verbatim, so a read that dropped the `$` would return its own text, clear the script's empty-history backstop, and carry no status the cycle check matches. The library is checked by sourcing rather than by reading, so an assignment the shell would run is resolved by the shell instead of by a guess about which text the shell would pick. Each rule is pinned by its own test, so a regression names which one it opened rather than only that extraction broke. How kubectl's printer treats an absent path is its own configuration rather than part of this contract, so it is left unasserted: guessing at it would put the expectation and the fixture back in the same hands. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
4475fb4c17 |
test(e2e): assert the System-mode OIDC uid claim mapping
The System lane read the rendered AuthenticationConfiguration for the issuer URL and the audience but not for the mapping that gives the user a UID. A configuration missing that mapping is still valid, so the tenant control plane starts and the suite passes while every OIDC user authenticates with an empty UID. The unit twin mocks the Secret without the mapping, so dropping the grep from the helper reddens it rather than leaving it unpinned. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
4bf95815a8 |
fix(kubernetes): render the aggregation-layer UID header for tenant OIDC (#3539)
## What this PR does Tenant clusters with `spec.oidc.mode: System` or `CustomConfig` authenticate users fine, but a UID does not survive the hop to an aggregated API server. The tenant kube-apiserver publishes `requestheader-uid-headers` into the `extension-apiserver-authentication` ConfigMap only when the flag is set, and the chart never set it. The aggregation layer still sends `X-Remote-Uid`, but the extension server trusts only the header names published in that ConfigMap, so it ignores the one it gets and the call arrives without a UID. The field documentation even told operators to add the flag by hand via `controlPlane.apiServer.extraArgs`. It forwards a UID rather than creating one. ServiceAccount tokens carry one, and for those the aggregation hop starts working. Users authenticated by `spec.oidc.mode: System` still have none, because the generated AuthenticationConfiguration maps `username` and `groups` only; giving them one means mapping a claim, which changes the identity of users who have none today, and is tracked separately. The chart now renders `--requestheader-uid-headers=X-Remote-Uid` on v1.32 and newer, and adds `--feature-gates=RemoteRequestHeaderUID=true` on v1.32, where the gate is still alpha and the apiserver rejects the flag without it. From v1.33 the gate is beta and on by default, so only the header flag is rendered. v1.31 gets nothing: the flag does not exist there, and writing it by hand stops the apiserver from starting. `--emulated-version` in the same passthrough moves that decision, because the gate resolves through the versioned spec at the emulated version. Rendering the gate on every version from v1.32 rather than on v1.32 alone was the alternative, and it would remove the migration step entirely, because an opt-out would then drop two entries instead of one and move the entry count by itself. It loses on what it leaves behind: a redundant gate term on every cluster, which stops the apiserver from starting on the day the gate is retired from the known list. The design also prefers rendering over failing for the same reason the fail branches are narrow, since a cluster stranded on a HelmRelease that no longer converges is a worse outcome than one that never gets the UID fix. The chart reads an operator entry the way the apiserver reads it: header list elements matched whole and untrimmed and folded as `EqualFold` folds them, gate names compared exactly, gate values resolved as `strconv.ParseBool` resolves them, `AllAlpha` and `AllBeta` blankets honoured, an empty value treated as no entry. On v1.32 an entry that says nothing about `RemoteRequestHeaderUID` gets a second one carrying the operator's own terms plus `RemoteRequestHeaderUID=true`. Carrying those terms into the merged copy is what makes it land correctly whether the tenant control plane collapses duplicate flags into the last one or hands both to the apiserver, which merges them in the order it reads them. Tests cover the render matrix over mode, version and operator input: every fail path with its exact message, the ownership and merge paths, duplicate entries in both orders, blanket gates, empty values, and case variants on the header name and on the gate value. The latest-version tenant lane creates its tenant with `spec.oidc.mode: System` and asserts the flag on the rendered KamajiControlPlane, so the tenant apiserver starts with the flag in CI. Nothing asserts the UID arriving at an aggregated apiserver; that needs an extension server inside the tenant and is not in this change. The layer where the count-keyed reset actually bites is the tenant Deployment's arguments, and asserting there needs a live tenant control plane, so it is not in this change either. The rule this follows is that a configuration which renders and boots today keeps rendering. Both flags are new, so an `extraArgs` shape reaching this code was rendering before it existed, and stranding a running tenant on a HelmRelease that no longer converges is a worse outcome than one that never gets the fix. A few shapes do stop rendering, and reaching any of them takes writing `--requestheader-uid-headers` into the passthrough by hand, attempting exactly what the chart now does and getting it wrong; in each the kube-apiserver fails option validation and never starts, so nothing that was working is lost. `docs/oidc-tenant.md` covers these shapes. One known limitation is in there and worth naming here: on v1.33 and newer, adding the documented opt-out is count-neutral in `extraArgs`, and so is removing it again, and on v1.33 and v1.34 adding or removing `--emulated-version=1.31`. The pinned Kamaji build rewrites a running apiserver's arguments only when that count changes and otherwise keeps every flag that has left the list, so either the header flag stays beside a now-disabled gate or the disabled gate stays beside the header flag, and the apiserver refuses to start. Changing the entry count in the same edit is the mitigation, and the fix belongs to the pin rather than to this chart, tracked in #3541. The paths that need no operator edit are clear: turning the feature on for a default tenant moves the count, and a tenant already carrying the opt-out renders byte-identically before and after. Out of scope: `mode: None` still publishes no UID header even though ServiceAccount tokens carry UIDs. That is pre-existing, and widening the condition changes the render for every default tenant. Tracked separately, along with the claim mapping above. Closes #3377 ### Screenshots Not a UI change. ### Downstream repositories - [x] No downstream repository is affected by this change Checked the trigger map against the diff: no package added or removed, no schema field, enum or default changed (description text only), no ApplicationDefinition surface touched, no hack/ layout moved. The managed-apps reference page picks up the new field description through release-time README regeneration. ### Release note ```release-note fix(kubernetes): tenant clusters with `spec.oidc.mode` set to `System` or `CustomConfig` now render `--requestheader-uid-headers=X-Remote-Uid` on the tenant kube-apiserver, plus the `RemoteRequestHeaderUID` feature gate on v1.32, so that an aggregated API server can read the caller's UID out of the request headers. Without that flag the tenant apiserver never publishes `requestheader-uid-headers`, and an aggregated API server ignores the UID header even for callers that have one, ServiceAccount tokens among them. It forwards a UID rather than creating one: users authenticated by `spec.oidc.mode: System` still have none, because the generated AuthenticationConfiguration maps `username` and `groups` only, and that is tracked separately. Adding the flag by hand through `controlPlane.apiServer.extraArgs` is no longer necessary, and an entry of your own is respected: the chart steps aside when yours already carries `X-Remote-Uid` or already enables the gate, merges `RemoteRequestHeaderUID=true` into a copy of a v1.32 `--feature-gates` entry that says nothing about the gate, and renders neither flag when you switch the gate off. The render fails where a `--requestheader-uid-headers` list of your own would stop the kube-apiserver from starting anyway: one that omits `X-Remote-Uid`, one with an empty element, and a list of your own while the gate is off. On v1.33 and newer there is a migration step, and it applies in both directions: the pinned Kamaji build rewrites a running apiserver's arguments only when the number of `extraArgs` entries changes, and otherwise keeps every flag that has left the list. Adding the documented opt-out keeps that number the same, so the header flag already on the Deployment stays beside the now-disabled gate. Removing the opt-out later keeps it the same too, so the disabled gate stays on the Deployment beside the header flag the chart renders again, although your spec no longer shows it. Adding or removing `--emulated-version=1.31` has the same shape both ways on v1.33 and v1.34. The apiserver refuses to start in each case, so change the entry count in the same edit. ``` |
||
|
scooby87
|
7d07bbe3ce |
[kafka] Migrate from ZooKeeper to KRaft mode (#2352)
## Summary - Replace ZooKeeper-based Kafka deployments with KRaft (Kafka Raft) metadata mode - Existing ZooKeeper instances migrate automatically and in place via a Helm `pre-install,pre-upgrade` hook Job that drives Strimzi's ZK-to-KRaft migration state machine - New instances deploy directly in KRaft mode with broker and controller `KafkaNodePool`s - The node pools are owned by the migration hook, not by regular chart manifests (see "Why the pools are hook-managed") - Kafka version is user-configurable (v3.9 default, v3.8 available) - Strimzi operator is unchanged in this PR (stays at 0.45.1-rc1); 0.45.x is the last line that supports ZooKeeper→KRaft migration - ZooKeeper removed from the dashboard UI (ApplicationDefinition) ## Migration strategy Both fresh installs and ZooKeeper→KRaft migrations are driven by a single Helm hook Job (`templates/migration-hook.yaml`, `pre-install,pre-upgrade`) that owns the `KafkaNodePool`s. On every install and upgrade the Job: 1. Ensures both node pools exist (idempotent): a broker pool named exactly `kafka` for a migrating cluster — so Strimzi adopts the existing `<cluster>-kafka-N` brokers and their PVCs in place — or `b-<hash>` for a fresh cluster, plus a controller pool `c-<hash>`. The controller pool pins `strimzi.io/next-node-ids` above the broker range so the broker pool keeps node IDs `[0..N-1]` and Strimzi cannot hand the existing broker IDs to the fresh controllers. The shared `kafka` name is claimed atomically (via `kubectl create`) so only one migration per namespace can win it. 2. Inspects the Kafka CR: no CR → fresh install (pools ensured; the main apply then creates the KRaft Kafka CR, nothing to migrate); already `strimzi.io/kraft=enabled` or `status.kafkaMetadataState: KRaft` → done; any other read error → fail closed. Otherwise (a ZooKeeper cluster) it migrates. 3. Annotates `strimzi.io/node-pools=enabled` then `strimzi.io/kraft=migration`, polls `status.kafkaMetadataState` through `ZooKeeper → KRaftMigration → KRaftDualWriting → KRaftPostMigration → PreKRaft → KRaft`, and only once a safe state is reached flips `strimzi.io/kraft=enabled`. If it times out in an intermediate state it aborts without finalising, so Helm fails the upgrade and the next reconcile retries from where it left off. Data is migrated in place: brokers keep their node IDs and reuse their existing PVCs, nothing is copied. Migration is one-way — Strimzi has no supported KRaft→ZooKeeper rollback once complete, so lowering the `version` field or rolling the app back to a pre-KRaft chart is unsupported (take a backup first). ### Why the pools are hook-managed The hook must create the pools and walk the migration state machine *before* the main release apply. A resource a hook creates cannot also be a regular chart manifest across the ZK→KRaft boundary: Helm's main apply finds the pool live in the cluster but absent from the previous (ZooKeeper) release revision and aborts with `original object KafkaNodePool ... not found`, leaving the release `failed` even though the cluster migrated fine (reproduced under server-side apply, the helm-controller default). Managing the pools only from the hook avoids that, so `templates/kafkanodepools.yaml` and the deployed-version gate ConfigMap (`templates/version.yaml`) are removed and the pools are applied idempotently by the Job on every install and upgrade. ## Bug fixes included - ZooKeeper `metricsConfig` referenced wrong ConfigMap key (`kafka-metrics-config.yml` instead of `zookeeper-metrics-config.yml`) - ZooKeeper `storageClass` used `kafka.storageClass` instead of `zookeeper.storageClass` - WorkloadMonitor used `.Values.replicas` instead of `.Values.kafka.replicas` - `validateNameLength` now caps a Kafka app name at the KRaft controller pod-hostname budget (`<release>-c-<hash>-<id>` must fit 63 chars), instead of admitting names that later fail the chart render forever ## What's next A follow-up PR will upgrade Strimzi to 0.51.0 and Kafka to 4.2 once all instances are confirmed migrated to KRaft. That PR is already prepared on branch `KRaft-upgrade-strimzi-051`. ## Test plan - [x] `helm template` renders correctly for new KRaft instances (no ZooKeeper refs) - [x] `helm template` with custom values (1 replica, external, topics, v3.8) - [x] Version validation rejects unsupported versions via JSON schema - [x] helm-unittest: 86/86 across 9 suites - [x] `hack/kafka-migration-hook.bats`: 12/12 — the real hook script executed against a stub kubectl (resolution branches, fail-closed reads, atomic collision guard, finalize guard) - [x] Operator template renders with the unchanged Strimzi 0.45.1-rc1 - [x] Live migration on a disposable cluster under server-side apply: fresh install → healthy KRaft, release `deployed`; in-place migration of a 20000-message ZooKeeper cluster → release `deployed`, broker PVCs adopted in place (same UIDs), and the KRaft data byte-for-byte identical to the pre-migration ZooKeeper data (matching sha256 over every `partition:offset:key:value`) ### Release note ```release-note [kafka] Enable KRaft metadata mode with automatic, in-place ZooKeeper-to-KRaft migration. Kafka version is now configurable (v3.9, v3.8). The Strimzi operator is unchanged (0.45.1-rc1). ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added chart-level Kafka version selector (default v3.9) and KRaft-aware node pool rendering with controller storage sizing. * **Updates** * Default Kafka appVersion updated to 3.9.0; operator and image tag defaults aligned to 0.45.0. * Templates now omit ZooKeeper resources and metrics when KRaft is in use. * **Tests** * New Helm unit tests for Kafka CR, node pools, metrics, topics, dashboard mappings, and workload monitors. * **Documentation** * Values, schema, and README updated to document new version and controller storage options. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
187e4ce12b |
test(e2e): assert the tenant OIDC UID header on the control plane
The render matrix proves the chart emits --requestheader-uid-headers, but nothing checked that the flag survives the path a real tenant takes: aggregated API admission of spec.oidc, the HelmRelease, and the rendered KamajiControlPlane. Assert it in both OIDC helpers, next to the existing --authentication-config check on the same resource. Read the arg list once into a variable and grep the capture twice. The existing check consumed a pipeline, which is single-use, and a second kubectl call would double the management-cluster reads these helpers already make. Only the header flag is asserted. The chart also renders --feature-gates=RemoteRequestHeaderUID=true, but only on v1.32 where the gate is still alpha, while this lane takes the highest version in the map, which is above that. The header flag is the part that holds wherever the flag exists at all. The bats case for the missing flag runs the helper as a standalone subshell under set -e rather than as an if condition. Shells suppress errexit for a command whose status is being tested, and that reaches inside the subshell, so the if spelling reports success and pins nothing. The binding query next to these assertions piped kubectl straight into sort and so returned sort's status. Its callers capture it in a command substitution, where errexit does not reach, so a failed query read as an empty binding list. It now returns kubectl's failure itself. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Alexey Artamonov
|
0b9332877e |
test(kafka): drop the false-green ZK->KRaft migration chainsaw suite
The kafka-zk-to-kraft-migration suite did not exercise the pre-upgrade hook, it reimplemented a simplified flow beside it: a controller pool named "controller" instead of the hashed c-<hash>, no strimzi.io/next-node-ids pin, and a single broker so the controller-vs-broker node-id collision could never occur. That is exactly the defect the hook exists to prevent, so the suite stayed green on a revision whose in-place migration lost every message — reproduced by hand 2/2 on a live cluster. A suite that cannot fail on this code is worse than no suite: it reads as migration coverage and invites trust it has not earned, while costing ~380s on every Kafka PR and asserting only that a PVC object exists, never reading a record back. The migration hook's bats suite and the live upgrade runs are the real coverage; the Strimzi bump would delete this suite regardless. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
ac92b6b95c |
fix(e2e): cap the bucket verify S3 reads so the step honors its timeout
A Chainsaw `script` op runs its shell under exec.CommandContext with stdout and stderr wired to in-process buffers. When the op timeout fires it SIGKILLs the shell process alone, not its descendants, and cmd.Wait() does not return until every process that inherited the pipe fds closes them. An mc that keeps retrying an S3 call after the shell is gone holds the pipe open, so the op runs to mc's own exhaustion instead of stopping at its ceiling. This is why the bucket suite's verify-every-bucket-takes-a-write step ran 606s under a 300s ceiling. verify-s3-access shares that shape, so wrap each S3 and apiserver read in its own `timeout`: a hung read dies at a known budget and the op timeout stays a backstop. The 30s kubectl cap matches the terminal `k` wrapper the redis suite carries. The two `mc cp` PUTs take 45s, the budget verify-every-bucket-takes-a-write already gives a write, because a working PUT can still wait while the master grows a volume for a freshly created bucket; 15s left too little headroom on that grow and could turn a slow-but-healthy write red on an unrelated PR. The 15s cap on the mc reads has no terminal precedent: the backup-access preflight caps `mc cp` at 15s too, but inside a retry loop where a 124 costs one attempt rather than the step, so it is not one. Land each secret's encoded value in a file before base64 -d so a timed-out kubectl read (exit 124) stops the step under set -e rather than masking as an empty-credential mc alias set further down. The readonly-upload deny probe branched on mc directly (if mc cp ...), which suspends set -e, so a timeout there (exit 124) was indistinguishable from the access-denied non-zero the probe expects: a hung deny path would exit 124, take the false branch, and reach a green cleanup, the exact window this change bounds. Capture the status instead: exit 0 is the readonly regression, 124 is a timed-out probe, both fail; any other non-zero is the expected denial. The suite's own catch read (the `kubectl get pods` in namespace cozy-objectstorage-controller) carried neither an op timeout nor an inner cap, so it inherits timeouts.exec (5m) and can hold the pipe the same way; give it a 30s inner cap and a 1m op timeout so the rule the rest of the file follows holds for it too. The .chainsaw.yaml notes still read the op ceiling as a reliable cut, the old SIGKILL as a de-facto cap and a smaller timeout as guaranteeing a kill; correct them, since the shell-only kill plus the pipe wait are why an inner cap on every read is required rather than optional. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Aleksei Sviridkin
|
92d2eede52 |
test(opensearch): separate the mount-plan rewrite from the roll onto it (#4279)
## What this is
`Refs #4259`. A diagnosis of the opensearch suite timeout plus the one
in-repo change it warrants: making the failing step say which of its two
candidates actually happened. It does **not** claim to fix the root
cause — see "What this does not do".
## What the run actually showed
The `verify-statefulset-rolled-onto-the-new-mount-plan` assert erred
with exactly two field failures, both under `status`: `(currentRevision
== updateRevision): false` and `updatedReplicas: field not found`. No
volume field appears in the error list, and `readyReplicas: 1` shows as
an unchanged line, so the pod was Ready at the deadline and nothing had
replaced it. Per e2e-testing.md ("Reading a failure diff: the actual
side is a projection"), the real cause is the error line, not the diff —
and a Chainsaw diff projects the live object onto the expected fields,
so the three `http-ca` / `http-cert` / `http-key` lines showing in the
diff are the expected tree, not proof the volumes are absent. Read that
way, the volume assertions matched: the three volumes were present on
the live template. The "volumes missing" reading is the projection trap
the doc warns against.
## Which candidate
Candidate (b): the template was rewritten onto the subPath mount plan,
and the operator has not restarted the pod onto it. This is consistent
with the source, traced against opensearch-k8s-operator v2.8.0 as
vendored:
- The chart renders the CR correctly — `generate` dropped, `secret` and
`caSecret` named — proven by
`packages/apps/opensearch/tests/tls_upgrade_test.yaml`.
- Given that CR, `pkg/reconcilers/tls.go` `handleHttp` appends exactly
the three volumes the step asserts (`http-ca` from
`opensearch-test.http-ca`, `http-key` and `http-cert` from
`opensearch-test-http-server`), and `pkg/reconcilers/cluster.go`
`reconcileNodeStatefulSet` writes that template onto the StatefulSet.
Three branches return ahead of that write — no node-pool config hash,
the `opensearch.role` recreate, and parallel recovery — but the
parallel-recovery gate is `ReadyReplicas < replicas-1`, unreachable at
one replica, so the write happens. The reconcilers that run ahead of the
cluster reconciler do not stop it either: the TLS reconciler runs first
and returns the admin-certificate path's result, empty in the ordinary
case, and `securityconfig.Reconcile` short-circuits to nil when the
security-config checksum is unchanged, which the move does not change.
So the operator does write the volumes.
- The nodePool StatefulSet is `OnDelete` (`pkg/builders/cluster.go` sets
`UpdateStrategy.Type: OnDelete` alongside `PodManagementPolicy:
OrderedReady`), so the StatefulSet controller never deletes the pod to
apply a template change: `podManagementPolicy` orders creation and
deletion during scaling, `updateStrategy` decides the update. The roll
is the operator's rolling-restart reconciler's to drive
(`pkg/reconcilers/rollingRestart.go`) — it deletes the pod itself. It
needs `status.Initialized`, it needs `ReadyReplicas == replicas` (a
Ready pod is a precondition, not the blocker), then it opens a client to
the OpenSearch API and calls `CheckClusterStatusForRestart`, which holds
the restart while the cluster is red; on yellow the gate now enables
shard allocation, waits for every data node to join, waits for shard
activity to settle, then proceeds (#4317 deleted the old
`.opensearch-observability` carve-out, and this suite builds the
operator with that patch). Any of those gates can sit indefinitely.
`status.currentRevision` never advances by itself under `OnDelete`; the
operator patches it once `updatedReplicas == replicas`, so the second
assert's own condition cannot go true unless that reconciler ran to the
end.
- The pod's startup probe is a TCP check on 9200
(`builders/cluster.go`), so "connection refused" is OpenSearch not
listening, not a probe-scheme mismatch. The plaintext `GET / HTTP/1.1`
from `curl/8.5.0` on the TLS listener is neither the startup/liveness
probe (TCP) nor the readiness probe (`curl -k https://localhost:9200`,
localhost, HTTPS) — it is external traffic to the LoadBalancer address,
a red herring for the stall.
## What this changes
The step now runs two asserts instead of one. The first waits for the
operator to rewrite the template onto the three volumes, on a
reconcile-scale budget (6m) — a failure here is the operator not writing
them (candidate a). The second waits for the roll to complete onto that
template, on the pod-lifetime budget (12m) — a failure here with the
first green is the roll not completing (candidate b): the operator never
restarting the pod, which the rolling-restart reconciler holds while
`CheckClusterStatusForRestart` reports the cluster red — on yellow the
gate enables shard allocation, waits for every data node to join, waits
for shard activity to settle, then proceeds — or a replacement that was
deleted but never came Ready. The second assert carries the three volume
projections and `observedGeneration == metadata.generation` as well, so
the roll is pinned to the template the first assert checked rather than
to whatever revision is current when the status converges. The suite
`catch` also captures the StatefulSet as YAML and as a describe, and the
operator's own log, because `updateRevision`, `currentRevision`,
`generation` and `updatedReplicas` are what separate the two halves —
describe alone omits them — and the restart decision is logged in the
operator, not in the cluster pods. The reconciler logs `Starting rolling
restart of the StatefulSet` (`rollingRestart.go:155`) once its early
gates (`Initialized`, ready-replica, update-pending) pass; it builds its
OpenSearch client just before, at `:138`, and that client pings the
cluster — so a run whose log lacks `Starting rolling restart` either had
an earlier gate hold it silently or had the client ping fail loudly, not
only the former, while that line present followed by a `Couldn't proceed
with rolling restart ... because <reason>` line names a health-stage
hold (the health gate is `CheckClusterStatusForRestart`, and it is not
the last: `PreparePodForDelete` follows it). The operator-log `tail` was
raised so it spans the whole assert window rather than a third of it.
## What this does not do
It does not fix the stall; #4317 does, and is already merged. The run
that captured this (`Run E2E tests`, job 104576486900) shows the
rolling-restart reconciler holding on health: the manager logged
`Starting rolling restart of the StatefulSet opensearch-test-nodes`
(`rollingRestart.go:155`, printed only after the `Initialized` and
ready-replica gates passed) and then, every ten seconds to the deadline,
`Couldn't proceed with rolling restart ... waiting for health to be
green` (`os_data_service.go:180`), which is reached only after
`GetFlatClusterSettings` returned 2xx (`os_client.go:224` makes anything
else an error). So the operator reached the cluster and the cluster
answered; health held the restart. #4259 established why, and the CA
move plays no part: on a single data node the bootstrap node leaving is
audited into a fresh `security-auditlog-*` index with one replica, that
replica has nowhere to go, the cluster stays yellow for good, and the
v2.8.0 restart gate opens only on green (or on a yellow
`.opensearch-observability`) so it never opens. #4317 backports
opensearch-project/opensearch-k8s-operator#1569 to fix that gate; this
PR is only the diagnostic split that names which half of the roll a
failure is. The split content itself is not the failure: on `49f3f059f`
both asserts passed, the second after 1m47s, and this change touches
only the step's comments and the operator-log tail, not the assertions.
With this change in place, a future occurrence names the half, and the
catch carries the roll's own fields and the operator log, so the
crust-gather can be read against a specific claim rather than an
ambiguous one.
### Release note
```release-note
NONE
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Tests**
* Improved OpenSearch StatefulSet failure diagnostics by capturing YAML,
detailed resource descriptions, and the latest operator pod logs.
* Expanded rolling-restart verification to check volume projections,
template updates, rollout readiness, observed and metadata generations,
and revision consistency.
* Separated template validation from rollout completion checks with
dedicated six- and twelve-minute time limits.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
Alexey Artamonov
|
8ef230f0bf |
Merge remote-tracking branch 'origin/main' into kafka-multi-instance
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> # Conflicts: # docs/storage-immutability.md # packages/apps/kafka/tests/topics_test.yaml # packages/system/kafka-rd/cozyrds/kafka.yaml |
||
|
Yan Bondarenko
|
8ec76a3614 |
fix(kafka): make topics[].config optional
Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> |
||
|
Alexey Artamonov
|
36bbc62445 |
test(opensearch): separate the mount-plan rewrite from the roll onto it
The verify-statefulset-rolled step folded two outcomes with two owners into one assert: that the operator rewrote the nodes template onto the subPath mount plan, and that the StatefulSet finished rolling onto it. When the roll stalled, the timeout could not say which half did not happen. The failure surfaced only as the status mismatch, while the volume projections in the diff read as absent even though they were present -- the projection trap e2e-testing.md documents, where the "actual" side of a Chainsaw diff is a projection and the real cause is the error line above it, not the diff. Split into two asserts so each candidate fails on its own line: the template rewrite on a reconcile-scale budget, the roll on the pod-lifetime one. The nodePool StatefulSet is OnDelete (opensearch-k8s-operator v2.8.0), so the roll is the operator's rolling-restart reconciler's to drive, not the StatefulSet controller's -- it deletes the pod itself, and currentRevision advances only once that reconciler patches it. A failure on the first assert is the operator not writing the volumes; a failure on the second with the first green is the roll not completing. The catch captures the StatefulSet as YAML and as a describe and the operator's own log, because updateRevision, currentRevision, generation and updatedReplicas are what separate the two halves -- describe omits them -- and the restart decision is logged in the operator, not in the cluster pods. The roll-gate comment ties each log signature to a v2.8.0 source line: the reconciler builds its OpenSearch client at rollingRestart.go:138, before it logs "Starting rolling restart" at :155, and that client pings the cluster, so the absence of the log line means an early gate held silently or the client ping failed loudly -- both, not just the first. The stall itself is fixed in #4317: a single-replica cluster is left permanently yellow by an index whose replica cannot be assigned. This change only makes the suite name which half of the roll a future failure stalled on. Refs #4259 Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> Assisted-by: LLM |
||
|
Aleksei Sviridkin
|
d3e228fed8 |
test(opensearch): roll a single-node cluster held yellow by a replica
The roll onto the new mount plan timed out on some runs and passed on others with identical content. What differed was cluster health at the moment the listener moved: on one data node an index that asks for a replica stays yellow for good, and the operator restarts a pod only once its health gate opens. The security audit log creates such an index on its first audited event, and whether that had happened yet was down to timing. Create the index in the suite so the roll always has to get through a yellow cluster, and read the health back so the step cannot keep passing while it proves nothing. The operator logs that health held a restart back, never what the health was, and cluster health is not a Kubernetes object. The catch block now reads it from the node, together with the shard table and the allocation explanation. Signed-off-by: Aleksei Sviridkin <f@lex.la> Assisted-by: LLM |
||
|
Alexey Artamonov
|
353f37e4fe |
test(kafka): widen the fresh-create e2e cleanup budget for KRaft
The fresh-create Kafka suite relied on the shared 5m cleanup default, but a KRaft cluster tears down broker and controller node pools, the entity operator, and their PVCs — more work than a single ZooKeeper node. Under a contended CI run (concurrent Postgres/OpenSearch/Redis suites) that teardown overran 5m and failed the suite after every assertion had already passed, so cleanup latency, not the test, was deciding the verdict. Give this suite its own 8m cleanup timeout; the migration suite runs in its own namespace and is unaffected. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
053ef16976 |
feat(kafka): allow multiple KRaft Kafka clusters per namespace
Fresh KRaft clusters no longer need to be alone in a namespace. Their node pools get short, release-hashed names — "b-<hash>" for brokers, "c-<hash>" for controllers — instead of the fixed "kafka"/"controller". Each release hashes differently, so the names are unique within the namespace (KafkaNodePool object names are namespace-unique) and any number of Kafka instances coexist. The hash keeps the pool name a fixed length: Strimzi derives every pod/PVC name as <cluster>-<pool>-<id> and the cluster name is already the release name, so embedding the release name in the pool name too would double it into every pod hostname and blow the 63-character DNS label limit for long release names. A render-time guard fails with a clear message when even the hashed controller node name would still exceed 63 characters. The fixed "kafka" name is still required to adopt an existing ZooKeeper cluster's <cluster>-kafka-N brokers and data in place during migration, so kafka.brokerPoolName resolves to "kafka" for a migrating/migrated cluster and "b-<hash>" for a fresh one. A second ZooKeeper->KRaft migration into a namespace that already holds a "kafka" pool owned by another cluster fails closed, both at render time and in the pre-upgrade hook (whose owner probe uses --ignore-not-found + an explicit exit-code check, so a transient kubectl error aborts rather than silently waiving the collision guard). This replaces the previous blanket "one Kafka per namespace" guard, which over-restricted the fresh case. The pool names are resolved from a live Helm lookup; that is safe because Helm propagates any non-NotFound read error and fails the whole render (pkg/engine/lookup_func.go), so a transient apiserver/RBAC miss retries rather than silently flipping a live cluster's pool name — an empty lookup means genuine absence or a never-applied client-side render. The migration hook and workload monitors follow the same naming. Tests: helm-unittest fixtures (via kubernetesProvider) cover every resolution branch and the fail-closed guards; hack/kafka-migration-hook.bats drives the migration Job's real shell script against a stub kubectl across the fresh / read-error / already-KRaft / owner-collision / happy-path / stuck / retry cases. Verified on a live cluster: in-place ZooKeeper->KRaft migration preserved all data and adopted the original broker PVCs, and two fresh clusters coexist with distinct pools, pods and PVCs. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
1a68157564 |
fix(kafka): harden migration hook and isolate the migration e2e
Addresses the second review round on the ZK->KRaft migration. Migration hook: - Fully qualify the Strimzi resources as kafkas.kafka.strimzi.io / kafkanodepools.kafka.strimzi.io. The bare plural "kafkas" also exists in this chart's apps.cozystack.io group, so a bare name resolves by per-cluster discovery priority and can hit the tenant kind — which the hook Role does not grant, failing the pre-upgrade hook deterministically. - Classify NotFound structurally via `kubectl get --ignore-not-found` instead of substring-matching stderr, so an unrelated "... not found" error can no longer be mistaken for a fresh install and permanently skip a live ZooKeeper cluster. This also drops the jq dependency. - Treat a KRaft-native / already-migrated CR (annotated strimzi.io/kraft=enabled) as done, never driving it backwards into kraft=migration. - Keep only `before-hook-creation` on the Job's hook-delete-policy so a failed attempt's pod logs survive for a post-mortem until the next run. - Route the kubectl image through cozy-lib.image (cluster registry mirror) and pin it by digest (clastix/kubectl:v1.32@sha256:...), matching the other cozystack hooks, instead of an unpinned docker.io tag. Controller quorum: - Fix the controller node-pool replica count at creation instead of deriving it from kafka.replicas on every reconcile. Strimzi 0.45 cannot scale a controller pool, so a day-2 edit of kafka.replicas (e.g. 1 -> 3) must not flip the quorum; the helper now keeps the existing "controller" pool's replica count when it is present. Docs: - Document that migration to KRaft is one-way: neither a version-field downgrade nor a pre-KRaft chart rollback is supported once migrated (Strimzi has no KRaft->ZooKeeper rollback after completion). E2E: - Run the ZK->KRaft chainsaw migration in its own namespace instead of the shared tenant-test. It raw-applies Strimzi resources plus a namespace-wide LimitRange and leaves deleteClaim:false PVCs, which polluted the sibling kafka suite. Raise the seed readiness timeout for CI headroom. Validated against a live cluster: migration adopts the broker PVC in place and reaches KRaft under a 128Mi LimitRange, in isolation. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
f9676961f3 |
fix(kafka): set resources on the migration node pools
The ZK->KRaft pre-upgrade hook created the adopting broker ("kafka") and
"controller" KafkaNodePools without a resources block. Those hook-created pools
govern the Kafka pods for the entire migration window — Helm applies the
resourced steady-state kafkanodepools.yaml only after the pre-upgrade hook
finishes. In a tenant namespace the LimitRange defaults every container to
128Mi, so the migration's controller (and rolled broker) JVMs OOM, never reach
KRaftPostMigration, and the hook times out — wedging the migration and thrashing
it on every reconcile.
Give both hook pools the same resources the steady-state pools get
(cozy-lib.resources.defaultingSanitize from kafka.resourcesPreset/resources).
This also avoids a redundant second broker roll (resource-less -> resourced)
after the hook completes.
Tests: a helm-unittest assertion that both migration pools render a resources
block; the Chainsaw ZK->KRaft suite now seeds under a 128Mi LimitRange (and
gives the seed cluster explicit resources), reproducing the tenant-quota
condition, and asserts the migration still reaches KRaft with the broker PVC
adopted. Validated green against a live cluster under the LimitRange.
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
|
||
|
Alexey Artamonov
|
649f7e0b63 |
fix(kafka): enforce one Kafka per namespace; simplify node pool names
Follow-up to the KRaft-migration review. The broker KafkaNodePool must be named exactly "kafka" so Strimzi adopts an existing ZooKeeper cluster's <cluster>-kafka-N brokers and data in place during migration, but KafkaNodePool object names are unique within a namespace — so two Kafka clusters cannot both own a "kafka" pool there. This is an upstream constraint and Strimzi's own guidance is one Kafka cluster per namespace (strimzi/strimzi-kafka-operator discussions/11120). Adopt that model instead of the earlier fresh-vs-migrated conditional naming: - New kafka.assertSingleInstance helper fails the render (with a clear message and the upstream reference) if the namespace already holds another Kafka cluster. Reconciles/upgrades of the same release are allowed; lookup returns nothing during `helm template`, so unit tests never trip it. - Node pools are now named "kafka" (broker) and "controller" unconditionally, which also adopts existing brokers on migration and yields clean <cluster>-kafka-N / <cluster>-controller-N names. The fresh-vs-migrated brokerPoolName helper is removed. - WorkloadMonitor pool-name selectors and the migration hook updated to match; unit tests updated for the fixed pool names. - README documents the one-Kafka-per-namespace rule and links the Strimzi discussion. The ZK->KRaft Chainsaw migration suite (seed ZooKeeper cluster, migrate, assert the original broker PVC survives and no wrongly-named PVC is created, cluster reaches KRaft) was run green against a live cluster. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
710cdd18bb |
fix(kafka): address KRaft-migration review — broker pool adoption, hook safety, tests
Addresses the branch review on the ZK->KRaft migration path. CRITICAL — broker node pool adoption. Strimzi derives broker/PVC names as <cluster>-<pool>-<id>; a pre-node-pool ZooKeeper cluster has brokers <cluster>-kafka-N, so the adopting broker pool must be named exactly "kafka" or the operator provisions fresh empty brokers and orphans the tenant's data. A new kafka.brokerPoolName helper resolves the broker pool to "kafka" for an existing (migrating) cluster and keeps the release-scoped "<release>-broker" for fresh installs, so multiple Kafka instances still coexist in one namespace. The migration hook creates the pool as "kafka" and fails closed if a "kafka" pool already belongs to another cluster (node pool names are namespace-unique, so in-place migration of co-namespaced ZK clusters is unsupported). Wired through kafkanodepools.yaml, workloadmonitor.yaml and the hook. MAJOR — migration robustness. - Raise the HelmRelease timeout: the migration Job can run tens of minutes but the release timeout defaults to 10m, so helm-controller aborted and thrashed the pre-upgrade hook. Add release.cozystack.io/helm-install-timeout=45m on the kafka ApplicationDefinition, above the Job's activeDeadlineSeconds (2400s). - Idempotent migration: do not re-issue kraft=migration once the cluster is past the ZooKeeper phase (an invalid backward transition Strimzi rejects on retry). - Fail closed on the decision gates: distinguish a genuine NotFound (skip) from any other kubectl error (abort), and never treat an empty kafkaMetadataState on an existing CR as "migrated". MINOR — version downgrade documented as unsupported; integer (not lexical) compare in the version gate; deleteClaim/PVC-retention and the one-per-namespace migration limitation documented in the README; docs/storage-immutability.md updated for KRaft. Tests — helm-unittest coverage for the broker pool name, the hook guards and the idempotent/fail-closed logic; a bats check for the helm-install-timeout budget ordering; and a Chainsaw ZK->KRaft migration suite that seeds a ZooKeeper cluster, drives the migration, and asserts the original broker PVC survives (not recreated) and the cluster reaches KRaft. The migration sequence and the Chainsaw assertions were validated against a live cluster. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Matthieu
|
42db2ca141 |
feat(kafka): migrate from ZooKeeper to KRaft mode with automatic migration
Replace ZooKeeper-based Kafka with KRaft (Kafka Raft) metadata mode. Strimzi 0.45 is the last line that still supports ZooKeeper, so this prepares the transition before ZooKeeper support is removed upstream. Chart changes: - The Kafka CR always renders strimzi.io/kraft=enabled with node pools and no spec.zookeeper block. Storage, replicas and resources move to separate broker + controller KafkaNodePools (a homogeneous separate-pool layout works for both fresh installs and migrations; Strimzi 0.45 does not support migration into a combined pool). - Kafka version is now user-configurable via the standard files/versions.yaml pattern (v3.9 default, v3.8 available); appVersion bumped to 3.9.0. - WorkloadMonitors track the broker and controller pools (no zookeeper monitor); the dashboard RBAC grants the controller monitor and drops the zookeeper one. Migration of existing ZooKeeper clusters is handled by a Helm pre-upgrade Job (templates/migration-hook.yaml) gated by a <release>-kafka-deployed-version ConfigMap, following the established seaweedfs/etcd hook pattern. The Job creates the broker + controller pools with Helm ownership labels, drives the Strimzi ZK->KRaft state machine (node-pools=enabled, kraft=migration -> kraft=enabled once KRaftPostMigration is reached) and is skipped once the ConfigMap records version >= "1". Testing: - Helm unit tests cover the Kafka CR, node pools, migration hook, metrics, topics, version map, dashboard RBAC and workload monitors. - The e2e suite is ported to the current Chainsaw framework (hack/e2e-chainsaw/kafka): it asserts the KRaft topology (both node pools, kafkaMetadataState=KRaft), the absence of ZooKeeper, and bootstrap endpoints. Rebased onto current main: TLS listener support, entity-operator resource guards, x-cozystack-options schema annotations and the WorkloadMonitor replica-path fix are all preserved and reconciled with KRaft. Signed-off-by: Matthieu <matthieu@hidora.com> Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Aleksei Sviridkin
|
c038cf25d3 |
test(seaweedfs): cover the bucket count a volume server can serve
A volume server started with -max 0 derives the slots it has left from the free space on its disk, less the room its unfilled volumes still claim, divided by the master's volume size limit, and each collection claims a batch of those slots on every volume server the replication spans. The number of buckets an instance can hold therefore has a ceiling well below what the free bytes suggest, and crossing it leaves every COSI object reporting ready while the S3 PUT times out. The existing case creates one bucket, which sits inside that ceiling at any sizing, so nothing here told a working ceiling from an exhausted one. This case creates three beside the platform's own backup bucket and requires each to accept an object, which is the only signal the exhaustion produces. Three rather than two because the count has to clear the ceiling whether or not cozy-backups and the unnamed default collection have grown yet, which this suite cannot observe. BucketAccess reconciles separately from the claim it belongs to, and the write step reads the Secret that access produces, so readiness of the release and the claim is not enough to enter it. Without the access assertions the step can fail on a missing Secret, which looks the same in the report as the capacity failure it is here to measure. The catch reads the master's slot accounting as well as its log. A grow failure in the log does not say whether the shipped limit is too coarse for the PVC or an earlier collection in the same run still holds the budget, and the per-server Max against Volumes does. The read runs inside the master, whose image ships curl, so it needs nothing on the host and adds no second fixed port to the reason this suite cannot run in parallel. That reason now covers two cases rather than one, which the shared config says. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Alexey Artamonov
|
150e18b3d3 |
docs(e2e): correct the Chainsaw Test-ordering claim in etcd and clickhouse suites
The etcd suite justified per-scenario teardown with a claim that Chainsaw runs Tests in name order, not file order. That was wrong, but restating it as document order is wrong too: at the pinned v0.2.15 Chainsaw guarantees no order between Tests at all. The runner registers every Test through testing.T.Run and the body calls t.Parallel() whenever spec.concurrent is nil or true — the case in both suites, since neither sets it. Go pauses those subtests and releases them together once the parent returns, and execution.parallel maps only to -test.parallel, a concurrency cap rather than an order. So parallel: 1 serialises the Tests without ordering them: not file, not document, not name. The E2E run on this branch's own head shows it — clickhouse-2 ran before clickhouse-1 with both etcd Tests interleaved between the clickhouse pair. The teardown decision the false claim justified still holds, for a reason that does not touch order: each etcd scenario is self-contained under skipDelete: true with a step-level finally, and v0.2.15 has no test-level finally, so a scenario is reclaimed only if it reclaims itself — true of whichever Test runs last regardless of order. Drop the ordering clause from the etcd teardown rationale and the ordering sentence from the clickhouse header rather than restating either. Comment-only; no behaviour changes. Verified against Chainsaw v0.2.15 source: pkg/runner/runner.go (t.Run plus t.Parallel gated on spec.concurrent) and pkg/runner/flags/flags.go (execution.parallel maps to test.parallel). Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Yan Bondarenko
|
9d2fddd4e6 |
test(kafka): assert release topics exist before the deletion check
The post-delete absence assertions could pass vacuously if the two release topics were never created. Asserting both topics first makes the E2E prove that the pre-delete cleanup actually removed them. Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> |
||
|
Yan Bondarenko
|
44ba13b527 |
fix(kafka): delete release topics before the topic operator is removed
Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> |
||
|
Aleksei Sviridkin
|
070dcb1e30 |
fix(e2e): wait for the redis replica link instead of sampling it once (#4252)
## What this PR does The `verify-tenant-ca-projection` step in the redis suite read `INFO replication` from both pods once and failed the run when a replica reported `role:slave` without `master_link_status:up`. That value is still in motion at that point, so one sampling decided the result. Redis reports the link up only once a replica reaches `REPL_STATE_CONNECTED`. The operator's readiness script admits a replica on two other predicates: `master_host` being off `127.0.0.1`, and `master_sync_in_progress` not being `1`. `master_host` is set the moment `REPLICAOF` is issued, ahead of the link coming up, and `master_sync_in_progress` is `1` only during the transfer phase, so every handshake state between the two is ready and unlinked. A dropped link re-enters the same window. `readyReplicas: 2` therefore establishes nothing about the link, and the comment on the step calling the check deterministic was wrong. The step now polls the topology to a bounded deadline and fails only if it does not settle, reporting each pod's observed state and the elapsed time. Nothing models the link declaratively: the RedisFailover CRD defines no status at all. So the poll carries the `TODO(e2e-replace-fixed-timeouts):` marker the e2e conventions require for a wait with no object behind it. What the step does now is wait for convergence where it used to sample once. No claim about flake rate follows from that. A green run says nothing about a failure that shows up in a minority of runs. Shell embedded in a chainsaw step has no unit-test surface in this repository, so this change is exercised by the suite and nothing else. Two pre-existing diagnostics defects in the same file turned up while reading it, tracked in #4246 and #4250. Neither is touched here. The first one matters a little more after this change: the poll runs its read path about a dozen times per step instead of twice. Closes #4238 ### 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: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: I walked the trigger map against the diff rather than the title. The change edits one file under `hack/e2e-chainsaw/`, moves and renames nothing, and alters no make target, so none of the listed triggers fire. The node-prerequisite trigger that reaches `ansible-cozystack` and `talm` is `hack/e2e-prepare-cluster.bats`, which this does not touch. ### Release note ```release-note fix(e2e): the redis TLS suite waits for the replica link to come up instead of reading it once, so a link that is still establishing no longer fails the run ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Tests** - Improved Redis TLS topology validation by allowing replication status time to stabilize before verification. - Extended the test operation timeout to reduce failures caused by slow environment startup or synchronization. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
4878250a1d |
fix(e2e): wait for the redis replica link instead of sampling it once
The redis-tls topology check read INFO replication from both pods once and failed the suite when a replica reported role:slave without master_link_status:up. That value is legitimately in motion there. Redis reports the link up only in REPL_STATE_CONNECTED, while the operator's readiness script admits a replica on two different predicates -- master_host off 127.0.0.1, and sync not in the transfer phase -- so every handshake state between the two is ready and unlinked, and a dropped link re-enters that window. Ready replicas carry no evidence about the link at all, which is more than the step's own comment allowed for and makes its claim of determinism wrong. Poll the topology to a 60s deadline instead, and on expiry report each pod's observed state along with how long it waited. Nothing models the link declaratively -- the RedisFailover CRD defines no status, so there is no condition to assert on and the poll carries the marker the e2e conventions require. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
65c183e311 |
fix(e2e): keep backup preflight failure evidence across attempts
The backup access preflight printed its port-forward capture only when the port-forward never came up, and truncated its mc log at the top of every attempt. On the path that fails in practice, a healthy port-forward and an S3 round trip that never completes, the output was the tail of the last attempt with nothing to say which stage it died at, which reads as a successful upload followed by a hung download. Print the port-forward capture alongside the mc log on the timeout path, append the mc log across attempts, and record which of PUT, GET, compare or DELETE each attempt reached. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
f9ea0da3de |
feat(backups): Kafka topic-metadata backup via a dedicated strategy driver (#4000)
## What this PR does Adds a dedicated `strategy.backups.cozystack.io/Kafka` Go driver that backs up the **topic metadata** of a Cozystack-managed `Kafka` application and wires it into the platform `cozy-default` flow — the counterpart to the app-specific drivers (CNPG, MariaDB, Altinity, MongoDB). It is a typed variant of the generic Job strategy: the controller renders a one-shot batch Job that talks to the Kafka Admin API, and adds on top an applicationRef Kind gate, a `Ready` precondition on the Strimzi `Kafka` cluster, and a per-run object key via `.BackupName` so distinct runs never overwrite each other. What it captures: every non-internal topic's partition count, replication factor and non-default configs (via `kafka-topics --describe`), stored as one small object per run in the system bucket; restore recreates them with `kafka-topics --create` + `kafka-configs --alter`. Deliberately **out of scope**: message payloads (a volume-snapshot strategy's job), consumer-group offsets (a log position, only meaningful with a byte-identical data restore), ACLs, quotas, and `KafkaUser`s. No PITR. Restored topics are created directly through the Admin API, so on the target they are unmanaged (no `KafkaTopic` CR) — the accepted tradeoff of an Admin-API-only driver. Both restore flows are implemented: **in-place** recreates dropped topics into the source, and **to-copy** applies the source's topics onto a freshly-bootstrapped empty `Kafka` (the "bootstrap a cluster, then apply the metadata state" flow). Contents: - Driver: `api/backups/strategy/v1alpha1/kafka_types.go`, `internal/backupcontroller/kafkastrategy_controller.go` (+ unit tests for its pure helpers), `internal/backupcontroller/kafkatypes/` (Strimzi `Kafka` readiness gate), dispatch cases in both controllers + `supportedBackupStrategyKinds()`, scheme registration, generated CRD + deepcopy. - Chart: `strategy-kafka-default.yaml` (gated on the bucket name), a `cozy-default` binding, read RBAC on `kafka.strimzi.io/kafkas`, and the Strimzi Kafka client image value. - Docs: a Kafka section in `docs/operations/backup-classes.md`. - Example + e2e: `examples/backups/kafka-metadata/` and `hack/e2e-chainsaw/kafka-metadata/` (`kafka-3-metadata-roundtrip`), an honest round-trip that proves a per-run `retention.ms` sentinel survives both restores. The original `packages/apps/kafka` chart is left untouched (it has no legacy backup mechanism). Validated: `go build ./...`, `go vet`, `go test ./internal/backupcontroller/...` (pure helpers plus a reconcile Kind-gate test and the RestoreJob cleanup regression test), `helm unittest` on the package (which now renders `strategy-kafka-default.yaml` and asserts its kind/name/`artifactURITemplate`), the `select-install`/`select-e2e` bats suites, `helm template` of the strategy CR, `bash -n` of the rendered script, and `make generate` (clean). The end-to-end round-trip (`kafka-3-metadata-roundtrip`) is enabled and mapped like the sibling backup suites, so CI builds this branch's controller and runs it live against a fresh sandbox; it has not been merged-and-run before, so that first live run must go green before merge. ### Screenshots N/A — no UI changes. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff. This PR adds a new `strategy.backups.cozystack.io/Kafka` CRD, a Go driver, chart wiring, docs and examples. It does not add or rename a `packages/apps`/`packages/extra` package, does not change any `ApplicationDefinition`, `values.schema.json`, version enum, `release.prefix`, output Secret/Service, the `Package`/`Plan`/`RestoreJob` CRDs the provider types by hand, any `hack/*.mk` anchor or make-target behaviour, node prerequisites, labels/annotations, or telemetry metrics. A brand-new `strategy.backups.cozystack.io` CRD travels to the Terraform provider as opaque JSON (it types only `Plan`/`RestoreJob`/`Package`), so it keeps working untouched. None of the downstream triggers match. - [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(backups): add a Kafka topic-metadata backup strategy bound by cozy-default — backs up topic definitions and configs (partitions, replication factor, non-default configs) via the Kafka Admin API, with in-place and to-copy restore; message data, consumer offsets, ACLs, quotas and KafkaUsers are out of scope ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added Kafka topic-metadata backup and restore support, excluding message data. - Supports ad-hoc and scheduled backups, in-place restoration, and restoration to a separate Kafka cluster. - Recreates missing topics and restores compatible configurations while validating partition and replication-factor constraints. - Waits for Kafka readiness and cleans up backup artifacts after completion. - Backup and restore jobs now enforce configurable deadlines to prevent stalled operations. - **Documentation** - Added guidance covering Kafka metadata backups, restoration limitations, and S3 configuration. - **Tests** - Added end-to-end coverage for backup, restoration, validation, and cleanup scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |