admin/cozystack
1
0 Fork 0
mirror of https://github.com/cozystack/cozystack.git synced 2026-11-09 14:02:56 +09:00
Code Issues Packages Projects Releases Wiki Activity
7,060 Commits 568 Branches 294 Tags
77f6cd1af892004fed62fcdd960f189f920ca86d
Commit Graph
233 Commits
Author SHA1 Message Date
Aleksei Sviridkin 312409ecd2 test(e2e): fail the suite whose pods outlive its cleanup (#4492) 
## What this PR does

A Chainsaw suite could pass while the workloads it created kept running
after its own cleanup, and nothing noticed until a later suite failed.
On one E2E run the brokers of four Kafka releases from suites that had
passed and deleted their applications (#4488), plus a TenantControlPlane
sitting on its finalizer with two apiserver pods (#4489), pushed pod
memory requests to the node limit. Harbor then failed on `Insufficient
memory` two suites later, under the wrong name.

This makes the suite that leaked fail, and name what it left behind.

- Chainsaw v0.2.15 has no hook that runs after a test's cleanup: its
`Configuration` carries `error.catch`, which runs only on failure. So
`make test-chainsaw` now runs `hack/e2e-chainsaw-run.sh`, which invokes
`chainsaw test` once per suite directory and checks the cluster between
invocations with `hack/e2e-chainsaw/_lib/leak-guard.sh`.
- Attribution: a pod is charged to a suite when all three hold. It is
not terminal. Its UID was not in the pod list taken before the suite.
The root of its ownerReferences chain was created at or after the suite
started. A platform pod replaced mid-suite by a rollout, an eviction or
a crash traces back to an object the install created, so it is not
charged. A pre-existing pod adopted by a controller recreated mid-suite
is excluded by the UID baseline. A leak is charged once, to its own
suite.
- The suite start is read off an apiserver, from the `creationTimestamp`
of a server-side dry-run create, rather than off the sandbox. With
several control-plane nodes the reading and a root's timestamp can come
from different apiservers, so the rule assumes their clocks agree more
closely than the gap between the reading and the suite's first object;
no skew margin is added. Source for that behaviour: the generic registry
`Store.create` stamps it through `rest.FillObjectMetaSystemFields`, then
`DryRunnableStorage.Create` copies it back without persisting
(`staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go`
and `dryrun.go` at Kubernetes v1.33.0).
- Budget: charged pods get 300s to disappear, the default
`timeouts.cleanup` in `.chainsaw.yaml`; a unit test keeps the two equal.
A suite that overrides its own cleanup timeout (kafka sets 8m) is
covered the same way, because the check runs only after `chainsaw test`
has returned, which is after every cleanup in the suite has finished
under its own timeout; the 300s start from there. The leaks this targets
sat about 110 minutes (Kafka brokers) and 30+ minutes (the
TenantControlPlane) past their suites. Every green suite logs
`leak-guard: suite=<dir> clear after Ns`, so the number can be checked
against later runs.
- Past the budget the run prints `LEAKED-PODS suite=<dir> budget=300s
count=N`, then one line per pod with its phase, deletion state, root and
`app.kubernetes.io/instance`. It raises a `leaked-pods` error annotation
and adds a failed `leak-guard` test case to `chainsaw-report.xml`. The
run continues through the remaining suites and exits non-zero at the
end, as the single invocation did on a red test.
- A suite that already failed on its own is checked once without the
wait. Its leftovers are reported as a `::warning` with `LEAKED-PODS
suite=<dir> checked-once=chainsaw-failed`, since some may still be
tearing down.
- Each pod gets one `kubectl wait` inside the remaining budget, and
nothing is retried. A timeout or a spent budget gives the verdict from
the last good pod list, also when the re-list after it fails, so a leak
fails the suite. A wait that fails only after running for its whole
`--timeout` counts as a timeout whatever it printed: kubectl maps an
expired context to "timed out waiting for the condition" only inside the
watch, so a deadline that passes during its initial lookup surfaces as
"context deadline exceeded" (`wait.go` and `delete.go` in kubectl
v1.33.0). A spent budget is caught before calling kubectl and never
passed as `--timeout=0`, which means "check once".
- Any other error on a wait or a re-list, before the budget is spent and
before any timeout, stops the waiting and is reported as `LEAKED-PODS
suite=<dir> count=N inconclusive="<error>"`. That is a warning, not a
leak failure, so an apiserver blip cannot become a leak red.
- A failed first reading after a suite, including a pod list that cannot
be parsed, leaves the suite unchecked. It fails with a `leak-guard`
annotation and a report case saying the cluster could not be read, not
that it leaked. Errors that are not leaks carry the annotation title
`leak-guard`, not `leaked-pods`.
- Known leaks are listed in `hack/e2e-chainsaw/_lib/leak-allowlist.txt`
as `<suite-dir> <issue-ref>`: `kubernetes-latest` and
`kubernetes-previous` against #4489 (both suites create a
TenantControlPlane through `_lib/run-kubernetes.sh`). `kafka` and
`kafka-metadata` were listed against #4488 until #4490 fixed it; those
entries are gone. An allowlisted suite prints its leak as a `::warning`
and does not fail. An entry that names a directory that is not a suite
stops the run before the first suite, so a rename cannot keep an
exemption alive. The PR fixing each issue deletes its lines.

Suite selection is unchanged. The runner runs the named directories when
there are any, and otherwise every directory directly under
`hack/e2e-chainsaw/`, each handed to Chainsaw's own discovery; it
refuses a tree with a test at its top level, where that split would stop
being exact. I verified this with Chainsaw v0.2.15's own
`discovery.DiscoverTests`, built from source at that tag. For the full
tree, for `kafka redis harbor` and for `kubernetes-latest`, the union of
the per-suite discoveries equals what a single `chainsaw test [suites]`
discovers: 57, 7 and 2 tests, identical lists. 57 matches the test count
in an earlier full run's report. A unit test pins that the runner passes
chainsaw the suites and flags it was given, in order. The order changes
too: suites used to run in Chainsaw's own order and now run one
directory at a time in alphabetical order, which moves the kubernetes
suites after the kafka ones. Before #4490 fixed #4488 this starved them
(see the E2E criterion below). With the fix merged the order has no
effect, and a future leak that lands before a memory-heavy suite is
named at the suite that leaked. One behaviour does change: a selected
name that is not a directory used to fail the single invocation before
any test ran; now only that suite fails and the others run.

`chainsaw-report.xml` keeps its path and shape: one `<testsuites>` root
with the summed counts, one `<testsuite>` per suite directory. Its
consumers are the `Collect chainsaw report` / `Upload chainsaw report`
steps in `pull-requests.yaml`, `e2e-fork.yaml`, `nightly.yaml` and
`e2e-tag.yaml`, which `docker cp` the file and upload it as an artifact.
Nothing in the repository parses it. The merge is unit-tested on two
per-suite reports in Chainsaw's real shape, one of them failed, and the
result is checked to parse as XML.

Tests: `hack/chainsaw-leak-guard_test.bats` (31 tests, in `make
unit-tests`; named without the `e2e-` prefix, which the unit lane
excludes) covers the attribution rule, the budget, the allowlist, the
merge and the runner, all against stub `kubectl` and `chainsaw`. Every
rule was mutation-checked: removing the guard call, having the Makefile
bypass the runner, inverting the allowlist match, flipping or tightening
the root-timestamp comparison, removing the UID baseline, removing the
terminal-phase filter, skipping the budget wait, changing the budget,
lowering or raising the depth cap, accepting a stale allowlist entry,
reading an unreadable pod list as empty, dropping the leak case from the
report, skipping directories in the selection, treating any failed owner
read as a missing owner, dropping the separate report for a suite that
already failed, dropping the not-run reason from the report, reading any
wait error as a timeout, removing the inconclusive branch, letting a
failed re-list after a timeout turn a leak inconclusive, ignoring a
failed re-list, reporting an unreadable cluster as a leak, reading an
unparsable pod list as clean, reporting a kubectl warning instead of the
error, passing the report name without its extension, wording a failed
re-list as a failed wait, dropping an allowlist's last line when it has
no newline, dropping the check that `--set-string` carries a value, and
reading a wait that ran its whole timeout but printed a context error as
inconclusive. Each one turns its named test red, and restoring turns it
green.

E2E criterion, registered before the run: in the `Run E2E tests` log of
this PR, every suite that ran prints `leak-guard: suite=<dir> clear
after`, a `LEAKED-PODS suite=<dir>` line carrying `allowlisted=`, or one
carrying `checked-once=` (a suite that failed on its own) or
`inconclusive=` (waiting stopped on a failed read), and no line matches
`LEAKED-PODS suite=.* budget=`. With #4490 merged, `kafka` and
`kafka-metadata` are no longer allowlisted and must print `leak-guard:
suite=kafka clear after` and `leak-guard: suite=kafka-metadata clear
after`, which is the live check that #4490 fixed #4488. The first run of
this PR (before #4490, run 36223896793) is the positive control: it
printed `LEAKED-PODS suite=kafka count=15` and `suite=kafka-metadata
count=8`, allowlisted, and every other suite cleared in 0-37s. That run
also went red on both kubernetes suites, because those 23 leaked brokers
left no memory for the second tenant worker VM (`0/3 nodes are
available: 3 Insufficient memory`), which the order change described
above made possible. The guard has not yet run against a real cluster,
so the first run of this PR should carry the `full-e2e` label; its
`clear after` lines are the first measured teardown times, and any
unexpected `LEAKED-PODS` line is either a real leak or a false positive
to understand before merge.

Follow-ups, known and out of scope here:

- The allowlist exempts a whole suite, so a new leak in an allowlisted
suite stays hidden until the issue's fix removes the line.
- Attribution is per suite directory, not per Chainsaw `Test`; the leak
lines carry the release instance, which usually names the test.
- Only pods are checked. A leaked PVC, LoadBalancer IP or bucket holds
no node memory and is not seen.
- The 300s budget rests on Chainsaw's cleanup timeout, not on a measured
pod-gone latency; the `clear after` lines are where that measurement
will come from.
- A platform object recreated mid-suite (for example a HelmRelease
uninstall/reinstall remediation, or a force-replace on an immutable
field) has a root newer than the suite start, and its pods would be
charged to whichever suite was running as a leak; the first CI runs are
where to look for this.
- The per-pod wait recomputes the remaining budget before each pod, and
no unit test pins that.
- `kubernetes-previous` is allowlisted for #4489 although the stuck
finalizer was seen only in `kubernetes-latest`; both suites create a
TenantControlPlane the same way, and the exemption hides any new leak
there until #4489 is fixed.
- Each clean suite now waits for its own pods to finish terminating
before the next starts, where the tail used to overlap the next suite.
The first full run's `clear after` lines say what that costs against the
E2E job's 215-minute cap.
- After a timeout followed by a failed re-list, the leak lines come from
the list taken before the wait, so a pod whose own wait had already
succeeded is still named and the count can be too high; the verdict is
unaffected.
- Likewise, when every wait succeeded and the re-list then fails, the
inconclusive report still lists the pods whose deletion was just
confirmed. In the boundary case where the budget runs out during the
last successful wait and the re-list then fails, the result is
inconclusive rather than a verdict from the last good list, because the
guard only treats the budget as spent when it finds it exhausted at a
check.
- A mistyped `COZY_LEAK_ALLOWLIST` path passes allowlist validation; it
fails closed, since no suite is then exempt.
- An allowlisted suite is not waited on, so `kubernetes-latest` and
`kubernetes-previous` will print `LEAKED-PODS ... allowlisted=` on most
runs even for pods still in normal teardown; waiting out the budget
first and applying the allowlist only to what remains would stop that
noise.
- A slow teardown in each of several suites can add up to 300s apiece,
against the E2E job's 215-minute cap. On the first full run of this PR
(run 36223896793) the `clear after` values of the suites that were
waited on summed to 210s; the last full run before this change took 158
of the 215 minutes.
- A pod whose owner's resource type is no longer served gets "the server
doesn't have a resource type" rather than NotFound, so its suite is
reported unchecked instead of leaked; it still fails.
- `kubectl wait --for=delete` waits for deletion, while the guard skips
pods in `Succeeded` or `Failed`; a pod that finishes but is never
deleted (a Job under a leaked root) spends the rest of the budget before
the suite shows `clear`. The verdict is right; only job time is lost.
- The unit tests do not pin the one-line empty report Chainsaw writes
for a directory with no tests (`_lib`), nor the branch that catches a
spent budget before calling `kubectl wait`; both are verified against
the Chainsaw and kubectl sources rather than by a test.
- The runner's per-suite report directory from `mktemp -d` is not
removed; the sandbox is discarded after the job.
- Each non-suite directory under `hack/e2e-chainsaw/` (today `_lib`)
costs one empty Chainsaw invocation, because the runner leaves the
decision of what is a test to Chainsaw.

### 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:

The diff adds files under `hack/`, changes the `test-chainsaw` target,
and rewords one comment and the failure line in the four E2E workflows.
The trigger map's `hack/` entry is for cozystack/ccp, whose skills
anchor on `hack/package.mk`, `hack/common-envs.mk` and `make generate`,
none of which change here. A GitHub code search for `chainsaw` across
the cozystack organisation finds hits only in this repository.

### Release note

```release-note
test(e2e): the E2E app suite now fails the suite whose pods outlive its own cleanup by more than 300s, naming each leftover pod and its owner; suites with a filed leak are listed in hack/e2e-chainsaw/_lib/leak-allowlist.txt and only warn.
```
 2026-10-01 14:03:13 +03:00
Aleksei Sviridkin 42b7239198 test(e2e): wait for the opensearch handoff before the first API call (#4462) 
<!-- 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

The opensearch suite sends its first OpenSearch API call, the index
create in `make-the-cluster-permanently-yellow`, as soon as the nodes
StatefulSet reports one ready replica. Two operator actions are released
at that moment and neither has finished: the bootstrap pod handing over
the cluster-manager role, and a rolling restart of the only data node.
#4412 is the first of them hitting the create with a 503. This PR makes
the step wait for both before its first call: an `error:` assert that
`opensearch-test-bootstrap-0` no longer exists, then an assert that the
nodes StatefulSet has settled on its update revision, in the same form
the suite already uses after the listener move.

Anchors below are opensearch-k8s-operator v2.8.0 with the in-tree
patches applied, paths under `opensearch-operator/`.

**Bootstrap handoff.**

- The operator starts retiring the bootstrap pod only once the cluster
is `Initialized` (`pkg/reconcilers/cluster.go:110-111`), and that flag
is set as soon as every cluster-manager pool has all its replicas ready
(`controllers/opensearchController.go:257`,
`pkg/builders/cluster.go:1146-1164`). A Ready data node therefore starts
the handoff and does not finish it.
- The voting exclusion is a cluster-manager action, so it goes through
the bootstrap pod and fails while that pod's security is not
initialized. The operator then keeps the pod and requeues after 10s
(`pkg/reconcilers/cluster.go:36`, `:186-190`).
- The pod is deleted only after the bootstrap node has left the last
committed voting configuration (`pkg/reconcilers/cluster.go:195-205`,
`opensearch-gateway/services/os_client.go:760-771`). Once the pod object
is gone, including its Terminating phase, the bootstrap process has
exited and holds no vote, and the only cluster-manager-eligible node
left is `opensearch-test-nodes-0`.
- The assert cannot pass vacuously. The cluster forms around the
bootstrap pod (`cluster.initial_master_nodes` names only that pod,
`pkg/builders/cluster.go:807`), and the pod is created unconditionally
while the cluster is not initialized
(`pkg/reconcilers/cluster.go:112-113`).

**Rolling restart on first boot.** The failing run's own operator log
and StatefulSet dump show it:

- `10:27:52.850` the reconcile in which `Initialized` became true.
- `10:27:53.251` "Failed to exclude the bootstrap node from the voting
configuration".
- `10:27:53.981` "Starting rolling restart of the StatefulSet
opensearch-test-nodes" (`pkg/reconcilers/rollingRestart.go:155`).
- `10:27:54.079` "Couldn't proceed with rolling restart … because
enabled shard allocation" (`:182`), the first step of the yellow path,
after which the next 10s requeue moves on to `PreparePodForDelete`
(`:195`) and deletes the pod.

The StatefulSet in the same dump is at generation 2 with
`currentRevision` `…744cc7b944` and `updateRevision` `…677c65d7fb`: the
pod template was rewritten after `opensearch-test-nodes-0` was created,
and the restart onto it waits only on `Initialized`
(`rollingRestart.go:79`, `:123`). With one replica that restart takes
down the node the step's requests go to, so a create that got past the
handoff could still land on a terminating or restarting node. Once the
StatefulSet reports `currentRevision == updateRevision` with its one
replica updated and ready, the node the step talks to is on its final
template and the operator has nothing pending.

The suite runs one topology: one replica in a single pool with the
cluster_manager, data and ingest roles, on a PVC. The bootstrap pod
comes back only through the emptyDir recovery path
(`pkg/reconcilers/cluster.go:452-478`), which a PVC-backed cluster never
takes, and a restart that was not pending leaves the second assert true
immediately.

I checked the rest of the suite for the same window. Apart from this
step, only the catch script (diagnostics, `|| true`) calls the
OpenSearch HTTP API. `verify-endpoint-serves-the-chart-chain` makes a
TLS handshake only, and it runs after the suite has already waited for
the listener roll to complete, on a single node that is its own
cluster-manager.

Budgets: 3m for the handoff, which took about two seconds in the failing
run; that leaves room for several 10s retire retries, a 30s requeue and
the default 30s termination grace period the bootstrap pod runs with. 6m
for the restart, shorter than the 12m the suite gives the listener roll
because nothing is pulled here: the health steps requeue every 10s and
the pod then restarts on an image the node already holds, where the
first boot, bootstrap included, took under two and a half minutes.

Residual risk, unproven: if the pod template were rewritten again after
the rollout assert passed, that assert would have passed on a template
that is not the final one. I found no mechanism for it. The StatefulSet
builder reads no cluster status (the only status read in
`pkg/builders/cluster.go` is the replica check in `AllMastersReady`), so
the transition to `Initialized` cannot rewrite the template by itself; a
rewrite would need a configuration or secret input to change after that
point. Which input changed on first boot in the failing run I could not
tell from the log, because the ControllerRevisions were not captured.

What I ran: `chainsaw lint test` with Chainsaw v0.2.15, the version the
e2e sandbox pins, passes on the changed suite. As a positive control,
lint rejects each new operation when I rename one of its keys.
`hack/select-e2e.sh` on this diff selects `opensearch` only.

What I did not run: the E2E suite itself. The race is timing-dependent
(one hit in six failed in-tree runs checked, per the issue), so a single
green run would not prove the fix either. The argument rests on the
operator code and the failing run's log cited above.

Closes #4412

### Screenshots

<!-- REQUIRED for UI changes: attach screenshots or screen recordings
demonstrating
the visual impact of your changes. PRs with UI changes without
screenshots will not be merged. -->

Not a UI change.

### 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:

The only file changed is
`hack/e2e-chainsaw/opensearch/chainsaw-test.yaml`, which no downstream
repository consumes.

### 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
NONE
```


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Bug Fixes**
* Cluster health checks now wait for bootstrap completion and verify
that all nodes are ready and up to date before proceeding.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
 2026-10-01 14:03:01 +03:00
Aleksei Sviridkin 5b4380dd3b fix(e2e): delete the failed S3 preflight object through a live tunnel 
The exit trap stopped the port-forward before running mc rm, so the
delete went to a closed port and failed every time, leaving the object
behind and printing a warning that said nothing about why. The delete
now runs first and the warning carries mc's own error, or says the
delete timed out when mc was killed before it could write one.

A PUT that timed out is now also treated as uploaded: it can land and
still outlive its time bound, and the trap never tried to remove such an
object. A PUT the server refused is not, so the common case of access
that never went live does not end in a delete doomed to the same refusal.

Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <f@lex.la>
 2026-10-01 02:10:21 +03:00
Aleksei Sviridkin b6cd6ab682 fix(e2e): print the port-forward log when the S3 preflight client fails 
mc alias set reaches the endpoint through the port-forward, so a tunnel
that came up and then broke is a likely cause of that failure, and its
log is the only record of it. The readiness and retry-timeout exits
already print it; this one threw it away.

Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <f@lex.la>
 2026-10-01 02:10:12 +03:00
Marian Koreniuk 7500a9fbae test(kubernetes): add the Proxmox e2e suite, parked disabled 
Upstream CI has no hypervisor, so this suite cannot run there; it ships
`.disabled` and is run by a lane that owns a Proxmox stand.

Named for what the chart actually creates. The resource definition prefixes the
release, so a Kubernetes CR called k8s-e2e produces
ProxmoxCluster/kubernetes-k8s-e2e in the tenant's own namespace, and the admin
kubeconfig Secret is kubernetes-k8s-e2e-admin-kubeconfig.

Stand-specific values are variables rather than hardcoded. Every one of them is
a bare ${COZY_PVE_*} that run-proxmox.sh expands with envsubst, supplying the
defaults itself — GNU envsubst leaves a ${VAR:-default} in the text exactly as
it is, set or not, and that reaches the apiserver as a string where the schema
wants an integer — and refusing a fixture with a variable still in it. The
StorageClass and the PVC probe read the same COZY_PROXMOX_STORAGE, so the volume
lands where the probe looks. The runner reads the tenant kubeconfig's .conf key,
the address a runner outside the management cluster can reach.
The stand's resource pool comes in through COZY_PVE_POOL, empty by default.
hack/proxmox-e2e-fixture_test.bats pins the shape of the fixture and the render.

The PVC probe reads the storage again after the claim is deleted: a deleted
claim whose disk is still there is the leak the pre-delete hook's Step 1b
exists to prevent. keep_tenant is honoured by the suite's own teardown, not
only by the workflow's. The workflow carries no fallback for the stand's
variables: an unset one fails the job before a key is sent anywhere. The
schedule runs only where vars.PROXMOX_NIGHTLY is true: the job targets a
self-hosted runner with the `proxmox` label, and a scheduled run on a repository
without one would sit in the queue until GitHub cancels it.

The suite also asserts the half of the deployment that lives in the management
cluster — the CSI controller and the cloud-controller-manager — because that is
the property the pair is designed around and a tenant-only assertion would pass
with the token in the wrong place.

Signed-off-by: Marian Koreniuk <moriarti@cp.if.ua>
Assisted-by: LLM
 2026-09-29 21:04:06 +02:00
Mattia Eleuteri bb3ddee233 fix(rabbitmq): bound the topology operator Secret cache 
messaging-topology-operator 1.14.2 caches every Secret in the cluster: its
ClusterRole grants secrets list/watch cluster-wide with no namespace or label
restriction, so the controller-runtime cache holds the whole set. Against the
vendored 500Mi limit this OOMKills on any cluster that has grown enough
Secrets, and it never recovers, because the initial LIST alone no longer fits.
While it is down no topology CR reconciles, for any tenant.

Observed on a cluster with 8211 Secrets across 291 namespaces (293 MB, 87% of
it Helm release history): a flat 333Mi working set for days, then the ceiling,
then a permanent crashloop. The operator was caching 8211 Secrets to read 2.

Upstream fixed this by gating the Secret informer on a label, released in
v1.20.0, and by bypassing the cache for Service reads, released in v1.20.1.
Refresh the vendored manifest to v1.20.2, the current patch release carrying
both, through the package's existing make update pipeline.

The bump alone would break every install: the app chart's credentials Secret is
referenced by User.spec.importCredentialsSecret, and from 1.20 the admission
webhook rejects a user-provided Secret that does not carry
rabbitmq.com/topology-operator=true. Label it in the same change, pin the
label in the chart's unit tests, and give the e2e RabbitMQ a user so the
webhook and the User reconcile are exercised against the real operator.

The RabbitmqCluster default-user Secret needs no label, it is read uncached via
apiReader, and the Status.Binding contract is unchanged since 1.14.2, so the
vendored cluster-operator 2.9.0 stays compatible and does not move here.

Refs #4128

Signed-off-by: Mattia Eleuteri <mattia@hidora.io>
Assisted-by: LLM
 2026-09-29 17:08:17 +02:00
Aleksei Sviridkin b6a30bd504 docs(e2e): describe the per-suite runner and the pod-leak guard 
The agent guide, the suite README and a workflow comment said the E2E
step is a single chainsaw invocation; it is now one per suite, and a
suite whose pods outlive its cleanup fails. Say how a pod is charged
to a suite, what the budget is, and how the allowlist is kept honest.
The workflows' failure line now points at LEAKED-PODS as well, since a
run that failed only on a leak has no assertion diff to point at.

Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <f@lex.la>
 2026-09-26 15:19:49 +03:00
Aleksei Sviridkin c5cd21af96 test(e2e): fail the suite whose pods outlive its cleanup 
A Chainsaw suite could pass while its workloads kept running after its
own cleanup: brokers of Kafka releases whose application was deleted,
a TenantControlPlane stuck on its finalizer holding its apiserver pods.
Nothing noticed until a later suite failed on Insufficient memory,
hours on and under the wrong name.

Chainsaw v0.2.15 has no hook after a test's cleanup, so the suite now
runs one `chainsaw test` per suite directory and checks between them.
A pod is charged to the suite when it is not terminal, did not exist
before the suite, and the root of its ownerReferences chain was created
at or after the suite started on the apiserver's clock. That leaves
alone a platform pod a rollout, an eviction or a crash replaces
mid-suite, though not one whose platform object is itself recreated
mid-suite. Charged pods get the default cleanup timeout of the chainsaw
configuration, five minutes, counted from the moment chainsaw returns;
past that the run names the suite, each pod and its root, and fails.
A timeout or a spent budget takes the verdict from the last good pod
list; any other read failure before then stops the wait and is
reported as inconclusive rather than as a leak; a failed first reading
after the suite leaves it unchecked, and it fails.

The runner hands chainsaw the tests a single invocation would discover
and merges the per-suite JUnit reports into the chainsaw-report.xml
the workflows already collect. Suites with a filed leak are exempt by
an allowlist that must name existing suite directories, so a rename
cannot keep an exemption alive.

Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <f@lex.la>
 2026-09-26 15:06:42 +03:00
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>
 2026-09-26 07:35:32 +03:00
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.
```
 2026-09-25 15:49:53 +03:00
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 -->
 2026-09-25 00:45:01 +04:00
Aleksei Sviridkin 4910bc536e test(e2e): wait for the opensearch handoff before the first API call 
The first OpenSearch API call in the suite ran as soon as the data node
reported Ready. At that point the operator's bootstrap pod can still be
the elected cluster-manager, and until its own security configuration
loads it answers a cluster-manager action such as an index create with
503. The operator deletes the pod only after it has left the committed
voting configuration, so once the pod object is gone the cluster-manager
is the Ready data node.

The same Initialized flag that starts the handoff also releases a
rolling restart that can be pending since first boot, and with one
replica that restart takes the only node down. The step therefore also
waits for the StatefulSet to settle on its update revision.

Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <f@lex.la>
 2026-09-24 23:19:46 +03:00
nightcityblade fdcbc5c83c docs(e2e): document jq requirement for bucket suite 
Assisted-by: LLM
Signed-off-by: nightcityblade <nightcityblade@gmail.com>
 2026-09-25 02:35:29 +08:00
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>
 2026-09-24 18:11:07 +03:00
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>
 2026-09-24 15:10:53 +03:00
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)
```
 2026-09-24 13:00:15 +04:00
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.
```
 2026-09-23 18:23:04 +04:00
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
 2026-09-23 18:00:15 +04:00
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
 2026-09-23 17:44:17 +04:00
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
 2026-09-23 17:44:17 +04:00
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
 2026-09-23 17:44:17 +04:00
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
 2026-09-23 17:44:17 +04:00
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
 2026-09-23 17:44:17 +04:00
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
 2026-09-23 17:44:17 +04:00
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
 2026-09-23 17:44:17 +04:00
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
 2026-09-23 17:44:17 +04:00
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 -->
 2026-09-23 17:14:56 +04:00
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 -->
 2026-09-23 16:40:30 +04:00
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>
 2026-09-23 11:56:12 +03:00
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>
 2026-09-23 11:56:11 +03:00
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>
 2026-09-23 11:56:11 +03:00
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
```
 2026-09-23 11:09:48 +04:00
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>
 2026-09-22 23:41:35 +03:00
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>
 2026-09-22 23:04:28 +03:00
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.
```
 2026-09-23 00:03:20 +04:00
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 -->
 2026-09-22 19:19:21 +03:00
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>
 2026-09-22 18:23:48 +03:00
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>
 2026-09-22 16:05:44 +03:00
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>
 2026-09-22 15:16:25 +03:00
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 -->
 2026-09-21 20:44:23 +04:00
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
 2026-09-21 12:05:33 +03:00
Yan Bondarenko 8ec76a3614 fix(kafka): make topics[].config optional 
Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com>
 2026-09-18 17:30:06 +04:00
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
 2026-09-18 14:58:00 +03:00
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
 2026-09-18 12:01:55 +03:00
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>
 2026-09-17 11:09:10 +03:00
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>
 2026-09-17 11:09:10 +03:00
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>
 2026-09-17 11:09:10 +03:00
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>
 2026-09-17 11:09:10 +03:00
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>
 2026-09-17 11:09:10 +03:00
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>
 2026-09-17 11:09:10 +03:00