mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-09 05:25:14 +09:00
8a6fe02ee4fa41b8f5baf6039251d00d5b506751
6386
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 IvanHunters
|
8a6fe02ee4 |
fix(bucket): stop the ACME cleanup hook from talking to localhost, and gate the class (#4289)
## What this PR does Fixes #4288, and closes the class behind it and #4276. `packages/apps/bucket/templates/hooks/cleanup-acme.yaml` passed `--request-timeout=30s` on all four of its kubectl calls. A non-zero value there makes kubectl skip its in-cluster credentials: client-go consults them only while the assembled config still equals the default, and the flag lands in that config, so every call went to kubectl's built-in default cluster and was refused. This package failed more quietly than kafka. Each call is guarded by `|| echo "WARNING: ..."` and the script sets no `-e`, so the hook exited 0 and printed `ACME cleanup complete.`, and `hook-succeeded` then deleted the Job holding those warnings. The ACME solver objects it exists to reap were left behind, with the only record removed moments after it was written. Its own suite matched the rendered command including the flag, so the broken form was pinned in place. The gate is the point of the change rather than a bonus. Two packages carried this, neither was visible in a rendered manifest, and both passed their unit suites. Nothing stops the next hook from reaching for the same flag, because it reads as ordinary defensive practice. `hack/check-hook-request-timeout.bats` fails any non-vendored template whose kubectl call carries a non-zero `--request-timeout`. The coarse part is stated in the file rather than left to be discovered: a template that names `KUBECONFIG` is skipped, because an explicit kubeconfig is not served by the in-cluster branch and the flag is harmless there. That is a per-file judgement, not per-command. It is what lets `apps/kubernetes` keep the flag on two calls that pass their own kubeconfig and stay green, and it is the first thing to tighten if a file ever mixes both styles. Checked in both directions rather than only on the clean tree: the gate passes as committed, fails if the flag is put back on any bucket call, fails if it is put back on the kafka call, and leaves the two `apps/kubernetes` files alone. It also carries a vacuity guard, because a collapsed glob would otherwise report success for having examined nothing. Bounding the work is still available where it belongs: `--timeout` for a watch, `activeDeadlineSeconds` for the Job, which this hook already had at 120s. **Depends on #4280** and is branched off it, so the gate is green here. #4280 removes the same flag from the kafka hook; merged before it, this check would be red on `main`. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map against the diff. A hook's kubectl flags and a new `hack/*.bats` check; no values key, schema, API, default or naming contract moves, and nothing under `hack/` is moved or renamed, nor does any make target change what it does. - [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(bucket): the ACME cleanup hook now reaches the apiserver instead of reporting success while doing nothing ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved ACME cleanup reliability by preventing cleanup commands from targeting an unavailable local Kubernetes endpoint. * Improved Kafka uninstall cleanup reliability by ensuring the pre-delete hook retains access to required credentials throughout execution. * Preserved existing execution time limits and warning-based failure handling for cleanup operations. * **Tests** * Added coverage to prevent regressions in Kubernetes cleanup command configuration and hook resource lifecycle behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
IvanHunters
|
b323d5d88c |
fix(kafka): keep the pre-delete hook's credentials as long as its Job (#4280)
## What this PR does Fixes #4276. Since #3938 landed, no Kafka release finishes deleting: the pre-delete hook cannot reach the apiserver, exits non-zero, and a non-zero pre-delete hook returns from `Uninstall.Run` before a single resource is removed. It is the inverse of what the hook was written for. #3793 was one release that would not delete; this made every release refuse to. The cause is `--request-timeout=20s` on the delete call. Any non-zero value for that flag makes kubectl skip its in-cluster credentials altogether: client-go consults them only while the assembled config still equals the default, and the flag lands in that config, so the branch that would load the ServiceAccount token never runs. kubectl talks to its built-in default cluster, `localhost:8080`, is refused, and the script reads the refusal as a failed delete. Measured on the pinned image rather than argued from documentation. With the token and `ca.crt` mounted and `KUBERNETES_SERVICE_HOST` set, the same delete call reaches the cluster with no flag, goes to `localhost:8080` with `--request-timeout=20s`, goes to `localhost:8080` with `1m`, and reaches the cluster again with `0`. The tree agrees. None of the three hooks that work pass it: `apps/kubernetes` names it in a comment only, `apps/harbor` and `apps/mariadb` never mention it, and `apps/tenant/templates/cleanup-job.yaml` records that its DELETE calls "carry kubectl's default `--request-timeout` of 0". The watch is still bounded by `--timeout`, and the Job by `activeDeadlineSeconds`, which is that same shape. The second commit was a separate change found on the way, and review turned it around. It matched the credentials to the Job on `before-hook-creation` alone, and a ServiceAccount, Role or RoleBinding has nothing like `ttlSecondsAfterFinished` to fall back on: helm removes a hook resource only when a policy says so, and `before-hook-creation` fires on a next activation that after the last uninstall never comes. All three would have sat in the tenant namespace for the life of the cluster, still granting delete over every KafkaTopic in it. They now carry `before-hook-creation,hook-succeeded`, the shape `apps/clickhouse` and `apps/kubernetes-nodes` already use, while the Job keeps `before-hook-creation` alone because the script exits 0 on the degraded path too and the Job's log is the only record that a finalizer outlived the wait. Reaping the credentials cannot strand a running pod: helm applies `hook-succeeded` only once the hook is terminal, and `activeDeadlineSeconds: 270` ends the Job inside the 5m uninstall hook timeout. Neither of these caused #4276, and a cluster run confirmed as much. Both are pinned. Restoring the flag turns the suite red, with `0` as well as with `20s`; the assertion matches `--request-timeout=` rather than the bare name, because the script's own comment names the flag. Dropping the reaping policy from the ServiceAccount alone, or from the Role alone, turns it red too, so the three are three assertions rather than one. Worth stating plainly, because it is the reason this reached `main`: none of this is visible in a rendered manifest. The asymmetry and the flag both rendered cleanly, passed 37 helm-unittest cases and 5 bats cases through two rounds of review and per-branch mutation testing, and only a live uninstall showed either. The e2e lane never ran on #3938 while it was open, because its workflows sat at `action_required` on all five heads a fork PR had. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map against the diff. This changes hook annotations and test assertions inside `packages/apps/kafka`; no values key, schema, API, default or naming contract moves, and the app itself is neither added nor renamed. - [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(kafka): deleting a Kafka release no longer wedges on a pre-delete hook that cannot reach the apiserver ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved Kafka app uninstall cleanup by removing an unnecessary request-timeout constraint while retaining existing overall timeout safeguards. - Updated cleanup resource policies to remove failed-hook retention and preserve resources through successful hook completion, supporting reliable repeated uninstall attempts. - **Tests** - Added coverage for cleanup command timeout behavior and resource deletion policies. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
IvanHunters
|
3a43bcf391 |
docs(kafka): say why the hook's Job and its credentials are reaped differently
The policies are right and nothing here changes them. What is missing is the reason, and this file has now been changed in both directions inside a week by people reasoning from the annotations alone, so the reason is the part worth keeping. The Job carries no hook-succeeded because the script exits 0 on the degraded path as well as the clean one, and its log is the only record that a finalizer outlived the wait; ttlSecondsAfterFinished reaps it on a timescale a human can act within. The ServiceAccount, Role and RoleBinding have no such clock, and before-hook-creation fires on a next activation that after the last uninstall never comes, so without a terminal policy they sit in the tenant namespace for the life of the cluster still granting delete over every KafkaTopic in it. Reaping them cannot strand a running pod, which is the objection the symmetrical shape was written against: helm applies hook-succeeded only once the hook is terminal, and activeDeadlineSeconds ends the Job inside the uninstall hook timeout. hook-failed would decide nothing, because a failing hook already reaps the earlier successful ones under hook-succeeded. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
Timur Tukaev
|
94885c2e93 |
docs(governance): remove the unused Directors role (#4296)
## What this PR does Removes the **Directors** role from `GOVERNANCE.md`. The role (non-technical leaders, elected by the maintainers, one vote each in decisions) was defined but never used — no Directors were identified or elected. It was intended for non-technical leadership, but those people ended up maintaining parts of the project directly, so the role stayed empty. Removing it so the documented decision-making body matches actual leadership (maintainers, by consensus with a simple-majority fallback). It can be reintroduced if a real need arises. Addresses cozystack/community#79. ### Downstream repositories - [x] No downstream repository is affected by this change ### Release note ```release-note docs(governance): remove the unused Directors role from GOVERNANCE.md ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Updated governance guidance to remove the Directors role and related leadership descriptions. * Clarified that voting is conducted by maintainers. * Updated consensus language to reflect the revised decision-making process. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
coderabbitai[bot]
|
a295e29b8a | fix(kafka): clean up pre-delete hook ServiceAccount and RBAC on success | ||
|
IvanHunters
|
3615958240 |
fix(bucket): stop the ACME cleanup hook from talking to localhost, and gate the class
The hook's four kubectl calls carried --request-timeout=30s. A non-zero value there makes kubectl skip its in-cluster credentials: client-go consults them only while the assembled config still equals the default, and the flag lands in that config, so every call went to kubectl's built-in default cluster and was refused. This package failed more quietly than kafka did. Each call is guarded by `|| echo WARNING` and the script sets no -e, so the hook exited 0 and printed "ACME cleanup complete."; hook-succeeded then deleted the Job that held those warnings. The ACME solver objects it exists to reap were left behind, and the only record went with the Job. Its own suite matched the rendered command including the flag, so the broken form was pinned. The gate is the point of this change. Two packages had it, neither was visible in a rendered manifest, and nothing stops the next hook from reaching for the same flag, since it reads as ordinary defensive practice. hack/check-hook-request-timeout.bats fails any non-vendored template whose kubectl call carries a non-zero --request-timeout, skipping files that name KUBECONFIG because an explicit kubeconfig is not served by the in-cluster branch. That per-file judgement is the coarse part and is written down in the file rather than left to be inferred: apps/kubernetes keeps the flag on two calls that pass their own kubeconfig, and stays green. Bounding the work is still available where it belongs: --timeout for a watch, activeDeadlineSeconds for the Job, which is what this hook already had at 120s. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
e63b8595d3 |
fix(kafka): drop the request timeout that hid the apiserver from the hook
A non-zero --request-timeout makes kubectl skip its in-cluster credentials altogether. client-go consults them only while the assembled config still equals the default, and the flag lands in that config, so the branch that would load the ServiceAccount token never runs. kubectl falls back to its built-in default cluster, localhost:8080, is refused, and the script reads the refusal as a failed delete and exits 1. A non-zero pre-delete hook returns from Uninstall.Run before any resource is removed, so no Kafka release could finish deleting and helm-controller retried forever. Measured on the pinned image rather than argued from the docs: with the token and ca.crt mounted and KUBERNETES_SERVICE_HOST set, the same delete reaches the cluster without the flag, goes to localhost:8080 with --request-timeout=20s, goes to localhost:8080 with 1m, and reaches the cluster again with 0. The three in-tree hooks that work do not pass it. apps/kubernetes names it in a comment only, apps/harbor and apps/mariadb never mention it, and apps/tenant records that its DELETE calls "carry kubectl's default --request-timeout of 0". The watch stays bounded by --timeout, and the Job by activeDeadlineSeconds, which is that same shape. The suite now rejects the flag. It needs the "=" because the explanation in the script names the flag too, and matching the bare name would fail on the comment rather than on a use. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
64d5689434 |
fix(kafka): keep the pre-delete hook's credentials as long as its Job
The Job carries no hook-succeeded, so it outlives the hook run and ttlSecondsAfterFinished reclaims it, which is what leaves a readable record when the finalizer outlives the wait. Its ServiceAccount, Role and RoleBinding still carried hook-succeeded and hook-failed, so Helm reaped them at the end of the run either way and left the Job behind them. A Job whose ServiceAccount is gone still starts a pod. kubelet mounts the projected volume without a token, client-go finds no in-cluster config, kubectl falls back to localhost:8080, and the script takes its catch-all branch and exits 1. A non-zero pre-delete hook returns from Uninstall.Run before any resource goes, so the release never finishes deleting and helm-controller retries it: the e2e run on the merge commit shows six such pods in five minutes, in two namespaces, every one of them dead the same way. The hook meant to prevent one stuck Kafka release was instead stopping all of them. Give all four objects the same before-hook-creation policy, so the credentials live exactly as long as the consumer and the next run replaces them. Both other in-tree hooks that reach the apiserver, apps/kubernetes and apps/harbor, are symmetric this way already. The suite now pins the policy on each of the three, because nothing compared them: the asymmetry rendered cleanly, passed 37 helm-unittest cases and 5 bats cases, and only a cluster showed it. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
6818795588 |
fix(seaweedfs): raise the bucket ceiling by sizing volumes smaller (#4251)
## What this PR does Raises how many buckets a SeaweedFS instance can serve, by lowering the master's `volumeSizeLimitMB` from 1000 to 400 and its two-copy growth count from 6 to 3. The size limit is exposed on the app chart, where an operator can reach it. Existing instances stay at 1000 unless they already name a limit, so for them the upgrade changes only the growth count. A platform migration does that where migrations run; on the `default` variant it is a manual step before upgrading. A unit test pins each constant separately, and an e2e case creates several buckets and requires each to take a write. Late in a full e2e run every S3 `PUT` starts timing out and a backup suite fails its access preflight. It reads like a credentials or COSI problem and is not one: the instance has no volume slots left, so the master cannot grow a volume for a newly created bucket. The disk is still almost entirely free, which is what makes it look like a flake. The COSI objects say nothing either way, the claim is ready and the access is granted. ### The ceiling A volume server started with `-max 0` takes a slot count derived from disk rather than a fixed one, replication `001` spends a slot on each of the two servers per logical volume, and a collection claims `COPY_2` volumes each time it grows. So the ceiling is set by the product of the size limit and `COPY_2`, and moving either number alone moves it. The mechanism is upstream in `MaybeAdjustVolumeMax` (`weed/storage/store.go`) and `findVolumeCount` (`weed/topology/volume_growth.go`); what matters for choosing the numbers is only that upshot. Do not divide the slot figures out of the nominal PVC size, because what the server counts is the space the filesystem makes available and not the volume's advertised size. At the upstream 1000 and 6 a 10Gi volume PVC gives nine slots per server and two collections, which the platform's own `cozy-backups` bucket and a single tenant bucket already use. That same PVC was seen to give nineteen slots at 500, which puts 400 at twenty-three to twenty-four by the same division, or eight collections. The span is the observation's own: the free space behind nineteen at 500 fits more than one filesystem geometry, and those divide differently at 400. Eight either way, because a collection that cannot get a whole batch of `COPY_2` keeps the volumes it did get and writes into them, so the count rounds up rather than down. Every figure here is for the shipped `replicationFactor: 2`. The master picks which of its four growth counts to apply from the replication the filer asks for, and the filer takes that from `replicationFactor`, so at 1 or 3 copies it reads `COPY_1` or `COPY_3` and the `COPY_2` this PR sets does not apply. Lowering the size limit still raises the ceiling there, since it is the divisor whatever the replication is. I measured this instead of only deriving it, on the real `weed` 4.31 binary: one master, two volume servers with `-max 0`, each on its own 1 GB filesystem image, replication `001`, then one `assign` per fresh collection until the master refused. Scaled 10:1 against the 10Gi PVC, so the limits scale with it. | `volumeSizeLimitMB` | `COPY_2` | slots reported | collections served | | --- | --- | --- | --- | | 100 | 6 | 10 | 2 | | 100 | 3 | 10 | 4 | | 50 | 3 | 20 | 7 | | 40 | 3 | 25 | 9 | | 25 | 6 | 40 | 7 | The fourth row is the pair this PR ships. The refusal arrives as `No writable volumes and no free volumes left`, with `volumes == max` on both servers and the disk nearly empty. The reported `max` matched the space the filesystem made available over the limit in every run, which is the rule the PVC figures above are divided with. Those runs sit on a 1 GB image, so their counts are the rule's, not the PVC's: a 10Gi volume has its own filesystem geometry and divides to its own numbers. ### Why both constants move The last two rows are the argument: lowering the size limit and lowering the growth count reach the same ceiling. They do not cost the same. Open descriptors scale with stored bytes rather than with the PVC, so at 400 a 10 TiB server keeps roughly 52k of them against 21k upstream, where taking the whole reduction out of the size limit would have put it at 105k, well past a 65536 `nofile` limit where one is set. Two stay open per volume, a `.dat` and an `.idx`; the `.vif` beside them is written and closed. That cost lands hardest on large instances, which never had a ceiling problem in the first place: at any of these limits a multi-TiB PVC has slots to spare. Lowering the growth count costs no descriptors at all. It costs write spread, since a bucket then spreads its writes over three volumes rather than six, and that cost is flat, it does not grow with the instance. So the reduction is split across both. Neither number is a new bet: 3 is what upstream already uses for three-copy replication. One price is shared by both routes. A collection absorbs `volumeSizeLimitMB × COPY_2` before it needs another grow, so growth requests arrive five times as often as they did at 6000 MB. A collection also starts with half as many writable volumes at `replicationFactor: 2`, and a volume server takes the writes into each volume one at a time, so a heavily parallel writer spreads over fewer volumes until the collection grows again. That holds for instances kept at 1000 too. ### On upgrade On a variant that runs platform migrations, the upgrade does not change the limit on an existing instance unless its operator already set one. Platform migration 57 writes `master.volumeSizeLimitMB: 1000` into every existing SeaweedFS instance that has not chosen a limit, except one whose `master` is not an object (below), so 400 applies only to instances created after this release. The `default` variant runs no platform migrations. A cluster there that installed SeaweedFS by hand has to apply the same patch before upgrading, and the operations guide has the command. Both guide steps end with a `complete` line. `jq` prints it only after reading the whole list, and the pinning loop stops at the first failed patch before it, so a run without that line is incomplete, whatever else it printed. A chosen limit is a whole number from 1 to 30000 and is kept. Anything else counts as not chosen and becomes 1000: the previous chart ignored the key and ran 1000 in its place, and the new schema would refuse it. A kept value other than 1000 takes effect with the upgrade itself. A lower one can strand buckets and a higher one takes the room new buckets grow into, so the guide has a query that lists those instances before upgrading. An instance kept at 1000 still gains: at `replicationFactor: 2` a grow now claims three volumes instead of six, so it has room for about twice as many new buckets. Three shapes get their own line in the Job log, with what they hold. A non-object `spec.values` is pinned too. helm-controller v1.5.0 ignores values it cannot read as a map and renders the chart defaults, so that instance runs at 1000 today and would drop to 400. The merge patch replaces the unreadable value, and nothing that applies is lost. A non-object `master` is left alone. Every v1 schema already refuses it, so that release does not render today, and the log says to set the limit in the same edit that repairs it. A `valuesFrom` with entries besides the `cozystack-values` Secret is named and pinned anyway. helm-controller merges `spec.values` over those entries, so a limit set there does not apply, unless it is set through `targetPath`, which can still win depending on the entries before it. The guide steps print the same three. The value goes into the application's HelmRelease, whose `spec.values` is the application spec, so it covers an instance created through the API and one the tenant module renders alike, the platform's own in `tenant-root` included. The tenant module renders that release without `spec.values`, which a new tenant chart test pins, and a Helm upgrade leaves alone a key that neither the previous nor the new manifest carries: I measured that on a kind cluster with the real HelmRelease CRD and Helm 4.3.0, on client-side, server-side and forced server-side apply, and read `createPatch` at the v4.1.1 that helm-controller v1.5.0 runs. Where migrations run, the order comes from the platform release itself. The migration is its pre-upgrade hook, and the `cozystack-packages` source every application chart is built from is a manifest of that same release, applied only after the hooks finish, so no instance meets the new chart unstamped and a failed migration stops the platform upgrade before that source moves. That holds while the platform source is pinned by digest, as it is out of the box; a source that follows a tag, a semver range or a branch lets source-controller move `cozystack-packages` on its own schedule, and an instance there can run at 400 until the migration has run. Until `cozystack-packages` moves, each patched release still runs its old chart, which ignores the key: no v1 release reads it, and none closes the `master` object in its values schema. The migration's test runs the script under the migrations image's shell against a fake `kubectl` that refuses any selector but the one cozystack-api lists SeaweedFS with, because an empty fleet is a legitimate answer and a mistyped selector would otherwise pass while pinning nothing. The same test runs the two guide steps, found by a marker comment above each, against the same fake and checks that they select the same releases as the migration. Lowering an existing instance is left to its operator, because it is not free. Its volumes between the new limit and the old go oversized on the next heartbeat and leave the master's writable set, and each affected collection needs one successful grow before its next write lands. Nothing is lost and the volumes stay readable. Those grows compete for one pool, and the budget is per collection rather than per server. An oversized volume is charged no headroom while one below the limit is charged all of its unfilled room, so the slots open to a stranded collection are free space less that room, divided by the limit; a grow then claims a whole `COPY_2` batch and takes as much of it as is there, and the volumes it creates are empty and charge their headroom straight back. Measured on the rig: two oversized collections with two free slots between them, and the first to write took both, leaving the second refused afterwards with the disk still reporting a quarter of itself free. Collections that keep writing are not free either: they were grown six at a time, and each of their volumes below 400 MB can hold back a whole 400 MB. Measured at a scaled pair, a server with free space for one batch per collection refused the only grow it needed, because a second, nearly empty collection held the room, and the same free space without that collection let the grow through. The master also grows, on its own schedule and with no write, any collection with no writable volume at or below 90 percent of the limit, which at 400 is 360 MB; measured at the same scaled pair, a collection with every volume in that band got two new volumes per server within one pass while a roomy one was left alone. So the precondition on every volume server is a batch for each collection with no volume at or below that line plus 400 MB for each volume still below the limit, and no volume size exempts an instance from it, since a larger volume carries more buckets and the budget scales with them. The `minFreeSpacePercent` reserve only comes into it on a server whose writable volumes have all passed the limit, since nothing there is charged headroom. It is five percent of what the filesystem reports rather than of the PVC's nominal size, so `All >= 8000 MiB` is where it reaches one 400 MB limit; a 10Gi PVC measures 9979 MiB and clears that, and such a server then refuses a grow only once its disk is down to the reserve. Everywhere else the headroom charge decides, and a refusal can come with the disk far from full whatever the reserve. Deleting a collection gives back a slot for each of its volumes, however little it stored, since the headroom charged to them leaves with them; only a server already at its reserve, where every volume is read-only and charged nothing, gets back no more than the bytes. Deleted objects return room only through a vacuum. The room left inside those volumes for further writes is given up and does not return as capacity, since such a volume is charged no headroom while its own bytes still come out of the budget. Lowering the growth count takes nothing away by contrast: no path removes a volume from the writable set for being surplus to the batch size, so an existing collection keeps what it has and converges as those volumes fill. [`docs/operations/seaweedfs-lowering-the-volume-size-limit.md`](https://github.com/cozystack/cozystack/blob/main/docs/operations/seaweedfs-lowering-the-volume-size-limit.md) has the check for it, and the way back. The ceiling cannot come out lower at any supported `replicationFactor`, which is the part worth stating plainly. The divisor halves, and the space the slot count is derived from grows at the same time, because a volume past the new limit stops being charged headroom against it. `COPY_1` and `COPY_3` are untouched, so at 1 and 3 copies the limit alone still moves the ceiling upward. ### Reaching the value Neither constant was reachable by an operator. The data-plane chart is pulled by `ExternalArtifact`, and the component publishing it declares no `install` section, so the reconciler drops a `Package`-level values override for it before any merge happens. Editing the chart was the only route, on a value whose right setting depends on the PVC size the operator chose. `master.volumeSizeLimitMB` is on the app chart now, with a schema entry. Worth being precise about which file the two numbers end up living in, because exposing one of them moves it. The app chart's HelmRelease sends the size limit on every release, so `packages/extra/seaweedfs/values.yaml` holds the 400 installs run with, and `packages/system/seaweedfs/values.yaml` keeps upstream's 1000 as the fallback a render without it gets. That fallback is deliberate: a platform upgrade can re-render the data-plane release from its artifact before the parent has sent anything new, and a lower value there would then reach the master first and drop the limit ahead of an operator's own setting. Nothing sends the growth count, so the data-plane chart's value is the live one for that half, and it is the part that does land in that window, where a grow costs 3000 MB against the 6000 MB it cost before. The budget the pair produces is written out next to the growth count, with a pointer from the size limit to it, so the two do not drift apart. The field is bounded at both ends, because either end takes an instance down. Zero is not a smaller limit but an off switch: the volume server leaves its slot calculation untouched at zero, keeps reporting no capacity, and nothing is ever grown. Past 30000 the master does not start at all, exiting on `volumeSizeLimitMB should be smaller than 30000`. The bound lives in the values schema, through the generator's `@minimum` and `@maximum` annotations, so Helm refuses the value before any template runs. The apps API reads that schema only for defaults, so it accepts a value out of range, and the release then fails to render. A case pins each edge along with the value just outside it. An explicit null gets past both bounds, because Helm drops a null key before it validates, and would reach the master as an empty `-volumeSizeLimitMB=` that the shipped image exits on at start, so the template requires the value and a case pins that as well. There is no second bound against the disk, and not for want of trying: a limit at or above the volume's own size does not break an instance. `MaybeAdjustVolumeMax` floors the slot count at one, and a 255 MiB disk against a 1000 MB limit reported `Max 1`, took an assign for a fresh collection and served back what was written to it. Such a server is capped at a single slot rather than dead, so a render refusal would stop a working release on every reconcile. The field description says instead that a value close to or above `volume.size` leaves each volume server a single slot. The growth count is deliberately not exposed beside the limit. A field wired to `COPY_2` would do nothing at 1 or 3 copies, for the reason above, while still looking like a setting, and a knob that is silently inert for two of the three supported values is worse than no knob. Making it track the replication factor is a feature with its own matrix, and it would rest on a path where the master's own default replication is separately broken. The size limit has no such dependency, so it is the one that ships. ### Picking a value 400 is sized for the shipped `volume.size: 10Gi`, which is where the slot and collection figures above come from. Going up, it holds until the two descriptors per volume meet a 65536 `nofile` limit, roughly 12.5 TiB per volume server. Going down, an instance needs 1600 MB of filesystem per volume server to serve a second collection at all, since the first claims the opening three slots. ### What the tests cover The existing bucket case creates one bucket, which sits inside the ceiling at any sizing, so it could not tell a working ceiling from an exhausted one. The new e2e case creates three beside the platform's own backup bucket and requires each to accept an object, which is the only signal the exhaustion produces. Each `mc` call there carries its own timeout, since a hung PUT is exactly that signal and the step timeout alone would not end it. It was observed red at the old sizing in [run 34948279363](https://github.com/cozystack/cozystack/actions/runs/34948279363), which ran the case against the unchanged constants: the pre-existing single-bucket case passed in 18s beside it, and the new one failed on the write to the second bucket after the first succeeded. Three rather than two so the count clears the ceiling without depending on how many slots the unnamed default collection holds, which the suite cannot observe. The unit tests pin each constant on its own, because losing one is a silent capacity regression rather than something a render would catch. The app-chart test asserts an overridden size limit as well as the default, since asserting the default alone passes just as well against a template that ignores the value and writes the number as a literal. It also pins both ends of the accepted range and the first value outside each, the `-max 0` zero on all four render paths it is written to, and the 10Gi volume default the slot figures above are measured for, which nothing else guarded. The data-plane suite pins the five percent free-space reserve as well, since the 8000 MiB threshold above is computed from it and nothing else reads it. The capacity case also reads the master's slot accounting in its `catch`. A grow failure in the master log does not say whether the shipped limit is too coarse for the PVC or an earlier collection in the same run is still holding the budget, and the per-server `Max` against `Volumes` does. Closes #4239, closes #4245, closes #4248 ### Screenshots No UI change. ### Downstream repositories Walked against the diff. The new field lands on `packages/extra/seaweedfs`, and the provider models bucket, generic, qdrant, redis and tenant, with no SeaweedFS resource among them, so there is no schema or expand/flatten pair over there for it to drift from. No package is added, renamed or removed, so the website app lists 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: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(seaweedfs): raise how many buckets a new SeaweedFS instance can serve on the default 10Gi volume PVC from two to eight at `replicationFactor: 2`, by defaulting the master volume size limit to 400 MB instead of 1000 and lowering the two-copy growth count from 6 to 3, and expose the limit as `master.volumeSizeLimitMB` on the app chart. **Action required on the `default` variant only:** it runs no platform migrations, so before upgrading, set `master.volumeSizeLimitMB: 1000` on every SeaweedFS instance that has no valid value and whose `master` is an object, with the step in [`docs/operations/seaweedfs-lowering-the-volume-size-limit.md`](https://github.com/cozystack/cozystack/blob/main/docs/operations/seaweedfs-lowering-the-volume-size-limit.md). If this is skipped, every such instance drops to 400 on the upgrade, and a bucket whose volumes have all reached 400 MB takes no writes until it can grow. On the variants that run platform migrations, migration 57 does this: it sets `master.volumeSizeLimitMB: 1000` on every instance that has not chosen a value, except one whose `master` is not an object, which no v1 chart renders and which it names in its Job's log instead, so nothing changes for an instance unless it already names a different value, which the previous chart accepted and ignored and this one applies; the same guide lists such instances before the upgrade, and the migration names them in its Job's log. An existing value that is not a whole number from 1 to 30000 is replaced with 1000, which is what the previous chart applied in its place. Instances at 1000 still get roughly twice the room for new buckets from the lower growth count. The upgrade rolls every SeaweedFS master StatefulSet once, since the growth count is part of the master's environment, and an instance that names a value other than 1000 rolls a second time when that value arrives. The pin lives in the instance's spec, so a tool that replaces the whole resource from a manifest without `master.volumeSizeLimitMB` removes it and the instance takes 400; add the field to such manifests before upgrading. Lowering the limit on an instance that already holds data takes every volume at or above the new value out of the master's writable set, and raising it takes the room new buckets grow into, so the same guide describes the free-space check to run first and how to set it back. ``` |
||
|
IvanHunters
|
c983390b39 |
ci(e2e): surface overlay drift and remove both causes of it (#4261)
## What this PR does Six commits against one script: one reports a silent degradation, three remove causes of it, two repair the tests and the reasons recorded beside them. **Why it matters.** A file whose config differs from the current-main artifact beyond its image lines keeps its committed ref. That is the right safe answer, and it is not a neutral one: the component then runs its last RELEASE image in a lane whose job is to exercise the tree under review, and the only trace is one line among hundreds. Seven packages took that path on the run behind #4257. One of them, `backupstrategy-controller`, served an image from v1.6.0 (22 July) against a tree carrying four backup strategy drivers added after it, so five of that run's seven chainsaw failures came from there, and three unrelated pull requests each spent a review round establishing that the red lane was not theirs. After this PR that list is empty. Against the public artifact at its own revision, with the tree checked out at that same revision: ```text before drift=5 overlaid=29 core/platform, cozystack-api, dashboard, kubeovn-webhook, linstor-gui after drift=0 overlaid=34 ``` **Naming the drifted packages.** One GitHub annotation after the existing tally. Which files get overlaid does not change, and a run with no drift emits nothing. **Cause one: a suffixed image key.** The classifier accepted a key only when it was exactly `image`, `repository`, `registry`, `tag` or `digest`. A chart rendering several images names the extras after what they are for, so `backupstrategy-controller` carries `chBackupClientImage`, `rabbitmqBackupClientImage` and `redisBackupClientImage`. The first two passed only through the `@sha256:` branch; the third is pinned by tag alone, so it read as a config change and cost the whole file its overlay. The fix accepts a prefix before `image`, and two things keep it narrow. A letter is required before `image`, which a key beginning with it has nothing to supply, so `imagePullPolicy` and `imagePullSecrets` stay config; the trailing colon is not what does that, and they match neither with it nor without. The value must also hold a repository path, which keeps out operator-settable knobs named after an image: `vddkImage: ""` in `core/platform` and `migration-controller` is configuration the build never stamps, and absorbing it would be the opposite of what this script is for. **Cause two: a blank line the build removes itself.** `yq -i` re-emits the document when it stamps a digest, and a blank line separating two top-level keys does not come back. So a file the PR never touched differs from the artifact on a line carrying no configuration at all, and loses its overlay for it: ```text $ diff repo/system/cozystack-api/values.yaml artifact/system/cozystack-api/values.yaml 4d3 < 6c5 < image: ...cozystack-api:v1.6.0@sha256:c4d374a9... --- > image: ...cozystack-api:main@sha256:2fb87cf8... ``` Running the stamping assignment over the committed file reproduces that diff exactly, which is the second signal behind the diagnosis rather than the first one twice. Blank lines are now discounted before the test; a line with content still counts, so a genuine config difference keeps the committed ref as before. This accounted for all five remaining packages, and it retires the explanation offered in #4265, which ruled `yq` reformatting out. That was wrong: the round-trip check behind it did not cover the blank line. `cozystack-api` and `dashboard` are not peripheral here. An e2e result about either was a result about the last release rather than about `main`. **Repairing the tests.** `hack/bats-no-exit-trap.bats` audits every `.bats` file under `hack` against the debt its own header declares, and this file was installing three traps beyond it. An EXIT trap inside an `@test` body replaces the one the bats binary installs, so a failing test prints no TAP line at all and the suite reads as green. The answer is to stop installing the handler rather than to raise the number, so those tests clean up at the end of the body instead. ### Tests Eight cases in `hack/overlay-main-images_test.bats`, each held by a mutation of the line it describes: - drift is summarised with the package names and the count; suppressing the annotation or dropping the names reddens the suite - a clean run emits no annotation - a suffixed image key pinned by tag alone overlays instead of drifting; reverting the classifier reddens it - a key merely STARTING with `image` is still config - an `*Image` key whose value is operator configuration rather than a path still drifts, so `vddkImage` is not absorbed - two drifted files in one package count as one package, and the name carries no `/images` suffix - a blank line lost to `yq` is not a config change; removing the filter reddens it - a real config line alongside a blank line still counts as drift; widening the filter to drop every line reddens this one and two others The suite's plan and its `ok` count agree at 21, which is the reconciliation the trap audit exists to protect. Green under the `bats` binary and under `hack/cozytest.sh`, which is what `make bats-unit-tests` runs and the only one of the two whose result decides the gate. ### What this does not close `redisBackupClientImage` is still pinned by tag with no digest, and the tag it names does not exist: ```text $ curl .../cozystack/redis-backup/tags/list {"tags":["nightly","0.0.0-nightly.20260911...","0.0.0-nightly.20260912...", ...]} ``` Only nightly tags are published, so `redis-backup:v1.6.0` resolves to nothing. Pinning it needs a published release tag first, which is #4264 rather than this PR. Making the overlay line-wise, so image lines transfer even when other lines genuinely differ, would go further still. It trades away the guarantee the script is built on, that a file comes wholly from one source, and that guarantee is deliberate and commented: a maintainer decision rather than a quiet change from a reviewer. ### Screenshots Not a UI change. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * GitHub Actions now warns when image overlay processing detects configuration drift. * Warnings list affected package names in sorted, de-duplicated form. * Image references with alphabetic prefixes are recognized during overlay processing. * Blank-line-only differences no longer trigger drift warnings. * **Tests** * Added coverage for image references, similarly named configuration keys, blank-line differences, and multi-package drift reporting. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Timur Tukaev
|
69c4fadf8a |
docs(security): document Security Champion rotation and response team (#4223)
## What this PR does Surfaces the **Security Champion rotation policy** and the **security response team's composition and diversity** into the public `SECURITY.md`. Until now these were defined only in the CVE-scanning pipeline's private governance document, which the General Technical Review flagged as a gap (the policy existed but was unpublished, and the rotating role was unassigned). Specifically: - Adds a **"Security response team and rotation"** subsection: names the response-team maintainers, notes the team spans more than one organization (Ænix, HIDORA) and more than one country, and documents the rotating **Security Champion** role — its responsibilities, a **6-month rotation** among maintainers, and a **one-week handoff overlap**. - Records **@tym83** as the current Security Champion, filling the role this file previously described as unfilled. - Updates the Trivy paragraph accordingly (the triage-clock owner is no longer "currently unfilled"). This is a policy change to a `CODEOWNERS`-protected file and is intended for maintainer (CODEOWNER) review rather than a wording check. ### Downstream repositories - [x] No downstream repository is affected by this change ### Release note ```release-note docs(security): document the Security Champion rotation policy and security response team in SECURITY.md ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added information about the security response team and its rotating Security Champion role. * Clarified ownership of security triage timing and response coordination. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
a2db2aeb8c |
fix(seaweedfs): raise the number of buckets a volume server can serve
A volume server started with -max 0 takes a slot count derived from disk rather than a fixed one, replication 001 spends a slot on each of the two servers per logical volume, and a collection claims COPY_2 volumes whenever it grows. So the bucket ceiling is set by the product of the size limit and COPY_2, and moving either number alone moves it. The mechanism is upstream in MaybeAdjustVolumeMax (weed/storage/store.go) and findVolumeCount (weed/topology/volume_growth.go). Do not divide these out of the nominal PVC size, because the count follows the space the filesystem makes available and not the volume's advertised size: the shipped 10Gi volume PVC was seen to give nine slots per server at the upstream 1000 and 19 at 500, which puts 400 at 23 to 24 by the same division, or eight collections against two. The span is the observation's own, since the free space behind 19 at 500 fits more than one filesystem geometry and those divide differently at 400; eight either way, because a collection that cannot get a whole batch of COPY_2 keeps the volumes it did get and writes into them. The division itself is measured: on a 1 GB image the master reports max as the filesystem's available space over the limit in every run, 25 slots and nine collections at the 40 and 3 that scale to this pair. Two is what the platform's own cozy-backups bucket and a single tenant bucket already use. Past that every S3 PUT to a new bucket times out while the disk still reads as nearly empty, and no COSI object reports anything wrong, because the claim and the access are both granted. 400 and 3 make the budget 1200 MB per collection. Splitting the reduction across both numbers rather than taking it out of the size limit alone keeps the cost off the curve that grows: open descriptors track stored bytes divided by the limit, so a 10 TiB server keeps roughly 52k of them at this limit against 21k upstream, where moving the size limit alone would have put it at 105k. A .dat and an .idx stay open per volume; the .vif beside them is written and closed. The ceiling reached is the same either way. Both figures describe the shipped replicationFactor of 2. The master picks its growth count from the replication the filer asks for, so at 1 and 3 copies it reads COPY_1 and COPY_3 instead and this COPY_2 does not apply; lowering the size limit still raises the ceiling there. The size limit was out of an operator's reach: it lived in the data-plane chart, which is pulled by ExternalArtifact from a component that declares no install section, so a Package-level values override for it is dropped before any merge. The app chart now declares master.volumeSizeLimitMB and sends it on every release, and 400 is set there. The data-plane chart keeps upstream's 1000 as its fallback on purpose. A platform upgrade can re-render that release from its artifact before the parent has sent anything new, and a lower fallback would then reach the master first and drop the limit ahead of an operator's own setting. COPY_2 is sent by nothing and lives in the data-plane chart, so it does land in that window, where a grow costs 3000 MB against the 6000 MB it cost before. The app chart also writes maxVolumes into dataDirs on every release, and because dataDirs is a list Helm replaces the data-plane chart's copy wholesale, so the zero that makes the slot count auto-derived at all comes from there. Losing it drops the server to the subchart's fixed seven without failing any render. The field is bounded at both ends, because either end takes an instance down. Zero is not a smaller limit but an off switch: the volume server leaves its slot calculation untouched at zero, keeps reporting no capacity, and nothing is ever grown. Past 30000 the master does not start at all. The bound sits in the values schema rather than in a template guard, so Helm refuses the value before any template runs. The apps API does not: it reads that schema only for defaults, so it accepts a value out of range, and the release then fails to render. An explicit null gets past both bounds, because Helm drops the key before it validates, and would render an empty -volumeSizeLimitMB= that the master exits on at start, measured on the shipped image, so the template requires the value. No bound against the disk goes with it, though the field invites one. A limit at or above the volume's own size does not break an instance: MaybeAdjustVolumeMax floors the slot count at one, measured on 4.31 with a 255 MiB disk against a 1000 MB limit, which reported Max 1 and served a write it then read back. Such a server is capped at a single slot rather than dead, so refusing it at render would be a capacity policy that stops a working release on every reconcile, not only on the upgrade. The field's description carries the relation instead, where the value is chosen. The growth count is deliberately not exposed beside the limit. The master picks which of its four growth counts to apply from the replication the filer requests, and the filer takes that from replicationFactor, so a field wired to COPY_2 would do nothing at 1 or 3 copies while still appearing to be a setting. A knob that is silently inert for two of the three supported values is worse than no knob. Two further prices. A collection absorbs the budget before it needs another grow, so growth requests arrive five times as often as at the upstream pair. A collection also starts with half as many writable volumes, and a volume server takes the writes into each volume one at a time (syncWrite holds that volume's dataFileAccessLock), so a heavily parallel writer spreads over fewer volumes until the collection grows again; that holds for instances kept at 1000 too. And lowering the limit on an instance that already holds data carries the other one: its volumes between the new limit and the old go oversized on the next heartbeat and leave the master's writable set, and each affected collection needs one successful grow before its next write lands. Nothing is lost and the volumes stay readable; the room left inside them for further writes is given up and does not return as capacity. Those grows compete for one pool, and the pool is smaller than it looks. An oversized volume is charged no headroom, while one below the limit is charged all of its unfilled room; a grow then claims a whole COPY_2 batch and takes as much of it as is there, and the volumes it creates are empty and charge their headroom straight back. Measured on a local SeaweedFS 4.31 setup: two oversized collections with two free slots between them, the first to write took both, and the second stayed refused with the disk still reporting a quarter of itself free. Collections that need no grow are not free either, since their volumes below the new limit were created six at a time and each can hold back a whole limit. Also measured there, with the limit and batch scaled down: a server with free space for one batch per collection refused the grow its one affected collection needed, because a second, nearly empty collection held the room, and the same free space without it let the grow through. The minFreeSpacePercent reserve only comes into it on a server whose writable volumes have all passed the limit, where nothing is charged headroom: from 8000 MiB of filesystem upward, a 10Gi PVC measuring 9979, five percent is at least 400 MB, and such a server then refuses a grow only once its disk is down to the reserve. Everywhere else the headroom charge decides, and a refusal can arrive with the disk far from full whatever the reserve. Deleting a collection gives the room back a slot per volume, however little it stored, since the headroom charged to those volumes leaves with them. So, on a variant that runs platform migrations, the upgrade does not change the limit on an existing instance unless its operator already set one, which the previous chart accepted and ignored. Platform migration 57 writes master.volumeSizeLimitMB: 1000 into every existing SeaweedFS instance that has not chosen a limit, and 400 applies only to instances created from here on. It leaves alone only an instance whose master is not an object, which no v1 chart renders, and names that one in its log. The default variant runs no platform migrations, so a cluster there that installed SeaweedFS by hand gets no pin, and the document below gives the same patch to apply before upgrading. A chosen limit is a whole number the new chart accepts; anything else counts as not chosen and is replaced with 1000, which is what the previous chart ran in its place, since it ignored the key and the new chart would refuse to render it. A valid value already set is kept, and since the previous chart ignored it, the upgrade is when it starts to apply; the document below lists the ones that change the limit ahead of it. Raised values are listed as well: existing buckets keep writing, but every volume below the new limit is charged up to it, and on a local SeaweedFS 4.31 setup seven nearly empty volumes left no free slot once the limit went up tenfold, so a new bucket was refused. The value goes into the application's HelmRelease, whose spec.values is the application spec, for an instance created through the API and for one the tenant module renders alike. The tenant module never renders spec.values on that release, and a Helm upgrade leaves a key alone that neither manifest carries: measured with Helm 4.3.0 on client-side, server-side and forced server-side apply, and read in createPatch at the v4.1.1 that helm-controller v1.5.0 runs. Where the migration runs, it is a pre-upgrade hook of the platform release, and the cozystack-packages source every application chart comes from is a manifest of that same release, applied only after its hooks; so no instance meets the new chart unstamped, and a failed migration stops the platform upgrade before that source moves. That holds while the platform source is pinned by digest, as it is out of the box; one that follows a tag, a semver range or a branch lets source-controller move cozystack-packages on its own schedule. Until cozystack-packages moves, each patched release keeps its old chart, which ignores the key, since no v1 release reads it or closes the master object in its schema. An instance kept at 1000 still gains, because at a replication factor of 2 a grow now claims three volumes rather than six, so it has room for roughly twice as many new buckets. Lowering an existing instance is left to its operator, and docs/operations/seaweedfs-lowering-the-volume-size-limit.md is the check for it. It lists each server's volume files and adds one limit for every volume below the limit and a batch for every collection with no volume at or below 90 percent of it, since the master grows such a collection on its own schedule without a write; measured the same way, a collection with every volume in that band and no writes got two new volumes per server within one pass. Where each disk type is served by as many volume servers as the replication factor, as the shipped two at a factor of 2 are, each collection's volumes sit on every server of that type, and that bounds the claim from above without assuming how the collections grew. The master places a new volume on any server of the requested disk type with room rather than where its collection already lives, whichever group or pool that server belongs to, and it reads an empty disk type as hdd; under MultiZone it also grows each zone on its own. So the document keeps zoned instances, and any disk type served by more servers than copies, at 1000. BREAKING CHANGE: on the default variant, which runs no platform migrations, set master.volumeSizeLimitMB to 1000 before upgrading on every SeaweedFS instance that has no valid value and whose master is an object, with the step in docs/operations/seaweedfs-lowering-the-volume-size-limit.md. On the variants that run platform migrations, migration 57 does this. If the step is skipped, every such instance drops to 400 on the upgrade: each of its volumes at or above 400 MB leaves the master's writable set, and a bucket whose volumes have all crossed that line takes no writes until it can grow. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
c038cf25d3 |
test(seaweedfs): cover the bucket count a volume server can serve
A volume server started with -max 0 derives the slots it has left from the free space on its disk, less the room its unfilled volumes still claim, divided by the master's volume size limit, and each collection claims a batch of those slots on every volume server the replication spans. The number of buckets an instance can hold therefore has a ceiling well below what the free bytes suggest, and crossing it leaves every COSI object reporting ready while the S3 PUT times out. The existing case creates one bucket, which sits inside that ceiling at any sizing, so nothing here told a working ceiling from an exhausted one. This case creates three beside the platform's own backup bucket and requires each to accept an object, which is the only signal the exhaustion produces. Three rather than two because the count has to clear the ceiling whether or not cozy-backups and the unnamed default collection have grown yet, which this suite cannot observe. BucketAccess reconciles separately from the claim it belongs to, and the write step reads the Secret that access produces, so readiness of the release and the claim is not enough to enter it. Without the access assertions the step can fail on a missing Secret, which looks the same in the report as the capacity failure it is here to measure. The catch reads the master's slot accounting as well as its log. A grow failure in the log does not say whether the shipped limit is too coarse for the PVC or an earlier collection in the same run still holds the budget, and the per-server Max against Volumes does. The read runs inside the master, whose image ships curl, so it needs nothing on the host and adds no second fixed port to the reason this suite cannot run in parallel. That reason now covers two cases rather than one, which the shared config says. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Timur Tukaev
|
2192547f1e |
docs(governance): remove the unused Directors role
GOVERNANCE.md defined a Directors role (non-technical leaders, elected by the maintainers, one vote in decisions), but no Directors were ever identified or elected. The role was intended for non-technical leadership, but those people ended up maintaining parts of the project directly, so it stayed unused. Remove it so the documented decision-making body matches actual leadership (maintainers, by consensus with a simple-majority fallback); it can be reintroduced if a real need arises. Addresses cozystack/community#79. Signed-off-by: Timur Tukaev <6355522@gmail.com> |
||
|
IvanHunters
|
b99db147a7 |
test(ci): isolate the commit-trailer fixture from global tag signing (#4271)
## What this PR does `make_repo` in `hack/check-commit-trailers.bats` silences `commit.gpgsign` in the fixture repository but leaves `tag.gpgsign` alone. Anyone who signs tags has that setting globally, it reaches the fixture, `git tag start` becomes an annotated tag, and git refuses it for want of a message. The helper opens all 27 tests, so the whole suite dies before a single test reaches its body, and `make bats-unit-tests` takes `make unit-tests` down with it. Details and the reproduction are in #4270. The first commit is that one line, next to the one that is already there. With it the suite went 27/27 on a machine where the global setting is set, and was unchanged on one where it is not. I ran it both ways. CI never saw this. The `Commit trailers` job runs the checker against explicit SHAs and does not touch the bats file, and a fresh runner has no global git configuration to leak in. The failure only lands on the local `make unit-tests` that `docs/agents/contributing.md` asks for before pushing, which is the worst place for it: an unexplained red in a file the contributor did not touch. That is also how the guard went missing in the first place, and it would go missing again the same way, so the second commit adds `make_repo holds against a hostile global configuration`. None of the three `git config` lines in `make_repo` is exercised on a runner: delete any of them and CI stays green. The test supplies the hostile configuration itself rather than waiting for a contributor to have one, so each guard is held by the suite. Pulling them one at a time gives three distinct reds: `tag.gpgsign` gives `fatal: no tag message?`, `commit.gpgsign` gives `gpg failed to sign the data`, and `core.hooksPath` lets a global `commit-msg` hook rewrite the message and fails the assertion on it. The suite is 28/28 both with the global settings and without, so a runner sees no change. `hack/release-changelog-behaviour.bats` has gaps of the same kind in its own fixture, which sets `commit.gpgsign` and nothing else. The missing `tag.gpgsign` is inert there because it creates no tag. The missing `core.hooksPath` is not: a global `pre-commit` hook that exits non-zero kills every `git commit` it makes. I left both alone rather than widen this diff, and noted them in the issue. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map against the diff. The only `hack/`-related trigger is `cozystack/ccp`, and it fires on moving or renaming something under `hack/` or changing what a make target does. This adds a line inside an existing file; the filename and the make target behave as before, except that the target stops failing. - [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 NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Repository fixtures now remain reliable despite conflicting global Git settings, including commit and tag signing, failing signing tools, and commit-message hooks. - Fixture setup preserves lightweight tags and the original seed commit message, ensuring consistent repository initialization across environments. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
IvanHunters
|
a19fd0274f |
fix(kafka): delete release topics before the topic operator is removed (#3938)
## What this PR does Fixes Kafka application deletion when a release-owned KafkaTopic still has Strimzi's `strimzi.io/topic-operator` finalizer. Without ordering, Helm removes the topic operator with the release, the KafkaTopic cannot finish deleting, and reinstalling the same release name times out on the leftover object (#3793). A namespace-scoped pre-delete hook deletes only KafkaTopics carrying the current release label while the operator is still running. It never removes finalizers directly. The hook uses a digest-pinned kubectl image, restricted RBAC, a 270-second job deadline, a 180-second deletion timeout, and a 20-second request timeout. The E2E absence checks allow five minutes, matching the hook's Helm budget. A wait that times out with the finalizer still held exits 0 on purpose, so the uninstall can finish rather than wedging the release under helm-controller's retry loop. That is a deliberate trade and the hook says so on stderr; the Job survives an hour under `ttlSecondsAfterFinished` so the record outlives the run. Validation: - `helm unittest packages/apps/kafka`: 37/37 passed, covering the hook resources, RBAC wiring, the digest pin and the `_cluster.images-registry` override, the release-scoped selector, and the Job deadline and delete-policy settings. - `hack/cozytest.sh hack/kafka-pre-delete-hook.bats`: 5/5 passed. The suite renders the hook and runs the script against a fake kubectl, because every property worth pinning there is an exit code and matching manifest text cannot establish one. Run under the runner `make bats-unit-tests` uses rather than the `bats` binary, which do not accept the same test file. - Both suites were mutation-checked: putting the timeout branch back to `exit 1`, flipping the catch-all to `exit 0`, dropping `--ignore-not-found`, inverting the guard, widening the selector to a sibling release, and setting `--wait=false` each turn one of them red. - The raw template parses with `yq`; all leading Helm-expression scalars are quoted. - Chainsaw asserts both release topics are absent after deletion, but the full E2E suite was not run locally because it requires a disposable cluster. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map against the chart hook and E2E test. No values/schema/API/default or downstream hardcoded contract changes. - [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): delete release-owned KafkaTopics before removing the Strimzi topic operator ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Kafka deletion now automatically removes associated Kafka topics before the Kafka resource is removed. - Cleanup waits for topics to disappear before checking ZooKeeper storage reclamation. - Deletion remains resilient when topic resources or their definitions are absent, or when finalizers delay removal. - Cleanup failures are reported appropriately without blocking deletion when a timeout occurs. - **Tests** - Added coverage for cleanup behavior, execution settings, permissions, and failure scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
IvanHunters
|
379c763fc3 |
test(ci): drop an inert key from the fixture's hostile configuration
tag.forceSignAnnotated governs annotated tags and `git tag <name>` creates none, so with tag.gpgsign unset the tag still resolves to a commit object. The key cannot make the new test fail and guards nothing the fixture could lose; carrying it implied a fourth guard that does not exist. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
ef3bfd71d3 |
docs(ci): record what actually narrows the overlay key branch
Two comments credited the trailing colon with keeping imagePullPolicy and imagePullSecrets out of the key branch. It does not: the branch requires a letter before image, which a key beginning with image has nothing to supply, so neither matches with the colon or without it, even when the value carries a slash. The colon earns its place elsewhere, as the point the value scan starts from, and removing it reddens the suffixed-key test rather than the pull-policy one. A reader acts on the recorded reason, so a wrong one costs more than none. Refer to that test by name rather than by ordinal: the number comes from the runner's output order and moves when anything is inserted above it. While here, give the pull-policy test a failure message about pull policies: it read 'a PR-edited package was overlaid', which belongs to a different case. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
609a014cc4 |
fix(ci): make the clean-run overlay test able to fail, and narrow the key branch
`! grep -q …` is a pipeline preceded by `!`, which POSIX exempts from set -e, and hack/cozytest.sh appends `return 0` to every converted test function, so a failed negation was discarded twice over. That line was the only assertion in 'no annotation when nothing drifted': mutating the drift>0 guard to `true` left the suite green under both runners. Replace all three negations with an explicit if/exit; the mutation now reddens test 19. The prefixed *Image branch matched any key ending in image, including operator-settable ones the build never stamps: vddkImage (core/platform, migration-controller) defaults to "" and a change there was absorbed from the artifact in silence, the opposite of what this script is for. Require the value to hold a repository path, which a ref always does and "" does not. The annotation counted ref-bearing files while naming deduplicated dirnames, so one package with a drifted values.yaml and a drifted images/*.tag read as two, and named <pkg>/images, which is not a package. Count the named list instead. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
e9f666bc25 |
fix(ci): stop a blank line lost to yq from costing a package its overlay
The overlay keeps a package on its committed release ref when the artifact copy
differs from the repo on any line that is not image-reference-bearing. A blank
line is such a line, and the build produces one on its own: yq -i re-emits the
document when it stamps a digest, and a blank line separating two top-level keys
does not come back.
So a file the PR never touched differs from the artifact on a line carrying no
configuration, and loses its overlay for it. That was the whole of the remaining
drift list. Against the public nightly artifact at its own revision, with the
tree checked out at that same revision:
before drift=5 overlaid=29 core/platform, cozystack-api, dashboard,
kubeovn-webhook, linstor-gui
after drift=0 overlaid=34
Each of the five differed from the artifact by one blank line and its own ref,
nothing else, which is reproducible locally by running the stamping assignment
over the committed file. cozystack-api and dashboard are not peripheral: an e2e
result about either was a result about the last release rather than about main.
Discount blank lines before the test. A line with content still counts, so a
real config difference keeps the committed ref as before.
Assisted-by: LLM
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
|
||
|
IvanHunters
|
80a41e4e6f |
test(ci): stop the overlay tests installing EXIT traps, and pin the key branch
hack/bats-no-exit-trap.bats audits every .bats file under hack against the debt its own header declares, and this file was installing three traps beyond it. An EXIT trap inside an @test body replaces the one the bats binary installs for its own bookkeeping, so a test that then fails prints no TAP line at all and the suite reads as green. That is the reason the count is exact rather than a ceiling, and the reason the answer here is to stop installing the handler rather than to raise the number. Clean up at the end of the body instead: both runners set -e, so on failure the cleanup is unreachable and the scratch directory survives for inspection. The key branch had nothing holding its shape. A test now pins each half: that a suffixed key such as clientImage is read as a ref, and that a key merely beginning with image is not. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
8ac99b67cc |
fix(ci): read a suffixed image key as a ref, not a config change
The classifier accepted a key only when it was exactly image, repository, registry, tag or digest. A chart that renders several images names the extras after what they are for, so backupstrategy-controller carries chBackupClientImage, rabbitmqBackupClientImage and redisBackupClientImage. The first two reach the classifier through the @sha256: branch; the third is pinned by tag alone, so it read as a config change and cost the WHOLE file its overlay. That package therefore kept its committed release ref in every lane that did not rebuild it, and served an image from v1.6.0 (22 July) against a tree carrying four backup strategy drivers added after it. Five of the seven chainsaw failures behind #4257 trace to that, across three unrelated pull requests. Accept a prefix before image in the key branch. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
618ab58482 |
ci(e2e): name the packages that kept release refs instead of current main
A file whose config differs from the artifact beyond its image lines keeps the committed ref. That is the safe answer, and it is not a neutral one: the component then runs its last release image in a lane whose job is to exercise the tree under review, and the only trace is one line among hundreds of overlay lines. Six packages took that path on a recent run, backupstrategy-controller among them, which is why four backup round-trip suites refused Kinds the tree supports and three unrelated pull requests each spent a round establishing the red lane was not theirs (#4257). Summarise the drifted packages in one GitHub annotation. Nothing about which files are overlaid changes; a run with no drift emits nothing. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
80ab355d66 |
test(ci): hold the fixture's config guards from CI
make_repo silences commit.gpgsign, tag.gpgsign and core.hooksPath because a contributor's global configuration reaches a fresh repository. A runner carries no such configuration, so none of the three is exercised there: the suite stays green with any of them deleted, and the failure lands only on the machine of whoever signs their commits or tags. That is how tag.gpgsign came to be missing without anything going red. The new test supplies the hostile configuration itself, so removing a guard now fails in CI rather than on a contributor's first local run. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
5a777e9cfc |
test(ci): isolate the commit-trailer fixture from global tag signing
make_repo silences commit.gpgsign in the fixture repository but not tag.gpgsign, so a contributor who signs tags has that setting reach the fixture. git tag start then becomes an annotated tag, git refuses it for want of a message, and since the helper opens all 27 tests the suite fails before a single one reaches its body, taking make unit-tests with it. CI never saw it: the Commit trailers job runs the checker against explicit SHAs and does not touch this file, and a fresh runner has no global configuration to leak. The failure only ever lands on the local run that contributing.md asks for before pushing. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
637098f222 |
test(kafka): anchor the release selector the hook suites assert on
Both suites matched "strimzi.io/cluster=kafka-test" as a bare substring, and that is a prefix of the sibling release kafka-test2 and of kafka-test-topics. Rendering the hook with either selector left both suites green, so neither pinned the scoping they exist to pin. Anchor on the leading "-l " and on the trailing delimiter; both variants now red in both suites, and the six earlier hook mutations still red exactly where they did. Drop the notMatchRegex on a leading slash. It cannot fail on its own: the anchored image pattern above rejects a leading slash first, and an empty _cluster.images-registry renders no slash to begin with. Its rationale moves into that pattern's comment. Record why activeDeadlineSeconds and the 180s wait have to move together. A deadline that fires mid-wait kills the Job, and with backoffLimit: 0 the hook then returns non-zero, which is the wedged uninstall the deliberate exit 0 on the timeout branch exists to avoid. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
415aa82409 |
test(kafka): drive the hook suite through the runner CI actually uses
The suite used bats' run helper. make bats-unit-tests does not invoke the bats binary: it runs hack/cozytest.sh over every hack/*.bats, and that runner implements @test and little else, so the file died at 'run: command not found' and took make unit-tests with it. Under bats it was 5/5, which is where the 5/5 in the previous commit came from. Save $? by hand instead, the shape app-cleanup-hooks_test.bats already uses; verified under both runners. The fake kubectl no longer defaults DELETE_RC to 0. An unset value made every failure-path assertion vacuous; now it exits 64 and says so. Cover both RESTMapper wordings for an absent CRD rather than just the first. Widen the image pattern to v[0-9]+\.[0-9]+(\.[0-9]+)? like harbor and mariadb: pinning v1\.32 reintroduced the breakage the digest was made pattern-based to avoid, one Renovate bump later. Assert the _cluster.images-registry override and the no-leading-slash default, neither of which was covered: the hook is the one image in this chart whose pull failure surfaces at uninstall. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
3326e0fb9e |
test(kafka): run the pre-delete hook against a fake kubectl
The hook's exit code is the whole contract: a non-zero one returns from Uninstall.Run before any release resource goes, keeping the topic operator alive; a zero one lets Helm remove it. delete_test.yaml matches manifest text, so it stayed green when the timeout branch was put back to exit 1, when the CRD-absent branch was, and when the selector was widened past this release. Add a bats suite in the shape hack/app-cleanup-hooks_test.bats already uses for qdrant and harbor: render the script, run it against a fake kubectl, pin the exit code of each branch. Each of the three mutations above now turns it red. Drop hook-succeeded from the delete policy and reclaim the Job with ttlSecondsAfterFinished instead. The script exits 0 on the degraded path too, where topics still hold the finalizer, so deleting on success removed the only record of that seconds after it was written. Assert both in delete_test.yaml. Pin the hook image by shape rather than by literal digest: Renovate rewrites digests under packages/*/templates/hooks/ and nothing rewrites tests/, so the literal turns the next bump red for no defect. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
Yan Bondarenko
|
210b7fe30c |
fix(kafka): stop the pre-delete hook from wedging the uninstall
Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> |
||
|
Yan Bondarenko
|
9d2fddd4e6 |
test(kafka): assert release topics exist before the deletion check
The post-delete absence assertions could pass vacuously if the two release topics were never created. Asserting both topics first makes the E2E prove that the pre-delete cleanup actually removed them. Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> |
||
|
Yan Bondarenko
|
44ba13b527 |
fix(kafka): delete release topics before the topic operator is removed
Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> |
||
|
Aleksei Sviridkin
|
070dcb1e30 |
fix(e2e): wait for the redis replica link instead of sampling it once (#4252)
## What this PR does The `verify-tenant-ca-projection` step in the redis suite read `INFO replication` from both pods once and failed the run when a replica reported `role:slave` without `master_link_status:up`. That value is still in motion at that point, so one sampling decided the result. Redis reports the link up only once a replica reaches `REPL_STATE_CONNECTED`. The operator's readiness script admits a replica on two other predicates: `master_host` being off `127.0.0.1`, and `master_sync_in_progress` not being `1`. `master_host` is set the moment `REPLICAOF` is issued, ahead of the link coming up, and `master_sync_in_progress` is `1` only during the transfer phase, so every handshake state between the two is ready and unlinked. A dropped link re-enters the same window. `readyReplicas: 2` therefore establishes nothing about the link, and the comment on the step calling the check deterministic was wrong. The step now polls the topology to a bounded deadline and fails only if it does not settle, reporting each pod's observed state and the elapsed time. Nothing models the link declaratively: the RedisFailover CRD defines no status at all. So the poll carries the `TODO(e2e-replace-fixed-timeouts):` marker the e2e conventions require for a wait with no object behind it. What the step does now is wait for convergence where it used to sample once. No claim about flake rate follows from that. A green run says nothing about a failure that shows up in a minority of runs. Shell embedded in a chainsaw step has no unit-test surface in this repository, so this change is exercised by the suite and nothing else. Two pre-existing diagnostics defects in the same file turned up while reading it, tracked in #4246 and #4250. Neither is touched here. The first one matters a little more after this change: the poll runs its read path about a dozen times per step instead of twice. Closes #4238 ### Screenshots Not a UI change. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: I walked the trigger map against the diff rather than the title. The change edits one file under `hack/e2e-chainsaw/`, moves and renames nothing, and alters no make target, so none of the listed triggers fire. The node-prerequisite trigger that reaches `ansible-cozystack` and `talm` is `hack/e2e-prepare-cluster.bats`, which this does not touch. ### Release note ```release-note fix(e2e): the redis TLS suite waits for the replica link to come up instead of reading it once, so a link that is still establishing no longer fails the run ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Tests** - Improved Redis TLS topology validation by allowing replication status time to stabilize before verification. - Extended the test operation timeout to reduce failures caused by slow environment startup or synchronization. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
999a8f5456 |
fix(e2e): keep backup preflight failure evidence across attempts (#4242)
## What this PR does The S3 access preflight that runs before a backup roundtrip prints its port-forward capture only when the port-forward never comes up. On the failure that actually happens, a healthy port-forward and an S3 roundtrip that never completes, that capture is written and then thrown away. The mc log is truncated at the top of every attempt on top of that, so what reaches the job log is the tail of the last attempt with nothing in it to say which stage died. One real run left a single line of evidence, an upload progress line, which reads as an upload that finished and a download that hung. The upload was what failed. The timeout path now prints both logs, the mc log accumulates across attempts, and every attempt records which of PUT, GET, compare or DELETE it reached. That is enough to tell a broken channel to S3 from S3 refusing the operation, and to see how many attempts ran and how each one ended. The port-forward log goes in whole rather than filtered. `kubectl port-forward` writes one `Handling connection for <port>` line per TCP connection, and whether those lines are there at all is the answer to whether mc ever reached the forwarder, so dropping them to cut noise would delete the signal this change exists to produce. Diagnosability only. Nothing changes about what the preflight passes or fails. Closes #4240 ### Screenshots No UI change. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff. Nothing under `hack/` is moved or renamed, no make target changes behaviour, and the node prerequisites the Ansible collection and talm mirror 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: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(e2e): a backup access preflight that times out now prints the port-forward log and the outcome of every attempt, not just the tail of the last one ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Backup access preflight checks now validate the complete upload, download, comparison, and cleanup sequence. - Retry results identify the specific operation that failed and preserve failure details across attempts, improving troubleshooting. - **Tests** - Added coverage for reporting the first failed upload. - Added timeout-path coverage for readiness checks, repeated operation failures, and reporting each affected stage. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
4878250a1d |
fix(e2e): wait for the redis replica link instead of sampling it once
The redis-tls topology check read INFO replication from both pods once and failed the suite when a replica reported role:slave without master_link_status:up. That value is legitimately in motion there. Redis reports the link up only in REPL_STATE_CONNECTED, while the operator's readiness script admits a replica on two different predicates -- master_host off 127.0.0.1, and sync not in the transfer phase -- so every handshake state between the two is ready and unlinked, and a dropped link re-enters that window. Ready replicas carry no evidence about the link at all, which is more than the step's own comment allowed for and makes its claim of determinism wrong. Poll the topology to a 60s deadline instead, and on expiry report each pod's observed state along with how long it waited. Nothing models the link declaratively -- the RedisFailover CRD defines no status, so there is no condition to assert on and the poll carries the marker the e2e conventions require. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Timofei Larkin
|
ae7a7115f5 |
fix(harbor-rd): carry the release prefix in the resource selectors (#4170)
<!-- Thank you for making a contribution! Here are some tips for you:
- Use Conventional Commits for the PR title: `type(scope): description`
- Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore
- Scopes are not an exhaustive list — pick the most specific scope for
the change and extend the list when a genuinely new area appears.
Examples:
- System components: dashboard, platform, operator, cilium, kube-ovn,
linstor, fluxcd, cluster-api
- Managed apps: postgres, mariadb, redis, kafka, clickhouse,
virtual-machine, kubernetes
- Development and maintenance: api, hack, tests, ci, docs, maintenance
- Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or
add a `BREAKING CHANGE:` footer
- If it's a work in progress, consider creating this PR as a draft.
- Don't hesistate to ask for opinion and review in the community chats,
even if it's still a draft.
- Add the label `kind/backport` if it's a bugfix that needs to be
backported to a previous version.
-->
## What this PR does
`harbor` declares `release.prefix: "harbor-"` and then selects its
Secret, Service and Ingress without it, so all three `resourceNames`
selectors match nothing. This corrects the three names and adds a guard
so the next omission fails a test instead of passing silently.
### Why they never matched
Release names are `<prefix><app name>`, but the lineage webhook
evaluates selectors against `{kind, name, namespace}`, where `name` is
the custom resource's name and carries no prefix. Every definition
therefore spells the prefix out itself, and the others do — `postgres-{{
.name }}-credentials`, `bucket-{{ .name }}-credentials`, and so on.
`harbor` is the only one that does not.
A selector that matches nothing raises no error: the webhook labels the
object `internal.cozystack.io/tenantresource=false` and continues, which
is indistinguishable from a selector that legitimately has nothing to
match. That is why this survived.
### What actually changes at runtime
Close to nothing, and that is the point — the manifest was wrong, not
the behaviour:
- the Secret already reaches tenants, because #2541 worked around this
exact mismatch by stamping `apps.cozystack.io/user-secret: "true"` on it
and matching by label. That workaround is deliberately left in place;
- the Service and Ingress labels flip from `false` to `true`, which
nothing consumes: `spec.services` and `spec.ingresses` are read only by
the webhook that computes the label, and the label's only consumer is
the TenantSecret registry, which handles Secrets. The dashboard reaches
both through the per-release Role, which names them correctly.
So this closes a latent defect before it bites, rather than fixing an
outage.
### The guard
`hack/check-rd-release-prefix.bats` asserts that every `resourceNames`
entry in `packages/system/*-rd/cozyrds/*.yaml` contains `<prefix>{{
.name }}`. `hack/*.bats` is auto-discovered by `BATS_UNIT_FILES`, so it
runs under `make unit-tests` with no registration.
The assertion is "contains", not "starts with", because operator-created
objects legitimately carry leading words of their own: mongodb excludes
`internal-mongodb-{{ .name }}-users`, which is correct and stays green.
Definitions with an empty prefix are skipped, since `hack/update-crd.sh`
generates the definitions for `packages/extra` charts with `PREFIX=""`.
A selector that is correct and still fails the check can opt out on its
own with a trailing `# cozy-lint:ignore rd-release-prefix -- <reason>`
comment. A directive without a reason does not count.
Verified by mutation: the guard fails on the three harbor selectors
before the fix and passes after. Across the rest of the tree `harbor`
was the only violation.
### What this PR does not do
It does not settle whether the prefix belongs in the template context
instead. That would remove the duplication, but it requires migrating
the definitions that spell the prefix out today, or they would double
it. The guard is the cheaper half and closes the class either way.
### Screenshots
<!-- REQUIRED for UI changes: attach screenshots or screen recordings
demonstrating
the visual impact of your changes. PRs with UI changes without
screenshots will not be merged. -->
### Downstream repositories
<!--
Cozystack is upstream for repositories that are NOT kept in sync with it
automatically, and nothing in CI compares the two sides, so a change
here can break them silently.
Which change forces what, and which file to touch, is the trigger map in
docs/agents/contributing.md under "Downstream Repositories". Walk it
before you tick anything.
IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM
TO FILL IN:
1. Walk the trigger map against the actual diff of this PR, file by
file. Do not decide from the PR title or from your memory of what you
changed.
2. For every repository you tick, open the follow-up PR there and link
it on the same line. A ticked box with no link claims work that does not
exist, which is worse than an empty box. Search that repository for an
open PR or issue covering it first, and link that instead of filing a
duplicate.
3. When the follow-up is out of scope, or needs a decision that is not
yours to make, open an issue in that repository instead, link that, and
say so in the PR body. Do not push a speculative PR into another
repository just to fill a line here.
4. Do not tick "No downstream repository is affected" to make the
checklist go away. If you are unsure, leave every box empty and say so
in the PR body, so a human decides.
-->
- [x] No downstream repository is affected by this change
- [ ] [cozystack/website](https://github.com/cozystack/website) -
follow-up:
- [ ]
[cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack)
- follow-up:
- [ ]
[cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack)
- follow-up:
- [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up:
- [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up:
- [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) -
follow-up:
- [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) -
follow-up:
- [ ]
[cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server)
- follow-up:
- [ ]
[cozystack/external-apps-example](https://github.com/cozystack/external-apps-example)
- follow-up:
- [ ] [cozystack/examples](https://github.com/cozystack/examples) -
follow-up:
- [ ] [cozystack/community](https://github.com/cozystack/community) -
follow-up:
Trigger map walked against the diff, which touches
`packages/system/harbor-rd/cozyrds/harbor.yaml` and adds
`hack/check-rd-release-prefix.bats`. The two nearest entries were
checked and neither fires:
- **terraform-provider-cozystack** lists "change `release.prefix`, or
rename an output Secret or Service". Neither happens here:
`release.prefix` is untouched and no object is renamed. The provider
concatenates the prefix itself to derive names, so it was already
computing the correct ones — it is this definition's selectors that were
wrong.
- **ccp** lists "rename `packages/system/<name>-rd/cozyrds/<name>.yaml`"
and "move or rename anything under `hack/`". The file is edited, not
renamed, and the new bats file is an addition; no make target changes
behaviour.
**website** would fire on a change to `ApplicationDefinition` semantics.
Semantics are unchanged — one definition's data is corrected — and
`content/en/docs/next/cozystack-api/application-definitions.md`
documents neither `resourceNames` nor the prefix rule, so there is
nothing there to bring in step.
### Release note
<!-- Write a release note:
- Explain what has changed internally and for users.
- Start with the same `type(scope):` prefix as in the PR title
- Follow the guidelines at
https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md.
-->
```release-note
fix(harbor-rd): the ApplicationDefinition selectors for Harbor's Secret, Service and Ingress now carry the release prefix, so they match the objects the chart creates. No user-visible change — the Secret already reached tenants through a label-based workaround, and nothing consumes the Service and Ingress labels.
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
- **Bug Fixes**
- Updated Harbor resource naming patterns to consistently include the
`harbor-` prefix for credentials, services, and ingress resources.
- Improved alignment between Harbor resource names and their configured
release prefixes.
- **Tests**
- Added validation to detect missing release prefixes in secret,
service, and ingress resource selectors across system application
definitions.
- Validation reports all affected selectors and supports documented
exceptions where necessary.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
Aleksei Sviridkin
|
65c183e311 |
fix(e2e): keep backup preflight failure evidence across attempts
The backup access preflight printed its port-forward capture only when the port-forward never came up, and truncated its mc log at the top of every attempt. On the path that fails in practice, a healthy port-forward and an S3 round trip that never completes, the output was the tail of the last attempt with nothing to say which stage it died at, which reads as a successful upload followed by a hung download. Print the port-forward capture alongside the mc log on the timeout path, append the mc log across attempts, and record which of PUT, GET, compare or DELETE each attempt reached. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
IvanHunters
|
79602e9ca0 |
fix(opensearch-operator): keep a single-replica cluster electable after bootstrap (#4231)
## What this PR does Builds the OpenSearch operator in-tree, the way kamaji and redis-operator already are, and carries one source patch. The operator deletes its bootstrap pod the moment `status.initialized` flips, without looking at the voting configuration. With `replicas: 1` there are exactly two cluster-manager-eligible nodes, OpenSearch keeps the voting configuration at an odd size of one, and the voter it keeps is the bootstrap node that formed the cluster. Deleting that pod takes away the only vote there was, and the survivor can never hold an election again, because changing the voting configuration itself needs a quorum. Full write-up in #4230. The patch excludes the bootstrap node from the voting configuration before the pod goes, and confirms the exclusion committed before deleting anything: 1. Pod already gone: nothing to do. 2. Build a cluster client. Cannot: requeue, keep the pod. 3. `POST /_cluster/voting_config_exclusions?node_names=<release>-bootstrap-0`. Fails: requeue, keep the pod. 4. Read the voting configuration back. Still a voter: requeue, keep the pod. 5. Delete the pod. 6. `DELETE /_cluster/voting_config_exclusions`. Fails: log and carry on, the pod is already gone and the list is only capped, not load-bearing. Step 4 is the one place this differs from the upstream attempt, which relies on the POST being blocking. I did not verify that property, and polling the committed configuration is correct whether or not it holds. Every failure to reach the cluster keeps the bootstrap pod and requeues. That is a deliberate trade: a bootstrap pod outliving its purpose costs one pod, a cluster that cannot elect a cluster-manager needs a human. ### How the values were arrived at Not from the documentation. Reproduced outside Kubernetes first, two containers with the env the operator sets, `cluster.initial_master_nodes` pointing at the first: ```text last_committed_config: ["D6Fijf-SRN-FwIXVGNehPw"] # the bootstrapper, alone POST .../voting_config_exclusions?node_names=osA -> 200 last_committed_config: ["oy8DPOV-SKKildaIZpgoPg"] # moved to the survivor ``` Without the exclusion, removing the first container leaves the second printing `cluster-manager not discovered or elected yet, an election requires a node with id [...]` forever, which is the same sentence the e2e run produced. ### Why carry it here rather than wait Upstream knows: [opensearch-k8s-operator#1448](https://github.com/opensearch-project/opensearch-k8s-operator/issues/1448) is open with `release-blocker`, and its third point is this exact case. Three PRs address it and none looks close: [#1483](https://github.com/opensearch-project/opensearch-k8s-operator/pull/1483) has changes-requested, a merge conflict and a red lint; [#1150](https://github.com/opensearch-project/opensearch-k8s-operator/pull/1150) and [#1430](https://github.com/opensearch-project/opensearch-k8s-operator/pull/1430) have not moved since July. When a release carries the fix, this patch and this image build come back out. The alternative considered and rejected was refusing `replicas: 1` in `values.schema.json`. It is a breaking change for existing releases and removes a configuration rather than fixing it. ### Before this can merge `values.yaml` deliberately carries no `manager.image` yet. The image does not exist, and committing a first-party reference without a digest is exactly what `docs/agents/image-refs.md` forbids. Once the first build here publishes it, I will add `repository` plus `tag@sha256:...` in a second commit and take the PR out of draft. ### Note on CI cost Adding the build line to the root `Makefile` puts it in `full_rebuild_pattern`, so every push to this branch rebuilds all 28 units rather than one. Verified with `hack/build-matrix.sh`. Unavoidable: the line is what puts the image in the matrix at all. ### What was checked locally - The image builds and the patch applies cleanly; `/manager --help` runs from the built image. - `go build ./...`, `go vet` and the operator's own `pkg/reconcilers` tests pass with the patch applied. - `gofmt` clean, `helm unittest` for the package green, `pre-commit` green on the changed files. - `hack/build-matrix.sh` selects the package for a package-only change. Not checked locally, and the reason this needs the e2e lane: that the fix actually keeps a real cluster electable. The `opensearch` chainsaw suite is the test for it. ### Screenshots Not a UI change. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(opensearch-operator): exclude the bootstrap node from the voting configuration before removing it, so a single-replica OpenSearch cluster stays able to elect a cluster-manager ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added support for building and publishing the OpenSearch operator image as part of the standard build workflow. - Operator image versions now stay aligned automatically with the packaged source version. - Added a packaged operator image configuration for consistent deployment. - Improved cluster initialization by safely excluding the bootstrap node from voting before removal and clearing the exclusion afterward. - Added retry handling when cluster state is unavailable or voting changes are not yet committed. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Andrei Kvapil
|
91112027a5 |
test(hack): let one selector opt out of the release-prefix guard
A repo-wide name check will eventually meet a selector that is correct and still fails it, such as an object named on the operator's terms. The only escape so far was an empty prefix, which exempts a whole definition, and a guard with no narrower way out is one people learn to route around. A trailing "cozy-lint:ignore rd-release-prefix -- <reason>" comment now exempts that one entry, and a directive without a reason does not count, so a bare suppression is never the easy path. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com> |
||
|
IvanHunters
|
b222cb6580 |
fix(opensearch-operator): keep a single-replica cluster electable after bootstrap
The operator deletes the bootstrap pod as soon as initialization completes and never looks at the voting configuration first. With one cluster_manager replica there are exactly two eligible nodes, OpenSearch keeps the configuration at an odd size of one, and the voter it keeps is the bootstrap node that formed the cluster. Deleting that pod takes away the only vote there was, and the survivor can never hold an election again, because changing the configuration itself needs a quorum. The cluster is unusable about a minute after it installs, and a node with no cluster-manager still answers the readiness probe, so nothing says so until something restarts the pod. Build the operator in-tree, the way kamaji and redis-operator already are, and carry a patch that excludes the bootstrap node from the voting configuration and waits for that to commit before deleting the pod. Reaching the cluster is a precondition rather than a nicety: every failure keeps the pod and requeues, because a bootstrap pod that outlives its purpose costs a pod, while a cluster that cannot elect does not recover without a human. Deleting a pod is a request rather than a wait, so clearing the exclusion afterwards can find the node still in the cluster and fail. That path requeues and is retried once the pod is really gone, which keeps a stale entry out of the capped exclusion list; clearing stays at the API default wait_for_removal=true so it cannot re-admit a node some other operation is still draining. Upstream carries the same defect, tracked as opensearch-project/opensearch-k8s-operator#1448. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
Aleksei Sviridkin
|
f9ea0da3de |
feat(backups): Kafka topic-metadata backup via a dedicated strategy driver (#4000)
## What this PR does Adds a dedicated `strategy.backups.cozystack.io/Kafka` Go driver that backs up the **topic metadata** of a Cozystack-managed `Kafka` application and wires it into the platform `cozy-default` flow — the counterpart to the app-specific drivers (CNPG, MariaDB, Altinity, MongoDB). It is a typed variant of the generic Job strategy: the controller renders a one-shot batch Job that talks to the Kafka Admin API, and adds on top an applicationRef Kind gate, a `Ready` precondition on the Strimzi `Kafka` cluster, and a per-run object key via `.BackupName` so distinct runs never overwrite each other. What it captures: every non-internal topic's partition count, replication factor and non-default configs (via `kafka-topics --describe`), stored as one small object per run in the system bucket; restore recreates them with `kafka-topics --create` + `kafka-configs --alter`. Deliberately **out of scope**: message payloads (a volume-snapshot strategy's job), consumer-group offsets (a log position, only meaningful with a byte-identical data restore), ACLs, quotas, and `KafkaUser`s. No PITR. Restored topics are created directly through the Admin API, so on the target they are unmanaged (no `KafkaTopic` CR) — the accepted tradeoff of an Admin-API-only driver. Both restore flows are implemented: **in-place** recreates dropped topics into the source, and **to-copy** applies the source's topics onto a freshly-bootstrapped empty `Kafka` (the "bootstrap a cluster, then apply the metadata state" flow). Contents: - Driver: `api/backups/strategy/v1alpha1/kafka_types.go`, `internal/backupcontroller/kafkastrategy_controller.go` (+ unit tests for its pure helpers), `internal/backupcontroller/kafkatypes/` (Strimzi `Kafka` readiness gate), dispatch cases in both controllers + `supportedBackupStrategyKinds()`, scheme registration, generated CRD + deepcopy. - Chart: `strategy-kafka-default.yaml` (gated on the bucket name), a `cozy-default` binding, read RBAC on `kafka.strimzi.io/kafkas`, and the Strimzi Kafka client image value. - Docs: a Kafka section in `docs/operations/backup-classes.md`. - Example + e2e: `examples/backups/kafka-metadata/` and `hack/e2e-chainsaw/kafka-metadata/` (`kafka-3-metadata-roundtrip`), an honest round-trip that proves a per-run `retention.ms` sentinel survives both restores. The original `packages/apps/kafka` chart is left untouched (it has no legacy backup mechanism). Validated: `go build ./...`, `go vet`, `go test ./internal/backupcontroller/...` (pure helpers plus a reconcile Kind-gate test and the RestoreJob cleanup regression test), `helm unittest` on the package (which now renders `strategy-kafka-default.yaml` and asserts its kind/name/`artifactURITemplate`), the `select-install`/`select-e2e` bats suites, `helm template` of the strategy CR, `bash -n` of the rendered script, and `make generate` (clean). The end-to-end round-trip (`kafka-3-metadata-roundtrip`) is enabled and mapped like the sibling backup suites, so CI builds this branch's controller and runs it live against a fresh sandbox; it has not been merged-and-run before, so that first live run must go green before merge. ### Screenshots N/A — no UI changes. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff. This PR adds a new `strategy.backups.cozystack.io/Kafka` CRD, a Go driver, chart wiring, docs and examples. It does not add or rename a `packages/apps`/`packages/extra` package, does not change any `ApplicationDefinition`, `values.schema.json`, version enum, `release.prefix`, output Secret/Service, the `Package`/`Plan`/`RestoreJob` CRDs the provider types by hand, any `hack/*.mk` anchor or make-target behaviour, node prerequisites, labels/annotations, or telemetry metrics. A brand-new `strategy.backups.cozystack.io` CRD travels to the Terraform provider as opaque JSON (it types only `Plan`/`RestoreJob`/`Package`), so it keeps working untouched. None of the downstream triggers match. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note feat(backups): add a Kafka topic-metadata backup strategy bound by cozy-default — backs up topic definitions and configs (partitions, replication factor, non-default configs) via the Kafka Admin API, with in-place and to-copy restore; message data, consumer offsets, ACLs, quotas and KafkaUsers are out of scope ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added Kafka topic-metadata backup and restore support, excluding message data. - Supports ad-hoc and scheduled backups, in-place restoration, and restoration to a separate Kafka cluster. - Recreates missing topics and restores compatible configurations while validating partition and replication-factor constraints. - Waits for Kafka readiness and cleans up backup artifacts after completion. - Backup and restore jobs now enforce configurable deadlines to prevent stalled operations. - **Documentation** - Added guidance covering Kafka metadata backups, restoration limitations, and S3 configuration. - **Tests** - Added end-to-end coverage for backup, restoration, validation, and cleanup scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Andrey Kolkov
|
01967b56d3 |
docs(backups): correct the Kafka .Mode doc and the embedded-script templating note
The KafkaSpec.Template doc listed .Mode as "backup" or "restore", but the driver also renders it as "cleanup" when reaping a removed Backup's object; the comment is copied verbatim into the CRD, so kubectl explain and the dashboard showed two values while a custom strategy's restore branch would run on the cleanup pass. List all three, matching the Redis sibling, and regenerate the CRD. The strategy template also claimed the embedded script "reads only env vars, never Helm/controller values, so no tpl is applied". The controller renders every string leaf of the PodTemplateSpec through internal/template, args[0] included, so the script is templated on every run. State that instead, without literal template braces that Helm would itself expand. Signed-off-by: Andrey Kolkov <androndo@gmail.com> Assisted-by: LLM |
||
|
Andrey Kolkov
|
deb4916f94 |
test(backups): pin the restore-side Kafka --describe literal-topic guard
The restore path wraps the topic in \Q...\E on both the --describe and the --alter calls so a name with a regex-special character cannot match a colliding sibling. Only the --alter guard was pinned: the colliding pair in the fixture gave both topics the same partition count and replication factor, so stripping \Q on the --describe read the sibling's identical numbers and nothing downstream diverged. Give the sibling a distinct, larger partition count. A stripped --describe \Q now matches it first (glob order places desc.pay-events before desc.pay.events), reads 9 > the backup's 7, and aborts with "cannot decrease partitions" so no alter is recorded and the assertion reddens. Also point the suite's scratch file at a per-test path via METADATA_FILE so runs cannot read back a prior test's records. Signed-off-by: Andrey Kolkov <androndo@gmail.com> Assisted-by: LLM |
||
|
Andrey Kolkov
|
02bf3e9970 |
fix(backups): probe live Kafka topics without a pipe and isolate the scratch file
The restore existence check piped the live topic list into `grep -qxF`. grep exits at the first match, so once the matched topic is far enough from the end of a list larger than the 64 KiB pipe buffer, the producing printf takes SIGPIPE, pipefail turns the pipeline into a failure, the `if` reads false, and a topic that exists is sent to --create, which fails with TopicExistsException. Read the list from a here-string instead: no pipe, no SIGPIPE, decided by content rather than list size. Also make the local metadata scratch file overridable (METADATA_FILE), like BIN already is, so the bats suite can point each test at its own path instead of sharing /tmp/kafka-metadata.txt across runs. The Job never sets it and keeps the default. Signed-off-by: Andrey Kolkov <androndo@gmail.com> Assisted-by: LLM |
||
|
Andrey Kolkov
|
ca2a6d6429 |
fix(backups): warn when a Kafka backupTimeout is rejected
kafkaRunDeadline silently substituted the 30m default for a backupTimeout that time.ParseDuration rejects or that falls below the one-minute floor, with no Event, condition or log line. An operator who set an unparseable or sub-minute value, then raised it, saw every run keep dying at DeadlineExceeded with nothing naming the discarded parameter. Return the applied deadline together with a message when the configured value is rejected, and record it as a BackupTimeoutInvalid Warning Event on the BackupJob and RestoreJob (the reconcilers already hold a Recorder). An honoured or unset value stays silent. TestKafkaRunDeadline now asserts the warning fires for the rejected cases and not otherwise. Signed-off-by: Andrey Kolkov <androndo@gmail.com> Assisted-by: LLM |
||
|
Andrei Kvapil
|
13549b5cfa |
test(hack): keep only what the code cannot say in the prefix guard header
The header retold both commit messages at several times their length. What stays is what nothing else records: why the check is "contains" rather than a prefix match, so tightening it does not break the nine selectors with their own leading words, and why an empty prefix is skipped. The latter now names the actual source of those definitions, since they live under packages/system and are generated from charts in packages/extra. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com> |
||
|
Andrei Kvapil
|
50f4834edc |
test(hack): run the release-prefix guard under cozytest.sh with yq
make unit-tests executes hack/*.bats through hack/cozytest.sh, which has no `run` helper, so the guard aborted before inspecting a single selector. It also depended on python3 and PyYAML, neither of which the build dependencies or the unit-test job provide. Walk the definitions with yq instead, as the neighbouring rd checks do, and fail when zero selectors were inspected so a broken glob or query cannot pass. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com> |
||
|
Aleksei Sviridkin
|
5b91c6b7c6 |
fix(registry): run validating admission on TenantSecret writes (#4226)
## What this PR does The apiserver hands validating admission to a storage as the `ValidateObjectFunc` it passes into `Create`, `Update` and `Delete`. Mutating admission runs in the handler, but validating webhooks and `ValidatingAdmissionPolicy` execute only when the storage calls that callback, as `rest.AdmissionToValidateObjectFunc` in `k8s.io/apiserver` builds it for exactly that purpose. TenantSecret bound all three callbacks to `_`, so every write answered as if no policy existed. The practical effect is that the cluster-wide `cozystack-no-delete-guardrail` policy, and any webhook an operator writes for `tenantsecrets`, are inert on this resource. `application` and `securitygroup` call all three callbacks; TenantSecret was the only write-capable storage in the tree that did not. The callbacks are now called on all four write paths, including the create-on-update branch, which is a create and so carries `createValidation` rather than `updateValidation`. What goes to admission is the aggregated TenantSecret the caller submitted, or the stored one it replaces, not the backing Secret, so a policy sees the resource it was written against. This is a behaviour change for anyone who has a policy matching `tenantsecrets` today: writes that silently passed will start being evaluated, and may now be denied. That is the point of the fix, but it is worth saying out loud before it ships. ### Tests Each write path is covered by a denying admission callback, asserting both that the callback runs and that the backing Secret is left alone when it denies. A nil callback keeps every path working, which is what the apiserver passes when no plugin handles the operation. Removing any one of the four calls, one at a time, turns the suite red. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff. It touches one aggregated storage's write paths: no package added, renamed or removed, no `values.schema.json`, no version enum, no changed default, no `kind` or `plural`, no `release.prefix`, no renamed output Secret or Service, no hand-typed CRD, and nothing under `hack/`. - [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(registry): run validating admission on TenantSecret writes, so ValidatingAdmissionPolicies and webhooks matching tenantsecrets are no longer bypassed ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - TenantSecret create, update, force-create, and delete operations now apply validation checks before completing changes. - Denied create or update requests are stopped before data is saved. - Delete validation runs after the current secret is retrieved and before removal. - TenantSecret write operations remain available when no validation callback is provided. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
682f24f0aa |
fix(opensearch): give the securityconfig update job its own resources (#4228)
## What this PR does The chart rendered `spec.security.config` without `updateJob`, and the operator passes that field straight through to the Job container it builds for `securityadmin`. With nothing set, a tenant namespace applies its own `tenant-range-limits` LimitRange default of 128Mi instead. `securityadmin.sh` runs `java` with no `-Xmx` of its own, so the JVM sizes its heap from the container limit and does not fit in 128Mi. The operator builds the Job with `backoffLimit: 0`, so the single OOM kill is final. Nothing writes the `.opendistro_security` index after that, every node sits behind `Not yet initialized (you may need to run securityadmin)`, and the release never reaches ready. This is what has been failing the `opensearch` chainsaw suite in every run that executes it since 2026-09-11. The suite waits 12 minutes for `status.readyReplicas` on `opensearch-test-nodes`, which is the whole 748s runtime of the red test. Full write-up in #4227. ### Where the numbers come from Measured against `opensearchproject/opensearch:2.11.1` on `linux/amd64`, running the operator's own command against a fresh single-node cluster: | Memory limit | Result | Peak | | --- | --- | --- | | 128Mi | OOMKilled mid-apply | 128Mi | | 192Mi | OOMKilled mid-apply | 192Mi | | 256Mi | success | 208Mi | | 512Mi | success | 253Mi | Having no `-Xmx`, the tool grows into whatever it is given, so the peak tracks the limit and the headroom is what matters rather than the peak itself. The values are constants and not a `resourcesPreset` lookup: the tool configures the cluster once and exits, so what it needs does not scale with the nodes it configures, and the smallest preset would hand it back the 128Mi it dies at. ### Upgrading an already-stuck release This does not unstick one on its own. The operator skips the Job entirely while the existing one carries a matching securityconfig checksum, and bumping the chart does not change that checksum, so the failed Job stays where it is. Delete it once and the operator rebuilds it with the resources above: ```bash kubectl -n <namespace> delete job <release>-securityconfig-update ``` A fresh install needs nothing. ### Not fixed here `backoffLimit: 0` is the operator's, and it is what turns one transient kill into a permanent dead cluster. Worth raising upstream separately. ### Screenshots Not a UI change. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(opensearch): stop the securityconfig update job being OOMKilled at the tenant namespace memory default, which left new clusters stuck with security uninitialized and every node unready ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * OpenSearch security configuration updates now use consistent CPU and memory allocations, regardless of the selected node sizing preset. * Improved reliability for update jobs by ensuring they receive defined minimum and maximum resource levels. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
bed172bea2 |
fix(registry): preserve dry-run in tap storage (#4225)
## What this PR does Tap `Create` and `Delete` bound the options parameter to `_` and built their own `metav1.CreateOptions` and `DeleteOptions` for the dynamic client, so the caller's dry-run directive never reached the backing write. `kubectl create --dry-run=server` on a tap created the Flux `OCIRepository` for real, and `kubectl delete tap <name> --dry-run=server` removed the `PackageSource` and its source for real. That is the opposite of what a server-side dry-run promises, and it is reachable from the dashboard and from any client that previews a change before applying it. The directive now reaches all five backing writes: the source create, the update taken on a repeat connect, the `PackageSource` delete, the delete of an unreferenced `OCIRepository`, and the orphan-source fallback, which had no access to the options at all and needed them threaded in. Field manager stays as the storage sets it. Tap synthesizes the backing objects rather than storing what the caller sent, so who owns their fields is a separate question from whether the write happens, and only the second one is a correctness bug. `taps` was the last write-capable storage in the aggregated apiserver still discarding its write options. #3937 covers application, tenant-secret and security-group, and excludes this one on the grounds that it uses a different client; the client is not what stood in the way, since `dynamic.Interface` takes the options by value, dry-run included. ### Tests The fake dynamic client's tracker ignores `DryRun` and persists regardless, so asserting that an object is absent after a dry-run would pass with or without the fix. The tests assert the options recorded on the backing action instead. Reverting any one of the five forwards, one at a time, turns the suite red. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff. It touches only the write plumbing of the `taps` aggregated storage: no package added, renamed or removed, no `values.schema.json`, no version enum, no changed default, no `kind` or `plural`, no `release.prefix`, no renamed output Secret or Service, no hand-typed CRD, and nothing under `hack/`. - [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(registry): honor server-side dry-run when connecting or disconnecting a tap, which previously performed the write for real ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Server-side dry-run requests for TAP connect and disconnect operations now prevent changes from being written to backing resources. * Dry-run settings are consistently applied when creating, updating, or deleting associated sources, including orphaned sources. * **Tests** * Added coverage confirming dry-run behavior across create and delete workflows, including requests without write options. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
IvanHunters
|
4198914dae |
fix(opensearch): give the securityconfig update job its own resources
The operator hands spec.security.config.updateJob.resources straight to the Job container and the chart left it unset, so a tenant namespace applied its LimitRange default of 128Mi instead. securityadmin.sh runs java with no -Xmx and is OOMKilled at that size, and the operator builds the Job with backoffLimit 0, so that one kill is final: the security index is never written and every node stays unready. Constants rather than a preset value, because the tool configures the cluster once and exits. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |