mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-09 05:25:14 +09:00
eee059666186890c2fb21ddf5552c3bdfd9fa56d
1151
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Aleksei Sviridkin
|
1071802e3f |
test(e2e): pin the release-history read against the upstream type (#3607)
## What this PR does
`hack/remediation-guard.bats` carried a test called "status.history
extraction pins HR v2 status.history shape". It wrote a HelmRelease
document containing the field names it expected, then read that same
document back with the same jsonpath through `yq`. What it established
is that `yq` can read a fixture the test itself composed.
Its own comment named the failure it was there to catch: a future Flux
release renames the field, the jsonpath matches nothing, the guard
reports no remediation cycle, and real cycles pass the e2e assertion. A
rename moves neither the fixture nor the expression, so the test stays
green through exactly that event. The comment also named where the truth
lives, `github.com/fluxcd/helm-controller/api/v2`, and then pinned a
copy of the author's expectations instead.
`internal/fluxcontract` supplies neither half. The expression is read
out of `hack/e2e-chainsaw/_lib/remediation-guard.sh` at test time, so it
is the one that ships rather than a copy. The object is a
`helmv2.HelmRelease` built from the upstream type at the version
`go.mod` holds, serialized and matched through
`k8s.io/client-go/util/jsonpath`, which is the engine kubectl's `-o
jsonpath` uses. A renamed or retyped field is then a compile error or a
failed match, and a guard that stops reading history fails the lookup
with a message saying so.
That is one test. Two more exist so that it reads the expression kubectl
actually receives, because the expression now lives in a named
assignment the test sources and the caller expands: between the value
checked here and the value that runs there is a shell variable, and each
step of that indirection can fail while the suite stays green. The first
follows the caller's own call: it reads the local handed to
`helmrelease_has_remediation_cycle`, requires that local to be assigned
exactly once, and requires that assignment to take the expansion of the
shared name rather than the bare name, which matters because kubectl
echoes a brace-less template back verbatim, so a read that lost its `$`
returns its own text, clears the script's empty-history check, and
matches no status. Starting from the call rather than from the text is
what makes the pin describe the read that runs: asking only whether a
well formed read exists somewhere in the file is satisfied by a second
assignment overwriting the same local, and by a read filling a local the
guard never consumes. The second holds that no shell library but the one
that owns the name assigns it, which does not follow from the first: an
assignment beside the read satisfies the first and still replaces the
value after this file has approved the one the library exported. It
covers every library in that directory rather than the caller alone,
because the caller sources three of them and the owning one first, so an
assignment in any of the others lands later and wins.
Those two match text with a pattern, and each carries both halves of its
contract. A refuse table lists the forms the pattern must reject, and an
accept table lists the forms it must keep accepting, one case per
freedom the code claims: for the call, the `if` prefix, the negation, an
expansion with or without braces; for the read, the spelling of `-o`,
the quoting of the substitution, an expansion with or without braces,
and the local's name, which stays free because the call and the read are
renamed together. Without the accept side those freedoms survive only
until someone tightens the pattern, and nothing reddens when they go.
Two boundaries, named rather than left to be found. The pin holds the
shape of the read, not the vocabulary of the answer: the guard greps for
`failed` and `uninstalled`, and the test supplies those two strings
itself, so a release status renamed upstream leaves the jsonpath
returning strings, the test green because it wrote them, and the shell
grep silently matching nothing. Those values come from Helm's release
status rather than the Flux type, so pinning them reaches past what this
change binds, and there is more than one way to do it. Second, a literal
history read that fills a local the guard never consumes is not caught,
and on its own that is harmless: the guard still runs the shared
expression, and the moment it stops doing so the read filling the
consumed local no longer matches and the caller test fails.
The caller-side half is a heuristic and is meant to be read as one. It
decides whether a piece of text is the read by matching it, and a
matcher that reasons about shell is wrong at some spelling by
construction, which is the same objection the file raises against
reading the library's assignment as text instead of sourcing it.
Sourcing is available for the library and not for the caller, so what is
left is to make the imprecision run one way: the count is of
occurrences, so an extra one reddens and a missed one cannot pass, and a
form outside the accepted set fails with the pattern printed rather than
quietly stopping asserting.
That direction is the whole argument for keeping it. A false red is a
broken build on a legitimate rewrite, and the failure names what it did
not recognise, so it costs a minute. A silent green is a guard that
asserts nothing for as long as nobody looks, which is the state this
branch exists to end. The ways the caller can break without anyone
noticing are worth that trade. A read that lost its `$` hands kubectl a
template with no braces, which comes back as the variable's own name; a
hand-written path that is valid but names the wrong field comes back as
that field's values. Both are non-empty, so the script's own
empty-history backstop stays quiet and the cycle grep matches nothing. A
deleted call is quieter still, since there is then no assertion to fail.
None of the three is visible to the library-side pin, which judges the
value the library hands out and never learns whether it reached kubectl.
What does fail loudly is an expression kubectl cannot use at all, and
that is the one case needing no pin.
The shape the fixed test now has already exists in this tree, in
`hack/seaweedfs-naming-audit.bats`, under "the audit's reconstruction
agrees with the chart helper it mirrors". That one also writes a
heredoc, which is what makes the pair worth comparing: it copies the
real `_naming.tpl` out of the chart, renders it through `helm`, and
checks the audit script's output against the render. The heredoc is a
harness for calling the real helper, not a record of what the author
expected it to say, and the two sides have different authors. A test
that writes both sides proves only that it is self-consistent.
Nothing is lost by the deletion. The removed test's tail fed extracted
statuses into `helmrelease_has_remediation_cycle`, and the seven tests
remaining in the bats file `hack/remediation-guard.bats` still cover
that, including `uninstalled then deployed still returns detected`. The
extraction half is now pinned more strictly than before: the old test
asserted only that the output was non-empty and contained one line, the
new one asserts the exact two bare tokens, so dropping the `{"\n"}`
separator also goes red.
CI runs it. `make test-controllers` is its own step in
`.github/workflows/pull-requests.yaml` and runs `go test ./internal/...
-count=1`, which picks the new package up without registration.
### Screenshots
Not a UI change.
### Downstream repositories
Walked the trigger map against the diff. It adds a Go test package under
`internal/`, deletes one test from `hack/remediation-guard.bats`, adds a
variable to `hack/e2e-chainsaw/_lib/remediation-guard.sh`, points the
kubectl invocation in `hack/e2e-chainsaw/_lib/run-kubernetes.sh` at that
variable, adds a sentence to `docs/agents/e2e-testing.md`, and records
`make test-controllers` in `docs/agents/overview.md`. No package is
added, renamed or removed, and nothing touches a `values.schema.json`, a
version enum, a `values.yaml` default, an `ApplicationDefinition`, a
CRD, a namespace, an annotation or a telemetry metric.
`hack/e2e-prepare-cluster.bats` is untouched, so the node contract that
`talm` and `ansible-cozystack` restate by hand does not move.
The closest call is `ccp`, whose skills anchor on files under `hack/`.
Its trigger is a move, a rename, or a make target that does something
different. This changes the contents of three files, not their names,
and no target behaves differently; `hack/package.mk` and
`hack/common-envs.mk` are untouched.
- [x] No downstream repository is affected by this change
- [ ] [cozystack/website](https://github.com/cozystack/website) -
follow-up:
- [ ]
[cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack)
- follow-up:
- [ ]
[cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack)
- follow-up:
- [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up:
- [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up:
- [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) -
follow-up:
- [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) -
follow-up:
- [ ]
[cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server)
- follow-up:
- [ ]
[cozystack/external-apps-example](https://github.com/cozystack/external-apps-example)
- follow-up:
- [ ] [cozystack/examples](https://github.com/cozystack/examples) -
follow-up:
### Release note
```release-note
NONE
```
|
||
|
Aleksei Sviridkin
|
90a1a947c5 |
feat(kube-ovn): bump kube-ovn to v1.15.26 (#4331)
## What this PR does Bumps kube-ovn from `v1.15.10` to `v1.15.26` (package `cozy-kubeovn` `0.38.0` -> `0.39.0`). The motivation is a cold-start failure mode in the OVN NB/SB raft leader checker. In `v1.15.10`, `checkDBClusterIntegrity` (`pkg/ovn_leader_checker/ovn.go:336`) called `os.Remove` on a healthy local raft DB (`ovn.go:367`) whenever a leader observed fewer members than expected. That count drops below the target transiently during cluster growth, a rolling restart, or recovery after a node address change, so the check could delete a valid DB and leave `ovn-central` unable to elect a leader. `v1.15.26` removes that function entirely, so the DB is no longer deleted. It is a removal of the faulty check, not a grace-period around it. The chart is re-vendored with `make update`, not hand-edited, so the templates match the image: - `charts/kube-ovn` is re-pulled from the `v1.15.26` tag and `patches/mtu.diff` is re-applied; the image build args are regenerated. - This brings the whole v1.15.26 chart: the reworked master-node affinity helpers, the widened kube-ovn CRD schema (`bandwidth.ingress`/`egress` from `integer` to int-or-string, `u2oFeatures` added), and the new `OVN_CENTRAL_MODE` (`cluster`/`single`) support with its `central-svc`, `central-pvc` and `central-hcp-rbac` templates. `OVN_CENTRAL_MODE` defaults to `cluster` (raft on hostPath), so an in-place upgrade keeps the existing topology. - Two vendored patches now included upstream in `v1.15.26` are dropped: `fix-keepvmip-portgroup.diff` (`hasAliveSiblingVMPod`, `pod.go:2437`) and `fix-vmim-scheduling-retry.diff` (scheduling requeue, `kubevirt.go:195`). `fix-vmim-target-pod-retry.diff` is kept: `v1.15.26` still returns nil at `kubevirt.go:215`. The image also picks up `kube-ovn-bfdd-supervisor`, which v1.15.26 adds (`makefiles/build.mk:33`) and upstream's `dist/images/Dockerfile:12` copies into `/kube-ovn`. `pkg/controller/vpc_egress_gateway.go:47` hardcodes that path for the `bfdd` container a `VpcEgressGateway` with `spec.bfd` gets, so a copy that stops at `kube-ovn-controller` ships a container that cannot exec. `hack/check-kubeovn-image-copies.sh` compares what our Dockerfile and upstream's copy into the image, `make update` runs it against the tag it pulls and `hack/check-kubeovn-image-copies.bats` runs it on every CI run, because `make update` otherwise rewrites only the ARG lines and the copy drifts at each bump. The `multus` commit moves the `kube-ovn-cni-plugins-checked-against` marker to `v1.15.26`. `CNI_PLUGINS_VERSION` is `v1.9.1` in kube-ovn's base image at both releases and in multus, so portmap and macvlan stay in lockstep; the marker is the only change. The image ref stays digest-pinned (`repository` + `tag@sha256`), which the promote-retag release contract requires for a cozystack-built image. The digest is carried over from `v1.15.10` as a placeholder and is refreshed to the real `v1.15.26` image by the release-candidate digest-vendoring step; no PR or main job rebuilds it. Verification: `helm unittest .` passes (22 tests), `helm template .` renders clean, the `cni-plugins-staging-contract` and `promote-retag` unit contracts are green, and the image-build `TestVMIMigrationSchedulingRetry` gate passes 6/6 on the patched v1.15.26 tree. > [!NOTE] > `start-db.sh` was substantially reworked upstream between `v1.15.10` and `v1.15.26`. The privileged fork E2E run installs into a sandbox and passes, but the in-tree E2E job is skipped on a fork PR, and a fresh install is not an upgrade. An in-place upgrade on a multi-node control plane (image rebuild plus OVN raft re-convergence) is still untested before merge. ### Screenshots N/A: no UI changes. ### Downstream repositories Walked the trigger map against the diff. The change is confined to `packages/system/kubeovn` (re-vendored chart, image build args, version pins) and a one-line marker in `packages/system/multus`. It does not add, rename or remove a package, does not touch platform values, variants, components, release assets, `ApplicationDefinition` semantics, the Talos version, or any hand-typed CRD schema in a downstream provider. No downstream trigger matches. - [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 feat(kube-ovn): bump kube-ovn to v1.15.26, which removes the OVN leader-checker integrity check that could delete a healthy raft DB and prevent ovn-central from electing a leader after a node address change or rolling restart ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added hosted control-plane support for OVN Central with StatefulSet deployment, persistent storage, service exposure, and dedicated RBAC. - Added split-cluster and data-plane-only configuration with external OVN endpoints. - Added configurable TLS, leader election, service, and storage settings. - Enhanced CRDs with overlay-only routing and flexible bandwidth quantities. - **Bug Fixes** - Improved VM migration retries during scheduling and preserved connectivity during live migration. - Improved upgrade handling and service configuration validation. - **Updates** - Upgraded Kube-OVN components and images to version 1.15.26. - Documented required OVN IPsec directory preparation for read-only filesystems. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
0f21d06520 |
fix(dashboard): raise the console nginx header buffers for large OIDC tokens (#4376)
## What this PR does A user whose OIDC access token carries many group claims cannot use the console: nginx answers `400 request header or cookie too large` before the request reaches the apiserver proxy. The config baked into the console image kept the nginx defaults, 1k for `client_header_buffer_size` and four buffers of 8k for `large_client_header_buffers`, while the token arrives as one Cookie header several kilobytes long. A header field has to fit into a single buffer, so the per-buffer size is what decides this. Both directives are now 16k, at the top level of that config rather than inside the server block: a header field read before the `Host` header has selected the virtual server is measured against the default server's buffers, so the same lines inside a server block cover that server only while it stays the default one. Measured in the pinned nginx base image: a 12 KB cookie gets 400 on the old config and 200 on the new one, a 20 KB one still gets 400. `nginx -t` accepts the result. This hop sees more than the browser sent. The gatekeeper in front of it runs with `--pass-access-token=true` and `--pass-authorization-header=true`, so the request arrives carrying the `kc-access` cookie, an `Authorization: Bearer` header and `X-Forwarded-Access-Token`, three separately bounded header fields where the edge saw one. This is the console image only. The platform ingress-nginx controller also runs on the defaults, which is #2853. Worth reading that trace again before fixing it there: the controller logged `upstream_status 400`, so it forwarded the request and the 400 came back from behind the gatekeeper, which is this layer. The check in `hack/console-nginx-header-buffers.bats` reads the config out of the Containerfile heredoc and fails when either directive is missing, below 16k, or no longer ahead of the first server block. That config lives inside an image build, so no chart template and no helm-unittest case can reach it. `make unit-tests` picks the file up on its own. No image reference changes here. CI stamps the console digest. Closes #3113 ### Screenshots No UI change. This is the nginx config in front of the SPA. ### Downstream repositories The diff is the console image's nginx config plus one new `hack/*.bats` file. `cozystack-ui` is archived and the console lives in this repo, so a console change belongs in this PR. Nothing under `hack/` moved or was renamed, and no make target changed behaviour. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(dashboard): the console no longer answers `400 request header or cookie too large` to users whose OIDC access token carries many group claims ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Increased the console’s NGINX request-header buffer capacity to support larger header fields and request headers. * Improved handling of requests with headers that previously exceeded the default buffer limits. * **Tests** * Added automated checks to verify the console’s NGINX header-buffer settings remain correctly configured. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
97cb2781db |
test(e2e): pin the release-history read against the upstream type
The test that claimed to pin the HelmRelease v2 history shape wrote a HelmRelease document containing the field names it expected, then read that same document back with the same jsonpath. It established that yq can read a fixture the test itself composed. Its own comment described the failure it was meant to catch: a future Flux release renames the field, the jsonpath matches nothing, the guard reports no remediation cycle, and real cycles pass the e2e assertion. A rename moves neither the fixture nor the expression, so the test stays green through exactly that event. The comment also named where the truth lives, github.com/fluxcd/helm-controller/api/v2, and pinned a copy of the author's expectations instead. Replace it with a Go test that supplies neither half. The expression is read out of the shell library that ships it, and the object is built from the upstream type at the version go.mod holds, then serialized and matched the way kubectl does. A renamed or retyped field is a compile error or a failed match; a guard that stops reading history fails the lookup with a message saying so. Both directions were confirmed by mutation. Locating the expression by searching the file is not enough on its own. Inline in a long script, locating it means deciding which occurrences are code and which are prose, and every way of getting that wrong fails green: an example quoted near a broken read satisfies the pin while the guard reads nothing. So the expression moves into a named assignment in remediation-guard.sh, which the guard already sources, and the test sources that library and takes the value the shell is left holding. Deciding which text is the assignment is then not the test's problem: an `export`, an indent, a second assignment later in the file all resolve the way the guard will resolve them, because the thing resolving them is the shell. Sourcing borrows the environment along with the shell, so the snippet unsets the name before it sources. `go test` hands the child its own environment, and a shell that had exported this name would answer in place of a library that stopped assigning it: the value comes back non-empty, the empty check stays quiet, and the pin passes over a guard that reads nothing. That is this file's own subject reaching it through the environment rather than through the source. Moving the expression out of the call site puts a step between what is pinned and what runs, so the call site is checked too, and the check starts from the call rather than from the text: the local the guard is handed must be assigned exactly once, by an anchored assignment whose jsonpath is the expansion of the shared name. That a well-formed read exists somewhere in the file is a weaker claim than it reads as, since a second assignment to the same local, or a read filling another one, satisfies it while the guard runs a literal nothing here pins. The expansion and not the name: kubectl echoes a braceless template back verbatim, so a read that dropped the `$` would return its own text, clear the script's empty-history backstop, and carry no status the cycle check matches. The library is checked by sourcing rather than by reading, so an assignment the shell would run is resolved by the shell instead of by a guess about which text the shell would pick. Each rule is pinned by its own test, so a regression names which one it opened rather than only that extraction broke. How kubectl's printer treats an absent path is its own configuration rather than part of this contract, so it is left unasserted: guessing at it would put the expectation and the fixture back in the same hands. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
4475fb4c17 |
test(e2e): assert the System-mode OIDC uid claim mapping
The System lane read the rendered AuthenticationConfiguration for the issuer URL and the audience but not for the mapping that gives the user a UID. A configuration missing that mapping is still valid, so the tenant control plane starts and the suite passes while every OIDC user authenticates with an empty UID. The unit twin mocks the Secret without the mapping, so dropping the grep from the helper reddens it rather than leaving it unpinned. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
4bf95815a8 |
fix(kubernetes): render the aggregation-layer UID header for tenant OIDC (#3539)
## What this PR does Tenant clusters with `spec.oidc.mode: System` or `CustomConfig` authenticate users fine, but a UID does not survive the hop to an aggregated API server. The tenant kube-apiserver publishes `requestheader-uid-headers` into the `extension-apiserver-authentication` ConfigMap only when the flag is set, and the chart never set it. The aggregation layer still sends `X-Remote-Uid`, but the extension server trusts only the header names published in that ConfigMap, so it ignores the one it gets and the call arrives without a UID. The field documentation even told operators to add the flag by hand via `controlPlane.apiServer.extraArgs`. It forwards a UID rather than creating one. ServiceAccount tokens carry one, and for those the aggregation hop starts working. Users authenticated by `spec.oidc.mode: System` still have none, because the generated AuthenticationConfiguration maps `username` and `groups` only; giving them one means mapping a claim, which changes the identity of users who have none today, and is tracked separately. The chart now renders `--requestheader-uid-headers=X-Remote-Uid` on v1.32 and newer, and adds `--feature-gates=RemoteRequestHeaderUID=true` on v1.32, where the gate is still alpha and the apiserver rejects the flag without it. From v1.33 the gate is beta and on by default, so only the header flag is rendered. v1.31 gets nothing: the flag does not exist there, and writing it by hand stops the apiserver from starting. `--emulated-version` in the same passthrough moves that decision, because the gate resolves through the versioned spec at the emulated version. Rendering the gate on every version from v1.32 rather than on v1.32 alone was the alternative, and it would remove the migration step entirely, because an opt-out would then drop two entries instead of one and move the entry count by itself. It loses on what it leaves behind: a redundant gate term on every cluster, which stops the apiserver from starting on the day the gate is retired from the known list. The design also prefers rendering over failing for the same reason the fail branches are narrow, since a cluster stranded on a HelmRelease that no longer converges is a worse outcome than one that never gets the UID fix. The chart reads an operator entry the way the apiserver reads it: header list elements matched whole and untrimmed and folded as `EqualFold` folds them, gate names compared exactly, gate values resolved as `strconv.ParseBool` resolves them, `AllAlpha` and `AllBeta` blankets honoured, an empty value treated as no entry. On v1.32 an entry that says nothing about `RemoteRequestHeaderUID` gets a second one carrying the operator's own terms plus `RemoteRequestHeaderUID=true`. Carrying those terms into the merged copy is what makes it land correctly whether the tenant control plane collapses duplicate flags into the last one or hands both to the apiserver, which merges them in the order it reads them. Tests cover the render matrix over mode, version and operator input: every fail path with its exact message, the ownership and merge paths, duplicate entries in both orders, blanket gates, empty values, and case variants on the header name and on the gate value. The latest-version tenant lane creates its tenant with `spec.oidc.mode: System` and asserts the flag on the rendered KamajiControlPlane, so the tenant apiserver starts with the flag in CI. Nothing asserts the UID arriving at an aggregated apiserver; that needs an extension server inside the tenant and is not in this change. The layer where the count-keyed reset actually bites is the tenant Deployment's arguments, and asserting there needs a live tenant control plane, so it is not in this change either. The rule this follows is that a configuration which renders and boots today keeps rendering. Both flags are new, so an `extraArgs` shape reaching this code was rendering before it existed, and stranding a running tenant on a HelmRelease that no longer converges is a worse outcome than one that never gets the fix. A few shapes do stop rendering, and reaching any of them takes writing `--requestheader-uid-headers` into the passthrough by hand, attempting exactly what the chart now does and getting it wrong; in each the kube-apiserver fails option validation and never starts, so nothing that was working is lost. `docs/oidc-tenant.md` covers these shapes. One known limitation is in there and worth naming here: on v1.33 and newer, adding the documented opt-out is count-neutral in `extraArgs`, and so is removing it again, and on v1.33 and v1.34 adding or removing `--emulated-version=1.31`. The pinned Kamaji build rewrites a running apiserver's arguments only when that count changes and otherwise keeps every flag that has left the list, so either the header flag stays beside a now-disabled gate or the disabled gate stays beside the header flag, and the apiserver refuses to start. Changing the entry count in the same edit is the mitigation, and the fix belongs to the pin rather than to this chart, tracked in #3541. The paths that need no operator edit are clear: turning the feature on for a default tenant moves the count, and a tenant already carrying the opt-out renders byte-identically before and after. Out of scope: `mode: None` still publishes no UID header even though ServiceAccount tokens carry UIDs. That is pre-existing, and widening the condition changes the render for every default tenant. Tracked separately, along with the claim mapping above. Closes #3377 ### Screenshots Not a UI change. ### Downstream repositories - [x] No downstream repository is affected by this change Checked the trigger map against the diff: no package added or removed, no schema field, enum or default changed (description text only), no ApplicationDefinition surface touched, no hack/ layout moved. The managed-apps reference page picks up the new field description through release-time README regeneration. ### Release note ```release-note fix(kubernetes): tenant clusters with `spec.oidc.mode` set to `System` or `CustomConfig` now render `--requestheader-uid-headers=X-Remote-Uid` on the tenant kube-apiserver, plus the `RemoteRequestHeaderUID` feature gate on v1.32, so that an aggregated API server can read the caller's UID out of the request headers. Without that flag the tenant apiserver never publishes `requestheader-uid-headers`, and an aggregated API server ignores the UID header even for callers that have one, ServiceAccount tokens among them. It forwards a UID rather than creating one: users authenticated by `spec.oidc.mode: System` still have none, because the generated AuthenticationConfiguration maps `username` and `groups` only, and that is tracked separately. Adding the flag by hand through `controlPlane.apiServer.extraArgs` is no longer necessary, and an entry of your own is respected: the chart steps aside when yours already carries `X-Remote-Uid` or already enables the gate, merges `RemoteRequestHeaderUID=true` into a copy of a v1.32 `--feature-gates` entry that says nothing about the gate, and renders neither flag when you switch the gate off. The render fails where a `--requestheader-uid-headers` list of your own would stop the kube-apiserver from starting anyway: one that omits `X-Remote-Uid`, one with an empty element, and a list of your own while the gate is off. On v1.33 and newer there is a migration step, and it applies in both directions: the pinned Kamaji build rewrites a running apiserver's arguments only when the number of `extraArgs` entries changes, and otherwise keeps every flag that has left the list. Adding the documented opt-out keeps that number the same, so the header flag already on the Deployment stays beside the now-disabled gate. Removing the opt-out later keeps it the same too, so the disabled gate stays on the Deployment beside the header flag the chart renders again, although your spec no longer shows it. Adding or removing `--emulated-version=1.31` has the same shape both ways on v1.33 and v1.34. The apiserver refuses to start in each case, so change the entry count in the same edit. ``` |
||
|
Aleksei Sviridkin
|
7d53f7af22 |
fix(dashboard): raise the console nginx header buffers for large OIDC tokens
The nginx config baked into the console image kept the nginx defaults for client_header_buffer_size (1k) and large_client_header_buffers (4 8k). An OIDC access token carrying many group claims arrives as a single Cookie header of several kilobytes, which no longer fits into one buffer, so nginx answered 400 "request header or cookie too large" before the request reached the /apis, /api or /k8s/ upstream. Raise both to 16k, at the top level of that config rather than inside the server block: a header field read before the Host header has selected the virtual server is measured against the default server's buffers, so the same lines inside a server block cover that server only while it stays the default one. Measured against the pinned nginx base image, a 12 KB cookie gets 400 on the old config and 200 on the new one. The ceiling moves rather than disappears: a 20 KB cookie is still rejected. This is the console hop alone. The platform ingress-nginx controller in front of it runs on the same defaults, so where the dashboard is published through an Ingress a token large enough to exceed them is still rejected at the edge; where it is published through the gateway, whose edge has no per-header ceiling of this kind, the console was the hop returning the 400. Raising the controller is a separate change in a separate package. The accompanying check extracts the config from the Containerfile heredoc and fails if either directive is missing, below 16k, or no longer ahead of the first server block: the config lives inside an image build, where no chart template and no helm-unittest case can reach it. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
scooby87
|
7d07bbe3ce |
[kafka] Migrate from ZooKeeper to KRaft mode (#2352)
## Summary - Replace ZooKeeper-based Kafka deployments with KRaft (Kafka Raft) metadata mode - Existing ZooKeeper instances migrate automatically and in place via a Helm `pre-install,pre-upgrade` hook Job that drives Strimzi's ZK-to-KRaft migration state machine - New instances deploy directly in KRaft mode with broker and controller `KafkaNodePool`s - The node pools are owned by the migration hook, not by regular chart manifests (see "Why the pools are hook-managed") - Kafka version is user-configurable (v3.9 default, v3.8 available) - Strimzi operator is unchanged in this PR (stays at 0.45.1-rc1); 0.45.x is the last line that supports ZooKeeper→KRaft migration - ZooKeeper removed from the dashboard UI (ApplicationDefinition) ## Migration strategy Both fresh installs and ZooKeeper→KRaft migrations are driven by a single Helm hook Job (`templates/migration-hook.yaml`, `pre-install,pre-upgrade`) that owns the `KafkaNodePool`s. On every install and upgrade the Job: 1. Ensures both node pools exist (idempotent): a broker pool named exactly `kafka` for a migrating cluster — so Strimzi adopts the existing `<cluster>-kafka-N` brokers and their PVCs in place — or `b-<hash>` for a fresh cluster, plus a controller pool `c-<hash>`. The controller pool pins `strimzi.io/next-node-ids` above the broker range so the broker pool keeps node IDs `[0..N-1]` and Strimzi cannot hand the existing broker IDs to the fresh controllers. The shared `kafka` name is claimed atomically (via `kubectl create`) so only one migration per namespace can win it. 2. Inspects the Kafka CR: no CR → fresh install (pools ensured; the main apply then creates the KRaft Kafka CR, nothing to migrate); already `strimzi.io/kraft=enabled` or `status.kafkaMetadataState: KRaft` → done; any other read error → fail closed. Otherwise (a ZooKeeper cluster) it migrates. 3. Annotates `strimzi.io/node-pools=enabled` then `strimzi.io/kraft=migration`, polls `status.kafkaMetadataState` through `ZooKeeper → KRaftMigration → KRaftDualWriting → KRaftPostMigration → PreKRaft → KRaft`, and only once a safe state is reached flips `strimzi.io/kraft=enabled`. If it times out in an intermediate state it aborts without finalising, so Helm fails the upgrade and the next reconcile retries from where it left off. Data is migrated in place: brokers keep their node IDs and reuse their existing PVCs, nothing is copied. Migration is one-way — Strimzi has no supported KRaft→ZooKeeper rollback once complete, so lowering the `version` field or rolling the app back to a pre-KRaft chart is unsupported (take a backup first). ### Why the pools are hook-managed The hook must create the pools and walk the migration state machine *before* the main release apply. A resource a hook creates cannot also be a regular chart manifest across the ZK→KRaft boundary: Helm's main apply finds the pool live in the cluster but absent from the previous (ZooKeeper) release revision and aborts with `original object KafkaNodePool ... not found`, leaving the release `failed` even though the cluster migrated fine (reproduced under server-side apply, the helm-controller default). Managing the pools only from the hook avoids that, so `templates/kafkanodepools.yaml` and the deployed-version gate ConfigMap (`templates/version.yaml`) are removed and the pools are applied idempotently by the Job on every install and upgrade. ## Bug fixes included - ZooKeeper `metricsConfig` referenced wrong ConfigMap key (`kafka-metrics-config.yml` instead of `zookeeper-metrics-config.yml`) - ZooKeeper `storageClass` used `kafka.storageClass` instead of `zookeeper.storageClass` - WorkloadMonitor used `.Values.replicas` instead of `.Values.kafka.replicas` - `validateNameLength` now caps a Kafka app name at the KRaft controller pod-hostname budget (`<release>-c-<hash>-<id>` must fit 63 chars), instead of admitting names that later fail the chart render forever ## What's next A follow-up PR will upgrade Strimzi to 0.51.0 and Kafka to 4.2 once all instances are confirmed migrated to KRaft. That PR is already prepared on branch `KRaft-upgrade-strimzi-051`. ## Test plan - [x] `helm template` renders correctly for new KRaft instances (no ZooKeeper refs) - [x] `helm template` with custom values (1 replica, external, topics, v3.8) - [x] Version validation rejects unsupported versions via JSON schema - [x] helm-unittest: 86/86 across 9 suites - [x] `hack/kafka-migration-hook.bats`: 12/12 — the real hook script executed against a stub kubectl (resolution branches, fail-closed reads, atomic collision guard, finalize guard) - [x] Operator template renders with the unchanged Strimzi 0.45.1-rc1 - [x] Live migration on a disposable cluster under server-side apply: fresh install → healthy KRaft, release `deployed`; in-place migration of a 20000-message ZooKeeper cluster → release `deployed`, broker PVCs adopted in place (same UIDs), and the KRaft data byte-for-byte identical to the pre-migration ZooKeeper data (matching sha256 over every `partition:offset:key:value`) ### Release note ```release-note [kafka] Enable KRaft metadata mode with automatic, in-place ZooKeeper-to-KRaft migration. Kafka version is now configurable (v3.9, v3.8). The Strimzi operator is unchanged (0.45.1-rc1). ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added chart-level Kafka version selector (default v3.9) and KRaft-aware node pool rendering with controller storage sizing. * **Updates** * Default Kafka appVersion updated to 3.9.0; operator and image tag defaults aligned to 0.45.0. * Templates now omit ZooKeeper resources and metrics when KRaft is in use. * **Tests** * New Helm unit tests for Kafka CR, node pools, metrics, topics, dashboard mappings, and workload monitors. * **Documentation** * Values, schema, and README updated to document new version and controller storage options. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
187e4ce12b |
test(e2e): assert the tenant OIDC UID header on the control plane
The render matrix proves the chart emits --requestheader-uid-headers, but nothing checked that the flag survives the path a real tenant takes: aggregated API admission of spec.oidc, the HelmRelease, and the rendered KamajiControlPlane. Assert it in both OIDC helpers, next to the existing --authentication-config check on the same resource. Read the arg list once into a variable and grep the capture twice. The existing check consumed a pipeline, which is single-use, and a second kubectl call would double the management-cluster reads these helpers already make. Only the header flag is asserted. The chart also renders --feature-gates=RemoteRequestHeaderUID=true, but only on v1.32 where the gate is still alpha, while this lane takes the highest version in the map, which is above that. The header flag is the part that holds wherever the flag exists at all. The bats case for the missing flag runs the helper as a standalone subshell under set -e rather than as an if condition. Shells suppress errexit for a command whose status is being tested, and that reaches inside the subshell, so the if spelling reports success and pins nothing. The binding query next to these assertions piped kubectl straight into sort and so returned sort's status. Its callers capture it in a command substitution, where errexit does not reach, so a failed query read as an empty binding list. It now returns kubectl's failure itself. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
9efe78ce44 |
test(e2e): hold the tenant-cluster install wait to an ordering rule (#3802)
A test that creates a tenant `Kubernetes` cluster must not end while that release is still installing. The kind drops the install wait on its main resources, but the chart's blocking post-install hook still holds the install open, so a test that asserts and ends hands the rest of it to teardown. Teardown then reds on a step that tests nothing. Reading the cluster mid-install is fine. Ending mid-install is not. `hack/e2e-chainsaw/_lib/run-kubernetes.sh` already waits for the parent release before its tenant-cluster assertions, and that wait is older than this change. Nothing held it there. This adds the guard that does. The guard reads the script, not Chainsaw fixtures: the cluster comes from a heredoc, so a guard walking `apply.file` would find nothing to check and pass. It requires the CR, then a readiness wait on the parent release, then the assertions the test ends on. The wait has to block, and it has to be unconditional in the body that holds it. One more case holds this script to being the only creator of that kind, since a second one would be governed by no rule here. The ordering is read within one function body, so splitting `run_kubernetes_test` reds the unit lane until the rule is pointed at the new shape. Timeout length stays with review. The floor depends on where the wait sits, and the script text does not carry that. Which spellings each matcher takes, which rewrites it refuses instead of judging, and the blind spots it names are written in the guard's header next to the code, each with its mechanism and whether it fails loudly or quietly. Fixtures mark their own lines, so a probe resolves by name instead of by a line number that drifts. No runtime code here: the guard, its fixtures, and the conventions that state the rule. ```release-note NONE ``` |
||
|
IvanHunters
|
a4547a6fe0 |
test(kube-ovn): compare image copies against upstream's Dockerfile
packages/system/kubeovn/images/kubeovn/Dockerfile is a hand-kept copy of upstream's dist/images/Dockerfile, and `make update` rewrites only its ARG VERSION and ARG TAG. A binary upstream starts shipping therefore does not reach our image on its own, and nothing read the two files together: kube-ovn-bfdd-supervisor went missing that way with the whole suite green. hack/check-kubeovn-image-copies.sh normalizes the COPY instructions of both files and reports either side's surplus. It compares only the stages built FROM kubeovn/kube-ovn-base, because our builder stage clones and patches kube-ovn and its COPY lines have no upstream counterpart, and it treats an empty result as a broken check rather than agreement. `make update` now runs it against the tag it pulls, so the next bump stops on a mismatch, and hack/check-kubeovn-image-copies.bats runs it on every CI run against a verbatim copy of upstream's Dockerfile at the vendored appVersion. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
myasnikovdaniil
|
41ef4a6437 |
fix(ci): teach the overlay to repair an unstamped images/*.tag ref (#4362)
## What `hack/overlay-main-images.sh` walks `images/*.tag`, but every branch of its line classifier needs either a YAML key or an `@sha256:`. A `.tag` file holds one bare reference and nothing else, so until a build stamps it the line has neither, the drift guard reads it as a non-ref change, and the committed reference survives. For a package onboarded with the `:v0.0.0` placeholder this repo already uses for a new image, that surviving reference is one no registry serves. The overlay is the only thing that would replace it for a PR that does not rebuild the package, so once such a package is on main every full-suite run that skips its build keeps the placeholder. The same placeholder inside a `values.yaml` repairs itself, because there it sits under an `image:` key the classifier recognises. The difference is the file shape, not the placeholder. ## Reproduction A fixture holding a committed bare tag against an artifact copy carrying a stamped one: ``` $ cd fixture/fakerepo && sh ../../hack/overlay-main-images.sh ../mainpkgs '[]' '' overlay: packages/system/x/values.yaml -> current-main drift (non-ref change) in packages/apps/x/images/x.tag -> keeping committed ref ``` Both files differ only in their reference. The `values.yaml` one is repaired; the `.tag` one is not. Adding any digest to the same placeholder makes it pass, which is what narrows this to the classifier rather than the file. ## Why it has not bitten yet 17 of the 19 `.tag` files in the tree carry `@sha256:` and pass on that branch alone. The only other digest-less one is `packages/extra/seaweedfs/images/seaweedfs-cosi-driver.tag`, and it survives only because it is third-party: `build-main` never rebuilds it, the artifact copy is byte-identical, and the classifier is never reached. The first digest-less first-party `.tag` is what turns the gap live, and one is coming in #3426. ## The fix `hack/promote-rewrite-tags.sh` already carries a second match branch for exactly this shape, with a comment saying why ("the second branch is what catches a bare `.tag` file, whose content has no YAML key at all"). This gives the overlay the same one, scoped to `*.tag` so an unkeyed line in a `values.yaml` still counts as drift, because only in a `.tag` file is a whole line known to be a reference and nothing else. Two tests in `hack/overlay-main-images_test.bats`: the unstamped placeholder is overlaid, and a `.tag` whose non-reference line differs still counts as drift, so the rule cannot quietly widen into "every line in a `.tag` file is a reference". Removing the fix fails the first one. ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Fixed image overlay handling for `.tag` files containing bare image references, allowing current references to be applied correctly. - Preserved drift detection when `.tag` files also contain unrelated, non-reference changes. - **Documentation** - Clarified how image-reference tooling handles bare references and package-tree processing. - **Tests** - Added coverage for bare-reference overlays and mixed reference/comment changes. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
c0ff783026 |
fix(hack): make bats unit tests pass on a BSD/macOS checkout (#4284)
## What this PR does Refs #4272 (Defects 1 and 2). On a clean checkout of `main`, `make unit-tests` is red out of the box on macOS for reasons that have nothing to do with the change under test: the BSD userland shipped with macOS differs from the GNU tools on the Linux CI runners, and CI never sees any of it. This PR closes the two worst of those, both scoped to `hack/`. **Defect 1 — the runner cannot parse a non-ASCII `@test` title.** `hack/cozytest.sh` rebuilds each test's function name by walking its title one character at a time with awk `substr()`. BSD awk aborts that walk on the first multibyte byte (`towc: multibyte conversion failure`) rather than returning it, so a file carrying a non-ASCII `@test` title never parses and not one of its tests runs — silently, because the suite only reports that the runner errored. Four `hack/*.bats` files carry such a title today. The fix pins `LC_ALL=C` on all three awk passes (title parse, run selection, and the exit-code scan of the captured log) so each reads the input byte-wise; the parse and selection passes must share the ctype or they would derive mismatched function names for one title. A new regression test, `hack/cozytest-nonascii-title.bats`, builds a multibyte-titled fixture at runtime and asserts the runner actually runs it; it is red against the pre-fix runner (mutation-verified) and green after. **Defect 2 — GNU-only `sed -i`.** `hack/nightly-mirror.sh`, `hack/promote-rewrite-tags.sh`, and the fixture builder in `hack/promote-rewrite-tags_test.bats` edit files in place with `sed -i`. BSD sed reads the argument after `-i` as a mandatory backup suffix, so `sed -i -e` and `sed -i -E` swallow the following flag as that suffix and the edit either errors or silently no-ops. Each is rerouted through a temp file, portable to both seds. The `nightly-mirror.sh` rewrite additionally relied on the GNU `\?` optional quantifier, which BSD BRE reads as a literal `?` so a bare host was never matched; it is replaced with the POSIX `\{0,1\}`, equivalent on both. All three suites are green on macOS after the change, and behaviour on GNU sed is unchanged. ### Scope This PR is deliberately scoped to Defects 1 and 2. Defect 3 from the issue — a GNU-only `date -d` in `hack/seaweedfs-naming-audit.sh` — is tracked separately as #4273 and is left untouched here. For transparency: with Defect 1 fixed, several `hack/*.bats` suites that previously never ran now run, and a handful still fail on macOS because they depend on GNU coreutils `timeout`, which base macOS lacks (`capture-dataplane`, `cozytest-capture-gate`, `run-kubernetes-join-timing_test`, `run-kubernetes-runner-cpu_test`, `runner-identity`). Those failures are pre-existing — identical before and after this change when the runner is swapped in place — and are a distinct defect from the two fixed here, so they are out of this PR's scope. A fully green `make unit-tests` on macOS additionally depends on #4273 and on that separate `timeout` gap. ### Verification All fixes were verified on a genuine macOS host (Darwin, BSD awk 20200816 / BSD sed). `hack/cozytest-nonascii-title.bats`, `hack/nightly-mirror_test.bats` (5 tests), and `hack/promote-rewrite-tags_test.bats` (17 tests) are green; the two files the issue reports as running zero tests (`overlay-main-images_test.bats`, `promote-gate-contract.bats`) now yield 13 and 15 `Test OK`, matching the issue's numbers. Each fix is mutation-proven: reverting it turns the corresponding suite red again. ### Downstream repositories The diff touches only internal `hack/` tooling — the bats test runner and two release scripts. No downstream repository vendors or restates these: `cozystack/ccp` and `cozystack/external-apps-example` gate on `hack/package.mk`, `hack/common-envs.mk` and `hack/update-crd.sh`, none of which this PR changes, and nothing renames or moves a file or alters what a make target does. - [x] No downstream repository is affected by this change ### Release note ```release-note fix(hack): make the bats unit-test runner parse non-ASCII `@test` titles under BSD awk, and route the in-place edits in `nightly-mirror.sh` and `promote-rewrite-tags.sh` through a temp file so they work under BSD sed — closing two of the host-tool gaps that make `make unit-tests` red on a clean macOS checkout ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved test runner compatibility with non-ASCII test titles and output. * Improved text-processing and file-rewrite behavior across BSD and GNU environments. * Preserved file permissions and metadata during automated rewrites. * Improved handling of host values and release tags across supported platforms. * **Tests** * Added coverage for non-ASCII test titles. * Updated rewrite tests for portable BSD and GNU tooling. * **Documentation** * Clarified supported BSD and GNU command-line tool compatibility. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
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> |
||
|
Aleksei Sviridkin
|
1c88014661 |
fix(installer): keep node_modules out of the packages artifact (#4290)
## What this PR does `flux push artifact` walks the whole `packages/` tree and reads neither `.gitignore` nor `.sourceignore`, so a working copy where somebody ran `pnpm install` under the vendored console ships the console's dependency tree inside the bundle. Helm then refuses the dashboard chart on every cluster that pulls it, because one dependency file sits above the 5MB per-file limit, and the build itself reports nothing: ```text Could not load chart: decompressed chart file "dashboard/images/console/node_modules/.pnpm/@rolldown+binding-darwin-arm64@1.0.0-rc.15/node_modules/@rolldown/binding-darwin-arm64/rolldown-binding.darwin-arm64.node" is larger than the maximum file size 5242880 ``` Passing `--ignore-paths` replaces flux's built-in list rather than extending it, so the defaults are restated alongside `node_modules/`. `hack/check-packages-artifact-ignores.bats` holds both halves: dropping `node_modules/`, dropping one of the defaults, or declaring the variable without passing it to the recipe each turn the test red. Its second case re-reads the defaults out of the installed flux, so an upstream addition surfaces as a failure rather than as a pattern quietly missing from the artifact. Measured on a tree with the console's dependencies installed: the artifact goes from 80MB carrying 32397 `node_modules` entries to 6.7MB carrying none, with the dashboard chart still in it. ### Screenshots Not a UI change. ### Downstream repositories - [x] No downstream repository is affected by this change The two workflows that push the same artifact themselves (`e2e-fork.yaml`, `nightly.yaml`) run from a clean CI checkout, where the tree has no `node_modules` to pick up. ### Release note ```release-note fix(installer): the packages OCI artifact no longer picks up node_modules from a developer's working copy, which made the dashboard chart unloadable on every cluster that pulled the bundle. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Package artifacts now exclude unnecessary files, including version-control metadata, common media and archive formats, and dependency directories. * This helps prevent oversized files from exceeding Helm’s per-file size limit. * **Tests** * Added validation to ensure package artifact ignore patterns remain complete and aligned with the packaging tool’s defaults. * Optional checks report when the packaging tool is unavailable without failing the test suite. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
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 -->
|
||
|
IvanHunters
|
c4f72325a6 |
fix(test): pass over the flux defaults check instead of aborting the suite
hack/cozytest.sh is not bats and defines no `skip`, so the guard for a missing flux killed the whole file with `skip: not found` and kept the unit job red. A red checks job also cancels finalize and e2e, so the recipe this suite exists to hold never ran in CI at all. The form the rest of the suite already uses returns 0 instead, as hack/release-changelog-behaviour.bats does for a missing node. Verified by mutation: with flux installed, dropping a pattern from FLUX_DEFAULT_IGNORES still turns the test red, so the early return does not make it vacuous. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
scooby87
|
6677c24beb |
fix(ci): fail the trailer check on an empty commit range (#4278)
## What `hack/check-commit-trailers.sh` printed `Commit trailers OK: 0 commit(s) checked.` and exited 0 when the range it was given resolved to zero commits, so a run that examined nothing was indistinguishable from a clean run. An empty range now prints a message naming the empty case and exits 1. ## Why The documented pre-push recipe in `docs/agents/contributing.md` is `hack/check-commit-trailers.sh origin/main..HEAD`. The range is resolved in the process working directory, so an agent whose shell sits in a different checkout than the branch resolves it against the wrong `HEAD` and gets nothing — and the old green there was a green nobody could tell from a real one, defeating the check precisely when the checkout is misconfigured. The CI path is unaffected: `.github/workflows/pre-commit.yml` builds the range from a pull request's merge base and head SHA, which GitHub refuses to open with no commits between the two, and which goes empty only once the base has absorbed the head's commits and there is nothing left to merge, where a red trailer check is harmless. ## Test TDD, bats: the previously buggy `accepts an empty range` test is inverted into `fails on an empty range instead of reporting OK`, which asserts a non-zero exit, that the message names the empty case, and that the `Commit trailers OK` line is absent. Mutation-verified — removing the guard turns it RED, restoring it turns it GREEN. Full suite: 28 passing, 0 failing. ## Release note ```release-note Local runs of `hack/check-commit-trailers.sh` now fail on an empty commit range instead of reporting OK, so the documented pre-push check no longer passes silently when it examined nothing. ``` Fixes #4263 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Commit trailer checks now fail when the specified commit range contains no commits. - Empty ranges no longer report a misleading successful validation. - Error output clearly indicates that no commits were checked. - **Tests** - Added coverage confirming empty commit ranges return a failure status and appropriate diagnostic output. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
myasnikovdaniil
|
a1b32f2e5a |
ci(workflows): build Talos only when its sources changed (#4021)
Stacked on #4020, not independently mergeable. Nothing on the same-repo PR path consumes either half of what `build-talos` produces. `image-talos` pushes `${REGISTRY}/talos:pr-N-sha`, which no chart in the tree references and which is not even version-tagged here (`PUBLISH_VERSIONED` and `PUBLISH_FLOATING` are both 0). `image-matchbox` stamps `packages/extra/bootbox/images/matchbox.tag`, and that one line is the whole content of `pr-patch-fragment-talos` - checked against the artifact from a real run - but bootbox is opt-in and the E2E platform Package does not enable it, so no run pulls the image. Unstamped tag leaves installer artifact pointing at released matchbox, which is a valid ref nothing on this path resolves. So the job ran a privileged siderolabs imager build on every PR for two artifacts with no reader. Now it runs when the PR touched `packages/core/talos`, and `finalize` accepts a skipped result as well as a successful one. Without that second half skipping it would block finalize, and therefore e2e, on nearly every PR. Forks keep the unconditional build, and the reason is not about talos. `e2e-fork.yaml` publish job requires at least one OCI archive to exist, an invariant that holds today only because this job always ran, so a fork PR touching nothing under `packages/` would arrive with an empty export and fail a legitimate run. Teaching that guard what `plan` expected to build is work in the fork allowlist path and belongs in its own change. Saving is about 1.5-2 min of critical path. Main point is removing a privileged image build that nobody reads from every pull request. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **CI Improvements** - Pull request validation now runs Talos builds only when relevant changes require them, while retaining full validation for fork-based contributions. - Finalization correctly handles skipped Talos builds and preserves generated boot artifacts when builds succeed. - Changed-file detection now accounts for renames, deletions, and shared build inputs. - **Documentation** - Updated testing and build guidance to reflect the current validation behavior. - **Tests** - Added comprehensive coverage for build classification, workflow outputs, skipped jobs, and artifact handling. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Myasnikov Daniil
|
1118b340a7 |
fix(ci): teach the overlay to repair an unstamped images/*.tag ref
hack/overlay-main-images.sh walks images/*.tag, but every branch of its line classifier needs either a YAML key or an @sha256:. A .tag file holds one bare reference and nothing else, so until a build stamps it the line has neither, the drift guard reads it as a non-ref change, and the committed reference survives. For a package onboarded with the :v0.0.0 placeholder this repo already uses for a new image, that surviving reference is one no registry serves. The overlay is the only thing that would replace it for a PR that does not rebuild the package, so once such a package is on main every full-suite run that skips its build keeps the placeholder. The same placeholder inside a values.yaml repairs itself, because there it sits under an `image:` key the classifier recognises: the difference is the file shape, not the placeholder. The gap is latent rather than new. 17 of the 19 .tag files in the tree carry @sha256: and pass on that branch alone; the only other digest-less one, extra/seaweedfs/images/seaweedfs-cosi-driver.tag, is third-party, so build-main never rebuilds it, the artifact copy is byte-identical and the classifier is never reached. The first digest-less first-party .tag is what turns it live. hack/promote-rewrite-tags.sh already carries a second match branch for this shape and states why. This is that branch, scoped to *.tag so an unkeyed line in a values.yaml still counts as drift. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Alexey Artamonov
|
7fd8376ccb |
fix(ci): fail the trailer check on an empty commit range
The pre-push recipe resolves origin/main..HEAD in the process working directory, so an agent whose shell sits in a checkout at or behind the base resolves it to zero commits and the check printed "Commit trailers OK" with exit 0. A run that examined nothing was indistinguishable from a clean one, defeating the recipe exactly when the checkout is wrong. Treat an empty range as a named failure so the misconfiguration is loud; the CI path is unaffected because it builds the range from a pull request's merge base and head, which is empty only once nothing is left to merge. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
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 |
||
|
Aleksei Sviridkin
|
8799a4e4aa |
fix(cluster-api): backport live migration before host eviction (#4291)
## What this PR does CAPK v0.1.10 handles host eviction by draining and deleting the guest VMI. It also looks up the physical host name in the guest cluster, and a recreated guest can remain cordoned. Backport CAPK #374, #389 and #392 onto the current provider API and runtime, with separate bounded submission, active migration and fallback windows, and ordinary VMIM objects without a priority, a field the pinned `kubevirt.io/api` v1.5.0 does not carry. The submitted VMIM deadline survives reconciliation; expired, invalid or legacy marker values enter the existing fallback path. Add the manager build to the package pipeline, read its digest pin through the provider imageUrl override, and ship VMIM create permission in the compressed provider manifest. Source tests and four packaging checks pass, including failed-build pin preservation and the rendered ConfigMap payload. The regression tests include unprocessed submissions, legacy and invalid deadline values, repeated reconciles, and a fresh active migration window after submission expiry. The updated manager also cross-compiles for linux/amd64 with Go1.26.4. The previous OCI build predates the submission-deadline correction and does not validate an image containing this change. The updated source still needs the maintainer-owned image build described below. The checked-in pin still identifies the original upstream image, so installing this source chart directly does not enable the backport. The regular main and release builds now include this package, stamp the built image digest, and package that exact artifact; the release workflow also commits the generated pin. Pre-merge E2E validation needs a maintainer-owned build because fork publication only allows image repositories already present in the base tree. ### 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: ### Release note ```release-note fix(cluster-api): attempt live migration before drain/delete on External eviction, use the guest node name for fallback drain, and uncordon recreated guests. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added a Cluster API Provider KubeVirt image pinned to version 0.1.10. - Added migration-first workload eviction with fallback draining and node recovery. - Added permission to create virtual machine migration resources. - Included the provider image in builds and deployment templates. - **Bug Fixes** - Preserved migration deadlines across retries and improved recovery after interrupted or completed migrations. - Added a bounded wait for migrations before falling back to draining. - **Documentation** - Documented the provider image version, patches, build process, and release considerations. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
ee09e73343 |
fix(kafka): make topics[].config optional (#3935)
## What this PR does Makes `topics[].config` optional across the Kafka typed API, generated schema, documentation, and chart contract. The KafkaTopic template already omits an empty config, and Strimzi accepts the field as optional; the Cozystack schema was the layer incorrectly rejecting such topics. The Helm test renders a topic without config and asserts `spec.config` is absent. The Chainsaw fixture also omits config and waits for the resulting KafkaTopic to reach Ready. Go API compatibility: `Topic.Config` changes from `runtime.RawExtension` to `*runtime.RawExtension`, so Go callers constructing that field must pass a pointer. The Terraform provider intentionally does not model Kafka topic tuning (`config`), so this does not require a provider follow-up. Validation: - `helm unittest packages/apps/kafka`: 36/36 passed. - `go vet ./kafka` in `api/apps/v1alpha1`: clean; the package has no test files, so this shows it compiles. - root `make generate`, which owns `zz_generated.deepcopy.go`, and `make -C packages/apps/kafka generate`: no diff. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map against all nine changed files. The provider's Kafka model deliberately excludes `topics[].config`; no downstream repository mirrors this nested field or its requiredness. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note fix(kafka): allow topics to omit the optional Strimzi config map ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Kafka topic configuration is now optional. Topics can be created without explicitly providing configuration settings. - Topic configuration is omitted from serialized resources when not specified. - **Bug Fixes** - Improved handling of topic resources without configuration, including correct readiness validation and rendering. - **Documentation** - Updated Kafka configuration documentation and schemas to reflect that topic configuration is optional. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Alexey Artamonov
|
8fbec92411 |
test(hack): match rewrite diagnostics literally in the mirror test
The two assertions checking the dry-run host-rewrite diagnostic used `grep -q` with patterns carrying the registry hostnames `iad.ocir.io` and `ghcr.io`. The dots are BRE wildcards, so the assertions pass on a diagnostic whose host bytes are corrupted (`iadXocir.io` matches), which is exactly the failure the test exists to catch. Switch both to `grep -Fq` so the host strings are compared literally. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
a9dd898684 |
fix(hack): correct the nightly-mirror dry-run diagnostic and BSD-sed comments
The --dry-run line advertised `sed -i 's|...|g'`, an idiom the real path stopped using once the edit moved through a temp file, and it printed only the first of the two expressions the rewrite applies -- understating what the run touches and re-describing a command the script no longer runs. Report the rewrite by intent instead, covering both the `<host>/` prefix and the bare-host scalar. Its test asserted the old `s|...|g` string, so it encoded the same stale idiom; retarget it at the new message and guard against `sed -i` reappearing in the plan. Also tighten two comments that overstate what BSD sed does: the `\?` note claimed the quantifier merely fails to match a bare host, but `["']\?` reads as "a quote then a literal ?" so the whole expression is dead on macOS, matching neither shape; and the `cat`-over-temp-file note now records that `cat` truncates before it writes, an interrupt-window trade taken to avoid `mv` stamping mktemp's 0600 onto rewritten files. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
cb08472c98 |
fix(hack): make in-place sed edits portable to BSD sed
nightly-mirror.sh and promote-rewrite-tags.sh, and the fixture builder
in promote-rewrite-tags_test.bats, rewrite files with `sed -i`. BSD sed
reads the argument after -i as a mandatory backup suffix, so `sed -i -e`
and `sed -i -E` consume the following flag as that suffix: the edit
either errors or silently no-ops, leaving `make unit-tests` red on macOS
while CI stays green on GNU sed.
Route each edit through a temp file instead. The nightly-mirror rewrite
also relied on the GNU `\?` optional quantifier, which BSD BRE reads as a
literal `?` so a bare host never matched; replace it with the POSIX
`\{0,1\}`, equivalent on both seds.
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
|
||
|
Alexey Artamonov
|
0473079448 |
fix(hack): parse non-ASCII @test titles under BSD awk
cozytest.sh rebuilds each test's function name by walking its @test title one character at a time with awk substr(). BSD awk aborts that walk on the first multibyte byte instead of returning it, so a file carrying a non-ASCII title never parses and none of its tests run -- silently, since the suite only reports that the runner errored. Four hack/*.bats files carry such a title today, and the gap is invisible to CI because GNU awk does not abort on the same input. Pin LC_ALL=C on all three awk passes -- the title parse, the run selection, and the exit-code scan of the captured log -- so each reads the input byte-wise. The parse and selection passes must share the ctype or they would derive mismatched function names for one title. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
324a342e61 |
test(seaweedfs): make the BSD-date shim honor the audit's real format string
The two end-to-end tests that reproduce issue #4273 on a GNU runner drive the audit through _write_fake_date, a `date` shim that rejects the GNU `-d` spelling so the script must take its BSD `date -j -f` branch. But the shim discarded the format argument the script passed and recomputed the epoch with its own hardcoded `%Y-%m-%dT%H:%M:%S`. So the one string those tests exist to protect -- the audit's BSD format spelling, which is the whole of #4273 -- was never actually exercised: a regression in it left the tests green. The only test that hits a real BSD `date -j -f` short- circuits on the GNU branch on the CI runner, so the gap was invisible exactly where the gate runs. The shim now captures both the format and the value and uses the format the audit passed. On a BSD host it honors it through real `-j -f`, so a wrong format fails as production would; on a GNU-only runner, which has no `-j` to enforce a format, it falls back to `-d` only for the exact format the audit is supposed to pass, so a regressed spelling is not silently rescued and the tests turn red on the very runner they guard. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
6aa4ca954a |
fix(seaweedfs): parse naming-audit timestamps portably across GNU and BSD date
The naming audit read PV and first_deployed timestamps with `date -u -d`, a spelling only GNU date accepts. On a host with BSD date the call failed, its stderr was discarded by the `2>/dev/null` on the same line, and `|| return 0` turned that host-tool failure into the audit's "no epoch" signal -- so every tenant read as MIXED with "direction cannot be established", as if the cluster lacked the evidence rather than the host lacking a readable clock. Since the audit gates a destructive runbook step, that indistinguishability is the defect. parse_epoch now tries the GNU spelling then the BSD one (`date -j -f`) and uses whichever the host speaks, so both call sites work on GNU and BSD hosts alike with no behaviour change for a valid timestamp on GNU. A timestamp that is present but readable by neither form is no longer folded into the absent-evidence branch: it is fatal and names the value, keeping a genuine parse failure loud and distinct from the legitimate "no bound PV age" case (unbound claim, NotFound PVC/PV) the audit conservatively degrades on. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Yan Bondarenko
|
0e4d6a305c |
test(cluster-api): declare the subshell EXIT traps in the CAPK bats suite
hack/bats-no-exit-trap.bats is an exact ratchet, and the new suite installs three traps inside explicit subshells without declaring them, so make unit-tests fails on the merge commit. The header now declares the count and says why the three are not debt. Assisted-by: LLM Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> |
||
|
Yan Bondarenko
|
c8f4906e93 |
fix(cluster-api): backport live migration before host eviction
Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> |
||
|
Yan Bondarenko
|
8ec76a3614 |
fix(kafka): make topics[].config optional
Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> |
||
|
Aleksei Sviridkin
|
88a7ea0e3c |
fix(kubeovn): retry VM migration setup after scheduling (#4253)
## What this PR does Kube-OVN can drop VM migration setup when a migration enters `Scheduling` before the target Pod is visible or the source or target node is known. The phase may remain unchanged afterward, leaving the target Pod waiting for network setup. Backport the final fix from kubeovn/kube-ovn#6834 to the pinned v1.15.10 image: return an error for the transient missing-node case so the existing rate-limited queue retries it. Keep the same-node behavior unchanged. That backport can be removed when updating to v1.15.14 or later. Add a separate retry guard for the target-Pod creation race. It returns an error when the informer cache has no target Pod yet, instead of forgetting the migration event. This guard is not part of #6834 and must remain until an equivalent fix is verified in the selected upstream version. Also include the existing kubeovn image target in the root build list. Without it, a kubeovn patch produces an empty PR build matrix and the image is omitted from the regular build and release paths. Validation: - All six regression cases pass on Linux/amd64 using the real controller workqueue. The new missing-target-Pod case fails without its guard and passes after the Pod appears in the informer cache, without changing the VMIM phase. The existing deferred-node, already-scheduled, and same-node cases continue to pass. - All 19 build-matrix tests pass; the full matrix includes kubeovn exactly once. - Both lint steps, pinned golangci-lint v2.10.1 and upstream `modernize -test -skipgenerated ./...`, pass for the Linux/amd64 target using Go 1.26.4. On macOS, modernize is built for the host and run with the Linux target environment, since `go tool` otherwise tries to execute a cross-compiled Linux binary. - All three packaged patches apply cleanly to pinned v1.15.10 with `git apply --check --whitespace=error`; the resulting source matches the tested tree. Formatting and source/build whitespace checks pass. The regression test covers controller queue behavior; the standard container E2E environment does not cover DRBD-backed live migration. The regular image build applies the backport and stamps the resulting digest before packaging. ### Screenshots Not applicable; no 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 change adds an existing image target to the build list; package layout, make target interfaces, chart values, APIs, and published image-reference formats stay the same. CCP discovers package image targets dynamically and does not carry the root build list. The website has no documented workaround for kubeovn/kube-ovn#6823 or #6834 to remove. ### Release note ```release-note fix(kubeovn): retry VM migration network setup when the target Pod or source/target scheduling information is not yet available, and include the Kube-OVN image in the regular build matrix. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved VM migration handling when source or destination node scheduling information is temporarily unavailable. - Migration events are now retried after scheduling completes instead of being dropped. - Preserved existing behavior when migration source and destination nodes are identical. - **Tests** - Added coverage for migration scheduling and retry scenarios, including partially assigned and same-node cases. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
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 |
||
|
Timofei Larkin
|
c398e0e116 |
Merge branch 'main' into chore/drop-http-cache
main took migration 57 and moved migrations.targetVersion to 58 while this branch was open, so that line is the one conflict: 57 at the merge base, 58 on main, 59 here. Resolved as 59, which is what this branch's migration 58 requires, since run-migrations.sh loops up to targetVersion - 1. Merged rather than rebased because the branch carries commits that are not mine to rewrite. Signed-off-by: Timofei Larkin <lllamnyp@gmail.com> |
||
|
Timofei Larkin
|
2cbc02bd10 |
fix(platform): freeze HTTPCache on the artifact the cluster runs
Pinning the frozen source to a digest written into the migration picks one release for every cluster, and v1.6.0 is not even the last one that ships http-cache: v1.6.1 through v1.6.3 and v1.7.0-alpha.1 all do, and 1.6 patch releases may keep doing so. A cluster on any of those would be rolled back to an older chart by the upgrade that was meant to leave its instances alone, and an air-gapped mirror resolves the constant only if someone mirrored that exact manifest. The hook runs before the upgrade re-points cozystack-packages, so the live object still names the release the instances render from, in whatever registry the operator serves it from and with whatever pull secret and verification they configured. Reading the digest off it makes the freeze a no-op by construction and needs no knowledge of which tag was last. spec.ref.digest is used when the installer pinned one; a tag or semver ref goes through status.artifact.revision, whose digest half is the manifest digest. status.artifact.digest is the checksum of the downloaded tarball and cannot stand in. A source with neither has nothing to freeze on and the migration stops there. The copy also carries the no-delete label the platform chart puts on the original, because it is the only chart source those instances have left. The tenant grant travels with the freeze as well. cozy:tenant:admin:base enumerates the apps.cozystack.io resources a tenant owner may write, and an application that has left the catalog has no business in that list: the next sweep over it takes the entry as dead config, and the failure is silent and remote, a Forbidden for the one person who still needs to remove what they have. The chart drops httpcaches, and a cluster that freezes gets a ClusterRole under the tenant admin aggregation label instead, applied before the upgrade drops the rule so the access never lapses and carrying the verb set the chart had so nothing changes for the owner. A cluster with no instance is granted nothing. The Package delete on the no-instance path has to block. The operator reconciles off an informer cache, so a reconcile in flight against the pre-delete entry re-creates the release it owns, and with it the ApplicationDefinition and the Helm storage the following lines remove. Returning immediately made the ordering a property of the command log rather than of the cluster. The preset check's SIGPIPE note named http-cache-rd as its example of a large enum, and that chart goes away here; kubernetes-rd carries the same enums. Numbered 58 rather than 57: main already carries a migration 57, the SeaweedFS volume-size-limit one, and its targetVersion is already 58. The whole shape of this retirement was worked out here rather than anywhere reusable, so docs/deprecating-applications.md writes it down as a procedure: how to pick between migrating onto a successor and freezing, what the prune takes with it, and what the migration owes a cluster on each branch. The next application to go should not have to rediscover it. Assisted-by: LLM Signed-off-by: Timofei Larkin <lllamnyp@gmail.com> |
||
|
Yan Bondarenko
|
55a2918fcb |
fix(kubeovn): retry VM migration setup after scheduling
Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> |
||
|
Aleksei Sviridkin
|
d3e228fed8 |
test(opensearch): roll a single-node cluster held yellow by a replica
The roll onto the new mount plan timed out on some runs and passed on others with identical content. What differed was cluster health at the moment the listener moved: on one data node an index that asks for a replica stays yellow for good, and the operator restarts a pod only once its health gate opens. The security audit log creates such an index on its first audited event, and whether that had happened yet was down to timing. Create the index in the suite so the roll always has to get through a yellow cluster, and read the health back so the step cannot keep passing while it proves nothing. The operator logs that health held a restart back, never what the health was, and cluster health is not a Kubernetes object. The catch block now reads it from the node, together with the shard table and the allocation explanation. Signed-off-by: Aleksei Sviridkin <f@lex.la> Assisted-by: LLM |
||
|
Alexey Artamonov
|
353f37e4fe |
test(kafka): widen the fresh-create e2e cleanup budget for KRaft
The fresh-create Kafka suite relied on the shared 5m cleanup default, but a KRaft cluster tears down broker and controller node pools, the entity operator, and their PVCs — more work than a single ZooKeeper node. Under a contended CI run (concurrent Postgres/OpenSearch/Redis suites) that teardown overran 5m and failed the suite after every assertion had already passed, so cleanup latency, not the test, was deciding the verdict. Give this suite its own 8m cleanup timeout; the migration suite runs in its own namespace and is unaffected. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Andrey Kolkov
|
6e18ceef53 |
fix(kafka): fail closed when the migration hook cannot list Kafka CRs
The "free the kafka pool" gate counted other ZooKeeper clusters through a `kubectl ... | awk` pipeline under `set -eu` without `pipefail`, its kubectl error dropped by `2>/dev/null`. A failed read then counted zero and kept the shared pool, stranding any co-namespaced cluster still waiting for the name. Separate the read status from the count, mirroring the drain read below, and cover it with a bats case. Assisted-By: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Alexey Artamonov
|
cd9f4d6a27 |
feat(kafka): auto-migrate every ZooKeeper Kafka in a namespace to KRaft
Extends the ZooKeeper->KRaft migration hook so that N Kafka clusters sharing one namespace all reach KRaft automatically, with no manual steps. This closes the last gap in the migration: previously only one cluster per namespace could migrate in place (the broker pool must be named exactly "kafka" to adopt the existing <cluster>-kafka-N brokers, and that name is namespace-unique), so a second co-namespaced ZooKeeper cluster hit the render guard and its HelmRelease never rendered again -- stuck on ZooKeeper with no way forward once a future Strimzi drops the migration. The clusters now serialize on the "kafka" name automatically. Whichever upgrade wins the name (claimed atomically via kubectl create) migrates in place; a cluster that loses fails its hook fast and its next reconcile retries. Once a migrated cluster sees other ZooKeeper clusters still waiting, it frees the name: it stands up its own release-hashed "b-<hash>" broker pool (node IDs pinned clear of the existing broker and controller ranges), enables Cruise Control, drains every partition off the "kafka"-pool brokers with a remove-brokers KafkaRebalance, deletes the drained pool, and removes Cruise Control again. The next waiting cluster's retry adopts the freed name; the last one to migrate keeps "kafka". Everything happens inside the one namespace -- the hook's Role is namespaced and it never reads or touches another. Robustness details learned from live runs: - The drain rebalance sets skipHardGoalCheck: true. A mandatory broker evacuation must not be blocked by Cruise Control's capacity hard goals; under a small resources preset the capacity model can briefly over-estimate CPU from noisy samples and reject the proposal with CpuCapacityGoal even when real CPU is near idle. - The drain wait refreshes the proposal (strimzi.io/rebalance=refresh) on any NotReady and keeps polling, rather than treating it as terminal: a just-enabled Cruise Control is unreachable for a minute or two, and its model settles as metric windows age out. This also makes the drain resumable -- a re-run refreshes a stale rebalance instead of reading its old failure forever. - Cruise Control is enabled with explicit resources (it is a JVM and a tenant LimitRange would OOM it at 128Mi), transiently -- kafka.yaml never sets spec.cruiseControl, so Helm's server-side apply does not fight the hook patching it on and off. Every phase is idempotent so a Job killed mid-drain (the drain scales with data size and can run for hours) resumes on retry; the Job deadline and the release's helm-install-timeout are raised accordingly. Verified on a live cluster across five runs of two co-namespaced ZooKeeper Kafkas migrating to KRaft: both reach KRaft automatically, the first drains onto its b-<hash> pool and frees the name, the second adopts "kafka" in place (broker node IDs and PVCs unchanged), and each cluster's data is byte-for-byte identical before and after. helm-unittest 87/87 and hack/kafka-migration-hook.bats 14/14 (the real hook script against a stub kubectl) cover the freeing decision, the atomic name claim, the drain's refresh/skip-hard-goal handling, and the "delete the pool only after the rebalance is Ready" guard. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
cb7578bb31 |
fix(kafka): data-safe, Helm-clean ZooKeeper->KRaft migration
Two live-reproduced blockers in the in-place ZK->KRaft migration are
fixed, plus the concurrency guard behind them.
1. Data loss (node IDs). The migration created the broker and controller
KafkaNodePools together without pinning node IDs. On Strimzi 0.45 the
controller pool deterministically took the existing broker IDs
[0..N-1], so the brokers were recreated empty at [N..] and their data
PVCs were orphaned (reproduced 2/2 on dev9: 20000 messages -> 0,
partitions leaderless, migration wedged in KRaftMigration). Naming the
broker pool "kafka" is necessary but NOT sufficient: Strimzi adopts by
node id, not pool name. The controller pool now pins
strimzi.io/next-node-ids above the broker range, so the broker pool
keeps [0..N-1] and adopts the existing <cluster>-kafka-N brokers and
their data in place.
2. Release stuck "failed" (Helm lifecycle). The pre-upgrade hook creates
the pools, and the chart also declared them as regular manifests, so
Helm's main apply tried to reconcile a resource that exists in the
cluster but was absent from the previous (ZooKeeper) release revision
and aborted with "original object KafkaNodePool ... not found",
leaving the HelmRelease failed even though the cluster migrated fine
(reproduced 3/3 under server-side apply, the production
helm-controller path). This is unavoidable while the pools are Helm
main-manifest resources across the ZK->KRaft boundary, so the pools
are now managed entirely by the migration Job (a pre-install,
pre-upgrade hook) on every install and upgrade, and removed from the
regular manifests. The Job ensures both pools (creating or updating
them idempotently), then drives the migration state machine.
templates/kafkanodepools.yaml and the deployed-version gate ConfigMap
(templates/version.yaml) are removed; the broker pool name is resolved
per path by kafka.brokerPoolName ("b-<hash>" fresh, "kafka"
migrating), and the render-time name-length guard moves to the Kafka
CR template.
3. Concurrency (one migration per namespace). The collision guard read
the "kafka" pool owner and then applied, a non-atomic read-then-write
two concurrent per-namespace migrations could both pass. When the pool
is absent the hook now uses "kubectl create" as the atomic arbiter:
exactly one concurrent creator wins, the loser re-reads the owner and
aborts.
Verified live on dev9 under server-side apply: a fresh install comes up
KRaft with pools at [0,1,2]/[3,4,5] and the release is "deployed"; an
in-place migration of a 20000-message ZooKeeper cluster ends "deployed",
adopts the broker PVCs in place (same UIDs), and the KRaft data is
byte-for-byte identical to the pre-migration ZooKeeper data (matching
sha256 over every partition:offset:key:value). helm-unittest 86/86 and
hack/kafka-migration-hook.bats 12/12 (the real hook script against a stub
kubectl) cover the resolution branches, the fail-closed reads, the atomic
collision guard and the finalize guard.
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
|
||
|
Alexey Artamonov
|
053ef16976 |
feat(kafka): allow multiple KRaft Kafka clusters per namespace
Fresh KRaft clusters no longer need to be alone in a namespace. Their node pools get short, release-hashed names — "b-<hash>" for brokers, "c-<hash>" for controllers — instead of the fixed "kafka"/"controller". Each release hashes differently, so the names are unique within the namespace (KafkaNodePool object names are namespace-unique) and any number of Kafka instances coexist. The hash keeps the pool name a fixed length: Strimzi derives every pod/PVC name as <cluster>-<pool>-<id> and the cluster name is already the release name, so embedding the release name in the pool name too would double it into every pod hostname and blow the 63-character DNS label limit for long release names. A render-time guard fails with a clear message when even the hashed controller node name would still exceed 63 characters. The fixed "kafka" name is still required to adopt an existing ZooKeeper cluster's <cluster>-kafka-N brokers and data in place during migration, so kafka.brokerPoolName resolves to "kafka" for a migrating/migrated cluster and "b-<hash>" for a fresh one. A second ZooKeeper->KRaft migration into a namespace that already holds a "kafka" pool owned by another cluster fails closed, both at render time and in the pre-upgrade hook (whose owner probe uses --ignore-not-found + an explicit exit-code check, so a transient kubectl error aborts rather than silently waiving the collision guard). This replaces the previous blanket "one Kafka per namespace" guard, which over-restricted the fresh case. The pool names are resolved from a live Helm lookup; that is safe because Helm propagates any non-NotFound read error and fails the whole render (pkg/engine/lookup_func.go), so a transient apiserver/RBAC miss retries rather than silently flipping a live cluster's pool name — an empty lookup means genuine absence or a never-applied client-side render. The migration hook and workload monitors follow the same naming. Tests: helm-unittest fixtures (via kubernetesProvider) cover every resolution branch and the fail-closed guards; hack/kafka-migration-hook.bats drives the migration Job's real shell script against a stub kubectl across the fresh / read-error / already-KRaft / owner-collision / happy-path / stuck / retry cases. Verified on a live cluster: in-place ZooKeeper->KRaft migration preserved all data and adopted the original broker PVCs, and two fresh clusters coexist with distinct pools, pods and PVCs. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Andrey KolkovandClaude Opus 4.8
|
03619f3ad2 |
test(kafka): drop source-grepping migration-timeout drift-guard
The removed bats file grepped two static literals in two separate packages (helm-install-timeout in system/kafka-rd, activeDeadlineSeconds in apps/kafka) and asserted an ordering between them. Substring matching cannot verify the contract it targets: the real invariant is between the running migration Job and the release timeout helm-controller actually enforces on the pre-upgrade hook, not between two YAML literals in the tree. It also greened only for the minute-format literal and errored on any legitimate reformat (e.g. "1h" / "2700s"). The timeout > Job-deadline relationship is already documented inline at both values. Its machine-checkable home is an e2e that upgrades an existing release through the HelmRelease pre-upgrade hook and confirms a real migration completes inside the release timeout (the chainsaw suite drives migration via kubectl, bypassing that path). Signed-off-by: Andrey Kolkov <androndo@gmail.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Alexey Artamonov
|
1a68157564 |
fix(kafka): harden migration hook and isolate the migration e2e
Addresses the second review round on the ZK->KRaft migration. Migration hook: - Fully qualify the Strimzi resources as kafkas.kafka.strimzi.io / kafkanodepools.kafka.strimzi.io. The bare plural "kafkas" also exists in this chart's apps.cozystack.io group, so a bare name resolves by per-cluster discovery priority and can hit the tenant kind — which the hook Role does not grant, failing the pre-upgrade hook deterministically. - Classify NotFound structurally via `kubectl get --ignore-not-found` instead of substring-matching stderr, so an unrelated "... not found" error can no longer be mistaken for a fresh install and permanently skip a live ZooKeeper cluster. This also drops the jq dependency. - Treat a KRaft-native / already-migrated CR (annotated strimzi.io/kraft=enabled) as done, never driving it backwards into kraft=migration. - Keep only `before-hook-creation` on the Job's hook-delete-policy so a failed attempt's pod logs survive for a post-mortem until the next run. - Route the kubectl image through cozy-lib.image (cluster registry mirror) and pin it by digest (clastix/kubectl:v1.32@sha256:...), matching the other cozystack hooks, instead of an unpinned docker.io tag. Controller quorum: - Fix the controller node-pool replica count at creation instead of deriving it from kafka.replicas on every reconcile. Strimzi 0.45 cannot scale a controller pool, so a day-2 edit of kafka.replicas (e.g. 1 -> 3) must not flip the quorum; the helper now keeps the existing "controller" pool's replica count when it is present. Docs: - Document that migration to KRaft is one-way: neither a version-field downgrade nor a pre-KRaft chart rollback is supported once migrated (Strimzi has no KRaft->ZooKeeper rollback after completion). E2E: - Run the ZK->KRaft chainsaw migration in its own namespace instead of the shared tenant-test. It raw-applies Strimzi resources plus a namespace-wide LimitRange and leaves deleteClaim:false PVCs, which polluted the sibling kafka suite. Raise the seed readiness timeout for CI headroom. Validated against a live cluster: migration adopts the broker PVC in place and reaches KRaft under a 128Mi LimitRange, in isolation. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
f9676961f3 |
fix(kafka): set resources on the migration node pools
The ZK->KRaft pre-upgrade hook created the adopting broker ("kafka") and
"controller" KafkaNodePools without a resources block. Those hook-created pools
govern the Kafka pods for the entire migration window — Helm applies the
resourced steady-state kafkanodepools.yaml only after the pre-upgrade hook
finishes. In a tenant namespace the LimitRange defaults every container to
128Mi, so the migration's controller (and rolled broker) JVMs OOM, never reach
KRaftPostMigration, and the hook times out — wedging the migration and thrashing
it on every reconcile.
Give both hook pools the same resources the steady-state pools get
(cozy-lib.resources.defaultingSanitize from kafka.resourcesPreset/resources).
This also avoids a redundant second broker roll (resource-less -> resourced)
after the hook completes.
Tests: a helm-unittest assertion that both migration pools render a resources
block; the Chainsaw ZK->KRaft suite now seeds under a 128Mi LimitRange (and
gives the seed cluster explicit resources), reproducing the tenant-quota
condition, and asserts the migration still reaches KRaft with the broker PVC
adopted. Validated green against a live cluster under the LimitRange.
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
|
||
|
Alexey Artamonov
|
649f7e0b63 |
fix(kafka): enforce one Kafka per namespace; simplify node pool names
Follow-up to the KRaft-migration review. The broker KafkaNodePool must be named exactly "kafka" so Strimzi adopts an existing ZooKeeper cluster's <cluster>-kafka-N brokers and data in place during migration, but KafkaNodePool object names are unique within a namespace — so two Kafka clusters cannot both own a "kafka" pool there. This is an upstream constraint and Strimzi's own guidance is one Kafka cluster per namespace (strimzi/strimzi-kafka-operator discussions/11120). Adopt that model instead of the earlier fresh-vs-migrated conditional naming: - New kafka.assertSingleInstance helper fails the render (with a clear message and the upstream reference) if the namespace already holds another Kafka cluster. Reconciles/upgrades of the same release are allowed; lookup returns nothing during `helm template`, so unit tests never trip it. - Node pools are now named "kafka" (broker) and "controller" unconditionally, which also adopts existing brokers on migration and yields clean <cluster>-kafka-N / <cluster>-controller-N names. The fresh-vs-migrated brokerPoolName helper is removed. - WorkloadMonitor pool-name selectors and the migration hook updated to match; unit tests updated for the fixed pool names. - README documents the one-Kafka-per-namespace rule and links the Strimzi discussion. The ZK->KRaft Chainsaw migration suite (seed ZooKeeper cluster, migrate, assert the original broker PVC survives and no wrongly-named PVC is created, cluster reaches KRaft) was run green against a live cluster. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
710cdd18bb |
fix(kafka): address KRaft-migration review — broker pool adoption, hook safety, tests
Addresses the branch review on the ZK->KRaft migration path. CRITICAL — broker node pool adoption. Strimzi derives broker/PVC names as <cluster>-<pool>-<id>; a pre-node-pool ZooKeeper cluster has brokers <cluster>-kafka-N, so the adopting broker pool must be named exactly "kafka" or the operator provisions fresh empty brokers and orphans the tenant's data. A new kafka.brokerPoolName helper resolves the broker pool to "kafka" for an existing (migrating) cluster and keeps the release-scoped "<release>-broker" for fresh installs, so multiple Kafka instances still coexist in one namespace. The migration hook creates the pool as "kafka" and fails closed if a "kafka" pool already belongs to another cluster (node pool names are namespace-unique, so in-place migration of co-namespaced ZK clusters is unsupported). Wired through kafkanodepools.yaml, workloadmonitor.yaml and the hook. MAJOR — migration robustness. - Raise the HelmRelease timeout: the migration Job can run tens of minutes but the release timeout defaults to 10m, so helm-controller aborted and thrashed the pre-upgrade hook. Add release.cozystack.io/helm-install-timeout=45m on the kafka ApplicationDefinition, above the Job's activeDeadlineSeconds (2400s). - Idempotent migration: do not re-issue kraft=migration once the cluster is past the ZooKeeper phase (an invalid backward transition Strimzi rejects on retry). - Fail closed on the decision gates: distinguish a genuine NotFound (skip) from any other kubectl error (abort), and never treat an empty kafkaMetadataState on an existing CR as "migrated". MINOR — version downgrade documented as unsupported; integer (not lexical) compare in the version gate; deleteClaim/PVC-retention and the one-per-namespace migration limitation documented in the README; docs/storage-immutability.md updated for KRaft. Tests — helm-unittest coverage for the broker pool name, the hook guards and the idempotent/fail-closed logic; a bats check for the helm-install-timeout budget ordering; and a Chainsaw ZK->KRaft migration suite that seeds a ZooKeeper cluster, drives the migration, and asserts the original broker PVC survives (not recreated) and the cluster reaches KRaft. The migration sequence and the Chainsaw assertions were validated against a live cluster. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Matthieu
|
42db2ca141 |
feat(kafka): migrate from ZooKeeper to KRaft mode with automatic migration
Replace ZooKeeper-based Kafka with KRaft (Kafka Raft) metadata mode. Strimzi 0.45 is the last line that still supports ZooKeeper, so this prepares the transition before ZooKeeper support is removed upstream. Chart changes: - The Kafka CR always renders strimzi.io/kraft=enabled with node pools and no spec.zookeeper block. Storage, replicas and resources move to separate broker + controller KafkaNodePools (a homogeneous separate-pool layout works for both fresh installs and migrations; Strimzi 0.45 does not support migration into a combined pool). - Kafka version is now user-configurable via the standard files/versions.yaml pattern (v3.9 default, v3.8 available); appVersion bumped to 3.9.0. - WorkloadMonitors track the broker and controller pools (no zookeeper monitor); the dashboard RBAC grants the controller monitor and drops the zookeeper one. Migration of existing ZooKeeper clusters is handled by a Helm pre-upgrade Job (templates/migration-hook.yaml) gated by a <release>-kafka-deployed-version ConfigMap, following the established seaweedfs/etcd hook pattern. The Job creates the broker + controller pools with Helm ownership labels, drives the Strimzi ZK->KRaft state machine (node-pools=enabled, kraft=migration -> kraft=enabled once KRaftPostMigration is reached) and is skipped once the ConfigMap records version >= "1". Testing: - Helm unit tests cover the Kafka CR, node pools, migration hook, metrics, topics, version map, dashboard RBAC and workload monitors. - The e2e suite is ported to the current Chainsaw framework (hack/e2e-chainsaw/kafka): it asserts the KRaft topology (both node pools, kafkaMetadataState=KRaft), the absence of ZooKeeper, and bootstrap endpoints. Rebased onto current main: TLS listener support, entity-operator resource guards, x-cozystack-options schema annotations and the WorkloadMonitor replica-path fix are all preserved and reconciled with KRaft. Signed-off-by: Matthieu <matthieu@hidora.com> Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |